Bug 40006 – [Chromium] enforce presence of named property query callback if named property enumerator is present

• Component: New Bugs
• Status: RESOLVED FIXED

[anton muhin]

[Chromium] enforce presence of named property query callback if named property enumerator is present

[anton muhin]

Created attachment 57560 [details]
Patch

[Nate Chapin]

Comment on attachment 57560 [details]
Patch

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..5c3356ff74f260128f2ebca04a9e6e5567b897b4 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()?

[anton muhin]

Nate, thanks a lot for a quick review.

(In reply to comment #2)
> (From update of attachment 57560 [details])
> 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?

Now we'll get compile time error which might be better then dieing, but up to you, if you think that we should die, I'd add that.

> 
> > diff --git a/WebCore/bindings/v8/V8NPObject.cpp b/WebCore/bindings/v8/V8NPObject.cpp
> > index f9cc94a9fe465beae74db7d73d512176801332e3..5c3356ff74f260128f2ebca04a9e6e5567b897b4 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>().

There is a difference in semantics: if empty handle is returned, it says to v8, 'I don't have this property, but you could look down the prototype chain', but if False is returned it means, 'no, there is no such property (and ignore what prototype chain would tell you'.  As we need all the properties, we'd better return empty handle here.  Still I agree the semantics is somewhat weird and I cannot immediately invent situation where False would be useful.

> 
> 
> > +    if (storage->contains(name) && name != "length")
> > +        return v8::True();
> > +
> > +    return v8::Handle<v8::Boolean>();
> 
> Same here: Why not v8::False()?

Ditto.

Nate, I'm cq+'ing it right now--cq has many pending patches so you should have enough time to remove cq+ if you'd like me to address anything.

[anton muhin]

or even better, I'll wait until it's all green (hopefully), even more time to revoke r+ :)

[Nate Chapin]

(In reply to comment #3)
> Nate, thanks a lot for a quick review.
> 
> (In reply to comment #2)
> > (From update of attachment 57560 [details] [details])
> > 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?
> 
> Now we'll get compile time error which might be better then dieing, but up to you, if you think that we should die, I'd add that.

I don't have any strong feelings, I just figured I'd throw it out as an option.  It's not really necessary though, since we just compile fail if there are missing custom bindings (and that's probably the closest existing situation to what you're doing).

> 
> > 
> > > diff --git a/WebCore/bindings/v8/V8NPObject.cpp b/WebCore/bindings/v8/V8NPObject.cpp
> > > index f9cc94a9fe465beae74db7d73d512176801332e3..5c3356ff74f260128f2ebca04a9e6e5567b897b4 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>().
> 
> There is a difference in semantics: if empty handle is returned, it says to v8, 'I don't have this property, but you could look down the prototype chain', but if False is returned it means, 'no, there is no such property (and ignore what prototype chain would tell you'.  As we need all the properties, we'd better return empty handle here.  Still I agree the semantics is somewhat weird and I cannot immediately invent situation where False would be useful.

Ah....I haven't done much with v8::Booleans, so I hadn't realized that there was a difference.  My quick search through the code tricked me.

> 
> > 
> > 
> > > +    if (storage->contains(name) && name != "length")
> > > +        return v8::True();
> > > +
> > > +    return v8::Handle<v8::Boolean>();
> > 
> > Same here: Why not v8::False()?
> 
> Ditto.
> 
> Nate, I'm cq+'ing it right now--cq has many pending patches so you should have enough time to remove cq+ if you'd like me to address anything.

Ok, yeah, I think cq+ is fine. Thanks for clarifying!

[WebKit Commit Bot]

Comment on attachment 57560 [details]
Patch

Clearing flags on attachment: 57560

Committed r60531: <http://trac.webkit.org/changeset/60531>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.