Fetch: Make Headers' constructor match the current spec IDL.

Fetch: Make Headers' constructor match the current spec IDL.

Instead of specifying several different constructors in the IDL (including
an overload for the defunct OpenEndedDictionary type), make Headers.idl
match the Fetch spec and just declare a single constructor that optionally
takes a HeadersInit type, which has also been modified to take a
record<ByteString,ByteString> instead of a Dictionary now that we support
records in the WebIDL compiler.

By using HeadersInit, which we define in the same IDL, we can get rid of
RequestInit::headers_dictionary altogether and leave most of the V8-Impl
type conversions to the auto-generated bindings code. Consequently, this
also lets us remove several Header::Create() overloads.

BUG=707365
R=tyoshino@chromium.org,yhirano@chromium.org

Review-Url: https://codereview.chromium.org/2691513002
Cr-Commit-Position: refs/heads/master@{#463623}
Committed: https://chromium.googlesource.com/chromium/src/+/07c092298a6243fc2c829e517cf2b19f4caea26c

[Raphael Kubo da Costa (rakuco), 2017-02-10 13:30:43]

PTAL. This is a simplification of existing code and I don't expect any behavior
changes, which is why I'm not adding any new tests.

[joecool, 2017-02-10 23:39:55]

Description was changed from

==========
Fetch: Make Headers' constructor match the current spec IDL.

Instead of specifying several different constructors in the IDL (including
an overload for the defunct OpenEndedDictionary type), make Headers.idl
match the Fetch spec and just declare a single constructor that optionally
takes a HeadersInit type.

By using HeadersInit, which we define in the same IDL, we can get rid of
RequestInit::headersDictionary altogether and leave most of the V8-Impl type
conversions to the auto-generated bindings code. Consequently, this also
lets us remove several Header::create() overloads, as we now only need to
worry about ByteStringSequenceSequenceOrDictionaryOrHeaders being passed.

R=tyoshino@chromium.org,yhirano@chromium.org
==========

to

==========
Fetch: Make Headers' constructor match the current spec IDL.

Instead of specifying several different constructors in the IDL (including
an overload for the defunct OpenEndedDictionary type), make Headers.idl
match the Fetch spec and just declare a single constructor that optionally
takes a HeadersInit type.

By using HeadersInit, which we define in the same IDL, we can get rid of
RequestInit::headersDictionary altogether and leave most of the V8-Impl type
conversions to the auto-generated bindings code. Consequently, this also
lets us remove several Header::create() overloads, as we now only need to
worry about ByteStringSequenceSequenceOrDictionaryOrHeaders being passed.

R=tyoshino@chromium.org,yhirano@chromium.org
==========

[yhirano, 2017-02-13 05:56:46]

I think this CL changes the behavior.

r = new Request('/', {headers: {'hoge': 'fuga'}});
rr = new Request(r, {headers: undefined})

Without this CL rr.headers has 'hoge', but with this CL it doesn't.

[yhirano, 2017-02-13 05:56:51]

https://codereview.chromium.org/2691513002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/fetch/Headers.cpp (right):

https://codereview.chromium.org/2691513002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/fetch/Headers.cpp:282: // FIXME: This method's
signature must change once we support record<K,V> in
TODO(rakuco):

[Raphael Kubo da Costa (rakuco), 2017-02-13 14:12:20]

Thanks for the review; patch v2 is up.

I now make some additional checks for IsEmpty() and IsUndefined() when parsing
"headers" in RequestInit -- this should all be unnecessary once the FIXME to
move this to an actual .IDL file is resolved.

I also added a new test for the behavior you reported; I did it in
http/tests/fetch, as we currently skip the Fetch tests from W3C's
web-platform-tests.

[yhirano, 2017-02-14 03:28:50]

https://codereview.chromium.org/2691513002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/Headers.cpp (right):

https://codereview.chromium.org/2691513002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/Headers.cpp:76: }
else {
  NOTREACHED();
}

https://codereview.chromium.org/2691513002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/RequestInit.cpp (right):

https://codereview.chromium.org/2691513002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/RequestInit.cpp:37: !v8Headers.IsEmpty()
&& !v8Headers->IsUndefined();
The same applies for null, I believe.

[Raphael Kubo da Costa (rakuco), 2017-04-07 12:16:21]

Description was changed from

==========
Fetch: Make Headers' constructor match the current spec IDL.

Instead of specifying several different constructors in the IDL (including
an overload for the defunct OpenEndedDictionary type), make Headers.idl
match the Fetch spec and just declare a single constructor that optionally
takes a HeadersInit type.

By using HeadersInit, which we define in the same IDL, we can get rid of
RequestInit::headersDictionary altogether and leave most of the V8-Impl type
conversions to the auto-generated bindings code. Consequently, this also
lets us remove several Header::create() overloads, as we now only need to
worry about ByteStringSequenceSequenceOrDictionaryOrHeaders being passed.

R=tyoshino@chromium.org,yhirano@chromium.org
==========

to

==========
Fetch: Make Headers' constructor match the current spec IDL.

Instead of specifying several different constructors in the IDL (including
an overload for the defunct OpenEndedDictionary type), make Headers.idl
match the Fetch spec and just declare a single constructor that optionally
takes a HeadersInit type, which has also been modified to take a
record<ByteString,ByteString> instead of a Dictionary now that we support
records in the WebIDL compiler.

By using HeadersInit, which we define in the same IDL, we can get rid of
RequestInit::headersDictionary altogether and leave most of the V8-Impl type
conversions to the auto-generated bindings code. Consequently, this also
lets us remove several Header::create() overloads.

BUG=707365
R=tyoshino@chromium.org,yhirano@chromium.org
==========

[Raphael Kubo da Costa (rakuco), 2017-04-07 12:24:56]

Sorry for the delay. I ended up taking a long detour that involved adding
support for record<> types in our WebIDL compiler and a few other things :-)

