Boris Zbarsky [:bzbarsky] Assignee
	Description • 10 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 1 • 10 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?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 2 • 10 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...

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

	(no longer active)
	Comment 4 • 10 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?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 5 • 10 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...

	Anne (:annevk)
	Comment 6 • 10 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 18 • 10 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 19 • 10 years ago

I filed bug 1046107 on the Exposed mess for ServiceWorker.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 24 • 10 years ago

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

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 25 • 10 years ago

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

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 26 • 10 years ago

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?

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 27 • 10 years ago

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?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 28 • 10 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 29 • 10 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 30 • 10 years ago

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.

	Ed Morley [:emorley]
	Comment 31 • 10 years ago

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