Add static attributes to IDL compiler

Add static attributes to IDL compiler

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

[Nils Barth (inactive), 2013-09-26 22:30:34]

Next up: static attributes! (*)
(* Blink only)

https://codereview.chromium.org/24867002/diff/9001/Source/bindings/scripts/un...
File Source/bindings/scripts/unstable/v8_attributes.py (right):

https://codereview.chromium.org/24867002/diff/9001/Source/bindings/scripts/un...
Source/bindings/scripts/unstable/v8_attributes.py:75: 'is_normal': not
attribute.is_static,
In future there will be other exceptions (RunTimeEnabled etc.);
we could skip for now, but this reduces history churn a bit.

https://codereview.chromium.org/24867002/diff/9001/Source/bindings/scripts/un...
Source/bindings/scripts/unstable/v8_attributes.py:89: def
getter_expression(interface, attribute):
This function will be more complicated in future, due to CallWith and arguments.

https://codereview.chromium.org/24867002/diff/9001/Source/bindings/scripts/un...
Source/bindings/scripts/unstable/v8_attributes.py:90: getter_base_name =
uncapitalize(attribute.name)
This is more complicated in general: we'll need a getter_base_name function
(e.g., for content attributes).

https://codereview.chromium.org/24867002/diff/9001/Source/bindings/scripts/un...
Source/bindings/scripts/unstable/v8_attributes.py:95: def scoped_name(interface,
attribute, function_base_name):
We need this for setters as well, and it will be more complex with
ImplementedBy.

[haraken, 2013-09-26 22:48:29]

Thanks for splitting the CL. This size of CL is easy to review.

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

https://codereview.chromium.org/24867002/diff/12001/Source/bindings/scripts/u...
Source/bindings/scripts/unstable/v8_attributes.py:67: cpp_value =
getter_expression(interface, attribute)

Shall we have getter_expression return the getter function name? And rename
getter_expression to cpp_getter_name?

Then you can write:

  contents = {
    'name': cpp_getter_name(interface, attribute)
  }

https://codereview.chromium.org/24867002/diff/12001/Source/bindings/scripts/u...
Source/bindings/scripts/unstable/v8_attributes.py:91: getter_name =
scoped_name(interface, attribute, getter_base_name)

I'd inline scoped_name here. (At least the name "scoped_name" sounds a bit
strange to me.)

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

https://codereview.chromium.org/24867002/diff/12001/Source/bindings/templates...
Source/bindings/templates/interface.cpp:9: {% for attribute in attributes if
attribute.is_normal %}

Could you use something like 'not if attribute.is_normal' (I don't know jinja
syntax), and remove is_normal ? We don't want to have both is_normal and
is_static.

https://codereview.chromium.org/24867002/diff/12001/Source/bindings/templates...
Source/bindings/templates/interface.cpp:31: // Attribute '{{attribute.name}}'
(Extended Attributes: '{{attribute.extended_attributes | join(' ')}}')

Let's remove this comment. This comment has been causing unnecessary
rebaselining of run-bindings-tests.

[Nils Barth (inactive), 2013-09-27 00:26:14]

> Thanks for splitting the CL. This size of CL is easy to review.

No problem!
Admittedly, at 20-30 line CL we'd probably have 200+ CLs for the compiler, but
individual review turnaround should be fast (^.-)v
(Many will probably be closer to 50-100 lines, so more like 100-120 CLs, but
I'll try to do as small as possible.)

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

https://codereview.chromium.org/24867002/diff/12001/Source/bindings/scripts/u...
Source/bindings/scripts/unstable/v8_attributes.py:67: cpp_value =
getter_expression(interface, attribute)
On 2013/09/26 22:48:30, haraken wrote:
> Shall we have getter_expression return the getter function name? And rename
> getter_expression to cpp_getter_name?

I've refactored & renamed getter_expression to getter_name (adding '()' at the
call site).
We'll want getter_expression in future, to handle arguments, but not needed now.

> Then you can write:
> 
>   contents = {
>     'name': cpp_getter_name(interface, attribute)
>   }

Note: 'name' is the attribute name.
We could use 'getter_name' instead of 'cpp_value', but we need cpp_value anyway
for return_v8_value_statement, so it's easier to use it directly.

https://codereview.chromium.org/24867002/diff/12001/Source/bindings/scripts/u...
Source/bindings/scripts/unstable/v8_attributes.py:91: getter_name =
scoped_name(interface, attribute, getter_base_name)
On 2013/09/26 22:48:30, haraken wrote:
> I'd inline scoped_name here.

Done.
We'll want to factor it out when we add setters, but for now it's ok to inline.

> (At least the name "scoped_name" sounds a bit strange to me.)

It changes 'getterName' to 'Scope::getterName' if necessary, otherwise to
'imp->getterName'.
Better name?

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

https://codereview.chromium.org/24867002/diff/12001/Source/bindings/templates...
Source/bindings/templates/interface.cpp:9: {% for attribute in attributes if
attribute.is_normal %}
On 2013/09/26 22:48:30, haraken wrote:
> Could you use something like 'not if attribute.is_normal' (I don't know jinja
> syntax), and remove is_normal ? We don't want to have both is_normal and
> is_static.

