Closed
Bug 1094545
Opened 10 years ago
Closed 9 years ago
Expose URLSearchParams in system scopes
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: bzbarsky, Assigned: bzbarsky)
Details
Attachments
(3 files, 1 obsolete file)
4.98 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
2.83 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
13.83 KB,
patch
|
Details | Diff | Splinter Review |
It looks safe to me, and is useful.
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8517860 -
Flags: review?(bobbyholley)
Comment 2•10 years ago
|
||
Comment on attachment 8517860 [details] [diff] [review] Expose the URL API in system globals Review of attachment 8517860 [details] [diff] [review]: ----------------------------------------------------------------- Needs xpcshell-tests. See https://wiki.mozilla.org/WebAPI/WebIDL_Review_Checklist
Attachment #8517860 -
Flags: review?(bobbyholley) → review-
Assignee | ||
Comment 3•10 years ago
|
||
This gives us the same coverage we have for URL in a sandbox. I could also try to refactor test_url.html to allow sharing code between it and this test if we want more extensive coverage
Attachment #8518197 -
Flags: review?(bobbyholley)
Assignee | ||
Updated•10 years ago
|
Attachment #8517860 -
Attachment is obsolete: true
Comment 4•10 years ago
|
||
Comment on attachment 8518197 [details] [diff] [review] With basic tests Review of attachment 8518197 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. r=bholley
Attachment #8518197 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8519148 -
Flags: review?(bobbyholley)
Comment 6•10 years ago
|
||
Comment on attachment 8519148 [details] [diff] [review] followup: only do system stuff for non-worker descriptors, and rename one consumer that was colliding with the new URL global. pending, but checking in now to fix CLOSED TREE Review of attachment 8519148 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/Codegen.py @@ +11887,5 @@ > > def definition_body(self): > descriptors = self.config.getDescriptors(hasInterfaceObject=True, > isExposedInSystemGlobals=True, > + workers=False, I don't grok this part.
Attachment #8519148 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 7•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e5aee097c86 and the followup as https://hg.mozilla.org/integration/mozilla-inbound/rev/156eee9f2d2a > I don't grok this part. We were putting both URL and URL_workers in ResolveSystemBindings, mapping them both to the "URL" identifier. That's wrong. We don't want worker-only descriptors here, because "System" if for mainthread-only globals, right?
Assignee | ||
Updated•10 years ago
|
Target Milestone: --- → mozilla36
Comment 8•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #7) > > I don't grok this part. > > We were putting both URL and URL_workers in ResolveSystemBindings, mapping > them both to the "URL" identifier. That's wrong. We don't want worker-only > descriptors here, because "System" if for mainthread-only globals, right? Ah I see - thanks.
Original patch and the followup backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/970e11385295 for xpcshell orange: https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3705214&repo=mozilla-inbound
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 10•10 years ago
|
||
That looks like more "const URL" bustage. :( I'll do more try pushes, I guess, but maybe we should just give up on this idea? :(
Flags: needinfo?(bobbyholley)
Comment 11•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #10) > That looks like more "const URL" bustage. :( > > I'll do more try pushes, I guess, but maybe we should just give up on this > idea? :( Seems like we can just find it on MXR, yeah? Just search for |const URL = "|, and skip the ones where a Window is involved (since the prototype setup there saves us).
Flags: needinfo?(bobbyholley)
Assignee | ||
Comment 12•10 years ago
|
||
Not sure how the prototype setup in Window saves us, fwiw. And it's hard to tell which of the 50is JS files with "const URL =" have window involved. But I can certainly keep doing try pushes until I get green.
Flags: needinfo?(bzbarsky)
Comment 13•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #12) > Not sure how the prototype setup in Window saves us, fwiw. Oh, I'd always thought that const would shadow an inherited property on the prototype, but looks like it doesn't. How the heck do we avoid breaking web pages when we introduce new DOM APIs then? > And it's hard to > tell which of the 50is JS files with "const URL =" have window involved. Well, most of them are browser-chrome tests, which involve window, and are hence unaffected here. > But I can certainly keep doing try pushes until I get green. I don't really know whether this is useful enough to spend time on, but it seems relatively tractable.
Assignee | ||
Comment 14•10 years ago
|
||
> Oh, I'd always thought that const would shadow an inherited property on the prototype Interface names are not on the prototype. > How the heck do we avoid breaking web pages when we introduce new DOM APIs then? What makes you think we do? ;) Doing this: const Node = 5; in a web page throws in both Firefox and Chrome (but not in Safari, fwiw). I'll see if I can get try to actually run my tests sometime....
Comment 15•10 years ago
|
||
The "URL" contention was the very reason we decided we should not expose identifiers to non-Window globals without opt-in API (like Cu.importGlobalProperties). Why do we turned the decision without fixing the issue? How many add-ons will be broken?
Comment 16•10 years ago
|
||
Hm, I said "Bug 611388 will fix this" in bug 920015 comment #12, but actually bug 589199 will also be needed. How about waiting until bug 589199 is fixed?
Assignee | ||
Comment 17•10 years ago
|
||
> The "URL" contention was the very reason we decided we should not expose identifiers to > non-Window globals without opt-in API We're not exposing on sandboxes, note. Just JSMs and xpcshell. > Why do we turned the decision without fixing the issue? Because I've seen several people recently asking why they can't use the URL API in their JS module. Because Cu.importGlobalProperties is totally undocumented (in spite of multiple requests that someone who actually understands it do so) so no one knows about it and telling people to use it is impossible because it takes too long to sort it out and explain what to do with it. Because we're trying to reduce differences between our stuff and the web. At least those are my reasons. > How many add-ons will be broken? Good question. In any case, I have this green on try, I think. Note that I think we should turn on URLSearchParams no matter what.
Assignee | ||
Comment 18•10 years ago
|
||
> How about waiting until bug 589199 is fixed? I'm not willing to wait on URLSearchParams for that. I might be willing to wait on URL, if we think it will actually affect the behavior there. Note that afaict bug 589199 is a bit far off, not least because the spec is not stable.
Assignee | ||
Comment 19•9 years ago
|
||
Assignee | ||
Comment 20•9 years ago
|
||
OK, I gave up on URL for now. Landed URLSearchParams as https://hg.mozilla.org/integration/mozilla-inbound/rev/0af22f6c4d4e
Summary: Expose URL in system scopes → Expose URLSearchParams in system scopes
https://hg.mozilla.org/mozilla-central/rev/0af22f6c4d4e
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•