bindings: Introduces on_{instance,prototype,interface} in the code generator.

bindings: Introduces on_{instance,prototype,interface} in the code generator.

Interface members may be located on one or two of instance object, prototype
object and/or interface object.  Refactors the code generator toward to
controlling the property location more logically based on the spec's
requirement.

This CL is intended to simply introduce new template variables:
- on_instance
- on_prototype
- on_interface
and intended to not change the behavior.

I'm planning to make the following changes.
- Make all attributes accessor-type properties.
- Remove is_expose_js_accessors template var.
- Register static members using install{Accessors,Methods,Constants}.
- Apply on_{instance,prototype,interface} to methods, too.
- Update v8_utilities.on_{instance,prototype,interface} functions to conform to the spec.
(The order is NOT chronological.)

BUG=

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

[Yuki, 2015-06-17 09:27:12]

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

[Yuki, 2015-06-17 09:27:13]

Could you guys review this CL?

[haraken, 2015-06-17 16:31:21]

LGTM with a bunch of nits.

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/core/v8...
Source/bindings/core/v8/V8DOMConfiguration.cpp:58: if
(attribute.propertyLocationConfiguration & V8DOMConfiguration::OnPrototype)

if => else if?

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/core/v8...
Source/bindings/core/v8/V8DOMConfiguration.cpp:60: if
(attribute.propertyLocationConfiguration & V8DOMConfiguration::OnInterface)

if => else if?

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/core/v8...
Source/bindings/core/v8/V8DOMConfiguration.cpp:103: if
(accessor.propertyLocationConfiguration & V8DOMConfiguration::OnPrototype)

Ditto.

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/core/v8...
Source/bindings/core/v8/V8DOMConfiguration.cpp:105: if
(accessor.propertyLocationConfiguration & V8DOMConfiguration::OnInterface)

Ditto.

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/scripts...
File Source/bindings/scripts/v8_utilities.py (right):

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/scripts...
Source/bindings/scripts/v8_utilities.py:230: # TODO: replace this with
[Constructor] and [NamedConstructor] extended attribute

TODO(yukishiino)

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/scripts...
Source/bindings/scripts/v8_utilities.py:414: # [Unforgeable],
[OverrideBuiltins], [Global], [PrimaryGlobal] and [ExposeJSAccessors] /
[DoNotExposeJSAccessors]

This comment is a bit confusing.

- You have the same comment on on_instance, on_prototype and on_interface.

- You say "[ExposeJSAccessors] / [DoNotExposeJSAccessors]", but the former
should exist on prototype whereas the latter should exist on instance.

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/scripts...
Source/bindings/scripts/v8_utilities.py:455: return False

It's a bit confusing that the priority of [ExposeJSAccessors] is different
between [ExposeJSAccessors] on members and [ExposeJSAccessors] on interfaces.

BTW, is there any [ExposeJSAccessors] used on interfaces?

(If you have a plan to remove [ExposeJSAccessors] in the near future, I'm OK
with the current code though.)

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/scripts...
Source/bindings/scripts/v8_utilities.py:462: return False

Just help me understand: When should on_interface return True?

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/templat...
File Source/bindings/templates/attributes.cpp (right):

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/templat...
Source/bindings/templates/attributes.cpp:535: {% if attribute.on_prototype %}

if => elif ?

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/templat...
Source/bindings/templates/attributes.cpp:538: {% if attribute.on_interface %}

if => elif ?

Maybe it would be better to use attribute.property_location, which takes either
of "instance", "prototype" or "interface", instead of introducing
attribute.on_instance, attribute.on_prototype and attribute.on_interface.

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/templat...
Source/bindings/templates/attributes.cpp:555: holder_check,

alphabetical order

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/templat...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:812: #error No one is actually using
per-member [Exposed] extended attribute.  Removed the implementation.

Removed => Remove

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:818: #error No one is actually using
per-member [Exposed] extended attribute.  Removed the implementation.

Ditto.

[bashi, 2015-06-18 02:16:28]

