WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
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
Committed
r105704
: <
http://trac.webkit.org/changeset/105704
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug