Implement URLSearchParams

RESOLVED FIXED in mozilla29

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: baku, Assigned: baku)

Tracking

({dev-doc-needed, feature, relnote})

Trunk
mozilla29
dev-doc-needed, feature, relnote
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(relnote-firefox 29+)

Details

(URL)

Attachments

(3 attachments, 27 obsolete attachments)

17.09 KB, patch
Details | Diff | Splinter Review
8.23 KB, patch
Details | Diff | Splinter Review
59.21 KB, patch
smaug
: review+
Details | Diff | Splinter Review
The spec seems a bit unfinished yet but this is needed for URL API - bug 887364
Depends on: 887364
Created attachment 769597 [details] [diff] [review]
patch 1 - URLQuery on main thread
Attachment #769597 - Flags: review?(ehsan)
Created attachment 769598 [details] [diff] [review]
patch 2 - URLQuery in workers
Attachment #769598 - Flags: review?(bent.mozilla)
Attachment #769598 - Flags: feedback?(ehsan)
Created attachment 769599 [details] [diff] [review]
patch 3 - URLQuery in URL API
Attachment #769599 - Flags: review?(ehsan)
Created attachment 769758 [details] [diff] [review]
patch 1 - URLQuery on main thread
Attachment #769597 - Attachment is obsolete: true
Attachment #769597 - Flags: review?(ehsan)
Attachment #769758 - Flags: review?(ehsan)
Created attachment 769759 [details] [diff] [review]
patch 3 - URLQuery in URL API
Attachment #769599 - Attachment is obsolete: true
Attachment #769599 - Flags: review?(ehsan)
Attachment #769759 - Flags: review?(ehsan)
Attachment #769759 - Flags: review?(bent.mozilla)
Assignee: nobody → amarchesini
Comment on attachment 769758 [details] [diff] [review]
patch 1 - URLQuery on main thread

Review of attachment 769758 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/URLQuery.cpp
@@ +76,5 @@
> +  aInput.EndReading(end);
> +  nsAString::const_iterator iter(start);
> +
> +  while (start != end) {
> +    nsString string;

Please use nsAutoString for strings living on the stack.  (There are other instances of this further down.)

@@ +127,5 @@
> +
> +  nsTArray<nsString> *newArray = new nsTArray<nsString>();
> +  for (uint32_t i = 0, len = aArray->Length(); i < len; ++i) {
> +    newArray->AppendElement(aArray->ElementAt(i));
> +  }

Please use nsTArray::AppendElements instead of rolling your own here.

@@ +155,5 @@
> +  }
> +
> +  for (uint32_t i = 0, len = array->Length(); i < len; ++i) {
> +    aRetval.AppendElement(array->ElementAt(i));
> +  }

Same here.

::: dom/base/test/test_urlQuery.html
@@ +82,5 @@
> +      { input: 'a=b&c=d&', data: { 'a' : ['b'], 'c' : ['d'] } },
> +      { input: '&&&a=b&&&&c=d&', data: { 'a' : ['b'], 'c' : ['d'] } },
> +      { input: 'a=a&a=b&a=c', data: { 'a' : ['a', 'b', 'c'] } },
> +      { input: 'a==a', data: { 'a' : ['=a'] } },
> +      { input: 'a=a+b+c+d', data: { 'a' : ['a b c d'] } },

Can you please add a case which tests the presence of names with '+' in them, and also test if the right thing happens if there are multiple name/value pairs, some of them with '+' in their name?
Attachment #769758 - Flags: review?(ehsan) → review+
Comment on attachment 769598 [details] [diff] [review]
patch 2 - URLQuery in workers

