RESOLVED FIXED 143324
JavaScriptCore API should support type checking for Array and Date
https://bugs.webkit.org/show_bug.cgi?id=143324
Summary JavaScriptCore API should support type checking for Array and Date
Geoffrey Garen
Reported 2015-04-01 17:04:59 PDT
JavaScriptCore API should support type checking for Array and Date
Attachments
Patch (7.46 KB, patch)
2015-04-01 17:05 PDT, Geoffrey Garen
darin: review+
Geoffrey Garen
Comment 1 2015-04-01 17:05:58 PDT
WebKit Commit Bot
Comment 2 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.
Sam Weinig
Comment 3 2015-04-02 08:57:48 PDT
Comment on attachment 249953 [details] Patch These probably need availability macros.
Darin Adler
Comment 4 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.
Geoffrey Garen
Comment 5 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.
Geoffrey Garen
Comment 6 2015-04-02 17:10:29 PDT
Joseph Pecoraro
Comment 7 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?
Joseph Pecoraro
Comment 8 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!
Note You need to log in before you can comment on or make changes to this bug.