Bug 72949 - [GTK] Add WebKitDownload to WebKit2 GTK+ API
Summary: [GTK] Add WebKitDownload to WebKit2 GTK+ API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on: 72946
Blocks: 72952
  Show dependency treegraph
 
Reported: 2011-11-22 06:24 PST by Carlos Garcia Campos
Modified: 2012-01-24 00:50 PST (History)
6 users (show)

See Also:


Attachments
Patch (22.64 KB, patch)
2011-11-22 06:32 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
New patch (33.40 KB, patch)
2011-11-29 12:30 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch (33.40 KB, patch)
2011-12-02 08:34 PST, Carlos Garcia Campos
mrobinson: review-
Details | Formatted Diff | Diff
Updated patch according to review (34.36 KB, patch)
2012-01-23 06:39 PST, Carlos Garcia Campos
mrobinson: review+
gustavo.noronha: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2011-11-22 06:24:21 PST
To represent a download operation.
Comment 1 Carlos Garcia Campos 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.
Comment 2 WebKit Review Bot 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
Comment 3 Carlos Garcia Campos 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.
Comment 4 Carlos Garcia Campos 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
Comment 5 Martin Robinson 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.
Comment 6 Carlos Garcia Campos 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.
Comment 7 Martin Robinson 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.
Comment 8 Carlos Garcia Campos 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.
Comment 9 Collabora GTK+ EWS bot 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
Comment 10 Martin Robinson 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.
Comment 11 Carlos Garcia Campos 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
Comment 12 Carlos Garcia Campos 2012-01-24 00:50:08 PST
Committed r105704: <http://trac.webkit.org/changeset/105704>