Bug 72949 – [GTK] Add WebKitDownload to WebKit2 GTK+ API

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

To represent a download operation.

[Carlos Garcia Campos]

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]

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]

Created attachment 117022 [details]
New patch

I moved most of the signals from the download client object to this one.

[Carlos Garcia Campos]

Created attachment 117631 [details]
Updated patch

Fixed a bug in the progress notification when downloading more than one file

[Martin Robinson]

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]

(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]

> > > 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]

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 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 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]

(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]

Committed r105704: <http://trac.webkit.org/changeset/105704>