Implement 'URLSearchParams'

Implement 'URLSearchParams'

This patch implements the 'URLSearchParams' interface (https://url.spec.whatwg.org/#interface-urlsearchparams), extracted
from Sigbjorn's excellent https://codereview.chromium.org/143313002/.
The bits of that patch which integrate 'URLSearchParams' with 'URL' and
'URLUtils' are dependent on Node moving to Oilpan, and will be added in
a subsequent patch.

Intent to Implement and Ship: https://groups.google.com/a/chromium.org/d/msg/blink-dev/grHZDbldP04/JdsoQ169AQAJ

BUG=303152
Committed: https://crrev.com/dff906eef78f37d1b44bb67785440366245ddb9d
Cr-Commit-Position: refs/heads/master@{#360086}

[Mike West, 2015-11-13 14:16:02]

mkwst@chromium.org changed reviewers:
+ jochen@chromium.org, philipj@opera.com, sigbjornf@opera.com

[Mike West, 2015-11-13 14:16:02]

Hi Jochen, Philip, and Sigbjorn!

Would you mind taking a look at this patch? Sigbjorn originally wrote it as part
of https://codereview.chromium.org/143313002/. Since that was a year and a half
ago, I suppose he qualifies as an unbiased reviewer of this extract. :)

I sent an I2S to blink-dev, so I won't be actually landing this until that
thread resolves. If folks want to wait on shipping, I'll add a runtime flag. If
not, I'll just do whatever you three tell me to do. :)

-mike

[sof, 2015-11-15 09:04:11]

https://codereview.chromium.org/1442643008/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/fast/domurl/resources/testharness-extras.js
(right):

https://codereview.chromium.org/1442643008/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/domurl/resources/testharness-extras.js:7:
function assert_throws_message(func, expected, name) {
This helper doesn't appear to be used, leave it out for now?

https://codereview.chromium.org/1442643008/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams-append.html
(right):

https://codereview.chromium.org/1442643008/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams-append.html:45:
assert_equals(params.get('third'), '', 'Search params object has name "second"
with value "2"');
Not your doing, but copy & paste gone wrong - could you sync the description?

https://codereview.chromium.org/1442643008/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams.html (right):

https://codereview.chromium.org/1442643008/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams.html:5: <link
rel="help" href="http://url.spec.whatwg.org/#interface-urlsearchparams">
Switch all of the test rel="help" to use https.

https://codereview.chromium.org/1442643008/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/URLSearchParams.cpp (right):

https://codereview.chromium.org/1442643008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/URLSearchParams.cpp:95: for
(Vector<std::pair<String, String>>::const_iterator it = m_params.begin(); it !=
m_params.end(); ++it) {
should we upgrade this one (and the ones below) to be range-based & more
compact?

https://codereview.chromium.org/1442643008/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/URLSearchParams.h (right):

https://codereview.chromium.org/1442643008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/URLSearchParams.h:37: virtual
~URLSearchParams();
Just noting -- ScriptWrappable doesn't have a destructor with Oilpan, so this
destructor won't have to be virtual once it is the default.

https://codereview.chromium.org/1442643008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/URLSearchParams.h:52: URLSearchParams(const
String&);
nit: put 'explicit' on these.

https://codereview.chromium.org/1442643008/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/URLSearchParams.idl (right):

https://codereview.chromium.org/1442643008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/URLSearchParams.idl:5: //
http://url.spec.whatwg.org/#interface-urlsearchparams
http => https

Including this spec reference in the commit message would be helpful too.

https://codereview.chromium.org/1442643008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/URLSearchParams.idl:11: GarbageCollected,
Should we expose URLSearchParams to workers here also?

[Mike West, 2015-11-15 11:45:04]

Description was changed from

==========
Implement 'URLSearchParams'

This patch implements the 'URLSearchParams' interface itself, extracted
from Sigbjorn's excellent https://codereview.chromium.org/143313002/.
The bits of that patch which integrate 'URLSearchParams' with 'URL' and
'URLUtils' are dependent on Node moving to Oilpan, and will be added in
a subsequent patch.

Intent to Implement and Ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/grHZDbldP04/JdsoQ169...

BUG=303152
==========

to

==========
Implement 'URLSearchParams'

This patch implements the 'URLSearchParams' interface
(https://url.spec.whatwg.org/#interface-urlsearchparams), extracted
from Sigbjorn's excellent https://codereview.chromium.org/143313002/.
The bits of that patch which integrate 'URLSearchParams' with 'URL' and
'URLUtils' are dependent on Node moving to Oilpan, and will be added in
a subsequent patch.

Intent to Implement and Ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/grHZDbldP04/JdsoQ169...

BUG=303152
==========

[Mike West, 2015-11-16 08:57:57]

Thanks for taking a look, Sigbjorn!

https://codereview.chromium.org/1442643008/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/fast/domurl/resources/testharness-extras.js
(right):

https://codereview.chromium.org/1442643008/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/domurl/resources/testharness-extras.js:7:
function assert_throws_message(func, expected, name) {
On 2015/11/15 at 09:04:11, sof wrote:
> This helper doesn't appear to be used, leave it out for now?

Yup.

https://codereview.chromium.org/1442643008/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams-append.html
(right):

https://codereview.chromium.org/1442643008/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams-append.html:45:
assert_equals(params.get('third'), '', 'Search params object has name "second"
with value "2"');
On 2015/11/15 at 09:04:11, sof wrote:
> Not your doing, but copy & paste gone wrong - could you sync the description?

Indeed!

https://codereview.chromium.org/1442643008/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams.html (right):

https://codereview.chromium.org/1442643008/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams.html:5: <link
rel="help" href="http://url.spec.whatwg.org/#interface-urlsearchparams">
On 2015/11/15 at 09:04:11, sof wrote:
> Switch all of the test rel="help" to use https.

Yup.

https://codereview.chromium.org/1442643008/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/URLSearchParams.cpp (right):

https://codereview.chromium.org/1442643008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/URLSearchParams.cpp:95: for
(Vector<std::pair<String, String>>::const_iterator it = m_params.begin(); it !=
m_params.end(); ++it) {
On 2015/11/15 at 09:04:11, sof wrote:
> should we upgrade this one (and the ones below) to be range-based & more
compact?

An excellent idea!

https://codereview.chromium.org/1442643008/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/URLSearchParams.h (right):

https://codereview.chromium.org/1442643008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/URLSearchParams.h:37: virtual
~URLSearchParams();
On 2015/11/15 at 09:04:11, sof wrote:
> Just noting -- ScriptWrappable doesn't have a destructor with Oilpan, so this
destructor won't have to be virtual once it is the default.

Added a comment.

https://codereview.chromium.org/1442643008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/URLSearchParams.h:52: URLSearchParams(const
String&);
On 2015/11/15 at 09:04:11, sof wrote:
> nit: put 'explicit' on these.

Done.

https://codereview.chromium.org/1442643008/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/URLSearchParams.idl (right):

https://codereview.chromium.org/1442643008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/URLSearchParams.idl:5: //
http://url.spec.whatwg.org/#interface-urlsearchparams
On 2015/11/15 at 09:04:11, sof wrote:
> http => https
> 
> Including this spec reference in the commit message would be helpful too.

*facepalm* Yes. I should have done both.

https://codereview.chromium.org/1442643008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/URLSearchParams.idl:11: GarbageCollected,
On 2015/11/15 at 09:04:11, sof wrote:
> Should we expose URLSearchParams to workers here also?

We should. Added a test.

[sof, 2015-11-16 10:11:53]

Expected output of various other tests that check what's provided by global
objects need to be updated.

The CL is in good shape & ready to go though, imo.

https://codereview.chromium.org/1442643008/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/URLSearchParams.idl (right):

https://codereview.chromium.org/1442643008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/URLSearchParams.idl:11: GarbageCollected,
On 2015/11/16 08:57:57, Mike West wrote:
> On 2015/11/15 at 09:04:11, sof wrote:
> > Should we expose URLSearchParams to workers here also?
> 
> We should. Added a test.

Nice, that's sufficient coverage.

https://codereview.chromium.org/1442643008/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/URLSearchParams.h (right):

https://codereview.chromium.org/1442643008/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/URLSearchParams.h:16: class DOMURLUtils;
nit: nop

[Mike West, 2015-11-16 10:34:23]

The CQ bit was checked by mkwst@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-16 10:35:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1442643008/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1442643008/40001

[Mike West, 2015-11-16 10:35:25]

On 2015/11/16 at 10:11:53, sigbjornf wrote:
> Expected output of various other tests that check what's provided by global
objects need to be updated.

Yes indeed. Sorry about that.

> The CL is in good shape & ready to go though, imo.

Thanks for taking the time to work through it!

Perhaps Philip or Jochen can both stamp this CL and the Intent thread, which I
think has one outstanding LGTMable slot. :)

[sof, 2015-11-16 11:35:40]

lgtm

[commit-bot: I haz the power, 2015-11-16 11:47:28]

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

[commit-bot: I haz the power, 2015-11-16 11:47:29]

Dry run: This issue passed the CQ dry run.

[philipj_slow, 2015-11-16 12:15:46]

Will also take a look at tests.

https://codereview.chromium.org/1442643008/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/URLSearchParams.idl (right):

https://codereview.chromium.org/1442643008/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/URLSearchParams.idl:8: Constructor(),
Does the IDL compiler not support the [Constructor(optional (USVString or
URLSearchParams) init = "")] syntax from the spec? If not, can you make sure
there's an issue block crbug.com/345506?

https://codereview.chromium.org/1442643008/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/URLSearchParams.idl:16:
[TreatReturnedNullStringAs=Null] USVString get(USVString name);
Can you use `USVString?` instead of `[TreatReturnedNullStringAs=Null]
USVString`?

https://codereview.chromium.org/1442643008/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/URLSearchParams.idl:20: 
Can you add a TODO for `iterable<USVString, USVString>`, or implement if it's
not blocked on something?

https://codereview.chromium.org/1442643008/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/URLSearchParams.cpp (right):

https://codereview.chromium.org/1442643008/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/URLSearchParams.cpp:37: m_params.clear();
Instead ASSERT that m_params is empty, until setInput is ever called from
outside the constructor?

https://codereview.chromium.org/1442643008/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/URLSearchParams.cpp:62: return
encodeWithURLEscapeSequences(input).replace("%20", "+");
Does this match https://url.spec.whatwg.org/#concept-urlencoded-byte-serializer
exactly?

[philipj_slow, 2015-11-16 12:47:09]

lgtm % nits

I wouldn't mind taking another look, but if I'm slow to respond please go ahead
and land :)

https://codereview.chromium.org/1442643008/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/domurl/resources/testharness-extras.js
(right):

https://codereview.chromium.org/1442643008/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/domurl/resources/testharness-extras.js:9:
function assert_syntax_error(f, msg) {
This is never used and assert_type_error() is only used in
urlsearchparams-constructor.html. I suggest inlining there or just using
assert_throw().

https://codereview.chromium.org/1442643008/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams-constructor.html
(right):

https://codereview.chromium.org/1442643008/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams-constructor.html:23:
assert_type_error(function () { new URLSearchParams(DOMException.prototype); });
A test for a string that isn't proper UTF-16 and throws an exception when
treated as USVString would be good.

https://codereview.chromium.org/1442643008/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams-constructor.html:25:
assert_true(params != null, 'constructor returned non-null value.');
assert_not_equals here and other != null tests.

https://codereview.chromium.org/1442643008/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams-constructor.html:54:
assert_equals(params.get('a'), 'b');
If you implement iterable<>, could this instead do an exhaustive test of
equality, like assert_array_equals?

https://codereview.chromium.org/1442643008/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams-constructor.html:70:
}, 'Parse +');
A test involving %2B would also be good, to ensure that the + handling and
percent decoding is in the right order.

https://codereview.chromium.org/1442643008/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams-has.html
(right):

https://codereview.chromium.org/1442643008/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams-has.html:21: }, 'Has
basics');
has() for consistency with below and other tests.

https://codereview.chromium.org/1442643008/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams-set.html
(right):

https://codereview.chromium.org/1442643008/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams-set.html:19: }, 'Set
basics');
set()

https://codereview.chromium.org/1442643008/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams-set.html:31: },
'URLSearchParams.set');
Something to distinguish this from above test, or merge tests?

https://codereview.chromium.org/1442643008/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams-stringifier.html
(right):

https://codereview.chromium.org/1442643008/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams-stringifier.html:81:
}, 'Serialize *-._');
Could you turn this into an exhaustive test for bytes 0x00-0xFF, with the
expectations on the same form as
https://url.spec.whatwg.org/#urlencoded-serializing ?

