Create fewer local v8::TryCatch objects in generated bindings code

Create fewer local v8::TryCatch objects in generated bindings code

When converting constructor and method arguments, switch from generating
the equivalent of

float x;
{
　　v8::TryCatch block;
　　x = ...;
　　if (UNLIKELY(block.HasCaught()) {
　　　　block.ReThrow();
　　　　return;
　　}
}
float y;
{
　　v8::TryCatch block;
　　y = ...;
　　/* check result again, same as above */
}

to generating the equivalent of

float x;
float y;
{
　　v8::TryCatch block;
　　x = ...;
　　if (UNLIKELY(block.HasCaught()) {
　　　　block.ReThrow();
　　　　return;
　　}
　　y = ...;
　　/* check result again, same as above */
}

in order to avoid creating and destroying one v8::TryCatch object for each
argument.

This reduces the size of the final binary by about 20 kB (64-bit Linux) and
improves performance by up to 30 % on favorable micro-benchmarks (repeatedly
calling some DOM function that takes many primitive-typed arguments.)

R=haraken, nbarth

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

[Jens Widell, 2014-05-05 15:00:30]

Please have a look, when convenient.

Background:

Was looking at the generated code, trying to spot any improvements that might
make it smaller, to reduce code footprint.  This patch was really just meant as
an experiment, to see what the code footprint cost of many local v8::TryCatch
object might be.

Result:

There doesn't seem to be a lot to be gained here, from a code footprint POV. 
Not anything warranting a rather complicated change like this, anyway.  :-)

From a performance POV however, this might actually be worthwhile.  The rather
pointless micro-benchmark

  function bench(context2d) {
    for (var i = 0; i < 10000000; ++i)
      context2d.ellipse(0, 0, 0, 0, 0, 0, 0, 0, 0, false);
  }

for instance runs a bit over 30 % faster with these changes.

[haraken, 2014-05-05 15:10:01]

The idea looks great to me. nbarth@, would you mind taking a first look?

[Jens Widell, 2014-05-06 09:15:16]

On 2014/05/05 15:00:30, Jens Lindström wrote:
> Please have a look, when convenient.

And, while you're here, maybe answer a question: what's the reason behind
handling constructor arguments separately from regular method arguments?  IOW,
why the constructor_argument() function in v8_interface.py, which looks like a
simplified copy of the generate_argument() function in v8_methods.py?

While trying to make this patch work, I had to copy some of
generate_argument()'s functionality in constructor_argument(), which felt a bit
unnecessary.

[Nils Barth (inactive), 2014-05-07 03:02:19]

Thanks Jens, idea and most code seems great!

haraken: performance impact likely to be more significant in future with more
type checking, right?

I don't think code complexity is a problem -- this is mostly just refactoring.

A few mostly stylistic points; substance is fine.

https://codereview.chromium.org/265293004/diff/1/Source/bindings/scripts/v8_i...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/265293004/diff/1/Source/bindings/scripts/v8_i...
Source/bindings/scripts/v8_interface.py:474:
v8_methods.argument_needs_try_catch(argument) for argument in
generated_arguments),
Could you add one more linebreak here?
(So for ... on separate line.)

