Bug 122531 – ObjCCallbackFunctionImpl shouldn't store a JSContext

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Hahnenberg]

The m_context field in ObjCCallbackFunctionImpl is vestigial and is only incidentally correct in the common case. It's also no longer necessary in that we can look up the current JSContext by looking using the globalObject of the callee when the function callback is invoked.

[Mark Hahnenberg]

Created attachment 213737 [details]
Patch

[Geoffrey Garen]

Comment on attachment 213737 [details]
Patch

r=me

[Mark Rowe (bdash)]

Adding new API requires a) sending it through API review, and b) tagging the new API with the appropriate availability macro.

[Mark Hahnenberg]

Created attachment 214218 [details]
Patch

[Mark Hahnenberg]

(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!

[Mark Rowe (bdash)]

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.

[Mark Hahnenberg]

Created attachment 214230 [details]
Patch

[Geoffrey Garen]

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*?

[Mark Hahnenberg]

(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.

[Build Bot]

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

[Build Bot]

Comment on attachment 214230 [details]
Patch

Attachment 214230 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4070008

[Mark Hahnenberg]

Created attachment 214285 [details]
Patch

[Geoffrey Garen]

Comment on attachment 214285 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 214285 [details]
Patch

Clearing flags on attachment: 214285

Committed r157468: <http://trac.webkit.org/changeset/157468>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.