Removing for now.
In future we will need is_normal, since we need to exclude static,
EnabledAtRuntime, EnabledPerContext, and interface Window { [Unforgeable] }
attributes, but we can do that when we get there.

https://codereview.chromium.org/24867002/diff/12001/Source/bindings/templates...
Source/bindings/templates/interface.cpp:31: // Attribute '{{attribute.name}}'
(Extended Attributes: '{{attribute.extended_attributes | join(' ')}}')
On 2013/09/26 22:48:30, haraken wrote:
> Let's remove this comment. This comment has been causing unnecessary
> rebaselining of run-bindings-tests.

Done, removed in Perl too!

[haraken, 2013-09-27 00:33:30]

LGTM

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

https://codereview.chromium.org/24867002/diff/19001/Source/bindings/scripts/u...
Source/bindings/scripts/unstable/v8_attributes.py:72: 'cpp_method_name':
cpp_method_name(attribute),

This is unused. If we have getter_name, setter_name and method_name, probably we
no longer need cpp_method_name?

https://codereview.chromium.org/24867002/diff/19001/Source/bindings/scripts/u...
Source/bindings/scripts/unstable/v8_attributes.py:78: 'v8_type':
v8_types.v8_type(idl_type),

Reorder these keys. For example: conditional_string => name => is_static =>
is_keep_alive_for_gc => v8_type => cpp_type => cpp_value

https://codereview.chromium.org/24867002/diff/19001/Source/bindings/scripts/u...
Source/bindings/scripts/unstable/v8_attributes.py:89: def getter_name(interface,
attribute):

getter_name => cpp_getter_name ?

https://codereview.chromium.org/24867002/diff/19001/Source/bindings/scripts/u...
Source/bindings/scripts/unstable/v8_attributes.py:90: getter_base_name =
uncapitalize(attribute.name)

getter_base_name => getter_method_name ?

[Nils Barth (inactive), 2013-09-27 01:09:32]

Thanks, committing!
One question about ordering: shall we use alphabetical order for dictionaries?

I'll try to break them up into understandable pieces so we don't have 40+ value
dictionaries (as in the WIP), but even for a 5-element dictionary order can be
confusing.

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

https://codereview.chromium.org/24867002/diff/19001/Source/bindings/scripts/u...
Source/bindings/scripts/unstable/v8_attributes.py:72: 'cpp_method_name':
cpp_method_name(attribute),
On 2013/09/27 00:33:31, haraken wrote:
> This is unused. If we have getter_name, setter_name and method_name, probably
we
> no longer need cpp_method_name?

Good catch, removed!
(Also some cleanup to v8_utilities followed:
we don't need to import the function here yet, since not yet doing
ImplementedAs.)

https://codereview.chromium.org/24867002/diff/19001/Source/bindings/scripts/u...
Source/bindings/scripts/unstable/v8_attributes.py:78: 'v8_type':
v8_types.v8_type(idl_type),
On 2013/09/27 00:33:31, haraken wrote:
> Reorder these keys. For example: conditional_string => name => is_static =>
> is_keep_alive_for_gc => v8_type => cpp_type => cpp_value

Fixed by alphabetizing - is this ok?
(General style for long lists is to alphabetize;
is there some other order or grouping I should use here?)

https://codereview.chromium.org/24867002/diff/19001/Source/bindings/scripts/u...
Source/bindings/scripts/unstable/v8_attributes.py:89: def getter_name(interface,
attribute):
On 2013/09/27 00:33:31, haraken wrote:
> getter_name => cpp_getter_name ?

That's a bit redundant: getter/setter is clearly C++ implementation (not JS
declaration), and everything (except auxiliary functions) is for inclusion in
the C++ output.

If it's *really* useful for clarity (e.g., cpp_type, cpp_value), we should use
cpp_, but otherwise it just makes everything bulky (just like we don't say
attribute_ everywhere).
I'm sure we don't want 'cpp_attribute_getter_function_name', though 'getter' is
a bit too short.

https://codereview.chromium.org/24867002/diff/19001/Source/bindings/scripts/u...
Source/bindings/scripts/unstable/v8_attributes.py:90: getter_base_name =
uncapitalize(attribute.name)
On 2013/09/27 00:33:31, haraken wrote:
> getter_base_name => getter_method_name ?

Good point, done!

[commit-bot: I haz the power, 2013-09-27 01:09:49]

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

[haraken, 2013-09-27 01:11:32]

On 2013/09/27 01:09:32, Nils Barth wrote:
> Thanks, committing!
> One question about ordering: shall we use alphabetical order for dictionaries?
> 
> I'll try to break them up into understandable pieces so we don't have 40+
value
> dictionaries (as in the WIP), but even for a 5-element dictionary order can be
> confusing.

Alphabetical order sounds good.

[Nils Barth (inactive), 2013-09-27 01:20:59]

On 2013/09/27 01:11:32, haraken wrote:
> Alphabetical order sounds good.

Got it, will do.

[commit-bot: I haz the power, 2013-09-27 03:00:59]

Change committed as 158414