Anne (:annevk) Reporter
	Description • 9 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.

	Andrea Marchesini [:baku] Assignee
	Comment 1 • 9 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?

	Anne (:annevk) Reporter
	Comment 2 • 9 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.

	Andrea Marchesini [:baku] Assignee
	Comment 4 • 9 years ago

A couple of bug IDs updated.

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 9 years ago

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?

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 9 years ago

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.

	Anne (:annevk) Reporter
	Comment 7 • 9 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.

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 9 years ago

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.

	Pulsebot
	Comment 11 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/058e26487efa
https://hg.mozilla.org/integration/mozilla-inbound/rev/50813b4c64c1

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 12 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/058e26487efa
https://hg.mozilla.org/mozilla-central/rev/50813b4c64c1

	Kohei Yoshino
	Comment 13 • 9 years ago

Posted the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/searchparams-is-now-only-on-url/