[webkit-reviews] review denied: [Bug 72949] [GTK] Add WebKitDownload to WebKit2 GTK+ API : [Attachment 117631] Updated patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Jan 19 10:52:28 PST 2012
Martin Robinson <mrobinson at webkit.org> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 72949: [GTK] Add WebKitDownload to WebKit2 GTK+ API
https://bugs.webkit.org/show_bug.cgi?id=72949
Attachment 117631: Updated patch
https://bugs.webkit.org/attachment.cgi?id=117631&action=review
------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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.
More information about the webkit-reviews
mailing list