Bug 116147

Summary: Add [EnabledAtRuntime] extended attribute support for global constructors
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: BindingsAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, darin, ggaren, haraken, joenotcharles, laszlo.gombos, rniwa, sam, simon.fraser, thorton
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 52011, 115853, 116343    
Attachments:
Description Flags
Patch
none
JSDOMWindow.cpp diff none

Description Chris Dumez 2013-05-15 04:32:57 PDT
The [EnabledAtRuntime] IDL extended attribute is not currently supported by the JSC bindings generator. Several of our global constructors are enabled at runtime and require custom code because of it. Additionally, those global constructors cannot be automatically generated because of this limitation.

List of affected global constructors:
[CustomGetter, Conditional=VIDEO] attribute HTMLAudioElementConstructorConstructor Audio; // Also a named constructor (see Bug 116116)
[Conditional=WEB_SOCKETS, CustomGetter] attribute WebSocketConstructor WebSocket;
[Conditional=SHARED_WORKERS, CustomGetter] attribute SharedWorkerConstructor SharedWorker;
Comment 1 Chris Dumez 2013-05-15 06:46:57 PDT
Created attachment 201822 [details]
Patch
Comment 2 Kentaro Hara 2013-05-15 06:52:39 PDT
Great improvement! JSC experts should take a look.
Comment 3 Ryosuke Niwa 2013-05-15 09:18:21 PDT
Comment on attachment 201822 [details]
Patch

Nice!
Comment 4 Ryosuke Niwa 2013-05-15 09:19:34 PDT
<rdar://problem/10155019>
Comment 5 Ryosuke Niwa 2013-05-15 09:19:55 PDT
*** Bug 52011 has been marked as a duplicate of this bug. ***
Comment 6 Chris Dumez 2013-05-15 09:35:04 PDT
(In reply to comment #5)
> *** Bug 52011 has been marked as a duplicate of this bug. ***

My patch only adds [EnabledAtRuntime] support for global constructors at the moment. It can be extended to regular attributes and operations later on.
Comment 7 Geoffrey Garen 2013-05-15 12:40:38 PDT
Comment on attachment 201822 [details]
Patch

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

> Source/WebCore/ChangeLog:18
> +        No new tests, no behavior change for layout tests.

Shouldn't this patch change the output of run-bindings-tests?

> Source/WebCore/Modules/websockets/WebSocket.cpp:129
> -static bool webSocketsAvailable = false;
> +static bool webSocketsAvailable = true;

Why did this default change?
Comment 8 Chris Dumez 2013-05-15 12:57:27 PDT
Comment on attachment 201822 [details]
Patch

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

>> Source/WebCore/ChangeLog:18
>> +        No new tests, no behavior change for layout tests.
> 
> Shouldn't this patch change the output of run-bindings-tests?

No it doesn't because it only affects *Constructor attributes on DOMWindow. I can upload a diff of the generated JSDOMWindow.cpp if this would help.

>> Source/WebCore/Modules/websockets/WebSocket.cpp:129
>> +static bool webSocketsAvailable = true;
> 
> Why did this default change?

We were not using RuntimeEnabledFeatures for WebSocket before (See removed JSDOMWindowWebSocketCustom.cpp in this patch). RuntimeEnabledFeatures::webSocketEnabled() depends on this flag.
This flag was disabled by default but it did not matter because it was not used.
Comment 9 Geoffrey Garen 2013-05-15 14:19:03 PDT
> I can upload a diff of the generated JSDOMWindow.cpp if this would help.

Sure, I'd like to take a look.

> 
> >> Source/WebCore/Modules/websockets/WebSocket.cpp:129
> >> +static bool webSocketsAvailable = true;
> > 
> > Why did this default change?
> 
> RuntimeEnabledFeatures::webSocketEnabled() depends on this flag.

I guess I'm just confused by the feature being enabled by default. Shouldn't "EnabledAtRuntime" be disabled by default, and then potentially enabled at runtime?
Comment 10 Chris Dumez 2013-05-15 23:37:52 PDT
Created attachment 201927 [details]
JSDOMWindow.cpp diff

2 new attributes are also generated in DerivedSources/WebCore/DOMWindowConstructors.idl:

[EnabledAtRuntime, Conditional=SHARED_WORKERS] attribute SharedWorkerConstructor SharedWorker;
[EnabledAtRuntime, Conditional=WEB_SOCKETS] attribute WebSocketConstructor WebSocket;
Comment 11 Chris Dumez 2013-05-15 23:43:20 PDT
(In reply to comment #9)
> > I can upload a diff of the generated JSDOMWindow.cpp if this would help.
> 
> Sure, I'd like to take a look.
> 
> > 
> > >> Source/WebCore/Modules/websockets/WebSocket.cpp:129
> > >> +static bool webSocketsAvailable = true;
> > > 
> > > Why did this default change?
> > 
> > RuntimeEnabledFeatures::webSocketEnabled() depends on this flag.
> 
> I guess I'm just confused by the feature being enabled by default. Shouldn't "EnabledAtRuntime" be disabled by default, and then potentially enabled at runtime?

[EnabledAtRuntime] merely indicates that the RuntimeEnabledFeatures class is used to determine if the feature should be enabled or not at runtime. The feature may or may not be enabled by default depending on its maturity.

There are a lot of runtime-enabled features that are enabled by default (isInputTypeDateEnabled, isVideoTrackEnabled, isInputTypeDateTimeLocalEnabled, isFullScreenAPIEnabled, ...).

If I don't enable WebSocket by default, then I would have to update each port to call WebSocket::setIsEnabled(true) because they all expect WebSocket to be enabled and they currently don't call WebSocket::setIsEnabled(), except for BlackBerry port.
Comment 12 Chris Dumez 2013-05-17 00:21:25 PDT
Geoffrey Garen, did you get a chance to take a look?
Comment 13 Geoffrey Garen 2013-05-17 10:50:35 PDT
Comment on attachment 201822 [details]
Patch

r=me
Comment 14 WebKit Commit Bot 2013-05-17 11:16:14 PDT
Comment on attachment 201822 [details]
Patch

Clearing flags on attachment: 201822

Committed r150276: <http://trac.webkit.org/changeset/150276>
Comment 15 WebKit Commit Bot 2013-05-17 11:16:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Chris Dumez 2013-05-17 14:50:45 PDT
I added [EnabledAtRuntime] documentation to the WebKit IDL wiki:
http://trac.webkit.org/wiki/WebKitIDL#EnabledAtRuntime