GNOME Bugzilla – Bug 675302
Delay tab loading on session restore
Last modified: 2013-02-12 16:09:53 UTC
Do not load all the restored tabs at once, but only the focused tab in each restored window. The others can be loaded when they are actually focused, if ever. Firefox seems to be doing this since version 13, looks like a good idea to me.
Also, when we switch to the overview no-tabs UI, this will make absolute sense anyways, so it seems to be the way to go.
Created attachment 234948 [details] [review] WIP I'm working on Not finished yet, think I need to use the stored favicon instead of using the loading spinner, like Firefox does. Also, I made set_{address,title} public, but I'll actually do something like what we have for error pages, instead. Uploading here so I can maybe get some early feedback =)
Created attachment 235130 [details] [review] Patch OK, this is worthier of review =)
Review of attachment 235130 [details] [review]: Looks great in general, thanks for hacking on it. ::: embed/ephy-embed.c @@ +419,3 @@ + if (priv->delayed_request) + g_clear_object (&priv->delayed_request); This is not needed if we do g_clear_object @@ +1196,3 @@ + + g_clear_object (&embed->priv->delayed_request); +} Can't we just do this automatically when the embed gets focus, becomes drawable (ie, visible + shown), or something like that? Feels like we should be able to avoid the notebook telling us. @@ +1211,3 @@ + return (gboolean)embed->priv->delayed_request; +} + Should add g_return_if_fail to the methods where needed. ::: embed/ephy-web-view.c @@ +2384,3 @@ + const char *uri, + const char *title) +{ Also missing g_return_if... @@ +2387,3 @@ + char *html = g_markup_printf_escaped ("<head><title>%s</title></head>", title); + + /* We want onlye the actual load to be the one recorded in history, but s/onlye/only/ ::: src/ephy-session.c @@ +1014,3 @@ + */ + embed = EPHY_EMBED (gtk_notebook_get_nth_page (GTK_NOTEBOOK (notebook), context->active_tab)); + ephy_embed_notify_switched_to (embed); See my comment in EphyEmbed, if EphyEmbed does by itself this is also not needed. ::: src/ephy-window.c @@ +4372,3 @@ +ephy_window_is_empty (EphyWindow *window) +{ + return !!!gtk_notebook_get_n_pages (window->priv->notebook); This method does not seem to be used anywhere (?)
Created attachment 235550 [details] [review] Patch using a signal emitted by the session So this tries to use the map signal and uses a session signal like we discussed. After playing with this a while I am not sure it's worth pursuing. Like you said on IRC, this does not help with keeping the feature fully inside EphyEmbed, and makes EphyEmbed depend on stuff that's in the src/ module, which requires us to add new include paths even, I think this is something we want to avoid. I think the more robust way of managing this for now is to have an explicit notification, for now, and we can revisit this when we switch to the new design, I'll post another patch with that, let me know what you think.
Created attachment 235551 [details] [review] Earlier patch with just the other comments addressed, but using switched_to
Well, I actually said Shell, not Session, so having the signal in EphyEmbedShell would work fine wrt include paths. But yes, as I also said this could defeat purpose anyway, so I'll have a look again at the patch.
Oh, duh, I thought you meant EphyShell, which would be as problematic as EphySession. Let me play with that idea beofre you get to this again, then. Now that I slept over it, I guess we could even use EphyEmbedSingle for the signal...
Created attachment 235595 [details] [review] Much better! EphyEmbedShell works nicely =)
Review of attachment 235595 [details] [review]: I think this looks great. Having a test would be good, I have learned that all the session code is flimsy as hell. ::: embed/ephy-embed.c @@ +1220,3 @@ + + g_object_ref (request); + embed->priv->delayed_request = request; Just a nitpick but it would probably be a good idea here to make sure we don't already have a delayed request set before saving the new one (so just unref one if it exists), and check the request type.
The only comment I have after testing it is that the "delayed tabs" do not immediately start to show the spinner when you switch to them, so for a second or two it seems that nothing is happening. That should probably be fixed.
Cool! In theory we should never set a delayed request more than once on an embed, but I guess we might in the future, so I'll add a g_clear_object() there. As for the test, I'll write one, but I think for testing this I'll need to actually display the window, I don't think realizing will be enough for getting the embed mapped. Would it be ok to add infrastructure to run the test inside an xvfb, for that? Btw, the session test is currently failing for me, because some about:* addresses are coming back from _get_address() with the ephy- prefix and some aren't, should it always have the ephy- prefix or never?
Created attachment 235708 [details] [review] Patch with test Testing by displaying the window works, if you think using Xvfb is a good idea I'll try to figure out why the test is not being able to look up icons and crashing when running under Xvfb heh
Created attachment 235728 [details] [review] Patch with test Testing by displaying the window works, if you think using Xvfb is a good idea I'll try to figure out why the test is not being able to look up icons and crashing when running under Xvfb heh
I think for now you can land it without the test while we figure out the Xvfb stuff. I went out of my way to make the tests not show windows (see the shell mode, etc), so I'd rather keep it that way. My comment was more like "we should really test this" more than "do not land it without the test". Also no idea about the test failure you mention, I'm not getting that :S
OK!
Comment on attachment 235728 [details] [review] Patch with test Landed without the test bits and a missing cast to G_OBJECT as ce7ab1e34804d9f7529bed13267f4619c37e32d9
I opened bug 693655 to track the test