Review of attachment 769598 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/URLQuery.h
@@ +18,5 @@
>  namespace mozilla {
>  
>  namespace dom {
>  
> +class IURLQuery

Please call this URLQueryBase or something.  IURLQuery looks an awful lot like an MSCOM interface name!

::: dom/workers/test/urlQuery_worker.js
@@ +76,5 @@
> +      { input: 'a=b&c=d&', data: { 'a' : ['b'], 'c' : ['d'] } },
> +      { input: '&&&a=b&&&&c=d&', data: { 'a' : ['b'], 'c' : ['d'] } },
> +      { input: 'a=a&a=b&a=c', data: { 'a' : ['a', 'b', 'c'] } },
> +      { input: 'a==a', data: { 'a' : ['=a'] } },
> +      { input: 'a=a+b+c+d', data: { 'a' : ['a b c d'] } },

Same comment about the test here as the other patch.
Attachment #769598 - Flags: feedback?(ehsan) → feedback+
Comment on attachment 769759 [details] [diff] [review]
patch 3 - URLQuery in URL API

Review of attachment 769759 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the below addressed, especially the comment about [Creator].

::: dom/base/URL.cpp
@@ +410,5 @@
> +  }
> +
> +  mQuery->CopyFromURLQuery(*aQuery);
> +
> +  nsString search;

nsAutoString...

::: dom/base/URLQuery.cpp
@@ +153,5 @@
>  
>  void
> +IURLQuery::SetObserver(IURLQueryObserver* aObserver)
> +{
> +  mObserver = aObserver;

Please assert that you don't have an observer before this.

@@ +250,5 @@
>    delete aArray;
>    return PL_DHASH_NEXT;
>  }
>  
> +class SerializeData

Please mark this as MOZ_STACK_CLASS.

@@ +257,5 @@
> +  SerializeData()
> +    : mFirst(true)
> +  {}
> +
> +  nsString mValue;

nsAutoString.

@@ +258,5 @@
> +    : mFirst(true)
> +  {}
> +
> +  nsString mValue;
> +  bool mFirst;

I'm not sure what uses mFirst here.  It seems like we never use its value to decide anything...

@@ +276,5 @@
> +                 (*start >= 0x61 && *start <= 0x7A)) {
> +        mValue.Append(*start);
> +      } else {
> +        mValue.AppendPrintf("%%%X", *start);
> +      }

Please add some comments about the hex magic going on here.

