Bug 143324 - JavaScriptCore API should support type checking for Array and Date
Summary: JavaScriptCore API should support type checking for Array and Date
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Geoffrey Garen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-04-01 17:04 PDT by Geoffrey Garen
Modified: 2015-04-04 11:04 PDT (History)
4 users (show)

See Also:


Attachments
Patch (7.46 KB, patch)
2015-04-01 17:05 PDT, Geoffrey Garen
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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!