searchParams is just on URL

RESOLVED FIXED in Firefox 45

Status

()

defect
RESOLVED FIXED
4 years ago
2 months ago

People

(Reporter: annevk, Assigned: baku)

Tracking

({dev-doc-needed, site-compat})

unspecified
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Reporter

Description

4 years ago
We redesigned the URL API a bit more. Now each of Location, HTMLAnchorElement, HTMLAreaElement, WorkerLocation, and URL, has their own set of members and behavior. It turns out that sharing their definitions was more trouble than needed.
Assignee

Comment 1

4 years ago
A part location, any particular reason why HTMLAnchorElement, HTMLAreaElement and URL should not shared the same interface?
Can you tell me which problem we are going to fix splitting these interfaces?
Flags: needinfo?(annevk)
Reporter

Comment 2

4 years ago
The reason Domenic and I decided on splitting was because they have different implementations. It did not seem worth it to share the interface when the implementation was quite a bit different.

There's probably some refactoring that can be done to share more of the implementation between these APIs, but the old setup was rather broken and had various bugs.
Flags: needinfo?(annevk)
Assignee

Updated

4 years ago
Assignee: nobody → amarchesini
Keywords: dev-doc-needed
Assignee

Updated

4 years ago
Blocks: 1227107
Assignee

Updated

4 years ago
Blocks: 1227110
Assignee

Comment 3

4 years ago
Posted patch url1.patch (obsolete) — Splinter Review
Attachment #8690800 - Flags: review?(bzbarsky)
Assignee

Comment 4

4 years ago
Posted patch url1.patch (obsolete) — Splinter Review
A couple of bug IDs updated.
Attachment #8690800 - Attachment is obsolete: true
Attachment #8690802 - Flags: review?(bzbarsky)
Attachment #8690800 - Flags: review?(bzbarsky)
I thought the whole point of using a mixin here was to avoid having to add a new thing in 4 (or 5 or, however many) places in the spec and have some get missed and get out of sync.  Have we given up on that?
Flags: needinfo?(annevk)
Comment on attachment 8690802 [details] [diff] [review]
url1.patch

Assuming for the moment that we actually want to make this change...

1)  I like all the complexity for URLSearchParams on Link being gone.  I'm just sorry you had to go through the pain of implementing it to start with.  :(  Let that be a lesson to us as far as trusting specs goes.  ;)

2)  All the stuff on URLUtils was marked [Throws] because Location can throw all over.  But now that we're doing separate IDLs for different things, is that still needed.  Please audit all the stuff on URL and HTMLHyperlinkElementUtils and remove the [Throws] where possible.  Followup bug, or separate patch in this bug, is better than adding to this patch, of course.

3)  You lost the CrossOriginWritable annotation on Location.href.  Have you run this through try?  How did it pass if so?  If it does actually pass try (which I doubt), please add tests that would have caught the issue.

4)  In URL.webidl, need those same comments about bug 824857, right?

5)  Please land the change to the argument type of Location.assign()/replace() as a separate changeset, if not a separate bug.  It, unlike most of the other changes here, can have compat fallout and it would be good to have bisects pick it up individually.

r=me with those fixed.
Attachment #8690802 - Flags: review?(bzbarsky) → review+
Reporter

Comment 7

4 years ago
Yes, and I'm also sorry :-(

There were a bunch of things that the specification was describing that didn't actually match what implementations did and rather than designing some new dispatch design we thought it would be better to have distinct classes.

I might at some point introduce some common concepts that these classes can share, but I don't expect us to try and merge them again.
Flags: needinfo?(annevk)
Assignee

Comment 8

4 years ago
Attachment #8690802 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Blocks: 1227206
Comment on attachment 8690896 [details] [diff] [review]
part 2 - Remove [Throws] where it's possible

>+++ b/dom/workers/URL.cpp

The ErrorResult in the MainThreadRun function there (for GetterRunnable, I assume) is now unused, right?

r=me, though the additional bitrot against my patches in bug 1224596 is sad.  Just go ahead and land, since I have no idea when those patches might get reviewed.
Attachment #8690896 - Flags: review?(bzbarsky) → review+

Comment 12

4 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/058e26487efa
https://hg.mozilla.org/mozilla-central/rev/50813b4c64c1
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.