IDL compiler: EventHandler for setters

Source/bindings/scripts/unstable/v8_attributes.py

Index: Source/bindings/scripts/unstable/v8_attributes.py
diff --git a/Source/bindings/scripts/unstable/v8_attributes.py b/Source/bindings/scripts/unstable/v8_attributes.py
index 5964433461c8598711d0de051c06208705b707fc..ea40d01875b2ab2b7be43810861528e2c0d0ce9e 100644
--- a/Source/bindings/scripts/unstable/v8_attributes.py
+++ b/Source/bindings/scripts/unstable/v8_attributes.py
@@ -120,10 +120,11 @@ def generate_attribute_and_includes(interface, attribute):
         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
@@ -150,7 +151,7 @@ def generate_attribute_and_includes(interface, attribute):
 
     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
@@ -205,12 +206,20 @@ def content_attribute_getter_base_name(attribute, includes, arguments):
     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))