Bug 143324

Summary: JavaScriptCore API should support type checking for Array and Date
Product: WebKit Reporter: Geoffrey Garen <ggaren>
Component: New BugsAssignee: Geoffrey Garen <ggaren>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, joepeck, mhahnenb, msaboff
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch darin: review+

Description Geoffrey Garen 2015-04-01 17:04:59 PDT 
JavaScriptCore API should support type checking for Array and Date
Comment 1 Geoffrey Garen 2015-04-01 17:05:58 PDT 
Created attachment 249953 [details]
Patch
Comment 2 WebKit Commit Bot 2015-04-01 17:08:20 PDT 
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.
Comment 3 Sam Weinig 2015-04-02 08:57:48 PDT 
Comment on attachment 249953 [details]
Patch

These probably need availability macros.
Comment 4 Darin Adler 2015-04-02 09:09:35 PDT 
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.
Comment 5 Geoffrey Garen 2015-04-02 11:04:46 PDT 
> 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.
Comment 6 Geoffrey Garen 2015-04-02 17:10:29 PDT 
Committed r182297: <http://trac.webkit.org/changeset/182297>
Comment 7 Joseph Pecoraro 2015-04-04 11:01:52 PDT 
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?
Comment 8 Joseph Pecoraro 2015-04-04 11:04:24 PDT 
(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!