LGTM

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/core/v8...
Source/bindings/core/v8/V8DOMConfiguration.cpp:106:
interfaceTemplate->SetAccessorProperty(name, getter, setter, accessor.attribute,
accessor.settings);
Add ASSERT()?

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/scripts...
File Source/bindings/scripts/v8_utilities.py (right):

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/scripts...
Source/bindings/scripts/v8_utilities.py:414: # [Unforgeable],
[OverrideBuiltins], [Global], [PrimaryGlobal] and [ExposeJSAccessors] /
[DoNotExposeJSAccessors]
On 2015/06/17 16:31:21, haraken wrote:
> 
> This comment is a bit confusing.
> 
> - You have the same comment on on_instance, on_prototype and on_interface.
> 
> - You say "[ExposeJSAccessors] / [DoNotExposeJSAccessors]", but the former
> should exist on prototype whereas the latter should exist on instance.

I'd recommend to have docstring to describe how this method (and other on_xxx
methods) is used.

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/scripts...
Source/bindings/scripts/v8_utilities.py:449: if ('Custom' in
member.extended_attributes):
nit: drop parenthesis.

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/templat...
File Source/bindings/templates/attributes.cpp (right):

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/templat...
Source/bindings/templates/attributes.cpp:555: holder_check,
On 2015/06/17 16:31:21, haraken wrote:
> 
> alphabetical order

These are for AttributeConfiguration struct so we can't make them alphabetical
order.

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/templat...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:812: #error No one is actually using
per-member [Exposed] extended attribute.  Removed the implementation.
On 2015/06/17 16:31:21, haraken wrote:
> 
> Removed => Remove

Maybe this error says that the implementation has gone? (We no longer support
[Exposed] on members?)

[Yuki, 2015-06-18 07:44:41]

Could you guys take another look?

Note that one member can be defined on both of instance and interface object (or
prototype and interface, etc.).  The spec does not require that it's exclusive
whether the member is on instance or on prototype.  So I'd like to have simple
bitflags about where a member is defined.

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/core/v8...
Source/bindings/core/v8/V8DOMConfiguration.cpp:58: if
(attribute.propertyLocationConfiguration & V8DOMConfiguration::OnPrototype)
On 2015/06/17 16:31:21, haraken wrote:
> 
> if => else if?

No, I'd like to make attribute.propertyLocationConfiguration bitflags, so the
member can be located on instance, prototype AND/OR interface.

For example, constants must be placed on both of prototype object and interface
object.  It's not necessarily exclusive.

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/core/v8...
Source/bindings/core/v8/V8DOMConfiguration.cpp:106:
interfaceTemplate->SetAccessorProperty(name, getter, setter, accessor.attribute,
accessor.settings);
On 2015/06/18 02:16:26, bashi1 wrote:
> Add ASSERT()?

Added ASSERT(accessor.propertyLocationConfiguration); to check that it never be
zero.

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/scripts...
File Source/bindings/scripts/v8_utilities.py (right):

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/scripts...
Source/bindings/scripts/v8_utilities.py:230: # TODO: replace this with
[Constructor] and [NamedConstructor] extended attribute
On 2015/06/17 16:31:21, haraken wrote:
> 
> TODO(yukishiino)

Done.

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/scripts...
Source/bindings/scripts/v8_utilities.py:414: # [Unforgeable],
[OverrideBuiltins], [Global], [PrimaryGlobal] and [ExposeJSAccessors] /
[DoNotExposeJSAccessors]
On 2015/06/18 02:16:27, bashi1 wrote:
> On 2015/06/17 16:31:21, haraken wrote:
> > 
> > This comment is a bit confusing.
> > 
> > - You have the same comment on on_instance, on_prototype and on_interface.
> > 
> > - You say "[ExposeJSAccessors] / [DoNotExposeJSAccessors]", but the former
> > should exist on prototype whereas the latter should exist on instance.
> 
> I'd recommend to have docstring to describe how this method (and other on_xxx
> methods) is used.

Done.

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/scripts...
Source/bindings/scripts/v8_utilities.py:414: # [Unforgeable],
[OverrideBuiltins], [Global], [PrimaryGlobal] and [ExposeJSAccessors] /
[DoNotExposeJSAccessors]
On 2015/06/17 16:31:21, haraken wrote:
> 
> This comment is a bit confusing.
> 
> - You have the same comment on on_instance, on_prototype and on_interface.
> 
> - You say "[ExposeJSAccessors] / [DoNotExposeJSAccessors]", but the former
> should exist on prototype whereas the latter should exist on instance.

Done.

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/scripts...
Source/bindings/scripts/v8_utilities.py:449: if ('Custom' in
member.extended_attributes):
On 2015/06/18 02:16:27, bashi1 wrote:
> nit: drop parenthesis.

