Bug 174974 – [WebIDL] Remove JS builtin bindings for FetchRequest, DOMWindowFetch and WorkerGlobalScopeFetch

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

[WebIDL] Remove JS builtin bindings for FetchRequest, DOMWindowFetch and WorkerGlobalScopeFetch

[Sam Weinig]

Created attachment 316740 [details]
Patch

[Sam Weinig]

Created attachment 316742 [details]
Patch

[Darin Adler]

Comment on attachment 316742 [details]
Patch

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

> Source/WebCore/Modules/fetch/FetchHeaders.h:90
> -    void setGuard(Guard);
> +    void setGuard(Guard guard)
> +    {
> +        ASSERT(!m_headers.size());
> +        m_guard = guard;
> +    }

I think this undoes something I did intentionally here. When bodies get to be non-trivial I take them out of the class definition so the class definition itself is easier to read. We should settle this as a style question so you and I don’t just keep doing and undoing this every time we edit the same file.

> Source/WebCore/Modules/fetch/FetchHeaders.h:93
>      FetchHeaders(Guard guard, HTTPHeaderMap&& headers = { })

Should this be explicit? We don’t want to convert from a Guard to a FetchHeaders implicitly, do we?

> Source/WebCore/Modules/fetch/FetchHeaders.h:103
> +    FetchHeaders(const FetchHeaders& other)
> +        : m_guard(other.m_guard)
> +        , m_headers(other.m_headers)
> +    {
> +    }

Same comment as for setGuard above, but also, why does the copy constructor need to be private?

If it does need to be private we should consider just writing "= default" instead of writing the function out. And we should consider also defining the move constructor, because we are getting rid of it as a side effect of defining the copy constructor. And we should also delete the copy and move assignment operators or make them private.

Or we can decide none of these things need to be private, delete this, and let everything take care of itself.

[Build Bot]

Comment on attachment 316742 [details]
Patch

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

New failing tests:
http/tests/inspector/network/fetch-network-data.html
imported/w3c/web-platform-tests/fetch/api/basic/scheme-blob.html
imported/w3c/web-platform-tests/fetch/api/basic/scheme-about.any.html
imported/w3c/web-platform-tests/fetch/api/basic/mode-same-origin.any.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-cookies.any.html
imported/w3c/web-platform-tests/fetch/api/policies/referrer-origin-when-cross-origin.html
imported/w3c/web-platform-tests/fetch/api/request/request-bad-port.html
imported/w3c/web-platform-tests/fetch/api/basic/scheme-others.html

[Build Bot]

Created attachment 316744 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 316742 [details]
Patch

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

New failing tests:
http/tests/inspector/network/fetch-network-data.html
imported/w3c/web-platform-tests/fetch/api/basic/scheme-blob.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-cookies.any.html
imported/w3c/web-platform-tests/fetch/api/basic/mode-same-origin.any.html
imported/w3c/web-platform-tests/fetch/api/basic/scheme-about.any.html
imported/w3c/web-platform-tests/fetch/api/policies/referrer-origin-when-cross-origin.html
imported/w3c/web-platform-tests/fetch/api/request/request-bad-port.html
imported/w3c/web-platform-tests/fetch/api/basic/scheme-others.html

[Build Bot]

Created attachment 316745 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 316742 [details]
Patch

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

New failing tests:
http/tests/inspector/network/fetch-network-data.html
imported/w3c/web-platform-tests/fetch/api/basic/scheme-blob.html
imported/w3c/web-platform-tests/fetch/api/basic/scheme-about.any.html
imported/w3c/web-platform-tests/fetch/api/basic/mode-same-origin.any.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-cookies.any.html
imported/w3c/web-platform-tests/fetch/api/policies/referrer-origin-when-cross-origin.html
imported/w3c/web-platform-tests/fetch/api/request/request-bad-port.html
imported/w3c/web-platform-tests/fetch/api/basic/scheme-others.html

[Build Bot]

Created attachment 316746 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 316742 [details]
Patch

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

New failing tests:
fast/images/low-memory-decode.html
imported/w3c/web-platform-tests/fetch/api/basic/scheme-blob.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-cookies.any.html
imported/w3c/web-platform-tests/fetch/api/basic/mode-same-origin.any.html
imported/w3c/web-platform-tests/fetch/api/basic/scheme-about.any.html
imported/w3c/web-platform-tests/fetch/api/policies/referrer-origin-when-cross-origin.html
imported/w3c/web-platform-tests/fetch/api/request/request-bad-port.html
imported/w3c/web-platform-tests/fetch/api/basic/scheme-others.html

[Build Bot]

Created attachment 316747 [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.12.5

[Sam Weinig]

(In reply to Darin Adler from comment #3)
> Comment on attachment 316742 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=316742&action=review
> 
> If it does need to be private we should consider just writing "= default"
> instead of writing the function out. And we should consider also defining
> the move constructor, because we are getting rid of it as a side effect of
> defining the copy constructor. And we should also delete the copy and move
> assignment operators or make them private.
> 
> Or we can decide none of these things need to be private, delete this, and
> let everything take care of itself.

The whole thing is a bit weird. The class is RefCounted, which makes it WTF_MAKE_NONCOPYABLE. So, just not declaring the copy-constructor is out. The copy constructor itself is only called from one place, the static create function that takes  a FetchHeaders. I think it's fine to leave as is for now. In the future, maybe make a static copy/clone function to make it clearer.

[Sam Weinig]

Created attachment 316749 [details]
For landing

[Darin Adler]

Comment on attachment 316749 [details]
For landing

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

> LayoutTests/ChangeLog:57
> [phone]  Sam Weinig  <[e-mail]>

Double change log entry.

> LayoutTests/imported/w3c/ChangeLog:17
> [phone]  Sam Weinig  <[e-mail]>

Double change log entry.

[Sam Weinig]

Committed r220050: <http://trac.webkit.org/changeset/220050>

[Radar WebKit Bug Importer]

<rdar://problem/33614806>

[Ms2ger (he/him; ⌚ UTC+1/+2)]

Re-opened since this is blocked by bug 175017

[Ms2ger (he/him; ⌚ UTC+1/+2)]

Sorry about that.