Closed Bug 1037715 Opened 6 years ago Closed 6 years ago

Implement .searchParams on Location

Categories

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

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34
Tracking Status
firefox34 --- disabled
firefox35 --- disabled
firefox36 --- disabled

People

(Reporter: bzbarsky, Assigned: baku)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 5 obsolete files)

I didn't do it in bug 832014, because updating it on navigation is a bit of a PITA.  :(
Attached 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+
Attached 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)
Attached 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-
Attached patch interdiffSplinter Review
Interdiff: I'm not 100% sure about the correct place in nsDocShell. I'm using SetCurrentURI...
Attachment #8466142 - Flags: review?(bzbarsky)
Attached 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
Attached 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+
Attached 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: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Depends on: 1064481
Depends on: 1082734
Blocks: 1105677
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.