bindings: Supports [Replaceable] accessors.

bindings: Supports [Replaceable] accessors.

Supports [Replaceable] DOM attributes on prototype chains.

BUG=43394
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190840

[Yuki, 2015-02-23 13:41:03]

yukishiino@chromium.org changed reviewers:
+ haraken@chromium.org

[Yuki, 2015-02-23 13:41:04]

Could you review this CL?

[haraken, 2015-02-23 13:44:06]

Just to confirm, but is it a right thing to move [Replaceable]/[PutForwards] DOM
attributes to prototype chains?

[Yuki, 2015-02-23 13:58:59]

On 2015/02/23 13:44:06, haraken wrote:
> Just to confirm, but is it a right thing to move [Replaceable]/[PutForwards]
DOM
> attributes to prototype chains?

Yes, I think we should move them to prototype chains.
https://heycam.github.io/webidl/#Replaceable
https://heycam.github.io/webidl/#PutForwards

[Replaceable]/[PutForwards] are completely the same as regular (non-static)
attributes in terms of getter.  Only for assigning, they behave in special
manners.

For [Replaceable], assigning to the attribute creates a new own property
|object.attribute| and it shadows the attribute on a prototype chain (it just
shadows, the attribute on the prototype chain keeps existing).  When |delete
object.attribute|, the own property is deleted as usual, and then the attribute
on the prototype chain gets unshadowed.

https://heycam.github.io/webidl/#es-attributes
"Otherwise, the property exists solely on the interface’s interface prototype
object."
should be applied to [Replaceable] and [PutForwards].

[haraken, 2015-02-23 14:48:53]

https://codereview.chromium.org/953473002/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_attributes.py (right):

https://codereview.chromium.org/953473002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_attributes.py:438: ('Replaceable' in
extended_attributes or

Why can we remove "'PutForwards' not in extended_attributes" ?

https://codereview.chromium.org/953473002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_attributes.py:440: return
'%sV8Internal::%sForceSetAttributeOnThisCallback' % (

Help me understand: What's a difference between a [Replaceable] attribute on a
prototype chain and a [Replaceable] attribute on an instance? I'm wondering why
you don't need to call ForceSet for a [Replaceable] attribute on a prototype
chain.

https://codereview.chromium.org/953473002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_attributes.py:444: 'Replaceable' not in
extended_attributes):

Fix indentation.

https://codereview.chromium.org/953473002/diff/20001/Source/bindings/template...
File Source/bindings/templates/attributes.cpp (right):

https://codereview.chromium.org/953473002/diff/20001/Source/bindings/template...
Source/bindings/templates/attributes.cpp:214: {% if not attribute.is_static and
not attribute.is_replaceable %}

For readability, it seems better to remove this branch. You can directly write
"v8::Local<v8::Object> holder = info.Holder()" just before where you actually
need the |holder|.

https://codereview.chromium.org/953473002/diff/20001/Source/bindings/template...
Source/bindings/templates/attributes.cpp:230: {% if is_check_security %}

Why do we have the is_check_security clause only in the attribute.is_replaceable
clause? I guess the is_check_security clause would be needed in other clauses
(e.g., the attribute.put_forwards clause) as well.

[Yuki, 2015-02-23 15:18:42]

https://codereview.chromium.org/953473002/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_attributes.py (right):

https://codereview.chromium.org/953473002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_attributes.py:438: ('Replaceable' in
extended_attributes or
On 2015/02/23 14:48:52, haraken wrote:
> 
> Why can we remove "'PutForwards' not in extended_attributes" ?

[Replaceable] and [PutForwards] conflict each other, and I assume that both of
them are never sepecified at a time.  Validation of IDLs should be done
somewhere else.

https://codereview.chromium.org/953473002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_attributes.py:440: return
'%sV8Internal::%sForceSetAttributeOnThisCallback' % (
On 2015/02/23 14:48:52, haraken wrote:
> 
> Help me understand: What's a difference between a [Replaceable] attribute on a
> prototype chain and a [Replaceable] attribute on an instance? I'm wondering
why
> you don't need to call ForceSet for a [Replaceable] attribute on a prototype
> chain.

Basically we have the same implementation for "replaceable on prototype" and
"replaceable on instance".  We call v8::Object::ForceSet to set an own property
in either case.

The point is that
a) for accessor version, the callback function takes only
v8::FunctionCallbackInfo, and there is no way to tell the property name from the
argument.  So, in this CL, we're going to create
  %sAttributeSetterCallback(v8::FunctionCallbackInfo)
