[WebIDL] Support [Exposed=*] extended attribute

• Component: New Bugs
• Status: RESOLVED FIXED

[Caitlin Potter (:caitp)]

[WebIDL] Support [Exposed=*] extended attribute

[Caitlin Potter (:caitp)]

Created attachment 439866 [details]
Patch

[Chris Dumez]

Comment on attachment 439866 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439866&action=review

> Source/WebCore/ChangeLog:12
> +        https://github.com/heycam/webidl/pull/526 for details.

Seems this has not landed yet. We should probably wait a bit before adding support.

[Caitlin Potter (:caitp)]

> See https://github.com/heycam/webidl/issues/468 and https://github.com/heycam/webidl/pull/526 for details.

This is pretty much just a shorthand which doesn't change any semantics really, and there seems to be some agreement on the syntax.

We want this to make it look nice and brief to expose an interface on all interfaces.

[Chris Dumez]

(In reply to Caitlin Potter (:caitp) from comment #3)
> > See https://github.com/heycam/webidl/issues/468 and https://github.com/heycam/webidl/pull/526 for details.
> 
> This is pretty much just a shorthand which doesn't change any semantics
> really, and there seems to be some agreement on the syntax.
> 
> We want this to make it look nice and brief to expose an interface on all
> interfaces.

From my reading, there is no real agreement yet. I don't think we should add non-standard things. Once the spec gets updated, I have no objection though.

[Radar WebKit Bug Importer]

<rdar://problem/84029794>

[Caitlin Potter (:caitp)]

(In reply to Chris Dumez from comment #4)
> (In reply to Caitlin Potter (:caitp) from comment #3)
> > > See https://github.com/heycam/webidl/issues/468 and https://github.com/heycam/webidl/pull/526 for details.
> > 
> > This is pretty much just a shorthand which doesn't change any semantics
> > really, and there seems to be some agreement on the syntax.
> > 
> > We want this to make it look nice and brief to expose an interface on all
> > interfaces.
> 
> From my reading, there is no real agreement yet. I don't think we should add
> non-standard things. Once the spec gets updated, I have no objection though.

Dan's PR has been merged into webidl https://github.com/whatwg/webidl/commit/59dae6fa5166ab059ba04fe6d82845c78c4a3960

[Chris Dumez]

(In reply to Caitlin Potter (:caitp) from comment #6)
> (In reply to Chris Dumez from comment #4)
> > (In reply to Caitlin Potter (:caitp) from comment #3)
> > > > See https://github.com/heycam/webidl/issues/468 and https://github.com/heycam/webidl/pull/526 for details.
> > > 
> > > This is pretty much just a shorthand which doesn't change any semantics
> > > really, and there seems to be some agreement on the syntax.
> > > 
> > > We want this to make it look nice and brief to expose an interface on all
> > > interfaces.
> > 
> > From my reading, there is no real agreement yet. I don't think we should add
> > non-standard things. Once the spec gets updated, I have no objection though.
> 
> Dan's PR has been merged into webidl
> https://github.com/whatwg/webidl/commit/
> 59dae6fa5166ab059ba04fe6d82845c78c4a3960

Nice, could you please take a look at the bindings EWS failure?

[Chris Dumez]

(In reply to Chris Dumez from comment #7)
> (In reply to Caitlin Potter (:caitp) from comment #6)
> > (In reply to Chris Dumez from comment #4)
> > > (In reply to Caitlin Potter (:caitp) from comment #3)
> > > > > See https://github.com/heycam/webidl/issues/468 and https://github.com/heycam/webidl/pull/526 for details.
> > > > 
> > > > This is pretty much just a shorthand which doesn't change any semantics
> > > > really, and there seems to be some agreement on the syntax.
> > > > 
> > > > We want this to make it look nice and brief to expose an interface on all
> > > > interfaces.
> > > 
> > > From my reading, there is no real agreement yet. I don't think we should add
> > > non-standard things. Once the spec gets updated, I have no objection though.
> > 
> > Dan's PR has been merged into webidl
> > https://github.com/whatwg/webidl/commit/
> > 59dae6fa5166ab059ba04fe6d82845c78c4a3960
> 
> Nice, could you please take a look at the bindings EWS failure?

May just need a rebaseline, I haven't checked but I bet it is related given the area of the patch.

[Caitlin Potter (:caitp)]

Created attachment 443095 [details]
Patch

[Caitlin Potter (:caitp)]

Created attachment 443097 [details]
Patch

Oops, missed the test failures. Rebaselined, hopefully that does it.

[Chris Dumez]

Comment on attachment 443097 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=443097&action=review

> Source/WebCore/bindings/scripts/preprocess-idls.pl:61
> +    "DedicatedWorker",

This is already implied by "Worker" above, no?

> Source/WebCore/bindings/scripts/preprocess-idls.pl:62
> +    "ServiceWorker",

ditto.

> Source/WebCore/bindings/scripts/preprocess-idls.pl:64
> +    "PaintWorklet",

This is already implied by "Worklet" above no?

> Source/WebCore/bindings/scripts/preprocess-idls.pl:65
> +    "AudioWorklet"

ditto.

Because of this, I worry that an interface marked as "Exposed=*" will generate several properties in the ServiceWorker prototype (one on ServiceWorkerPrototype, one on WorkerPrototype).
Am I missing something?

> Source/WebCore/bindings/scripts/test/JS/JSDedicatedWorkerGlobalScope.cpp:72
> +    { "ExposedStar", static_cast<unsigned>(JSC::PropertyAttribute::DontEnum), NoIntrinsic, { (intptr_t)static_cast<PropertySlot::GetValueFunc>(jsDedicatedWorkerGlobalScope_ExposedStarConstructor), (intptr_t) static_cast<PutPropertySlot::PutValueFunc>(0) } },

Notice that DedicatedWorkerGlobalScope now has a ExposedStar property, even though DedicatedWorkerGlobalScope is a subclass of WorkerGlobalScope...

> Source/WebCore/bindings/scripts/test/JS/JSWorkerGlobalScope.cpp:94
> +    { "ExposedStar", static_cast<unsigned>(JSC::PropertyAttribute::DontEnum), NoIntrinsic, { (intptr_t)static_cast<PropertySlot::GetValueFunc>(jsWorkerGlobalScope_ExposedStarConstructor), (intptr_t) static_cast<PutPropertySlot::PutValueFunc>(0) } },

...And WorkerGlobalScope has the WorkerGlobalScope already.

[Caitlin Potter (:caitp)]

Created attachment 443115 [details]
Patch

[Caitlin Potter (:caitp)]

Comment on attachment 443097 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=443097&action=review

>> Source/WebCore/bindings/scripts/preprocess-idls.pl:61
>> +    "DedicatedWorker",
> 
> This is already implied by "Worker" above, no?

Yep -- I guess I had the idea that this could include all supported values, but I've removed Worker/Worklet and left a comment indicating what this is used for.

[Chris Dumez]

Comment on attachment 443115 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=443115&action=review

> Source/WebCore/bindings/scripts/preprocess-idls.pl:61
> +my @supportedGlobalContexts = (

This is the opposite of what I expected. I thought this would be "Window", "Worker", "Worklet".
This has an impact on which prototype object the property is exposed so it is important to get right. Does the spec say which one is right (Your patch proposal or my proposal?). If not, we may want to request that the specification is clarified.

[Chris Dumez]

Comment on attachment 443115 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=443115&action=review

r=me with updated comment.

>> Source/WebCore/bindings/scripts/preprocess-idls.pl:61
>> +my @supportedGlobalContexts = (
> 
> This is the opposite of what I expected. I thought this would be "Window", "Worker", "Worklet".
> This has an impact on which prototype object the property is exposed so it is important to get right. Does the spec say which one is right (Your patch proposal or my proposal?). If not, we may want to request that the specification is clarified.

Ok, I just checked the spec and what you implemented seems to match the spec. The spec says "only interfaces marked as "[Global]". Indeed, DedicatedWorkerGlobalScope is marked as [Global] but not WorkerGlobalScope.

I think your comment should be clarified to mention that it should only contains interfaces that have the [Global] extended attribute.

[Caitlin Potter (:caitp)]

Created attachment 443120 [details]
Patch

Update comment to be more specific

[Caitlin Potter (:caitp)]

Created attachment 443151 [details]
Patch for landing

[EWS]

commit-queue failed to commit attachment 443151 [details] to WebKit repository. To retry, please set cq+ flag again.

[Caitlin Potter (:caitp)]

Created attachment 443160 [details]
Patch for landing

[Caitlin Potter (:caitp)]

Created attachment 443163 [details]
Patch for landing

[EWS]

Committed r285196 (243822@main): <https://commits.webkit.org/243822@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 443163 [details].