[webkit-reviews] review denied: [Bug 175836] [JSC API] We should support the symbol type in our C/Obj-C API : [Attachment 319642] fixed to not have macro value in public header

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Keith Miller
<keith_miller at apple.com>'s request for review:
Bug 175836: [JSC API] We should support the symbol type in our C/Obj-C API
https://bugs.webkit.org/show_bug.cgi?id=175836

Attachment 319642: fixed to not have macro value in public header

https://bugs.webkit.org/attachment.cgi?id=319642&action=review




--- Comment #19 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 319642
  --> https://bugs.webkit.org/attachment.cgi?id=319642
fixed to not have macro value in public header

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

Next steps are fixing review comments, updating patch to trunk, API review, and
notifying documentation about new API.

> Source/JavaScriptCore/API/JSValue.h:158
> ++ (JSValue *)valueWithNewSymbolFromDescription:(NSString *)description
InContext:(JSContext *)context NS_AVAILABLE(10_13, 11_0);

Its "inContext". (Capitalization is part of the method signature so it's API.)

> Source/JavaScriptCore/API/JSValue.h:327
> +#if (defined(__MAC_OS_X_VERSION_MIN_REQUIRED) &&
__MAC_OS_X_VERSION_MIN_REQUIRED < 101300) ||
(defined(__IPHONE_OS_VERSION_MIN_REQUIRED) && __IPHONE_OS_VERSION_MIN_REQUIRED
< 110000)
> +
> +// These declarations exist to ensure compatablity when targeting older OSs.
> +- (JSValue *)valueForProperty:(NSString *)property;
> +- (void)setValue:(id)value forProperty:(NSString *)property;
> +- (BOOL)deleteProperty:(NSString *)property;
> +- (BOOL)hasProperty:(NSString *)property;
> +- (void)defineProperty:(NSString *)property descriptor:(id)descriptor;
> +
> +#else // (defined(__MAC_OS_X_VERSION_MIN_REQUIRED) &&
__MAC_OS_X_VERSION_MIN_REQUIRED < 101300) ||
(defined(__IPHONE_OS_VERSION_MIN_REQUIRED) && __IPHONE_OS_VERSION_MIN_REQUIRED
< 110000)

Can we move these to an out-of-the-way place?

To match your other comment, I would say "These declarations provide
compatibility when targeting SDKs for legacy OSs. In macOS 10.13 and iOS11 and
below, 'property' was an NSString *.

> Source/JavaScriptCore/API/JSValue.h:334
> + at discussion Before macOS 10.13 and iOS 11 <code>property</code> was an
NSString *, this was relaxed to id so JavaScript Symbol properties could be
accessed.

I would say "Corresponds to the JavaScript operation object[property]. Pass an
NSString * to access a named property. Other valid properties include symbols,
numbers, and stringifiable objects. In macOS 10.13 and iOS11 and below,
'property' was an NSString *. " (Same in other places.)

> Source/JavaScriptCore/API/JSValue.mm:250
> +#define USE_NSSTRING (defined(__MAC_OS_X_VERSION_MIN_REQUIRED) &&
__MAC_OS_X_VERSION_MIN_REQUIRED < 101300) ||
(defined(__IPHONE_OS_VERSION_MIN_REQUIRED) && __IPHONE_OS_VERSION_MIN_REQUIRED
< 110000)

Let's call this USE_NSSTRING_FOR_PROPERTY_KEY to avoid a name conflict from
overly broad naming. We usually use "USE(X)" to mean that X is a project-wide
feature. We do use NSString, so we need a more specific name.

> Source/JavaScriptCore/API/tests/testapi.cpp:105
> +//	 {
> +//	     APIString description("dope");
> +//	     JSValueRef symbol = JSValueMakeSymbol(context, description);
> +//	     ScriptResult result = evaluateScript("typeof(this) == 'symbol'",
symbol);
> +//	 }

Wat.

> Source/JavaScriptCore/API/tests/testapi.mm:575
>	   JSContext *context = [[JSContext alloc] init];

These tests need more beef.