where we can tell the property name as "{{attribute.name}}".

b) for attribute (non-accessor) version, the callback function takes two
arguments: property name as v8::String and the v8::PropertyCallbackInfo.  So we
can re-use the same function, named %sForceSetAttributeOnThisCallback, for each
property.

https://codereview.chromium.org/953473002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_attributes.py:444: 'Replaceable' not in
extended_attributes):
On 2015/02/23 14:48:52, haraken wrote:
> 
> Fix indentation.

We need this indentation.  Otherwise, I got a lint warning:
"visually indented line with same indent as next logical line".

I followed this way:
http://stackoverflow.com/questions/21592569/how-to-use-continuation-line-over...

https://codereview.chromium.org/953473002/diff/20001/Source/bindings/template...
File Source/bindings/templates/attributes.cpp (right):

https://codereview.chromium.org/953473002/diff/20001/Source/bindings/template...
Source/bindings/templates/attributes.cpp:214: {% if not attribute.is_static and
not attribute.is_replaceable %}
On 2015/02/23 14:48:52, haraken wrote:
> 
> For readability, it seems better to remove this branch. You can directly write
> "v8::Local<v8::Object> holder = info.Holder()" just before where you actually
> need the |holder|.

Most of attributes need |holder|.  Only static attribute and replaceable
attribute do not need the holder.  Removing this branch makes things very
difficult, I think.

For example, line 218 and line 225, 243 use |holder|.  It's hard to write
"holder = info.Holder()" just before them without duplication.

https://codereview.chromium.org/953473002/diff/20001/Source/bindings/template...
Source/bindings/templates/attributes.cpp:230: {% if is_check_security %}
On 2015/02/23 14:48:52, haraken wrote:
> 
> Why do we have the is_check_security clause only in the
attribute.is_replaceable
> clause? I guess the is_check_security clause would be needed in other clauses
> (e.g., the attribute.put_forwards clause) as well.

The code was copied from
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

I've not yet investigated about PutForwards.  I'll fix it if we need the check
for PutForwards, too, when I work on PutForwards.

However, PutForwards delegates the assignment to another attribute, so it's
possible that it's not the same as Replaceable.

[haraken, 2015-02-23 17:34:00]

Thanks for a lot of clarifications!

https://codereview.chromium.org/953473002/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_attributes.py (right):

https://codereview.chromium.org/953473002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_attributes.py:438: ('Replaceable' in
extended_attributes or
On 2015/02/23 15:18:42, Yuki wrote:
> On 2015/02/23 14:48:52, haraken wrote:
> > 
> > Why can we remove "'PutForwards' not in extended_attributes" ?
> 
> [Replaceable] and [PutForwards] conflict each other, and I assume that both of
> them are never sepecified at a time.  Validation of IDLs should be done
> somewhere else.

Sounds like a good idea.

https://codereview.chromium.org/953473002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_attributes.py:440: return
'%sV8Internal::%sForceSetAttributeOnThisCallback' % (
On 2015/02/23 15:18:42, Yuki wrote:
> On 2015/02/23 14:48:52, haraken wrote:
> > 
> > Help me understand: What's a difference between a [Replaceable] attribute on
a
> > prototype chain and a [Replaceable] attribute on an instance? I'm wondering
> why
> > you don't need to call ForceSet for a [Replaceable] attribute on a prototype
> > chain.
> 
> Basically we have the same implementation for "replaceable on prototype" and
> "replaceable on instance".  We call v8::Object::ForceSet to set an own
property
> in either case.
> 
> The point is that
> a) for accessor version, the callback function takes only
> v8::FunctionCallbackInfo, and there is no way to tell the property name from
the
> argument.  So, in this CL, we're going to create
>   %sAttributeSetterCallback(v8::FunctionCallbackInfo)
> where we can tell the property name as "{{attribute.name}}".
> 
> b) for attribute (non-accessor) version, the callback function takes two
> arguments: property name as v8::String and the v8::PropertyCallbackInfo.  So
we
> can re-use the same function, named %sForceSetAttributeOnThisCallback, for
each
> property.