As it is, one has to dig into url_canon_internal.cc to check if it's per spec or
not, and I'm not sure without seeing a test :)

https://codereview.chromium.org/1442643008/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams-stringifier.html:119:
}, 'URLSearchParams.toString');
Seems like this is both testing that '=' survives a roundtrip and that
params.toString() and params+'' are equivalent? The latter seems like it could
be folded into the first test as assert_equals(params.toString(), 'a+b=c'), so
that this test can have a better description.

[Mike West, 2015-11-16 14:11:10]

The CQ bit was checked by mkwst@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-16 14:11:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1442643008/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1442643008/80001

[Mike West, 2015-11-16 14:12:11]

I see new comments! I'll address them while you skim the new code. :)

https://codereview.chromium.org/1442643008/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/URLSearchParams.idl (right):

https://codereview.chromium.org/1442643008/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/URLSearchParams.idl:8: Constructor(),
On 2015/11/16 at 12:15:46, philipj wrote:
> Does the IDL compiler not support the [Constructor(optional (USVString or
URLSearchParams) init = "")] syntax from the spec? If not, can you make sure
there's an issue block crbug.com/345506?

I didn't think it would work, but it looks like it might. Hooray!

https://codereview.chromium.org/1442643008/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/URLSearchParams.idl:16:
[TreatReturnedNullStringAs=Null] USVString get(USVString name);
On 2015/11/16 at 12:15:46, philipj wrote:
> Can you use `USVString?` instead of `[TreatReturnedNullStringAs=Null]
USVString`?

