Summary: | ObjCCallbackFunctionImpl shouldn't store a JSContext | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Hahnenberg <mhahnenberg> | ||||||||||
Component: | JavaScriptCore | Assignee: | Mark Hahnenberg <mhahnenberg> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | buildbot, commit-queue, mrowe, rniwa | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Mark Hahnenberg
2013-10-08 16:25:38 PDT
Created attachment 213737 [details]
Patch
Comment on attachment 213737 [details]
Patch
r=me
Adding new API requires a) sending it through API review, and b) tagging the new API with the appropriate availability macro. Created attachment 214218 [details]
Patch
(In reply to comment #4) > Created an attachment (id=214218) [details] > Patch @bdash: Not sure what availability macro to use, so I just winged it. Your input would be valuable! Comment on attachment 214218 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=214218&action=review > Source/JavaScriptCore/API/JSContextRef.h:136 > +#if defined(__MAC_OS_X_VERSION_MIN_REQUIRED) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 1070 > +/*! > +@function > +@abstract Gets the global context of a JavaScript execution context. > +@param ctx The JSContext whose global context you want to get. > +@result ctx's global context. > +*/ > +JS_EXPORT JSGlobalContextRef JSContextGetGlobalContext(JSContextRef ctx); > +#endif This isn't how availability macros are used. You use something like NS_AVAILABLE_MAC(10_7) at the end of the method declaration: JS_EXPORT JSGlobalContextRef JSContextGetGlobalContext(JSContextRef ctx) NS_AVAILABLE_MAC(10_7); There's a slightly different macro to use if this should be exposed on iOS as well. If this header file is used by non-Mac platforms you may need to add some creative #ifing so that other platforms don't choke on the availability macros. Created attachment 214230 [details]
Patch
Comment on attachment 214230 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=214230&action=review > Source/JavaScriptCore/API/JSContextRef.h:134 > +JS_EXPORT JSGlobalContextRef JSContextGetGlobalContext(JSContextRef ctx) CF_AVAILABLE(10_7, NA); Does NA mean not available on iOS? Should we be using AVAILABLE_IN_WEBKIT_VERSION*? (In reply to comment #8) > (From update of attachment 214230 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=214230&action=review > > > Source/JavaScriptCore/API/JSContextRef.h:134 > > +JS_EXPORT JSGlobalContextRef JSContextGetGlobalContext(JSContextRef ctx) CF_AVAILABLE(10_7, NA); > > Does NA mean not available on iOS? NA does mean not available. I wasn't sure which version of iOS first had the symbol in the binary, but I figured I could go back and fill it in later. > > Should we be using AVAILABLE_IN_WEBKIT_VERSION*? I think the standard these days is to use OS versions rather than WebKit versions. I spoke with bdash, and he recommended using the OS-provided availability macros (see above). I ended up using the CF_* version rather than the NS_* version due to the fact that this header is typically included in C/C++ compilation units. Comment on attachment 214230 [details] Patch Attachment 214230 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4068010 Comment on attachment 214230 [details] Patch Attachment 214230 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4070008 Created attachment 214285 [details]
Patch
Comment on attachment 214285 [details]
Patch
r=me
Comment on attachment 214285 [details] Patch Clearing flags on attachment: 214285 Committed r157468: <http://trac.webkit.org/changeset/157468> All reviewed patches have been landed. Closing bug. |