Add forEach() to CSSVariablesMap

Add forEach() to CSSVariablesMap

This adds an implementation of forEach from WebIDL MapClass to
CSSVariablesMap.

Specification:
http://dev.w3.org/2006/webapi/WebIDL/#MapClass
http://dev.w3.org/csswg/css-variables/#the-CSSVariablesMap-interface

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

[alancutter (OOO until 2018), 2013-07-29 09:31:02]

Hi Levi, would you be able to review this change?

+arv for CSSOM API change.

[arv (Not doing code reviews), 2013-07-31 16:45:24]

Is there a test somewhere that asserts that `style.var === style.var`?

https://chromiumcodereview.appspot.com/21006006/diff/1/LayoutTests/fast/css/v...
File LayoutTests/fast/css/variables/cssom-computed-style.html (right):

https://chromiumcodereview.appspot.com/21006006/diff/1/LayoutTests/fast/css/v...
LayoutTests/fast/css/variables/cssom-computed-style.html:2: if
(window.testRunner)
Maybe you should switch to testharness.js or js-test-pre.js

Ad-hoc tests are harder to maintain.

https://chromiumcodereview.appspot.com/21006006/diff/1/LayoutTests/fast/css/v...
LayoutTests/fast/css/variables/cssom-computed-style.html:40: pre.innerText +=
"Enumerate variables:\n";
don't use innerText... I know you are just following the pattern here so this is
just an FYI for future code/tests.

Did you know that every time you do "innerText +=" a layout is done?