Sure.

https://codereview.chromium.org/1442643008/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/URLSearchParams.idl:20: 
On 2015/11/16 at 12:15:46, philipj wrote:
> Can you add a TODO for `iterable<USVString, USVString>`, or implement if it's
not blocked on something?

Sure. I even added a test!

https://codereview.chromium.org/1442643008/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/URLSearchParams.cpp (right):

https://codereview.chromium.org/1442643008/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/URLSearchParams.cpp:37: m_params.clear();
On 2015/11/16 at 12:15:46, philipj wrote:
> Instead ASSERT that m_params is empty, until setInput is ever called from
outside the constructor?

Sure. That won't happen until we integrate with URL (which can set the `query`).

https://codereview.chromium.org/1442643008/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/URLSearchParams.cpp:62: return
encodeWithURLEscapeSequences(input).replace("%20", "+");
On 2015/11/16 at 12:15:46, philipj wrote:
> Does this match
https://url.spec.whatwg.org/#concept-urlencoded-byte-serializer exactly?

1. It matches Firefox for all the tests in this CL (except iteration, which
Firefox doesn't currently implement).
2. If it doesn't, then more then URLSearchParams is broken. :)

[Mike West, 2015-11-16 14:15:56]

Patchset #4 (id:60001) has been deleted

[philipj_slow, 2015-11-16 14:17:33]

Off-topic: There's a bit of duplication in MediaFragmentURIParser. The spec
(currently unmaintained) would have to be updated, but it would be nice to
eventually merge these things.

https://codereview.chromium.org/1442643008/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/URLSearchParams.cpp (right):

https://codereview.chromium.org/1442643008/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/URLSearchParams.cpp:62: return
encodeWithURLEscapeSequences(input).replace("%20", "+");
On 2015/11/16 14:12:11, Mike West wrote:
> On 2015/11/16 at 12:15:46, philipj wrote:
> > Does this match
> https://url.spec.whatwg.org/#concept-urlencoded-byte-serializer exactly?
> 
> 1. It matches Firefox for all the tests in this CL (except iteration, which
> Firefox doesn't currently implement).
> 2. If it doesn't, then more then URLSearchParams is broken. :)

Right, it's a good thing that we're using shared code paths for both encoding
and decoding percent escapes, I just wouldn't be surprised to find some little
corner case that doesn't match. For toString() I made a suggestion about the
tests, but for parsing I think it would be a bit overkill to do exhaustive
testing.

[commit-bot: I haz the power, 2015-11-16 15:50:44]

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

[commit-bot: I haz the power, 2015-11-16 15:50:44]

Dry run: This issue passed the CQ dry run.

[Mike West, 2015-11-17 10:23:26]

WDYT, Philip?

https://codereview.chromium.org/1442643008/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/domurl/resources/testharness-extras.js
(right):

https://codereview.chromium.org/1442643008/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/domurl/resources/testharness-extras.js:9:
function assert_syntax_error(f, msg) {
On 2015/11/16 at 12:47:08, philipj wrote:
> This is never used and assert_type_error() is only used in
urlsearchparams-constructor.html. I suggest inlining there or just using
assert_throw().

ok.

https://codereview.chromium.org/1442643008/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams-constructor.html
(right):

https://codereview.chromium.org/1442643008/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams-constructor.html:25:
assert_true(params != null, 'constructor returned non-null value.');
On 2015/11/16 at 12:47:08, philipj wrote:
> assert_not_equals here and other != null tests.

Sure.

https://codereview.chromium.org/1442643008/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams-constructor.html:54:
assert_equals(params.get('a'), 'b');
On 2015/11/16 at 12:47:08, philipj wrote:
> If you implement iterable<>, could this instead do an exhaustive test of
equality, like assert_array_equals?

Suuuuure.

https://codereview.chromium.org/1442643008/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams-constructor.html:70:
}, 'Parse +');
On 2015/11/16 at 12:47:08, philipj wrote:
> A test involving %2B would also be good, to ensure that the + handling and
percent decoding is in the right order.

Done.

https://codereview.chromium.org/1442643008/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams-has.html
(right):

https://codereview.chromium.org/1442643008/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams-has.html:21: }, 'Has
basics');
On 2015/11/16 at 12:47:08, philipj wrote:
> has() for consistency with below and other tests.

Done.

https://codereview.chromium.org/1442643008/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams-set.html
(right):

https://codereview.chromium.org/1442643008/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams-set.html:19: }, 'Set
basics');
On 2015/11/16 at 12:47:08, philipj wrote:
> set()

Done.

https://codereview.chromium.org/1442643008/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams-set.html:31: },
'URLSearchParams.set');
On 2015/11/16 at 12:47:08, philipj wrote:
> Something to distinguish this from above test, or merge tests?

