Bug 215992 - EnabledBySetting extended attributes on a partial interface do not get merged with EnabledBySetting extended attributes on properties of that partial interface
Summary: EnabledBySetting extended attributes on a partial interface do not get merged...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar
Depends on:
Blocks: 215981
  Show dependency treegraph
 
Reported: 2020-08-30 16:01 PDT by Sam Weinig
Modified: 2020-08-30 18:25 PDT (History)
7 users (show)

See Also:


Attachments
Patch (27.22 KB, patch)
2020-08-30 16:17 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2020-08-30 16:01:12 PDT
While investigating https://bugs.webkit.org/show_bug.cgi?id=215981, one failure was in the generation of NavigatorMediaDevices.idl which currently looks like:

[
    Conditional=MEDIA_STREAM,
    EnabledAtRuntime=MediaDevices,
] partial interface Navigator {
    [SameObject, SecureContext, ContextAllowsMediaDevices] readonly attribute MediaDevices mediaDevices;
    [Custom, EnabledBySetting=LegacyGetUserMedia] undefined getUserMedia(object constraints, object? successCallback, object? errorCallback);
};

One thing to note here is that the extended attributes on the partial interface declaration itself (Conditional=MEDIA_STREAM and EnabledAtRuntime=MediaDevices) are really shorthand for mapping those extended attributes to all the member properties.

In the mentioned bug, I want to change the partial interface wide "EnabledAtRuntime=MediaDevices" to "EnabledBySetting=MediaDevices", which maps the EnabledBySetting to each member of the partial interface. The issue is that the code generator currently overwrites "EnabledBySetting=LegacyGetUserMedia" on the getUserMedia operation, but really it would make more sense to merge them.

This seems to be a general problem for many potentially mergeable extended attributes, we already kind of merge "Conditional" extended attributes, but only for operations for some reason.
Comment 1 Sam Weinig 2020-08-30 16:17:05 PDT
Created attachment 407579 [details]
Patch
Comment 2 Darin Adler 2020-08-30 16:50:39 PDT
Comment on attachment 407579 [details]
Patch

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

> Source/WebCore/bindings/scripts/test/JS/JSTestEnabledBySettingSupplemental.cpp:5
> +    JSTestEnabledBySettingSupplemental.cpp at every build. This file must not be tried to compile.

Not new or part of this patch, but just noticed:

1) We might want to reword "This file must not be tried to compile." since that's not English.

2) If we don’t want people to compile something normally we’d put #error in it.
Comment 3 EWS 2020-08-30 18:20:53 PDT
Committed r266339: <https://trac.webkit.org/changeset/266339>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407579 [details].
Comment 4 Radar WebKit Bug Importer 2020-08-30 18:21:17 PDT
<rdar://problem/68045887>
Comment 5 Sam Weinig 2020-08-30 18:25:33 PDT
(In reply to Darin Adler from comment #2)
> Comment on attachment 407579 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407579&action=review
> 
> > Source/WebCore/bindings/scripts/test/JS/JSTestEnabledBySettingSupplemental.cpp:5
> > +    JSTestEnabledBySettingSupplemental.cpp at every build. This file must not be tried to compile.
> 
> Not new or part of this patch, but just noticed:
> 
> 1) We might want to reword "This file must not be tried to compile." since
> that's not English.
> 
> 2) If we don’t want people to compile something normally we’d put #error in
> it.

Will do this with https://bugs.webkit.org/show_bug.cgi?id=215995.