Implement .searchParams on Location

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
5 years ago
4 months ago

People

(Reporter: bzbarsky, Assigned: baku)

Tracking

({dev-doc-complete})

unspecified
mozilla34
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox34 disabled, firefox35 disabled, firefox36 disabled)

Details

Attachments

(2 attachments, 5 obsolete attachments)

I didn't do it in bug 832014, because updating it on navigation is a bit of a PITA.  :(
Posted patch location.patch (obsolete) — Splinter Review
This patch doesn't have any mochitest, yet.
Attachment #8463339 - Flags: review?(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.
Attachment #8463339 - Flags: review?(bzbarsky) → review+
Posted patch location.patch (obsolete) — Splinter Review
In this patch URLSearchParams is owned by nsDocShell.
Attachment #8463339 - Attachment is obsolete: true
Attachment #8463974 - Flags: review?(bzbarsky)
Posted patch location.patch (obsolete) — Splinter Review
Attachment #8463974 - Attachment is obsolete: true
Attachment #8463974 - Flags: review?(bzbarsky)
Attachment #8464160 - Flags: review?(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.
Attachment #8464160 - Flags: review?(bzbarsky) → review-
Posted patch interdiffSplinter Review
Interdiff: I'm not 100% sure about the correct place in nsDocShell. I'm using SetCurrentURI...
Attachment #8466142 - Flags: review?(bzbarsky)
Posted patch location.patch (obsolete) — Splinter Review
Attachment #8464160 - Attachment is obsolete: true
Attachment #8466143 - Flags: review?(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.
Attachment #8466142 - Flags: review?(bzbarsky) → review+
Attachment #8466143 - Flags: review?(bzbarsky) → review+
Depends on: 1048288
Depends on: 1032511
Posted patch patch 2 (obsolete) — Splinter Review
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
Attachment #8468459 - Flags: review?(ehsan)
Attachment #8468459 - Flags: review?(ehsan) → review+
Posted patch patchSplinter Review
I just merged the 2 patches in 1.
Attachment #8466143 - Attachment is obsolete: true
Attachment #8468459 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9b035f87f26a
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Depends on: 1064481
Depends on: 1082734
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.