bindings: Reduces the binary size by reducing # of callback functions.

bindings: Reduces the binary size by reducing # of callback functions.

Reduces the binary size of libblink by reducing the number of callback
functions for constructor attributes.

This CL introduces a map from WrapperTypeInfo to its interface name in string.

The size (in bytes) of out/Release/lib/libblink_web.so on Linux 64-bit build:
r191842
    43,752,904
r191842 + r191179 reverted
    43,521,840
r191842 + this CL
    43,551,952

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

[Yuki, 2015-03-16 13:03:04]

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

[Yuki, 2015-03-16 13:03:05]

Could you review this CL?

[vivekg, 2015-03-16 13:06:23]

Sounds great. If possible, can you please share the size saving details?

[Yuki, 2015-03-16 13:17:44]

On 2015/03/16 13:06:23, vivekg_ wrote:
> Sounds great. If possible, can you please share the size saving details?

Oops, I forgot to mention the size.  Updated the CL description.

[vivekg, 2015-03-16 13:39:18]

Is is possible to add the test coverage for
ConstructorAttributeSetterCallbackAsAccessor and
ConstructorAttributeSetterCallbackAsProperty generated code in the bindings
test?

[Yuki, 2015-03-16 14:01:50]

On 2015/03/16 13:39:18, vivekg_ wrote:
> Is is possible to add the test coverage for
> ConstructorAttributeSetterCallbackAsAccessor and
> ConstructorAttributeSetterCallbackAsProperty generated code in the bindings
> test?

Done.  Since we stopped supporting [ExposeJSAccessor], we can't generate
"AsAccessor" version for now.  We'll switch to it in near future, and then, the
test will be automatically updated.

[vivekg, 2015-03-16 14:06:13]

On 2015/03/16 at 14:01:50, yukishiino wrote:
> On 2015/03/16 13:39:18, vivekg_ wrote:
> > Is is possible to add the test coverage for
> > ConstructorAttributeSetterCallbackAsAccessor and
> > ConstructorAttributeSetterCallbackAsProperty generated code in the bindings
> > test?
> 
> Done.  Since we stopped supporting [ExposeJSAccessor], we can't generate
"AsAccessor" version for now.  We'll switch to it in near future, and then, the
test will be automatically updated.

Thank you. lgtm. :)

[haraken, 2015-03-17 00:06:08]

Mostly looks good.

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

https://codereview.chromium.org/1008353002/diff/20001/Source/bindings/templat...
Source/bindings/templates/attributes.cpp:432: attribute.is_expose_js_accessors)
else (

Can we clean up these branches somehow? It looks too complicated.

https://codereview.chromium.org/1008353002/diff/20001/Source/bindings/templat...
File Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/1008353002/diff/20001/Source/bindings/templat...
Source/bindings/templates/interface_base.cpp:76: {% if loop.first %}

Instead of using loop.first, can we introduce
has_constructor_attribute_setter_for_properties? (We normally introduce has_xxx
variables like this.)

https://codereview.chromium.org/1008353002/diff/20001/Source/bindings/templat...
Source/bindings/templates/interface_base.cpp:77: static void
{{cpp_class}}ConstructorAttributeSetterCallbackAsProperty(v8::Local<v8::String>,
v8::Local<v8::Value> v8Value, const v8::PropertyCallbackInfo<void>& info)

I'm just curious, but how many constructor attributes still need the
PropertyCallbackInfo version?

https://codereview.chromium.org/1008353002/diff/20001/Source/bindings/templat...
Source/bindings/templates/interface_base.cpp:80: do {

What is the do...while for?

https://codereview.chromium.org/1008353002/diff/20001/Source/bindings/templat...
Source/bindings/templates/interface_base.cpp:81: v8::Local<v8::Value> data =
info.Data();

Not related to your CL, we want to deprecate info.Data(). It would be great if
there is a way to remove info.Data() from the constructor attribute setter.

https://codereview.chromium.org/1008353002/diff/20001/Source/bindings/templat...
Source/bindings/templates/interface_base.cpp:89:
{{cpp_class}}ForceSetAttributeOnThis(v8String(info.GetIsolate(),
wrapperTypeInfo->interfaceName), v8Value, info);

Not related to your CL, I want to understand why this is ForceSet (which is
discouraged) instead of Set. Can we change the ForceSet to Set and see what
tests fail?

https://codereview.chromium.org/1008353002/diff/20001/Source/bindings/templat...
Source/bindings/templates/interface_base.cpp:100: {% if loop.first %}

Ditto. I'd prefer has_constructor_attribute_setter_for_accessors.

https://codereview.chromium.org/1008353002/diff/20001/Source/bindings/templat...
Source/bindings/templates/interface_base.cpp:104: do {

What is the do...while for?

https://codereview.chromium.org/1008353002/diff/20001/Source/bindings/templat...
Source/bindings/templates/interface_base.cpp:140: (attribute.constructor_type
and attribute.name == attribute.constructor_type)) %}

Maybe I'm wrong, but can we avoid introducing the 'attribute.name ==
attribute.constructor_type' check by storing a "proper" interface name (e.g., a
"WebKit"-prefixed name) on wrapperTypeInfo->interfaceName?

