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

Source/bindings/scripts/v8_methods.py

 # Copyright (C) 2013 Google Inc. All rights reserved.
 #
 # Redistribution and use in source and binary forms, with or without
 # modification, are permitted provided that the following conditions are
 # met:
 #
 #     * Redistributions of source code must retain the above copyright
 # notice, this list of conditions and the following disclaimer.
 #     * Redistributions in binary form must reproduce the above
 # copyright notice, this list of conditions and the following disclaimer
@@ skipping 22 matching lines @@
 Design doc: http://www.chromium.org/developers/design-documents/idl-compiler
 """
 
 from idl_types import IdlType, IdlUnionType, inherits_interface
 from v8_globals import includes
 import v8_types
 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
     idl_type = method.idl_type
     is_static = method.is_static
     name = method.name
 
     idl_type.add_includes_for_type()
     this_cpp_value = cpp_value(interface, method, len(arguments))
 
@@ skipping 19 matching lines @@
         includes.add('core/dom/custom/CustomElementCallbackDispatcher.h')
 
     has_event_listener_argument = any(
         argument for argument in arguments
         if argument.idl_type.name == 'EventListener')
     is_check_security_for_frame = (
         'CheckSecurity' in interface.extended_attributes and
         '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,
         'deprecate_as': v8_utilities.deprecate_as(method),  # [DeprecateAs]
         'do_not_check_signature': not(is_static or
             v8_utilities.has_extended_attribute(method,
                 ['DoNotCheckSecurity', 'DoNotCheckSignature', 'NotEnumerable',
                  'ReadOnly', 'RuntimeEnabled', 'Unforgeable'])),
         'function_template': function_template(),
         'idl_type': idl_type.base_type,
@@ skipping 40 matching lines @@
         'world_suffixes': ['', 'ForMainWorld'] if 'PerWorldBindings' in extended_attributes else [''],  # [PerWorldBindings]
     }
 
 
 def generate_argument(interface, method, argument, index):
     extended_attributes = argument.extended_attributes
     idl_type = argument.idl_type
     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,
         'has_event_listener_argument': any(
             argument_so_far for argument_so_far in method.arguments[:index]
             if argument_so_far.idl_type.name == 'EventListener'),
         'has_legacy_overload_string':  # [LegacyOverloadString]
             'LegacyOverloadString' in extended_attributes,
         'has_type_checking_interface':
             (has_extended_attribute_value(interface, 'TypeChecking', 'Interface') or
@@ skipping 86 matching lines @@
 
     script_wrappable = 'impl' if inherits_interface(interface_name, 'Node') else ''
     return idl_type.v8_set_return_value(cpp_value, extended_attributes, script_wrappable=script_wrappable, release=release, for_main_world=for_main_world)
 
 
 def v8_value_to_local_cpp_value(argument, index):
     extended_attributes = argument.extended_attributes
     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
         extended_attributes.get('Default') == 'NullString'):
         v8_value = 'argumentOrNull(info, %s)' % 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)
 
 
 ################################################################################
 # Auxiliary functions
 ################################################################################
 
 # [NotEnumerable]
 def property_attributes(method):
     extended_attributes = method.extended_attributes
     property_attributes_list = []
     if 'NotEnumerable' in extended_attributes:
         property_attributes_list.append('v8::DontEnum')
     if 'ReadOnly' in extended_attributes:
         property_attributes_list.append('v8::ReadOnly')
     if property_attributes_list:
         property_attributes_list.insert(0, 'v8::DontDelete')
     return property_attributes_list
 
 
 def union_arguments(idl_type):
     """Return list of ['result0Enabled', 'result0', 'result1Enabled', ...] for union types, for use in setting return value"""
     return [arg
             for i in range(len(idl_type.member_types))
             for arg in ['result%sEnabled' % i, 'result%s' % i]]
 
 IdlType.union_arguments = property(lambda self: None)
 IdlUnionType.union_arguments = property(union_arguments)