IDL compiler: constructor attributes

IDL compiler: constructor attributes

(Attributes named *Constructor.)
Pretty straightforward; some reordering for clarity.

BUG=239771
NOTRY=true

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

[Nils Barth (inactive), 2013-10-23 11:31:34]

https://codereview.chromium.org/36793003/diff/1/Source/bindings/scripts/code_...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/36793003/diff/1/Source/bindings/scripts/code_...
Source/bindings/scripts/code_generator_v8.pm:1249: 
Consistency: blank line after function.

https://codereview.chromium.org/36793003/diff/1/Source/bindings/scripts/unsta...
File Source/bindings/scripts/unstable/v8_attributes.py (right):

https://codereview.chromium.org/36793003/diff/1/Source/bindings/scripts/unsta...
Source/bindings/scripts/unstable/v8_attributes.py:321: # Attribute configuration
(Reordered in order of the configuration line.)

[haraken, 2013-10-23 12:01:26]

[Constructor] on attributes is used only by Window.HTMLImageElement, right?

hmm, actually I might not want to add complexity just to support
Window.HTMLImageElement. Once we support overloading for named constructors, we
can auto-generate code for HTMLImageElement constructors and thus get rid of
[Constructor] on attributes.  (In any event, it doesn't make sense to have
[Constructor] IDL attribute for DOM attributes.)

Would it be possible to add a hard-coded hack for Window.HTMLImageElement to the
code generator and avoid implementing [Constructor] on attributes? I hope the
hack will also enable us to get rid of the complexity in the IDL parser that
we've added to support [Constructor] on attributes.

[Nils Barth (inactive), 2013-10-24 02:23:32]

On 2013/10/23 12:01:26, haraken wrote:
> [Constructor] on attributes is used only by Window.HTMLImageElement, right?

This isn't [CustomConstructor]; it's attributes with a name of the form
*Constructor,
e.g.,
Window.idl:
attribute DocumentConstructor XMLDocument;
These are used 50 places in the usual IDLs, and 456 places in the constructors
on
global objects (generated IDLs).


You're right that [CustomConstructor] is only used one place,
on attributes ending in ConstructorConstructor (!):
[CustomConstructor] attribute HTMLImageElementConstructorConstructor Image;
...and that's the next CL (and last attribute one!).
When doing that one, let's see how much complexity is needed;
I may be able to just hard-code it.

[haraken, 2013-10-24 03:33:02]

Sorry about the confusion! This CL is about XXXConstructor in Window.idl...

https://codereview.chromium.org/36793003/diff/80001/Source/bindings/scripts/u...
File Source/bindings/scripts/unstable/v8_attributes.py (right):

https://codereview.chromium.org/36793003/diff/80001/Source/bindings/scripts/u...
Source/bindings/scripts/unstable/v8_attributes.py:86: 'data':
installation_data(attribute),

data => wrapper_type_info

https://codereview.chromium.org/36793003/diff/80001/Source/bindings/scripts/u...
Source/bindings/scripts/unstable/v8_attributes.py:110:
includes.update(v8_types.includes_for_type(idl_type))

We might want to call includes.update() always in this method (e.g., at the head
of this method)?

https://codereview.chromium.org/36793003/diff/80001/Source/bindings/scripts/u...
Source/bindings/scripts/unstable/v8_attributes.py:360: def
installation_data(attribute):

installation_data => wrapper_type_info

https://codereview.chromium.org/36793003/diff/80001/Source/bindings/scripts/u...
Source/bindings/scripts/unstable/v8_attributes.py:363: return '&V8%s::info' %
v8_types.constructor_type(attribute.data_type)

Shall we rename |info| to |wrapperTypeInfo|? It's confusing with |info| for
PropertyCallbackInfo.

https://codereview.chromium.org/36793003/diff/80001/Source/bindings/templates...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/36793003/diff/80001/Source/bindings/templates...
Source/bindings/templates/interface.cpp:18: return;

Not related to your CL, I'd guess this should be ASSERT(!perContextData). I'd be
happy if you could take a look later.

