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

Chris Dumez
Reported 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;
Attachments
Patch (19.37 KB, patch)
2013-05-15 06:46 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
JSDOMWindow.cpp diff (1.59 KB, patch)
2013-05-15 23:37 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Chris Dumez
Comment 1 2013-05-15 06:46:57 PDT
Created attachment 201822 [details] Patch
Kentaro Hara
Comment 2 2013-05-15 06:52:39 PDT
Great improvement! JSC experts should take a look.
Ryosuke Niwa
Comment 3 2013-05-15 09:18:21 PDT
Comment on attachment 201822 [details] Patch Nice!
Ryosuke Niwa
Comment 4 2013-05-15 09:19:34 PDT
<rdar://problem/10155019>
Ryosuke Niwa
Comment 5 2013-05-15 09:19:55 PDT
*** Bug 52011 has been marked as a duplicate of this bug. ***
Chris Dumez
Comment 6 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.
Geoffrey Garen
Comment 7 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?
Chris Dumez
Comment 8 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.
Geoffrey Garen
Comment 9 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?
Chris Dumez
Comment 10 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;
Chris Dumez
Comment 11 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.
Chris Dumez
Comment 12 2013-05-17 00:21:25 PDT
Geoffrey Garen, did you get a chance to take a look?
Geoffrey Garen
Comment 13 2013-05-17 10:50:35 PDT
Comment on attachment 201822 [details] Patch r=me
WebKit Commit Bot
Comment 14 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>
WebKit Commit Bot
Comment 15 2013-05-17 11:16:17 PDT
All reviewed patches have been landed. Closing bug.
Chris Dumez
Comment 16 2013-05-17 14:50:45 PDT
I added [EnabledAtRuntime] documentation to the WebKit IDL wiki: http://trac.webkit.org/wiki/WebKitIDL#EnabledAtRuntime
Note You need to log in before you can comment on or make changes to this bug.