[webkit-reviews] review granted: [Bug 28724] [ES5] Implement getOwnPropertyDescriptor : [Attachment 38578] JavaScriptCore portion of patch

[bugzilla-daemon at webkit.org]

Gavin Barraclough <barraclough at apple.com> has granted Oliver Hunt
<oliver at apple.com>'s request for review:
Bug 28724: [ES5] Implement getOwnPropertyDescriptor
https://bugs.webkit.org/show_bug.cgi?id=28724

Attachment 38578: JavaScriptCore portion of patch
https://bugs.webkit.org/attachment.cgi?id=38578&action=review

------- Additional Comments from Gavin Barraclough <barraclough at apple.com>
> +bool JSVariableObject::symbolTableGet(const Identifier& propertyName,
PropertyDescriptor& descriptor)

I don't really like this name.	I expect get to just get values, whereas this
gets a description.  It seems that all other methods that retrieve
PropertyDescriptors have 'Descriptor' in their name, so it seems like this
method should too.

>      /**
> +	* Simplified version of getStaticPropertyDescriptor in case there are
no functions, only "values".
> +	* Using this instead of getStaticPropertyDescriptor removes the need
for a FuncImp class.
> +	*/

The phrase "in case there are no functions" reads oddly to me (and with a
different meaning than I think you intend).  I would suggest that "in cases
where there are no functions" reads very differently to me (and with a meaning
closer to what I think you want).  The same goes for the comment on partner
method for only functions.

>  ObjectConstructor::ObjectConstructor(ExecState* exec, PassRefPtr<Structure>
structure, ObjectPrototype* objectPrototype, Structure*
prototypeFunctionStructure)
> -    : InternalFunction(&exec->globalData(), structure, Identifier(exec,
"Object"))
> +: InternalFunction(&exec->globalData(), structure, Identifier(exec,
"Object"))
>  {
>      // ECMA 15.2.3.1
>      putDirectWithoutTransition(exec->propertyNames().prototype,
objectPrototype, DontEnum | DontDelete | ReadOnly);
> -
> +    
>      // no. of arguments for constructor
>      putDirectWithoutTransition(exec->propertyNames().length, jsNumber(exec,
1), ReadOnly | DontEnum | DontDelete);
> -
> +    
>      putDirectFunctionWithoutTransition(exec, new (exec)
NativeFunctionWrapper(exec, prototypeFunctionStructure, 1,
exec->propertyNames().getPrototypeOf, objectConstructorGetPrototypeOf),
DontEnum);
> +    putDirectFunctionWithoutTransition(exec, new (exec)
NativeFunctionWrapper(exec, prototypeFunctionStructure, 2,
exec->propertyNames().getOwnPropertyDescriptor,
objectConstructorGetOwnPropertyDescriptor), DontEnum);
>  }

3x garbled whitespace.