Add constants and primitive type readonly attributes to Python IDL compiler

Add constants and primitive type readonly attributes to Python IDL compiler

This adds constants to the Python IDL compiler.

As there are no files with only constants, to have real test cases I've
also supported primitive type readonly attributes (currently only have
wrapped readonly attributes),
also moving attribute templates into a macro file.

This allows us to move the following 3 files to Python:
* MallocStatistics.idl
* MemoryInfo.idl
* PerformanceNavigation.idl

The Python generates exactly the same code as the Perl.

About 80 IDL files use constants, but usual other elements as well.

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

[Nils Barth (inactive), 2013-08-22 06:00:49]

Here's the constants (+ some attributes)-only CL!

[kouhei (in TOK), 2013-08-22 06:14:32]

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

https://codereview.chromium.org/23068032/diff/1/Source/bindings/scripts/code_...
Source/bindings/scripts/code_generator_v8.py:275: pointer_type:
add doc for None?
None: return raw value form (e.g. Foo)

[Nils Barth (inactive), 2013-08-22 06:33:48]

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

https://codereview.chromium.org/23068032/diff/1/Source/bindings/scripts/code_...
Source/bindings/scripts/code_generator_v8.py:275: pointer_type:
On 2013/08/22 06:14:32, kouhei wrote:
> add doc for None?
> None: return raw value form (e.g. Foo)

Good point!
Fixed (in more detail).
I made this optional b/c it's unnecessary if this isn't a pointer type (we might
be able to split off a separate function for that), but in fact it's simpler to
just default to raw unless a type is specified.

I've also converted the strings to format strings, so they look just like
templates and are more readable, and renamed the parameter to idl_type, as
that's what it is.

[haraken, 2013-08-22 07:31:33]

Thanks for the CL! The implementation looks good. Most comments below are just
about naming and styling issues.

- In order to keep the generated code the same between the old flow and the new
flow, you can address some of the belo comments in a follow-up CL.

- I'm neutral (i.e. not opposed but not agreed) to introducing template macros.
I don't think the macros (or template modularization) helps to improve
readability or maintainability.

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