Done.

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/scripts...
Source/bindings/scripts/v8_utilities.py:455: return False
On 2015/06/17 16:31:21, haraken wrote:
> 
> It's a bit confusing that the priority of [ExposeJSAccessors] is different
> between [ExposeJSAccessors] on members and [ExposeJSAccessors] on interfaces.

Well, however, we should not place static members on the prototype object even
if [ExposeJSAccessors] is specified for an interface.  [ExposeJSAccessors] on
interfaces means that "regular members" will be placed on the prototype object. 
We need to check if the member is a regular member or not before applying the
interface-wide default setting.


> BTW, is there any [ExposeJSAccessors] used on interfaces?

No, there is no effective ones.

> (If you have a plan to remove [ExposeJSAccessors] in the near future, I'm OK
> with the current code though.)

Optimistically, we can get rid of [ExposeJSAccessors] in a month or two, I
think.

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/scripts...
Source/bindings/scripts/v8_utilities.py:462: return False
On 2015/06/17 16:31:21, haraken wrote:
> 
> Just help me understand: When should on_interface return True?

constants and static members must be defined on the interface object.

I'm planning to handle them here.

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/templat...
File Source/bindings/templates/attributes.cpp (right):

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/templat...
Source/bindings/templates/attributes.cpp:538: {% if attribute.on_interface %}
On 2015/06/17 16:31:21, haraken wrote:
> 
> if => elif ?
> 
> Maybe it would be better to use attribute.property_location, which takes
either
> of "instance", "prototype" or "interface", instead of introducing
> attribute.on_instance, attribute.on_prototype and attribute.on_interface.

I intend to make property_location bitflags.  One member can be placed at many
places.

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/templat...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:812: #error No one is actually using
per-member [Exposed] extended attribute.  Removed the implementation.
On 2015/06/18 02:16:27, bashi1 wrote:
> On 2015/06/17 16:31:21, haraken wrote:
> > 
> > Removed => Remove
> 
> Maybe this error says that the implementation has gone? (We no longer support
> [Exposed] on members?)

Yes, I meant so.  I updated the error message so it's clearer.

[haraken, 2015-06-18 07:50:51]

