[webkit-reviews] review granted: [Bug 174974] [WebIDL] Remove JS builtin bindings for FetchRequest, DOMWindowFetch and WorkerGlobalScopeFetch : [Attachment 316742] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 174974: [WebIDL] Remove JS builtin bindings for FetchRequest,
DOMWindowFetch and WorkerGlobalScopeFetch
https://bugs.webkit.org/show_bug.cgi?id=174974

Attachment 316742: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 316742
  --> https://bugs.webkit.org/attachment.cgi?id=316742
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.