https://chromiumcodereview.appspot.com/21006006/diff/1/LayoutTests/fast/css/v...
LayoutTests/fast/css/variables/cssom-computed-style.html:44: for (var i in
varList) {
do not use for-in loops to loop over arrays. Use forEach or c style for loop.

https://chromiumcodereview.appspot.com/21006006/diff/1/LayoutTests/fast/css/v...
File LayoutTests/fast/css/variables/cssom-foreach.html (right):

https://chromiumcodereview.appspot.com/21006006/diff/1/LayoutTests/fast/css/v...
LayoutTests/fast/css/variables/cssom-foreach.html:29: function outputItem(item)
{ output.innerText += "    " + item[0] + ": " + item[1] + "\n"; }
new line after {

https://chromiumcodereview.appspot.com/21006006/diff/1/LayoutTests/fast/css/v...
File LayoutTests/fast/css/variables/cssom-read.html (right):

https://chromiumcodereview.appspot.com/21006006/diff/1/LayoutTests/fast/css/v...
LayoutTests/fast/css/variables/cssom-read.html:15: varList.sort();
Why are we sorting this. The order should be deterministic already.

https://chromiumcodereview.appspot.com/21006006/diff/1/LayoutTests/fast/css/v...
LayoutTests/fast/css/variables/cssom-read.html:16: for (var i in varList) {
forEach or c style loop

https://chromiumcodereview.appspot.com/21006006/diff/1/Source/core/css/CSSVar...
File Source/core/css/CSSVariablesMap.idl (right):

https://chromiumcodereview.appspot.com/21006006/diff/1/Source/core/css/CSSVar...
Source/core/css/CSSVariablesMap.idl:35: void
forEach(CSSVariablesMapForEachCallback callback, any thisArg);
can this just be?

void forEach(CSSVariablesMapForEachCallback callback, optional any thisArg);

[alancutter (OOO until 2018), 2013-08-05 09:33:49]

+darktears. Could you please review the changes to core/css?

+haraken. Could you please review the IDL generator changes?
callback.h and callback.cpp needed to be suffixed with .tmpl to avoid triggering
the style check going crazy on Jinja2 templates.


> Is there a test somewhere that asserts that `style.var === style.var`?

Added check for style.var === style.var.

https://codereview.chromium.org/21006006/diff/1/LayoutTests/fast/css/variable...
File LayoutTests/fast/css/variables/cssom-computed-style.html (right):

https://codereview.chromium.org/21006006/diff/1/LayoutTests/fast/css/variable...
LayoutTests/fast/css/variables/cssom-computed-style.html:2: if
(window.testRunner)
On 2013/07/31 16:45:24, arv wrote:
> Maybe you should switch to testharness.js or js-test-pre.js
> 
> Ad-hoc tests are harder to maintain.

I agree with doing this. Perhaps change all variables tests to use the test
utils in a separate patch.

https://codereview.chromium.org/21006006/diff/1/LayoutTests/fast/css/variable...
LayoutTests/fast/css/variables/cssom-computed-style.html:40: pre.innerText +=
"Enumerate variables:\n";
On 2013/07/31 16:45:24, arv wrote:
> don't use innerText... I know you are just following the pattern here so this
is
> just an FYI for future code/tests.
> 
> Did you know that every time you do "innerText +=" a layout is done?

Thanks for the advice. I'll avoid using it in future.

https://codereview.chromium.org/21006006/diff/1/LayoutTests/fast/css/variable...
LayoutTests/fast/css/variables/cssom-computed-style.html:44: for (var i in
varList) {
On 2013/07/31 16:45:24, arv wrote:
> do not use for-in loops to loop over arrays. Use forEach or c style for loop.

Done.

https://codereview.chromium.org/21006006/diff/1/LayoutTests/fast/css/variable...
File LayoutTests/fast/css/variables/cssom-foreach.html (right):

https://codereview.chromium.org/21006006/diff/1/LayoutTests/fast/css/variable...
LayoutTests/fast/css/variables/cssom-foreach.html:29: function outputItem(item)
{ output.innerText += "    " + item[0] + ": " + item[1] + "\n"; }
On 2013/07/31 16:45:24, arv wrote:
> new line after {

Done.

https://codereview.chromium.org/21006006/diff/1/LayoutTests/fast/css/variable...
File LayoutTests/fast/css/variables/cssom-read.html (right):

https://codereview.chromium.org/21006006/diff/1/LayoutTests/fast/css/variable...
LayoutTests/fast/css/variables/cssom-read.html:15: varList.sort();
On 2013/07/31 16:45:24, arv wrote:
> Why are we sorting this. The order should be deterministic already.

Done.

https://codereview.chromium.org/21006006/diff/1/LayoutTests/fast/css/variable...
LayoutTests/fast/css/variables/cssom-read.html:16: for (var i in varList) {
On 2013/07/31 16:45:24, arv wrote:
> forEach or c style loop

Done.

https://codereview.chromium.org/21006006/diff/1/Source/core/css/CSSVariablesM...
File Source/core/css/CSSVariablesMap.idl (right):

https://codereview.chromium.org/21006006/diff/1/Source/core/css/CSSVariablesM...
Source/core/css/CSSVariablesMap.idl:35: void
forEach(CSSVariablesMapForEachCallback callback, any thisArg);
On 2013/07/31 16:45:24, arv wrote:
> can this just be?
> 
> void forEach(CSSVariablesMapForEachCallback callback, optional any thisArg);

Done.

[haraken, 2013-08-05 10:52:41]

Sorry for nit-picking. I do want to keep the new code generator clean. Thanks
for understanding!

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

https://codereview.chromium.org/21006006/diff/10001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.py:276: call_with_this_value =
'ThisValue' in operation.extended_attributes.get('CallWith', '').split('|')

Could you implement a helper method so that we can write this as follows?

  call_with_this_value = 'ThisValue' in
values_of_extended_attribute(operation.extended_attributes, 'CallWith')

https://codereview.chromium.org/21006006/diff/10001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.py:280: declaration_arguments =
[idl_definitions.IdlArgument('thisValue', 'any')] + declaration_arguments

Can't you write this as follows?

  operation.arguments += [idl_definitions.IdlArgument('thisValue', 'any')]

https://codereview.chromium.org/21006006/diff/10001/Source/bindings/templates...
File Source/bindings/templates/callback.cpp.tmpl (right):

https://codereview.chromium.org/21006006/diff/10001/Source/bindings/templates...
Source/bindings/templates/callback.cpp.tmpl:72:
{{method.this_value_argument.cpp_to_js_conversion | indent(4)}}

Can you hard-code the code?

  v8::Handle<v8::Value> thisHandle = thisValue.v8Value();

Then this_value_argument won't be needed.

https://codereview.chromium.org/21006006/diff/10001/Source/bindings/templates...
Source/bindings/templates/callback.cpp.tmpl:73: if
({{method.this_value_argument.name}}Handle.IsEmpty()) {

Ditto.

  if (thisHandle.IsEmpty()) {

https://codereview.chromium.org/21006006/diff/10001/Source/bindings/templates...
Source/bindings/templates/callback.cpp.tmpl:78:
ASSERT({{method.this_value_argument.name}}Handle->IsObject());

  ASSERT(thisHandle->IsObject());

https://codereview.chromium.org/21006006/diff/10001/Source/bindings/templates...
Source/bindings/templates/callback.cpp.tmpl:95: {% set this_value_param =
'v8::Handle<v8::Object>::Cast(' + method.this_value_argument.name + 'Handle), '
if method.call_with_this_value else '' %}

How about setting call_with_this_value to
"v8::Handle<v8::Object>::Cast(thisHandle)" (or "") ? Then you can just use
call_with_this_value in the template.

We might want to rename call_with_this_value to call_with_this_handle.

In summary, we won't need this_value_argument and this_value_param. We want to
rename call_with_this_value to call_with_this_handle.

[alancutter (OOO until 2018), 2013-08-06 04:47:13]

> Sorry for nit-picking. I do want to keep the new code generator clean. Thanks
for understanding!

Perfectly okay. I want to see the new Python generator nice and clean as well.
:)

Again I needed to add .tmpl to the template files to get past the style checker,
I've done so for interface.cpp/h and callback_interface.cpp/h for consistency.
The other template files in Blink use this naming scheme as well. See
Source/core/scripts/templates.

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

https://codereview.chromium.org/21006006/diff/10001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.py:276: call_with_this_value =
'ThisValue' in operation.extended_attributes.get('CallWith', '').split('|')
On 2013/08/05 10:52:41, haraken wrote:
> 
> Could you implement a helper method so that we can write this as follows?
> 
>   call_with_this_value = 'ThisValue' in
> values_of_extended_attribute(operation.extended_attributes, 'CallWith')

Done.

https://codereview.chromium.org/21006006/diff/10001/Source/bindings/templates...
File Source/bindings/templates/callback.cpp.tmpl (right):

https://codereview.chromium.org/21006006/diff/10001/Source/bindings/templates...
Source/bindings/templates/callback.cpp.tmpl:72:
{{method.this_value_argument.cpp_to_js_conversion | indent(4)}}
On 2013/08/05 10:52:41, haraken wrote:
> 
> Can you hard-code the code?
> 
>   v8::Handle<v8::Value> thisHandle = thisValue.v8Value();
> 
> Then this_value_argument won't be needed.

Done.

https://codereview.chromium.org/21006006/diff/10001/Source/bindings/templates...
Source/bindings/templates/callback.cpp.tmpl:95: {% set this_value_param =
'v8::Handle<v8::Object>::Cast(' + method.this_value_argument.name + 'Handle), '
if method.call_with_this_value else '' %}
On 2013/08/05 10:52:41, haraken wrote:
> 
> How about setting call_with_this_value to
> "v8::Handle<v8::Object>::Cast(thisHandle)" (or "") ? Then you can just use
> call_with_this_value in the template.
> 
> We might want to rename call_with_this_value to call_with_this_handle.
> 
> In summary, we won't need this_value_argument and this_value_param. We want to
> rename call_with_this_value to call_with_this_handle.

I think call_with_this_value is more appropriate as it matches the extended
attribute form [CallWith=ThisValue] but I've changed it to call_with_this_handle
anyway.

this_value_param/this_handle_param is still suitable in the Jinja2 template, but
it can be removed from the Python code.

[haraken, 2013-08-06 05:02:03]

https://codereview.chromium.org/21006006/diff/17001/Source/bindings/templates...
File Source/bindings/templates/callback_interface.cpp.tmpl (right):

https://codereview.chromium.org/21006006/diff/17001/Source/bindings/templates...
Source/bindings/templates/callback_interface.cpp.tmpl:58:
{{method.return_cpp_type}}
{{v8_class_name}}::{{method.name}}({{this_value_param}}{{method.argument_declaration}})

How about putting 'ScriptValue thisValue, ' into method.argument_declaration, so
that we don't need to introduce this_value_param in the template?

Probably my previous comment was confusing? I meant, you can put all the
information into operation.arguments in the code generator (which will be
converted to method.argument_declaration and method.arguments), and just use
them in the template.

https://codereview.chromium.org/21006006/diff/17001/Source/bindings/templates...
Source/bindings/templates/callback_interface.cpp.tmpl:97: return
!invokeCallback(m_callback.newLocal(isolate),
{{this_handle_param}}{{method.arguments | length}}, argv, callbackReturnValue,
scriptExecutionContext());

Ditto. You might want to put 'v8::Handle<v8::Object>::Cast(thisHandle), ' into
method.arguments, so that you can avoid this_handle_param.

https://codereview.chromium.org/21006006/diff/17001/Source/bindings/templates...
File Source/bindings/templates/callback_interface.h.tmpl (right):

https://codereview.chromium.org/21006006/diff/17001/Source/bindings/templates...
Source/bindings/templates/callback_interface.h.tmpl:60: virtual
{{method.return_cpp_type}}
{{method.name}}({{this_value_param}}{{method.argument_declaration}});

Ditto.

[Nils Barth (inactive), 2013-08-06 05:45:17]

Hi Alan,
Sorry for arriving late; just noticed that there was something for me to add
here.

* Could you not rename the Jinja template files?
* Extended attributes are a bit tricky;
I've suggested alternative functions which handle the corner cases and make the
calling a bit simpler.

WDYT?

https://codereview.chromium.org/21006006/diff/17001/Source/bindings/derived_s...
File Source/bindings/derived_sources.gyp (right):

https://codereview.chromium.org/21006006/diff/17001/Source/bindings/derived_s...
Source/bindings/derived_sources.gyp:58: 'code_generator_template_files': [
You're adding a .tmpl extension to the template files.
I think it's better to leave these files as is.

While I understand the motivation, AFAICT Jinja templates
generally use the usual extension:
"A template is simply a text file. It can generate any text-based format (HTML,
XML, CSV, LaTeX, etc.). It doesn’t have a specific extension, .html or .xml are
just fine."
http://jinja.pocoo.org/docs/templates/

If you want syntax highlighting (which is going to be tricky),
there is some code here:
http://jinja.pocoo.org/docs/integration/
...but it looks like we'll have to wrap our own if we want
C++ and Jinja.

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

https://codereview.chromium.org/21006006/diff/17001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.py:213: return
extended_attributes.get(key, '').split('|')
Extended attributes are internally a bit tricky;
I'd suggest using the following functions instead.
Since extended attribute values are frequently used as flags,
a direct checker function "has_extended_attribute_value" seems useful.

Issues include:
* A extended attribute can be absent, present with no values, or present with a
list of values.
* Both '|' and '&' can be used as value separators.

def has_extended_attribute_value(extended_attributes, key, value):
    return (key in extended_attributes and
            value in extended_attributes_values(extended_attributes, key))


def extended_attribute_values(extended_attributes, key):
    if key not in extended_attributes:
        return None
    values_string = extended_attributes[key]
    if not values_string:
        return []
    return re.split('[|&]', values_string)

https://codereview.chromium.org/21006006/diff/17001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.py:330: call_with_this_handle =
'ThisValue' in values_for_extended_attributes(operation.extended_attributes,
'CallWith')
Using above functions, this can be rewritten as:
call_with_this_handle =
has_extended_attribute_value(operation.extended_attributes, 'CallWith',
'ThisValue')

[alancutter (OOO until 2018), 2013-08-07 00:40:23]

> Sorry for arriving late; just noticed that there was something for me to add
here.

No problem! Your review is appreciated.

> * Could you not rename the Jinja template files?

At the moment we're not in a workable situation with the .cpp and .h extensions
as almost any change will fail the presubmit because the style checker thinks
they're C++ files.
There needs to be some way to disable the style checker for these template
files. I chose adding .tmpl to the extension because all the other Jinja2 files
in the repo follow that naming scheme.

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

https://codereview.chromium.org/21006006/diff/17001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.py:213: return
extended_attributes.get(key, '').split('|')
On 2013/08/06 05:45:18, Nils Barth wrote:
> Extended attributes are internally a bit tricky;
> I'd suggest using the following functions instead.
> Since extended attribute values are frequently used as flags,
> a direct checker function "has_extended_attribute_value" seems useful.
> 
> Issues include:
> * A extended attribute can be absent, present with no values, or present with
a
> list of values.
> * Both '|' and '&' can be used as value separators.
> 
> def has_extended_attribute_value(extended_attributes, key, value):
>     return (key in extended_attributes and
>             value in extended_attributes_values(extended_attributes, key))
> 
> 
> def extended_attribute_values(extended_attributes, key):
>     if key not in extended_attributes:
>         return None
>     values_string = extended_attributes[key]
>     if not values_string:
>         return []
>     return re.split('[|&]', values_string)

Sounds good! Done.

https://codereview.chromium.org/21006006/diff/17001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.py:330: call_with_this_handle =
'ThisValue' in values_for_extended_attributes(operation.extended_attributes,
'CallWith')
On 2013/08/06 05:45:18, Nils Barth wrote:
> Using above functions, this can be rewritten as:
> call_with_this_handle =
> has_extended_attribute_value(operation.extended_attributes, 'CallWith',
> 'ThisValue')

Done.

https://codereview.chromium.org/21006006/diff/17001/Source/bindings/templates...
File Source/bindings/templates/callback_interface.cpp.tmpl (right):

https://codereview.chromium.org/21006006/diff/17001/Source/bindings/templates...
Source/bindings/templates/callback_interface.cpp.tmpl:58:
{{method.return_cpp_type}}
{{v8_class_name}}::{{method.name}}({{this_value_param}}{{method.argument_declaration}})
On 2013/08/06 05:02:03, haraken wrote:
> 
> How about putting 'ScriptValue thisValue, ' into method.argument_declaration,
so
> that we don't need to introduce this_value_param in the template?
> 
Done.

https://codereview.chromium.org/21006006/diff/17001/Source/bindings/templates...
Source/bindings/templates/callback_interface.cpp.tmpl:97: return
!invokeCallback(m_callback.newLocal(isolate),
{{this_handle_param}}{{method.arguments | length}}, argv, callbackReturnValue,
scriptExecutionContext());
On 2013/08/06 05:02:03, haraken wrote:
> 
> Ditto. You might want to put 'v8::Handle<v8::Object>::Cast(thisHandle), ' into
> method.arguments, so that you can avoid this_handle_param.

This will change the value of {{method.arguments | length}} and insert the
thisHandle parameter as an argument to the Javascript callback which is not what
is intended. It only needs to be an argument to the C++ callback method.

[haraken, 2013-08-07 00:55:54]

LGTM for the code generator changes. Thank you very much for improving the
half-baked generator!

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

https://codereview.chromium.org/21006006/diff/30001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.py:216: def
extended_attribute_values(extended_attributes, key):

Nit: Do you need this helper? You might be able to inline this method in
has_extended_attribute_value.

https://codereview.chromium.org/21006006/diff/30001/Source/bindings/templates...
File Source/bindings/templates/callback_interface.cpp.tmpl (right):

https://codereview.chromium.org/21006006/diff/30001/Source/bindings/templates...
Source/bindings/templates/callback_interface.cpp.tmpl:95: {% set
this_handle_param = 'v8::Handle<v8::Object>::Cast(thisHandle), ' if
method.call_with_this_handle else '' %}

Nit: this_handle_param => this_handle_parameter (or simply this_handle)

[alancutter (OOO until 2018), 2013-08-07 01:12:47]

> Thank you very much for improving the
half-baked generator!

No problem! I'm glad to be familiarising myself with the new code. :)

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

https://codereview.chromium.org/21006006/diff/30001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.py:216: def
extended_attribute_values(extended_attributes, key):
On 2013/08/07 00:55:54, haraken wrote:
> 
> Nit: Do you need this helper? You might be able to inline this method in
> has_extended_attribute_value.

I think the motivation for making it a separate function was to let other parts
of the code generator grab the list of values easily if they needed it.
Personally I think it's a distinct enough bit of functionality to warrant being
in its own function.

https://codereview.chromium.org/21006006/diff/30001/Source/bindings/templates...
File Source/bindings/templates/callback_interface.cpp.tmpl (right):

https://codereview.chromium.org/21006006/diff/30001/Source/bindings/templates...
Source/bindings/templates/callback_interface.cpp.tmpl:95: {% set
this_handle_param = 'v8::Handle<v8::Object>::Cast(thisHandle), ' if
method.call_with_this_handle else '' %}
On 2013/08/07 00:55:54, haraken wrote:
> 
> Nit: this_handle_param => this_handle_parameter (or simply this_handle)

Done.
I chose this_handle_parameter because it includes the ", ".

[Nils Barth (inactive), 2013-08-07 01:43:50]

On 2013/08/07 00:40:23, alancutter wrote:
> > Sorry for arriving late; just noticed that there was something for me to add
> here.
> 
> No problem! Your review is appreciated.
> 
> > * Could you not rename the Jinja template files?
> 
> At the moment we're not in a workable situation with the .cpp and .h
extensions
> as almost any change will fail the presubmit because the style checker thinks
> they're C++ files.
> There needs to be some way to disable the style checker for these template
> files. I chose adding .tmpl to the extension because all the other Jinja2
files
> in the repo follow that naming scheme.

OIC – thanks, great catch!

I've posted a CL to fix this:
Issue 22514002: Disable check-webkit-style for Jinja templates
https://codereview.chromium.org/22514002/
...but I wouldn't block on this.

LGTM to land with rename, and once other CL lands we can remove the .tmpl
extension
in both places.

[Nils Barth (inactive), 2013-08-07 01:50:34]

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

https://codereview.chromium.org/21006006/diff/30001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.py:216: def
extended_attribute_values(extended_attributes, key):
On 2013/08/07 01:12:48, alancutter wrote:
> On 2013/08/07 00:55:54, haraken wrote:
> > 
> > Nit: Do you need this helper? You might be able to inline this method in
> > has_extended_attribute_value.
> 
> I think the motivation for making it a separate function was to let other
parts
> of the code generator grab the list of values easily if they needed it.
> Personally I think it's a distinct enough bit of functionality to warrant
being
> in its own function.

For reference, I suggested these functions;
as Alan says, I think it's useful to have a separate function,
esp. as the exact handling is a bit tricky and bug-prone.

Agreed that in this case we just need "has_extended_attribute_value",
since we're just checking one value, but in more complex cases I
can see it being easier to use the list of values instead of checking each
individually.

In any case, b/c of the many returns, rather than inlining we'd want
a nested function (to avoid deep nesting), so we don't gain much
compactness-wise either. (>.<)

[alancutter (OOO until 2018), 2013-08-07 02:53:45]

On 2013/08/07 01:43:50, Nils Barth wrote:
> OIC – thanks, great catch!
> 
> I've posted a CL to fix this:
> Issue 22514002: Disable check-webkit-style for Jinja templates
> https://codereview.chromium.org/22514002/
> ...but I wouldn't block on this.
> 
> LGTM to land with rename, and once other CL lands we can remove the .tmpl
> extension
> in both places.

Sounds great! I'll revert the rename once that patch lands. :)

[alancutter (OOO until 2018), 2013-08-07 07:50:38]

Reverted template rename.
IDL generator changes have received LGTM.

darktears: Could you please review the changes to core/css?

[haraken, 2013-08-07 12:28:36]

Sorry for nit-picking: I noticed that template files in core/scripts/templates/
have names like foo.h.tmpl and foo.cpp.tmpl. So we might want to follow the
naming rule as well.

[arv (Not doing code reviews), 2013-08-07 14:16:22]

https://codereview.chromium.org/21006006/diff/42001/LayoutTests/fast/css/vari...
File LayoutTests/fast/css/variables/cssom-computed-style.html (right):

https://codereview.chromium.org/21006006/diff/42001/LayoutTests/fast/css/vari...
LayoutTests/fast/css/variables/cssom-computed-style.html:46: varList.sort();
I'm still kind of concerned about this sort. The order of forEach should be
deterministic and the sort might hide a bug where the order isn't.

https://codereview.chromium.org/21006006/diff/42001/LayoutTests/fast/css/vari...
File LayoutTests/fast/css/variables/cssom-read.html (right):

https://codereview.chromium.org/21006006/diff/42001/LayoutTests/fast/css/vari...
LayoutTests/fast/css/variables/cssom-read.html:14:
styleVar.forEach(function(value, name) { varList.push([name, value]); });
Why not just print inside the forEach? No need for temp varList:

function outputForEach (styleVar) {
  styleVar.forEach(function(value, name) {
    pre.innerText += name + ": " + value + "\n";
  });
}

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

https://codereview.chromium.org/21006006/diff/42001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.py:347: argument_declaration = ',
'.join([argument_declaration(argument) for argument in operation.arguments])
No need for temp array:

argument_declaration = ', '.join(argument_declaration(argument) for argument in
operation.arguments)

https://codereview.chromium.org/21006006/diff/42001/Source/bindings/templates...
File Source/bindings/templates/callback_interface.cpp (right):

https://codereview.chromium.org/21006006/diff/42001/Source/bindings/templates...
Source/bindings/templates/callback_interface.cpp:96: return
!invokeCallback(m_callback.newLocal(isolate),
{{this_handle_parameter}}{{method.arguments | length}}, argv,
callbackReturnValue, scriptExecutionContext());
I feel like there is an issue here if this_handle_parameter is '', don't you get
a trailing comma?

[alancutter (OOO until 2018), 2013-08-08 01:50:57]

> Sorry for nit-picking: I noticed that template files in
core/scripts/templates/
have names like foo.h.tmpl and foo.cpp.tmpl. So we might want to follow the
naming rule as well.

nbarth: Are we able to get consensus on this?

https://codereview.chromium.org/21006006/diff/42001/LayoutTests/fast/css/vari...
File LayoutTests/fast/css/variables/cssom-computed-style.html (right):

https://codereview.chromium.org/21006006/diff/42001/LayoutTests/fast/css/vari...
LayoutTests/fast/css/variables/cssom-computed-style.html:46: varList.sort();
On 2013/08/07 14:16:23, arv wrote:
> I'm still kind of concerned about this sort. The order of forEach should be
> deterministic and the sort might hide a bug where the order isn't.

The order of the computed style's forEach() is determined by the hash algorithm
used for AtomicString (which has been subject to change in recent times and
alter the ordering seen). The spec does not specify the order in which items are
accessed by forEach() so this test should be order agnostic.

The order in inline styles is much less dependent on unrelated things like hash
algorithms and where I test altering the variable map during the forEach() there
is a well defined expectation of ordering. For those tests it makes more sense
to remove the sort().

https://codereview.chromium.org/21006006/diff/42001/LayoutTests/fast/css/vari...
File LayoutTests/fast/css/variables/cssom-read.html (right):

https://codereview.chromium.org/21006006/diff/42001/LayoutTests/fast/css/vari...
LayoutTests/fast/css/variables/cssom-read.html:14:
styleVar.forEach(function(value, name) { varList.push([name, value]); });
On 2013/08/07 14:16:23, arv wrote:
> Why not just print inside the forEach? No need for temp varList:
> 
> function outputForEach (styleVar) {
>   styleVar.forEach(function(value, name) {
>     pre.innerText += name + ": " + value + "\n";
>   });
> }

Thanks for pointing this out, I didn't refactor after removing varList.sort();

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

https://codereview.chromium.org/21006006/diff/42001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.py:347: argument_declaration = ',
'.join([argument_declaration(argument) for argument in operation.arguments])
On 2013/08/07 14:16:23, arv wrote:
> No need for temp array:
> 
> argument_declaration = ', '.join(argument_declaration(argument) for argument
in
> operation.arguments)

I just realised I'm clobbering the argument_declaration function here. Fixed!

I could remove the temp variable but it would end up looking something like:
            method = {
                ...
                'argument_declaration': ('ScriptValue thisValue, ' *
call_with_this_value) + ', '.join([generate_argument_declaration(argument) for
argument in operation.arguments])
                ...
            }

https://codereview.chromium.org/21006006/diff/42001/Source/bindings/templates...
File Source/bindings/templates/callback_interface.cpp (right):

https://codereview.chromium.org/21006006/diff/42001/Source/bindings/templates...
Source/bindings/templates/callback_interface.cpp:96: return
!invokeCallback(m_callback.newLocal(isolate),
{{this_handle_parameter}}{{method.arguments | length}}, argv,
callbackReturnValue, scriptExecutionContext());
On 2013/08/07 14:16:23, arv wrote:
> I feel like there is an issue here if this_handle_parameter is '', don't you
get
> a trailing comma?

This patch uses both code paths, the generated code becomes:

    return !invokeCallback(m_callback.newLocal(isolate),
v8::Handle<v8::Object>::Cast(thisHandle), 3, argv, callbackReturnValue,
scriptExecutionContext());

or

    return !invokeCallback(m_callback.newLocal(isolate), 3, argv,
callbackReturnValue, scriptExecutionContext());


The comma is part of this_handle_parameter when it is set so if it is '' no
additional comma gets inserted.

[Nils Barth (inactive), 2013-08-08 02:07:17]

On 2013/08/07 12:28:36, haraken wrote:
>> Sorry for nit-picking: I noticed that template files in
core/scripts/templates/
>> have names like foo.h.tmpl and foo.cpp.tmpl. So we might want to follow the
>> naming rule as well.

On 2013/08/08 01:50:57, alancutter wrote:
> nbarth: Are we able to get consensus on this?

Hi haraken, the core templates should be renamed; no need to rename anything
here.

This .tmpl extension was due to presubmit scripts (check-webkit-style) being
too aggressive, namely checking Jinja templates (due to .cpp/.h extension),
so we needed .tmpl to prevent these for being checked.
(Alan noticed this earlier in this CL.)

I fixed the checking in this CL:
Issue 22514002: Disable check-webkit-style for Jinja templates
https://codereview.chromium.org/22514002/

Usually Jinja files use the extension of the file they're generating,
so I've posted a CL renaming the core templates to remove .tmpl:
Issue 22546004: Remove .tmpl extension from Jinja templates in core
https://codereview.chromium.org/22546004/

So template renaming can be discussed at that CL (if there's any issue),
and we needn't worry about it for this one.

[arv (Not doing code reviews), 2013-08-08 14:24:50]

LGTM

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

https://codereview.chromium.org/21006006/diff/56001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.py:347: argument_declaration = ',
'.join([generate_argument_declaration(argument) for argument in
operation.arguments])
I was referring to the []. join takes an iterable and you are using an array
comprehension to build the array, just to then iterate over the array again. You
can, simply remove the [] from your code:

argument_declaration = ', '.join(generate_argument_declaration(argu
     ment) for argument in operation.arguments)

[alancutter (OOO until 2018), 2013-08-09 05:33:26]

Thanks for the review.

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

https://codereview.chromium.org/21006006/diff/56001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.py:347: argument_declaration = ',
'.join([generate_argument_declaration(argument) for argument in
operation.arguments])
On 2013/08/08 14:24:51, arv wrote:
> I was referring to the []. join takes an iterable and you are using an array
> comprehension to build the array, just to then iterate over the array again.
You
> can, simply remove the [] from your code:
> 
> argument_declaration = ', '.join(generate_argument_declaration(argu
>      ment) for argument in operation.arguments)

I see now, that is better.
Done.

[alancutter (OOO until 2018), 2013-08-09 05:35:12]

Patch review status:
This patch still needs a review for the changes to core/css before it is
appropriate for me to land it.

[alancutter (OOO until 2018), 2013-08-19 07:08:04]

On 2013/08/09 05:35:12, alancutter wrote:
> Patch review status:
> This patch still needs a review for the changes to core/css before it is
> appropriate for me to land it.

darktears: poke

[alancutter (OOO until 2018), 2013-08-23 08:33:15]

esprehn: Would you be able to review the CSS changes in this patch?

[alancutter (OOO until 2018), 2013-09-02 01:07:29]

Poke.

[abarth-chromium, 2013-09-02 03:25:15]

On 2013/09/02 01:07:29, alancutter wrote:
> Poke.

From your previous comment, it looks like you're intending to poke esprehn. 
It's nice to include the name of the person whose attention you're looking for
otherwise folks like me worry that we've forgotten to do something...

[esprehn, 2013-09-02 07:22:09]

On 2013/09/02 03:25:15, abarth wrote:
> On 2013/09/02 01:07:29, alancutter wrote:
> > Poke.
> 
> From your previous comment, it looks like you're intending to poke esprehn. 
> It's nice to include the name of the person whose attention you're looking for
> otherwise folks like me worry that we've forgotten to do something...

I'll look this over Tuesday when I get back from vacation.

[alancutter (OOO until 2018), 2013-09-02 23:53:05]

On 2013/09/02 07:22:09, esprehn wrote:
> On 2013/09/02 03:25:15, abarth wrote:
> > On 2013/09/02 01:07:29, alancutter wrote:
> > > Poke.
> > 
> > From your previous comment, it looks like you're intending to poke esprehn. 
> > It's nice to include the name of the person whose attention you're looking
for
> > otherwise folks like me worry that we've forgotten to do something...

abarth: Sorry for the noise.
 
> I'll look this over Tuesday when I get back from vacation.

esprehn: Thanks!

[alancutter (OOO until 2018), 2013-09-10 02:13:42]

esprehn: Poke.

[Nils Barth (inactive), 2013-09-10 02:16:46]

On 2013/09/10 02:13:42, alancutter wrote:
> esprehn: Poke.

I believe the new term of art is "squeak": (^.-)v
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/7caxMJ8b7iA

[esprehn, 2013-09-10 02:25:17]

I'll review tonight, sorry the Rietveld extension I use to do reviews fast has
started making Chrome hang (http://crbug.com/282603), so I've been slower to get
back to things. If someone wanted to fix that...

[alancutter (OOO until 2018), 2013-09-12 00:56:11]

On 2013/09/10 02:25:17, esprehn wrote:
> I'll review tonight, sorry the Rietveld extension I use to do reviews fast has
> started making Chrome hang (http://crbug.com/282603), so I've been slower to
get
> back to things. If someone wanted to fix that...

esprehn: Squeak!

[esprehn, 2013-09-12 01:13:41]

I'm not very comfortable with the static map, was that so we don't grow the size
of the object? It's not clear how the map works since you clear it in the
destructor, but the map itself is keeping the things alive? The destructor
should never run then.

https://codereview.chromium.org/21006006/diff/87001/LayoutTests/fast/css/vari...
File LayoutTests/fast/css/variables/cssom-computed-style.html (right):

https://codereview.chromium.org/21006006/diff/87001/LayoutTests/fast/css/vari...
LayoutTests/fast/css/variables/cssom-computed-style.html:25: pre.innerText +=
((preStyle.var === preStyle.var) ? "pass": "fail") + "\n";
This isn't a good test, you need to trigger a gc between the two calls to .var

https://codereview.chromium.org/21006006/diff/87001/LayoutTests/fast/css/vari...
File LayoutTests/fast/css/variables/cssom-foreach.html (right):

https://codereview.chromium.org/21006006/diff/87001/LayoutTests/fast/css/vari...
LayoutTests/fast/css/variables/cssom-foreach.html:1: <script>
Needs a doctype

https://codereview.chromium.org/21006006/diff/87001/LayoutTests/fast/css/vari...
LayoutTests/fast/css/variables/cssom-foreach.html:49: output.innerText += "Test
calling forEach with non-function as first argument: ";
This test seems too long, break it into multiple tests and each one can have a
description() or you can use the debug() function to print lines. You should use
the shouldBe functions as well and not invent your own functions below.

https://codereview.chromium.org/21006006/diff/87001/LayoutTests/fast/css/vari...
LayoutTests/fast/css/variables/cssom-foreach.html:212: ["  innerAdded", "pass"],
Please break this test down into simple steps or multiple tests. Tests that take
arrays of values are super hard to debug.

https://codereview.chromium.org/21006006/diff/87001/LayoutTests/fast/css/vari...
File LayoutTests/fast/css/variables/cssom-read.html (right):

https://codereview.chromium.org/21006006/diff/87001/LayoutTests/fast/css/vari...
LayoutTests/fast/css/variables/cssom-read.html:17: pre.innerText +=
((pre.style.var === pre.style.var) ? "pass": "fail") + "\n";
Same deal here.

https://codereview.chromium.org/21006006/diff/87001/LayoutTests/fast/css/vari...
LayoutTests/fast/css/variables/cssom-read.html:51: pre.innerText += "d: " +
pre.style.var.get("d") + "\n";
innerText += like this is really expensive, this test could be written a lot
simpler and nicer with shouldBe and or something...

https://codereview.chromium.org/21006006/diff/87001/Source/core/css/CSSComput...
File Source/core/css/CSSComputedStyleDeclaration.h (right):

https://codereview.chromium.org/21006006/diff/87001/Source/core/css/CSSComput...
Source/core/css/CSSComputedStyleDeclaration.h:64: HashMap<AtomicString,
String>::const_iterator m_it;
A typedef HashMap<AtomicString, String> VariablesMap would make this nicer.

https://codereview.chromium.org/21006006/diff/87001/Source/core/css/CSSVariab...
File Source/core/css/CSSVariablesMap.cpp (right):

https://codereview.chromium.org/21006006/diff/87001/Source/core/css/CSSVariab...
Source/core/css/CSSVariablesMap.cpp:81: RefPtr<CSSVariablesIterator> iterator =
m_styleDeclaration->variablesIterator();
Is the iterator really necessary? It'd be much nicer if you used a normal C++
iterator.

https://codereview.chromium.org/21006006/diff/87001/Source/core/css/CSSVariab...
File Source/core/css/CSSVariablesMap.h (right):

https://codereview.chromium.org/21006006/diff/87001/Source/core/css/CSSVariab...
Source/core/css/CSSVariablesMap.h:57: void
forEach(PassRefPtr<CSSVariablesMapForEachCallback>, ScriptValue thisArg) const;
When do we pass a ScriptValue by value instead of pointer or reference?

https://codereview.chromium.org/21006006/diff/87001/Source/core/css/StyleProp...
File Source/core/css/StylePropertySet.cpp (right):

https://codereview.chromium.org/21006006/diff/87001/Source/core/css/StyleProp...
Source/core/css/StylePropertySet.cpp:289: if (iterators) {
Put the assignment in the if: if (VariablesIterator* iterator = ...)

https://codereview.chromium.org/21006006/diff/87001/Source/core/css/StyleProp...
Source/core/css/StylePropertySet.cpp:563: if (iterators) {
Same

https://codereview.chromium.org/21006006/diff/87001/Source/core/css/StyleProp...
Source/core/css/StylePropertySet.cpp:660:
DEFINE_STATIC_LOCAL(VariablesIteratorsMap, map, ());
This is scary, I'd rather we didn't have any static global maps.

https://codereview.chromium.org/21006006/diff/87001/Source/core/css/StyleProp...
File Source/core/css/StylePropertySet.h (right):

https://codereview.chromium.org/21006006/diff/87001/Source/core/css/StyleProp...
Source/core/css/StylePropertySet.h:206: PassRefPtr<CSSVariablesIterator>
variablesIterator() { return VariablesIterator::create(this); }
We should really expose var().begin() and var().end() and use a normal C++
iterator.

[alancutter (OOO until 2018), 2013-09-12 03:45:50]

On 2013/09/12 01:13:41, esprehn wrote:
> I'm not very comfortable with the static map, was that so we don't grow the
size
> of the object? It's not clear how the map works since you clear it in the
> destructor, but the map itself is keeping the things alive? The destructor
> should never run then.

Thanks for the review, I'll remove the static map and make the tests use
existing testing resources.

[alancutter (OOO until 2018), 2013-09-13 03:06:13]

esprehn: ptal

https://codereview.chromium.org/21006006/diff/87001/LayoutTests/fast/css/vari...
File LayoutTests/fast/css/variables/cssom-computed-style.html (right):

https://codereview.chromium.org/21006006/diff/87001/LayoutTests/fast/css/vari...
LayoutTests/fast/css/variables/cssom-computed-style.html:25: pre.innerText +=
((preStyle.var === preStyle.var) ? "pass": "fail") + "\n";
On 2013/09/12 01:13:42, esprehn wrote:
> This isn't a good test, you need to trigger a gc between the two calls to .var

This check was suggested by arv after I had been creating new CSSVariablesMaps
with each call to var.
I agree that a GC needs to be done between calls however I don't think it's
possible to accurately test that using Javascript alone. The comparison would
require holding refs to the objects.
It does test at the very least that new vars are not being instantiated
unnecessarily.

https://codereview.chromium.org/21006006/diff/87001/LayoutTests/fast/css/vari...
File LayoutTests/fast/css/variables/cssom-foreach.html (right):

https://codereview.chromium.org/21006006/diff/87001/LayoutTests/fast/css/vari...
LayoutTests/fast/css/variables/cssom-foreach.html:1: <script>
On 2013/09/12 01:13:42, esprehn wrote:
> Needs a doctype

Done.

https://codereview.chromium.org/21006006/diff/87001/LayoutTests/fast/css/vari...
LayoutTests/fast/css/variables/cssom-foreach.html:49: output.innerText += "Test
calling forEach with non-function as first argument: ";
On 2013/09/12 01:13:42, esprehn wrote:
> This test seems too long, break it into multiple tests and each one can have a
> description() or you can use the debug() function to print lines. You should
use
> the shouldBe functions as well and not invent your own functions below.

Done.

https://codereview.chromium.org/21006006/diff/87001/LayoutTests/fast/css/vari...
File LayoutTests/fast/css/variables/cssom-read.html (right):

https://codereview.chromium.org/21006006/diff/87001/LayoutTests/fast/css/vari...
LayoutTests/fast/css/variables/cssom-read.html:51: pre.innerText += "d: " +
pre.style.var.get("d") + "\n";
On 2013/09/12 01:13:42, esprehn wrote:
> innerText += like this is really expensive, this test could be written a lot
> simpler and nicer with shouldBe and or something...

Done.

https://codereview.chromium.org/21006006/diff/87001/Source/core/css/CSSComput...
File Source/core/css/CSSComputedStyleDeclaration.h (right):

https://codereview.chromium.org/21006006/diff/87001/Source/core/css/CSSComput...
Source/core/css/CSSComputedStyleDeclaration.h:64: HashMap<AtomicString,
String>::const_iterator m_it;
On 2013/09/12 01:13:42, esprehn wrote:
> A typedef HashMap<AtomicString, String> VariablesMap would make this nicer.

Done.

https://codereview.chromium.org/21006006/diff/87001/Source/core/css/CSSVariab...
File Source/core/css/CSSVariablesMap.cpp (right):

https://codereview.chromium.org/21006006/diff/87001/Source/core/css/CSSVariab...
Source/core/css/CSSVariablesMap.cpp:81: RefPtr<CSSVariablesIterator> iterator =
m_styleDeclaration->variablesIterator();
On 2013/09/12 01:13:42, esprehn wrote:
> Is the iterator really necessary? It'd be much nicer if you used a normal C++
> iterator.

After the changes to remove the static map of iterators from this patch I needed
to add some methods to the iterator class. I think this prevents me from using a
regular C++ iterator interface.

https://codereview.chromium.org/21006006/diff/87001/Source/core/css/CSSVariab...
File Source/core/css/CSSVariablesMap.h (right):

https://codereview.chromium.org/21006006/diff/87001/Source/core/css/CSSVariab...
Source/core/css/CSSVariablesMap.h:57: void
forEach(PassRefPtr<CSSVariablesMapForEachCallback>, ScriptValue thisArg) const;
On 2013/09/12 01:13:42, esprehn wrote:
> When do we pass a ScriptValue by value instead of pointer or reference?

Made pointer overload private.
Changed copy parameter to be a reference.

https://codereview.chromium.org/21006006/diff/87001/Source/core/css/StyleProp...
File Source/core/css/StylePropertySet.cpp (right):

https://codereview.chromium.org/21006006/diff/87001/Source/core/css/StyleProp...
Source/core/css/StylePropertySet.cpp:660:
DEFINE_STATIC_LOCAL(VariablesIteratorsMap, map, ());
On 2013/09/12 01:13:42, esprehn wrote:
> This is scary, I'd rather we didn't have any static global maps.

Moved the vector of CSSVariablesIterators to the CSSVariablesMap class, removed
the static map.

[alancutter (OOO until 2018), 2013-09-19 03:59:08]

esprehn: Poke.

[esprehn, 2013-09-28 00:44:36]

lgtm. In general you shouldn't access private vars like this, your constructor
should take the value and you should build it externally and then pass it in or
have a method like take() that does swap() internally.

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

https://codereview.chromium.org/21006006/diff/108001/Source/bindings/scripts/...
Source/bindings/scripts/v8_utilities.py:97: return re.split('[|&]',
values_string)
This will compile the regex repeatedly...

https://codereview.chromium.org/21006006/diff/108001/Source/core/css/StylePro...
File Source/core/css/StylePropertySet.cpp (right):

https://codereview.chromium.org/21006006/diff/108001/Source/core/css/StylePro...
Source/core/css/StylePropertySet.cpp:569: RefPtr<VariablesIterator> iterator =
adoptRef(new VariablesIterator(propertySet));
Instead of all this accessing of private vars like m_remainingNames you should
put a Vector on the stack here and then fill it and VariablesIterator's
constructor should take it.

https://codereview.chromium.org/21006006/diff/108001/Source/core/css/StylePro...
Source/core/css/StylePropertySet.cpp:573: for (int i = propertyCount; i--;) {
missing space after ;

[alancutter (OOO until 2018), 2013-09-30 02:54:51]

Thanks for reviewing esprehn. :)

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

https://codereview.chromium.org/21006006/diff/108001/Source/bindings/scripts/...
Source/bindings/scripts/v8_utilities.py:97: return re.split('[|&]',
values_string)
On 2013/09/28 00:44:36, esprehn wrote:
> This will compile the regex repeatedly...

Moved to precompiled module variable.

https://codereview.chromium.org/21006006/diff/108001/Source/core/css/StylePro...
File Source/core/css/StylePropertySet.cpp (right):

https://codereview.chromium.org/21006006/diff/108001/Source/core/css/StylePro...
Source/core/css/StylePropertySet.cpp:569: RefPtr<VariablesIterator> iterator =
adoptRef(new VariablesIterator(propertySet));
On 2013/09/28 00:44:36, esprehn wrote:
> Instead of all this accessing of private vars like m_remainingNames you should
> put a Vector on the stack here and then fill it and VariablesIterator's
> constructor should take it.

Done.

https://codereview.chromium.org/21006006/diff/108001/Source/core/css/StylePro...
Source/core/css/StylePropertySet.cpp:573: for (int i = propertyCount; i--;) {
On 2013/09/28 00:44:36, esprehn wrote:
> missing space after ;

Done.

[commit-bot: I haz the power, 2013-09-30 07:48:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alancutter@chromium.org/21006006/115001

[commit-bot: I haz the power, 2013-09-30 08:47:21]

Change committed as 158524