On 2015/06/18 07:44:41, Yuki wrote:
> Could you guys take another look?
> 
> Note that one member can be defined on both of instance and interface object
(or
> prototype and interface, etc.).  The spec does not require that it's exclusive
> whether the member is on instance or on prototype.  So I'd like to have simple
> bitflags about where a member is defined.
> 
>
https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/core/v8...
> File Source/bindings/core/v8/V8DOMConfiguration.cpp (right):
> 
>
https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/core/v8...
> Source/bindings/core/v8/V8DOMConfiguration.cpp:58: if
> (attribute.propertyLocationConfiguration & V8DOMConfiguration::OnPrototype)
> On 2015/06/17 16:31:21, haraken wrote:
> > 
> > if => else if?
> 
> No, I'd like to make attribute.propertyLocationConfiguration bitflags, so the
> member can be located on instance, prototype AND/OR interface.
> 
> For example, constants must be placed on both of prototype object and
interface
> object.  It's not necessarily exclusive.
> 
>
https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/core/v8...
> Source/bindings/core/v8/V8DOMConfiguration.cpp:106:
> interfaceTemplate->SetAccessorProperty(name, getter, setter,
accessor.attribute,
> accessor.settings);
> On 2015/06/18 02:16:26, bashi1 wrote:
> > Add ASSERT()?
> 
> Added ASSERT(accessor.propertyLocationConfiguration); to check that it never
be
> zero.
> 
>
https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/scripts...
> File Source/bindings/scripts/v8_utilities.py (right):
> 
>
https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/scripts...
> Source/bindings/scripts/v8_utilities.py:230: # TODO: replace this with
> [Constructor] and [NamedConstructor] extended attribute
> On 2015/06/17 16:31:21, haraken wrote:
> > 
> > TODO(yukishiino)
> 
> Done.
> 
>
https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/scripts...
> Source/bindings/scripts/v8_utilities.py:414: # [Unforgeable],
> [OverrideBuiltins], [Global], [PrimaryGlobal] and [ExposeJSAccessors] /
> [DoNotExposeJSAccessors]
> On 2015/06/18 02:16:27, bashi1 wrote:
> > On 2015/06/17 16:31:21, haraken wrote:
> > > 
> > > This comment is a bit confusing.
> > > 
> > > - You have the same comment on on_instance, on_prototype and on_interface.
> > > 
> > > - You say "[ExposeJSAccessors] / [DoNotExposeJSAccessors]", but the former
> > > should exist on prototype whereas the latter should exist on instance.
> > 
> > I'd recommend to have docstring to describe how this method (and other
on_xxx
> > methods) is used.
> 
> Done.
> 
>
https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/scripts...
> Source/bindings/scripts/v8_utilities.py:414: # [Unforgeable],
> [OverrideBuiltins], [Global], [PrimaryGlobal] and [ExposeJSAccessors] /
> [DoNotExposeJSAccessors]
> On 2015/06/17 16:31:21, haraken wrote:
> > 
> > This comment is a bit confusing.
> > 
> > - You have the same comment on on_instance, on_prototype and on_interface.
> > 
> > - You say "[ExposeJSAccessors] / [DoNotExposeJSAccessors]", but the former
> > should exist on prototype whereas the latter should exist on instance.
> 
> Done.
> 
>
https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/scripts...
> Source/bindings/scripts/v8_utilities.py:449: if ('Custom' in
> member.extended_attributes):
> On 2015/06/18 02:16:27, bashi1 wrote:
> > nit: drop parenthesis.
> 
> Done.
> 
>
https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/scripts...
> Source/bindings/scripts/v8_utilities.py:455: return False
> On 2015/06/17 16:31:21, haraken wrote:
> > 
> > It's a bit confusing that the priority of [ExposeJSAccessors] is different
> > between [ExposeJSAccessors] on members and [ExposeJSAccessors] on
interfaces.
> 
> Well, however, we should not place static members on the prototype object even
> if [ExposeJSAccessors] is specified for an interface.  [ExposeJSAccessors] on
> interfaces means that "regular members" will be placed on the prototype
object. 
> We need to check if the member is a regular member or not before applying the
> interface-wide default setting.
> 
> 
> > BTW, is there any [ExposeJSAccessors] used on interfaces?
> 
> No, there is no effective ones.
> 
> > (If you have a plan to remove [ExposeJSAccessors] in the near future, I'm OK
> > with the current code though.)
> 
> Optimistically, we can get rid of [ExposeJSAccessors] in a month or two, I
> think.
> 
>
https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/scripts...
> Source/bindings/scripts/v8_utilities.py:462: return False
> On 2015/06/17 16:31:21, haraken wrote:
> > 
> > Just help me understand: When should on_interface return True?
> 
> constants and static members must be defined on the interface object.
> 
> I'm planning to handle them here.
> 
>
https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/templat...
> File Source/bindings/templates/attributes.cpp (right):
> 
>
https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/templat...
> Source/bindings/templates/attributes.cpp:538: {% if attribute.on_interface %}
> On 2015/06/17 16:31:21, haraken wrote:
> > 
> > if => elif ?
> > 
> > Maybe it would be better to use attribute.property_location, which takes
> either
> > of "instance", "prototype" or "interface", instead of introducing
> > attribute.on_instance, attribute.on_prototype and attribute.on_interface.
> 
> I intend to make property_location bitflags.  One member can be placed at many
> places.
> 
>
https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/templat...
> File Source/bindings/templates/interface.cpp (right):
> 
>
https://codereview.chromium.org/1181113006/diff/40001/Source/bindings/templat...
> Source/bindings/templates/interface.cpp:812: #error No one is actually using
> per-member [Exposed] extended attribute.  Removed the implementation.
> On 2015/06/18 02:16:27, bashi1 wrote:
> > On 2015/06/17 16:31:21, haraken wrote:
> > > 
> > > Removed => Remove
> > 
> > Maybe this error says that the implementation has gone? (We no longer
support
> > [Exposed] on members?)
> 
> Yes, I meant so.  I updated the error message so it's clearer.

Thanks for the clarification! Still LGTM.

[bashi, 2015-06-18 09:10:32]

still LGTM

[Yuki, 2015-06-18 09:19:50]

The CQ bit was checked by yukishiino@chromium.org

[commit-bot: I haz the power, 2015-06-18 09:20:29]

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

[commit-bot: I haz the power, 2015-06-18 10:14:26]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197336