https://codereview.chromium.org/36793003/diff/80001/Source/bindings/templates...
Source/bindings/templates/interface.cpp:28: {% if has_constructor_getters or
has_replaceable_attributes %}

This looks strange. Why do we need to generate ReplaceableAttributeSetter when
we have has_constructor_getters ? I think constructor getters are not relevant
to replaceable setters.

[Nils Barth (inactive), 2013-10-24 05:28:38]

On 2013/10/24 03:33:02, haraken wrote:
> Sorry about the confusion!

n/p

> This CL is about XXXConstructor in Window.idl...
...and:
WorkerGlobalScope.idl
WorkerGlobalScopeFileSystem.idl
WorkerGlobalScopeIndexedDatabase.idl
WindowMediaSource.idl
...and constructors on global objects::
DedicatedWorkerGlobalScopeConstructors.idl
SharedWorkerGlobalScopeConstructors.idl
WindowConstructors.idl
WorkerGlobalScopeConstructors.idl

[Nils Barth (inactive), 2013-10-24 05:33:03]

Revised!

I have 3 or 4 followups (if that's ok):
1. Always have includes_for_type for attributes.
2. (Maybe??) split constructor attributes setter from reflected attributes
setter (??)
3. Rename ::|info| to ::|wrapperTypeInfo|
4. Check if:
if (!perContextData)
     return;
...should instead be:
ASSERT(perContextData);

https://codereview.chromium.org/36793003/diff/80001/Source/bindings/scripts/u...
File Source/bindings/scripts/unstable/v8_attributes.py (right):

https://codereview.chromium.org/36793003/diff/80001/Source/bindings/scripts/u...
Source/bindings/scripts/unstable/v8_attributes.py:86: 'data':
installation_data(attribute),
On 2013/10/24 03:33:03, haraken wrote:
> 
> data => wrapper_type_info

Done.

https://codereview.chromium.org/36793003/diff/80001/Source/bindings/scripts/u...
Source/bindings/scripts/unstable/v8_attributes.py:110:
includes.update(v8_types.includes_for_type(idl_type))
On 2013/10/24 03:33:03, haraken wrote:
> We might want to call includes.update() always in this method (e.g., at the
head
> of this method)?

I was thinking the same thing!
That's a Perl change too (b/c otherwise omitted for custom getters),
so I'd prefer to do this in a followup CL.

https://codereview.chromium.org/36793003/diff/80001/Source/bindings/scripts/u...
Source/bindings/scripts/unstable/v8_attributes.py:360: def
installation_data(attribute):
On 2013/10/24 03:33:03, haraken wrote:
> 
> installation_data => wrapper_type_info

Done.

https://codereview.chromium.org/36793003/diff/80001/Source/bindings/scripts/u...
Source/bindings/scripts/unstable/v8_attributes.py:363: return '&V8%s::info' %
v8_types.constructor_type(attribute.data_type)
On 2013/10/24 03:33:03, haraken wrote:
> Shall we rename |info| to |wrapperTypeInfo|? It's confusing with |info| for
> PropertyCallbackInfo.

That clarifies things!
Simple rename (in Perl too, and lotta rebasing), so followup?

https://codereview.chromium.org/36793003/diff/80001/Source/bindings/templates...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/36793003/diff/80001/Source/bindings/templates...
Source/bindings/templates/interface.cpp:18: return;
On 2013/10/24 03:33:03, haraken wrote:
> Not related to your CL, I'd guess this should be ASSERT(!perContextData). I'd
be
> happy if you could take a look later.

Will do!

https://codereview.chromium.org/36793003/diff/80001/Source/bindings/templates...
Source/bindings/templates/interface.cpp:28: {% if has_constructor_getters or
has_replaceable_attributes %}
On 2013/10/24 03:33:03, haraken wrote:
> 
> This looks strange. Why do we need to generate ReplaceableAttributeSetter when
> we have has_constructor_getters ? I think constructor getters are not relevant
> to replaceable setters.

This is strange, but it is used.
Fixing this would require changing Perl and generated code;
shall I do in a followup?

I've renamed this to |has_constructor_attributes| and added a comment.

It is because constructor attributes use the
ReplaceableAttributeSetter(Callback),
I believe b/c it's a single interface-level setter.
You can see this in this CL, in |setter_callback_name|,
which checks /Constructor$/ and if so, uses ReplaceableAttributeSetter.

Maybe we should split this into two separate functions?
(...or at least rename it, like *InterfaceAttributeSetter ?)

There are two interfaces (omitting inheritance/implements/etc.)
that have both [Replaceable] attributes and constructor attributes:
Window and WorkerGlobalScope.

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

l. 3095 & 3096:
$getter = "${implClassName}V8Internal::${implClassName}ConstructorGetter";
$setter =
"${implClassName}V8Internal::${implClassName}ReplaceableAttributeSetterCallback";

[haraken, 2013-10-24 09:13:53]

LGTM.

https://codereview.chromium.org/36793003/diff/150001/Source/bindings/scripts/...
File Source/bindings/scripts/unstable/v8_attributes.py (right):

https://codereview.chromium.org/36793003/diff/150001/Source/bindings/scripts/...
Source/bindings/scripts/unstable/v8_attributes.py:324: if
attribute.data_type.endswith('Constructor'):

Not related to this CL, data_type => idl_type ?

https://codereview.chromium.org/36793003/diff/150001/Source/bindings/scripts/...
Source/bindings/scripts/unstable/v8_attributes.py:395: def
is_constructor(attribute):

is_constructor => is_constructor_attribute

https://codereview.chromium.org/36793003/diff/150001/Source/bindings/template...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/36793003/diff/150001/Source/bindings/template...
Source/bindings/templates/interface.cpp:29: {# Constructor attributes use
ReplaceableAttributeSetter as an interface-level setter #}

Thanks, I understood the situation. Both replaceable setters and constructor
attributes are using ReplaceableAttributeSetter because they want to set values
with ForceSet. The point is just that they want to use ForceSet.

Probably a right fix would be to rename ReplaceableAttributeSetter() to
ForceSetAttributeOnThis(). Or you can simply inline info.This()->ForceSet.

(Then you can remove the above comment.)

[Nils Barth (inactive), 2013-10-24 09:41:51]

Thanks for comments; 2 followups:
1. ReplaceableAttributeSetter => ForceSetAttributeOnThis
2. data_type => idl_type

https://codereview.chromium.org/36793003/diff/150001/Source/bindings/scripts/...
File Source/bindings/scripts/unstable/v8_attributes.py (right):

https://codereview.chromium.org/36793003/diff/150001/Source/bindings/scripts/...
Source/bindings/scripts/unstable/v8_attributes.py:324: if
attribute.data_type.endswith('Constructor'):
On 2013/10/24 09:13:54, haraken wrote:
> 
> Not related to this CL, data_type => idl_type ?

I was planning on doing that at some point; will do now.
(Most natural would be |type|, but that's a reserved word,
hence |data_type|.)

https://codereview.chromium.org/36793003/diff/150001/Source/bindings/scripts/...
Source/bindings/scripts/unstable/v8_attributes.py:395: def
is_constructor(attribute):
On 2013/10/24 09:13:54, haraken wrote:
> 
> is_constructor => is_constructor_attribute

Done.

https://codereview.chromium.org/36793003/diff/150001/Source/bindings/template...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/36793003/diff/150001/Source/bindings/template...
Source/bindings/templates/interface.cpp:29: {# Constructor attributes use
ReplaceableAttributeSetter as an interface-level setter #}
On 2013/10/24 09:13:54, haraken wrote:
> 
> Thanks, I understood the situation. Both replaceable setters and constructor
> attributes are using ReplaceableAttributeSetter because they want to set
values
> with ForceSet. The point is just that they want to use ForceSet.
> 
> Probably a right fix would be to rename ReplaceableAttributeSetter() to
> ForceSetAttributeOnThis().

Will do in quick followup CL (b/c generated code and Perl changes).

> Or you can simply inline info.This()->ForceSet.

Unfortunately it's more complicated for
[CheckSecurity] interface (>.<)
We may still be able to just inline it, but we'll want to
look at it then (when doing interfaces).

[commit-bot: I haz the power, 2013-10-24 09:42:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nbarth@chromium.org/36793003/210001

[commit-bot: I haz the power, 2013-10-24 09:42:25]

Failed to apply patch for Source/bindings/scripts/unstable/v8_attributes.py:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/bindings/scripts/unstable/v8_attributes.py
  Hunk #1 succeeded at 53 (offset -4 lines).
  Hunk #2 succeeded at 80 (offset -5 lines).
  Hunk #3 succeeded at 96 with fuzz 2 (offset -5 lines).
  Hunk #4 FAILED at 114.
  Hunk #5 FAILED at 228.
  Hunk #6 succeeded at 304 (offset -7 lines).
  Hunk #7 succeeded at 325 (offset -7 lines).
  Hunk #8 succeeded at 366 (offset -7 lines).
  2 out of 8 hunks FAILED -- saving rejects to file
Source/bindings/scripts/unstable/v8_attributes.py.rej

Patch:       Source/bindings/scripts/unstable/v8_attributes.py
Index: Source/bindings/scripts/unstable/v8_attributes.py
diff --git a/Source/bindings/scripts/unstable/v8_attributes.py
b/Source/bindings/scripts/unstable/v8_attributes.py
index
38ef754b1e43bbae70cc757069244df96adb5465..f0d2b740b6f2cd2881871903f8735b7b5e136ad7
100644
--- a/Source/bindings/scripts/unstable/v8_attributes.py
+++ b/Source/bindings/scripts/unstable/v8_attributes.py
@@ -57,6 +57,7 @@ def generate_attributes_common(interface, attributes):
     v8_class_name = v8_utilities.v8_class_name(interface)
     return {
         'has_per_context_enabled_attributes':
any(attribute['per_context_enabled_function_name'] for attribute in attributes),
+        'has_constructor_attributes': any(attribute['is_constructor'] for
attribute in attributes),
         'has_replaceable_attributes': any(attribute['is_replaceable'] for
attribute in attributes),
         'has_runtime_enabled_attributes':
any(attribute['runtime_enabled_function_name'] for attribute in attributes),
         'installed_attributes': '%sAttributes' % v8_class_name if attributes
else '0',
@@ -84,6 +85,7 @@ def generate_attribute_and_includes(interface, attribute):
         'has_custom_getter': has_custom_getter,
         'has_custom_setter': has_custom_setter,
         'idl_type': idl_type,
+        'is_constructor': is_constructor_attribute(attribute),
         'is_getter_raises_exception': has_extended_attribute(attribute,
('GetterRaisesException', 'RaisesException')),
         'is_keep_alive_for_gc': is_keep_alive_for_gc(attribute),
         'is_nullable': attribute.is_nullable,
@@ -99,7 +101,11 @@ def generate_attribute_and_includes(interface, attribute):
         'v8_type': v8_types.v8_type(idl_type),
         'runtime_enabled_function_name':
v8_utilities.runtime_enabled_function_name(attribute),  # [RuntimeEnabled]
         'world_suffixes': ['', 'ForMainWorld'] if 'PerWorldBindings' in
extended_attributes else [''],  # [PerWorldBindings]
+        'wrapper_type_info': wrapper_type_info(attribute),
     }
+    if is_constructor_attribute(attribute):
+        includes.update(v8_types.includes_for_type(idl_type))
+        return contents, includes
     if not has_custom_getter:
         generate_getter(interface, attribute, contents, includes)
     if not attribute.is_read_only and not has_custom_setter:
@@ -108,6 +114,8 @@ def generate_attribute_and_includes(interface, attribute):
     return contents, includes
 
 
+# Getter
+
 def generate_getter(interface, attribute, contents, includes):
     idl_type = attribute.data_type
     includes.update(v8_types.includes_for_type(idl_type))
@@ -222,6 +230,8 @@ def is_keep_alive_for_gc(attribute):
              idl_type.startswith('HTML'))))
 
 
+# Setter
+
 def generate_setter(interface, attribute, contents, includes):
     idl_type = attribute.data_type
     extended_attributes = attribute.extended_attributes
@@ -305,14 +315,20 @@ def scoped_name(interface, attribute, base_name):
     return 'imp->%s' % base_name
 
 
+# Attribute configuration
+
 def getter_callback_name(interface, attribute):
+    if attribute.data_type.endswith('Constructor'):
+        return
'{0}V8Internal::{0}ConstructorGetter'.format(cpp_name(interface))
     return '%sV8Internal::%sAttributeGetterCallback' % (cpp_name(interface),
attribute.name)
 
 
 # [Replaceable]
 def setter_callback_name(interface, attribute):
     cpp_class_name = cpp_name(interface)
-    if 'Replaceable' in attribute.extended_attributes:
+    if ('Replaceable' in attribute.extended_attributes or
+        attribute.data_type.endswith('Constructor')):
+        # FIXME: rename to ForceSetAttributeOnThisCallback, since also used for
Constructors
         return
'{0}V8Internal::{0}ReplaceableAttributeSetterCallback'.format(cpp_class_name)
     # FIXME: support [PutForwards]
     if attribute.is_read_only:
@@ -320,6 +336,26 @@ def setter_callback_name(interface, attribute):
     return '%sV8Internal::%sAttributeSetterCallback' % (cpp_class_name,
attribute.name)
 
 
+# [PerWorldBindings]
+def getter_callback_name_for_main_world(interface, attribute):
+    if 'PerWorldBindings' not in attribute.extended_attributes:
+        return '0'
+    return '%sV8Internal::%sAttributeGetterCallbackForMainWorld' %
(cpp_name(interface), attribute.name)
+
+
+def setter_callback_name_for_main_world(interface, attribute):
+    if ('PerWorldBindings' not in attribute.extended_attributes or
+        attribute.is_read_only):
+        return '0'
+    return '%sV8Internal::%sAttributeSetterCallbackForMainWorld' %
(cpp_name(interface), attribute.name)
+
+
+def wrapper_type_info(attribute):
+    if not is_constructor_attribute(attribute):
+        return '0'
+    return '&V8%s::info' % v8_types.constructor_type(attribute.data_type)
+
+
 # [DoNotCheckSecurity], [DoNotCheckSecurityOnGetter],
[DoNotCheckSecurityOnSetter], [Unforgeable]
 def access_control_list(attribute):
     extended_attributes = attribute.extended_attributes
@@ -341,22 +377,13 @@ def access_control_list(attribute):
 def property_attributes(attribute):
     extended_attributes = attribute.extended_attributes
     property_attributes_list = []
-    if 'NotEnumerable' in extended_attributes:
+    if ('NotEnumerable' in extended_attributes or
+        is_constructor_attribute(attribute)):
         property_attributes_list.append('v8::DontEnum')
     if 'Unforgeable' in extended_attributes:
         property_attributes_list.append('v8::DontDelete')
     return property_attributes_list or ['v8::None']
 
 
-# [PerWorldBindings]
-def getter_callback_name_for_main_world(interface, attribute):
-    if 'PerWorldBindings' not in attribute.extended_attributes:
-        return '0'
-    return '%sV8Internal::%sAttributeGetterCallbackForMainWorld' %
(cpp_name(interface), attribute.name)
-
-
-def setter_callback_name_for_main_world(interface, attribute):
-    if ('PerWorldBindings' not in attribute.extended_attributes or
-        attribute.is_read_only):
-        return '0'
-    return '%sV8Internal::%sAttributeSetterCallbackForMainWorld' %
(cpp_name(interface), attribute.name)
+def is_constructor_attribute(attribute):
+    return attribute.data_type.endswith('Constructor')

[commit-bot: I haz the power, 2013-10-24 09:46:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nbarth@chromium.org/36793003/260001

[commit-bot: I haz the power, 2013-10-24 09:46:28]

Change committed as 160434