Ah, now I understand what this CL is doing :)

It seems a bit confusing to me to use different names between attributes on
prototype chains and attributes on instances. Can we use the same method name?

For example, what happens if we just remove the if branch at line 437 ? (What
happens if we unconditionally use '%sV8Internal::%sAttributeSetterCallback'?)

At the very least, by using the same method name for attributes on prototype
chains and attributes on instances, I guess we can remove a bunch of
'is_expose_js_accessors' checks from this CL, because we no longer need to
distinguish the two methods.

[haraken, 2015-02-23 17:36:41]

On 2015/02/23 17:34:00, haraken wrote:
> Thanks for a lot of clarifications!
> 
>
https://codereview.chromium.org/953473002/diff/20001/Source/bindings/scripts/...
> File Source/bindings/scripts/v8_attributes.py (right):
> 
>
https://codereview.chromium.org/953473002/diff/20001/Source/bindings/scripts/...
> Source/bindings/scripts/v8_attributes.py:438: ('Replaceable' in
> extended_attributes or
> On 2015/02/23 15:18:42, Yuki wrote:
> > On 2015/02/23 14:48:52, haraken wrote:
> > > 
> > > Why can we remove "'PutForwards' not in extended_attributes" ?
> > 
> > [Replaceable] and [PutForwards] conflict each other, and I assume that both
of
> > them are never sepecified at a time.  Validation of IDLs should be done
> > somewhere else.
> 
> Sounds like a good idea.
> 
>
https://codereview.chromium.org/953473002/diff/20001/Source/bindings/scripts/...
> Source/bindings/scripts/v8_attributes.py:440: return
> '%sV8Internal::%sForceSetAttributeOnThisCallback' % (
> On 2015/02/23 15:18:42, Yuki wrote:
> > On 2015/02/23 14:48:52, haraken wrote:
> > > 
> > > Help me understand: What's a difference between a [Replaceable] attribute
on
> a
> > > prototype chain and a [Replaceable] attribute on an instance? I'm
wondering
> > why
> > > you don't need to call ForceSet for a [Replaceable] attribute on a
prototype
> > > chain.
> > 
> > Basically we have the same implementation for "replaceable on prototype" and
> > "replaceable on instance".  We call v8::Object::ForceSet to set an own
> property
> > in either case.
> > 
> > The point is that
> > a) for accessor version, the callback function takes only
> > v8::FunctionCallbackInfo, and there is no way to tell the property name from
> the
> > argument.  So, in this CL, we're going to create
> >   %sAttributeSetterCallback(v8::FunctionCallbackInfo)
> > where we can tell the property name as "{{attribute.name}}".
> > 
> > b) for attribute (non-accessor) version, the callback function takes two
> > arguments: property name as v8::String and the v8::PropertyCallbackInfo.  So
> we
> > can re-use the same function, named %sForceSetAttributeOnThisCallback, for
> each
> > property.
> 
> Ah, now I understand what this CL is doing :)
> 
> It seems a bit confusing to me to use different names between attributes on
> prototype chains and attributes on instances. Can we use the same method name?
> 
> For example, what happens if we just remove the if branch at line 437 ? (What
> happens if we unconditionally use '%sV8Internal::%sAttributeSetterCallback'?)
> 
> At the very least, by using the same method name for attributes on prototype
> chains and attributes on instances, I guess we can remove a bunch of
> 'is_expose_js_accessors' checks from this CL, because we no longer need to
> distinguish the two methods.

Note that for normal attribute getters/setters, we use the same method name for
both prototype and instance version and just switches the method parameters
(FunctionCallbackInfo or PropertyCallbackInfo).

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[Yuki, 2015-02-25 14:19:29]

https://codereview.chromium.org/953473002/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_attributes.py (right):

