Bug 156057

Summary: Create test infrastructure to support <a download> feature
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: WebKit Misc.Assignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Major CC: adam, bfulgham, buildbot, eoconnor, rniwa
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 156056, 156099, 156100, 156583    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews100 for mac-yosemite
none
Archive of layout-test-results from ews104 for mac-yosemite-wk2
none
Archive of layout-test-results from ews126 for ios-simulator-wk2
none
Archive of layout-test-results from ews117 for mac-yosemite
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews102 for mac-yosemite
none
Patch aestes: review+

Description Brent Fulgham 2016-03-30 22:19:32 PDT
We should be sure our implementation meets the functional and safety criteria defined in <https://html.spec.whatwg.org/multipage/semantics.html#downloading-resources>:

Specifically we want to ensure:

1. Can be used with blob: URLs and data: URLs
2. If the HTTP header Content-Disposition: is present and gives a different filename than this attribute, the HTTP header has priority over this attribute.
3. If this attribute is present and Content-Disposition: is set to inline, we should probably match Chrome and give priority to the download attribute.
4. We should only honor this attribute for links to resources with the same-origin.
Comment 1 Brent Fulgham 2016-03-31 00:24:04 PDT
Local testing confirms that data: URLs work properly. I'll add tests for blob URLs the Content-Disposition headers to confirm.
Comment 2 Brent Fulgham 2016-03-31 12:44:35 PDT
These existing tests now pass:

fast/dom/HTMLAnchorElement/anchor-nodownload.html
fast/dom/HTMLAnchorElement/anchor-download.html
fast/dom/HTMLAnchorElement/anchor-nodownload-set.html
fast/dom/HTMLAnchorElement/anchor-download-unset.html
Comment 3 Brent Fulgham 2016-04-01 00:18:51 PDT
Created attachment 275377 [details]
Patch
Comment 4 Build Bot 2016-04-01 01:03:07 PDT
Comment on attachment 275377 [details]
Patch

Attachment 275377 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1079646

New failing tests:
http/tests/security/anchor-download-allow-sameorigin.html
http/tests/security/anchor-download-allow-data.html
Comment 5 Build Bot 2016-04-01 01:03:10 PDT
Created attachment 275384 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 6 Build Bot 2016-04-01 01:07:40 PDT
Comment on attachment 275377 [details]
Patch

Attachment 275377 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1079654

New failing tests:
fast/dom/HTMLAnchorElement/anchor-download.html
fast/dom/HTMLAnchorElement/anchor-nodownload-set.html
http/tests/security/anchor-download-allow-sameorigin.html
http/tests/security/anchor-download-allow-data.html
Comment 7 Build Bot 2016-04-01 01:07:42 PDT
Created attachment 275385 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 8 Build Bot 2016-04-01 01:13:00 PDT
Comment on attachment 275377 [details]
Patch

Attachment 275377 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1079652

New failing tests:
http/tests/security/anchor-download-allow-sameorigin.html
http/tests/security/anchor-download-allow-data.html
Comment 9 Build Bot 2016-04-01 01:13:03 PDT
Created attachment 275387 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.10.5
Comment 10 Build Bot 2016-04-01 01:16:58 PDT
Comment on attachment 275377 [details]
Patch

Attachment 275377 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1079656

New failing tests:
http/tests/security/anchor-download-allow-sameorigin.html
http/tests/security/anchor-download-allow-data.html
Comment 11 Build Bot 2016-04-01 01:17:01 PDT
Created attachment 275388 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 12 Brent Fulgham 2016-04-01 11:01:13 PDT
Created attachment 275419 [details]
Patch
Comment 13 Brent Fulgham 2016-04-01 11:12:09 PDT
Created attachment 275420 [details]
Patch
Comment 14 Brent Fulgham 2016-04-01 11:30:33 PDT
Created attachment 275423 [details]
Patch
Comment 15 Build Bot 2016-04-01 12:14:00 PDT
Comment on attachment 275423 [details]
Patch

Attachment 275423 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1082450

New failing tests:
fast/dom/HTMLAnchorElement/anchor-nodownload-set.html
fast/dom/HTMLAnchorElement/anchor-download.html
Comment 16 Build Bot 2016-04-01 12:14:03 PDT
Created attachment 275424 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 17 Brent Fulgham 2016-04-01 12:26:55 PDT
Created attachment 275425 [details]
Patch
Comment 18 Andy Estes 2016-04-01 13:00:50 PDT
Comment on attachment 275425 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=275425&action=review

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleNavigationAction.h:73
> +    bool m_hasDownloadAttribute;

Would it make sense to store the download attribute string instead? Do you foresee that ever being useful?

> Tools/WebKitTestRunner/TestController.cpp:1676
> +    String message = String::format("Download started.\n");

Why String::format()?

> Tools/WebKitTestRunner/TestController.cpp:1701
> +    String message = String::format("Download failed.\n");

Ditto.

> Tools/WebKitTestRunner/TestController.cpp:1723
> +    String message = String::format("Download cancelled.\n");

Ditto.

> Tools/WebKitTestRunner/InjectedBundle/TestRunner.h:70
> +    void waitUntilDownload();

waitUntilDownload() is a little ambiguous. I can't tell by reading this if it'll wait until the download starts or finishes. How about something like waitUntilDownloadDone(), waitUntilDownloadFinished(), or waitUntilDownloaded()?

I presume this name came from Blink, and changing it might make it harder to import their tests, but I think a good name is worth that inconvenience.
Comment 19 Brent Fulgham 2016-04-01 13:06:35 PDT
Comment on attachment 275425 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=275425&action=review

>> Tools/WebKitTestRunner/TestController.cpp:1676
>> +    String message = String::format("Download started.\n");
> 
> Why String::format()?

Yeah, that's dumb! I'll fix this (and the other places).

>> Tools/WebKitTestRunner/InjectedBundle/TestRunner.h:70
>> +    void waitUntilDownload();
> 
> waitUntilDownload() is a little ambiguous. I can't tell by reading this if it'll wait until the download starts or finishes. How about something like waitUntilDownloadDone(), waitUntilDownloadFinished(), or waitUntilDownloaded()?
> 
> I presume this name came from Blink, and changing it might make it harder to import their tests, but I think a good name is worth that inconvenience.

Agreed. I'll change it to "waitUntilDownloadFinished".

> Tools/WebKitTestRunner/InjectedBundle/TestRunner.h:294
> +

Whoops!
Comment 20 Brent Fulgham 2016-04-01 13:08:10 PDT
Comment on attachment 275425 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=275425&action=review

>> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleNavigationAction.h:73
>> +    bool m_hasDownloadAttribute;
> 
> Would it make sense to store the download attribute string instead? Do you foresee that ever being useful?

Sure. That allows a tri-state value (no 'download' attribute, 'download' without an argument, or 'download="abc"'), which might be of interest in tests. I was just trying to make the smallest possible change.
Comment 21 Brent Fulgham 2016-04-01 14:10:36 PDT
Committed r198955: <http://trac.webkit.org/changeset/198955>