After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 672525 - history-window: favicon not loaded for newly visited hosts
history-window: favicon not loaded for newly visited hosts
Status: RESOLVED FIXED
Product: epiphany
Classification: Core
Component: Interface
git master
Other Linux
: Normal normal
: ---
Assigned To: Epiphany Maintainers
Epiphany Maintainers
Depends on:
Blocks:
 
 
Reported: 2012-03-21 09:00 UTC by Claudio Saavedra
Modified: 2012-03-27 09:47 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
ephy-hosts-store: listen to favicon changes in the wk database (3.42 KB, patch)
2012-03-21 14:31 UTC, Claudio Saavedra
accepted-commit_after_freeze Details | Review
ephy-hosts-store: listen to favicon changes in the wk database (3.60 KB, patch)
2012-03-26 14:34 UTC, Claudio Saavedra
none Details | Review
ephy-hosts-store: listen to favicon changes in the wk database (3.80 KB, patch)
2012-03-26 14:49 UTC, Claudio Saavedra
committed Details | Review

Description Claudio Saavedra 2012-03-21 09:00:47 UTC
If you visit a page in a new host (www.gnome.org for instance) for the first time while the history window is open, the host is added to the "Sites" view but the favicon is not loaded. This happens because the page is added to the history before the favicon is downloaded by webkit, so by the time this is requested to the icon database this can't find it yet.

A potential solution would be to listen the "icon-loaded" favicon database signal, but this might have some performance implications.
Comment 1 Claudio Saavedra 2012-03-21 14:31:50 UTC
Created attachment 210240 [details] [review]
ephy-hosts-store: listen to favicon changes in the wk database
Comment 2 Xan Lopez 2012-03-23 15:04:13 UTC
Review of attachment 210240 [details] [review]:

::: lib/widgets/ephy-hosts-store.c
@@ +62,3 @@
+    } else if (cmp > 0)
+      break;
+  } while (gtk_tree_model_iter_next (model, &iter));

Nitpick, I think this would be cleaner as a while (valid && !found) { loop, with valid coming from get_iter_first and iter_next and found being the string comparison (and of course you set the favicon if found is TRUE...).

Looks good otherwise.
Comment 3 Claudio Saavedra 2012-03-26 14:34:20 UTC
Created attachment 210623 [details] [review]
ephy-hosts-store: listen to favicon changes in the wk database

The comparison is also used to determine whether there's any point in continuing
with the linear search. I applied your suggestions with a few changes to make this more 
clear.
Comment 4 Claudio Saavedra 2012-03-26 14:49:55 UTC
Created attachment 210627 [details] [review]
ephy-hosts-store: listen to favicon changes in the wk database

This adds a small optimization to avoid traversing the list for each icon coming
from the database and to do it only for hosts.
Comment 5 Xan Lopez 2012-03-27 09:37:30 UTC
Review of attachment 210627 [details] [review]:

Looks good.