::: dom/base/URLQuery.h
@@ +18,5 @@
>  namespace mozilla {
>  
>  namespace dom {
>  
> +class IURLQueryObserver

Nit: URLQueryObserver.

::: dom/webidl/URLUtils.webidl
@@ +27,5 @@
>             attribute DOMString port;
>             attribute DOMString pathname;
>             attribute DOMString search;
> +           [Creator]
> +           attribute URLQuery? query;

Why did you mark this as Creator?  Creator should only be used on getters that return a new object each time they're called.
Attachment #769759 - Flags: review?(ehsan) → review+
Created attachment 770665 [details] [diff] [review]
patch 1 - URLQuery on main thread
Attachment #769758 - Attachment is obsolete: true
Created attachment 770666 [details] [diff] [review]
patch 1 - URLQuery on main thread - InterDiff

Can you review this interdiff? It contains the decode % part that was missing in the first patch.
Attachment #770666 - Flags: review?(ehsan)
Created attachment 770702 [details] [diff] [review]
patch 3 - URLQuery in URL API

In order to remove the [creator] I had to make URLQuery a nsWrapperCache.
Attachment #769759 - Attachment is obsolete: true
Attachment #769759 - Flags: review?(bent.mozilla)
Attachment #770702 - Flags: review?(bent.mozilla)
Created attachment 770703 [details] [diff] [review]
patch 2 - URLQuery in workers
Attachment #769598 - Attachment is obsolete: true
Attachment #769598 - Flags: review?(bent.mozilla)
Attachment #770703 - Flags: review?(bent.mozilla)
Comment on attachment 770666 [details] [diff] [review]
patch 1 - URLQuery on main thread - InterDiff

Review of attachment 770666 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/URLQuery.cpp
@@ +140,5 @@
> +      nsAString::const_iterator second(first);
> +      ++second;
> +
> +#define ASCII_HEX_DIGIT( x )        \
> +   (x != end &&                     \

Nit: please take this check out of the macro.
Attachment #770666 - Flags: review?(ehsan) → review+
Created attachment 770843 [details] [diff] [review]
patch 1 - URLQuery on main thread
Attachment #770665 - Attachment is obsolete: true
Attachment #770666 - Attachment is obsolete: true
Keywords: dev-doc-needed
Attachment #770702 - Flags: review?(bent.mozilla) → review?(khuey)
Comment on attachment 770703 [details] [diff] [review]
patch 2 - URLQuery in workers

Hopefully khuey can grab these.
Attachment #770703 - Flags: review?(bent.mozilla) → review?(khuey)
Comment on attachment 770703 [details] [diff] [review]
patch 2 - URLQuery in workers

Review of attachment 770703 [details] [diff] [review]:
-----------------------------------------------------------------

This should also be updated to use cycle collection.
Attachment #770703 - Flags: review?(khuey) → review-
Comment on attachment 770702 [details] [diff] [review]
patch 3 - URLQuery in URL API

Review of attachment 770702 [details] [diff] [review]:
-----------------------------------------------------------------

Canceling this since I r-d everything it depends on.
Attachment #770702 - Flags: review?(khuey)
Created attachment 792938 [details] [diff] [review]
patch 1 - URLQuery on main thread

rebased
Attachment #770843 - Attachment is obsolete: true
Created attachment 792941 [details] [diff] [review]
patch 2 - URLQuery in workers

it works, but it can be probably implemented differently.
Attachment #770703 - Attachment is obsolete: true
Attachment #792941 - Flags: feedback?(khuey)
Created attachment 792942 [details] [diff] [review]
patch 3 - URLQuery in URL API
Attachment #770702 - Attachment is obsolete: true
Attachment #792942 - Flags: feedback?(khuey)

Comment 21

4 years ago
I'm considering a few tweaks since the current API is a bit weird: http://lists.w3.org/Archives/Public/public-script-coord/2013JulSep/0574.html
Created attachment 818253 [details] [diff] [review]
patch 2 - URLQuery in workers
Attachment #792941 - Attachment is obsolete: true
Attachment #792941 - Flags: feedback?(khuey)
Attachment #818253 - Flags: review?(khuey)
Created attachment 824590 [details] [diff] [review]
patch 1 - URLQuery on main thread
Attachment #792938 - Attachment is obsolete: true
Attachment #824590 - Flags: review?(ehsan)
Created attachment 824591 [details] [diff] [review]
patch 2 - URLQuery in workers
Attachment #818253 - Attachment is obsolete: true
Attachment #818253 - Flags: review?(khuey)
Attachment #824591 - Flags: review?(khuey)
Created attachment 824592 [details] [diff] [review]
patch 3 - URLQuery in URL API
Attachment #792942 - Attachment is obsolete: true
Attachment #792942 - Flags: feedback?(khuey)
Attachment #824592 - Flags: review?(khuey)
https://tbpl.mozilla.org/?tree=Try&rev=b33e7a352e99
What spec is this in?  I don't see it in the URL spec anymore.

Comment 28

4 years ago
It was renamed because query created a conflict with future extensions to the DOM.
Summary: Implement URLQuery → Implement URLSearchParams
I'll submit a patch for renaming it.
Created attachment 826834 [details] [diff] [review]
patch 4 - renaming URLQuery to URLSearchParams
Attachment #826834 - Flags: review?(ehsan)
Comment on attachment 824590 [details] [diff] [review]
patch 1 - URLQuery on main thread

Review of attachment 824590 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/URLQuery.cpp
@@ +234,5 @@
> +URLQuery::DeleteEnumerator(const nsAString& aName, nsTArray<nsString>* aArray,
> +                           void *userData)
> +{
> +  delete aArray;
> +  return PL_DHASH_NEXT;

PL_DHASH_REMOVE.

::: dom/base/URLQuery.h
@@ +6,5 @@
> +#ifndef mozilla_dom_URLQuery_h
> +#define mozilla_dom_URLQuery_h
> +
> +#include "mozilla/dom/BindingDeclarations.h"
> +#include "mozilla/dom/URLQueryBinding.h"

You shouldn't need this include in the header.

@@ +7,5 @@
> +#define mozilla_dom_URLQuery_h
> +
> +#include "mozilla/dom/BindingDeclarations.h"
> +#include "mozilla/dom/URLQueryBinding.h"
> +#include "nsCycleCollectionParticipant.h"

Or this one.

@@ +10,5 @@
> +#include "mozilla/dom/URLQueryBinding.h"
> +#include "nsCycleCollectionParticipant.h"
> +#include "nsDataHashtable.h"
> +#include "nsHashKeys.h"
> +#include "nsAutoPtr.h"

Or this one, afaict.  You might have to forward declare already_AddRefed if you remove this.

@@ +72,5 @@
> +  static PLDHashOperator
> +  DeleteEnumerator(const nsAString& aName, nsTArray<nsString>* aArray,
> +                   void *userData);
> +
> +  nsDataHashtable<nsStringHashKey, nsTArray<nsString>* > mQuery;

but would it make more sense for this to be nsAutoPtr<nsTArray<nsString>>?
Comment on attachment 824591 [details] [diff] [review]
patch 2 - URLQuery in workers

Review of attachment 824591 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/WorkerScope.cpp
@@ +1481,5 @@
>        !TextEncoderBinding::GetConstructorObject(aCx, global) ||
>        !XMLHttpRequestBinding_workers::GetConstructorObject(aCx, global) ||
>        !XMLHttpRequestUploadBinding_workers::GetConstructorObject(aCx, global) ||
>        !URLBinding_workers::GetConstructorObject(aCx, global) ||
> +      !URLQueryBinding::GetConstructorObject(aCx, global) ||

Isn't this beautiful?
Attachment #824591 - Flags: review?(khuey) → review+
Created attachment 827351 [details] [diff] [review]
patch 1 - URLQuery on main thread
Attachment #824590 - Attachment is obsolete: true
Attachment #824590 - Flags: review?(ehsan)
Attachment #827351 - Flags: review?(ehsan)
Created attachment 827354 [details] [diff] [review]
patch 3 - URLQuery in URL API
Attachment #824592 - Attachment is obsolete: true
Attachment #824592 - Flags: review?(khuey)
Attachment #827354 - Flags: review?(ehsan)
Created attachment 827355 [details] [diff] [review]
patch 4 - renaming URLQuery to URLSearchParams
Attachment #826834 - Attachment is obsolete: true
Attachment #826834 - Flags: review?(ehsan)
Attachment #827355 - Flags: review?(ehsan)
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #32)
> Comment on attachment 824591 [details] [diff] [review]
> patch 2 - URLQuery in workers
> 
> Review of attachment 824591 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/workers/WorkerScope.cpp
> @@ +1481,5 @@
> >        !TextEncoderBinding::GetConstructorObject(aCx, global) ||
> >        !XMLHttpRequestBinding_workers::GetConstructorObject(aCx, global) ||
> >        !XMLHttpRequestUploadBinding_workers::GetConstructorObject(aCx, global) ||
> >        !URLBinding_workers::GetConstructorObject(aCx, global) ||
> > +      !URLQueryBinding::GetConstructorObject(aCx, global) ||
> 
> Isn't this beautiful?

It's actually quite cool. Yes.
Comment on attachment 827351 [details] [diff] [review]
patch 1 - URLQuery on main thread

Review of attachment 827351 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing the review request because of the spec changes noted below.  Please check with Anne to make sure the spec is stable before spending time to fix your patch!

Thanks

::: dom/webidl/URLQuery.webidl
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + *
> + * The origin of this IDL file is
> + * http://url.spec.whatwg.org/#urlquery

Please update the URL.

@@ +22,5 @@
> +  void append(DOMString name, DOMString value);
> +  boolean has(DOMString name);
> +  void delete(DOMString name);
> +  readonly attribute unsigned long size;
> +};

It seems like the IDL has changed since you wrote this patch (for example, the size attribute doesn't exist any more, the ctor uses a union, and it also uses [EnsureUTF16] now.
Attachment #827351 - Flags: review?(ehsan)
Comment on attachment 827355 [details] [diff] [review]
patch 4 - renaming URLQuery to URLSearchParams

Review of attachment 827355 [details] [diff] [review]:
-----------------------------------------------------------------

I'd really prefer if you made this change inside the rest of the patches in your queue.  Running sed on each one should be easy enough, please let me know if there is a good reason why this is a separate patch.
Attachment #827355 - Flags: review?(ehsan)
Comment on attachment 827354 [details] [diff] [review]
patch 3 - URLQuery in URL API

Review of attachment 827354 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/Link.cpp
@@ +573,5 @@
> +Link::SetQuery(URLQuery* aQuery)
> +{
> +  if (!aQuery) {
> +    return;
> +  }

This is wrong too.

::: dom/base/URL.cpp
@@ +411,5 @@
> +URL::SetQuery(URLQuery* aQuery)
> +{
> +  if (!aQuery) {
> +    return;
> +  }

This is wrong, isn't it?  You should be able to successfully set the query object to null from js.  You should also add a test for this.

::: dom/base/URLQuery.cpp
@@ +268,5 @@
>  {
>    mQuery.Clear();
>  }
>  
> +class SerializeData MOZ_STACK_CLASS

You want:

class MOZ_STACK_CLASS SerializeData

::: dom/base/URLQuery.h
@@ +94,3 @@
>    nsClassHashtable<nsStringHashKey, nsTArray<nsString>> mQuery;
> +
> +  URLQueryObserver* mObserver;

What guarantees this pointer to always be valid?  For example, if you hold a reference to the URLQuery object from script and then let GC/CC kill the C++ URL object then this will become a dangling pointer, right?

::: dom/bindings/Configuration.py
@@ +166,5 @@
>                  return d
>  
> +        if workers:
> +            for d in self.descriptorsByName[interfaceName]:
> +                return d

This should be reviewed by Kyle or Boris.

::: dom/tests/mochitest/general/test_interfaces.html
@@ +553,5 @@
>      "TreeWalker",
>      "UIEvent",
>      "UndoManager",
>      "URL",
> +    "URLQuery",

This needs review from a DOM peer as well.
Attachment #827354 - Flags: review?(ehsan) → review-

Comment 40

4 years ago
The current specification is relatively stable. I would support shipping it unprefixed. We will add enumeration and such going forward, but those parts of ES6 are not stable yet so they are left out, which is also why size was removed.
> It seems like the IDL has changed since you wrote this patch (for example,
> the size attribute doesn't exist any more, the ctor uses a union, and it
> also uses [EnsureUTF16] now.

The union constructor and [EnsureUTF16] were not supported yet by our paris-bindings.
This is a follow up.
> ::: dom/base/URL.cpp
> @@ +411,5 @@
> > +URL::SetQuery(URLQuery* aQuery)
> > +{
> > +  if (!aQuery) {
> > +    return;
> > +  }
> 
> This is wrong, isn't it?  You should be able to successfully set the query
> object to null from js.  You should also add a test for this.

The spec doesn't say so. This is not allowed. There is a test about this.
(In reply to comment #42)
> > ::: dom/base/URL.cpp
> > @@ +411,5 @@
> > > +URL::SetQuery(URLQuery* aQuery)
> > > +{
> > > +  if (!aQuery) {
> > > +    return;
> > > +  }
> > 
> > This is wrong, isn't it?  You should be able to successfully set the query
> > object to null from js.  You should also add a test for this.
> 
> The spec doesn't say so. This is not allowed. There is a test about this.

I see, I stand corrected then!
Created attachment 8342465 [details] [diff] [review]
patch 1 - URLSearchParams on main thread
Attachment #827351 - Attachment is obsolete: true
Attachment #827355 - Attachment is obsolete: true
Created attachment 8342466 [details] [diff] [review]
patch 2 - URLSearchParams workers
Attachment #824591 - Attachment is obsolete: true
Created attachment 8342468 [details] [diff] [review]
patch 3 - URLSearchParams in URL API
Attachment #827354 - Attachment is obsolete: true
Attachment #8342468 - Flags: review?(ehsan)
Attachment #8342468 - Flags: review?(bzbarsky)
Comment on attachment 8342468 [details] [diff] [review]
patch 3 - URLSearchParams in URL API

Review of attachment 8342468 [details] [diff] [review]:
-----------------------------------------------------------------

(It's usually *very* helpful if you can submit an interdiff, especially for patches which have been reviewed before so that the reviewer can only look at the changes and not everything from scratch.)

::: content/base/src/Link.h
@@ +61,5 @@
>    void SetHost(const nsAString &aHost);
>    void SetHostname(const nsAString &aHostname);
>    void SetPathname(const nsAString &aPathname);
>    void SetSearch(const nsAString &aSearch);
> +  void SetSearchParams(mozilla::dom::URLSearchParams* aSearchParams);

Nit: please drop mozilla::dom.

@@ +134,5 @@
>    bool HasCachedURI() const { return !!mCachedURI; }
>  
> +  // CC methods
> +  void Unlink();
> +  void Traverse(nsCycleCollectionTraversalCallback &cb);

Why are you implementing these methods by hand and not using our CC macros?

::: content/html/content/src/HTMLAnchorElement.cpp
@@ +53,5 @@
> +NS_IMPL_CYCLE_COLLECTION_CLASS(HTMLAnchorElement)
> +
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(HTMLAnchorElement,
> +                                                  nsGenericHTMLElement)
> +  tmp->Link::Traverse(cb);

Hmm, please ask smaug/mccr8 to see if there is a better way of doing this.

::: content/html/content/src/HTMLAreaElement.h
@@ +160,5 @@
>  
> +  URLSearchParams* GetSearchParams()
> +  {
> +    return Link::GetSearchParams();
> +  }

Couldn't you just use the base class version of these methods?  I'm not sure why you need to define these two methods here.

@@ +162,5 @@
> +  {
> +    return Link::GetSearchParams();
> +  }
> +
> +  void SetSearchParams(mozilla::dom::URLSearchParams* aSearchParams)

Nit: please drop mozilla::dom.

::: dom/base/URLSearchParams.h
@@ +22,5 @@
>  public:
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_CLASS(URLSearchParamsObserver)
> +
> +  virtual ~URLSearchParamsObserver() {}

Why do you need a virtual destructor?  It seems to me like this object is destroyed through its Release() method which knows about the concrete type of the object, so a virtual destructor is not necessary.
Attachment #8342468 - Flags: review?(ehsan) → feedback+
Comment on attachment 8342468 [details] [diff] [review]
patch 3 - URLSearchParams in URL API

>+++ b/content/base/src/Link.cpp

Do we really need to force creation of mSearchParams in SetSearch?  Couldn't we just do it like so:

  if (mSearchParams) {
    mSearchParams->ParseInput(aSearch);
  }

and not worry about forcing eager creation of the search params object?

Note that when we _do_ create the search params object we need to make sure that it's inited based on our URI, of course.  I don't see what ensures that in this patch, actually; what makes sure that GetSearchParams() returns an object that has the right data in it already?

>+Link::SetSearchParams(URLSearchParams* aSearchParams)

This doesn't seem to do what the spec says.  The spec says that if aSearchParams has no URL associated with it to just use it, else to clone.

Also, this codepath ends up copying aSearchParams into mSearchParams, then serializing mSearchParams, then calling SetSearch, which will reparse mSearchParams, no?

URLSearchParamsUpdated has a similar issue, afaict.

>+++ b/content/html/content/src/HTMLAreaElement.h

You don't need to add explicit forwards to the Link methods here, afaict.

>+++ b/dom/base/URL.cpp
>+URL::SetSearchParams(URLSearchParams* aSearchParams)

This has the same issue as the Link version.

And again, the serialize and then reparse thing here is a bit odd.

>+++ b/dom/base/URLSearchParams.cpp
>+  return mozilla::dom::URLSearchParamsBinding::Wrap(aCx, aScope, this);

Why the explicit prefixing?


>+URLSearchParams::SerializeEnumerator(const nsAString& aName, nsTArray<nsString>* aArray,
>+                              void *userData)

Fix indent?

>+++ b/dom/base/URLSearchParams.h
>+class URLSearchParamsObserver : public nsISupports
>+  NS_DECL_CYCLE_COLLECTING_ISUPPORTS

No.  This is very very bad.  This will give elements inheriting from this class _two_ CC refcounts.

There is no readson to make this class cycle collected or to give it a refcount.  In fact, it's really just an interface, right?  So just give it the pure virtual URLSearchParamsUpdated method and nothing else.  Then URL will implment (non-inherited!) CC and refcounting, and nodes will implement theirs as they already do.

This part really needs to be fixed.

One other spec question.  The spec sure sounds like query object can be null.  How can that happen?  That would enable me to determine whether the behavior of the setter at http://url.spec.whatwg.org/#dom-url-searchparams make sense in the cases when query object is null.
Attachment #8342468 - Flags: review?(bzbarsky) → review-
Created attachment 8342947 [details] [diff] [review]
patch 3 - URLSearchParams in URL API

Thanks for the review. Now the code looks cleaner. What about this patch?
Attachment #8342468 - Attachment is obsolete: true
Attachment #8342947 - Flags: review?(bzbarsky)
Comment on attachment 8342947 [details] [diff] [review]
patch 3 - URLSearchParams in URL API

>+Link::SetSearchParams(URLSearchParams* aSearchParams)
>+    mSearchParams = aSearchParams;

Do we not need to SetObserver(this) here?  I think we do.  Please add a test for this.

>+++ b/content/html/content/src/HTMLAreaElement.h

You don't need to add explicit forwards to the Link methods here, afaict.  Still.

>+URL::SetSearchParams(URLSearchParams* aSearchParams)
>+    mSearchParams = aSearchParams;

Again, need SetObserver().

Two more issues I just realized:

1)  What updates the URLSearchParames when setting .href?  Or just setAttribute on the DOM node?  Nothing as far as I see.  Need tests.

2)  The handling of %-escaping looks weird to me.  We're doing it on PRUnichars, but %-escaping really needs to happen on bytes.  Presumably somewhere in there we should be converting to UTF-8 or something?  In fact, http://url.spec.whatwg.org/#concept-urlencoded-serializer (which is linked to from <http://url.spec.whatwg.org/#concept-uq-update>) pretty explicitly says to convert to UTF-8 before applying http://url.spec.whatwg.org/#concept-urlencoded-byte-serializer as far as I can tell.
Attachment #8342947 - Flags: review?(bzbarsky) → review-
Created attachment 8344158 [details] [diff] [review]
patch 3 - URLSearchParams in URL API
Attachment #8342947 - Attachment is obsolete: true
Attachment #8344158 - Flags: review?(bzbarsky)
Created attachment 8344159 [details] [diff] [review]
interdiff for patch 3
Attachment #8344159 - Flags: review?(bzbarsky)
Comment on attachment 8344159 [details] [diff] [review]
interdiff for patch 3

This will cause ResetLinkState to eagerly create the nsIURI, but we want to avoid doing that if at all possible...

Better would be to have a way to mark the URLSearchParams as out-of-date and then have it ask to be updated if it's out-of-date and someone wants something from it, perhaps?

It looks like you changed ParseInput to work on UTF-8.  That might even be the right thing (though in that case imo we should just make ParseInput take a UTF-8 string instead of having callers convert from UTF-8 to UTF-16 and having it convert back).  I haven't checked the spec to see whether it specifies the behavior here and whether your change is actually black-box detectable (if it is, add a test?).  But what I was talking about with %-escaping wasn't ParseInput but Serialize().  Right now it will happily encode UTF-16 code units as a single %-escape, which is not what it should be doing.
Attachment #8344159 - Flags: review?(bzbarsky) → review-
Attachment #8344158 - Flags: review?(bzbarsky) → review-
Created attachment 8346232 [details] [diff] [review]
patch 3 - URLSearchParams in URL API
Attachment #8344158 - Attachment is obsolete: true
Attachment #8344159 - Attachment is obsolete: true
Attachment #8346232 - Flags: review?(bzbarsky)
Comment on attachment 8346232 [details] [diff] [review]
patch 3 - URLSearchParams in URL API

I'm confused a bit.  I just realized that attachment 8344158 [details] [diff] [review] does not correspond to the interdiff from attachment 8344159 [details] [diff] [review].  What _does_ it correspond to?

Can I just diff this latest attachment against something to get a list of the changes that I really need to review?  I'll assume diffing against attachment 8344158 [details] [diff] [review] is a diff against something I already reviewed...

> URL::URLSearchParamsNeedsUpdates()

Should still PerseInput even if search is empty, no?  Otherwise the URLSearchParams will just keep asking....  and will fail its "I'm now valid" assert.  This applies to both URL implementations.

r=me with that fixed, I think.
Attachment #8346232 - Flags: review?(bzbarsky) → review+
Created attachment 8346554 [details] [diff] [review]
patch 3 - URLSearchParams in URL API
Attachment #8346232 - Attachment is obsolete: true
Attachment #8346554 - Flags: review?(bugs)
Created attachment 8346555 [details] [diff] [review]
patch 3 - interdiff
Attachment #8346555 - Flags: review?(bugs)
Comment on attachment 8346555 [details] [diff] [review]
patch 3 - interdiff


>-NS_IMPL_CYCLE_COLLECTING_ADDREF(URL)
>-NS_IMPL_CYCLE_COLLECTING_RELEASE(URL)
>+// Here we use workers::URL to avoid conflicts in GC between this object and
>+// URL on the main-thread.
>+NS_IMPL_CYCLE_COLLECTING_ADDREF(workers::URL)
>+NS_IMPL_CYCLE_COLLECTING_RELEASE(workers::URL)

The comment is wrong. The reason for using worker::URL is to have different refcnt logging than for
main thread URL.
Attachment #8346555 - Flags: review?(bugs) → review+
Comment on attachment 8346554 [details] [diff] [review]
patch 3 - URLSearchParams in URL API

Assuming interdiff was ok,
Attachment #8346554 - Flags: review?(bugs) → review+
https://tbpl.mozilla.org/?tree=Try&rev=de77d53ba5cb

just to be sure before landing it.
Attachment #8346555 - Attachment is obsolete: true
Keywords: checkin-needed
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0e3beadca4ba
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/058037e3baf4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f97f09f7c75b
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0e3beadca4ba
https://hg.mozilla.org/mozilla-central/rev/058037e3baf4
https://hg.mozilla.org/mozilla-central/rev/f97f09f7c75b
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29

Updated

4 years ago
Depends on: 951230

Updated

4 years ago
Flags: in-testsuite?
Does this need any more automation coverage?
Flags: needinfo?(amarchesini)
This patch contains mochitests. But maybe I don't understand the question. To me is in-testsuite+
Flags: needinfo?(amarchesini)
Flags: in-testsuite? → in-testsuite+
This is awesome for any single page application that uses query parameters. Too bad it didn't make the release notes.
relnote-firefox: --- → ?
Keywords: feature
Keywords: relnote
Not too late - have added this to the notes:

Implemented URLSearchParams from the URL specification (see [MDN][1] for details )


  [1]: http://Implemented%20URLSearchParams%20from%20the%20URL%20specification


Just fyi - next time there's something note-worthy this close to release date, ping Release Managers in IRC to make sure they see the request, we tend to stop triaging relnote-firefox? once the notes are out for review (~4 days prior to release).
relnote-firefox: ? → 29+
The url for that link is https://developer.mozilla.org/en-US/docs/Web/API/URLUtils - not what's in comment 66
(In reply to Lukas Blakk [:lsblakk] from comment #67)
> The url for that link is
> https://developer.mozilla.org/en-US/docs/Web/API/URLUtils - not what's in
> comment 66

Right. But this page is not up-to-date.
URLUtils has:

           attribute URLSearchParams searchParams;

that is what this bug is about.
Andrea, it's a wiki.  Mind just updating it?
Done. I also created a draft for URLSearchParams.
I created https://developer.mozilla.org/en-US/docs/Web/API/URLUtils.searchParams with examples and also improved https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams
Keywords: dev-doc-needed → dev-doc-complete
Switching back to dev-doc-needed because we have a (nice) stub but not the whole set of docs. (And we need to have it on our radar even if we don't act on it quickly)
Keywords: dev-doc-complete → dev-doc-needed

Updated

3 years ago
Depends on: 1085284
You need to log in before you can comment on or make changes to this bug.