Add support for URL.searchParams getter.

Add support for URL.searchParams getter.

Add the missing piece to our URLSearchParams implementation;
the readonly attribute for URL, URL.searchParams:

 https://url.spec.whatwg.org/#dom-url-searchparams

The currently spec'ed connection between URL and URLSearchParams is
a lot less general than previous designs, hence the object lifetime
complexities it ran into (see https://codereview.chromium.org/143313002/)
falls away.

Intent to Implement and Ship (for URLSearchParams and this URL attribute):

 https://groups.google.com/a/chromium.org/d/msg/blink-dev/grHZDbldP04/JdsoQ169AQAJ

R=mkwst
BUG=303152
Committed: https://crrev.com/181f15d385bd66883ed39a58899a25d04838e2fe
Cr-Commit-Position: refs/heads/master@{#386189}

[sof, 2016-04-05 11:39:56]

Description was changed from

==========
Add readonly URLUtils.searchParams property.

R=
BUG=
==========

to

==========
Add support for URL.searchParams getter.

Add the missing piece to our URLSearchParams implementation;
the readonly attribute for URL.

The currently spec'ed connection between URL and URLSearchParams is
a lot less general than previous designs, hence the object lifetime
complexities it ran into (see https://codereview.chromium.org/143313002/),
falls away.

R=
BUG=303152
==========

[sof, 2016-04-05 11:52:54]

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

[sof, 2016-04-05 11:52:54]

please take a look.

Specs have become considerably less general wrt URLSearchParams integration
since last looking (a long while ago), so procrastination & unforeseen delays
sometimes work out for the better&simpler.

[Mike West, 2016-04-05 12:43:51]

Hooray for procrastination! Thank you for taking this on.. LGTM.

Do you plan on updating the WPT version of these tests as well?

https://codereview.chromium.org/1860623002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/domurl/url-searchparams.html (right):

https://codereview.chromium.org/1860623002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/domurl/url-searchparams.html:31: 'use
strict';
Why do we need strict mode for this test?

https://codereview.chromium.org/1860623002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/domurl/url-searchparams.html:48: url.search
= 'e=f&g=h';
Can you add a test that verifies that `?e=f&g=h` has the same result (leading
`?`)?

https://codereview.chromium.org/1860623002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/DOMURL.cpp (right):

https://codereview.chromium.org/1860623002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/DOMURL.cpp:47: }
Whoops! Nice catch. Is there a test for this?

https://codereview.chromium.org/1860623002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/DOMURL.cpp:75: if (!value.isEmpty() &&
value[0] == '?')
Nit: It might make sense to move this check into `updateSearchParams`.

https://codereview.chromium.org/1860623002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/DOMURLUtils.h (right):

https://codereview.chromium.org/1860623002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/DOMURLUtils.h:57: void setQuery(const
String&);
Nit: Maybe `setSearchInternal`? Distinguishing between "search" and "query"
seems strange.

[sof, 2016-04-05 15:23:18]

Thanks for the quick review.


> Do you plan on updating the WPT version of these tests as well?

Maybe..where/what? :)

I've still got some "global interface listing" weird diff errors to contend with
here though.

https://codereview.chromium.org/1860623002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/domurl/url-searchparams.html (right):

https://codereview.chromium.org/1860623002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/domurl/url-searchparams.html:31: 'use
strict';
On 2016/04/05 12:43:51, Mike West (OOO through 31st) wrote:
> Why do we need strict mode for this test?

Assignment to readonly attributes will be ignored and not throw in non-strict
mode; see Note at the end of the http://heycam.github.io/webidl/#es- attributes
section.

https://codereview.chromium.org/1860623002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/domurl/url-searchparams.html:48: url.search
= 'e=f&g=h';
On 2016/04/05 12:43:51, Mike West (OOO through 31st) wrote:
> Can you add a test that verifies that `?e=f&g=h` has the same result (leading
> `?`)?

Done.