[Yuki, 2015-03-17 08:35:20]

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

https://codereview.chromium.org/1008353002/diff/20001/Source/bindings/templat...
Source/bindings/templates/attributes.cpp:432: attribute.is_expose_js_accessors)
else (
On 2015/03/17 00:06:07, haraken wrote:
> 
> Can we clean up these branches somehow? It looks too complicated.

Tried my best.

https://codereview.chromium.org/1008353002/diff/20001/Source/bindings/templat...
File Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/1008353002/diff/20001/Source/bindings/templat...
Source/bindings/templates/interface_base.cpp:76: {% if loop.first %}
On 2015/03/17 00:06:07, haraken wrote:
> 
> Instead of using loop.first, can we introduce
> has_constructor_attribute_setter_for_properties? (We normally introduce
has_xxx
> variables like this.)

Done.

https://codereview.chromium.org/1008353002/diff/20001/Source/bindings/templat...
Source/bindings/templates/interface_base.cpp:77: static void
{{cpp_class}}ConstructorAttributeSetterCallbackAsProperty(v8::Local<v8::String>,
v8::Local<v8::Value> v8Value, const v8::PropertyCallbackInfo<void>& info)
On 2015/03/17 00:06:07, haraken wrote:
> 
> I'm just curious, but how many constructor attributes still need the
> PropertyCallbackInfo version?

We've not yet moved any of constructor attribute to prototype chains.  I've
removed accessor version in this CL to make it look simple.

https://codereview.chromium.org/1008353002/diff/20001/Source/bindings/templat...
Source/bindings/templates/interface_base.cpp:80: do {
On 2015/03/17 00:06:08, haraken wrote:
> 
> What is the do...while for?

Added a comment.  It's for use of 'break'.
We need to call TRACE_EVENT_SET_SAMPLING_STATE("v8", "V8Execution") at the end.

https://codereview.chromium.org/1008353002/diff/20001/Source/bindings/templat...
Source/bindings/templates/interface_base.cpp:81: v8::Local<v8::Value> data =
info.Data();
On 2015/03/17 00:06:08, haraken wrote:
> 
> Not related to your CL, we want to deprecate info.Data(). It would be great if
> there is a way to remove info.Data() from the constructor attribute setter.

We could use "hidden value" instead of info.Data(), but it doesn't help much
since hidden values are also discouraged...  I'll keep it in mind.

https://codereview.chromium.org/1008353002/diff/20001/Source/bindings/templat...
Source/bindings/templates/interface_base.cpp:89:
{{cpp_class}}ForceSetAttributeOnThis(v8String(info.GetIsolate(),
wrapperTypeInfo->interfaceName), v8Value, info);
On 2015/03/17 00:06:07, haraken wrote:
> 
> Not related to your CL, I want to understand why this is ForceSet (which is
> discouraged) instead of Set. Can we change the ForceSet to Set and see what
> tests fail?

My understanding is:
  Set = regular assignment, which means named/indexed properties are supported.
  ForceSet = bypass interceptors such as named/indexed properties.

We shouldn't use regular Set inside a setter callback, which may cause an
infinite loop.  Since we know 1) the attribute is a replaceable attribute or
constructor attribute, and 2) V8 (JS) found this is the proper setter callback,
thus we should simply set the value without looking for a setter again.

