To represent a download operation.
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.
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
Created attachment 117022 [details] New patch I moved most of the signals from the download client object to this one.
Created attachment 117631 [details] Updated patch Fixed a bug in the progress notification when downloading more than one file
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.
(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.
> > > 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.
Created attachment 123551 [details] Updated patch according to review Updated to apply on current git master and addressed review comments.
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
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.
(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
Committed r105704: <http://trac.webkit.org/changeset/105704>