https://codereview.chromium.org/1860623002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/DOMURL.cpp (right):

https://codereview.chromium.org/1860623002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/DOMURL.cpp:47: }
On 2016/04/05 12:43:51, Mike West (OOO through 31st) wrote:
> Whoops! Nice catch. Is there a test for this?

The final test in url-constructor.html

https://codereview.chromium.org/1860623002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/DOMURL.cpp:75: if (!value.isEmpty() &&
value[0] == '?')
On 2016/04/05 12:43:51, Mike West (OOO through 31st) wrote:
> Nit: It might make sense to move this check into `updateSearchParams`.

That won't quite work if you set url.href with something like
"http://example.org/??a=b"; it then chopping off the leading "?".

Added a test in url-searchparams.html to verify this.

https://codereview.chromium.org/1860623002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/DOMURLUtils.h (right):

https://codereview.chromium.org/1860623002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/DOMURLUtils.h:57: void setQuery(const
String&);
On 2016/04/05 12:43:51, Mike West (OOO through 31st) wrote:
> Nit: Maybe `setSearchInternal`? Distinguishing between "search" and "query"
> seems strange.

Good idea, done.

[Mike West, 2016-04-05 16:11:00]

Still LGTM, thanks!

On 2016/04/05 at 15:23:18, sigbjornf wrote:
> > Do you plan on updating the WPT version of these tests as well?
> 
> Maybe..where/what? :)

You can just copy/paste them in here:
https://github.com/w3c/web-platform-tests/tree/master/url.

>
https://codereview.chromium.org/1860623002/diff/60001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/fast/domurl/url-searchparams.html:31: 'use
strict';
> On 2016/04/05 12:43:51, Mike West (OOO through 31st) wrote:
> > Why do we need strict mode for this test?
> 
> Assignment to readonly attributes will be ignored and not throw in non-strict
mode; see Note at the end of the http://heycam.github.io/webidl/#es- attributes
section.

Huh. I didn't know that. Ok!

[sof, 2016-04-05 18:32:21]

On 2016/04/05 16:11:00, Mike West (OOO through 31st) wrote:
> Still LGTM, thanks!
> 

thanks; asking for blink-dev permission to ship before taking the next step.


> On 2016/04/05 at 15:23:18, sigbjornf wrote:
> > > Do you plan on updating the WPT version of these tests as well?
> > 
> > Maybe..where/what? :)
> 
> You can just copy/paste them in here:
> https://github.com/w3c/web-platform-tests/tree/master/url.
> 

Great, will do that tomorrow.

> >
>
https://codereview.chromium.org/1860623002/diff/60001/third_party/WebKit/Layo...
> > third_party/WebKit/LayoutTests/fast/domurl/url-searchparams.html:31: 'use
> strict';
> > On 2016/04/05 12:43:51, Mike West (OOO through 31st) wrote:
> > > Why do we need strict mode for this test?
> > 
> > Assignment to readonly attributes will be ignored and not throw in
non-strict
> mode; see Note at the end of the http://heycam.github.io/webidl/#es-
attributes
> section.
> 
> Huh. I didn't know that. Ok!

Over the top to have four tests exercising the throwing of the same exception
though, but one day .searchParams might just become writable again..

[sof, 2016-04-05 18:36:36]

Description was changed from

==========
Add support for URL.searchParams getter.

Add the missing piece to our URLSearchParams implementation;
the readonly attribute for URL.

