Bug 143324 – JavaScriptCore API should support type checking for Array and Date

• Component: New Bugs
• Status: RESOLVED FIXED

[Geoffrey Garen]

JavaScriptCore API should support type checking for Array and Date

[Geoffrey Garen]

Created attachment 249953 [details]
Patch

[WebKit Commit Bot]

Attachment 249953 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/JSValueRef.h:139:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:148:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Comment on attachment 249953 [details]
Patch

These probably need availability macros.

[Darin Adler]

Comment on attachment 249953 [details]
Patch

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

Should make these properties as suggested by the API experts here at Apple.

> Source/JavaScriptCore/API/JSValueRef.cpp:179
> +    JSValue jsValue = toJS(exec, value);
> +    return jsValue.inherits(JSArray::info());

I’d like this better as a one-liner.

> Source/JavaScriptCore/API/JSValueRef.cpp:192
> +    JSValue jsValue = toJS(exec, value);
> +    return jsValue.inherits(DateInstance::info());

Ditto.

> Source/JavaScriptCore/API/tests/testapi.c:1416
> +    JSStringRelease(str);

Nice to fix the leak. I think the code below also leaks an exception object.

[Geoffrey Garen]

> View in context:
> https://bugs.webkit.org/attachment.cgi?id=249953&action=review
> 
> Should make these properties as suggested by the API experts here at Apple.

I'll follow up with a patch to do that for all of these related is* methods, once the discussion is settled.

> > Source/JavaScriptCore/API/JSValueRef.cpp:179
> > +    JSValue jsValue = toJS(exec, value);
> > +    return jsValue.inherits(JSArray::info());
> 
> I’d like this better as a one-liner.
> 
> > Source/JavaScriptCore/API/JSValueRef.cpp:192
> > +    JSValue jsValue = toJS(exec, value);
> > +    return jsValue.inherits(DateInstance::info());
> 
> Ditto.

OK.

> Nice to fix the leak. I think the code below also leaks an exception object.

The exception local variable is a JSValueRef, so it's garbage collected.

[Geoffrey Garen]

Committed r182297: <http://trac.webkit.org/changeset/182297>

[Joseph Pecoraro]

Comment on attachment 249953 [details]
Patch

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

> Source/JavaScriptCore/API/JSValue.h:422
> +- (BOOL)isArray;

Should these have NS_AVAILABLE macros?

> Source/JavaScriptCore/API/JSValueRef.h:139
> +JS_EXPORT bool JSValueIsArray(JSContextRef ctx, JSValueRef value);

Should these have CF_AVAILABLE macros?

[Joseph Pecoraro]

(In reply to comment #7)
> Comment on attachment 249953 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=249953&action=review
> 
> > Source/JavaScriptCore/API/JSValue.h:422
> > +- (BOOL)isArray;
> 
> Should these have NS_AVAILABLE macros?
> 
> > Source/JavaScriptCore/API/JSValueRef.h:139
> > +JS_EXPORT bool JSValueIsArray(JSContextRef ctx, JSValueRef value);
> 
> Should these have CF_AVAILABLE macros?

Ahh, I see when this landed it had them!