1017988 - Implement [Exposed] in WebIDL

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Going to do this, modulo See https://www.w3.org/Bugs/Public/show_bug.cgi?id=25495

Comment 1

[Boris Zbarsky [:bzbarsky]]

Oh, there's another issue with the spec as it stands, that the Google folks raised.  The parsing is ambiguous, because the same separator is used for the values of Exposed and between different extended attributes.  Consider:

  [Exposed=Window,ChromeOnly]

and compare to:

  [Exposed=Window,Worker]

My gut feeling is that we should just make the value a string for now, so basically have [Exposed="Window"] or [Exposed="Worker"] or [Exposed="Window,Worker"] or [Exposed="Worker,Window"] and then post-process the value by splitting on ',' to convert it to a set of names.  Any brighter ideas?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Alternately, we could support the values Window, Worker, All for now.

Note to self: technically per spec we need to support things like [Global=Worker,DedicatedWorker] for this stuff to work.  That has the same parsing issue, though...

Comment 3

[Anne (:annevk)]

To be clear, there's also:

  https://www.w3.org/Bugs/Public/show_bug.cgi?id=24417
  https://www.w3.org/Bugs/Public/show_bug.cgi?id=22646#c23
  https://www.w3.org/Bugs/Public/show_bug.cgi?id=24959

In the last of those bugs the suggestion is made to use & rather than , to separate values.

Comment 4

[(no longer active)]

(In reply to comment #2)
> Alternately, we could support the values Window, Worker, All for now.
> 
> Note to self: technically per spec we need to support things like
> [Global=Worker,DedicatedWorker] for this stuff to work.  That has the same
> parsing issue, though...

We can hardcode the values when parsing both [Global] and [Exposed], right?

Comment 5

[Boris Zbarsky [:bzbarsky]]

> https://www.w3.org/Bugs/Public/show_bug.cgi?id=24417

I think the current spec text covers this already, fwiw.

> https://www.w3.org/Bugs/Public/show_bug.cgi?id=22646#c23

Not going to worry about that for now.

>  https://www.w3.org/Bugs/Public/show_bug.cgi?id=24959

Yes, that's comment 1.

> We can hardcode the values when parsing both [Global] and [Exposed], right?

So far we only seem to have things that are exposed on window, worker, or both, so we could.  But once people start doing stuff with exposing stuff only on dedicated workers or whatnot, that will get more complicated...

Comment 6

[Anne (:annevk)]

With shared workers I definitely expect that to be the case. There will likely be new workers going forward too.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Make our Web IDL parser fail on warnings from the underlying ply code

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Add support for parenthesized lists as values of extended attributes in Web IDL

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Allow the Global extended attribute to take a value

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Rejigger how we do partial interfaces so that we can keep track of which partial interface a given member came from

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Store the set of known global names on our outermost IDLScope

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Implement support for [PrimaryGlobal]

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Add support for parsing the Exposed Web IDL attribute and determining whether a given interface member should be exposed in a particular global

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: part 8. Add [Exposed] extended attributes as needed

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: part 9. Don't codegen window-only things on worker descriptors

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: part 10. Disallow Pref annotations on things that are exposed in workers

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: part 11. Codegen the definition of interface objects on worker globals

Comment 18

[Boris Zbarsky [:bzbarsky]]

Note that this is NOT implementing automatic checks for [Exposed] on interface members before actually exposing them.  I'd like to get the basic infrastructure for tracking exposure sets in place first; then we can figure out how to sanely do that part.

Comment 19

[Boris Zbarsky [:bzbarsky]]

I filed bug 1046107 on the Exposed mess for ServiceWorker.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 4, updated to not fail the silly serviceworker tests

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 8 updated to not expose the random event stuff hixie wants to expose; that can happen in a followup if we want it

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 11, with test_worker_interfaces updated now that we're properly hiding Console

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: Also make the test accept DataStore/DataStoreCursor, since now we'll be exposing them

Comment 24

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8464531 [details] [diff] [review]
part 1.  Make our Web IDL parser fail on warnings from the underlying ply code

Review of attachment 8464531 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/parser/WebIDL.py
@@ +5218,5 @@
>                                  # becomes a speedup again.
>                                  # , picklefile='WebIDLGrammar.pkl'
>                              )
> +        logger.reportGrammarErrors()
> +        

nit: extra whitespace on empty line

