Make [Exposed] mandatory, remove [PrimaryGlobal] #423

tobie · 2017-08-23T13:59:03Z

Additionally:

Annotate all interfaces in examples with [Exposed].
Make [Global] allow a single identifier.

Fixes #365.
Fixes https://www.w3.org/Bugs/Public/show_bug.cgi?id=26425.

Preview | Diff

annevk

Pretty pretty good.

annevk · 2017-08-23T14:02:18Z

index.bs

@@ -4684,16 +4739,24 @@ An interface |A| is considered to be a
    For example, if author 1 writes:

    <pre highlight="webidl">
+        [Global,Exposed=Window]


This should be Global=Window, no?

Not according to the current spec which says (emphasis mine):

If the [Global] or [PrimaryGlobal] extended attribute is declared with an identifier list argument, then those identifiers are the interface’s global names; otherwise, the interface has a single global name, which is the interface’s identifier.

I suggest we require an identifier since omitting it would only benefit the Window case as far as I can tell and it seems better to be explicit for these.

annevk · 2017-08-23T14:03:26Z

index.bs

+    Note: this is always the case for [=namespaces=],
+    non-[=callback interface|callback=][=interfaces=],
+    and [=callback interfaces=] which declare [=constants=],
+    as they must be annotated with the [Exposed] extended attribute.


Cannot use a must in a note.

annevk · 2017-08-23T14:04:33Z

index.bs

        interface Window {
          // ...
        };

        // By using the same identifier Worker for both SharedWorkerGlobalScope
        // and DedicatedWorkerGlobalScope, both can be addressed in an [Exposed]
        // extended attribute at once.
-        [Global=Worker]
+        [Exposed=Window, Global=Worker]


This looks wrong.

You probably need the full [Exposed=DedicatedWorker, Global=(Worker,DedicatedWorker)] thing here.

From the text right above, my understanding is this should work:

// By using the same identifier Worker for both SharedWorkerGlobalScope // and DedicatedWorkerGlobalScope, both can be addressed in an [Exposed] // extended attribute at once. [Exposed=Worker, Global=Worker] interface SharedWorkerGlobalScope : WorkerGlobalScope { // ... }; [Exposed=Worker, Global=Worker] interface DedicatedWorkerGlobalScope : WorkerGlobalScope { // ... };

Well that works, but that makes no sense. You don't want to expose the global interface object in multiple globals.

annevk · 2017-08-23T14:04:39Z

index.bs

        interface SharedWorkerGlobalScope : WorkerGlobalScope {
          // ...
        };

-        [Global=Worker]
+        [Exposed=Window, Global=Worker]


This as well.

annevk · 2017-08-23T14:06:24Z

index.bs

-[=takes no arguments|take no arguments=]
-or [=takes an identifier list|take an identifier list=].
+The [{{Global}}] extended attribute must either
+[=takes no arguments|take no arguments=],


I don't think we need this variant really.

I don't really have an opinion about this. This was how it was specified before (maybe implicitly, just for PrimaryGlobal).

annevk · 2017-08-23T14:16:52Z

Note that this cannot land until the majority of specifications are updated to use [Exposed]. @zcorpan and I made some effort toward that goal, but it's not done. We probably need tooling to start complaining.

tobie · 2017-08-23T14:41:08Z

We probably need tooling to start complaining.

Filed w3c/respec#1370 and speced/bikeshed#1077.

tobie · 2017-08-23T18:28:31Z

Pending question here is whether the [Exposed] extended attribute requirement should be relaxed for interfaces annotated with [NoInterfaceObject] (aka mixins).

domenic · 2017-08-23T18:43:31Z

I won't have time to review this, sorry.

I don't think [Exposed] should be required for [NoInterfaceObject], or for future mixins.

annevk · 2017-08-31T13:37:07Z

Personally I wouldn't mind it if we changed this and slowly chased old specifications to comply.

@bzbarsky should probably sign off on that though.

bzbarsky · 2017-08-31T15:04:28Z

It seems ok to me. Forcing all interfaces to always have extended attributes is a bit weird (leading to the question of why that required bit is not using dedicated syntax), but the general idea does have the benefit of forcing people to think about where their new API should be available.

Mixins are complicated. Right now, the spec requires that the exposure set of a mixin be a subset of the exposure set of an interface. That's because the exposure set of the mixin is the default exposure set of all the things being mixed in, and it doesn't make much sense to expose members in places where the interface is not exposed. So we may well want to continue requiring an exposure set for mixins; if not, we need to figure out how things should actually behave. In the proposal where mixins are not required to have [Exposed], what is the exposure set of their members, exactly?

For the same reason, we should probably require [Exposed] even for [NoInterfaceObject], again because it affects exposure of the members.

bzbarsky · 2017-08-31T15:05:46Z

