Bug 32359

Summary: [GTK] Don't assume downloads are always synchronous
Product: WebKit Reporter: Christian Dywan <christian>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Major CC: gns, webkit.review.bot
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Check if the destination URI is set and clarify the documentation
none
Check destination URI, clarify docs and test downloads both ways
none
Check destination URI, clarify docs and test downloads both ways #2 gns: review+, gns: commit-queue-

Description Christian Dywan 2009-12-09 20:32:39 PST
The download-requested signal assumes if the download was handled, it can start it right away. This is wrong if the application needs to handle the download asynchronously. In that case, WebKit should leave it up to the API user to start or cancel the download as appropriate.
Comment 1 Christian Dywan 2009-12-09 20:37:42 PST
Created attachment 44590 [details]
Check if the destination URI is set and clarify the documentation
Comment 2 WebKit Review Bot 2009-12-09 20:42:00 PST
style-queue ran check-webkit-style on attachment 44590 [details] without any errors.
Comment 3 Gustavo Noronha (kov) 2009-12-16 03:06:22 PST
Comment on attachment 44590 [details]
Check if the destination URI is set and clarify the documentation

This works for me. Can you add a unittest, to make sure this works, to testdownload?
Comment 4 Christian Dywan 2009-12-17 02:23:35 PST
Created attachment 45044 [details]
Check destination URI, clarify docs and test downloads both ways

Updated the patch, to test downloads with having the destination set in the callback, and again with setting it later. The test breaks without the frame loader change.
Comment 5 WebKit Review Bot 2009-12-17 02:24:05 PST
Attachment 45044 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/gtk/tests/testdownload.c:29:  Declaration has space between * and variable name in WebKitDownload* theDownload  [whitespace/declaration] [3]
WebKit/gtk/tests/testdownload.c:137:  Declaration has space between * and variable name in gchar* filename  [whitespace/declaration] [3]
WebKit/gtk/tests/testdownload.c:138:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/tests/testdownload.c:148:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/tests/testdownload.c:171:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 5
Comment 6 Christian Dywan 2009-12-17 02:38:21 PST
Created attachment 45046 [details]
Check destination URI, clarify docs and test downloads both ways #2

Bracket and indentation fixed. The globals the bot complains about are correct as far as I can see.
Comment 7 WebKit Review Bot 2009-12-17 02:39:54 PST
Attachment 45046 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/gtk/tests/testdownload.c:29:  Declaration has space between * and variable name in WebKitDownload* theDownload  [whitespace/declaration] [3]
WebKit/gtk/tests/testdownload.c:137:  Declaration has space between * and variable name in gchar* filename  [whitespace/declaration] [3]
Total errors found: 2
Comment 8 Gustavo Noronha (kov) 2009-12-17 07:33:51 PST
Comment on attachment 45046 [details]
Check destination URI, clarify docs and test downloads both ways #2

Good, but please fix the issues pointed out by the style bot =).
Comment 9 Gustavo Noronha (kov) 2009-12-17 07:34:28 PST
(In reply to comment #8)
> (From update of attachment 45046 [details])
> Good, but please fix the issues pointed out by the style bot =).

In fact, don't. Please report those as bugs to the bot =P.
Comment 10 Christian Dywan 2009-12-17 11:16:31 PST
2009-12-17  Christian Dywan  <christian@twotoasts.de>

        Reviewed by Gustavo Noronha Silva.

        [GTK] Don't assume downloads are always synchronous
        http://bugs.webkit.org/show_bug.cgi?id=32359

        * tests/testdownload.c:
        (download_requested_cb):
        (set_filename):
        (test_webkit_download_perform):
        (test_webkit_download_synch):
        (test_webkit_download_asynch):
        (main): Test downloads synchronously and asynchronously.
        * webkit/webkitwebview.cpp:
        (webkit_web_view_class_init):
        (webkit_web_view_request_download): Only try to start a requested
        download if the destination URI is set and clarify the documentation.