Patch v3 is up. It has been rebased and now HeadersInit takes a
record<ByteString, ByteString> instead of a Dictionary.

The most controversial part of the CL is probably the change to RequestInit.cpp
and how we determine if |header| was set. I am now following the consensus from
https://github.com/whatwg/fetch/issues/479 to determine if |header| was set or
not. In summary:
- undefined means "RequestInit's header member was not set".
- {} means "RequestInit's header member has been set, and the list of headers is
empty".
- null means "RequestInit's header member has been set to an invalid value, and
a TypeError will be thrown when it is converted to a HeadersInit".

[Raphael Kubo da Costa (rakuco), 2017-04-07 12:26:38]

The CQ bit was checked by raphael.kubo.da.costa@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 12:26:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 13:56:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-07 13:56:10]

Dry run: This issue passed the CQ dry run.

[Raphael Kubo da Costa (rakuco), 2017-04-10 11:11:12]

Description was changed from

==========
Fetch: Make Headers' constructor match the current spec IDL.

Instead of specifying several different constructors in the IDL (including
an overload for the defunct OpenEndedDictionary type), make Headers.idl
match the Fetch spec and just declare a single constructor that optionally
takes a HeadersInit type, which has also been modified to take a
record<ByteString,ByteString> instead of a Dictionary now that we support
records in the WebIDL compiler.

By using HeadersInit, which we define in the same IDL, we can get rid of
RequestInit::headersDictionary altogether and leave most of the V8-Impl type
conversions to the auto-generated bindings code. Consequently, this also
lets us remove several Header::create() overloads.

BUG=707365
R=tyoshino@chromium.org,yhirano@chromium.org
==========

to

==========
Fetch: Make Headers' constructor match the current spec IDL.

Instead of specifying several different constructors in the IDL (including
an overload for the defunct OpenEndedDictionary type), make Headers.idl
match the Fetch spec and just declare a single constructor that optionally
takes a HeadersInit type, which has also been modified to take a
record<ByteString,ByteString> instead of a Dictionary now that we support
records in the WebIDL compiler.

By using HeadersInit, which we define in the same IDL, we can get rid of
RequestInit::headers_dictionary altogether and leave most of the V8-Impl
type conversions to the auto-generated bindings code. Consequently, this
also lets us remove several Header::Create() overloads.

BUG=707365
R=tyoshino@chromium.org,yhirano@chromium.org
==========

[Raphael Kubo da Costa (rakuco), 2017-04-10 12:22:41]

Patch rebased after the Blink rename that happened last weekend.

[Raphael Kubo da Costa (rakuco), 2017-04-10 12:22:46]

The CQ bit was checked by raphael.kubo.da.costa@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-10 12:23:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-10 13:00:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-10 13:00:13]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[yhirano, 2017-04-11 03:32:41]

lgtm