Distinguished.

https://codereview.chromium.org/1442643008/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams-stringifier.html
(right):

https://codereview.chromium.org/1442643008/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams-stringifier.html:81:
}, 'Serialize *-._');
On 2015/11/16 at 12:47:08, philipj wrote:
> Could you turn this into an exhaustive test for bytes 0x00-0xFF, with the
expectations on the same form as
https://url.spec.whatwg.org/#urlencoded-serializing ?
> 
> As it is, one has to dig into url_canon_internal.cc to check if it's per spec
or not, and I'm not sure without seeing a test :)

Done.

https://codereview.chromium.org/1442643008/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams-stringifier-expected.txt
(right):

https://codereview.chromium.org/1442643008/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams-stringifier-expected.txt:43:
'
Committing this with failing tests, because we're wrong and Firefox is right.

[philipj_slow, 2015-11-17 11:00:17]

lgtm % minor nits

https://codereview.chromium.org/1442643008/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams-stringifier-expected.txt
(right):

https://codereview.chromium.org/1442643008/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams-stringifier-expected.txt:43:
'
On 2015/11/17 10:23:26, Mike West wrote:
> Committing this with failing tests, because we're wrong and Firefox is right.

Excellent, testing pays off :)

https://codereview.chromium.org/1442643008/diff/140001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams-stringifier.html
(right):

