Bug 67311 – Add EnabledAtRuntime support for constants.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Aaron Colwell]

Add EnabledAtRuntime support for constants.

[Aaron Colwell]

Created attachment 105821 [details]
Patch

[Aaron Colwell]

Currently V8 code generation allows EnabledAtRuntime on methods and attributes, but not constants. This patch allows constants to be runtime enabled so experimental features that have constants, like MediaSource (https://bugs.webkit.org/show_bug.cgi?id=67306), can completely hide themselves in the DOM. Without this patch the attributes and methods would be hidden, but the constants would be visible.

[Aaron Colwell]

ping

[Aaron Colwell]

Created attachment 109302 [details]
Patch

[Aaron Colwell]

Will you review this please.

[Adam Barth]

Comment on attachment 109302 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2113
>          # FIXME: we need the static_cast here only because of one constant, NodeFilter.idl
>          # defines "const unsigned long SHOW_ALL = 0xFFFFFFFF".  It would be better if we
>          # handled this here, and converted it to a -1 constant in the c++ output.

Can you move this comment inside the else branch?  That's really where it belongs.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2253
> +        static const BatchedConstant constData = {"${name}", static_cast<signed int>(${value})};
> +        batchConfigureConstants(desc, proto, &constData, 1);

Its too bad we don't get much of a batching savings here, but I don't see how to avoid that.  Maybe if we grouped by condition?  That doesn't seem worthwhile.

[Aaron Colwell]

Comment on attachment 109302 [details]
Patch

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

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2113
>>          # handled this here, and converted it to a -1 constant in the c++ output.
> 
> Can you move this comment inside the else branch?  That's really where it belongs.

Done.

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2253
>> +        batchConfigureConstants(desc, proto, &constData, 1);
> 
> Its too bad we don't get much of a batching savings here, but I don't see how to avoid that.  Maybe if we grouped by condition?  That doesn't seem worthwhile.

Yeah I didn't like that either, but I figured grouping by condition was going to make this very difficult to read for not a whole lot of gain.

[Aaron Colwell]

Created attachment 109307 [details]
Patch

[WebKit Review Bot]

Comment on attachment 109307 [details]
Patch

Clearing flags on attachment: 109307

Committed r96409: <http://trac.webkit.org/changeset/96409>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.