[webkit-reviews] review denied: [Bug 25031] Chrome allows elements with names to override collection properties : [Attachment 29233] Fix

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied Dave Moore
<davemoore at google.com>'s request for review:
Bug 25031: Chrome allows elements with names to override collection properties
https://bugs.webkit.org/show_bug.cgi?id=25031

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

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
Don't forget bug URL in ChangeLog description.

> +	   (WebCore::collectionNamedPropertyGetter):
> +	   (WebCore::nodeCollectionNamedPropertyGetter):

It's a good practice to add brief descriptions to each line on what's being
done.

> +	   if (!value.IsEmpty())
> +	     return value;

4 spaces.

> +
> +	   // Search local callback properties next to find IDL defined
> +	   // properties.
> +	   if (info.Holder()->HasRealNamedCallbackProperty(name))
> +	     return v8::Handle<v8::Value>();

4 spaces.

Also, I think this should be return notHandledbyInterceptor().

> +	   if (!value.IsEmpty())
> +	     return value;

4 spaces.

> +	   if (info.Holder()->HasRealNamedCallbackProperty(name))
> +	     return v8::Handle<v8::Value>();

4 spaces, notHandledByInterceptor()

> +2009-04-03  Dave Moore  <davemoore at google.com>
> +
> +	   Reviewed by NOBODY (OOPS!).

Need a good description of what the tests test.

> +	       function test()
> +	       {
> +		   if (window.layoutTestController)
> +		       layoutTestController.dumpAsText();

Is this where we should put attempting to override length?

> +
> +		   var sl = document.getElementById("sl");
> +		   if (sl.length == 1)