Oh, and I'm OK with changing specifications slowly. The only issue is all the unmaintained specs we have, but we can live with that...

I can probably get bugs filed on the specs Gecko implements when we update to this PR (because I'll have to fix them all locally anyway to build the browser).

annevk · 2017-08-31T15:10:27Z

The idea for mixins would be that you inherit the exposure of all the interface you mix into. And if you some members cannot be mixed into all those places you could restrict them with local overrides.

bzbarsky · 2017-08-31T15:30:52Z

OK, does this PR do that? I, like Domenic, do not have time to read the huge diff carefully in the near term, but a quick skim doesn't show it doing that very well. For example, "The exposure set of the interface is the exposure set of the interface that implements it" is nonsense, because "the interface that implements it" is multiple interfaces with possibly-different exposure sets.

And again, there are non-mixin [NoInterfaceObject] interfaces in the platform...

tobie · 2017-08-31T15:42:13Z

It seems ok to me. Forcing all interfaces to always have extended attributes is a bit weird (leading to the question of why that required bit is not using dedicated syntax), but the general idea does have the benefit of forcing people to think about where their new API should be available.

Yeah, I agree on both counts. What's your suggestion, should we stay with extended attributed or move to dedicated syntax?

For example, "The exposure set of the interface is the exposure set of the interface that implements it" is nonsense, because "the interface that implements it" is multiple interfaces with possibly-different exposure sets.

That phrasing indeed doesn't work. My intent was to say that if [Exposed] was specified on the mixin itself or on one or more of its members, the mixin members would be exposed in the intersection of the identifiers specified in [Exposed] and the exposure set of the interface implementing it. Otherwise, the mixing members would just be exposed in the globals of the exposure set of the interface implementing it.

And again, there are non-mixin [NoInterfaceObject] interfaces in the platform...

Indeed. We made some incorrect assumptions here.

Maybe this will be easier to spec once we have mixins?

bzbarsky · 2017-08-31T15:53:20Z

What's your suggestion, should we stay with extended attributed or move to dedicated syntax?

I haven't thought of non-insane dedicated syntax so far, so I'm probably OK staying with extended attributes.

annevk · 2017-09-01T07:59:30Z

And again, there are non-mixin [NoInterfaceObject] interfaces in the platform...

Do we have a list? (Created #430 to discuss this.)

I thought that in this PR we basically didn't change the behavior for [NoInterfaceObject] and continued to require using [Exposed] as we did already.

tobie · 2017-09-01T08:03:48Z

Here's the list: https://gist.github.com/tobie/bb83f160e93a084afdfb74ae29fddf2a#file-legacynio-w-manual-filtering

bzbarsky · 2017-09-01T13:44:21Z

I thought that in this PR we basically didn't change the behavior for [NoInterfaceObject]

"change" from what? With this PR as it currently stands, interfaces that are [NoInterfaceObject] are not required to list [Exposed], just as before this PR. But the PR gets rid of the concept of PrimaryGlobal and the default exposure set, so the exposure set for members of such interfaces that don't list [Exposed] is no longer defined after this PR in its current form.

tabatkins · 2017-09-25T22:28:58Z

I'm not completely clear from reading this: is [Exposed] allowed/required/disallowed on partial namespaces and interfaces?

tobie · 2017-09-25T23:16:03Z

I'm not completely clear from reading this: is [Exposed] allowed/required/disallowed on partial namespaces and interfaces?

Allowed (it's shorthand for annotating every member).

annevk · 2017-09-27T09:05:50Z

Apologies about adding the merge commit to the PR by the way. I somehow missed this had still outstanding issues and thought that was all that was still needed.

I suspect it's easier to wait for mixins and delay this a little bit, though if tooling can start requiring it that would be good of course.

tobie · 2017-09-27T12:41:03Z

I suspect it's easier to wait for mixins and delay this a little bit, though if tooling can start requiring it that would be good of course.

Agree on both counts.

tabatkins · 2017-09-27T17:28:06Z

It's committed now speced/bikeshed@a1b4829 as a Complain About option. I need to do some refactoring to enable me to turn this on by default. (Right now, my "default" mechanism gets wiped out if anything sets the value, so it doesn't handle things that can accumulate, like Complain About.

This extended attribute has been required by Web IDL since whatwg/webidl#423 . In order for WebGL to be accessible by web workers using OffscreenCanvas, the attribute being added to all WebGL interfaces is: [Exposed=(Window,Worker)] Fixes KhronosGroup#2571 .

* Add [Exposed] Web IDL attribute to all WebGL interfaces. This extended attribute has been required by Web IDL since whatwg/webidl#423 . In order for WebGL to be accessible by web workers using OffscreenCanvas, the attribute being added to all WebGL interfaces is: [Exposed=(Window,Worker)] Fixes #2571 . * Address review feedback from jgilbert

This CL makes the code align with making Exposed mandatory for interfaces. See the following for more details: web-platform-tests/wpt#18382 whatwg/webidl#423 Change-Id: I293e80cd0d953f2f47480f838eecd5f588ec6d80 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1768918 Reviewed-by: Luke Bj <lukebjerring@chromium.org> Commit-Queue: Nicolás Peña Moreno <npm@chromium.org> Cr-Commit-Position: refs/heads/master@{#689988}

tobie requested a review from annevk August 23, 2017 13:59

annevk reviewed Aug 23, 2017

View reviewed changes

tobie mentioned this pull request Aug 23, 2017

Make [Exposed=...] mandatory on all interfaces and namespaces speced/respec#1370

Closed

tobie mentioned this pull request Aug 23, 2017

Log a warning for interfaces and namespaces missing [Exposed] speced/bikeshed#1077

Open

tobie requested a review from domenic August 23, 2017 18:26

tobie removed the request for review from domenic August 25, 2017 07:39

annevk approved these changes Aug 31, 2017

View reviewed changes

annevk mentioned this pull request Sep 1, 2017

No mixin [NoInterfaceObject] usage #430

Open

tobie mentioned this pull request Sep 1, 2017

Designing mixins #363

Closed

This was referenced Sep 30, 2017

Add mixins #433

Merged

Mixin follow-up #450

Open

kenrussell mentioned this pull request Jul 19, 2018

Add [Exposed] Web IDL attribute to all WebGL interfaces. KhronosGroup/WebGL#2673

Merged

saschanaz mentioned this pull request Nov 19, 2018

Add [Exposed] and migrate to new mixin syntax WebBluetoothCG/web-bluetooth#415

Merged

saschanaz mentioned this pull request Aug 24, 2019

Add explicit [Exposed] WICG/webusb#169

Merged

saschanaz mentioned this pull request Oct 1, 2019

Remove [PrimaryGlobal] servo/servo#24336

Closed

ExE-Boss mentioned this pull request Mar 9, 2020

docs(README): Remove mentions of [PrimaryGlobal] jsdom/webidl2js#173

Merged

saschanaz mentioned this pull request Sep 17, 2020

What does [Exposed] mean on [LegacyNoInterfaceObject] interfaces? #919

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make [Exposed] mandatory, remove [PrimaryGlobal] #423

Make [Exposed] mandatory, remove [PrimaryGlobal] #423

tobie commented Aug 23, 2017 •

edited

Loading

annevk left a comment

annevk Aug 23, 2017

tobie Aug 23, 2017

annevk Aug 23, 2017

annevk Aug 23, 2017

annevk Aug 23, 2017

annevk Aug 23, 2017

tobie Aug 23, 2017

annevk Aug 23, 2017

annevk Aug 23, 2017

annevk Aug 23, 2017

tobie Aug 23, 2017

annevk commented Aug 23, 2017 •

edited

Loading

tobie commented Aug 23, 2017

tobie commented Aug 23, 2017

domenic commented Aug 23, 2017

annevk commented Aug 31, 2017

bzbarsky commented Aug 31, 2017

bzbarsky commented Aug 31, 2017

annevk commented Aug 31, 2017

bzbarsky commented Aug 31, 2017

tobie commented Aug 31, 2017

bzbarsky commented Aug 31, 2017

annevk commented Sep 1, 2017 •

edited

Loading

tobie commented Sep 1, 2017

bzbarsky commented Sep 1, 2017

tabatkins commented Sep 25, 2017

tobie commented Sep 25, 2017

annevk commented Sep 27, 2017

tobie commented Sep 27, 2017

tabatkins commented Sep 27, 2017

Make [Exposed] mandatory, remove [PrimaryGlobal] #423

Make [Exposed] mandatory, remove [PrimaryGlobal] #423

Conversation

tobie commented Aug 23, 2017 • edited Loading

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk commented Aug 23, 2017 • edited Loading

tobie commented Aug 23, 2017

tobie commented Aug 23, 2017

domenic commented Aug 23, 2017

annevk commented Aug 31, 2017

bzbarsky commented Aug 31, 2017

bzbarsky commented Aug 31, 2017

annevk commented Aug 31, 2017

bzbarsky commented Aug 31, 2017

tobie commented Aug 31, 2017

bzbarsky commented Aug 31, 2017

annevk commented Sep 1, 2017 • edited Loading

tobie commented Sep 1, 2017

bzbarsky commented Sep 1, 2017

tabatkins commented Sep 25, 2017

tobie commented Sep 25, 2017

annevk commented Sep 27, 2017

tobie commented Sep 27, 2017

tabatkins commented Sep 27, 2017

tobie commented Aug 23, 2017 •

edited

Loading

annevk commented Aug 23, 2017 •

edited

Loading

annevk commented Sep 1, 2017 •

edited

Loading