https://codereview.chromium.org/1008353002/diff/20001/Source/bindings/templat...
Source/bindings/templates/interface_base.cpp:100: {% if loop.first %}
On 2015/03/17 00:06:08, haraken wrote:
> 
> Ditto. I'd prefer has_constructor_attribute_setter_for_accessors.
> 

Done.

https://codereview.chromium.org/1008353002/diff/20001/Source/bindings/templat...
Source/bindings/templates/interface_base.cpp:104: do {
On 2015/03/17 00:06:07, haraken wrote:
> 
> What is the do...while for?

Done.

https://codereview.chromium.org/1008353002/diff/20001/Source/bindings/templat...
Source/bindings/templates/interface_base.cpp:140: (attribute.constructor_type
and attribute.name == attribute.constructor_type)) %}
On 2015/03/17 00:06:08, haraken wrote:
> 
> Maybe I'm wrong, but can we avoid introducing the 'attribute.name ==
> attribute.constructor_type' check by storing a "proper" interface name (e.g.,
a
> "WebKit"-prefixed name) on wrapperTypeInfo->interfaceName?

No, we cannot.  At least there is case that 1) the proper interface name is
unprefixed name, and 2) we have both of prefixed and unprefixed constructor
attributes.

For example, we have
  "WebKitTransitionEvent" attribute to create a TransitionEvent, and
  "TransitionEvent" attribute, too, to create a TransitionEvent.

It's better for compatibility.

[haraken, 2015-03-17 08:48:08]

LGTM

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

https://codereview.chromium.org/1008353002/diff/40001/Source/bindings/scripts...
Source/bindings/scripts/v8_attributes.py:522:
context['needs_constructor_setter_callback'] = context['name'] !=
context['constructor_type']

Can we add a comment and mention why we need to check the equality?

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

https://codereview.chromium.org/1008353002/diff/40001/Source/bindings/templat...
Source/bindings/templates/interface_base.cpp:109: {% if not
(attribute.has_custom_setter or

not (A or (B and not C)) => not A and (not B or C) ?

https://codereview.chromium.org/1008353002/diff/40001/Source/bindings/templat...
Source/bindings/templates/interface_base.cpp:113: {% if not
(attribute.constructor_type and not attribute.needs_constructor_setter_callback)
%}

not (B and not C) => not B or C ?

[Yuki, 2015-03-17 09:10:05]

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

https://codereview.chromium.org/1008353002/diff/40001/Source/bindings/scripts...
Source/bindings/scripts/v8_attributes.py:522:
context['needs_constructor_setter_callback'] = context['name'] !=
context['constructor_type']
On 2015/03/17 08:48:07, haraken wrote:
> 
> Can we add a comment and mention why we need to check the equality?

Done.

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

https://codereview.chromium.org/1008353002/diff/40001/Source/bindings/templat...
Source/bindings/templates/interface_base.cpp:109: {% if not
(attribute.has_custom_setter or
On 2015/03/17 08:48:07, haraken wrote:
> 
> not (A or (B and not C)) => not A and (not B or C) ?

Done.

https://codereview.chromium.org/1008353002/diff/40001/Source/bindings/templat...
Source/bindings/templates/interface_base.cpp:113: {% if not
(attribute.constructor_type and not attribute.needs_constructor_setter_callback)
%}
On 2015/03/17 08:48:08, haraken wrote:
> 
> not (B and not C) => not B or C ?

Done.

[Yuki, 2015-03-17 09:10:15]

The CQ bit was checked by yukishiino@chromium.org

[Yuki, 2015-03-17 09:10:15]

The patchset sent to the CQ was uploaded after l-g-t-m from vivekg@chromium.org,
haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1008353002/#ps60001
(title: "Addressed review comments.")

[commit-bot: I haz the power, 2015-03-17 09:10:23]

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

[commit-bot: I haz the power, 2015-03-17 09:16:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-17 09:16:23]

Try jobs failed on following builders:
  android_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_db...)
  android_blink_compile_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_re...)

[Yuki, 2015-03-17 09:34:44]

The CQ bit was checked by yukishiino@chromium.org

[Yuki, 2015-03-17 09:34:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, vivekg@chromium.org
Link to the patchset: https://codereview.chromium.org/1008353002/#ps80001
(title: "Synced.")

[commit-bot: I haz the power, 2015-03-17 09:35:07]

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

[commit-bot: I haz the power, 2015-03-17 12:19:31]

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