The currently spec'ed connection between URL and URLSearchParams is
a lot less general than previous designs, hence the object lifetime
complexities it ran into (see https://codereview.chromium.org/143313002/),
falls away.

R=
BUG=303152
==========

to

==========
Add support for URL.searchParams getter.

Add the missing piece to our URLSearchParams implementation;
the readonly attribute for URL, URL.searchParams:

 https://url.spec.whatwg.org/#dom-url-searchparams

The currently spec'ed connection between URL and URLSearchParams is
a lot less general than previous designs, hence the object lifetime
complexities it ran into (see https://codereview.chromium.org/143313002/)
falls away.

Intent to Implement and Ship (for URLSearchParams and this URL attribute):


https://groups.google.com/a/chromium.org/d/msg/blink-dev/grHZDbldP04/JdsoQ169...

R=mkwst
BUG=303152
==========

[esprehn, 2016-04-05 21:06:19]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2016-04-05 21:06:19]

How does this deal with cycles? The URLSearchParams has a strong ref to the URL,
if you then make a cycle in JS on the wrappers from the URL -> URLSearchParams
then the URL is keeping the search params alive in JS, and the search params is
keeping the url alive in C++. So neither side will ever collect the objects I
think.

[sof, 2016-04-05 21:40:08]

On 2016/04/05 21:06:19, esprehn wrote:
> How does this deal with cycles? The URLSearchParams has a strong ref to the
URL,
> if you then make a cycle in JS on the wrappers from the URL -> URLSearchParams
> then the URL is keeping the search params alive in JS, and the search params
is
> keeping the url alive in C++. So neither side will ever collect the objects I
> think.

It's a good question; I'll investigate if that back reference from
URLSearchParams could somehow keep the wrapper to URL alive indefinitely.

[sof, 2016-04-06 10:34:16]

On 2016/04/05 21:40:08, sof wrote:
> On 2016/04/05 21:06:19, esprehn wrote:
> > How does this deal with cycles? The URLSearchParams has a strong ref to the
> URL,
> > if you then make a cycle in JS on the wrappers from the URL ->
URLSearchParams
> > then the URL is keeping the search params alive in JS, and the search params
> is
> > keeping the url alive in C++. So neither side will ever collect the objects
I
> > think.
> 
> It's a good question; I'll investigate if that back reference from
> URLSearchParams could somehow keep the wrapper to URL alive indefinitely.

If you really conjure up a mutual dependency on the v8 side with

  var url = new URL(..);
  var us  = url.searchParams;
  url.us = us;
  us.url = url;

the underlying "host objects" (DOMURL and URLSearchParams) will be retained if
|url| or |us| are referred to. Reasonable behavior.

If you sever the dependencies and drop the v8 reference to |url|, then
URLSearchParams is retained along with DOMURL. If you proceed to drop the
reference to |us|, then both URLSearchParams and DOMURL will be GCed.

So, the question is if holding onto the |us| wrapper should also keep
URLSearchParam's associated DOMURL object alive. Not the heaviest of objects,
but as there's no benefit in updating an otherwise unobservable object, I
suggest we don't.

Adjusted URLSearchParams::m_urlObject to be weak; thanks for bringing up the
issue.

[sof, 2016-04-06 12:54:41]

On 2016/04/05 18:32:21, sof wrote:
> On 2016/04/05 16:11:00, Mike West (OOO through 31st) wrote:
> > Still LGTM, thanks!
> > 
> 
> thanks; asking for blink-dev permission to ship before taking the next step.
> 
> 
> > On 2016/04/05 at 15:23:18, sigbjornf wrote:
> > > > Do you plan on updating the WPT version of these tests as well?
> > > 
> > > Maybe..where/what? :)
> > 
> > You can just copy/paste them in here:
> > https://github.com/w3c/web-platform-tests/tree/master/url.
> > 
> 
> Great, will do that tomorrow.
> 

https://github.com/w3c/web-platform-tests/pull/2756

[sof, 2016-04-08 11:44:04]

Going ahead and landing this one now, added feature dashboard entry + OWP
tracking bug (https://bugs.chromium.org/p/chromium/issues/detail?id=601750)

[sof, 2016-04-08 11:44:11]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2016-04-08 11:44:12]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1860623002/#ps140001
(title: "weaken URLSearchParam's associated URL object reference")

[commit-bot: I haz the power, 2016-04-08 11:44:23]

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

