Bug 159672 – [Fetch API] Request should be created with any HeadersInit data

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

Currently, it only supports Headers objects

[youenn fablet]

Created attachment 283403 [details]
Patch

[Build Bot]

Comment on attachment 283403 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/fetch/api/credentials/authentication-basic.html

[Build Bot]

Created attachment 283406 [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

[youenn fablet]

Created attachment 283407 [details]
Removing WK1 expectation

[Alex Christensen]

Comment on attachment 283407 [details]
Removing WK1 expectation

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

> Source/WebCore/bindings/js/JSDictionary.cpp:259
> +        JSValue headersConstructor = JSFetchHeaders::getConstructor(exec->vm(), jsCast<JSDOMGlobalObject*>(exec->lexicalGlobalObject()));

Is it possible to do anything tricky with the prototype to make this give us the wrong constructor?  Is that correct?

[youenn fablet]

(In reply to comment #5)
> Comment on attachment 283407 [details]
> Removing WK1 expectation
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283407&action=review
> 
> > Source/WebCore/bindings/js/JSDictionary.cpp:259
> > +        JSValue headersConstructor = JSFetchHeaders::getConstructor(exec->vm(), jsCast<JSDOMGlobalObject*>(exec->lexicalGlobalObject()));
> 
> Is it possible to do anything tricky with the prototype to make this give us
> the wrong constructor?  Is that correct?

I am not sure to follow.

This code is what is used to create a Headers constructor object when calling window.Header or window.@Header. Or when accessing to the constructor from Headers prototype.

The constructor object is not created upfront but only when being accessed the first time. I don't think user script can modify the related JSGlobalObject map to add wrong entries.

User script may modify window prototype, override window.Header or change Header.prototype, add new properties to Header constructor.
But these lines should remain functional.

[youenn fablet]

Ping review?

[Chris Dumez]

Comment on attachment 283407 [details]
Removing WK1 expectation

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

>>> Source/WebCore/bindings/js/JSDictionary.cpp:259
>>> +        JSValue headersConstructor = JSFetchHeaders::getConstructor(exec->vm(), jsCast<JSDOMGlobalObject*>(exec->lexicalGlobalObject()));
>> 
>> Is it possible to do anything tricky with the prototype to make this give us the wrong constructor?  Is that correct?
> 
> I am not sure to follow.
> 
> This code is what is used to create a Headers constructor object when calling window.Header or window.@Header. Or when accessing to the constructor from Headers prototype.
> 
> The constructor object is not created upfront but only when being accessed the first time. I don't think user script can modify the related JSGlobalObject map to add wrong entries.
> 
> User script may modify window prototype, override window.Header or change Header.prototype, add new properties to Header constructor.
> But these lines should remain functional.

It seems wasteful to construct a temporary JSFetchHeaders object. Can we construct a FetchHeaders directly from he JSValue without going through a JSFetchHeaders?

[youenn fablet]

Comment on attachment 283407 [details]
Removing WK1 expectation

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

>>>> Source/WebCore/bindings/js/JSDictionary.cpp:259
>>>> +        JSValue headersConstructor = JSFetchHeaders::getConstructor(exec->vm(), jsCast<JSDOMGlobalObject*>(exec->lexicalGlobalObject()));
>>> 
>>> Is it possible to do anything tricky with the prototype to make this give us the wrong constructor?  Is that correct?
>> 
>> I am not sure to follow.
>> 
>> This code is what is used to create a Headers constructor object when calling window.Header or window.@Header. Or when accessing to the constructor from Headers prototype.
>> 
>> The constructor object is not created upfront but only when being accessed the first time. I don't think user script can modify the related JSGlobalObject map to add wrong entries.
>> 
>> User script may modify window prototype, override window.Header or change Header.prototype, add new properties to Header constructor.
>> But these lines should remain functional.
> 
> It seems wasteful to construct a temporary JSFetchHeaders object. Can we construct a FetchHeaders directly from he JSValue without going through a JSFetchHeaders?

OK, let's create a JS-builtin that will implement https://fetch.spec.whatwg.org/#concept-headers-fill

[youenn fablet]

Created attachment 284096 [details]
Patch

[youenn fablet]

Created attachment 284099 [details]
Patch

[youenn fablet]

Created attachment 284327 [details]
Rebasing

[youenn fablet]

Created attachment 284329 [details]
Fixing CMake build

[Sam Weinig]

Comment on attachment 284329 [details]
Fixing CMake build

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

> Source/WebCore/Modules/fetch/WorkerGlobalScopeFetch.idl:33
> +    [JSBuiltin] Promise fetch(any input, optional Dictionary init);

Is there anyway to take advantage of our WebIDL Dictionary support for init?

[youenn fablet]

Comment on attachment 284329 [details]
Fixing CMake build

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

>> Source/WebCore/Modules/fetch/WorkerGlobalScopeFetch.idl:33
>> +    [JSBuiltin] Promise fetch(any input, optional Dictionary init);
> 
> Is there anyway to take advantage of our WebIDL Dictionary support for init?

The init dictionary is passed to FetchRequest constructor, which was and still is heavily based on using Dictionary (see FetchRequest::initializeWith).
That said, moving part of this initializeWith code in FetchRequest JS built-in might ease maintenance and readability.

[youenn fablet]

Created attachment 284408 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 284408 [details]
Patch for landing

Clearing flags on attachment: 284408

Committed r203641: <http://trac.webkit.org/changeset/203641>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 160116

[WebKit Commit Bot]

Comment on attachment 284408 [details]
Patch for landing

Rejecting attachment 284408 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 284408, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
0000000000000000
|--- LayoutTests/platform/mac-wk1/imported/w3c/web-platform-tests/fetch/api/credentials/authentication-basic-expected.txt	(revision 0)
|+++ LayoutTests/platform/mac-wk1/imported/w3c/web-platform-tests/fetch/api/credentials/authentication-basic-expected.txt	(working copy)
--------------------------
No file to patch.  Skipping patch.
1 out of 1 hunk ignored

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/1738806

[WebKit Commit Bot]

Comment on attachment 284408 [details]
Patch for landing

Clearing flags on attachment: 284408

Committed r203675: <http://trac.webkit.org/changeset/203675>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.