https://codereview.chromium.org/265293004/diff/1/Source/bindings/scripts/v8_m...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/265293004/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:44: return not
(argument.get("is_callback_interface") or
Single quotes.
Also, can't these just be usual key access?
(No need for dict.get() that I can see; if there is, it's preferred to make sure
these keys are always created, to simplify these functions.)

https://codereview.chromium.org/265293004/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:94: 'arguments_need_try_catch': any(
Fix linebreaks, wrapping as follows:
        'arguments_need_try_catch': any(argument_needs_try_catch(argument)
                                        for argument in generated_arguments),

https://codereview.chromium.org/265293004/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:156: if argument.is_variadic and not
is_variadic_wrapper_type:
Slightly more direct:
if argument.is_variadic and not idl_typ.is_wrapper_type:

https://codereview.chromium.org/265293004/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:157: cpp_type =
idl_type.cpp_type_args(extended_attributes=extended_attributes,
Prefer single assignment:
element_cpp_type = ...

cpp_type = v8_types.cpp_template_type(vector_type, element_cpp_type)

https://codereview.chromium.org/265293004/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:158:
used_in_cpp_sequence=is_variadic_wrapper_type)
You can drop used_in_cpp_sequence, as it's always False, which is default.

https://codereview.chromium.org/265293004/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:162: cpp_type =
idl_type.cpp_type_args(extended_attributes=extended_attributes,
Do we need to pass extended_attributes here?

(It's only needed in rare cases generally, so we've been omitting it for
simplicity and to assert that this *isn't* happening here.)

https://codereview.chromium.org/265293004/diff/1/Source/bindings/scripts/v8_t...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/265293004/diff/1/Source/bindings/scripts/v8_t...
Source/bindings/scripts/v8_types.py:449: if not declare_variable:
I prefer to avoid 'not' in if...else, so:
if declare_variable:
     args.insert
else:
    macro += ...

https://codereview.chromium.org/265293004/diff/1/Source/bindings/scripts/v8_t...
Source/bindings/scripts/v8_types.py:451: macro += "_NO_DECL"
...and single quotes.

https://codereview.chromium.org/265293004/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/265293004/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:75:
{######################################}
Could you rename this as
generate_argument_var_declaration
(or, if you prefer, *var_definition, which is slightly more correct)
...to make it explicit?

https://codereview.chromium.org/265293004/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:76: {% macro
generate_argument_var(argument) %}
This is a one-line macro, so please put the ; at the call site and strip
trailing newlines (using {%- ).

This is a bit confusing, so it's detailed at the style guide:
http://dev.chromium.org/developers/jinja
at:
Treat one-line macros as in-line (no indent, {%- to strip trailing linebreak,
semicolon ; at call site)
http://dev.chromium.org/developers/jinja#TOC-Treat-one-line-macros-as-in-line...

https://codereview.chromium.org/265293004/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:214: {% macro generate_arguments(method,
world_suffix) %}
Could you move this *up* above the generate_argument_var and generate_argument
macros?
(Order here is top-down.)

https://codereview.chromium.org/265293004/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:216: {{generate_argument_var(argument) |
indent}}
No |indent|, trailing ; here.

https://codereview.chromium.org/265293004/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:444: {% if constructor.arguments %}
Could you move this {% if %} test into the generate_arguments macro?
(Removes lots of duplication and simplifies call.)

[haraken, 2014-05-07 03:06:14]

> haraken: performance impact likely to be more significant in future with more
> type checking, right?

Yes, I think so.

[Nils Barth (inactive), 2014-05-07 03:20:15]

On 2014/05/06 09:15:16, Jens Lindström wrote:
> And, while you're here, maybe answer a question: what's the reason behind
> handling constructor arguments separately from regular method arguments?
> IOW, why the constructor_argument() function in v8_interface.py, which looks
like
> a simplified copy of the generate_argument() function in v8_methods.py?

Legacy cruft, thanks for noting - removed!
Remove v8_interface.constructor_argument
https://codereview.chromium.org/274503002/

Historically constructors (at least argument handling) *were* just treated as a
simple case of methods, as reflected in the code.

In fact there's no need to make a distinction (we can treat them identically),
so it's better to merge them - thanks!

[Daniel Bratell, 2014-05-07 09:06:52]

On 2014/05/05 15:00:30, Jens Lindström wrote:
> Please have a look, when convenient.
> 
> Background:
> 
> Was looking at the generated code, trying to spot any improvements that might
> make it smaller, to reduce code footprint.  This patch was really just meant
as
> an experiment, to see what the code footprint cost of many local v8::TryCatch
> object might be.

I checked to see why there was no footprint reduction (however small) from this
patch and it seems it accidentally adds some try-catch blocks for functions with
string arguments. After that is fixed, there should be a (small) size
improvement as well.

[Jens Widell, 2014-05-07 10:34:46]

Thanks for reviewing, Nils!

I've uploaded three new patch sets.  The first addresses most of your comments,
the second is a rebase to get your removal of constructor_argument() and the
third fixes the issue Daniel pointed out, where functions with only string
arguments had an unused v8::TryCatch object created.

I think those unused v8::TryCatch objects happened to cancel out most of the
code footprint gain from this patch. With that fixed, I'm now seeing at least
20-something kilobytes of savings. Fixing it ought to also fix a slight
performance regression for the functions in question.

https://codereview.chromium.org/265293004/diff/1/Source/bindings/scripts/v8_m...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/265293004/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:44: return not
(argument.get("is_callback_interface") or
On 2014/05/07 03:02:19, Nils Barth wrote:
> Single quotes.
> Also, can't these just be usual key access?
> (No need for dict.get() that I can see; if there is, it's preferred to make
sure
> these keys are always created, to simplify these functions.)

Neither 'is_callback_interface' nor 'is_variadic_wrapper_type' was set by
constructor_argument(), and I used dict.get() for 'idl_type' too for
consistency, I guess.  But since you removed constructor_argument(), there's no
longer any need for dict.get().

https://codereview.chromium.org/265293004/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:162: cpp_type =
idl_type.cpp_type_args(extended_attributes=extended_attributes,
On 2014/05/07 03:02:19, Nils Barth wrote:
> Do we need to pass extended_attributes here?
> 
> (It's only needed in rare cases generally, so we've been omitting it for
> simplicity and to assert that this *isn't* happening here.)

Not passing it makes run-bindings-test fail with this error:
http://pastebin.com/MtuKxRL4

https://codereview.chromium.org/265293004/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/265293004/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:76: {% macro
generate_argument_var(argument) %}
On 2014/05/07 03:02:19, Nils Barth wrote:
> This is a one-line macro, so please put the ; at the call site and strip
> trailing newlines (using {%- ).
> 
> This is a bit confusing, so it's detailed at the style guide:
> http://dev.chromium.org/developers/jinja
> at:
> Treat one-line macros as in-line (no indent, {%- to strip trailing linebreak,
> semicolon ; at call site)
>
http://dev.chromium.org/developers/jinja#TOC-Treat-one-line-macros-as-in-line...

Thanks for the pointers! The white-space handling was a bit confusing,
especially before I discovered the {%- modifier.

https://codereview.chromium.org/265293004/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:444: {% if constructor.arguments %}
On 2014/05/07 03:02:19, Nils Barth wrote:
> Could you move this {% if %} test into the generate_arguments macro?
> (Removes lots of duplication and simplifies call.)

Would love to. One problem: I can't seem to figure out how to make it not
produce an empty line containing only four spaces for methods with no arguments.
Help? :-)

[Nils Barth (inactive), 2014-05-08 00:55:13]

LGTM CG-wise, with nits!
Jens, could you summarize benefits (code size and performance) in the
description?

haraken, could you take a look at it once Jens summarizes the effect?

A few minor nits, with one whitespace idea.

Anyway, excellent CL, and pleasure to meet you Jens!

https://codereview.chromium.org/265293004/diff/1/Source/bindings/scripts/v8_m...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/265293004/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:44: return not
(argument.get("is_callback_interface") or
On 2014/05/07 10:34:46, Jens Lindström wrote:
> Neither 'is_callback_interface' nor 'is_variadic_wrapper_type' was set by
> constructor_argument(), and I used dict.get() for 'idl_type' too for
> consistency, I guess.  But since you removed constructor_argument(), there's
no
> longer any need for dict.get().

Got it -- I thought it might be something like that;
glad it's slicker now!

https://codereview.chromium.org/265293004/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:162: cpp_type =
idl_type.cpp_type_args(extended_attributes=extended_attributes,
On 2014/05/07 10:34:46, Jens Lindström wrote:
> On 2014/05/07 03:02:19, Nils Barth wrote:
> > Do we need to pass extended_attributes here?
> > 
> > (It's only needed in rare cases generally, so we've been omitting it for
> > simplicity and to assert that this *isn't* happening here.)
> 
> Not passing it makes run-bindings-test fail with this error:
> http://pastebin.com/MtuKxRL4

Got it, looks like there is a use in constructors;
thanks for checking!

https://codereview.chromium.org/265293004/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/265293004/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:76: {% macro
generate_argument_var(argument) %}
On 2014/05/07 10:34:46, Jens Lindström wrote:
> Thanks for the pointers! The white-space handling was a bit confusing,
> especially before I discovered the {%- modifier.

Jinja whitespace handling was the bane of my life for a while (>.<)
...so I'd like to save everyone else the pain.

https://codereview.chromium.org/265293004/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:444: {% if constructor.arguments %}
On 2014/05/07 10:34:46, Jens Lindström wrote:
> On 2014/05/07 03:02:19, Nils Barth wrote:
> > Could you move this {% if %} test into the generate_arguments macro?
> > (Removes lots of duplication and simplifies call.)
> 
> Would love to. One problem: I can't seem to figure out how to make it not
> produce an empty line containing only four spaces for methods with no
arguments.
> Help? :-)

Oh, good point (>.<)
That's exactly the trickiest point when using macros.
Could you try trimming trailing whitespace with -}} ?
(There are other hacky solutions, like putting the line break *before* the }} or
stripping leading whitespace before.)

If you can't get it working with a bit of fiddling, it's no problem;
this is exactly where it stops being worth worrying.

https://codereview.chromium.org/265293004/diff/100001/Source/bindings/scripts...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/265293004/diff/100001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:43: def
argument_needs_try_catch(argument):
I would tend to write this as a single boolean, as in:
return not(# These cases ...
           ... or
           ... or
           ... or
           # String and enumeration ...
           ...)

https://codereview.chromium.org/265293004/diff/100001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:53: if
argument['v8_value_to_local_cpp_value'].startswith('TOSTRING_'):
How about checking for DOMString directly, as in:
if argument['idl_type'].preprocessed_type.name == 'String':
?

[haraken, 2014-05-08 03:45:05]

This is great CL. LGTM with comments.

https://codereview.chromium.org/265293004/diff/100001/Source/bindings/scripts...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/265293004/diff/100001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:48: argument['is_variadic_wrapper_type']):

I wonder why this condition isn't something like
"argument.idl_type.is_wrapper_type()".

Did you confirm that the condition in which the TryCatch block is generated is
exactly the same after this CL?

https://codereview.chromium.org/265293004/diff/100001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:166: if argument.is_variadic and not
idl_type.is_wrapper_type:

Would you elaborate on why we need this branch? I don't quite understand why we
need to add the complexity.

https://codereview.chromium.org/265293004/diff/100001/Source/bindings/v8/V8Bi...
File Source/bindings/v8/V8BindingMacros.h (right):

https://codereview.chromium.org/265293004/diff/100001/Source/bindings/v8/V8Bi...
Source/bindings/v8/V8BindingMacros.h:49: #define TONATIVE_VOID_NO_DECL(var,
value) \

Nit: _NO_DECL => _INTERNAL ?

It's dangerous to allow people to use this macro unless they understand what
they're doing.

https://codereview.chromium.org/265293004/diff/100001/Source/bindings/v8/V8Bi...
Source/bindings/v8/V8BindingMacros.h:104: if (UNLIKELY(!var.prepare()))       \

Can we use TOSTRING_VOID_NO_DECL here?

https://codereview.chromium.org/265293004/diff/100001/Source/bindings/v8/V8St...
File Source/bindings/v8/V8StringResource.h (right):

https://codereview.chromium.org/265293004/diff/100001/Source/bindings/v8/V8St...
Source/bindings/v8/V8StringResource.h:183: void operator= (v8::Handle<v8::Value>
object)

Nit: Unnecessary space after '='.

[Jens Widell, 2014-05-08 06:22:05]

https://codereview.chromium.org/265293004/diff/100001/Source/bindings/scripts...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/265293004/diff/100001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:48: argument['is_variadic_wrapper_type']):
On 2014/05/08 03:45:06, haraken wrote:
> 
> I wonder why this condition isn't something like
> "argument.idl_type.is_wrapper_type()".

The 'argument' here is the dictionary returned by generate_argument(), rather
than the IDLAttribute object. I implemented this function that way to be able to
use exactly the same conditions as in the methods.cpp template code for when no
TryCatch is needed.

> Did you confirm that the condition in which the TryCatch block is generated is
> exactly the same after this CL?

I tried to. I've reviewed the changes to the (generated) bindings tests code,
and couldn't spot any TryCatch being declared and not used. Not after fixing the
bug that we generated them too often, that is.

If we don't generate a TryCatch when it's needed we break compilation, so that
will at least be very obvious. It doesn't happen currently. If we do generate a
TryCatch when it's not needed, it'll cost a bit in both performance and
footprint, and should be fixed, but overall I'm quite sure this patch does
reduce the number of declared TryCatch objects, and should thus be an overall
win.

https://codereview.chromium.org/265293004/diff/100001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:53: if
argument['v8_value_to_local_cpp_value'].startswith('TOSTRING_'):
On 2014/05/08 00:55:13, Nils Barth wrote:
> How about checking for DOMString directly, as in:
> if argument['idl_type'].preprocessed_type.name == 'String':
> ?

I did try something similar (looking just at argument['idl_type'] IIRC) at
first, but it failed for variadic (string) arguments, which are handled by a
special code path in v8_methods.v8_value_to_local_cpp_value() that generates

  TONATIVE_VOID_NO_DECL({name}, toNativeArguments<{cpp_type}>(info, {index}))

I ended up with this simple/low-level approach instead. The case is that the
template in methods.cpp ends up just emitting
{{argument.v8_value_to_local_cpp_value}}, and in that case it's specifically
TOSTRING_* that doesn't make use of a TryCatch.

https://codereview.chromium.org/265293004/diff/100001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:166: if argument.is_variadic and not
idl_type.is_wrapper_type:
On 2014/05/08 03:45:06, haraken wrote:
> 
> Would you elaborate on why we need this branch? I don't quite understand why
we
> need to add the complexity.

I added it to get argument['cpp_type'] correct. Prior to this patch, the
template macro in methods.cpp only used that value sometimes, and it wasn't
always correct for argument kinds that didn't use it. It was specifically not
used when the template macro used argument['v8_value_to_local_cpp_value']
instead, which prior to this patch included the variable declaration.

But I don't remember exactly why I added this branch, and I'm not sure it is
actually needed. I'll revisit it and either report here why I believe it is
needed, or simplify the code.

https://codereview.chromium.org/265293004/diff/100001/Source/bindings/v8/V8Bi...
File Source/bindings/v8/V8BindingMacros.h (right):

https://codereview.chromium.org/265293004/diff/100001/Source/bindings/v8/V8Bi...
Source/bindings/v8/V8BindingMacros.h:104: if (UNLIKELY(!var.prepare()))       \
On 2014/05/08 03:45:06, haraken wrote:
> 
> Can we use TOSTRING_VOID_NO_DECL here?

Make this

  type var;
  TOSTRING_VOID_NO_DECL(var, value);

you mean? Yes, that would work.

We've seen some indication that the splitting the declaration and
initialization/assignment of V8StringResource<> objects might lead to a small
increase in generated code, though. I'd consider it an acceptable cost in the
method case, but changing this macro that way might add the same cost to all
string attribute setters.

[Nils Barth (inactive), 2014-05-08 06:25:15]

https://codereview.chromium.org/265293004/diff/100001/Source/bindings/scripts...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/265293004/diff/100001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:53: if
argument['v8_value_to_local_cpp_value'].startswith('TOSTRING_'):
On 2014/05/08 06:22:06, Jens Lindström wrote:
> On 2014/05/08 00:55:13, Nils Barth wrote:
> > How about checking for DOMString directly, as in:
> > if argument['idl_type'].preprocessed_type.name == 'String':
> > ?
> 
> I did try something similar (looking just at argument['idl_type'] IIRC) at
> first, but it failed for variadic (string) arguments, which are handled by a
> special code path in v8_methods.v8_value_to_local_cpp_value() that generates
> 
>   TONATIVE_VOID_NO_DECL({name}, toNativeArguments<{cpp_type}>(info, {index}))
> 
> I ended up with this simple/low-level approach instead. The case is that the
> template in methods.cpp ends up just emitting
> {{argument.v8_value_to_local_cpp_value}}, and in that case it's specifically
> TOSTRING_* that doesn't make use of a TryCatch.

Got it, thanks for thoughtful reply.
Yup, fine to do this to handle variadics, and good point about it really being
tied to the macro!

[Jens Widell, 2014-05-08 07:00:15]

https://codereview.chromium.org/265293004/diff/100001/Source/bindings/scripts...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/265293004/diff/100001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:43: def
argument_needs_try_catch(argument):
On 2014/05/08 00:55:13, Nils Barth wrote:
> I would tend to write this as a single boolean, as in:

And I tend to split conditions into multiple statements by the time I feel it
necessary to document the individual sub-expressions. :-)

I'll convert this back into a single expression, keeping the comments.

[Jens Widell, 2014-05-08 16:38:05]

https://codereview.chromium.org/265293004/diff/100001/Source/bindings/scripts...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/265293004/diff/100001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:43: def
argument_needs_try_catch(argument):
On 2014/05/08 07:00:15, Jens Lindström wrote:
> On 2014/05/08 00:55:13, Nils Barth wrote:
> > I would tend to write this as a single boolean, as in:
> 
> And I tend to split conditions into multiple statements by the time I feel it
> necessary to document the individual sub-expressions. :-)
> 
> I'll convert this back into a single expression, keeping the comments.

