Closed Bug 1094545 Opened 5 years ago Closed 5 years ago

Expose URLSearchParams in system scopes

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

Attachments

(3 files, 1 obsolete file)

It looks safe to me, and is useful.
Attachment #8517860 - Flags: review?(bobbyholley)
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-
Attached patch With basic testsSplinter Review
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)
Attachment #8517860 - Attachment is obsolete: true
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+
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+
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?
Target Milestone: --- → mozilla36
(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.
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)
(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)
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)
(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.
> 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....
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?
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?
> 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.
> 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.
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: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.