Implement URLSearchParams.

Implement URLSearchParams.

Provide an implementation of the URLUtils' query object, URLSearchParams:

 http://url.spec.whatwg.org/#interface-urlsearchparams

It providing a more structured view of the URLUtils.search attribute,
allowing its name-value pairs to be separately manipulated and
updated. The URLSearchParams object can be separately constructed also.

The implementation depends on Oilpan to conveniently express the dependencies
between a URL query object and its URL objects (i.e., objects implementing
the URLUtils interface.)

R=arv@chromium.org,ch.dumez@samsung.com,abarth@chromium.org
BUG=303152

[sof, 2014-01-20 14:59:14]

Please take a look when you next have a spare moment.

I'm looking for some code pointers/guidance on how to represent the dual
dependency between a URLUtils implementation and its URLSearchParams query
object -- see the DOMURLUtils::setSearchParams() FIXME comment for the reasons
why this is needed. (I am aware of SVG objects handling such cycles via V8
wrappers, but perhaps there is a simpler pattern/solution lurking?)

[abarth-chromium, 2014-01-20 19:19:28]

The way I would do this is as follow:

1) Make DOMURLUtils own DOMURLSearchParams with an OwnPtr.
2) Make DOMURLSearchParams have a raw pointer to its DOMURLUtils parent.
3) Don't make DOMURLSearchParams inherit from RefCounted.  Instead, implement
ref() and deref() by calling the ref() and deref() methods on DOMURLUtils.

We use this pattern in a few places.  See, for example, DOMImplementation.  This
pattern is called "ref forwarding".  It's useful when there's a strict
parent/child relationship between two objects and the two objects live and die
together.

To be completely correct, you also need to add a reference from the V8 wrapper
for DOMURLUtils to the V8 wrapper for DOMURLSearchParams so that the latter
isn't garbage collected while the former still exists.

This stuff is all ugliness, which is why I'm excited about oilpan.

[sof, 2014-01-20 20:03:48]

On 2014/01/20 19:19:28, abarth wrote:
> The way I would do this is as follow:
> 
> 1) Make DOMURLUtils own DOMURLSearchParams with an OwnPtr.
> 2) Make DOMURLSearchParams have a raw pointer to its DOMURLUtils parent.
> 3) Don't make DOMURLSearchParams inherit from RefCounted.  Instead, implement
> ref() and deref() by calling the ref() and deref() methods on DOMURLUtils.
> 
> We use this pattern in a few places.  See, for example, DOMImplementation. 
This
> pattern is called "ref forwarding".  It's useful when there's a strict
> parent/child relationship between two objects and the two objects live and die
> together.
> 
> To be completely correct, you also need to add a reference from the V8 wrapper
> for DOMURLUtils to the V8 wrapper for DOMURLSearchParams so that the latter
> isn't garbage collected while the former still exists.

Thanks very much for outlining object ownerships & explaining the pattern; a
nice scheme to forward ref/derefs like that. It seems to fit here, I'll go over
the details & try to adopt.

> 
> This stuff is all ugliness, which is why I'm excited about oilpan.

Me too, can't wait to start moving code over.
Me too, but that's a nice trick to forward

[abarth-chromium, 2014-01-20 21:23:56]

On 2014/01/20 20:03:48, sof wrote:
> On 2014/01/20 19:19:28, abarth wrote:
> > To be completely correct, you also need to add a reference from the V8
wrapper
> > for DOMURLUtils to the V8 wrapper for DOMURLSearchParams so that the latter
> > isn't garbage collected while the former still exists.
> 
> Thanks very much for outlining object ownerships & explaining the pattern; a
> nice scheme to forward ref/derefs like that. It seems to fit here, I'll go
over
> the details & try to adopt.

By the way, I think there's a way to get the bindings code generator to add the
hidden reference from the parent to the child....

[Inactive, 2014-01-20 22:06:12]

https://codereview.chromium.org/143313002/diff/30001/Source/core/dom/DOMURLSe...
File Source/core/dom/DOMURLSearchParams.cpp (right):

https://codereview.chromium.org/143313002/diff/30001/Source/core/dom/DOMURLSe...
Source/core/dom/DOMURLSearchParams.cpp:114: return searchParams;
.release()

https://codereview.chromium.org/143313002/diff/30001/Source/core/dom/DOMURLSe...
Source/core/dom/DOMURLSearchParams.cpp:121: return searchParams;
.release()

https://codereview.chromium.org/143313002/diff/30001/Source/core/dom/DOMURLSe...
Source/core/dom/DOMURLSearchParams.cpp:124: return searchParams;
.release()

https://codereview.chromium.org/143313002/diff/30001/Source/core/dom/DOMURLSe...
Source/core/dom/DOMURLSearchParams.cpp:131: escaped.replace("%20", "+");
nit: Can return here directly.

https://codereview.chromium.org/143313002/diff/30001/Source/core/dom/DOMURLUt...
File Source/core/dom/DOMURLUtils.cpp (right):

https://codereview.chromium.org/143313002/diff/30001/Source/core/dom/DOMURLUt...
Source/core/dom/DOMURLUtils.cpp:37: if (m_searchParams.get())
FYI, you don't need this '.get()' in your conditionals (same below).

[sof, 2014-01-21 13:20:10]

Thanks Christophe, now hopefully addressed.

https://codereview.chromium.org/143313002/diff/30001/Source/core/dom/DOMURLSe...
File Source/core/dom/DOMURLSearchParams.cpp (right):

https://codereview.chromium.org/143313002/diff/30001/Source/core/dom/DOMURLSe...
Source/core/dom/DOMURLSearchParams.cpp:114: return searchParams;
On 2014/01/20 22:06:12, Chris Dumez wrote:
> .release()

Done.

https://codereview.chromium.org/143313002/diff/30001/Source/core/dom/DOMURLSe...
Source/core/dom/DOMURLSearchParams.cpp:121: return searchParams;
On 2014/01/20 22:06:12, Chris Dumez wrote:
> .release()

Done.

https://codereview.chromium.org/143313002/diff/30001/Source/core/dom/DOMURLSe...
Source/core/dom/DOMURLSearchParams.cpp:124: return searchParams;
On 2014/01/20 22:06:12, Chris Dumez wrote:
> .release()

Done.

https://codereview.chromium.org/143313002/diff/30001/Source/core/dom/DOMURLSe...
Source/core/dom/DOMURLSearchParams.cpp:131: escaped.replace("%20", "+");
On 2014/01/20 22:06:12, Chris Dumez wrote:
> nit: Can return here directly.

Done.

https://codereview.chromium.org/143313002/diff/30001/Source/core/dom/DOMURLUt...
File Source/core/dom/DOMURLUtils.cpp (right):