Updated now. I formatted it

    return not (
        # These cases ...
        ... or
        ... or
        ... or
        # String and enumeration ...
        ...)

instead, since check-webkit-style rejected your suggested formatting; "at least
two spaces before inline comment  [pep8/E261] [5]".

https://codereview.chromium.org/265293004/diff/100001/Source/bindings/v8/V8Bi...
File Source/bindings/v8/V8BindingMacros.h (right):

https://codereview.chromium.org/265293004/diff/100001/Source/bindings/v8/V8Bi...
Source/bindings/v8/V8BindingMacros.h:49: #define TONATIVE_VOID_NO_DECL(var,
value) \
On 2014/05/08 03:45:06, haraken wrote:
> 
> Nit: _NO_DECL => _INTERNAL ?
> 
> It's dangerous to allow people to use this macro unless they understand what
> they're doing.

Renamed.

https://codereview.chromium.org/265293004/diff/100001/Source/bindings/v8/V8St...
File Source/bindings/v8/V8StringResource.h (right):

https://codereview.chromium.org/265293004/diff/100001/Source/bindings/v8/V8St...
Source/bindings/v8/V8StringResource.h:183: void operator= (v8::Handle<v8::Value>
object)
On 2014/05/08 03:45:06, haraken wrote:
> 
> Nit: Unnecessary space after '='.

