Bug 189729

Summary: JS bindings generator should support EnabledAtRuntime for static methods
Product: WebKit Reporter: Justin Michaud <justin_michaud>
Component: BindingsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, esprehn+autocc, ews, kondapallykalyan, rniwa, sam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 189694    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Justin Michaud 2018-09-18 19:14:59 PDT
[EnabledAtRuntime=Feature] static void blah(); should add/remove the method when Feature is enabled or disabled.
Comment 1 Justin Michaud 2018-09-18 19:27:31 PDT
Created attachment 350088 [details]
Patch
Comment 2 Justin Michaud 2018-09-19 10:08:31 PDT
Created attachment 350129 [details]
Patch
Comment 3 Chris Dumez 2018-09-19 15:20:28 PDT
Comment on attachment 350129 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3658
> +    my $globalObjectPtr = ($globalObjectIsParam? "&globalObject" : "globalObject()");

missing space before '?'. Also, I think we usually omit the parentheses.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3679
> +    my $globalObjectPtr = ($globalObjectIsParam? "&globalObject" : "globalObject()");

Same comments as above.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:7199
> +    push(@attributes, @{$interface->mapLike->attributes}) if $interface->mapLike;

Could you clarify why maplike is unconditionally treated as runtime enabled?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:7205
> +        if ($attribute->extendedAttributes->{EnabledAtRuntime}) {

Same comment as below. Should probably use NeedsRuntimeCheck()

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:7211
> +    push(@operations, @{$interface->iterable->operations}) if IsKeyValueIterableInterface($interface);

Could you clarify why iterable is unconditionally treated as runtime enabled?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:7212
> +    push(@operations, @{$interface->mapLike->operations}) if $interface->mapLike;

ditto.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:7213
> +    push(@operations, @{$interface->serializable->operations}) if $interface->serializable;

ditto.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:7221
> +        if ($operation->extendedAttributes->{EnabledAtRuntime}) {

We normally do not special-case EnabledAtRuntime and instead rely on NeedsRuntimeCheck(). Also note that EnabledAtRuntime is kind of deprecated in favor of EnabledBySetting, unless you need multithreaded access.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:7293
> +        my $runtimeEnableConditionalString = GenerateRuntimeEnableConditionalString($interface, $operationOrAttribute, "true");

GenerateRuntimeEnableConditionalString supports other things that EnabledAtRuntime. For e.g. it supports EnabledBySetting (which is preferred nowadays). Therefore, it is unfortunate that GetRuntimeEnabledStaticProperties() only returns EnabledAtRuntime things.
Comment 4 Justin Michaud 2018-09-19 16:59:27 PDT
Created attachment 350161 [details]
Patch
Comment 5 Chris Dumez 2018-09-20 09:18:44 PDT
Comment on attachment 350161 [details]
Patch

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

r=me

> Source/WebCore/ChangeLog:6
> +        Add support for EnabledAtRuntime to static methods in the JS bindings

The description should come after...

> Source/WebCore/ChangeLog:9
> +        Reviewed by NOBODY (OOPS!).

... the reviewed by line.
Comment 6 Justin Michaud 2018-09-20 09:53:04 PDT
Created attachment 350219 [details]
Patch
Comment 7 WebKit Commit Bot 2018-09-20 10:31:51 PDT
Comment on attachment 350219 [details]
Patch

Clearing flags on attachment: 350219

Committed r236266: <https://trac.webkit.org/changeset/236266>
Comment 8 WebKit Commit Bot 2018-09-20 10:31:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2018-09-20 10:33:40 PDT
<rdar://problem/44646048>