https://codereview.chromium.org/953473002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_attributes.py:440: return
'%sV8Internal::%sForceSetAttributeOnThisCallback' % (
On 2015/02/23 17:33:59, haraken wrote:
> On 2015/02/23 15:18:42, Yuki wrote:
> > On 2015/02/23 14:48:52, haraken wrote:
> > > 
> > > Help me understand: What's a difference between a [Replaceable] attribute
on
> a
> > > prototype chain and a [Replaceable] attribute on an instance? I'm
wondering
> > why
> > > you don't need to call ForceSet for a [Replaceable] attribute on a
prototype
> > > chain.
> > 
> > Basically we have the same implementation for "replaceable on prototype" and
> > "replaceable on instance".  We call v8::Object::ForceSet to set an own
> property
> > in either case.
> > 
> > The point is that
> > a) for accessor version, the callback function takes only
> > v8::FunctionCallbackInfo, and there is no way to tell the property name from
> the
> > argument.  So, in this CL, we're going to create
> >   %sAttributeSetterCallback(v8::FunctionCallbackInfo)
> > where we can tell the property name as "{{attribute.name}}".
> > 
> > b) for attribute (non-accessor) version, the callback function takes two
> > arguments: property name as v8::String and the v8::PropertyCallbackInfo.  So
> we
> > can re-use the same function, named %sForceSetAttributeOnThisCallback, for
> each
> > property.
> 
> Ah, now I understand what this CL is doing :)
> 
> It seems a bit confusing to me to use different names between attributes on
> prototype chains and attributes on instances. Can we use the same method name?
> 
> For example, what happens if we just remove the if branch at line 437 ? (What
> happens if we unconditionally use '%sV8Internal::%sAttributeSetterCallback'?)
> 
> At the very least, by using the same method name for attributes on prototype
> chains and attributes on instances, I guess we can remove a bunch of
> 'is_expose_js_accessors' checks from this CL, because we no longer need to
> distinguish the two methods.

Done.

[haraken, 2015-02-25 14:27:34]

LGTM

https://codereview.chromium.org/953473002/diff/80001/Source/bindings/template...
File Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/953473002/diff/80001/Source/bindings/template...
Source/bindings/templates/interface_base.cpp:68: if (info.This()->IsObject())

Not related to your CL, I guess this is always true. I think we can change this
to ASSERT(info.This()->IsObject()) in a follow-up CL.

https://codereview.chromium.org/953473002/diff/80001/Source/bindings/template...
Source/bindings/templates/interface_base.cpp:74: static void
{{cpp_class}}ForceSetAttributeOnThisCallback(v8::Local<v8::String> name,
v8::Local<v8::Value> v8Value, const v8::PropertyCallbackInfo<void>& info)

Not related to your CL, do we need this method indirection?

[Yuki, 2015-02-25 14:33:12]

https://codereview.chromium.org/953473002/diff/80001/Source/bindings/template...
File Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/953473002/diff/80001/Source/bindings/template...
Source/bindings/templates/interface_base.cpp:68: if (info.This()->IsObject())
On 2015/02/25 14:27:34, haraken wrote:
> 
> Not related to your CL, I guess this is always true. I think we can change
this
> to ASSERT(info.This()->IsObject()) in a follow-up CL.

Will do.

https://codereview.chromium.org/953473002/diff/80001/Source/bindings/template...
Source/bindings/templates/interface_base.cpp:74: static void
{{cpp_class}}ForceSetAttributeOnThisCallback(v8::Local<v8::String> name,
v8::Local<v8::Value> v8Value, const v8::PropertyCallbackInfo<void>& info)
On 2015/02/25 14:27:34, haraken wrote:
> 
> Not related to your CL, do we need this method indirection?

I'm still studying what constructor attributes are, but they seem very close to
replaceable attributes.  (So I'm working on replaceable attributes before
constructor attributes.)

Maybe, this function will be gone when I work on constructor attributes.

[Yuki, 2015-02-25 14:33:38]

The CQ bit was checked by yukishiino@chromium.org

[commit-bot: I haz the power, 2015-02-25 14:33:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/953473002/80001

[commit-bot: I haz the power, 2015-02-25 16:27:26]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190840