Fixed.

[Jens Widell, 2014-05-08 16:53:05]

PTAL. The change in latest patch set is a bit non-trivial.

https://codereview.chromium.org/265293004/diff/100001/Source/bindings/scripts...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/265293004/diff/100001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:166: if argument.is_variadic and not
idl_type.is_wrapper_type:
On 2014/05/08 06:22:06, Jens Lindström wrote:
> On 2014/05/08 03:45:06, haraken wrote:
> > 
> > Would you elaborate on why we need this branch? I don't quite understand why
> we
> > need to add the complexity.

> But I don't remember exactly why I added this branch, and I'm not sure it is
> actually needed. I'll revisit it and either report here why I believe it is
> needed, or simplify the code.

So, the problem was variadic arguments, again. :-) Neither the original code,
nor any one of the two branches in my modified code, generated the correct
'cpp_type' in all cases. (The original code obviously did in all the cases where
'cpp_type' was used then.) I've redesigned it a bit now, in a way that I think
is better.

[Nils Barth (inactive), 2014-05-09 00:46:07]

CG LGTM - revised logic is v. logical.

https://codereview.chromium.org/265293004/diff/140001/Source/bindings/scripts...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/265293004/diff/140001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:275: index=index)
You can wrap this line too now.

[haraken, 2014-05-09 00:53:57]

