Bug 174974 - [WebIDL] Remove JS builtin bindings for FetchRequest, DOMWindowFetch and WorkerGlobalScopeFetch
Summary: [WebIDL] Remove JS builtin bindings for FetchRequest, DOMWindowFetch and Work...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar
Depends on: 175017
Blocks:
  Show dependency tree / graph
 
Reported: 2017-07-30 10:01 PDT by Sam Weinig
Modified: 2017-08-01 05:30 PDT (History)
6 users (show)

See Also:

Attachments
Patch (40.42 KB, patch)
2017-07-30 10:03 PDT, Sam Weinig 		no flags Details | Formatted Diff | Diff
Patch (40.98 KB, patch)
2017-07-30 10:15 PDT, Sam Weinig 		darin: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-elcapitan (1.03 MB, application/zip)
2017-07-30 11:23 PDT, Build Bot 		no flags Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (1.13 MB, application/zip)
2017-07-30 11:26 PDT, Build Bot 		no flags Details
Archive of layout-test-results from ews115 for mac-elcapitan (1.83 MB, application/zip)
2017-07-30 11:53 PDT, Build Bot 		no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (1.19 MB, application/zip)
2017-07-30 11:53 PDT, Build Bot 		no flags Details
For landing (71.87 KB, patch)
2017-07-30 12:26 PDT, Sam Weinig 		no flags Details | Formatted Diff | Diff
Show Obsolete (5) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2017-07-30 10:01:58 PDT 
[WebIDL] Remove JS builtin bindings for FetchRequest, DOMWindowFetch and WorkerGlobalScopeFetch
Comment 1 Sam Weinig 2017-07-30 10:03:26 PDT 
Created attachment 316740 [details]
Patch
Comment 2 Sam Weinig 2017-07-30 10:15:35 PDT 
Created attachment 316742 [details]
Patch
Comment 3 Darin Adler 2017-07-30 10:38:09 PDT 
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.
Comment 4 Build Bot 2017-07-30 11:23:45 PDT Comment hidden (obsolete)
Comment 5 Build Bot 2017-07-30 11:23:46 PDT Comment hidden (obsolete)
Comment 6 Build Bot 2017-07-30 11:26:48 PDT Comment hidden (obsolete)
Comment 7 Build Bot 2017-07-30 11:26:49 PDT Comment hidden (obsolete)
Comment 8 Build Bot 2017-07-30 11:53:35 PDT Comment hidden (obsolete)
Comment 9 Build Bot 2017-07-30 11:53:36 PDT Comment hidden (obsolete)
Comment 10 Build Bot 2017-07-30 11:53:48 PDT Comment hidden (obsolete)
Comment 11 Build Bot 2017-07-30 11:53:49 PDT Comment hidden (obsolete)
Comment 12 Sam Weinig 2017-07-30 12:25:06 PDT 
(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.
Comment 13 Sam Weinig 2017-07-30 12:26:06 PDT 
Created attachment 316749 [details]
For landing
Comment 14 Darin Adler 2017-07-30 12:55:42 PDT 
Comment on attachment 316749 [details]
For landing

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

> LayoutTests/ChangeLog:57
> +2017-07-28  Sam Weinig  <sam@webkit.org>

Double change log entry.

> LayoutTests/imported/w3c/ChangeLog:17
> +2017-07-28  Sam Weinig  <sam@webkit.org>

Double change log entry.
Comment 15 Sam Weinig 2017-07-30 13:27:21 PDT 
Committed r220050: <http://trac.webkit.org/changeset/220050>
Comment 16 Radar WebKit Bug Importer 2017-07-30 13:28:30 PDT 
<rdar://problem/33614806>
Comment 17 Ms2ger (he/him; ⌚ UTC+1/+2) 2017-08-01 05:29:00 PDT 
Re-opened since this is blocked by bug 175017
Comment 18 Ms2ger (he/him; ⌚ UTC+1/+2) 2017-08-01 05:30:20 PDT 
Sorry about that.