[webkit-reviews] review denied: [Bug 25739] Upstream V8 bindings for HTMLOptionsCollection and HTMLSelectElementCollection : [Attachment 30252] patch

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied Nate Chapin
<japhet at google.com>'s request for review:
Bug 25739: Upstream V8 bindings for HTMLOptionsCollection and
HTMLSelectElementCollection
https://bugs.webkit.org/show_bug.cgi?id=25739

Attachment 30252: patch
https://bugs.webkit.org/attachment.cgi?id=30252&action=review

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
> +INDEXED_PROPERTY_SETTER(HTMLOptionsCollection) {

brace on new line.

> +v8::Handle<v8::Value> getOptionsCollectionSetter(uint32_t index,
v8::Handle<v8::Value> value, HTMLSelectElement* base) {

brace on new line, and to prefix instead of get. Use "get" when the result is a
ptr argument. http://webkit.org/coding/coding-style.html

I wonder though if this method really belongs in V8Collection.h (perhaps need a
V8Collection.cpp added?)
  

> Property changes on:
WebCore/bindings/v8/custom/V8HTMLOptionsCollectionCustom.h
> ___________________________________________________________________
> Name: svn:executable
>    + *
> 

Whoops! What are these props doing here?

> +NAMED_PROPERTY_GETTER(HTMLSelectElementCollection) {

brace on new line.

> +    // Search local callback properties next to find IDL defined 
properties.
> +    if (info.Holder()->HasRealNamedCallbackProperty(name))
> +	       return notHandledByInterceptor();

Something funny with indent here. 4 spaces.

> +    if (!items.size())
> +	   return v8::Handle<v8::Value>();

return notHandledByInterceptor();

> +INDEXED_PROPERTY_SETTER(HTMLSelectElementCollection) {

brace on new line.

> +    return WebCore::getOptionsCollectionSetter(index, value, select);

No need for namespace prefix here. We're in WebCore.

> Property changes on:
WebCore/bindings/v8/custom/V8HTMLSelectElementCollectionCustom.cpp
> ___________________________________________________________________
> Name: svn:executable
>    + *
> 

No extra props, pls :)