LGTM. Looks nicer!

[Jens Widell, 2014-05-09 06:55:45]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-05-09 06:58:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jl@opera.com/265293004/180001

[commit-bot: I haz the power, 2014-05-09 08:21:21]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/7048)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6792)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/6298)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/6766)

[Jens Widell, 2014-05-09 08:36:56]

On 2014/05/09 08:21:21, I haz the power (commit-bot) wrote:
> FYI, CQ is re-trying this CL (attempt #1).
> Please consider checking whether the failures are real,
> and report flakes to mailto:chrome-troopers@google.com.
> The failing builders are:
>   linux_blink_dbg on tryserver.blink
>
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/7048)
>   linux_blink_rel on tryserver.blink
>
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6792)
>   mac_blink_rel on tryserver.blink
>
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/6298)
>   win_blink_rel on tryserver.blink
>
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/6766)

Failure is, if I'm not mistaken, due to the "new" v8::TryCatch being in scope,
and catching, exceptions thrown using throwTypeError(). I thought I had tested
that to make sure it didn't and that those exceptions were still thrown, but it
would seem I tested something else, or that something has changed.

Adding a "block.ReThrow()" call after each throwTypeError() call would fix it
(fixed the failing test, at least) but is a bit sad. Other suggestions, anyone?

[commit-bot: I haz the power, 2014-05-09 08:43:02]

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

[commit-bot: I haz the power, 2014-05-09 08:43:02]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/6298)

[Jens Widell, 2014-05-09 10:42:31]

On 2014/05/09 08:36:56, Jens Lindström wrote:
> Failure is, if I'm not mistaken, due to the "new" v8::TryCatch being in scope,
> and catching, exceptions thrown using throwTypeError(). I thought I had tested
> that to make sure it didn't and that those exceptions were still thrown, but
it
> would seem I tested something else, or that something has changed.

Actually, I don't understand what is going on here.

In createLinearGradientMethod() in V8CanvasRenderingContext2D.cpp the exception
thrown if the first argument is Infinity is propagated just fine, even though
block.HasCaught() returns true after the call to throwTypeError(). Not even
calling block.Reset() makes the exception go away.

In checkIntersectionMethod() in V8SVGSVGElement.cpp the exception thrown if the
first argument isn't an SVGElement gets lost. In this case block.HasCaught()
also returns true after the call to throwTypeError(), and calling
block.ReThrow() makes the exception reappear.

What am I missing?

[Jens Widell, 2014-05-09 11:55:30]

On 2014/05/09 10:42:31, Jens Lindström wrote:
> What am I missing?

Alright, it seems the difference isn't in how those two methods behave. The
difference is apparently that the calling script can catch the exception with a
try{}catch(){}, but that the exception doesn't show up as an uncaught exception
if the calling script doesn't. I'm assuming the correct thing to do is to call
block.ReThrow() before returning whenever an exception is thrown.

And the really odd thing is that adding those 276 block.ReThrow() calls somehow
*reduces* code footprint by 48 bytes. :-) (But I guess that might be because
many function already had an exit branch that called block.ReThrow() just before
returning, meaning adding this call at some additional exit branches actually
made it possible for the compiler to reuse more code.)

[Jens Widell, 2014-05-09 12:01:22]

haraken, nbarth: would you mind having yet another look at this?  Very sorry
about the noise...

[Nils Barth (inactive), 2014-05-09 12:07:01]

On 2014/05/09 11:55:30, Jens Lindström wrote:
> I'm assuming the correct thing to do is to call
> block.ReThrow() before returning whenever an exception is thrown.