https://codereview.chromium.org/23068032/diff/1/Source/bindings/scripts/code_...
Source/bindings/scripts/code_generator_v8.py:109:
CPP_VALUE_TO_JS_VALUE_RETURN_DICT = {

JS_VALUE => V8_VALUE

We're mixing "js" and "v8".

https://codereview.chromium.org/23068032/diff/1/Source/bindings/scripts/code_...
Source/bindings/scripts/code_generator_v8.py:139: def
cpp_value_to_js_value_return(data_type, cpp_value, callback_info=''):

Why don't you simply call this cpp_value_v8_value ? Does 'return' matter?

https://codereview.chromium.org/23068032/diff/1/Source/bindings/scripts/code_...
Source/bindings/scripts/code_generator_v8.py:168: name =
constant.extended_attributes.get('Reflect', constant.name)

name => reflected_name

https://codereview.chromium.org/23068032/diff/1/Source/bindings/scripts/code_...
Source/bindings/scripts/code_generator_v8.py:184: value = value_raw

I don't fully understand why this is needed. Can we get rid of the complexity by
replacing "0X" with "0x" in the IDL files? Or if it's really needed, you can do
the conversion in the parser. (We don't want to mess up the code generator for
edge cases no one cares about.)

https://codereview.chromium.org/23068032/diff/1/Source/bindings/scripts/code_...
Source/bindings/scripts/code_generator_v8.py:188: 'name_reflect': name,  #
FIXME: use name_reflect as correct 'name'

name_reflect => reflected_name

https://codereview.chromium.org/23068032/diff/1/Source/bindings/scripts/code_...
Source/bindings/scripts/code_generator_v8.py:199: def
getter_expression(interface, attribute):

getter_expression => attribute_getter_name ?

You can concatenate 'imp->' in the caller side.

https://codereview.chromium.org/23068032/diff/1/Source/bindings/scripts/code_...
Source/bindings/scripts/code_generator_v8.py:382: # FIXME: eliminate
should_keep_attribute_alive

Remove the FIXME. We will need should_keep_attribute_alive.

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/att...
File Source/bindings/templates/attributes.cpp (right):

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/att...
Source/bindings/templates/attributes.cpp:1: {% macro attribute_getter(attribute)
%}

attribute_getter => define_attribute_getter

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/att...
Source/bindings/templates/attributes.cpp:17: {% macro
attribute_return_keep_property_alive(attribute) %}

attribute_return_keep_property_alive => keep_attribute_alive

It looks like we can make the naming consistent by starting the name from a
verb.

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/att...
Source/bindings/templates/attributes.cpp:30: {% macro
attribute_getter_callback(attribute) %}

attribute_getter_callback => define_attribute_getter_callback

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/att...
Source/bindings/templates/attributes.cpp:42: {% macro batched_attribute() %}

batched_attribute => install_attributes

"Batched" sounds a bit unclear. "Install" would be better.

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/att...
Source/bindings/templates/attributes.cpp:43: static const
V8DOMConfiguration::BatchedAttribute {{v8_class_name}}Attrs[] = {

{{v8_class_name}}Attrs[] => {{v8_class_name}}Attributes[]

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/att...
Source/bindings/templates/attributes.cpp:45: // Attribute '{{attribute.name}}'

You can remove this comment.

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/att...
Source/bindings/templates/attributes.cpp:46: {"{{attribute.name}}",
{{cpp_class_name}}V8Internal::{{attribute.name}}AttrGetterCallback, 0, 0, 0, 0
/* no data */, static_cast<v8::AccessControl>(v8::DEFAULT),
static_cast<v8::PropertyAttribute>(v8::None), 0 /* on instance */},

You can remove /*no data*/. It isn't helpful.

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/con...
File Source/bindings/templates/constants.cpp (right):

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/con...
Source/bindings/templates/constants.cpp:4: {% macro class_consts() %}

class_consts => define_constants

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/con...
Source/bindings/templates/constants.cpp:5: static const
V8DOMConfiguration::BatchedConstant {{v8_class_name}}Consts[] = {

{{v8_class_name}}Consts[] => {{v8_class_name}}Constants[]

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/con...
Source/bindings/templates/constants.cpp:20: 

Nit: Unnecessary empty line.

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/con...
Source/bindings/templates/constants.cpp:25:
COMPILE_ASSERT({{constant.value_raw}} ==
{{cpp_class_name}}::{{constant.name_reflect}},
{{cpp_class_name}}Enum{{constant.name_reflect}}IsWrongUseDoNotCheckConstants);

{{cpp_class_name}}Enum{{constant.name_reflect}}IsWrongUseDoNotCheckConstants =>
TheValueOf{{cpp_class_name}}::{{constant.name_reflect}}Doesn'tMatchWithImplementation

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/con...
Source/bindings/templates/constants.cpp:35: {% macro const_data(constant) %}

const_data => install_runtime_enabled_constant

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/con...
Source/bindings/templates/constants.cpp:41: static const
V8DOMConfiguration::BatchedConstant constData = {"{{constant.name}}",
static_cast<signed int>({{constant.value}})};

constData => constantConfiguration ?

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/con...
Source/bindings/templates/constants.cpp:46: {%- endif %}

Ditto. What is the '-' ?

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/con...
Source/bindings/templates/constants.cpp:52: {% macro batch_configure_constants()
%}

batch_configure_constants => install_constants

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/con...
Source/bindings/templates/constants.cpp:53:
V8DOMConfiguration::batchConfigureConstants(desc, proto,
{{v8_class_name}}Consts, WTF_ARRAY_LENGTH({{v8_class_name}}Consts), isolate);

Probably we want to rename the method to installConstants.

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/int...
File Source/bindings/templates/interface.cpp (left):

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/int...
Source/bindings/templates/interface.cpp:155: 

Nit: This empty line would be needed.

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/int...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/int...
Source/bindings/templates/interface.cpp:62: {{attribute_getter(attribute) -}}

What is the '-' ?

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/int...
Source/bindings/templates/interface.cpp:124: 

Nit: Unnecessary empty line.

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/int...
Source/bindings/templates/interface.cpp:136: 

Ditto.

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/int...
File Source/bindings/templates/interface.h (right):

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/int...
Source/bindings/templates/interface.h:126: inline v8::Handle<v8::Value>
toV8(PassRefPtr< {{cpp_class_name}} > impl, v8::Handle<v8::Object>
creationContext, v8::Isolate* isolate)

Nit: You don't need a space before {{cpp_class_name}}. The same comment for
other places in this CL.

[Nils Barth (inactive), 2013-08-22 09:08:12]

Thanks for comments!
Revised; overall notes are:
1. There are lots of hacks for Perl compatibility.
Will fix easy ones in immediate followup (changing both Perl and Python), and
hard ones once Perl is removed.
This balances keeping new CG and templates clean with landing code ASAP and not
wasting time on old CG.

2. Changed JS to V8 throughout, for consistency.

3. Template macro names as declarative nouns.
As discussed in comments below, I agree that for functions we should use verbs.
However, for macros in templates it's not doing anything, and instead just being
*substituted* (it's declarative).
Thus it's clearer if the name of the macro is the name of the contents, like
attr_getter_callback for AttrGetterCallback.
This means you can just mentally substitute it.

I had one or two minor questions below, otherwise should be ready to go (and
then can fix some Perl hacks).

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

https://codereview.chromium.org/23068032/diff/1/Source/bindings/scripts/code_...
Source/bindings/scripts/code_generator_v8.py:109:
CPP_VALUE_TO_JS_VALUE_RETURN_DICT = {
On 2013/08/22 07:31:33, haraken wrote:
> 
> JS_VALUE => V8_VALUE
> 
> We're mixing "js" and "v8".

Got it; changed throughout.

https://codereview.chromium.org/23068032/diff/1/Source/bindings/scripts/code_...
Source/bindings/scripts/code_generator_v8.py:139: def
cpp_value_to_js_value_return(data_type, cpp_value, callback_info=''):
On 2013/08/22 07:31:33, haraken wrote:
> Why don't you simply call this cpp_value_v8_value ? Does 'return' matter?

cpp_value_v8_value and cpp_value_v8_value_return are quite different; one
returns a conversion *expression* (which can be used many places), while the
other returns a setReturn... *statement* (which is used before a return;
statement).
I've changed the name to cpp_value_v8_value_set_return and distinguished the
comments.

You can see what the functions look like at the WIP:
https://codereview.chromium.org/17572008/diff/181001/Source/bindings/scripts/...

The expressions are of the form
'v8String({cpp_value}, {isolate})'
...while the set return statements are of the from
'v8SetReturnValueUnsigned({callback_info}, {cpp_value});'

https://codereview.chromium.org/23068032/diff/1/Source/bindings/scripts/code_...
Source/bindings/scripts/code_generator_v8.py:168: name =
constant.extended_attributes.get('Reflect', constant.name)
On 2013/08/22 07:31:33, haraken wrote:
> 
> name => reflected_name

Done.

https://codereview.chromium.org/23068032/diff/1/Source/bindings/scripts/code_...
Source/bindings/scripts/code_generator_v8.py:184: value = value_raw
On 2013/08/22 07:31:33, haraken wrote:
> I don't fully understand why this is needed.

I have no idea why this is needed either, hence all the FIXMEs.
However, it's what the Perl currently generates.
So it's quicker to land this (this doesn't change anything) and then see if we
can fix it.

> Can we get rid of the complexity by replacing "0X" with "0x" in the IDL files?
> Or if it's really needed, you can do the conversion in the parser.
> (We don't want to mess up the code generator for edge cases no one cares
about.)

I *think* we can just skip this; the only actual effect is listing 0xFFFFFFFF as
-1 instead of 4294967295.

In a followup, I'll see if we remove this from the Perl as well.
If it causes no problems, it cleans up the code quickly.
If it causes problems, we can fix it later.

https://codereview.chromium.org/23068032/diff/1/Source/bindings/scripts/code_...
Source/bindings/scripts/code_generator_v8.py:188: 'name_reflect': name,  #
FIXME: use name_reflect as correct 'name'
On 2013/08/22 07:31:33, haraken wrote:
> 
> name_reflect => reflected_name

Done.

https://codereview.chromium.org/23068032/diff/1/Source/bindings/scripts/code_...
Source/bindings/scripts/code_generator_v8.py:199: def
getter_expression(interface, attribute):
On 2013/08/22 07:31:33, haraken wrote:
> getter_expression => attribute_getter_name ?

attribute_getter is redundant; good point about name vs. expression, but we do
want a function returning the expression (see below).

> You can concatenate 'imp->' in the caller side.

The main function that generates these includes the 'imp->':
get_attribute_function_call_expression in v8_attributes.py:
https://codereview.chromium.org/17572008/diff/181001/Source/bindings/scripts/...

We can probably refactor getter_expression into getter_name + getter_expression
(adding the imp-> in the expression function), but for now I'm keeping it *very*
simple.

https://codereview.chromium.org/23068032/diff/1/Source/bindings/scripts/code_...
Source/bindings/scripts/code_generator_v8.py:382: # FIXME: eliminate
should_keep_attribute_alive
On 2013/08/22 07:31:33, haraken wrote:
> Remove the FIXME. We will need should_keep_attribute_alive.

Got it!
(There was a FIXME asking to remove it, but I guess we need it.)

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/att...
File Source/bindings/templates/attributes.cpp (right):

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/att...
Source/bindings/templates/attributes.cpp:1: {% macro attribute_getter(attribute)
%}
On 2013/08/22 07:31:33, haraken wrote:
> attribute_getter => define_attribute_getter

For functions, I agree that verb names are better,
but for template macros it's clearer to have the macro name agree with the
content,
since it's declarative, so AttrGetter -> attr_getter (changed for consistency).

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/att...
Source/bindings/templates/attributes.cpp:17: {% macro
attribute_return_keep_property_alive(attribute) %}
On 2013/08/22 07:31:33, haraken wrote:
> 
> attribute_return_keep_property_alive => keep_attribute_alive
> 
> It looks like we can make the naming consistent by starting the name from a
> verb.

For these block templates, I'm not sure how to name them;
something like "the effective statement".
Here the bottom line seems to be set_return, but we want to keep it alive.
Also, we know it's an attribute, so how about set_return_keep_alive?

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/att...
Source/bindings/templates/attributes.cpp:30: {% macro
attribute_getter_callback(attribute) %}
On 2013/08/22 07:31:33, haraken wrote:
> attribute_getter_callback => define_attribute_getter_callback

(As above, declarative template, not function.)

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/att...
Source/bindings/templates/attributes.cpp:42: {% macro batched_attribute() %}
On 2013/08/22 07:31:33, haraken wrote:
> 
> batched_attribute => install_attributes
> 
> "Batched" sounds a bit unclear. "Install" would be better.

I'm naming the macros after what they define (per above), so given name
{{v8_class_name}}Attrs,
actually class_attrs is better (confused type vs. name).

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/att...
Source/bindings/templates/attributes.cpp:43: static const
V8DOMConfiguration::BatchedAttribute {{v8_class_name}}Attrs[] = {
On 2013/08/22 07:31:33, haraken wrote:
> {{v8_class_name}}Attrs[] => {{v8_class_name}}Attributes[]

(Perl compatibility.)

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/att...
Source/bindings/templates/attributes.cpp:45: // Attribute '{{attribute.name}}'
On 2013/08/22 07:31:33, haraken wrote:
> You can remove this comment.

(Perl compatibility.)

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/att...
Source/bindings/templates/attributes.cpp:46: {"{{attribute.name}}",
{{cpp_class_name}}V8Internal::{{attribute.name}}AttrGetterCallback, 0, 0, 0, 0
/* no data */, static_cast<v8::AccessControl>(v8::DEFAULT),
static_cast<v8::PropertyAttribute>(v8::None), 0 /* on instance */},
On 2013/08/22 07:31:33, haraken wrote:
> You can remove /*no data*/. It isn't helpful.

(Perl compatibility.)

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/con...
File Source/bindings/templates/constants.cpp (right):

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/con...
Source/bindings/templates/constants.cpp:4: {% macro class_consts() %}
On 2013/08/22 07:31:33, haraken wrote:
> class_consts => define_constants

(See above.)

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/con...
Source/bindings/templates/constants.cpp:5: static const
V8DOMConfiguration::BatchedConstant {{v8_class_name}}Consts[] = {
On 2013/08/22 07:31:33, haraken wrote:
> {{v8_class_name}}Consts[] => {{v8_class_name}}Constants[]

(Perl.)

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/con...
Source/bindings/templates/constants.cpp:20: 
On 2013/08/22 07:31:33, haraken wrote:
> 
> Nit: Unnecessary empty line.

(Perl.)

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/con...
Source/bindings/templates/constants.cpp:25:
COMPILE_ASSERT({{constant.value_raw}} ==
{{cpp_class_name}}::{{constant.name_reflect}},
{{cpp_class_name}}Enum{{constant.name_reflect}}IsWrongUseDoNotCheckConstants);
On 2013/08/22 07:31:33, haraken wrote:
> 
> {{cpp_class_name}}Enum{{constant.name_reflect}}IsWrongUseDoNotCheckConstants
=>
>
TheValueOf{{cpp_class_name}}::{{constant.name_reflect}}Doesn'tMatchWithImplementation

Perl.
...and should:
Doesn'tMatchWithImplementation
...be:
DoesntMatchWithImplementation
(no ' ?)

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/con...
Source/bindings/templates/constants.cpp:35: {% macro const_data(constant) %}
On 2013/08/22 07:31:33, haraken wrote:
> const_data => install_runtime_enabled_constant

(See above.)

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/con...
Source/bindings/templates/constants.cpp:41: static const
V8DOMConfiguration::BatchedConstant constData = {"{{constant.name}}",
static_cast<signed int>({{constant.value}})};
On 2013/08/22 07:31:33, haraken wrote:
> constData => constantConfiguration ?

I *think* the "Configuration" is just for initialization; after that we should
just think of it as data.

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/con...
Source/bindings/templates/constants.cpp:46: {%- endif %}
On 2013/08/22 07:31:33, haraken wrote:
> Ditto. What is the '-' ?

It removes trailing whitespace; I'm using it here so we can have the endif on a
separate line (for clarity), otherwise it would read:
{% if constant.conditional_string %}
#endif // {{constant.conditional_string}}{% endif %}

There used to be a *huge* number of these all over the templates, but I've
reduced it to only the necessary ones (or clarity, like here); this simplifies
the templates a lot and means we exactly match the Perl linebreaks too (except
in one place where I'll want to fix the Perl CG, as there's an indented blank
line). (It wasn't much work, once I understood how it worked.)

An overall "FIXME" is to remove all of these once we've removed Perl, but it's a
huge waste of time to fix whitespace in the Perl CG!

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/con...
Source/bindings/templates/constants.cpp:52: {% macro batch_configure_constants()
%}
On 2013/08/22 07:31:33, haraken wrote:
> batch_configure_constants => install_constants

(See above.)

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/con...
Source/bindings/templates/constants.cpp:53:
V8DOMConfiguration::batchConfigureConstants(desc, proto,
{{v8_class_name}}Consts, WTF_ARRAY_LENGTH({{v8_class_name}}Consts), isolate);
On 2013/08/22 07:31:33, haraken wrote:
> Probably we want to rename the method to installConstants.

FIXME added!

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/int...
File Source/bindings/templates/interface.cpp (left):

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/int...
Source/bindings/templates/interface.cpp:155: 
On 2013/08/22 07:31:33, haraken wrote:
> Nit: This empty line would be needed.

(Perl.)

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/int...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/int...
Source/bindings/templates/interface.cpp:62: {{attribute_getter(attribute) -}}
On 2013/08/22 07:31:33, haraken wrote:
> What is the '-' ?

For whitespace control; see above.
One annoyance with Jinja is you can't write:
{{some_macro}}
...on a line by itself and have it be *empty* if the macro returns nothing,
because there's that newline after the macro.
You can avoid this by wrapping it in an {% if %} ... {% endif %},
since *control blocks* have the trailing newline stripped, but that makes it
less clear.
That's also why the {% endfor %} is not on a separate line.

In some cases we can be careful and get the spacing right, but here it's simpler
to just strip the space manually.

Again, an overall FIXME once Perl is gone is to make the templates clearer by
removing all these hacks.

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/int...
Source/bindings/templates/interface.cpp:124: 
On 2013/08/22 07:31:33, haraken wrote:
> Nit: Unnecessary empty line.

(Perl.)

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/int...
Source/bindings/templates/interface.cpp:136: 
On 2013/08/22 07:31:33, haraken wrote:
> 
> Ditto.

(Perl.)

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/int...
File Source/bindings/templates/interface.h (right):

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/int...
Source/bindings/templates/interface.h:126: inline v8::Handle<v8::Value>
toV8(PassRefPtr< {{cpp_class_name}} > impl, v8::Handle<v8::Object>
creationContext, v8::Isolate* isolate)
On 2013/08/22 07:31:33, haraken wrote:
> Nit: You don't need a space before {{cpp_class_name}}. The same comment for
> other places in this CL.

(Perl compatibility.)

[haraken, 2013-08-22 11:05:27]

> 1. There are lots of hacks for Perl compatibility.
Will fix easy ones in immediate followup (changing both Perl and Python), and
> hard ones once Perl is removed.
> This balances keeping new CG and templates clean with landing code ASAP and
not
> wasting time on old CG.

That being said, it's a bit sad thing that there are a lot of places where you
use undesirable names just to keep the Perl compatibility. Since it would be
easy to rename variables in the Perl generator (you can just search and
replace), I might want to make the rename before this CL and then get back to
this CL (or you can do the rename in this CL in one go).

> 3. Template macro names as declarative nouns.
> As discussed in comments below, I agree that for functions we should use
verbs.
> However, for macros in templates it's not doing anything, and instead just
being
> *substituted* (it's declarative).
> Thus it's clearer if the name of the macro is the name of the contents, like
> attr_getter_callback for AttrGetterCallback.
> This means you can just mentally substitute it.

I don't much care about nouns or verbs, but let's keep in mind that we use
consistent names through the IDL compiler. For example, it is inconsistent that
you use set_return_keep_alive in the template but use
should_attribute_keep_alive in the code generator.

Actually, this is one of the reasons I want to avoid modularizing templates. If
you don't modularize the templates, these concerns are gone -- we don't need to
care about naming consistency between template variables and template macros.

What's the benefit of modularizing the templates? I don't see any benefit in the
modularization, because in most cases one template macro is used by only one
template. Then we can just expand the macro in the template... The less
redirection, the more readable.

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

https://codereview.chromium.org/23068032/diff/1/Source/bindings/scripts/code_...
Source/bindings/scripts/code_generator_v8.py:139: def
cpp_value_to_js_value_return(data_type, cpp_value, callback_info=''):
> The expressions are of the form
> 'v8String({cpp_value}, {isolate})'
> ...while the set return statements are of the from
> 'v8SetReturnValueUnsigned({callback_info}, {cpp_value});'

Makes sense. Then set_v8_return_value ?

https://codereview.chromium.org/23068032/diff/1/Source/bindings/scripts/code_...
Source/bindings/scripts/code_generator_v8.py:184: value = value_raw
On 2013/08/22 09:08:13, Nils Barth wrote:
> On 2013/08/22 07:31:33, haraken wrote:
> > I don't fully understand why this is needed.
> 
> I have no idea why this is needed either, hence all the FIXMEs.
> However, it's what the Perl currently generates.
> So it's quicker to land this (this doesn't change anything) and then see if we
> can fix it.

OK, then let's fix it in a follow-up CL.

https://codereview.chromium.org/23068032/diff/1/Source/bindings/scripts/code_...
Source/bindings/scripts/code_generator_v8.py:199: def
getter_expression(interface, attribute):

If this function is going to be used by other getters (indexed getters, named
getters etc), getter_expression sounds nice. Otherwise, attribute_getter_name
would sound nicer.

Given that it is the caller side that declares "imp", I'd put "imp" in the
caller side. Specifically, the caller side can say
"imp->"+attribute_getter_name(interface, attribute).

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/att...
File Source/bindings/templates/attributes.cpp (right):

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/att...
Source/bindings/templates/attributes.cpp:42: {% macro batched_attribute() %}
On 2013/08/22 09:08:13, Nils Barth wrote:
> On 2013/08/22 07:31:33, haraken wrote:
> > 
> > batched_attribute => install_attributes
> > 
> > "Batched" sounds a bit unclear. "Install" would be better.
> 
> I'm naming the macros after what they define (per above), so given name
> {{v8_class_name}}Attrs,
> actually class_attrs is better (confused type vs. name).

I'm suggesting naming everything consistently. For example, the fact that you
use "attributes" in the code generator but use "attrs" in the template is
inconsistent.

- You can rename V8DOMConfiguraton::BatchedAttribute to
V8DOMConfiguration::InstalledAttributes.
- You can rename {{v8_class_name}}Attrs[] to {{v8_class_name}}Attributes.
- You can rename class_attrs to installed_attributes

etc.

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/con...
File Source/bindings/templates/constants.cpp (right):

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/con...
Source/bindings/templates/constants.cpp:41: static const
V8DOMConfiguration::BatchedConstant constData = {"{{constant.name}}",
static_cast<signed int>({{constant.value}})};
On 2013/08/22 09:08:13, Nils Barth wrote:
> On 2013/08/22 07:31:33, haraken wrote:
> > constData => constantConfiguration ?
> 
> I *think* the "Configuration" is just for initialization; after that we should
> just think of it as data.

This is used just for initialization. That's the reason why I want to rename it
to "Configuration" :) At the same time, I want to rename "Batch" to "Install" to
make it clearer.

[Nils Barth (inactive), 2013-08-23 01:29:04]

(Reply to general comments; specific replies follow.)

On 2013/08/22 11:05:27, haraken wrote:
> That being said, it's a bit sad thing that there are a lot of places where you
> use undesirable names just to keep the Perl compatibility. Since it would be
> easy to rename variables in the Perl generator (you can just search and
> replace), I might want to make the rename before this CL and then get back to
> this CL (or you can do the rename in this CL in one go).

Agreed regarding changing names ASAP; will do before this CL.

It's a bit easier to do after, since that way I can change both generators in
sync
(and not have to do one, then update this CL), but it makes this CL and the
history uglier than necessary.
For future CLs, I'll change names in Perl as soon as identified, so the Python
CL is clean.


> I don't much care about nouns or verbs, but let's keep in mind that we use
> consistent names through the IDL compiler. For example, it is inconsistent
that
> you use set_return_keep_alive in the template but use
> should_attribute_keep_alive in the code generator.

Good point.
This macro is one place where naming is tricky, as it's a block, not
a whole function; elsewhere naming is clear.
(I'll think if there's a better name.)


> If you don't modularize the templates, these concerns are gone -- we don't
need to
> care about naming consistency between template variables and template macros.

That's a good point, though we do want consistency between the templates and the
code generator generally!
There are very few of these block macros -- it's primarily "whole function"
macros.
Some other discrepancies (from Koji's code) include a deprecation_notification
macro
which calls the UseCounter::countDeprecation function.


> What's the benefit of modularizing the templates? I don't see any benefit in
the
> modularization, because in most cases one template macro is used by only one
> template. Then we can just expand the macro in the template... The less
> redirection, the more readable.

I'm only using modularization for the overall interface.cpp file, plus one or
two
places in attribute getters.
(There are some other macros in macros.cpp that I haven't looked at much.)
The benefit is clearly showing structure, and making the overall interface.cpp
file
much shorter.
It's very hard to read long macros, especially since the nesting in unclear:
you can't tell where you are. Inlining the macros yields long code that is hard
to scan.

I agree that having deeply nested macros gets very confusing (we don't want a
macro for
each if block), but it's almost all only 1 level deep, in rare cases 2.

For example, take a look at interface_wip.cpp, which is currently 500+ lines.

https://codereview.chromium.org/17572008/diff/181001/Source/bindings/template...

In the section:
{% for function in functions %}
{% if function.name and not function.is_custom %}
{% for for_main_world_suffix in function.for_main_world_suffixes %}
{{method(function, for_main_world_suffix)}}{% endfor %}
{% endif %}
{% for for_main_world_suffix in function.for_main_world_suffixes %}
{{method_callback(function, for_main_world_suffix)}}{% endfor %}
{% endfor %}
{% if not has_custom_constructor %}
{{make_constructors()}}{% endif %}

...the method, method_callback, and make_constructors macros are only called
here.
They could be inlined, but this makes the overall structure very clear:
we have methods, then method callbacks, then constructors.

OTOH, look at the code starting at line 322:
static v8::Handle<v8::FunctionTemplate> Configure{{v8_class_name}}Template
It's very long (it takes 15% of the file); both this function and the overall
file
would be more readable were this a separate macro.

The only place (other than 1-to-1 macro/function in interface.cpp) a macro seems
useful
is in attribute getters.

If you take a look at:
https://codereview.chromium.org/17572008/diff/181001/Source/bindings/template...
...you'll see that over half the function is one big fork of two cases:
should_keep_property_alive
or not, and the nesting is very confusing.
Reading this you need to jump around anyway, either in the if block or to a
separate function.
So that's the reason for this macro now: these two macros will grow a lot, and
this avoids having
to refactor later.

[Nils Barth (inactive), 2013-08-23 01:50:04]

Inline replies; I'll make a "Perl naming" CL first, per below
comments, then update this one to match.

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

https://codereview.chromium.org/23068032/diff/1/Source/bindings/scripts/code_...
Source/bindings/scripts/code_generator_v8.py:139: def
cpp_value_to_js_value_return(data_type, cpp_value, callback_info=''):
On 2013/08/22 11:05:28, haraken wrote:
> > The expressions are of the form
> > 'v8String({cpp_value}, {isolate})'
> > ...while the set return statements are of the from
> > 'v8SetReturnValueUnsigned({callback_info}, {cpp_value});'
> 
> Makes sense. Then set_v8_return_value ?

Good point; actually, v8_set_return_value seems even clearer, no?

https://codereview.chromium.org/23068032/diff/1/Source/bindings/scripts/code_...
Source/bindings/scripts/code_generator_v8.py:184: value = value_raw
On 2013/08/22 11:05:28, haraken wrote:
> OK, then let's fix it in a follow-up CL.

Got it!

https://codereview.chromium.org/23068032/diff/1/Source/bindings/scripts/code_...
Source/bindings/scripts/code_generator_v8.py:199: def
getter_expression(interface, attribute):
Looking at this more, at this point there's no need for a separate function,
since it's barely doing anything.
Once we actually have complex logic, we can see how it's sensible to factor.

On 2013/08/22 11:05:28, haraken wrote:
> If this function is going to be used by other getters (indexed getters, named
> getters etc), getter_expression sounds nice. Otherwise, attribute_getter_name
> would sound nicer.

It's just used for regular getters.

> Given that it is the caller side that declares "imp", I'd put "imp" in the
> caller side. Specifically, the caller side can say
> "imp->"+attribute_getter_name(interface, attribute).

Got it, that's good to know!
In that case the function just becomes "uncapitalize", so it's unnecessary
abstraction.

For context, the function is GetterExpression in Perl,
and get_attribute_function_call_expression in the WIP:
https://codereview.chromium.org/17572008/diff/181001/Source/bindings/scripts/...
It's a pretty long function, and there's adding/stripping of imp->,
so when we actually start landing this, we'll probably want to refactor it so
it's sensible.

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/att...
File Source/bindings/templates/attributes.cpp (right):

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/att...
Source/bindings/templates/attributes.cpp:42: {% macro batched_attribute() %}
On 2013/08/22 11:05:28, haraken wrote:
> I'm suggesting naming everything consistently. For example, the fact that you
> use "attributes" in the code generator but use "attrs" in the template is
> inconsistent.
> 
> - You can rename V8DOMConfiguraton::BatchedAttribute to
> V8DOMConfiguration::InstalledAttributes.
> - You can rename {{v8_class_name}}Attrs[] to {{v8_class_name}}Attributes.
> - You can rename class_attrs to installed_attributes

Got it, will change in Perl first, then update this.

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/con...
File Source/bindings/templates/constants.cpp (right):

https://codereview.chromium.org/23068032/diff/1/Source/bindings/templates/con...
Source/bindings/templates/constants.cpp:41: static const
V8DOMConfiguration::BatchedConstant constData = {"{{constant.name}}",
static_cast<signed int>({{constant.value}})};
On 2013/08/22 11:05:28, haraken wrote:
> On 2013/08/22 09:08:13, Nils Barth wrote:
> This is used just for initialization. That's the reason why I want to rename
it
> to "Configuration" :) At the same time, I want to rename "Batch" to "Install"
to
> make it clearer.

Got it, will do!

[haraken, 2013-08-23 02:09:47]

> Agreed regarding changing names ASAP; will do before this CL.
> 
> It's a bit easier to do after, since that way I can change both generators in
> sync
> (and not have to do one, then update this CL), but it makes this CL and the
> history uglier than necessary.
> For future CLs, I'll change names in Perl as soon as identified, so the Python
> CL is clean.

Thanks. Rather than leaving FIXMEs, it's better to fix them in the same CL if
the fix is easy :)


> > What's the benefit of modularizing the templates? I don't see any benefit in
> the
> > modularization, because in most cases one template macro is used by only one
> > template. Then we can just expand the macro in the template... The less
> > redirection, the more readable.
> 
> I'm only using modularization for the overall interface.cpp file, plus one or
> two
> places in attribute getters.
> (There are some other macros in macros.cpp that I haven't looked at much.)
> The benefit is clearly showing structure, and making the overall interface.cpp
> file
> much shorter.
> It's very hard to read long macros, especially since the nesting in unclear:
> you can't tell where you are. Inlining the macros yields long code that is
hard
> to scan.
> 
> I agree that having deeply nested macros gets very confusing (we don't want a
> macro for
> each if block), but it's almost all only 1 level deep, in rare cases 2.

OK, but please be careful not to modularize too much. Too much modularization
just makes code confusing:

- I'm fine with having common_macros.cpp for macros that are shared by multiple
templates (e.g., UseCounter::countDeprecation).

- I'm OK with having attributes.cpp and methods.cpp, because they are
complicated. However, I don't want to have attributes.h, methods.h,
indexed_properties.cpp, named_properties.cpp, constants.cpp,
tov8_conversions.cpp etc.

- As you mentioned, I don't want to have deep nested macros more than 1.


> though we do want consistency between the templates and the code generator
generally!

This sounds like a good idea.

[Nils Barth (inactive), 2013-08-23 02:15:32]

On 2013/08/23 02:09:47, haraken wrote:
> OK, but please be careful not to modularize too much. Too much modularization
> just makes code confusing:
> 
> - I'm fine with having common_macros.cpp for macros that are shared by
multiple
> templates (e.g., UseCounter::countDeprecation).
> 
> - I'm OK with having attributes.cpp and methods.cpp, because they are
> complicated. However, I don't want to have attributes.h, methods.h,
> indexed_properties.cpp, named_properties.cpp, constants.cpp,
> tov8_conversions.cpp etc.

Got it!

[Nils Barth (inactive), 2013-08-26 07:35:07]

Finalized (hopefully).

This fixes per comments, including:
* Remove excess template nesting (attribute_getter now flat).

A few other changes:
* Treats string constants properly (adds quotes) -- caught by test!
* Fixes one inconsistency in Perl CG (doesn't always include comment on
Conditional endif)
* Refactors TestConstants.idl out, adding tests for EnableRuntime (were
missing); this caught the string constants bug (fixed above).
* FileError.idl has now been changed to be more complicated (it has inheritance
and DeprecatedAs), so no longer moved to Python flow

There are a couple FIXMEs (change one name, remove some headers) that require
Perl changes, which I'll do in a followup CL.

[haraken, 2013-08-26 09:33:40]

Thanks for the iterative updates! LGTM.

Some of the below comments will be hard to address without changing the Perl
generator. You can do them in follow-up CLs.

https://codereview.chromium.org/23068032/diff/26001/Source/bindings/scripts/c...
File Source/bindings/scripts/code_generator_v8.py (right):

https://codereview.chromium.org/23068032/diff/26001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.py:142: expression_format_string =
V8_SET_RETURN_VALUE_DICT[this_cpp_type]

You can just write:

  return
V8_SET_RETURN_VALUE_DICT[this_cpp_type].format(callback_info=callback_info,
cpp_value=cpp_value)

?

https://codereview.chromium.org/23068032/diff/26001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.py:183: 'enable_function':
runtime_enable_function_name(constant),

Nit: enable_function => runtime_enable_function_name ?

https://codereview.chromium.org/23068032/diff/26001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.py:272: # Fall back to pointer

Nit: I'd drop the comment.

https://codereview.chromium.org/23068032/diff/26001/Source/bindings/templates...
File Source/bindings/templates/attributes.cpp (right):

https://codereview.chromium.org/23068032/diff/26001/Source/bindings/templates...
Source/bindings/templates/attributes.cpp:34: {% macro class_attributes() %}

Where is install_attributes() or something like that? It looks inconsistent that
we have install_consts() in constants.cpp but don't have install_attributes() in
attributes.cpp.

Also, I might want to remove class_attributes() by moving the contents to
install_attributes(). See my comment in constants.cpp.

https://codereview.chromium.org/23068032/diff/26001/Source/bindings/templates...
File Source/bindings/templates/constants.cpp (right):

https://codereview.chromium.org/23068032/diff/26001/Source/bindings/templates...
Source/bindings/templates/constants.cpp:3: {% macro class_constants() %}

I think you can remove class_constants() and move the contents to
install_constants(). Just like runtime enabled constants, you can define normal
constants just before installing the constants.

https://codereview.chromium.org/23068032/diff/26001/Source/bindings/templates...
Source/bindings/templates/constants.cpp:38: {% macro
constant_configuration(constant) %}

constant_configuration => install_runtime_enabled_constants (for consistency
with install_constants below)

Also, you can pass 'constants' (instead of 'constant') and write the for loop in
the macro.

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

https://codereview.chromium.org/23068032/diff/26001/Source/bindings/templates...
Source/bindings/templates/interface.cpp:86: UNUSED_PARAM(proto); // In some
cases, it will not be used.

Nit: I'd drop the comment.

https://codereview.chromium.org/23068032/diff/26001/Source/bindings/templates...
Source/bindings/templates/interface.cpp:91: {% endif %}

I want to simply write this as follows (See my comments in constants.cpp):

  {% if constants %}
  {{install_runtime_enabled_constants()}}
  {{install_constants()}}
  {% endif %}

https://codereview.chromium.org/23068032/diff/26001/Source/bindings/tests/idl...
File Source/bindings/tests/idls/TestConstants.idl (right):

https://codereview.chromium.org/23068032/diff/26001/Source/bindings/tests/idl...
Source/bindings/tests/idls/TestConstants.idl:32: const unsigned short
CONST_VALUE_0 = 0;

CONST_VALUE_0 => ConstValue0 (according to Blink coding style)

https://codereview.chromium.org/23068032/diff/26001/Source/bindings/tests/idl...
Source/bindings/tests/idls/TestConstants.idl:53: [Reflect=CONST_IMPL] const
short CONST_JAVASCRIPT = 1;

You can keep these tests in TestObject.idl. We might not want to increase the
number of test files because it just increases diff when reviewing. (e.g., toV8
family is generated to each test result file.)

https://codereview.chromium.org/23068032/diff/26001/Source/bindings/tests/res...
File Source/bindings/tests/results/V8TestConstants.cpp (right):

https://codereview.chromium.org/23068032/diff/26001/Source/bindings/tests/res...
Source/bindings/tests/results/V8TestConstants.cpp:111: static
v8::Handle<v8::FunctionTemplate>
ConfigureV8TestConstantsTemplate(v8::Handle<v8::FunctionTemplate> desc,
v8::Isolate* isolate, WrapperWorldType currentWorldType)

ConfigureV8TestConstantsTemplate => InstallDOMClassTemplate ? (consistency with
installDOMClassTemplate)

https://codereview.chromium.org/23068032/diff/26001/Source/bindings/tests/res...
Source/bindings/tests/results/V8TestConstants.cpp:124: if
(RuntimeEnabledFeatures::rUNTIME_ENABLED_CONSTEnabled()) {

"rUNTIME" looks wrong.

[Nils Barth (inactive), 2013-08-27 02:59:27]

Thanks for comments; revised and finalized.
A few changes require more Perl CG changes, so I'll do in a followup.

https://codereview.chromium.org/23068032/diff/26001/Source/bindings/scripts/c...
File Source/bindings/scripts/code_generator_v8.py (right):

https://codereview.chromium.org/23068032/diff/26001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.py:142: expression_format_string =
V8_SET_RETURN_VALUE_DICT[this_cpp_type]
On 2013/08/26 09:33:40, haraken wrote:
> You can just write:
> 
>   return
> V8_SET_RETURN_VALUE_DICT[this_cpp_type].format(callback_info=callback_info,
> cpp_value=cpp_value)

That would be simpler now (with only one case), but when we support more cases,
they'll each have their own format string, so it's easier to do all the filling
in at the end.
...and adding more types (to attributes) is what I'm planning to do next!

You can see in the WIP:
https://codereview.chromium.org/17572008/diff/181001/Source/bindings/scripts/...

https://codereview.chromium.org/23068032/diff/26001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.py:183: 'enable_function':
runtime_enable_function_name(constant),
> Nit: enable_function => runtime_enable_function_name ?

Good point, done!

https://codereview.chromium.org/23068032/diff/26001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.py:272: # Fall back to pointer
> Nit: I'd drop the comment.

Done (covered in docstring anyway).

https://codereview.chromium.org/23068032/diff/26001/Source/bindings/templates...
File Source/bindings/templates/attributes.cpp (right):

https://codereview.chromium.org/23068032/diff/26001/Source/bindings/templates...
Source/bindings/templates/attributes.cpp:34: {% macro class_attributes() %}
On 2013/08/26 09:33:40, haraken wrote:
> Where is install_attributes() or something like that?

V8DOMConfiguration::installAttributes is only called once, on Window (Perl line
4189).
V8DOMConfiguration::installAttribute is called for RuntimeEnabled attributes,
which we haven't ported to Python yet.

> It looks inconsistent that we have install_consts() in constants.cpp
> but don't have install_attributes() in attributes.cpp.

It's in the WIP, but not in this CL:
https://codereview.chromium.org/17572008/diff/181001/Source/bindings/template...

> Also, I might want to remove class_attributes() by moving the contents to
> install_attributes().

Got it (see comments below).
It involves a bit of messing reordering of the Perl CG, so I'll do in a
follow-up.

https://codereview.chromium.org/23068032/diff/26001/Source/bindings/templates...
File Source/bindings/templates/constants.cpp (right):

https://codereview.chromium.org/23068032/diff/26001/Source/bindings/templates...
Source/bindings/templates/constants.cpp:3: {% macro class_constants() %}
On 2013/08/26 09:33:40, haraken wrote:
> I think you can remove class_constants() and move the contents to
> install_constants(). Just like runtime enabled constants, you can define
normal
> constants just before installing the constants.

Got it; it's just a variable used in the installConstants call.
It's a bit messy to rearrange the Perl, so I'll do this in a followup.

https://codereview.chromium.org/23068032/diff/26001/Source/bindings/templates...
Source/bindings/templates/constants.cpp:38: {% macro
constant_configuration(constant) %}
On 2013/08/26 09:33:40, haraken wrote:
> constant_configuration => install_runtime_enabled_constants (for consistency
> with install_constants below)

Actually merged into install_constants!

> Also, you can pass 'constants' (instead of 'constant') and write the for loop
in
> the macro.

In this case this works, because we can wrap the call site in {% if constants
%};
I rearranged Perl slightly.

In general it's tricky though:
As a subtle Jinja issue, we can't just have a possibly-blank macro on a line by
itself,
as then in *always* generates a newline:
{{foo}}
{{bar}}
{{zork}}
...has 3 newlines, and if the macro is blank, you just get blank lines (this
causes lots of noise).
There's auto-newline stripping around blocks though, so:
{% if foo %}{{bar(foo)}}{% endif %}
or:
{% if foo %}
{{bar(foo)}}
{% endif %}
work (no noise), which is why we have that everywhere.
Same issue with:
{% for constant in constants %}{{constant_configuration(constant)}}{% endfor %}

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

https://codereview.chromium.org/23068032/diff/26001/Source/bindings/templates...
Source/bindings/templates/interface.cpp:86: UNUSED_PARAM(proto); // In some
cases, it will not be used.
On 2013/08/26 09:33:40, haraken wrote:
> Nit: I'd drop the comment.

Done, removed throughout (including Perl).
(It is a pretty useless comment.)

https://codereview.chromium.org/23068032/diff/26001/Source/bindings/templates...
Source/bindings/templates/interface.cpp:91: {% endif %}
On 2013/08/26 09:33:40, haraken wrote:
> I want to simply write this as follows (See my comments in constants.cpp):
> 
>   {% if constants %}
>   {{install_runtime_enabled_constants()}}
>   {{install_constants()}}
>   {% endif %}

Per note in constants.cpp, this has a subtle issue (excess blank line) if there
are no runtime enabled constants, but it can be fixed  by merging the two macros
(so we can use block statement stripping), since planning to merge
class_constants in too, so we'll only need one constant macro.

https://codereview.chromium.org/23068032/diff/26001/Source/bindings/tests/idl...
File Source/bindings/tests/idls/TestConstants.idl (right):

https://codereview.chromium.org/23068032/diff/26001/Source/bindings/tests/idl...
Source/bindings/tests/idls/TestConstants.idl:32: const unsigned short
CONST_VALUE_0 = 0;
On 2013/08/26 09:33:40, haraken wrote:
> 
> CONST_VALUE_0 => ConstValue0 (according to Blink coding style)

Constants in IDL files are generally UPPER_CASE.
See:
http://www.w3.org/TR/WebIDL/#idl-constants
and:
git grep const -- '*.idl'

https://codereview.chromium.org/23068032/diff/26001/Source/bindings/tests/idl...
Source/bindings/tests/idls/TestConstants.idl:53: [Reflect=CONST_IMPL] const
short CONST_JAVASCRIPT = 1;
On 2013/08/26 09:33:40, haraken wrote:
> You can keep these tests in TestObject.idl.

I need to split them out for Python to test them (>.<)
(since Python doesn't pass the rest of TestObject.idl).


> We might not want to increase the number of test files because it
> just increases diff when reviewing.
> (e.g., toV8 family is generated to each test result file.)

We already have this problem, and it's inevitable (unless we have only one test
file), due to lack of orthogonality in always-used features.
We can split most features into small unit tests, but basic changes change
everything :(

Best solution is to have some very basic files (like TestInterfaceEmpty.idl) to
make it easy to review these basic changes, and then we can just skip the others
to check that it's the same change:
it's a bit of noise but it's easy to check that it's just the same 2 line
change.

Also any *basic* change (e.g., toV8, getters) should be in a separate CL anyway
exactly b/c it's spread throughout, so we want to be sure that's the only
change.

https://codereview.chromium.org/23068032/diff/26001/Source/bindings/tests/res...
File Source/bindings/tests/results/V8TestConstants.cpp (right):

https://codereview.chromium.org/23068032/diff/26001/Source/bindings/tests/res...
Source/bindings/tests/results/V8TestConstants.cpp:111: static
v8::Handle<v8::FunctionTemplate>
ConfigureV8TestConstantsTemplate(v8::Handle<v8::FunctionTemplate> desc,
v8::Isolate* isolate, WrapperWorldType currentWorldType)
On 2013/08/26 09:33:40, haraken wrote:
> ConfigureV8TestConstantsTemplate => InstallDOMClassTemplate ? (consistency
with
> installDOMClassTemplate)

This function actually returns a template; most of the install* functions are
void
(i.e., they just install), so this feels different.

https://codereview.chromium.org/23068032/diff/26001/Source/bindings/tests/res...
Source/bindings/tests/results/V8TestConstants.cpp:124: if
(RuntimeEnabledFeatures::rUNTIME_ENABLED_CONSTEnabled()) {
On 2013/08/26 09:33:40, haraken wrote:
> "rUNTIME" looks wrong.

It's pretty ugly, because the CG assumes CamelCase, which isn't true for
constants.
This requires a Perl change too; I'll do in followup.

[commit-bot: I haz the power, 2013-08-27 03:00:51]

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

[haraken, 2013-08-27 03:39:00]

All comments (except for one point below) make sense. I'd be happy if you could
fix the remaining issues in follow-up CLs.

https://codereview.chromium.org/23068032/diff/26001/Source/bindings/tests/idl...
File Source/bindings/tests/idls/TestConstants.idl (right):

https://codereview.chromium.org/23068032/diff/26001/Source/bindings/tests/idl...
Source/bindings/tests/idls/TestConstants.idl:53: [Reflect=CONST_IMPL] const
short CONST_JAVASCRIPT = 1;
That said, until we have a good framework for unit-testing, we don't want to
have TestAttributes.idl, TestOperations.idl etc. It just makes reviewing
annoying. We can put all basic tests into TestObject.idl.

As we've discussed, bindings-tests are not well organized (It's just for
reviewing). Thus we don't want to speed down our work by trying hard to make
bindings-tests workable. (Also, bindings-tests can confuse other developers who
want to modify the generators in the intermediate state.) I'm even OK with
landing the new Python generator without any bindings-tests. You can just verify
that the Python generator generates exactly the same code for real IDL files.

[commit-bot: I haz the power, 2013-08-27 04:19:18]

Change committed as 156763

[Nils Barth (inactive), 2013-08-27 07:37:31]

(For reference: discussed testing offline, agreed not a priority for now.)