https://codereview.chromium.org/1442643008/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams-stringifier.html:101:
}, "Serialize 0x" + intToHex(i) + " -> '" + String.fromCodePoint(i) + "'");
Can you say U+00AA instead of 0xAA to clarify that this isn't really testing
bytes? For the same reason, can you also say that not all bytes can appear in
valid UTF-8, and therefore some bytes are not covered? Fortunately, those bytes
are all >=0xC0, so they all fall into the "otherwise" bucket. (You could hit
more of them by going beyond U+00FF, but I don't think that's necessary.)

https://codereview.chromium.org/1442643008/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/URLSearchParams.cpp (right):

https://codereview.chromium.org/1442643008/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/URLSearchParams.cpp:20: bool
next(ScriptState* scriptState, String& key, String& value, ExceptionState&
exception) override
Maybe drop the scriptState and exception names to show that they are unused?

https://codereview.chromium.org/1442643008/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/URLSearchParams.cpp:32:
Vector<std::pair<String, String>> m_params;
If URLSearchParamsIterationSource were put on the Oilpan heap I suppose that the
vector wouldn't need to be copied, but I suppose that would impact other
IterationSources as well. Nothing to do unless someone has a good idea.

[Mike West, 2015-11-17 12:39:35]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2015-11-17 12:39:37]

