Bug 20020 - Proposed enhancement to JavaScriptCore API
Summary: Proposed enhancement to JavaScriptCore API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-07-12 13:01 PDT by Kelvin Sherlock
Modified: 2008-09-22 16:02 PDT (History)
3 users (show)

See Also:


Attachments
Add new API functions (JSObjectMake(Array|Date|Error|RegExp) (12.51 KB, patch)
2008-07-12 13:07 PDT, Kelvin Sherlock
ggaren: review-
Details | Formatted Diff | Diff
test of new functionality (44.38 KB, application/zip)
2008-07-12 13:11 PDT, Kelvin Sherlock
no flags Details
Add new API functions (JSObjectMake(Array|Date|Error|RegExp) (12.21 KB, patch)
2008-08-03 19:08 PDT, Kelvin Sherlock
ggaren: review-
Details | Formatted Diff | Diff
Add new API functions (JSObjectMake(Array|Date|Error|RegExp) (13.40 KB, patch)
2008-08-04 16:00 PDT, Kelvin Sherlock
sam: review-
Details | Formatted Diff | Diff
patch (15.21 KB, patch)
2008-09-22 15:55 PDT, Sam Weinig
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kelvin Sherlock 2008-07-12 13:01:56 PDT
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.
Comment 1 Kelvin Sherlock 2008-07-12 13:07:50 PDT
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.
Comment 2 Kelvin Sherlock 2008-07-12 13:11:00 PDT
Created attachment 22257 [details]
test of new functionality

XCode project to test new functionality, written in objective c.
Comment 3 Mark Rowe (bdash) 2008-07-12 16:23:46 PDT
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.
Comment 4 Sam Weinig 2008-07-20 15:51:39 PDT
A better place to add tests for new API functionality would be the testapi.c and testapi.js files.
Comment 5 Geoffrey Garen 2008-07-29 17:13:47 PDT
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."

+2008-07-12  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.
Comment 6 Kelvin Sherlock 2008-08-03 19:08:57 PDT
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();
Comment 7 Geoffrey Garen 2008-08-04 12:29:46 PDT
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 :(.
Comment 8 Kelvin Sherlock 2008-08-04 16:00:44 PDT
Created attachment 22643 [details]
Add new API functions (JSObjectMake(Array|Date|Error|RegExp)

fix typo, add copyright, added more info to change log.
Comment 9 Sam Weinig 2008-08-04 19:28:19 PDT
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 (ksherlock@gmail.com)

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

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

r-
Comment 10 Sam Weinig 2008-09-22 15:55:26 PDT
Created attachment 23678 [details]
patch
Comment 11 Geoffrey Garen 2008-09-22 15:58:45 PDT
Comment on attachment 23678 [details]
patch

r=me
Comment 12 Sam Weinig 2008-09-22 16:02:12 PDT
Fixed in r36784.