[WebIDL] Support [Exposed=*] extended attribute
Created attachment 439866 [details] Patch
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.
> 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.
(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.
<rdar://problem/84029794>
(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
(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?
(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.
Created attachment 443095 [details] Patch
Created attachment 443097 [details] Patch Oops, missed the test failures. Rebaselined, hopefully that does it.
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.
Created attachment 443115 [details] Patch
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.
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.
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.
Created attachment 443120 [details] Patch Update comment to be more specific
Created attachment 443151 [details] Patch for landing
commit-queue failed to commit attachment 443151 [details] to WebKit repository. To retry, please set cq+ flag again.
Created attachment 443160 [details] Patch for landing
Created attachment 443163 [details] Patch for landing
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].