Thanks!

https://codereview.chromium.org/1442643008/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams-stringifier-expected.txt
(right):

https://codereview.chromium.org/1442643008/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/domurl/urlsearchparams-stringifier-expected.txt:43:
'
On 2015/11/17 at 11:00:17, philipj wrote:
> On 2015/11/17 10:23:26, Mike West wrote:
> > Committing this with failing tests, because we're wrong and Firefox is
right.
> 
> Excellent, testing pays off :)

Well, "pays off". I was happier before we knew we had bugs. :)

https://codereview.chromium.org/1442643008/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/URLSearchParams.cpp (right):

https://codereview.chromium.org/1442643008/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/URLSearchParams.cpp:20: bool
next(ScriptState* scriptState, String& key, String& value, ExceptionState&
exception) override
On 2015/11/17 at 11:00:17, philipj wrote:
> Maybe drop the scriptState and exception names to show that they are unused?

Done.

https://codereview.chromium.org/1442643008/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/URLSearchParams.cpp:32:
Vector<std::pair<String, String>> m_params;
On 2015/11/17 at 11:00:17, philipj wrote:
> If URLSearchParamsIterationSource were put on the Oilpan heap I suppose that
the vector wouldn't need to be copied, but I suppose that would impact other
IterationSources as well. Nothing to do unless someone has a good idea.

I was surprised I had to write any of this, actually. It seems like the basics
should be provided already. :/

[Mike West, 2015-11-17 12:39:37]

The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com,
philipj@opera.com
Link to the patchset: https://codereview.chromium.org/1442643008/#ps160001
(title: "Nits.")

[commit-bot: I haz the power, 2015-11-17 12:40:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1442643008/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1442643008/160001

[commit-bot: I haz the power, 2015-11-17 13:40:37]

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

[commit-bot: I haz the power, 2015-11-17 13:40:38]

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

[Mike West, 2015-11-17 14:09:52]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2015-11-17 14:09:52]

The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com,
philipj@opera.com
Link to the patchset: https://codereview.chromium.org/1442643008/#ps180001
(title: "Test.")

[commit-bot: I haz the power, 2015-11-17 14:10:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1442643008/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1442643008/180001

[commit-bot: I haz the power, 2015-11-17 16:24:34]

Committed patchset #9 (id:180001)

[commit-bot: I haz the power, 2015-11-17 16:25:30]

Patchset 9 (id:??) landed as
https://crrev.com/dff906eef78f37d1b44bb67785440366245ddb9d
Cr-Commit-Position: refs/heads/master@{#360086}