[webkit-reviews] review denied: [Bug 20020] Proposed enhancement to JavaScriptCore API : [Attachment 22256] Add new API functions (JSObjectMake(Array|Date|Error|RegExp)

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Kelvin Sherlock
<ksherlock at gmail.com>'s request for review:
Bug 20020: Proposed enhancement to JavaScriptCore API
https://bugs.webkit.org/show_bug.cgi?id=20020

Attachment 22256: Add new API functions (JSObjectMake(Array|Date|Error|RegExp)
https://bugs.webkit.org/attachment.cgi?id=22256&action=edit

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
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.