GNOME Bugzilla – Bug 758470
Overview should wait for snapshot service
Last modified: 2016-03-12 02:21:15 UTC
Whenever I open Epiphany, I get an overview filled with placeholder images instead of snapshots of web pages. Probably best to wait for the snapshot service to be ready before showing the browser UI. Or we could replace the images asynchronously, but that might look jarring for the placeholder images to be visibly replaced by snapshots. Either way would be better than leaving them all as placeholders.
Created attachment 319320 [details] [review] web-overview: Do not drop thumbnail update requests When the overview is opened in a new tab, it normally works fine, but when the overview is opened in the first tab when starting Epiphany, there are no thumbnails for any of the sites: just empty placeholder images, at least on my machine. That's not good. The snapshot service does not have the images ready immediately when requested by EphyAboutHandler, so EphyAboutHandler is not able to insert them into the overview HTML when it is created. Instead, it inserts a placeholder image. Then, when the snapshot service has finished getting the thumbnail, it makes a D-Bus call to the web extension to convey the file URI for the thumbnail (for each URL in the overview). EphyWebOverview replaces the placeholder image with the thumbnail when it receives the call. The problem is the call can occur before WebKit has finished loading the overview HTML, in which case the thumbnail changes get dropped and we wind up with a bunch of placeholder images. Instead, remember the thumbnail change requests as they come in, then apply them all after the document has loaded.
Review of attachment 319320 [details] [review]: Good catch! ::: embed/web-extension/ephy-web-overview.c @@ +254,3 @@ + g_hash_table_foreach_remove (overview->delayed_thumbnail_changes, + apply_delayed_thumbnail_change, + overview); The hash table is not going to be used after this, no? Can we destroy the table here as well? @@ +270,2 @@ update_thumbnail_element_style (item->thumbnail, path); + return; I don't remember why I iterate the whole list, but if you are sure it's not needed we should fix other methods that do the same (in a follow up). @@ +280,3 @@ + * snapshot on demand. + */ + g_hash_table_insert (overview->delayed_thumbnail_changes, g_strdup (url), g_strdup (path)); And maybe we can create the hash table here on demand, since this doesn't always happen, no?
(In reply to Carlos Garcia Campos from comment #2) > Review of attachment 319320 [details] [review] [review]: > > Good catch! > > ::: embed/web-extension/ephy-web-overview.c > @@ +254,3 @@ > + g_hash_table_foreach_remove (overview->delayed_thumbnail_changes, > + apply_delayed_thumbnail_change, > + overview); > > The hash table is not going to be used after this, no? Can we destroy the > table here as well? Yeah we can; that will only save a couple bytes of memory, but why not.... > @@ +270,2 @@ > update_thumbnail_element_style (item->thumbnail, path); > + return; > > I don't remember why I iterate the whole list, but if you are sure it's not > needed we should fix other methods that do the same (in a follow up). Definitely not needed; you can see nothing else happens there. Will push a follow-up. > @@ +280,3 @@ > + * snapshot on demand. > + */ > + g_hash_table_insert (overview->delayed_thumbnail_changes, g_strdup (url), > g_strdup (path)); > > And maybe we can create the hash table here on demand, since this doesn't > always happen, no? Right, OK. There's also no valid reason to use a hash table, except it has a nicer API for this than GList....
(In reply to Michael Catanzaro from comment #3) > Definitely not needed; you can see nothing else happens there. Will push a > follow-up. Well it could be needed if the history service were to report the same URL twice for your most-visited sites, but that would be another (hypothetical) bug.
Created attachment 319380 [details] [review] web-overview: Do not drop thumbnail update requests When the overview is opened in a new tab, it normally works fine, but when the overview is opened in the first tab when starting Epiphany, there are no thumbnails for any of the sites: just empty placeholder images, at least on my machine. That's not good. The snapshot service does not have the images ready immediately when requested by EphyAboutHandler, so EphyAboutHandler is not able to insert them into the overview HTML when it is created. Instead, it inserts a placeholder image. Then, when the snapshot service has finished getting the thumbnail, it makes a D-Bus call to the web extension to convey the file URI for the thumbnail (for each URL in the overview). EphyWebOverview replaces the placeholder image with the thumbnail when it receives the call. The problem is the call can occur before WebKit has finished loading the overview HTML, in which case the thumbnail changes get dropped and we wind up with a bunch of placeholder images. Instead, remember the thumbnail change requests as they come in, then apply them all after the document has loaded.
Created attachment 319381 [details] [review] web-overview: Do not drop thumbnail update requests When the overview is opened in a new tab, it normally works fine, but when the overview is opened in the first tab when starting Epiphany, there are no thumbnails for any of the sites: just empty placeholder images, at least on my machine. That's not good. The snapshot service does not have the images ready immediately when requested by EphyAboutHandler, so EphyAboutHandler is not able to insert them into the overview HTML when it is created. Instead, it inserts a placeholder image. Then, when the snapshot service has finished getting the thumbnail, it makes a D-Bus call to the web extension to convey the file URI for the thumbnail (for each URL in the overview). EphyWebOverview replaces the placeholder image with the thumbnail when it receives the call. The problem is the call can occur before WebKit has finished loading the overview HTML, in which case the thumbnail changes get dropped and we wind up with a bunch of placeholder images. Instead, remember the thumbnail change requests as they come in, then apply them all after the document has loaded.
I am always hitting the g_assert_not_reached() I added in this patch when starting Ephy with an overview tab saved in the session; the problem is the list before the assert is empty (not yet sure why). I'm not sure what changed; maybe last time I only tested Ephy with no session saved.
I did verify it's not related to the D-Bus rework.
The following fix has been pushed: b5e57d6 web-overview: Don't crash when unexpected thumbnail change is received
Created attachment 320658 [details] [review] web-overview: Don't crash when unexpected thumbnail change is received Sometimes we receive thumbnail changes for pages that aren't in the overview. It's probably a bug, but there don't seem to be any user-visible effects, so let's not crash over this.
(In reply to Michael Catanzaro from comment #7) > I am always hitting the g_assert_not_reached() I added in this patch when > starting Ephy with an overview tab saved in the session; the problem is the > list before the assert is empty (not yet sure why). Wrong, rather, we reach the NULL at the end of the list, then l is NULL. GLists are a bit confusing in a backtrace. :)
It regressed due to the private D-Bus connections (bug #761009), I didn't notice at first, but there is a significant delay between when new_connection_cb runs in the UI process and when dbus_connection_created_cb runs in the web process, in which thumbnail requests are being lost. :/
Created attachment 322543 [details] [review] embed-shell: Do not drop overview thumbnail updates here either Wait for the EphyWebExtension in the web process to start listening for method calls before sending them. We know it's ready when it sends the PageCreated signal.
Note, I used g_object_set_data and a timeout source, rather than adding an initialized property to EphyWebExtensionProxy and using g_object_notify, because (a) it was a bit easier (not a valid reason), and (b) I've been toying with using gdbus-codegen to generate EphyWebExtensionProxy and so am not keen on adding more custom code there. Can easily change that if you prefer. I also investigated adding some generic code to EphyWebExtensionProxy to delay arbitrary method calls, but that's quite messy to be used by only these thumbnail requests, and it completely gives up on (b).
Also we don't technically need the weak pointer in DelayedThumbnailUpdateData, because ephy_web_extension_proxy_history_set_url_thumbnail returns harmlessly if it's passed an NULL EphyWebExtensionProxy, but I'm not sure that's desirable behavior: I don't see any reason why it should be NULL-safe when EphyEmbedShell can track for sure whether its EphyWebExtensionProxys are NULL.
Attachment 322543 [details] pushed as b77c556 - embed-shell: Do not drop overview thumbnail updates here either