GNOME Bugzilla – Bug 678612
Port downloads to WebKit2
Last modified: 2012-06-27 12:17:38 UTC
Created attachment 217005 [details] [review] Pass suggested filename to define_destination_uri() Downloads API is a bit different in WebKit2, so in this case we also need some refactorings.
Created attachment 217006 [details] [review] remove _ephy_download_new() internal function
Created attachment 217007 [details] [review] add ephy_download_widget_download_finished()
Created attachment 217008 [details] [review] add get_destination_basename_from_download() helper function
Created attachment 217009 [details] [review] Patch
Review of attachment 217005 [details] [review]: OK.
Review of attachment 217006 [details] [review]: Makes sense.
Review of attachment 217007 [details] [review]: I think ephy_download_widget_download_is_finished would be better no? You can push with the rename, no need to re-upload the patch. Good cleanup!
Review of attachment 217008 [details] [review]: ::: lib/widgets/ephy-download-widget.c @@ +94,3 @@ +#endif + if (!dest) + return NULL; I'm a bit confused. Isn't this basically breaking the WK1 case?
(In reply to comment #8) > Review of attachment 217008 [details] [review]: > > ::: lib/widgets/ephy-download-widget.c > @@ +94,3 @@ > +#endif > + if (!dest) > + return NULL; > > I'm a bit confused. Isn't this basically breaking the WK1 case? It's a mistake I made when split the patch, the idea is indeed the opposite, dest = NUL for webkit2, and use wk1 api to get the destination. Then, the wk2 patch adds wk2 api to get the destination.
Created attachment 217224 [details] [review] Updated: add get_destination_basename_from_download() helper function Updated the patch to fix the issue pointed out by xan.
Created attachment 217226 [details] [review] Updated patch Just rebased
Review of attachment 217224 [details] [review]: OK.
Review of attachment 217226 [details] [review]: ::: embed/ephy-download.c @@ +950,3 @@ + } else { + ephy_download_do_download_action (download, EPHY_DOWNLOAD_ACTION_NONE); + } The style guide says don't do braces for one-liners (I think the code was already broken here though). This code was recently improved, so you need to update it: - if AUTO_DOWNLOADS is set also check if priv->action is NONE - otherwise do priv->action, not NONE. (Basically copy the WK1 stuff).
Created attachment 217374 [details] [review] Updated patch
Review of attachment 217374 [details] [review]: ::: embed/ephy-web-view.c @@ +1756,3 @@ + single = ephy_embed_shell_get_embed_single (embed_shell); + request = webkit_response_policy_decision_get_request (response_decision); + g_signal_emit_by_name (single, "handle-content", mime_type, uri, &handled); Hrm, where is uri being initialized here? I guess the line just got lost in copy&paste? If so just fix it and commit.
(In reply to comment #15) > Review of attachment 217374 [details] [review]: > > ::: embed/ephy-web-view.c > @@ +1756,3 @@ > + single = ephy_embed_shell_get_embed_single (embed_shell); > + request = webkit_response_policy_decision_get_request (response_decision); > + g_signal_emit_by_name (single, "handle-content", mime_type, uri, &handled); > > Hrm, where is uri being initialized here? I guess the line just got lost in > copy&paste? If so just fix it and commit. Good catch, it seems I removed that line by mistake resolving merge conflicts.