[webkit-reviews] review granted: [Bug 40006] [Chromium] enforce presence of named property query callback if named property enumerator is present : [Attachment 57560] Patch

[bugzilla-daemon at webkit.org]

Nate Chapin <japhet at chromium.org> has granted anton muhin
<antonm at chromium.org>'s request for review:
Bug 40006: [Chromium] enforce presence of named property query callback if
named property enumerator is present
https://bugs.webkit.org/show_bug.cgi?id=40006

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

------- Additional Comments from Nate Chapin <japhet at chromium.org>
A couple of nits, looks ok otherwise.

> +    # If there is a custom enumerator, there MUST be custom query to
properly communicate property attributes.

Should we be die'ing if this isn't the case?

> diff --git a/WebCore/bindings/v8/V8NPObject.cpp
b/WebCore/bindings/v8/V8NPObject.cpp
> index
f9cc94a9fe465beae74db7d73d512176801332e3..5c3356ff74f260128f2ebca04a9e6e5567b89
7b4 100644
> --- a/WebCore/bindings/v8/V8NPObject.cpp
> +++ b/WebCore/bindings/v8/V8NPObject.cpp
> @@ -243,6 +243,12 @@ v8::Handle<v8::Value>
npObjectGetIndexedProperty(v8::Local<v8::Object> self, uin
>      return npObjectGetProperty(self, identifier, v8::Number::New(index));
>  }
>  
> +v8::Handle<v8::Boolean> npObjectQueryProperty(v8::Local<v8::String> name,
const v8::AccessorInfo& info)
> +{
> +    NPIdentifier identifier = getStringIdentifier(name);
> +    return npObjectGetProperty(info.Holder(), identifier, name).IsEmpty() ?
v8::Handle<v8::Boolean>() : v8::True();

Nit: I don't know what the canonical way to use the V8 API is in this respect,
but I would think it clearer to use v8::False() instead of
v8::Handle<v8::Boolean>().


> +    if (storage->contains(name) && name != "length")
> +	   return v8::True();
> +
> +    return v8::Handle<v8::Boolean>();

Same here: Why not v8::False()?