https://codereview.chromium.org/2691513002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/Headers.cpp (right):

https://codereview.chromium.org/2691513002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/Headers.cpp:7: #include
"bindings/core/v8/Dictionary.h"
Do we still need this?

https://codereview.chromium.org/2691513002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/RequestInit.h (right):

https://codereview.chromium.org/2691513002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/RequestInit.h:8: #include
"bindings/core/v8/Dictionary.h"
Not needed.

[Raphael Kubo da Costa (rakuco), 2017-04-11 08:02:21]

Thanks for the review; I've removed those unnecessary includes in patch v6,
which I'm going to land.

https://codereview.chromium.org/2691513002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/Headers.cpp (right):

https://codereview.chromium.org/2691513002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/Headers.cpp:7: #include
"bindings/core/v8/Dictionary.h"
On 2017/04/11 03:32:41, yhirano wrote:
> Do we still need this?

Nope; removed in patch v6.

https://codereview.chromium.org/2691513002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/RequestInit.h (right):

https://codereview.chromium.org/2691513002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/RequestInit.h:8: #include
"bindings/core/v8/Dictionary.h"
On 2017/04/11 03:32:41, yhirano wrote:
> Not needed.

Done.

[Raphael Kubo da Costa (rakuco), 2017-04-11 08:02:28]

The CQ bit was checked by raphael.kubo.da.costa@intel.com

[Raphael Kubo da Costa (rakuco), 2017-04-11 08:02:29]

The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/2691513002/#ps100001
(title: "Remove unnecessary includes")

[commit-bot: I haz the power, 2017-04-11 08:03:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 09:22:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 09:22:51]

Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Raphael Kubo da Costa (rakuco), 2017-04-11 09:32:59]

The CQ bit was checked by raphael.kubo.da.costa@intel.com

[commit-bot: I haz the power, 2017-04-11 09:33:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 09:37:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 09:37:59]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Raphael Kubo da Costa (rakuco), 2017-04-11 12:18:26]

The CQ bit was checked by raphael.kubo.da.costa@intel.com

[Raphael Kubo da Costa (rakuco), 2017-04-11 12:18:27]

The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/2691513002/#ps120001
(title: "Rebase again")

[commit-bot: I haz the power, 2017-04-11 12:18:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 14:35:11]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1491913106713850,
"parent_rev": "dc97d9498faa6f7e040b8556ddd37c8cc2e00c5d", "commit_rev":
"07c092298a6243fc2c829e517cf2b19f4caea26c"}

[commit-bot: I haz the power, 2017-04-11 14:36:03]

Description was changed from

==========
Fetch: Make Headers' constructor match the current spec IDL.

Instead of specifying several different constructors in the IDL (including
an overload for the defunct OpenEndedDictionary type), make Headers.idl
match the Fetch spec and just declare a single constructor that optionally
takes a HeadersInit type, which has also been modified to take a
record<ByteString,ByteString> instead of a Dictionary now that we support
records in the WebIDL compiler.

By using HeadersInit, which we define in the same IDL, we can get rid of
RequestInit::headers_dictionary altogether and leave most of the V8-Impl
type conversions to the auto-generated bindings code. Consequently, this
also lets us remove several Header::Create() overloads.

BUG=707365
R=tyoshino@chromium.org,yhirano@chromium.org
==========

to

==========
Fetch: Make Headers' constructor match the current spec IDL.

Instead of specifying several different constructors in the IDL (including
an overload for the defunct OpenEndedDictionary type), make Headers.idl
match the Fetch spec and just declare a single constructor that optionally
takes a HeadersInit type, which has also been modified to take a
record<ByteString,ByteString> instead of a Dictionary now that we support
records in the WebIDL compiler.

By using HeadersInit, which we define in the same IDL, we can get rid of
RequestInit::headers_dictionary altogether and leave most of the V8-Impl
type conversions to the auto-generated bindings code. Consequently, this
also lets us remove several Header::Create() overloads.

BUG=707365
R=tyoshino@chromium.org,yhirano@chromium.org

Review-Url: https://codereview.chromium.org/2691513002
Cr-Commit-Position: refs/heads/master@{#463623}
Committed:
https://chromium.googlesource.com/chromium/src/+/07c092298a6243fc2c829e517cf2...
==========

[commit-bot: I haz the power, 2017-04-11 14:36:04]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/07c092298a6243fc2c829e517cf2...