[commit-bot: I haz the power, 2016-04-08 11:46:45]

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

[commit-bot: I haz the power, 2016-04-08 11:46:46]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[sof, 2016-04-08 13:33:34]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2016-04-08 13:33:35]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1860623002/#ps160001
(title: "rebased")

[commit-bot: I haz the power, 2016-04-08 13:33:50]

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

[commit-bot: I haz the power, 2016-04-08 13:43:33]

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

[commit-bot: I haz the power, 2016-04-08 13:43:34]

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

[sof, 2016-04-08 15:49:03]

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

[sof, 2016-04-08 15:49:04]

tkent@, philipj@: approve of the webexposed changes being made?

[philipj_slow, 2016-04-08 19:44:37]

On 2016/04/08 15:49:04, sof wrote:
> tkent@, philipj@: approve of the webexposed changes being made?

lgtm

[sof, 2016-04-08 20:21:13]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2016-04-08 20:21:26]

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

[commit-bot: I haz the power, 2016-04-08 20:27:41]

Description was changed from

==========
Add support for URL.searchParams getter.

Add the missing piece to our URLSearchParams implementation;
the readonly attribute for URL, URL.searchParams:

 https://url.spec.whatwg.org/#dom-url-searchparams

The currently spec'ed connection between URL and URLSearchParams is
a lot less general than previous designs, hence the object lifetime
complexities it ran into (see https://codereview.chromium.org/143313002/)
falls away.

Intent to Implement and Ship (for URLSearchParams and this URL attribute):


https://groups.google.com/a/chromium.org/d/msg/blink-dev/grHZDbldP04/JdsoQ169...

R=mkwst
BUG=303152
==========

to

==========
Add support for URL.searchParams getter.

Add the missing piece to our URLSearchParams implementation;
the readonly attribute for URL, URL.searchParams:

 https://url.spec.whatwg.org/#dom-url-searchparams

The currently spec'ed connection between URL and URLSearchParams is
a lot less general than previous designs, hence the object lifetime
complexities it ran into (see https://codereview.chromium.org/143313002/)
falls away.

Intent to Implement and Ship (for URLSearchParams and this URL attribute):


https://groups.google.com/a/chromium.org/d/msg/blink-dev/grHZDbldP04/JdsoQ169...

R=mkwst
BUG=303152
==========

[commit-bot: I haz the power, 2016-04-08 20:27:43]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-04-08 20:29:53]

Description was changed from

==========
Add support for URL.searchParams getter.

Add the missing piece to our URLSearchParams implementation;
the readonly attribute for URL, URL.searchParams:

 https://url.spec.whatwg.org/#dom-url-searchparams

The currently spec'ed connection between URL and URLSearchParams is
a lot less general than previous designs, hence the object lifetime
complexities it ran into (see https://codereview.chromium.org/143313002/)
falls away.

Intent to Implement and Ship (for URLSearchParams and this URL attribute):


https://groups.google.com/a/chromium.org/d/msg/blink-dev/grHZDbldP04/JdsoQ169...

R=mkwst
BUG=303152
==========

to

==========
Add support for URL.searchParams getter.

Add the missing piece to our URLSearchParams implementation;
the readonly attribute for URL, URL.searchParams:

 https://url.spec.whatwg.org/#dom-url-searchparams

The currently spec'ed connection between URL and URLSearchParams is
a lot less general than previous designs, hence the object lifetime
complexities it ran into (see https://codereview.chromium.org/143313002/)
falls away.

Intent to Implement and Ship (for URLSearchParams and this URL attribute):


https://groups.google.com/a/chromium.org/d/msg/blink-dev/grHZDbldP04/JdsoQ169...

R=mkwst
BUG=303152

Committed: https://crrev.com/181f15d385bd66883ed39a58899a25d04838e2fe
Cr-Commit-Position: refs/heads/master@{#386189}
==========

[commit-bot: I haz the power, 2016-04-08 20:29:54]

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