Bug 20020 – Proposed enhancement to JavaScriptCore API

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Kelvin Sherlock]

It would be very convenient to be able to create builtin objects (Array, Date, Error, RegExp) directly through the exported API. The SpiderMonkey JSAPI, for comparison, has JS_NewArrayObject and JS_NewRegExpObject functions.

[Kelvin Sherlock]

Created attachment 22256 [details]
Add new API functions (JSObjectMake(Array|Date|Error|RegExp)

Add new API functions to JSObjectRef.cpp and tweak visibility of contructor functions.

[Kelvin Sherlock]

Created attachment 22257 [details]
test of new functionality

XCode project to test new functionality, written in objective c.

[Mark Rowe (bdash)]

Comment on attachment 22256 [details]
Add new API functions (JSObjectMake(Array|Date|Error|RegExp)

If you would like the patch to be reviewed, please set the review flag to ?. It should only be set to + by a reviewer.

[Sam Weinig]

A better place to add tests for new API functionality would be the testapi.c and testapi.js files.

[Geoffrey Garen]

Comment on attachment 22256 [details]
Add new API functions (JSObjectMake(Array|Date|Error|RegExp)

Hi Kevin. Nice additions. We've been talking about this forever -- I'm glad you finally did it!

As the submit policy says, "Please also add your copyright (name and year) to the relevant files for changes that are more than 10 lines of code."

[phone]  Kelvin Sherlock  <set EMAIL_ADDRESS environment variable>

Please add your email address to this changelog.

 436  @abstract Convenience method for creating a JavaScript Date Object.

You switched this line between JSObjectMakeArray and JSObjectMakeDate.

 439  @param arguments A JSValue array of data to populate the Array with . Pass NULL if argumentCount is 0.

Typo: "with ." should be "with.".

+ @param arguments A JSValue array of data to populate the Array with . Pass NULL if argumentCount is 0.

This is a little controversial. All the other convenience functions you've added do exactly what their JavaScript constructors do, but this one doesn't.

That said, I think the behavior here is a lot less weird than the behavior of the JavaScript constructor, so maybe that's OK, but I think we should document the difference more clearly. 

I would change all the abstracts to read, "Creates a JavaScript X object, as if by invoking the built-in X constructor." Then, I would change the JSObjectMakeArray abstract to read, "Creates a JavaScript Array object." Finally, I would add an @discussion to JSObjectMakeArray that reads, "The behavior of this function does not exactly match the behavior of the built-in Array constructor. Specifically, if one argument is supplied, this function returns an array with one element."

 447  @abstract Convenience method for creating a JavaScript Array Object.

You switched this line between JSObjectMakeArray and JSObjectMakeDate.

 465 JS_EXPORT JSObjectRef JSObjectMakeDate(JSContextRef ctx, size_t argumentCount, const JSValueRef arguments[], JSValueRef *exception);    

You included this declaration and documentation twice.

[Kelvin Sherlock]

Created attachment 22627 [details]
Add new API functions (JSObjectMake(Array|Date|Error|RegExp)

cleaned up documentation from previous patch, removed JSLock, added call to exec->globalData().heap->registerThread();

[Geoffrey Garen]

Comment on attachment 22627 [details]
Add new API functions (JSObjectMake(Array|Date|Error|RegExp)

Your copyright should go into JSObjectRef.h, as well.

The ChangeLog should mention the bug number and what the API added. If you're not going to specifically comment on each function you changed, you can leave that boilerplate function list out.

+ @abstract Creates a JavaScript RefExp object

Typo: "RefExp" should be "RegExp".

Otherwise, this is ready to land.

I have to r- over the copyright issue, though :(.

[Kelvin Sherlock]

Created attachment 22643 [details]
Add new API functions (JSObjectMake(Array|Date|Error|RegExp)

fix typo, add copyright, added more info to change log.

[Sam Weinig]

Comment on attachment 22643 [details]
Add new API functions (JSObjectMake(Array|Date|Error|RegExp)

The * should be on the other side for JSValueRef *exception.  There is also an extra space.
+JSObjectRef JSObjectMakeArray(JSContextRef ctx, size_t argumentCount, const JSValueRef arguments[],  JSValueRef *exception)

+JSObjectRef JSObjectMakeDate(JSContextRef ctx, size_t argumentCount, const JSValueRef arguments[],  JSValueRef *exception)

+JSObjectRef JSObjectMakeDate(JSContextRef ctx, size_t argumentCount, const JSValueRef arguments[],  JSValueRef *exception)

+JSObjectRef JSObjectMakeRegExp(JSContextRef ctx, size_t argumentCount, const JSValueRef arguments[],  JSValueRef *exception)

Same here and few other places.
+    JSObject *result;


We like to use ++i where we can.  This issue exists in a number of  places.
+        for (size_t i = 0; i < argumentCount; i++)

The braces are not needed.
+    } else {
+        result = constructEmptyArray(exec);
+    }

It is 2008!  :)
+ * Copyright (C) 2007 Kelvin W Sherlock ([e-mail])

There are some uses of tabs that need to be removed.

You should also add some tests to the testapi.c test.

r-

[Sam Weinig]

Created attachment 23678 [details]
patch

[Geoffrey Garen]

Comment on attachment 23678 [details]
patch

r=me

[Sam Weinig]

Fixed in r36784.