Comment 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8464537 [details] [diff] [review]
part 7.  Add support for parsing the Exposed Web IDL attribute and determining whether a given interface member should be exposed in a particular global

Review of attachment 8464537 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/parser/WebIDL.py
@@ +525,5 @@
>  
>      def finish(self, scope):
>          if self._finished:
>              return
> +        self._finished = True        

nit: extra whitespace at EOL

@@ +757,5 @@
> +        for member in self.members:
> +            if not member.exposureSet.issubset(self.exposureSet):
> +                raise WebIDLError("Interface member has larger exposure set "
> +                                  "than the interface itself",
> +                                  [member.location, self.location])            

here too

Comment 26

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8464540 [details] [diff] [review]
part 10.  Disallow Pref annotations on things that are exposed in workers

Review of attachment 8464540 [details] [diff] [review]:
-----------------------------------------------------------------

I assume you've tested that we do the right thing here inside hte workers now?

Comment 27

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8466183 [details] [diff] [review]
Part 8 updated to not expose the random event stuff hixie wants to expose; that can happen in a followup if we want it

Review of attachment 8466183 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with these fixed

::: dom/webidl/Console.webidl
@@ +9,1 @@
>  interface Console {

How is this ChromeOnly?

::: dom/webidl/InstallEvent.webidl
@@ +18,1 @@
>  interface InstallEvent : InstallPhaseEvent {

How is it possible that something that is [Exposed] on Window inherits from something that is [Exposed] on ServiceWorker.  Why don't we catch that?

::: dom/webidl/Navigator.webidl
@@ +54,1 @@
>  interface NavigatorLanguage {

NavigatorLanguage is not hooked up on workers yet, AFAICT.

::: dom/webidl/XMLHttpRequest.webidl
@@ +134,1 @@
>    readonly attribute Document? responseXML;

Can this just be Throws now?

Comment 28

[Boris Zbarsky [:bzbarsky]]

Fixed the whitespace bits.

> I assume you've tested that we do the right thing here inside hte workers now?

As far as I can tell, yes.  I did some manual testing, code inspection, and we have an automated test for the set of interfaces exposed in DedicatedWorker instances.

> How is this ChromeOnly?

The interface object is.  So window.console is exposed, but window.Console is only visible in chrome scopes for now, until there's an actual spec or something.

> How is it possible that something that is [Exposed] on Window inherits from something
> that is [Exposed] on ServiceWorker.  Why don't we catch that?

Because I didn't implement that.  I'll add a check for this to part 7 and make InstallPhaseEvent exposed in (Window,ServiceWorkers).  Good catch.

> NavigatorLanguage is not hooked up on workers yet, AFAICT.

Also good catch.  Fixed.

> Can this just be Throws now?

Yes, in part 9.  Fixed.

Comment 29

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/545ce58ee699
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9fb308fb072
https://hg.mozilla.org/integration/mozilla-inbound/rev/b09a906ead30
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fef6abf1e32
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c98d58a6168
https://hg.mozilla.org/integration/mozilla-inbound/rev/6de7023b9d99
https://hg.mozilla.org/integration/mozilla-inbound/rev/105d8ea002aa
https://hg.mozilla.org/integration/mozilla-inbound/rev/cea6d1d8e0d1
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e4c9130dee7
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf0848334ad2
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d59b6f2a929

Comment 30

[Boris Zbarsky [:bzbarsky]]

I filed bug 1048695 on controlling interface member visibility with [Exposed].

I filed bug 1048698 on being able to expose things on all XPConnect globals via Exposed.

I filed bug 1048699 on improving support for things like Exposed=SharedWorker.

Comment 31

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/545ce58ee699
https://hg.mozilla.org/mozilla-central/rev/e9fb308fb072
https://hg.mozilla.org/mozilla-central/rev/b09a906ead30
https://hg.mozilla.org/mozilla-central/rev/7fef6abf1e32
https://hg.mozilla.org/mozilla-central/rev/3c98d58a6168
https://hg.mozilla.org/mozilla-central/rev/6de7023b9d99
https://hg.mozilla.org/mozilla-central/rev/105d8ea002aa
https://hg.mozilla.org/mozilla-central/rev/cea6d1d8e0d1
https://hg.mozilla.org/mozilla-central/rev/9e4c9130dee7
https://hg.mozilla.org/mozilla-central/rev/cf0848334ad2
https://hg.mozilla.org/mozilla-central/rev/3d59b6f2a929