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

Source/bindings/scripts/v8_methods.py

Index: Source/bindings/scripts/v8_methods.py
diff --git a/Source/bindings/scripts/v8_methods.py b/Source/bindings/scripts/v8_methods.py
index e748e34c7bbfadf458d5369c37211ca0d93ef026..d2540aa28c00805b60d300f1eb3f39999be354e9 100644
--- a/Source/bindings/scripts/v8_methods.py
+++ b/Source/bindings/scripts/v8_methods.py
@@ -40,6 +40,22 @@ import v8_utilities
 from v8_utilities import has_extended_attribute_value
 
 
+def argument_needs_try_catch(argument):

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

I would tend to write this as a single boolean, as in:
return not(# These cases ...
           ... or
           ... or
           ... or
           # String and enumeration ...
           ...)

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

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]

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]".

+    # These cases are handled by separate code paths in the generate_argument()
+    # macro in Source/bindings/templates/methods.cpp.
+    if (argument['is_callback_interface'] or
+        argument['idl_type'] == 'SerializedScriptValue' or
+        argument['is_variadic_wrapper_type']):

[haraken, 2014/05/08 03:45:06]

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?

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

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.
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.

+        return False
+
+    # String and enumeration arguments converted using one of the TOSTRING_*
+    # macros in Source/bindings/v8/V8BindingMacros.h don't use a v8::TryCatch.
+    if argument['v8_value_to_local_cpp_value'].startswith('TOSTRING_'):

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

How about checking for DOMString directly, as in:
if argument['idl_type'].preprocessed_type.name == 'String':
?

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

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.

[Nils Barth (inactive), 2014/05/08 06:25:16]

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!

+        return False
+
+    return True
+
+
 def generate_method(interface, method):
     arguments = method.arguments
     extended_attributes = method.extended_attributes
@@ -79,10 +95,14 @@ def generate_method(interface, method):
         'DoNotCheckSecurity' not in extended_attributes)
     is_raises_exception = 'RaisesException' in extended_attributes
 
+    generated_arguments = [generate_argument(interface, method, argument, index)
+                           for index, argument in enumerate(arguments)]
+
     return {
         'activity_logging_world_list': v8_utilities.activity_logging_world_list(method),  # [ActivityLogging]
-        'arguments': [generate_argument(interface, method, argument, index)
-                      for index, argument in enumerate(arguments)],
+        'arguments': generated_arguments,
+        'arguments_need_try_catch': any(argument_needs_try_catch(argument)
+                                        for argument in generated_arguments),
         'conditional_string': v8_utilities.conditional_string(method),
         'cpp_type': idl_type.cpp_type,
         'cpp_value': this_cpp_value,
@@ -143,8 +163,17 @@ def generate_argument(interface, method, argument, index):
     this_cpp_value = cpp_value(interface, method, index)
     is_variadic_wrapper_type = argument.is_variadic and idl_type.is_wrapper_type
 
+    if argument.is_variadic and not idl_type.is_wrapper_type:

[haraken, 2014/05/08 03:45:06]

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

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

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.

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

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.

+        element_cpp_type = idl_type.cpp_type_args(extended_attributes=extended_attributes)
+        vector_type = v8_types.cpp_ptr_type('Vector', 'HeapVector', idl_type.gc_type)
+        cpp_type = v8_types.cpp_template_type(vector_type, element_cpp_type)
+    else:
+        cpp_type = idl_type.cpp_type_args(extended_attributes=extended_attributes,
+                                          used_as_argument=not is_variadic_wrapper_type,
+                                          used_in_cpp_sequence=is_variadic_wrapper_type)
+
     return {
-        'cpp_type': idl_type.cpp_type_args(used_in_cpp_sequence=is_variadic_wrapper_type),
+        'cpp_type': cpp_type,
         'cpp_value': this_cpp_value,
         'enum_validation_expression': idl_type.enum_validation_expression,
         'has_default': 'Default' in extended_attributes,
@@ -251,9 +280,8 @@ def v8_value_to_local_cpp_value(argument, index):
     idl_type = argument.idl_type
     name = argument.name
     if argument.is_variadic:
-        vector_type = v8_types.cpp_ptr_type('Vector', 'HeapVector', idl_type.gc_type)
-        return 'TONATIVE_VOID({vector_type}<{cpp_type}>, {name}, toNativeArguments<{cpp_type}>(info, {index}))'.format(
-            vector_type=vector_type, cpp_type=idl_type.cpp_type, name=name,
+        return 'TONATIVE_VOID_NO_DECL({name}, toNativeArguments<{cpp_type}>(info, {index}))'.format(
+            cpp_type=idl_type.cpp_type, name=name,
             index=index)
     # [Default=NullString]
     if (argument.is_optional and idl_type.name == 'String' and
@@ -262,7 +290,7 @@ def v8_value_to_local_cpp_value(argument, index):
     else:
         v8_value = 'info[%s]' % index
     return idl_type.v8_value_to_local_cpp_value(extended_attributes, v8_value,
-                                                name, index=index)
+                                                name, index=index, declare_variable=False)
 
 
 ################################################################################