That sounds right!
haraken?

Also, would it be possible for you to document this somewhere?
(This is clearly quite confusing.)
haraken: What's the right place to document this?
(Is this V8 or bindings?)

[Nils Barth (inactive), 2014-05-09 12:07:42]

On 2014/05/09 12:01:22, Jens Lindström wrote:
> haraken, nbarth: would you mind having yet another look at this?  Very sorry
> about the noise...

No problem; you hit a subtle point.
haraken, I'm not sure how this works: could you take a look at Jens's recent
notes?

[Jens Widell, 2014-05-09 13:18:14]

On 2014/05/09 12:07:01, Nils Barth wrote:
> Also, would it be possible for you to document this somewhere?
> (This is clearly quite confusing.)
> haraken: What's the right place to document this?
> (Is this V8 or bindings?)

I would have wanted TryCatch's documentation in v8.h to say something, if it is
indeed a requirement to call TryCatch::ReThrow() if Isolate::ThrowException() is
used. But honestly it sort of feels like a bug somewhere in the interaction
between TryCatch and Isolate::ThrowException. The observable behavior makes no
sense. (Maybe Isolate::ThrowException() is illegal with a TryCatch in scope?
That would obviously be a problem for this patch, if so.)

[Jens Widell, 2014-05-09 13:42:09]

The ReThrow() issues I was seeing seem related to
https://code.google.com/p/chromium/issues/detail?id=362388.

https://codereview.chromium.org/265293004/diff/200001/Source/bindings/v8/V8Bi...
File Source/bindings/v8/V8BindingMacros.h (right):

https://codereview.chromium.org/265293004/diff/200001/Source/bindings/v8/V8Bi...
Source/bindings/v8/V8BindingMacros.h:110: return;
It seems we need a "block.ReThrow()" here as well. But this macro can't safely
assume that there is a 'block' declared, so we'd need two macros or a macro that
allows us to do something before the return.

[haraken, 2014-05-09 14:07:43]