https://codereview.chromium.org/143313002/diff/30001/Source/core/dom/DOMURLUt...
Source/core/dom/DOMURLUtils.cpp:37: if (m_searchParams.get())
On 2014/01/20 22:06:12, Chris Dumez wrote:
> FYI, you don't need this '.get()' in your conditionals (same below).

Yes, thanks - tidied now.

[sof, 2014-01-21 13:34:29]

On 2014/01/20 21:23:56, abarth wrote:
> On 2014/01/20 20:03:48, sof wrote:
> > On 2014/01/20 19:19:28, abarth wrote:
> > > To be completely correct, you also need to add a reference from the V8
> wrapper
> > > for DOMURLUtils to the V8 wrapper for DOMURLSearchParams so that the
latter
> > > isn't garbage collected while the former still exists.
> > 
> > Thanks very much for outlining object ownerships & explaining the pattern; a
> > nice scheme to forward ref/derefs like that. It seems to fit here, I'll go
> over
> > the details & try to adopt.
> 
> By the way, I think there's a way to get the bindings code generator to add
the
> hidden reference from the parent to the child....

I ended up doing the registration the other way around -- the child attaching
itself to the parent's wrapper to keep itself alive.

However, as URLUtils is not an interface object (and implemented by an alias,
DOMURLUtils), this had to be done via a custom visitDOMWrapper() for
URLSearchParams that needs to be able to handle the objects that do implement
URLUtils. The non-interface nature of URLUtils also added indirections when
implementing ref count forwarding.

[sof, 2014-01-21 13:42:05]

FTR, the FF tests for URLSearchParams are "ok",

  https://bugzilla.mozilla.org/show_bug.cgi?id=887836

i.e., it expectedly fails on 'size' tests, an attribute that is no longer in the
spec. And the FF implementation doesn't handle the nullable DOMString result
from get() as required.

[arv (Not doing code reviews), 2014-01-21 14:40:32]

Thanks for picking this up. I started this when I initially implement the URL
interface but I put it on hold due to the API naming changes going on. I
uploaded my patch so that you can include the tests from there and compare the
implementations if you care. They are very similar.

I'm still not sure the GC behavior is going to be correct. URLSearchParams can
live by itself, without an owning URL and in that case does it not need to be
RefCounted?

Abarth?

https://codereview.chromium.org/143313002/diff/140001/Source/bindings/v8/cust...
File Source/bindings/v8/custom/V8URLSearchParamsCustom.cpp (right):

https://codereview.chromium.org/143313002/diff/140001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8URLSearchParamsCustom.cpp:60: DOMURL* domURL =
static_cast<DOMURL*>(urlObject);
Maybe extract to a template helper function so you do not have to repeat this
code?

https://codereview.chromium.org/143313002/diff/140001/Source/core/dom/DOMURLS...
File Source/core/dom/DOMURLSearchParams.cpp (right):

https://codereview.chromium.org/143313002/diff/140001/Source/core/dom/DOMURLS...
Source/core/dom/DOMURLSearchParams.cpp:121: RefPtr<DOMURLSearchParams>
searchParams = adoptRef(new DOMURLSearchParams());
We usually create a matching constructor and then have the create function just
in the .h file forward to that

