IDL compiler: EventHandler for setters

Source/bindings/scripts/unstable/v8_attributes.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 102 matching lines @@
     contents['cpp_value'] = cpp_value
 
     if this_is_keep_alive_for_gc:
         v8_set_return_value_statement = 'v8SetReturnValue(info, wrapper)'
         includes.update(v8_types.includes_for_type(idl_type))
         includes.add('bindings/v8/V8HiddenPropertyName.h')
     else:
         v8_set_return_value_statement = v8_types.v8_set_return_value(idl_type, cpp_value, includes, callback_info='info', isolate='info.GetIsolate()', extended_attributes=extended_attributes, script_wrappable='imp')
     contents['v8_set_return_value'] = v8_set_return_value_statement
 
-    if (idl_type == 'EventHandler' and
-        interface.name in ['Window', 'WorkerGlobalScope'] and
-        attribute.name == 'onerror'):
-            includes.add('bindings/v8/V8ErrorHandler.h')
+    if idl_type == 'EventHandler':
+        includes.update(v8_types.includes_for_type('EventHandler'))

[haraken, 2013/10/19 12:41:29]

I guess we should always call
includes.update(v8_types.includes_for_type(idl_type))) in this method. Since the
getter is using the IDL type, there would be no reason to not call
includes.update().

Then you can remove includes.update() here and includes.update() in the 'if
this_is_keep_alive_for_gc' branch.

[Nils Barth (inactive), 2013/10/21 02:41:38]

This would simplify the code.
This involves Perl changes (for consistency), so I'll post in a followup CL.

+        if (interface.name in ['Window', 'WorkerGlobalScope'] and
+            attribute.name == 'onerror'):
+                includes.add('bindings/v8/V8ErrorHandler.h')
 
     # [CheckSecurityForNode]
     is_check_security_for_node = 'CheckSecurityForNode' in extended_attributes
     if is_check_security_for_node:
         includes.add('bindings/v8/BindingSecurity.h')
     v8_utilities.generate_measure_as(attribute, contents, includes)  # [MeasureAs]
 
     # [DeprecateAs]
     v8_utilities.generate_deprecate_as(attribute, contents, includes)
     if is_check_security_for_node or is_getter_raises_exception:
         includes.update(set(['bindings/v8/ExceptionMessages.h',
                              'bindings/v8/ExceptionState.h']))
 
     contents.update({
         'activity_logging_getter': v8_utilities.has_activity_logging(attribute, includes, 'Getter'),  # [ActivityLogging]
         'is_check_security_for_node': is_check_security_for_node,
         'is_getter_raises_exception': is_getter_raises_exception,
         'is_unforgeable': 'Unforgeable' in extended_attributes,
     })
 
     # Setter
     if not has_setter:
         return contents, includes
 
     contents.update({
         'v8_value_to_local_cpp_value': v8_types.v8_value_to_local_cpp_value(idl_type, attribute.extended_attributes, 'jsValue', 'cppValue', includes, 'info.GetIsolate()'),
-        'cpp_setter': setter_expression(attribute, contents),
+        'cpp_setter': setter_expression(attribute, contents, includes),
     })
 
     return contents, includes
 
 
 def getter_expression(interface, attribute, contents, includes):
     arguments = []
     if 'Reflect' in attribute.extended_attributes:
         getter_base_name = content_attribute_getter_base_name(attribute, includes, arguments)
     else:
@@ skipping 34 matching lines @@
     arguments.append(scoped_name)
 
     idl_type = attribute.data_type
     if idl_type in CONTENT_ATTRIBUTE_GETTER_NAMES:
         return CONTENT_ATTRIBUTE_GETTER_NAMES[idl_type]
     if 'URL' in attribute.extended_attributes:
         return 'getURLAttribute'
     return 'fastGetAttribute'
 
 
-def setter_expression(attribute, contents):
+def setter_expression(attribute, contents, includes):
     arguments = v8_utilities.call_with_arguments(attribute, contents)
     idl_type = attribute.data_type
-    # FIXME: should be able to eliminate WTF::getPtr in most or all cases
-    cpp_value = 'WTF::getPtr(cppValue)' if v8_types.interface_type(idl_type) and not v8_types.array_type(idl_type) else 'cppValue'
-    arguments.append(cpp_value)
+    if idl_type == 'EventHandler':
+        includes.add('bindings/v8/V8EventListenerList.h')

[haraken, 2013/10/19 12:41:29]

Ditto. I think we can just always call
includes.update(v8_types.includes_for_type(idl_type))) in this method. Then you
can remove the includes.add('bindings/v8/V8EventListenerList.h').

[Nils Barth (inactive), 2013/10/21 02:41:38]

Will do (in same "includes for type" followup CL)!

V8EventListenerList.h is only used for setters and methods
(not getters), which is why it's currently here (and in method generator),
but consolidating in includes_for_type simplifies code.

+        isolated_world = 'isolatedWorldForIsolate(info.GetIsolate())'
+        arguments.extend(['V8EventListenerList::getEventListener(jsValue, true, ListenerFindOrCreate)', isolated_world])
+        contents['event_handler_getter_expression'] = 'imp->%s(%s)' % (cpp_name(attribute), isolated_world)

[haraken, 2013/10/19 12:41:29]

Not related to your CL, it looks ugly to get an isolated world in generated code
and pass the isolated world to outside. For consistency with other places, we
should instead pass an isolate to outside and have the outside get an isolated
world from the isolate.

Would you fix this in a follow-up CL?

[Nils Barth (inactive), 2013/10/21 02:41:38]

Will do.

+    elif v8_types.interface_type(idl_type) and not v8_types.array_type(idl_type):
+        # FIXME: should be able to eliminate WTF::getPtr in most or all cases
+        arguments.append('WTF::getPtr(cppValue)')
+    else:
+        arguments.append('cppValue')
+
     return 'imp->set%s(%s)' % (capitalize(cpp_name(attribute)), ', '.join(arguments))
 
 
 def is_keep_alive_for_gc(attribute):
     idl_type = attribute.data_type
     extended_attributes = attribute.extended_attributes
     return (
         'KeepAttributeAliveForGC' in extended_attributes or
         # For readonly attributes, for performance reasons we keep the attribute
         # wrapper alive while the owner wrapper is alive, because the attribute
@@ skipping 54 matching lines @@
     if 'PerWorldBindings' not in attribute.extended_attributes:
         return '0'
     return '%sV8Internal::%sAttributeGetterCallbackForMainWorld' % (cpp_name(interface), attribute.name)
 
 
 def setter_callback_name_for_main_world(interface, attribute):
     if ('PerWorldBindings' not in attribute.extended_attributes or
         attribute.is_read_only):
         return '0'
     return '%sV8Internal::%sAttributeSetterCallbackForMainWorld' % (cpp_name(interface), attribute.name)