I'm sorry I haven't caught up the discussion, but v8::TryCatch is not nestable
due to a V8 bug (https://code.google.com/p/v8/issues/detail?id=2166). Is it
related?

[Jens Widell, 2014-05-09 14:18:12]

On 2014/05/09 14:07:43, haraken wrote:
> I'm sorry I haven't caught up the discussion, but v8::TryCatch is not nestable
> due to a V8 bug (https://code.google.com/p/v8/issues/detail?id=2166). Is it
> related?

It seems related. In the string conversion case, we do have nested TryCatch
objects now. But the situation in the bindings code might be such that things
can work despite the V8 bugs; we don't want to actually catch any exceptions, we
always propagate them, and in the string conversion case, we don't rely on the
outer TryCatch seeing the exception. Though it seems we need to call ReThrow()
on the outer TryCatch for the exception to propagate correctly, which must mean
that the outer TryCatch sees the exception too.

[haraken, 2014-05-09 14:20:31]

On 2014/05/09 14:18:12, Jens Lindström wrote:
> On 2014/05/09 14:07:43, haraken wrote:
> > I'm sorry I haven't caught up the discussion, but v8::TryCatch is not
nestable
> > due to a V8 bug (https://code.google.com/p/v8/issues/detail?id=2166). Is it
> > related?
> 
> It seems related. In the string conversion case, we do have nested TryCatch
> objects now. But the situation in the bindings code might be such that things
> can work despite the V8 bugs; we don't want to actually catch any exceptions,
we
> always propagate them, and in the string conversion case, we don't rely on the
> outer TryCatch seeing the exception. Though it seems we need to call ReThrow()
> on the outer TryCatch for the exception to propagate correctly, which must
mean
> that the outer TryCatch sees the exception too.

I'll be in Munich next week, so I'll chat with V8 guys about a plan for fixing
the V8 bug 2166.

[Jens Widell, 2014-05-09 14:28:32]

On 2014/05/09 14:20:31, haraken wrote:
> I'll be in Munich next week, so I'll chat with V8 guys about a plan for fixing
> the V8 bug 2166.

And I'll be in Zürich on BlinkOn 2, and it doesn't feel like I'll manage to land
this today anyway.

The TOSTRING_* case needs to be adjusted for propagation to work, and it's a bit
problematic since in v8_types.v8_value_to_local_cpp_value(), where we generate
the TOSTRING_VOID() expression, we don't know if there is an outer v8::TryCatch
declared that we should call ReThrow() on.

Don't know what the best approach here is. Passing a reference to the outer
TryCatch in to V8StringResource::prepare() would eliminate the nested TryCatch
issue, but I don't know if that's the best approach. (And there's also the issue
that we don't know if we have an outer TryCatch.)

Seems the devil really is in the details...

[haraken, 2014-05-09 14:33:52]

On 2014/05/09 14:28:32, Jens Lindström wrote:
> On 2014/05/09 14:20:31, haraken wrote:
> > I'll be in Munich next week, so I'll chat with V8 guys about a plan for
fixing
> > the V8 bug 2166.
> 
> And I'll be in Zürich on BlinkOn 2, and it doesn't feel like I'll manage to
land
> this today anyway.

Great, I'll be in Zurich and then in Munich. Let's chat f2f then!

[Jens Widell, 2014-05-19 10:41:12]

Patch rebased (PS10, no conflicts) and then updated (PS11) to call
block.ReThrow() after V8StringResource::prepare() when there's a local
v8::TryCatch. This fixes exception propagation when a toString() function throws
an exception while converting a string argument.

Things got a bit hairy since use of TOSTRING_VOID_INTERNAL() itself doesn't
trigger the declaration of a v8::TryCatch, and it thus can't assume one has been
declared. So I introduced another macro variant,
TOSTRING_VOID_INTERNAL_RETHROW(), that is used when there is a v8::TryCatch.
Whether there is a v8::TryCatch or not depends on all the arguments, so I had to
change the code that calculates that to look at IdlArgument object instead of
the dictionaries returned by generate_argument(), since generate_argument() now
needs to know.

This is not super elegant, and I'm not super happy about it. A simpler fix would
of course have been to always declare a v8::TryCatch when
TOSTRING_VOID_INTERNAL() was used, but that would have increased binary
footprint and reduced performance for any method/constructor that takes only
string arguments.

[haraken, 2014-05-20 06:46:06]

On 2014/05/19 10:41:12, Jens Lindström wrote:
> Patch rebased (PS10, no conflicts) and then updated (PS11) to call
> block.ReThrow() after V8StringResource::prepare() when there's a local
> v8::TryCatch. This fixes exception propagation when a toString() function
throws
> an exception while converting a string argument.
> 
> Things got a bit hairy since use of TOSTRING_VOID_INTERNAL() itself doesn't
> trigger the declaration of a v8::TryCatch, and it thus can't assume one has
been
> declared. So I introduced another macro variant,
> TOSTRING_VOID_INTERNAL_RETHROW(), that is used when there is a v8::TryCatch.
> Whether there is a v8::TryCatch or not depends on all the arguments, so I had
to
> change the code that calculates that to look at IdlArgument object instead of
> the dictionaries returned by generate_argument(), since generate_argument()
now
> needs to know.
> 
> This is not super elegant, and I'm not super happy about it. A simpler fix
would
> of course have been to always declare a v8::TryCatch when
> TOSTRING_VOID_INTERNAL() was used, but that would have increased binary
> footprint and reduced performance for any method/constructor that takes only
> string arguments.

Thanks for the confirmation.

Yeah, in terms of programmability, we want to always declare a v8::TypeCatch
when TOSTRING_VOID_INTERNAL() is used.

I'm not sure but applying this CL (https://codereview.chromium.org/242913002/)
will make the situation better (or worse)?

[Jens Widell, 2014-05-20 06:55:28]

On 2014/05/20 06:46:06, haraken wrote:
> Yeah, in terms of programmability, we want to always declare a v8::TypeCatch
> when TOSTRING_VOID_INTERNAL() is used.
>
> I'm not sure but applying this CL (https://codereview.chromium.org/242913002/)
> will make the situation better (or worse)?

It's a trade-off, I guess. Declaring one where there wasn't one before increases
code footprint. Having a single TryCatch in V8StringResource::prepare() gives
less code. (Unless prepare() is inlined by the compiler, of course, which I
guess it might be.)

With 242913002, we could use a prepare() that doesn't catch and always have the
TryCatch in the generated bindings code, in which case TOSTRING_VOID_INTERNAL()
would be essentially the same as TONATIVE_VOID_INTERNAL(). Performance-wise this
ought to be better for all cases where both TOSTRING_* and TONATIVE_* are use
within the same method, since we'd reduce the number of initialized TryCatch
objects.

From a code readability and maintainability POV it would of course nice if we
could reduce the number of cases that need special treatment, so the more string
arguments work like other primitive type arguments, the nicer it'll be.

[Jens Widell, 2014-05-21 13:41:43]

On 2014/05/20 06:55:28, Jens Lindström wrote:
> On 2014/05/20 06:46:06, haraken wrote:
> > Yeah, in terms of programmability, we want to always declare a v8::TypeCatch
> > when TOSTRING_VOID_INTERNAL() is used.
> >
> > I'm not sure but applying this CL
(https://codereview.chromium.org/242913002/)
> > will make the situation better (or worse)?

> With 242913002, we could use a prepare() that doesn't catch and always have
the
> TryCatch in the generated bindings code, in which case
TOSTRING_VOID_INTERNAL()
> would be essentially the same as TONATIVE_VOID_INTERNAL(). Performance-wise
this
> ought to be better for all cases where both TOSTRING_* and TONATIVE_* are use
> within the same method, since we'd reduce the number of initialized TryCatch
> objects.

haraken: do you want me to wait for these other patches to land and then adjust
this CL as described above, or land this first as is (if try-bots approve) and
then follow-up later on?

[haraken, 2014-05-21 14:00:02]

On 2014/05/21 13:41:43, Jens Lindström wrote:
> On 2014/05/20 06:55:28, Jens Lindström wrote:
> > On 2014/05/20 06:46:06, haraken wrote:
> > > Yeah, in terms of programmability, we want to always declare a
v8::TypeCatch
> > > when TOSTRING_VOID_INTERNAL() is used.
> > >
> > > I'm not sure but applying this CL
> (https://codereview.chromium.org/242913002/)
> > > will make the situation better (or worse)?
> 
> > With 242913002, we could use a prepare() that doesn't catch and always have
> the
> > TryCatch in the generated bindings code, in which case
> TOSTRING_VOID_INTERNAL()
> > would be essentially the same as TONATIVE_VOID_INTERNAL(). Performance-wise
> this
> > ought to be better for all cases where both TOSTRING_* and TONATIVE_* are
use
> > within the same method, since we'd reduce the number of initialized TryCatch
> > objects.
> 
> haraken: do you want me to wait for these other patches to land and then
adjust
> this CL as described above, or land this first as is (if try-bots approve) and
> then follow-up later on?

Sorry for being late!

I chatted with a V8 guy and he is landing the CL just right now. The V8 roll
will be made by the end of this week. So I'm sorry for keeping you waiting for a
long time, but it would be safer to wait for the change in.

In my understanding, if you land your CL to Blink now, exception handling will
start failing once the V8-side CL is in (Correct me if I'm wrong). If so, I
guess it'd be better to wait for the V8-side CL in.

[Jens Widell, 2014-05-21 14:15:28]

On 2014/05/21 14:00:02, haraken wrote:
> In my understanding, if you land your CL to Blink now, exception handling will
> start failing once the V8-side CL is in (Correct me if I'm wrong). If so, I
> guess it'd be better to wait for the V8-side CL in.

I don't think the V8-side change affects this CL. "My" code simple makes sure to
always call ReThrow() on any TryCatch that catches anything, and then
immediately returns. This always worked even with nested TryCatch objects, I
think. It was not rethrowing that behaved erratically, in particular if you
meant to actually catch (and drop) an exception, rather than propagating it.

[haraken, 2014-05-21 14:47:06]

On 2014/05/21 14:15:28, Jens Lindström wrote:
> On 2014/05/21 14:00:02, haraken wrote:
> > In my understanding, if you land your CL to Blink now, exception handling
will
> > start failing once the V8-side CL is in (Correct me if I'm wrong). If so, I
> > guess it'd be better to wait for the V8-side CL in.
> 
> I don't think the V8-side change affects this CL. "My" code simple makes sure
to
> always call ReThrow() on any TryCatch that catches anything, and then
> immediately returns. This always worked even with nested TryCatch objects, I
> think. It was not rethrowing that behaved erratically, in particular if you
> meant to actually catch (and drop) an exception, rather than propagating it.

ah, makes sense. Then it would be OK to land the CL now and then fix it in a
follow-up. Looking at the patch set 12.

[haraken, 2014-05-21 14:56:44]

LGTM

https://codereview.chromium.org/265293004/diff/260001/Source/bindings/scripts...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/265293004/diff/260001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:56: (base_type == 'DOMString' and not
argument.is_variadic))

I'm a bit concerned that this list becomes out-dated. Then we will miss adding a
TryCatch block that is needed. (But I have no good idea to address the issue.)

https://codereview.chromium.org/265293004/diff/260001/Source/bindings/scripts...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/265293004/diff/260001/Source/bindings/scripts...
Source/bindings/scripts/v8_types.py:458: if method_has_try_catch and macro ==
'TOSTRING_VOID_INTERNAL':

Would you add a comment about why we need the special casing for Strings?

[Jens Widell, 2014-05-21 15:07:01]

https://codereview.chromium.org/265293004/diff/260001/Source/bindings/scripts...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/265293004/diff/260001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:56: (base_type == 'DOMString' and not
argument.is_variadic))
On 2014/05/21 14:56:45, haraken wrote:
> 
> I'm a bit concerned that this list becomes out-dated. Then we will miss adding
a
> TryCatch block that is needed. (But I have no good idea to address the issue.)

I'm a bit concerned about maintainability here as well, but can't think of a
better way to do it, and not doing it increases code size.

The case where we miss a TryCatch is at least very loud and clear: compilation
breaks. It would probably happen to someone changing this script. Worst case is
that some innocent IDL file change mysteriously breaks compilation (after
someone changed the script and forgot to keep this list updated.)

But, well, I can't think of anything better than this code right now, and think
it solves a problem that needs solving.

https://codereview.chromium.org/265293004/diff/260001/Source/bindings/scripts...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/265293004/diff/260001/Source/bindings/scripts...
Source/bindings/scripts/v8_types.py:458: if method_has_try_catch and macro ==
'TOSTRING_VOID_INTERNAL':
On 2014/05/21 14:56:45, haraken wrote:
> 
> Would you add a comment about why we need the special casing for Strings?

Comment added.

[haraken, 2014-05-21 15:33:53]

On 2014/05/21 15:07:01, Jens Lindström wrote:
>
https://codereview.chromium.org/265293004/diff/260001/Source/bindings/scripts...
> File Source/bindings/scripts/v8_methods.py (right):
> 
>
https://codereview.chromium.org/265293004/diff/260001/Source/bindings/scripts...
> Source/bindings/scripts/v8_methods.py:56: (base_type == 'DOMString' and not
> argument.is_variadic))
> On 2014/05/21 14:56:45, haraken wrote:
> > 
> > I'm a bit concerned that this list becomes out-dated. Then we will miss
adding
> a
> > TryCatch block that is needed. (But I have no good idea to address the
issue.)
> 
> I'm a bit concerned about maintainability here as well, but can't think of a
> better way to do it, and not doing it increases code size.
> 
> The case where we miss a TryCatch is at least very loud and clear: compilation
> breaks. It would probably happen to someone changing this script. Worst case
is
> that some innocent IDL file change mysteriously breaks compilation (after
> someone changed the script and forgot to keep this list updated.)
> 
> But, well, I can't think of anything better than this code right now, and
think
> it solves a problem that needs solving.

Makes sense, still LGTM.

[Jens Widell, 2014-05-21 15:47:26]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-05-21 17:48:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jl@opera.com/265293004/280001

[commit-bot: I haz the power, 2014-05-22 00:55:46]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/9171)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/8985)

[commit-bot: I haz the power, 2014-05-22 06:57:07]

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

[commit-bot: I haz the power, 2014-05-22 06:57:08]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/9171)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/8985)

[Jens Widell, 2014-05-22 09:55:26]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-05-22 09:55:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jl@opera.com/265293004/280001

[commit-bot: I haz the power, 2014-05-22 11:30:34]

Change committed as 174546