1037715 - Implement .searchParams on Location

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

I didn't do it in bug 832014, because updating it on navigation is a bit of a PITA.  :(

Comment 1

[Andrea Marchesini [:baku]]

Attachment: location.patch

This patch doesn't have any mochitest, yet.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8463339 [details] [diff] [review]
location.patch

This doesn't update the searchParams when navigating the via link click or document.open or meta refresh or all the other methods that don't involve going through Location to trigger the navigation.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: location.patch

In this patch URLSearchParams is owned by nsDocShell.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: location.patch

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8464160 [details] [diff] [review]
location.patch

>@@ -9703,16 +9704,34 @@ nsDocShell::InternalLoad(nsIURI * aURI,

This seems like the wrong place to update mURLSearchParams.  I mean, this load might not even happen: the user might immediately hit the stop button.

You probably want to do this when the docshell's URI actually changes, no?

>+nsISupports*
>+nsDocShell::GetURLSearchParams()

Why not just make this return mozilla::dom::URLSearchParams*?  Seems like that would make for a much nicer API.  See imgIContainer.getFrame for an example of how to do that.

>+    nsRefPtr<URLSearchParams> docShellSearchParams = tmp->DocShellSearchParams();
>+    if (docShellSearchParams) {

If it can return null, it should be called GetDocShellSearchParams.

I guess it returning null is OK because that means the docshell is dead and hence so is its search params and so it's not pointing to us anymore?  If so, worth documenting.  But also, shouldn't the removal from the docshell params be in the dtor?

>   if (!CallerSubsumes())

Document that this is here to cover navigation attempts via the SearchParams too?  And make sure the spec says that, and if not, file spec bugs.

>+    MOZ_ASSERT(searchParams);

Why?  I think it can totally be null here if the docshell is already dead.

>+  MOZ_ASSERT(aSearchParams == searchParams);

Why not just use aSearchParams then, instead of calling DocShellSearchParams()?

>+    // Just to avoid wrong data if the GetURI fails.
>+    mSearchParams = nullptr;

No, that's not OK, since nsLocation::SearchParams is promising to return non-null.

I didn't read the test very carefully.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: interdiff

Interdiff: I'm not 100% sure about the correct place in nsDocShell. I'm using SetCurrentURI...

Comment 7

[Andrea Marchesini [:baku]]

Attachment: location.patch

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8466142 [details] [diff] [review]
interdiff

Yes, this looks great.  SetCurrentURI is correct, since Location uses GetCurrentURI to get the URI it works with.

Comment 9

[Andrea Marchesini [:baku]]

Attachment: patch 2

Ehsan, can you review this code? This is the continuation of location.URLSearchParams and it fixes an issue when location is deleted after its nsDocShell. In this case without this patch we leak a URLSearchParams.

This patch does this:

1. in the dtor of nsDocShell, if there is a URLSearchParams object, all its observers are unregistered.

2. nsDocShell::GetURLSearchParams can return null, so that nsLocation::Unlink doesn't crash if nsDocShell is already gone, or partially gone.

Green on try: https://tbpl.mozilla.org/?tree=Try&rev=3c0428954046

Comment 10

[Andrea Marchesini [:baku]]

Attachment: patch

I just merged the 2 patches in 1.

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b035f87f26a

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9b035f87f26a

Comment 13

[Jean-Yves Perrier [:teoli]]

Doc updated:
https://developer.mozilla.org/en-US/docs/Web/API/Location
(added URLUtils.searchParams that was already existing)
and
https://developer.mozilla.org/en-US/Firefox/Releases/34