GNOME Bugzilla – Bug 593743
Sort out tab, window, loading, <insert your fav> title madness
Last modified: 2010-06-28 08:21:39 UTC
I'm attaching two patches, the first one modifying ephy-web-view.c avoids "" titles while loading pages. Instead it shows "Loading...". The current behaviour is not modified other than the corner case when the pages is loading BUT there is no loading_title yet (not sure who to blame). loading_title getter is not used anywhere btw, and can't think of it as useful, it's possible functionality is already covered by title_composite. Anyway, the logic now is: if we are loading then tell the use what are we loading or if we don't know yet just let them know we are loading; if we are not loading and the page is blank then say so, otherwise just show the real title. The second patch makes tab titles sync with window titles, we are using composited title for windows but plain title for tabs, the current behaviour allows pages with "Loading slashdot.org" titles but Blank page tab titles, this is obviously evil and misleading.
Created attachment 142190 [details] [review] [PATCH] ephy-web-view: avoid empty titles while loading Bug 593743 - Sort out tab, window, loading, <insert your fav> title madness --- embed/ephy-web-view.c | 14 +++++--------- 1 files changed, 5 insertions(+), 9 deletions(-)
Created attachment 142191 [details] [review] [PATCH] ephy-notebook: sync tab title with window title Bug 593743 - Sort out tab, window, loading, <insert your fav> title madness --- src/ephy-notebook.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
On second thought, the real problem with the empty titles is that loading_title is not set by us until we get a response from the host. Deserves further investigation, this would qualify as a work around only.
*** Bug 606098 has been marked as a duplicate of this bug. ***
Created attachment 150987 [details] [review] Proposed patch This patch adds a new variable to handle the state of the title, the behavior of the title is like the one in Chrome and Firefox.
(In reply to comment #5) > Created an attachment (id=150987) [details] [review] > Proposed patch > > This patch adds a new variable to handle the state of the title, the behavior > of the title is like the one in Chrome and Firefox. I don't really like this approach, I think we are just confusing things more and more here. You are workarounding the fact that 'is_blank' has stopped working, in a way, and adding a new else branch in the get_composite_title, with new logic and a new title flag. Logically the page is still blank, so we should be using the original if, but in the WebKit port we know set the address earlier than in the gecko port through the provisional data source, which contains the address the user has typed (which not be even valid), see load_status_changed_cb in ephy-embed.c. I think the first step in fixing this correctly is to clear this situation, reflecting the new situation correctly in the 'is_blank' flag by acknowledging that the address set from the provisional data source is not final and the page is still blank for our purposes. Then can move on to fix this bug.
Xan is right, the whole state code is a little mess, the is_blank actually did not affect this problem because the code is using get_title, not get_title_composite. And things like the NEGOTIATING are not being correctly used. I'm going to check this code in more detail. I've reopened the blank page bug 606098, and added a patch that fixes the current situation, and I'm going to work in deeper review in this bug. BTW, we could use this bug to agree about the loading sequence, after talking with Xan a little bit this is an initial proposal: 1) "" (object created, rendering the tab), currently is "Blank page", until LOAD_PROVISIONAL (very short time) 2) "[S] <typed_uri>", from LOAD_PROVISIONAL to LOAD_COMMITTED 3) "[S] <committed_uri>, from LOAD_COMMITTED to didReceiveTitle 4) "[S] <title>, from didReceiveTitle to LOAD_FINISHED 5) "<title>" from LOAD_FINISHED Comments?
Created attachment 151954 [details] [review] Proposed patch This is the first proposal, it needs more testing and probably review the code structure.
Created attachment 152341 [details] [review] Proposed patch First review proposal, check it and send comments about it.
(In reply to comment #9) > Created an attachment (id=152341) [details] [review] > Proposed patch > > First review proposal, check it and send comments about it. So, I think the direction the patch moves things is good, but I think you are doing more changes in one go than we should do (the biggest one is handling LOAD_FAILED explicitly, which AFAICT we were not doing before). So, as usual, let's split this to make things easier: I agree than getting rid of the epiphany custom states and using the webkit ones directly is a good thing, so let's do first one patch that does that and nothing else.
Created attachment 153357 [details] [review] Remove ephy net states patch Thanks for the comments. I've divided the patch in three steps as suggested: 1. Remove ephy net states 2. Refactor load status 3. Handle failed status And this is the first patch.
Created attachment 153358 [details] [review] Refactor web view load status
Created attachment 153359 [details] [review] Handle failed load status
Review of attachment 153357 [details] [review]: :) ::: embed/ephy-web-view.c @@ +2332,3 @@ * @view: an #EphyWebView * @uri: the uri associated with @view + * @state: an #WebKitLoadStatus an -> a
Comment on attachment 153357 [details] [review] Remove ephy net states patch Looks good!
Comment on attachment 153358 [details] [review] Refactor web view load status You dropped the call to update_navigation_flags somewhere along the way; the function is defined but you never call it. Also, the style guidelines (see HACKING) say to not use braces for one line clauses, would be good to take to opportunity to do that. And one nitpick, 'eview' for the EphyWebView is kind of icky, just 'view' is fine IMHO.
Comment on attachment 153359 [details] [review] Handle failed load status I think we should have as title something like "Problem occurred while loading %s", with the failed URL in there.
Created attachment 159922 [details] [review] Remove ephy net states patch Updated and reviewed patch
Created attachment 159923 [details] [review] Refactor web view load status patch Updated and reviewed patch
Created attachment 159924 [details] [review] Refactor web view load status patch Updated and reviewed patch
Created attachment 159926 [details] [review] Refactor web view load status patch I messed up with the patches badly, sorry. Trying to fix the mess.
Created attachment 159927 [details] [review] Handle failed load status patch Reviewed and updated
Created attachment 159930 [details] [review] Handle failed load status patch Small style review.
Comment on attachment 159922 [details] [review] Remove ephy net states patch ok!
Created attachment 160253 [details] [review] Fix the load title in the provisional state
Created attachment 160254 [details] [review] Handle failed load status patch
Comment on attachment 160253 [details] [review] Fix the load title in the provisional state The patch looks good, but the commit message is backwards: you are avoiding setting the provisional uri as title, not the other way around.
Comment on attachment 160254 [details] [review] Handle failed load status patch Looks fine, but should be 'A problem occurred...' ?
All patches landed, closing bug.
There's still a problem with the title here: Whenever you try to download a file with epiphany, the main window title always changes to "A problem occurred while loading <URL>", and then it either shows the dialog to select the directory or automatically starts the download process (depending on your config). I've been taking a look to this yesterday and it seems the problem comes because WebKit always emits the "load-error" signal whenever an unhandled mime-type is going to be retrieved through epiphany, resulting in the following error code: WEBKIT_POLICY_ERROR_FRAME_LOAD_INTERRUPTED_BY_POLICY_CHANGE. I was digging for a while with gdb and it looks policyChecker()->stopCheck() is getting called whenever this (downloading a file) happens, which ends up emitting this signal and therefore being captured in ephy and updating the title according to this piece of code: static gboolean load_error_cb (WebKitWebView *web_view, WebKitWebFrame *frame, gchar *uri, GError *error, gpointer user_data) { EphyWebView *view = EPHY_WEB_VIEW (web_view); if (error->code != WEBKIT_NETWORK_ERROR_CANCELLED) { gchar *message; message = g_strdup_printf (_("A problem occurred while loading %s"), uri); ephy_web_view_set_title (view, message); g_free (message); _ephy_web_view_set_icon_address (view, NULL); } else { EphyWebViewPrivate *priv = view->priv; if (priv->expire_address_now) { const gchar* prev_uri; prev_uri = webkit_web_view_get_uri (web_view); ephy_web_view_set_typed_address (view, NULL); ephy_web_view_set_address (view, prev_uri); } } return FALSE; } Hence, I'm not wondering what to do... perhaps there's a way not to get this change of policy being done (modify FrameLoaderClient::cancelPolicyCheck() not to do anything when, for instance, current status is LOAD_FINISHED?), or we should just add an extra condition to the "if" above like this: if (error->code != WEBKIT_NETWORK_ERROR_CANCELLED && error->code != WEBKIT_POLICY_ERROR_FRAME_LOAD_INTERRUPTED_BY_POLICY_CHANGE) { [...] } else { [...] } This second option would hide the problem for sure, but not sure that would be the right thing to do... Opinions? BTW, I think reopening this bug would be in place, but can't do that since I'm not the reporter. Diego? :-)
Created attachment 164590 [details] [review] Don't report errors in title for policy change load errors Hmmm... seeing this in FrameLoaderClientGtk.cpp: bool FrameLoaderClient::shouldFallBack(const ResourceError& error) { return !(error.isCancellation() || error.errorCode() == WEBKIT_POLICY_ERROR_FRAME_LOAD_INTERRUPTED_BY_POLICY_CHANGE || error.errorCode() == WEBKIT_PLUGIN_ERROR_WILL_HANDLE_LOAD); } ...looks like perhaps the idea of ignoring load errors in ephy-web-view because of policy changes (along with cancellations), wouldn't be so crazy. Still don't understand the whole thing behind this lines but just in case it was the right thing to do, I'm attaching the patch right now. But still, eager to hear opinions or comments that help me understand why this happens this way, so don't be shy :-)
(In reply to comment #30) > There's still a problem with the title here: > > Whenever you try to download a file with epiphany, the main window title always > changes to "A problem occurred while loading <URL>", and then it either shows > the dialog to select the directory or automatically starts the download process > (depending on your config). > > Opinions? > I think you should open a new bug with this description and propose a patch with what you discovered, if we do not do that we would end up with huge bugs one about each topic. Use something like you proposed but check if the else code is not also a problem in that case: if (error->code != WEBKIT_NETWORK_ERROR_CANCELLED && error->code != WEBKIT_POLICY_ERROR_FRAME_LOAD_INTERRUPTED_BY_POLICY_CHANGE) { [...] } else { [...] }
(In reply to comment #32) > [...] > I think you should open a new bug with this description and propose a patch > with what you discovered, if we do not do that we would end up with huge bugs > one about each topic. Done: https://bugzilla.gnome.org/show_bug.cgi?id=623012 > Use something like you proposed but check if the else code is not also a > problem in that case: > > if (error->code != WEBKIT_NETWORK_ERROR_CANCELLED && > error->code != > WEBKIT_POLICY_ERROR_FRAME_LOAD_INTERRUPTED_BY_POLICY_CHANGE) { > > [...] > } else { > [...] > } I think is not, but I'll double check it anyway :-) Thanks for the feedback