Bug 40006 - [Chromium] enforce presence of named property query callback if named property enumerator is present
Summary: [Chromium] enforce presence of named property query callback if named propert...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: anton muhin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-01 09:58 PDT by anton muhin
Modified: 2010-06-01 20:28 PDT (History)
5 users (show)

See Also:


Attachments
Patch (7.46 KB, patch)
2010-06-01 10:06 PDT, anton muhin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description anton muhin 2010-06-01 09:58:47 PDT
[Chromium] enforce presence of named property query callback if named property enumerator is present
Comment 1 anton muhin 2010-06-01 10:06:35 PDT
Created attachment 57560 [details]
Patch
Comment 2 Nate Chapin 2010-06-01 10:43:51 PDT
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()?
Comment 3 anton muhin 2010-06-01 10:50:04 PDT
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.
Comment 4 anton muhin 2010-06-01 10:50:54 PDT
or even better, I'll wait until it's all green (hopefully), even more time to revoke r+ :)
Comment 5 Nate Chapin 2010-06-01 10:55:40 PDT
(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!
Comment 6 WebKit Commit Bot 2010-06-01 20:28:28 PDT
Comment on attachment 57560 [details]
Patch

Clearing flags on attachment: 57560

Committed r60531: <http://trac.webkit.org/changeset/60531>
Comment 7 WebKit Commit Bot 2010-06-01 20:28:36 PDT
All reviewed patches have been landed.  Closing bug.