Implement [Exposed] in WebIDL

RESOLVED FIXED in mozilla34

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

unspecified
mozilla34
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(11 attachments, 4 obsolete attachments)

4.09 KB, patch
khuey
: review+
Details | Diff | Splinter Review
2.44 KB, patch
khuey
: review+
Details | Diff | Splinter Review
4.58 KB, patch
khuey
: review+
Details | Diff | Splinter Review
2.41 KB, patch
khuey
: review+
Details | Diff | Splinter Review
12.83 KB, patch
khuey
: review+
Details | Diff | Splinter Review
31.51 KB, patch
khuey
: review+
Details | Diff | Splinter Review
7.55 KB, patch
khuey
: review+
Details | Diff | Splinter Review
12.24 KB, patch
khuey
: review+
Details | Diff | Splinter Review
14.62 KB, patch
khuey
: review+
Details | Diff | Splinter Review
32.18 KB, patch
khuey
: review+
Details | Diff | Splinter Review
15.05 KB, patch
khuey
: review+
Details | Diff | Splinter Review
(Assignee)

Comment 1

4 years ago
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?
(Assignee)

Comment 2

4 years ago
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

4 years ago
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

4 years ago
(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?
(Assignee)

Comment 5

4 years ago
> 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

4 years ago
With shared workers I definitely expect that to be the case. There will likely be new workers going forward too.
(Assignee)

Updated

4 years ago
Depends on: 1045561
(Assignee)

Updated

4 years ago
Depends on: 1045743
(Assignee)

Comment 7

4 years ago
Created attachment 8464531 [details] [diff] [review]
part 1.  Make our Web IDL parser fail on warnings from the underlying ply code
Attachment #8464531 - Flags: review?(khuey)
(Assignee)

Updated

4 years ago
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
(Assignee)

Comment 8

4 years ago
Created attachment 8464532 [details] [diff] [review]
part 2.  Add support for parenthesized lists as values of extended attributes in Web IDL
Attachment #8464532 - Flags: review?(khuey)
(Assignee)

Comment 9

4 years ago
Created attachment 8464533 [details] [diff] [review]
part 3.  Allow the Global extended attribute to take a value
Attachment #8464533 - Flags: review?(khuey)
Created attachment 8464534 [details] [diff] [review]
part 4.  Rejigger how we do partial interfaces so that we can keep track of which partial interface a given member came from
Attachment #8464534 - Flags: review?(khuey)
Created attachment 8464535 [details] [diff] [review]
part 5. Store the set of known global names on our outermost IDLScope
Attachment #8464535 - Flags: review?(khuey)
Created attachment 8464536 [details] [diff] [review]
part 6.  Implement support for [PrimaryGlobal]
Attachment #8464536 - Flags: review?(khuey)
Created 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
Attachment #8464537 - Flags: review?(khuey)
Created attachment 8464538 [details] [diff] [review]
part 8.  Add [Exposed] extended attributes as needed
Attachment #8464538 - Flags: review?(khuey)
Created attachment 8464539 [details] [diff] [review]
part 9.  Don't codegen window-only things on worker descriptors
Attachment #8464539 - Flags: review?(khuey)
Created attachment 8464540 [details] [diff] [review]
part 10.  Disallow Pref annotations on things that are exposed in workers
Attachment #8464540 - Flags: review?(khuey)
Created attachment 8464541 [details] [diff] [review]
part 11.  Codegen the definition of interface objects on worker globals
Attachment #8464541 - Flags: review?(khuey)
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.
Whiteboard: [need review]
I filed bug 1046107 on the Exposed mess for ServiceWorker.
(Assignee)

Updated

4 years ago
Blocks: 1007135
Created attachment 8466182 [details] [diff] [review]
Part 4, updated to not fail the silly serviceworker tests
Attachment #8466182 - Flags: review?(khuey)
(Assignee)

Updated

4 years ago
Attachment #8464534 - Attachment is obsolete: true
Attachment #8464534 - Flags: review?(khuey)
Created 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
Attachment #8466183 - Flags: review?(khuey)
(Assignee)

Updated

4 years ago
Attachment #8464538 - Attachment is obsolete: true
Attachment #8464538 - Flags: review?(khuey)
Created attachment 8466184 [details] [diff] [review]
Part 11, with test_worker_interfaces updated now that we're properly hiding Console
Attachment #8466184 - Flags: review?(khuey)
(Assignee)

Updated

4 years ago
Attachment #8464541 - Attachment is obsolete: true
Attachment #8464541 - Flags: review?(khuey)
Created attachment 8466609 [details] [diff] [review]
Also make the test accept DataStore/DataStoreCursor, since now we'll be exposing them
Attachment #8466609 - Flags: review?(khuey)
(Assignee)

Updated

4 years ago
Attachment #8466184 - Attachment is obsolete: true
Attachment #8466184 - Flags: review?(khuey)
Blocks: 1047777
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
Attachment #8464531 - Flags: review?(khuey) → review+
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
Attachment #8464537 - Flags: review?(khuey) → review+
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?
Attachment #8464540 - Flags: review?(khuey) → review+
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?
Attachment #8466183 - Flags: review?(khuey) → review+
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.
(Assignee)

Updated

4 years ago
Blocks: 1048695
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.
Blocks: 1048698, 1048699

Updated

3 years ago
Blocks: 742171
You need to log in before you can comment on or make changes to this bug.