RESOLVED FIXED Bug 72949
[GTK] Add WebKitDownload to WebKit2 GTK+ API
https://bugs.webkit.org/show_bug.cgi?id=72949
Summary [GTK] Add WebKitDownload to WebKit2 GTK+ API
Carlos Garcia Campos
Reported 2011-11-22 06:24:21 PST
To represent a download operation.
Attachments
Patch (22.64 KB, patch)
2011-11-22 06:32 PST, Carlos Garcia Campos
no flags
New patch (33.40 KB, patch)
2011-11-29 12:30 PST, Carlos Garcia Campos
no flags
Updated patch (33.40 KB, patch)
2011-12-02 08:34 PST, Carlos Garcia Campos
mrobinson: review-
Updated patch according to review (34.36 KB, patch)
2012-01-23 06:39 PST, Carlos Garcia Campos
mrobinson: review+
gustavo.noronha: commit-queue-
Carlos Garcia Campos
Comment 1 2011-11-22 06:32:44 PST
Created attachment 116215 [details] Patch This is part for the download client implementation, reported as a separate bug report to make it easier to review. It doesn't include unit tests, because they will be added by following patches.
WebKit Review Bot
Comment 2 2011-11-22 06:36:25 PST
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Carlos Garcia Campos
Comment 3 2011-11-29 12:30:16 PST
Created attachment 117022 [details] New patch I moved most of the signals from the download client object to this one.
Carlos Garcia Campos
Comment 4 2011-12-02 08:34:25 PST
Created attachment 117631 [details] Updated patch Fixed a bug in the progress notification when downloading more than one file
Martin Robinson
Comment 5 2012-01-19 10:52:29 PST
Comment on attachment 117631 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=117631&action=review Look nice! r- only because finished isn't emitted during failure. > Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:125 > + * The URI where the download will be saved. Might want to be a bit more explicit: The local URI to where the download... > Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:138 > + * The response of the download. How about: The #WebKitURIResponse associated with this download. ? > Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:193 > + * This signal is emitted when download completes. In case of errors, > + * the download finishes when #WebKitDownload::failed signal is emitted. The loader client emits a finished message as well as a failed message. Let's copy that behavior here. > Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:260 > + * This signal is emitted after #WebKitDownload::decide-destination to > + * notify that destination file has been created successfully at @destination. Might want to mention that this happens when the file is empty and not when the file is finished downloading. > Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:312 > + // in more then 0.7 secs from now, or the last notified progress "more than 0.7 seconds ago" is slightly clearer here. > Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:319 > + && (currentElapsed - priv->lastElapsed) < 0.7 Why 0.7 seconds? Wouldn't it be better to use a value like 60 frames a second? That will ensure that download progress bars animate smoothly.
Carlos Garcia Campos
Comment 6 2012-01-20 03:45:24 PST
(In reply to comment #5) > (From update of attachment 117631 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=117631&action=review > > Look nice! r- only because finished isn't emitted during failure. Thanks for reviewing! > > Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:125 > > + * The URI where the download will be saved. > > Might want to be a bit more explicit: The local URI to where the download... Sure. > > Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:138 > > + * The response of the download. > > How about: The #WebKitURIResponse associated with this download. ? Better. > > Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:193 > > + * This signal is emitted when download completes. In case of errors, > > + * the download finishes when #WebKitDownload::failed signal is emitted. > > The loader client emits a finished message as well as a failed message. Let's copy that behavior here. Right, this patch was written before the loader client simplification, no it makes sense to follow the same approach. > > Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:260 > > + * This signal is emitted after #WebKitDownload::decide-destination to > > + * notify that destination file has been created successfully at @destination. > > Might want to mention that this happens when the file is empty and not when the file is finished downloading. Good point. > > Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:312 > > + // in more then 0.7 secs from now, or the last notified progress > > "more than 0.7 seconds ago" is slightly clearer here. Ok. > > Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:319 > > + && (currentElapsed - priv->lastElapsed) < 0.7 > > Why 0.7 seconds? Wouldn't it be better to use a value like 60 frames a second? That will ensure that download progress bars animate smoothly. I have no idea, I copied this from wk1 code.
Martin Robinson
Comment 7 2012-01-20 08:02:05 PST
> > > Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:319 > > > + && (currentElapsed - priv->lastElapsed) < 0.7 > > > > Why 0.7 seconds? Wouldn't it be better to use a value like 60 frames a second? That will ensure that download progress bars animate smoothly. > > I have no idea, I copied this from wk1 code. I think 30-60 frames per second might be the way to go here.
Carlos Garcia Campos
Comment 8 2012-01-23 06:39:04 PST
Created attachment 123551 [details] Updated patch according to review Updated to apply on current git master and addressed review comments.
Collabora GTK+ EWS bot
Comment 9 2012-01-23 14:15:02 PST
Comment on attachment 123551 [details] Updated patch according to review Attachment 123551 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11275070
Martin Robinson
Comment 10 2012-01-23 15:56:07 PST
Comment on attachment 123551 [details] Updated patch according to review View in context: https://bugs.webkit.org/attachment.cgi?id=123551&action=review Nice! > Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:215 > + * after an error and ##WebKitDownload::finished signal is emitted after this one. You have two ## here instead of just one. I'm not sure if that's reponsible for making the doc build fail or not. Might want to double-check it before you land.
Carlos Garcia Campos
Comment 11 2012-01-24 00:46:57 PST
(In reply to comment #10) > (From update of attachment 123551 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=123551&action=review > > Nice! > > > Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:215 > > + * after an error and ##WebKitDownload::finished signal is emitted after this one. > > You have two ## here instead of just one. I'm not sure if that's reponsible for making the doc build fail or not. Might want to double-check it before you land. Nop, the problem is that I forgot to add the <SECTION> tag for WebKitDownload in webkit2gtk-sections.txt
Carlos Garcia Campos
Comment 12 2012-01-24 00:50:08 PST
Note You need to log in before you can comment on or make changes to this bug.