GNOME Bugzilla – Bug 678993
There can be two widgets for the same download in WebKit2
Last modified: 2013-01-04 13:53:08 UTC
Created attachment 217431 [details] [review] Patch Working on context menu I noticed that when a download is started from the context menu (not the default one) the download appers twice in the downloads bar. Attached patch fixes the issue.
Why is this better than making the code not fire the signal twice?
The signal is not fired twice, the difference is that in wk1, download-request signal is only emitted for downloads started automatically by the web view, for example, as a result of policy decision. In WebKit2, the download-started signal is emitted by the web context every time a new download starts, even if the download has been manually started.
OK, let me put it this way: I think the code should be changed to take this properly into account instead of just scanning the list to make sure there are no duplicates.
I don't know what you mean by take this properly into account. I'll explain the situation in more detail. - When a download is created automatically by webkit (as a result of a policy decision, or as an action of the default context menu, etc.) ephy is notified for the first time by the WebKitWebContext::download-started signal. In this case, we need to create a EphyDownload object for the given WebKitDownload. ephy_download_new_for_download() adds the new download to the list of downloads in EphyEmbedShell. - When a download is created manually by ephy, ephy_download_new_for_download() adds the new download to the list of downloads in EphyEmbedShell, and when the download is started in the web process, the download-started signal is emitted. In this case the EphyDownload for the given WebKitDownload already exists and has already been added to the list of downloads in EphyEmbedShell. In download_started callback we don't know if the download was started by ephy or webkit, and we only have a WebKitDownload object. A better solution might be to use a GHashTable to map WebKitDownload to EphyDownload instead of a GList, but the solution would be the same, checking whether the download is in the map already and create the EphyDownload if it's not in the map. Another solution could be to always create the EphyDownload in download_started callback for the given WebKitDownload, but that would make everything more difficult, because right after creating a download manually a set of properties are set like the destination, action, connect to complete signal, etc. Passing all that info to the callback would be painful.
Created attachment 218078 [details] [review] New patch Simpler solution.
Review of attachment 218078 [details] [review]: OK. I think I'd prefer to do something like you suggest in the last bug, even if it would complicate the code a bit, but we can live with this hack for now in order to fix the big bug. Just update the patch for the API changes and add a comment explaining why we have to do this. ::: embed/ephy-download.c @@ +1083,2 @@ ephy_download->priv->download = g_object_ref (download); #ifdef HAVE_WEBKIT2 Please add a large comment here explaining why do we do this, you can copy&paste most of your comment in the bug.