static PassRefPtr<URLQuery> create(const String& queryString
{
    return adoptRef(new URLQuery(queryString);
}
static PassRefPtr<URLQuery> create(const DOMURLSearchParams& init
{
    return adoptRef(new URLQuery(init);
}

https://codereview.chromium.org/143313002/diff/140001/Source/core/dom/DOMURLS...
Source/core/dom/DOMURLSearchParams.cpp:132: if (!init)
Can this happen? Should we throw an exception if so?

https://codereview.chromium.org/143313002/diff/140001/Source/core/dom/DOMURLS...
Source/core/dom/DOMURLSearchParams.cpp:142: // Encode "%20" as "*"; as required
and much prettier.
Useless comment

https://codereview.chromium.org/143313002/diff/140001/Source/core/dom/DOMURLS...
Source/core/dom/DOMURLSearchParams.cpp:150: for (Vector<std::pair<String,
String> >::const_iterator it = m_params.begin(); it != m_params.end(); ++it) {
for (int i = 0; i < m_params.size(); ++i) {

ends up being cleaner since you can then skip isFirst

https://codereview.chromium.org/143313002/diff/140001/Source/core/dom/DOMURLS...
Source/core/dom/DOMURLSearchParams.cpp:160: if (!value.isEmpty()) {
This does not look correct to me. We should always print "=".

http://url.spec.whatwg.org/#concept-urlencoded-serializer

https://codereview.chromium.org/143313002/diff/140001/Source/core/dom/URLSear...
File Source/core/dom/URLSearchParams.idl (right):

https://codereview.chromium.org/143313002/diff/140001/Source/core/dom/URLSear...
Source/core/dom/URLSearchParams.idl:39: [ImplementedAs=appendNameValue] void
append(DOMString name, DOMString value);
Why all the renames? I think it is cleaner to just keep the same names as the
spec uses. It is less mental overhead to have to translate between the two.

[arv (Not doing code reviews), 2014-01-21 14:41:10]

On 2014/01/21 14:40:32, arv wrote:
> Thanks for picking this up. I started this when I initially implement the URL
> interface but I put it on hold due to the API naming changes going on. I
> uploaded my patch so that you can include the tests from there and compare the
> implementations if you care. They are very similar.

Oops, meant to include the CL number:

https://codereview.chromium.org/141003005/

[Inactive, 2014-01-21 15:25:58]

https://codereview.chromium.org/143313002/diff/140001/Source/core/dom/DOMURL.h
File Source/core/dom/DOMURL.h (right):

https://codereview.chromium.org/143313002/diff/140001/Source/core/dom/DOMURL....
Source/core/dom/DOMURL.h:73: virtual ImplementedBy implementedBy() const
OVERRIDE FINAL { return DOMURLUtils::DomURL; }
No need for FINAL (ditto below). The class is already FINAL.

https://codereview.chromium.org/143313002/diff/140001/Source/core/dom/DOMURLU...
File Source/core/dom/DOMURLUtils.cpp (right):

https://codereview.chromium.org/143313002/diff/140001/Source/core/dom/DOMURLU...
Source/core/dom/DOMURLUtils.cpp:36: KURL url = impl->url();
We should probably ASSERT to make sure impl is not null.

https://codereview.chromium.org/143313002/diff/140001/Source/core/dom/DOMURLU...
File Source/core/dom/DOMURLUtils.h (right):

https://codereview.chromium.org/143313002/diff/140001/Source/core/dom/DOMURLU...
Source/core/dom/DOMURLUtils.h:33: #include "wtf/RefPtr.h"
You're using an OwnPtr, not a RefPtr.

https://codereview.chromium.org/143313002/diff/140001/Source/core/dom/URLSear...
File Source/core/dom/URLSearchParams.idl (right):

https://codereview.chromium.org/143313002/diff/140001/Source/core/dom/URLSear...
Source/core/dom/URLSearchParams.idl:40: [ImplementedAs=deleteAllWithName]void
delete(DOMString name);
nit: missing space before 'void'.

https://codereview.chromium.org/143313002/diff/140001/Source/core/dom/URLSear...
Source/core/dom/URLSearchParams.idl:44: [ImplementedAs=setNameValue]void
set(DOMString name, DOMString value);
Ditto.

https://codereview.chromium.org/143313002/diff/140001/Source/core/dom/URLUtil...
File Source/core/dom/URLUtils.idl (right):

https://codereview.chromium.org/143313002/diff/140001/Source/core/dom/URLUtil...
Source/core/dom/URLUtils.idl:43: attribute URLSearchParams? searchParams;
Why mark it as nullable if it cannot be null? (at least according to your
implementation. It makes the implementation a bit more complex for no reason.

[sof, 2014-01-21 15:28:49]

On 2014/01/21 14:41:10, arv wrote:
> On 2014/01/21 14:40:32, arv wrote:
> > Thanks for picking this up. I started this when I initially implement the
URL
> > interface but I put it on hold due to the API naming changes going on. I
> > uploaded my patch so that you can include the tests from there and compare
the
> > implementations if you care. They are very similar.
> 
> Oops, meant to include the CL number:
> 
> https://codereview.chromium.org/141003005/

I did see your comment on the bug about URLQuery not being stable & on hold at
the time, but really ought to have checked with you first if you had written
code when you paused work on URLUtils. I'm sorry -- will bring in the tests, and
carefully go through the code.

[sof, 2014-01-21 21:16:46]

Good feedback and comments, thanks.

Two issues I've yet to address:

 - unattached URLSearchParams quite possibly leaking.
 - bringing in arv's tests.

https://codereview.chromium.org/143313002/diff/140001/Source/core/dom/DOMURL.h
File Source/core/dom/DOMURL.h (right):

https://codereview.chromium.org/143313002/diff/140001/Source/core/dom/DOMURL....
Source/core/dom/DOMURL.h:73: virtual ImplementedBy implementedBy() const
OVERRIDE FINAL { return DOMURLUtils::DomURL; }
On 2014/01/21 15:25:58, Chris Dumez wrote:
> No need for FINAL (ditto below). The class is already FINAL.

Done.

https://codereview.chromium.org/143313002/diff/140001/Source/core/dom/DOMURLS...
File Source/core/dom/DOMURLSearchParams.cpp (right):

https://codereview.chromium.org/143313002/diff/140001/Source/core/dom/DOMURLS...
Source/core/dom/DOMURLSearchParams.cpp:121: RefPtr<DOMURLSearchParams>
searchParams = adoptRef(new DOMURLSearchParams());
On 2014/01/21 14:40:33, arv wrote:
> We usually create a matching constructor and then have the create function
just
> in the .h file forward to that
> 
> static PassRefPtr<URLQuery> create(const String& queryString
> {
>     return adoptRef(new URLQuery(queryString);
> }
> static PassRefPtr<URLQuery> create(const DOMURLSearchParams& init
> {
>     return adoptRef(new URLQuery(init);
> }

That's quite doable..and done.

https://codereview.chromium.org/143313002/diff/140001/Source/core/dom/DOMURLS...
Source/core/dom/DOMURLSearchParams.cpp:132: if (!init)
On 2014/01/21 14:40:33, arv wrote:
> Can this happen? Should we throw an exception if so?

Typically yes, a TypeError due to failed object conversion. But we can delegate
exception handling to the "super constructor" that handles URLSearchParams
overload resolution -- I notice the generated version wasn't quite right, so
ended up writing a custom one.

https://codereview.chromium.org/143313002/diff/140001/Source/core/dom/DOMURLS...
Source/core/dom/DOMURLSearchParams.cpp:142: // Encode "%20" as "*"; as required
and much prettier.
On 2014/01/21 14:40:33, arv wrote:
> Useless comment

Gone.

https://codereview.chromium.org/143313002/diff/140001/Source/core/dom/DOMURLS...
Source/core/dom/DOMURLSearchParams.cpp:150: for (Vector<std::pair<String,
String> >::const_iterator it = m_params.begin(); it != m_params.end(); ++it) {
On 2014/01/21 14:40:33, arv wrote:
> for (int i = 0; i < m_params.size(); ++i) {
> 
> ends up being cleaner since you can then skip isFirst

Yep, thanks

https://codereview.chromium.org/143313002/diff/140001/Source/core/dom/DOMURLS...
Source/core/dom/DOMURLSearchParams.cpp:160: if (!value.isEmpty()) {
On 2014/01/21 14:40:33, arv wrote:
> This does not look correct to me. We should always print "=".
> 
> http://url.spec.whatwg.org/#concept-urlencoded-serializer

Oops, quite right. Fixed + adjusted expected test outcomes.

https://codereview.chromium.org/143313002/diff/140001/Source/core/dom/DOMURLU...
File Source/core/dom/DOMURLUtils.cpp (right):

https://codereview.chromium.org/143313002/diff/140001/Source/core/dom/DOMURLU...
Source/core/dom/DOMURLUtils.cpp:36: KURL url = impl->url();
On 2014/01/21 15:25:58, Chris Dumez wrote:
> We should probably ASSERT to make sure impl is not null.

When the context makes it unmistakably clear that a non-null pointer is to be
expected, I don't see the value of adding such asserts.

https://codereview.chromium.org/143313002/diff/140001/Source/core/dom/URLUtil...
File Source/core/dom/URLUtils.idl (right):

https://codereview.chromium.org/143313002/diff/140001/Source/core/dom/URLUtil...
Source/core/dom/URLUtils.idl:43: attribute URLSearchParams? searchParams;
On 2014/01/21 15:25:58, Chris Dumez wrote:
> Why mark it as nullable if it cannot be null? (at least according to your
> implementation. It makes the implementation a bit more complex for no reason.

Yes, the query object cannot be null here, so leave out the unneeded generality.

[sof, 2014-01-22 13:49:16]

On 2014/01/21 21:16:46, sof wrote:
> Good feedback and comments, thanks.
> 
> Two issues I've yet to address:
> 
>  - unattached URLSearchParams quite possibly leaking.
>  - bringing in arv's tests.
> 

Both addressed now.

(If anyone calls the handling of ownerships here pretty, I would have to
disagree :-) )

[arv (Not doing code reviews), 2014-01-22 16:12:05]

A few nits. I would also prefer if abarth could verify that the ref counting
code is fine.

https://codereview.chromium.org/143313002/diff/90003/LayoutTests/fast/domurl/...
File LayoutTests/fast/domurl/url-search-expected.txt (right):

https://codereview.chromium.org/143313002/diff/90003/LayoutTests/fast/domurl/...
LayoutTests/fast/domurl/url-search-expected.txt:1: This is a
testharness.js-based test.
This file can be deleted now. The test runner was updated since this was
written.

https://codereview.chromium.org/143313002/diff/90003/LayoutTests/fast/domurl/...
File LayoutTests/fast/domurl/url-searchparams-constructor.html (right):

https://codereview.chromium.org/143313002/diff/90003/LayoutTests/fast/domurl/...
LayoutTests/fast/domurl/url-searchparams-constructor.html:7: function
assert_typeerror(f, msg) {
assert_type_error

https://codereview.chromium.org/143313002/diff/90003/LayoutTests/fast/domurl/...
LayoutTests/fast/domurl/url-searchparams-constructor.html:10:
assert_unreached("Expected to throw a TypeError");
This is a common error (maybe you even got it from me?). If this assertion fails
then it get caught in the catch.

Here is better way to handle this:

function assert_type_error(f, msg) {
    var e;
    try {
        f();
    } catch (ex) {
        e = ex;
    }
    assert_true(e instanceof TypeError);
    if (msg)
        assert_equals(e.message, msg);
}

https://codereview.chromium.org/143313002/diff/90003/LayoutTests/fast/domurl/...
File LayoutTests/fast/domurl/url-searchparams.html (right):

https://codereview.chromium.org/143313002/diff/90003/LayoutTests/fast/domurl/...
LayoutTests/fast/domurl/url-searchparams.html:55: }, "URLSearchParams GC
handling");
Is there a test for object identity?

var url = new URL(...);
var params = url.searchParams;
assert_equals(url.searchParams, params);

Are we supposed to keep the object identity when assigning?

var url = new URL(...);
var params = new URLSearchParams(...);
assert_equals(url.searchParams, params);

https://codereview.chromium.org/143313002/diff/90003/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8URLSearchParamsCustom.cpp (right):

https://codereview.chromium.org/143313002/diff/90003/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8URLSearchParamsCustom.cpp:47: void
V8URLSearchParams::constructorCustom(const v8::FunctionCallbackInfo<v8::Value>&
info)
It is not clear to me why this needs a custom constructor?

https://codereview.chromium.org/143313002/diff/90003/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8URLSearchParamsCustom.cpp:58: if
(V8URLSearchParams::hasInstance(info[0], info.GetIsolate(),
worldType(info.GetIsolate()))) {
I thought hasInstance was updated. Did you sync recently?

https://codereview.chromium.org/143313002/diff/90003/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8URLSearchParamsCustom.cpp:72: void
V8URLSearchParams::visitDOMWrapper(void* object, const
v8::Persistent<v8::Object>& wrapper, v8::Isolate* isolate)
Could this be done with GenerateVisitDOMWrapper=ownerMethodName if we changed
the codegen slightly?

No need to do this now but it looks possible and custom binding code is always
something we should try to minimize.

https://codereview.chromium.org/143313002/diff/90003/Source/core/dom/DOMURLUt...
File Source/core/dom/DOMURLUtils.cpp (right):

https://codereview.chromium.org/143313002/diff/90003/Source/core/dom/DOMURLUt...
Source/core/dom/DOMURLUtils.cpp:196: if (searchParams->urlObject()) {
Is there a test covering this?

[sof, 2014-01-22 17:15:46]

Some comments for now, will follow up later tonight.

https://codereview.chromium.org/143313002/diff/90003/LayoutTests/fast/domurl/...
File LayoutTests/fast/domurl/url-searchparams.html (right):

https://codereview.chromium.org/143313002/diff/90003/LayoutTests/fast/domurl/...
LayoutTests/fast/domurl/url-searchparams.html:55: }, "URLSearchParams GC
handling");
On 2014/01/22 16:12:05, arv wrote:
> Is there a test for object identity?
> 
> var url = new URL(...);
> var params = url.searchParams;
> assert_equals(url.searchParams, params);
> 
> Are we supposed to keep the object identity when assigning?
> 
> var url = new URL(...);
> var params = new URLSearchParams(...);
> assert_equals(url.searchParams, params);

Gnarly stuff. The setter has a copying semantics if the url object is set on the
URLSearchParams object, but one that is pretty odd:

  var url = new URL(..);
  var us = new URLSearchParams(..);
  url.searchParams = us;
  us.set(..); // has an effect on url.search
  url.searchParams = us;
  us.set(..); // not updating anything.

Do we want to provide this and/or should it be clarified spec wise first?

https://codereview.chromium.org/143313002/diff/90003/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8URLSearchParamsCustom.cpp (right):

https://codereview.chromium.org/143313002/diff/90003/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8URLSearchParamsCustom.cpp:47: void
V8URLSearchParams::constructorCustom(const v8::FunctionCallbackInfo<v8::Value>&
info)
On 2014/01/22 16:12:05, arv wrote:
> It is not clear to me why this needs a custom constructor?

Ah, it is possible to do without -- I overlooked step 11 of
http://heycam.github.io/webidl/#es-union and the ToString() conversion that
should bring.

Nice; less custom code.

https://codereview.chromium.org/143313002/diff/90003/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8URLSearchParamsCustom.cpp:72: void
V8URLSearchParams::visitDOMWrapper(void* object, const
v8::Persistent<v8::Object>& wrapper, v8::Isolate* isolate)
On 2014/01/22 16:12:05, arv wrote:
> Could this be done with GenerateVisitDOMWrapper=ownerMethodName if we changed
> the codegen slightly?
> 
> No need to do this now but it looks possible and custom binding code is always
> something we should try to minimize.

Definitely agree with that. The complexity enters due to the NoInterfaceObject
nature of URLUtils, so we have to bridge over to whatever concrete wrapper type
of the object that implements URLUtils. I didn't see a good fit with
[GenerateVisitDOMWrapper] (with possible tweaks) when considering using it
initially, but there could well be a feasible way.

(Another very minor spanner in the works here is that the combination of
ImplementedAs + GenerateVisitDOMWrapper is not quite supported by the codegen.)

[sof, 2014-01-22 17:24:54]

https://codereview.chromium.org/143313002/diff/90003/LayoutTests/fast/domurl/...
File LayoutTests/fast/domurl/url-searchparams.html (right):

https://codereview.chromium.org/143313002/diff/90003/LayoutTests/fast/domurl/...
LayoutTests/fast/domurl/url-searchparams.html:55: }, "URLSearchParams GC
handling");
On 2014/01/22 17:15:47, sof wrote:
> On 2014/01/22 16:12:05, arv wrote:
> > Is there a test for object identity?
> > 
> > var url = new URL(...);
> > var params = url.searchParams;
> > assert_equals(url.searchParams, params);
> > 
> > Are we supposed to keep the object identity when assigning?
> > 
> > var url = new URL(...);
> > var params = new URLSearchParams(...);
> > assert_equals(url.searchParams, params);
> 
> Gnarly stuff. The setter has a copying semantics if the url object is set on
the
> URLSearchParams object, but one that is pretty odd:
> 
>   var url = new URL(..);
>   var us = new URLSearchParams(..);
>   url.searchParams = us;
>   us.set(..); // has an effect on url.search
>   url.searchParams = us;
>   us.set(..); // not updating anything.
> 
> Do we want to provide this and/or should it be clarified spec wise first?

Oh, and if you set 'us' yet again, it will start having an effect once more.

[arv (Not doing code reviews), 2014-01-22 18:31:43]

On 2014/01/22 17:24:54, sof wrote:
>
https://codereview.chromium.org/143313002/diff/90003/LayoutTests/fast/domurl/...
> File LayoutTests/fast/domurl/url-searchparams.html (right):
> 
>
https://codereview.chromium.org/143313002/diff/90003/LayoutTests/fast/domurl/...
> LayoutTests/fast/domurl/url-searchparams.html:55: }, "URLSearchParams GC
> handling");
> On 2014/01/22 17:15:47, sof wrote:
> > On 2014/01/22 16:12:05, arv wrote:
> > > Is there a test for object identity?
> > > 
> > > var url = new URL(...);
> > > var params = url.searchParams;
> > > assert_equals(url.searchParams, params);
> > > 
> > > Are we supposed to keep the object identity when assigning?
> > > 
> > > var url = new URL(...);
> > > var params = new URLSearchParams(...);
> > > assert_equals(url.searchParams, params);
> > 
> > Gnarly stuff. The setter has a copying semantics if the url object is set on
> the
> > URLSearchParams object, but one that is pretty odd:
> > 
> >   var url = new URL(..);
> >   var us = new URLSearchParams(..);
> >   url.searchParams = us;
> >   us.set(..); // has an effect on url.search
> >   url.searchParams = us;
> >   us.set(..); // not updating anything.
> > 
> > Do we want to provide this and/or should it be clarified spec wise first?
> 
> Oh, and if you set 'us' yet again, it will start having an effect once more.

I'm OK with copy on set since that is what the spec says. If we do not  do the
copy it gets strange if the same URLSearchParams object is used for multiple
URLs.

[sof, 2014-01-22 18:37:31]

On 2014/01/22 18:31:43, arv wrote:
> On 2014/01/22 17:24:54, sof wrote:
> >
>
https://codereview.chromium.org/143313002/diff/90003/LayoutTests/fast/domurl/...
> > File LayoutTests/fast/domurl/url-searchparams.html (right):
> > 
> >
>
https://codereview.chromium.org/143313002/diff/90003/LayoutTests/fast/domurl/...
> > LayoutTests/fast/domurl/url-searchparams.html:55: }, "URLSearchParams GC
> > handling");
> > On 2014/01/22 17:15:47, sof wrote:
> > > On 2014/01/22 16:12:05, arv wrote:
> > > > Is there a test for object identity?
> > > > 
> > > > var url = new URL(...);
> > > > var params = url.searchParams;
> > > > assert_equals(url.searchParams, params);
> > > > 
> > > > Are we supposed to keep the object identity when assigning?
> > > > 
> > > > var url = new URL(...);
> > > > var params = new URLSearchParams(...);
> > > > assert_equals(url.searchParams, params);
> > > 
> > > Gnarly stuff. The setter has a copying semantics if the url object is set
on
> > the
> > > URLSearchParams object, but one that is pretty odd:
> > > 
> > >   var url = new URL(..);
> > >   var us = new URLSearchParams(..);
> > >   url.searchParams = us;
> > >   us.set(..); // has an effect on url.search
> > >   url.searchParams = us;
> > >   us.set(..); // not updating anything.
> > > 
> > > Do we want to provide this and/or should it be clarified spec wise first?
> > 
> > Oh, and if you set 'us' yet again, it will start having an effect once more.
> 
> I'm OK with copy on set since that is what the spec says. If we do not  do the
> copy it gets strange if the same URLSearchParams object is used for multiple
> URLs.

How so? The last one "wins".

[arv (Not doing code reviews), 2014-01-22 18:48:27]

On 2014/01/22 18:37:31, sof wrote:
> On 2014/01/22 18:31:43, arv wrote:
> > On 2014/01/22 17:24:54, sof wrote:
> > >
> >
>
https://codereview.chromium.org/143313002/diff/90003/LayoutTests/fast/domurl/...
> > > File LayoutTests/fast/domurl/url-searchparams.html (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/143313002/diff/90003/LayoutTests/fast/domurl/...
> > > LayoutTests/fast/domurl/url-searchparams.html:55: }, "URLSearchParams GC
> > > handling");
> > > On 2014/01/22 17:15:47, sof wrote:
> > > > On 2014/01/22 16:12:05, arv wrote:
> > > > > Is there a test for object identity?
> > > > > 
> > > > > var url = new URL(...);
> > > > > var params = url.searchParams;
> > > > > assert_equals(url.searchParams, params);
> > > > > 
> > > > > Are we supposed to keep the object identity when assigning?
> > > > > 
> > > > > var url = new URL(...);
> > > > > var params = new URLSearchParams(...);
> > > > > assert_equals(url.searchParams, params);
> > > > 
> > > > Gnarly stuff. The setter has a copying semantics if the url object is
set
> on
> > > the
> > > > URLSearchParams object, but one that is pretty odd:
> > > > 
> > > >   var url = new URL(..);
> > > >   var us = new URLSearchParams(..);
> > > >   url.searchParams = us;
> > > >   us.set(..); // has an effect on url.search
> > > >   url.searchParams = us;
> > > >   us.set(..); // not updating anything.
> > > > 
> > > > Do we want to provide this and/or should it be clarified spec wise
first?
> > > 
> > > Oh, and if you set 'us' yet again, it will start having an effect once
more.
> > 
> > I'm OK with copy on set since that is what the spec says. If we do not  do
the
> > copy it gets strange if the same URLSearchParams object is used for multiple
> > URLs.
> 
> How so? The last one "wins".

We don't want to associate the URLSearchParams with more than one URL and we
assign to a second URL what do we do with the first one?

[sof, 2014-01-22 19:25:25]

On 2014/01/22 18:48:27, arv wrote:
> On 2014/01/22 18:37:31, sof wrote:
> > On 2014/01/22 18:31:43, arv wrote:
> > > On 2014/01/22 17:24:54, sof wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/143313002/diff/90003/LayoutTests/fast/domurl/...
> > > > File LayoutTests/fast/domurl/url-searchparams.html (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/143313002/diff/90003/LayoutTests/fast/domurl/...
> > > > LayoutTests/fast/domurl/url-searchparams.html:55: }, "URLSearchParams GC
> > > > handling");
> > > > On 2014/01/22 17:15:47, sof wrote:
> > > > > On 2014/01/22 16:12:05, arv wrote:
> > > > > > Is there a test for object identity?
> > > > > > 
> > > > > > var url = new URL(...);
> > > > > > var params = url.searchParams;
> > > > > > assert_equals(url.searchParams, params);
> > > > > > 
> > > > > > Are we supposed to keep the object identity when assigning?
> > > > > > 
> > > > > > var url = new URL(...);
> > > > > > var params = new URLSearchParams(...);
> > > > > > assert_equals(url.searchParams, params);
> > > > > 
> > > > > Gnarly stuff. The setter has a copying semantics if the url object is
> set
> > on
> > > > the
> > > > > URLSearchParams object, but one that is pretty odd:
> > > > > 
> > > > >   var url = new URL(..);
> > > > >   var us = new URLSearchParams(..);
> > > > >   url.searchParams = us;
> > > > >   us.set(..); // has an effect on url.search
> > > > >   url.searchParams = us;
> > > > >   us.set(..); // not updating anything.
> > > > > 
> > > > > Do we want to provide this and/or should it be clarified spec wise
> first?
> > > > 
> > > > Oh, and if you set 'us' yet again, it will start having an effect once
> more.
> > > 
> > > I'm OK with copy on set since that is what the spec says. If we do not  do
> the
> > > copy it gets strange if the same URLSearchParams object is used for
multiple
> > > URLs.
> > 
> > How so? The last one "wins".
> 
> We don't want to associate the URLSearchParams with more than one URL and we
> assign to a second URL what do we do with the first one?

That would be undesirable, and I'm not suggesting it should be, merely that
url.searchParams = url.searchParams be a no-op. All other inter-URL copying of
these attached URLSearchParams would be a copy operation (like now.)

[sof, 2014-01-22 19:27:08]

https://codereview.chromium.org/143313002/diff/90003/LayoutTests/fast/domurl/...
File LayoutTests/fast/domurl/url-search-expected.txt (right):

https://codereview.chromium.org/143313002/diff/90003/LayoutTests/fast/domurl/...
LayoutTests/fast/domurl/url-search-expected.txt:1: This is a
testharness.js-based test.
On 2014/01/22 16:12:05, arv wrote:
> This file can be deleted now. The test runner was updated since this was
> written.

Great; removed the other ones of its ilk in this directory also.

https://codereview.chromium.org/143313002/diff/90003/LayoutTests/fast/domurl/...
File LayoutTests/fast/domurl/url-searchparams-constructor.html (right):

https://codereview.chromium.org/143313002/diff/90003/LayoutTests/fast/domurl/...
LayoutTests/fast/domurl/url-searchparams-constructor.html:10:
assert_unreached("Expected to throw a TypeError");
On 2014/01/22 16:12:05, arv wrote:
> This is a common error (maybe you even got it from me?). If this assertion
fails
> then it get caught in the catch.
> 
> Here is better way to handle this:
> 
> function assert_type_error(f, msg) {
>     var e;
>     try {
>         f();
>     } catch (ex) {
>         e = ex;
>     }
>     assert_true(e instanceof TypeError);
>     if (msg)
>         assert_equals(e.message, msg);
> }

Thanks, bug was all mine :), fixed. (Would be nice to get some variation on this
into testharness.js, assert_throws() doesn't cover all exception testing.)

https://codereview.chromium.org/143313002/diff/90003/Source/core/dom/DOMURLUt...
File Source/core/dom/DOMURLUtils.cpp (right):

https://codereview.chromium.org/143313002/diff/90003/Source/core/dom/DOMURLUt...
Source/core/dom/DOMURLUtils.cpp:196: if (searchParams->urlObject()) {
On 2014/01/22 16:12:05, arv wrote:
> Is there a test covering this?

Yes, url-searchparam-constructor.html ("URLSearchParams constructor, object.")

[abarth-chromium, 2014-01-22 20:52:38]

https://codereview.chromium.org/143313002/diff/600001/Source/bindings/v8/cust...
File Source/bindings/v8/custom/V8URLSearchParamsCustom.cpp (right):

https://codereview.chromium.org/143313002/diff/600001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8URLSearchParamsCustom.cpp:45: void
V8URLSearchParams::visitDOMWrapper(void* object, const
v8::Persistent<v8::Object>& wrapper, v8::Isolate* isolate)
Can we do this without custom code?  I see that we have some code like this in
the generated bindings.  Why can't we generate the code for this case?

https://codereview.chromium.org/143313002/diff/600001/Source/core/dom/DOMURLS...
File Source/core/dom/DOMURLSearchParams.h (right):

https://codereview.chromium.org/143313002/diff/600001/Source/core/dom/DOMURLS...
Source/core/dom/DOMURLSearchParams.h:88: int m_refCount;
Hum...  I didn't realize that it can exist in that state.  I'm not sure we want
to have two ways of managing a ref count on this object.  That seems overly
complicated.  Maybe we should wait until oilpan lands before implementing this
API.  I don't see a clean way of doing it given our current memory management
techniques.

[sof, 2014-01-22 21:07:28]

https://codereview.chromium.org/143313002/diff/600001/Source/bindings/v8/cust...
File Source/bindings/v8/custom/V8URLSearchParamsCustom.cpp (right):

https://codereview.chromium.org/143313002/diff/600001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8URLSearchParamsCustom.cpp:45: void
V8URLSearchParams::visitDOMWrapper(void* object, const
v8::Persistent<v8::Object>& wrapper, v8::Isolate* isolate)
On 2014/01/22 20:52:39, abarth wrote:
> Can we do this without custom code?  I see that we have some code like this in
> the generated bindings.  Why can't we generate the code for this case?

https://codereview.chromium.org/143313002/#msg7 tried to explain why. I'll
follow up with an attempt at a clearer explanation tomorrow.

[sof, 2014-01-23 08:34:27]

(+harakan, nbarth as a FYI.)

Re: https://codereview.chromium.org/143313002/#msg23 and why a custom
visitDOMWrapper() is needed -- here's an attempt to explain why in (long) form:

We've got the following two interfaces,

 [NoInterfaceObject] interface URLUtils { attribute URLSearchParams
searchParams; ... }
 interface URLSearchParams { ... }

The current URLSearchParams object kept on a URLUtils instance has a connection
to its parent, meaning that operations performed on it (URLSearchParams) has an
effect on the URLUtils instance. Hence, the lifetime of URLUtils must be at
least as long as its URLSearchParams. (Consequently, the URLSearchParams is kept
as an OwnPtr on a URLUtils instance.)

To ensure that the current URLSearchParams object stays referenced during GCs,
we need to implement a visitDOMWrapper() method that registers the
URLSearchParams -> URLUtils dependency. Something you can express in IDL as 

  [GenerateVisitDOMWrapper=document|owner|..]

which generates a visit method that fetches out the parent object by calling a
method on the C++ object implementing the interface we're generating a binding
to (URLSearchParams.) For a known set of common cases (e.g., Node-derived
parents) this works out well.

If that doesn't fit, an alternative is to use [SetReference], 

  [SetReference(ParentType parent)]
  
which looks up the wrapper in the DOMDataStore as follows:

  ParentType parent = impl->parent();
  if (parent) {
      if (!DOMDataStore::containsWrapper<V8ParentType>(parent, isolate))
          wrap(...);
      DOMDataStore::setWrapperReference<V8ParentType>(wrapper, parent, isolate)
  }
  setObjectGroup(object, wrapper, isolate);

This looks suitable for URLSearchParams, as it isn't tied to a Node-based parent
necessarily, but the basic assumption of SetReference is that the concrete type
name for the parent is known. And it isn't for URLSearchParams -- the URLUtils
is [NoInterfaceObject], so it might be implemented by multiple objects (URL,
HTMLAnchorElement, HTMLAreaElement currently.) Hence, we're unable to get at the
type name T of the object implementing the URLUtils interface _and_ that of its
wrapper type (V8T.)

Which is why automatic generation of visitDOMWrapper() cannot be used here, but
a custom one is provided instead. Which requires a type discovery method on the
URLUtils instances to let the custom visitDOMWrapper() determine what particular
instance it is faced with. Once that instance type has been resolved, the code
to register with the wrapper object is boilerplate. 

I could well be overlooking some other code generator feature, but this
[NoInterfaceObject] constellation seems unique.

[sof, 2014-01-23 09:03:48]

On 2014/01/22 20:52:38, abarth wrote:
...
> 
>
https://codereview.chromium.org/143313002/diff/600001/Source/core/dom/DOMURLS...
> File Source/core/dom/DOMURLSearchParams.h (right):
> 
>
https://codereview.chromium.org/143313002/diff/600001/Source/core/dom/DOMURLS...
> Source/core/dom/DOMURLSearchParams.h:88: int m_refCount;
> Hum...  I didn't realize that it can exist in that state.  I'm not sure we
want
> to have two ways of managing a ref count on this object.  That seems overly
> complicated.  Maybe we should wait until oilpan lands before implementing this
> API.  I don't see a clean way of doing it given our current memory management
> techniques.

Good to know that some unknown encoding pattern that would avoid the complexity,
hasn't been overlooked.

URLSearchParams completes the implementation of the URL(Utils) API, nothing
more. Waiting on Oilpan shouldn't be a problem.

[sof, 2014-01-24 07:11:36]

https://codereview.chromium.org/143313002/diff/90003/LayoutTests/fast/domurl/...
File LayoutTests/fast/domurl/url-searchparams.html (right):

https://codereview.chromium.org/143313002/diff/90003/LayoutTests/fast/domurl/...
LayoutTests/fast/domurl/url-searchparams.html:55: }, "URLSearchParams GC
handling");
On 2014/01/22 16:12:05, arv wrote:
> Is there a test for object identity?
> 
> var url = new URL(...);
> var params = url.searchParams;
> assert_equals(url.searchParams, params);
> 
> Are we supposed to keep the object identity when assigning?
> 
> var url = new URL(...);
> var params = new URLSearchParams(...);
> assert_equals(url.searchParams, params);

Added a test for assumed behavior of the setter + filed
https://www.w3.org/Bugs/Public/show_bug.cgi?id=24367 for clarification re:
wanted semantics of self-updating.

[sof, 2014-01-29 11:12:54]

The evolving version of the XHR spec supports URLSearchParams over send(),

  http://xhr.spec.whatwg.org/#the-send()-method

That will be addressed separately, just making a note of it here.

[sof, 2014-01-31 14:15:51]

On 2014/01/24 07:11:36, sof wrote:
>
https://codereview.chromium.org/143313002/diff/90003/LayoutTests/fast/domurl/...
> File LayoutTests/fast/domurl/url-searchparams.html (right):
> 
>
https://codereview.chromium.org/143313002/diff/90003/LayoutTests/fast/domurl/...
> LayoutTests/fast/domurl/url-searchparams.html:55: }, "URLSearchParams GC
> handling");
> On 2014/01/22 16:12:05, arv wrote:
> > Is there a test for object identity?
> > 
> > var url = new URL(...);
> > var params = url.searchParams;
> > assert_equals(url.searchParams, params);
> > 
> > Are we supposed to keep the object identity when assigning?
> > 
> > var url = new URL(...);
> > var params = new URLSearchParams(...);
> > assert_equals(url.searchParams, params);
> 
> Added a test for assumed behavior of the setter + filed
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=24367 for clarification re:
> wanted semantics of self-updating.

A new & more general version of updating has now been spec'ed --
http://url.spec.whatwg.org/#concept-uq-update

The lifetime & ownerships of objects will change as a result; the decision to
wait on Oilpan seems even wiser.

[sof, 2014-06-15 20:34:11]

Reviving(*) this CL, using Oilpan to express the needed object dependencies, as
was previously agreed as the better way forward.

If possible, I'd like to get this CL ready so that when Oilpan is ready to be
enabled by default for (HTML)Elements, it can be pushed along (i.e., it cannot
land before that time.)

Changes since last time:

 - URLSearchParams now maintains a set of URL objects that it should propagate
updates to [spec change.]
 - URLUtils.searchParams is no longer a nullable attribute [spec change.]
 - Removed all the ref counting machinery for Oilpan.

* - I think it makes sense to continue on this (older) review as it has some
context, but if the preference is to start anew, I can certainly do that.

[haraken, 2014-06-16 01:00:56]

In terms of oilpan, looks good.

https://codereview.chromium.org/143313002/diff/1180001/Source/core/dom/URLSea...
File Source/core/dom/URLSearchParams.idl (right):

https://codereview.chromium.org/143313002/diff/1180001/Source/core/dom/URLSea...
Source/core/dom/URLSearchParams.idl:19: [ImplementedAs=setNameValue] void
set(DOMString name, DOMString value);

I guess you're a bit overly using [ImplementedAs]. append/get/getAll/has/set
won't need [ImplementedAs].

[sof, 2014-06-16 11:27:43]

https://codereview.chromium.org/143313002/diff/1180001/Source/core/dom/URLSea...
File Source/core/dom/URLSearchParams.idl (right):

https://codereview.chromium.org/143313002/diff/1180001/Source/core/dom/URLSea...
Source/core/dom/URLSearchParams.idl:19: [ImplementedAs=setNameValue] void
set(DOMString name, DOMString value);
On 2014/06/16 01:00:56, haraken wrote:
> 
> I guess you're a bit overly using [ImplementedAs]. append/get/getAll/has/set
> won't need [ImplementedAs].

Maybe so, but they're not too informative as C++ method names.

[Nils Barth (inactive), 2014-06-17 01:19:28]

https://codereview.chromium.org/143313002/diff/1180001/Source/core/dom/URLSea...
File Source/core/dom/URLSearchParams.idl (right):

https://codereview.chromium.org/143313002/diff/1180001/Source/core/dom/URLSea...
Source/core/dom/URLSearchParams.idl:19: [ImplementedAs=setNameValue] void
set(DOMString name, DOMString value);
On 2014/06/16 11:27:43, sof wrote:
> On 2014/06/16 01:00:56, haraken wrote:
> > 
> > I guess you're a bit overly using [ImplementedAs]. append/get/getAll/has/set
> > won't need [ImplementedAs].
> 
> Maybe so, but they're not too informative as C++ method names.

As a rule we avoid [ImplementedAs] unless strictly necessary, to align with
spec;
with naming the parameters, at least the function declarations are clear:
bool has(const String& name) const;
void set(const String& name, const String& value);

...and seem less redundant than:
bool hasName(const String&) const;
void setNameValue(const String& name, const String& value);

[Nils Barth (inactive), 2014-06-17 01:24:12]

Documentation + IDL nits.

https://codereview.chromium.org/143313002/diff/1180001/Source/core/dom/URLSea...
File Source/core/dom/URLSearchParams.idl (right):

https://codereview.chromium.org/143313002/diff/1180001/Source/core/dom/URLSea...
Source/core/dom/URLSearchParams.idl:5: [
Could you link to spec here, as:
// http://url.spec.whatwg.org/#interface-urlsearchparams

https://codereview.chromium.org/143313002/diff/1180001/Source/core/dom/URLSea...
Source/core/dom/URLSearchParams.idl:6: Constructor(),
Could you add a FIXME:
// FIXME: should be (ｈttp://crbug.com/240176):
// Constructor(optional (ScalarValueString or URLSearchParams) init = ""),

https://codereview.chromium.org/143313002/diff/1180001/Source/core/dom/URLSea...
Source/core/dom/URLSearchParams.idl:8: Constructor(DOMString queryString),
Could you add a FIXME:
// FIXME: use ScalarValueString http://crbug.com/379009
(currently in progress by Joshua)

https://codereview.chromium.org/143313002/diff/1180001/Source/core/dom/URLSea...
Source/core/dom/URLSearchParams.idl:10: GarbageCollected,
nit: alpha please

[sof, 2014-06-17 14:24:24]

https://codereview.chromium.org/143313002/diff/1180001/Source/core/dom/URLSea...
File Source/core/dom/URLSearchParams.idl (right):

https://codereview.chromium.org/143313002/diff/1180001/Source/core/dom/URLSea...
Source/core/dom/URLSearchParams.idl:6: Constructor(),
On 2014/06/17 01:24:12, Nils Barth wrote:
> Could you add a FIXME:
> // FIXME: should be (ｈttp://crbug.com/240176):
> // Constructor(optional (ScalarValueString or URLSearchParams) init = ""),

Done.

https://codereview.chromium.org/143313002/diff/1180001/Source/core/dom/URLSea...
Source/core/dom/URLSearchParams.idl:8: Constructor(DOMString queryString),
On 2014/06/17 01:24:12, Nils Barth wrote:
> Could you add a FIXME:
> // FIXME: use ScalarValueString http://crbug.com/379009
> (currently in progress by Joshua)

Thanks, I'm aware of that CL (and the spec IDL here being a prototypical user of
it); will rebase and substitute type names when it lands. And follow up on this
comment with a "Done."

(No risk of this landing first.)

https://codereview.chromium.org/143313002/diff/1180001/Source/core/dom/URLSea...
Source/core/dom/URLSearchParams.idl:19: [ImplementedAs=setNameValue] void
set(DOMString name, DOMString value);
On 2014/06/17 01:19:28, Nils Barth wrote:
> On 2014/06/16 11:27:43, sof wrote:
> > On 2014/06/16 01:00:56, haraken wrote:
> > > 
> > > I guess you're a bit overly using [ImplementedAs].
append/get/getAll/has/set
> > > won't need [ImplementedAs].
> > 
> > Maybe so, but they're not too informative as C++ method names.
> 
> As a rule we avoid [ImplementedAs] unless strictly necessary, to align with
> spec;
> with naming the parameters, at least the function declarations are clear:
> bool has(const String& name) const;
> void set(const String& name, const String& value);
> 
> ...and seem less redundant than:
> bool hasName(const String&) const;
> void setNameValue(const String& name, const String& value);

Won't belabour; dropped suffixes where possible.

[Nils Barth (inactive), 2014-06-18 03:01:44]

thanks, LGTM IDL-wise!

[sof, 2014-06-23 21:54:03]

https://codereview.chromium.org/143313002/diff/1180001/Source/core/dom/URLSea...
File Source/core/dom/URLSearchParams.idl (right):

https://codereview.chromium.org/143313002/diff/1180001/Source/core/dom/URLSea...
Source/core/dom/URLSearchParams.idl:8: Constructor(DOMString queryString),
On 2014/06/17 14:24:24, sof wrote:
> On 2014/06/17 01:24:12, Nils Barth wrote:
> > Could you add a FIXME:
> > // FIXME: use ScalarValueString http://crbug.com/379009
> > (currently in progress by Joshua)
> 
> Thanks, I'm aware of that CL (and the spec IDL here being a prototypical user
of
> it); will rebase and substitute type names when it lands. And follow up on
this
> comment with a "Done."
> 
> (No risk of this landing first.)

Rebased & switched URLSearchParams to use ScalarValueString instead of
DOMString.

[PhistucK, 2014-09-13 14:58:21]

What else is needed in order to move forward with this?

[PhistucK, 2014-09-13 15:08:39]

phistuck@gmail.com changed reviewers:
+ phistuck@gmail.com

[sof, 2014-09-13 17:54:33]

On 2014/09/13 14:58:21, PhistucK wrote:
> What else is needed in order to move forward with this?

The "what" includes Oilpan, and as this involves Node-derived objects, that
hierarchy has to switch to Oilpan first.

[PhistucK, 2014-09-13 18:00:22]

On 2014/09/13 17:54:33, sof wrote:
> On 2014/09/13 14:58:21, PhistucK wrote:
> > What else is needed in order to move forward with this?
> 
> The "what" includes Oilpan, and as this involves Node-derived objects, that
> hierarchy has to switch to Oilpan first.

Oh, months away, then...
Thank you!

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

Closing, URLSearchParams support (as it is now spec'ed; simpler than the initial
design) landed as:

 https://codereview.chromium.org/1456553002
 https://codereview.chromium.org/1860623002