GNOME Bugzilla – Bug 623012
Title changes to wrong text when downloading files
Last modified: 2010-12-01 10:13:05 UTC
(opening a new bug as pointed out in bug 593743) 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 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...
Created attachment 164788 [details] [review] Patch proposal 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 :-)
I think your patch it is correct, we have to control that error case in epiphany. Could you check if should add WEBKIT_PLUGIN_ERROR_WILL_HANDLE_LOAD also to the conditions?
(In reply to comment #2) > I think your patch it is correct, we have to control that error case in > epiphany. Could you check if should add WEBKIT_PLUGIN_ERROR_WILL_HANDLE_LOAD > also to the conditions? Of course, but if you could give some advice in how to get that error happening that would be great, as it would help me to reproduce a proper scenario to test the patch.
> (In reply to comment #3) > Of course, but if you could give some advice in how to get that error happening > that would be great, as it would help me to reproduce a proper scenario to test > the patch. I've tested it, we have to add that value also to the condition. Could you test it and upload the patch? You just have to open a resource that is not going to be rendered using the webview, for instance an mp3. http://www.thesoundofindie.com/archive/2006/20061023/TF-HappyBirthday.mp3
(In reply to comment #4) > > (In reply to comment #3) > > Of course, but if you could give some advice in how to get that error happening > > that would be great, as it would help me to reproduce a proper scenario to test > > the patch. > > I've tested it, we have to add that value also to the condition. Could you test > it and upload the patch? You just have to open a resource that is not going to > be rendered using the webview, for instance an mp3. You're talking about the error WEBKIT_PLUGIN_ERROR_WILL_HANDLE_LOAD, right? If affirmative, I'll try to test it asap then, that is, hopefully today :-)
(In reply to comment #5) > > [...] > > You're talking about the error WEBKIT_PLUGIN_ERROR_WILL_HANDLE_LOAD, right? > Yes
Created attachment 167430 [details] [review] Patch proposal (In reply to comment #4) > I've tested it, we have to add that value also to the condition. Could you test > it and upload the patch? You just have to open a resource that is not going to > be rendered using the webview, for instance an mp3. > > http://www.thesoundofindie.com/archive/2006/20061023/TF-HappyBirthday.mp3 It works fine as you suggested. Now attaching the new patch. Sorry for the delay.
(In reply to comment #7) > > [...] > > It works fine as you suggested. Now attaching the new patch. > Now that I check the code I'm not sure if this two situations should to the else clause or just avoid doing any action. The else action IIRC is what should happen when the user cancels the loading clicking the stop/cancel button. Maybe we should just add another if in the else with the cancelled value and forget about this values.
Alex and me were discussing about this and at the end we found the behavior got with this last patch is acceptable, since it works fine in both cases: downloading a file or rendering a content through a plugin (i.e. play a mp3 file). It's arguable, though, whether ephy should show the URI of the resource to be downloaded in the address bar for the case of downloading a file (in the other case -the plugin- it just works fine), but IMHO (and I thik alex agrees on that) it's correct not to do it so since: - When you download a file you're not rendering anything in the browser so changing the address bar would make no sense. - After confirm/cancel the download in the dialog you usually go back to the source web, so it makes sense the address bar still shows the same URI - A poor argument, I know it, but it's what other browsers do :-) In the case of the plugin, it's true that the code suggests that the previous URI to the plugin's would be put in the address bar, but somehow that's not happening and everything just seems to work fine for that case (you click on the mp3 file link, the address bar changes, the web view renders the plugin and everything works fine). We're not sure yet why in this case it works fine (maybe it's because the previous URI when the plugin is being loaded has already changed) so it's worth mentioning the "issue" here, but we do think that for the time being it's better to move things forward and get this patch (which works ok) in and maybe later on to file a new bug if we're able to reliably reproduce any. So, wrapping up... we agreed the last patch is ok and could be committed now. Any other opinion?
Created attachment 175557 [details] [review] Proposed patch Fisrt attempt to control all the errors in the load_error_cb.
Review of attachment 175557 [details] [review]: ::: embed/ephy-web-view.c @@ +1997,3 @@ +static void set_load_error (EphyWebView *view, + gchar *uri) Perhaps we can call this set_main_frame_load_error or something? I guess in the future we'll have inline error messages like firefox/chrome. @@ +1998,3 @@ +static void set_load_error (EphyWebView *view, + gchar *uri) +{ In ephy we try to not use the g* types, read HACKING for the details. Also I guess this can be const char * if I'm really picky. @@ +2018,3 @@ EphyWebView *view = EPHY_WEB_VIEW (web_view); + if (webkit_web_view_get_main_frame (web_view) == frame) { If you are going to do nothing in the method if this is not the case just exit early. @@ +2027,3 @@ + (error->domain == WEBKIT_POLICY_ERROR) || + (error->domain == WEBKIT_PLUGIN_ERROR)); + g_return_if_fail is better, this will crash in release builds. @@ +2060,3 @@ + case WEBKIT_PLUGIN_ERROR_WILL_HANDLE_LOAD: + case WEBKIT_POLICY_ERROR_FRAME_LOAD_INTERRUPTED_BY_POLICY_CHANGE: + default: Maybe a comment here explaining we let WebKit do its thing.
Created attachment 175567 [details] [review] Proposed patch Addressed all the issues in the former patch.
Created attachment 175568 [details] [review] Proposed patch Forgot to upload all the fixes.
Review of attachment 175568 [details] [review]: Ok.
pushed http://git.gnome.org/browse/epiphany/commit/?id=ef817ec85f6854a34dae62f977c834a2fe1d3c6e