Summary: | JSContext Inspection: Provide a way to use a non-Main RunLoop for Inspector JavaScript Evaluations | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||||||||||
Component: | JavaScriptCore | Assignee: | Joseph Pecoraro <joepeck> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | bunhere, cdumez, commit-queue, gyuyoung.kim, joepeck, sergio | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Joseph Pecoraro
2014-06-26 17:25:37 PDT
Created attachment 233949 [details]
[PATCH] Proposed Fix
I tested this on OS X and iOS with a simple test application to check edge conditions. Everything behaved as I expected. I want to do some more testing on this, and maybe include an updated patch with some basic JSC tests for the property.
Unfortunately there is some duplicated code in this patch between the global shared path and this debuggable specific path. I was not sure how best to share the code that didn't just look uglier.
Attachment 233949 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/API/JSContextRef.cpp:45: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Source/JavaScriptCore/API/JSContextRefPrivate.h:144: The parameter name "runLoop" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 2 in 9 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 233949 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=233949&action=review Looks good. > Source/JavaScriptCore/API/JSContextPrivate.h:51 > +@property (setter=_setDebuggerRunLoop:) CFRunLoopRef _debuggerRunLoop NS_AVAILABLE(10_10, 8_0); It is annoying this needs to use a C type in ObjC. But at least NSRunLoop has a getter for CFRunLoopRef. I would say make this use NSRunLoop and have the implementation for _setDebuggerRunLoop: translate to a CFRunLoopRef. But there is no way to turn a CFRunLoopRef into a NSRunLoop. Comment on attachment 233949 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=233949&action=review >> Source/JavaScriptCore/API/JSContextPrivate.h:51 >> +@property (setter=_setDebuggerRunLoop:) CFRunLoopRef _debuggerRunLoop NS_AVAILABLE(10_10, 8_0); > > It is annoying this needs to use a C type in ObjC. But at least NSRunLoop has a getter for CFRunLoopRef. > > I would say make this use NSRunLoop and have the implementation for _setDebuggerRunLoop: translate to a CFRunLoopRef. But there is no way to turn a CFRunLoopRef into a NSRunLoop. Correct. I think its better to have a property than a one way. If we made this NSRunLoop, we could just assume nobody will change the run loop out from under the ObjC API using the C API. Created attachment 233999 [details]
[PATCH] For Bots 1
Attachment 233999 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/API/JSContextRef.cpp:45: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Source/JavaScriptCore/API/JSContextRefPrivate.h:145: The parameter name "runLoop" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 2 in 9 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 234002 [details]
[PATCH] For Bots 2
Created attachment 234006 [details]
[PATCH] For Bots 3
Comment on attachment 234006 [details] [PATCH] For Bots 3 View in context: https://bugs.webkit.org/attachment.cgi?id=234006&action=review > Source/JavaScriptCore/API/JSContextRefPrivate.h:150 > +#if USE(CF) > +/*! > +@function > +@abstract Gets the run loop used by the Web Inspector debugger when evaluating JavaScript in this context. > +@param ctx The JSGlobalContext whose setting you want to get. > +*/ > +JS_EXPORT CFRunLoopRef JSGlobalContextGetDebuggerRunLoop(JSGlobalContextRef ctx) CF_AVAILABLE(10_10, 8_0); > + > +/*! > +@function > +@abstract Sets the run loop used by the Web Inspector debugger when evaluating JavaScript in this context. > +@param ctx The JSGlobalContext that you want to change. > +@param runLoop The new value of the setting for the context. > +*/ > +JS_EXPORT void JSGlobalContextSetDebuggerRunLoop(JSGlobalContextRef ctx, CFRunLoopRef) CF_AVAILABLE(10_10, 8_0); > +#endif Using USE(CF) inside of a Private header is not going to fly. I don't think there is any clean way to make this clean in the private header. Lets make this an ObjC only SPI. Move the C API bits to an Internal header. Maybe than I can switch to NSRunLoop. Created attachment 234016 [details]
[PATCH] For Bots 4
|