[bindings] partial interfaces should not violate componentization

Source/bindings/scripts/v8_interface.py

Index: Source/bindings/scripts/v8_interface.py
diff --git a/Source/bindings/scripts/v8_interface.py b/Source/bindings/scripts/v8_interface.py
index 9cb32a59af27c5984346bf23bf7c82a820cc6830..3bdc8720478d3043a8e38bf48ddcf3a31a8f6b2a 100644
--- a/Source/bindings/scripts/v8_interface.py
+++ b/Source/bindings/scripts/v8_interface.py
@@ -77,9 +77,23 @@ def interface_context(interface):
     includes.update(INTERFACE_CPP_INCLUDES)
     header_includes = set(INTERFACE_H_INCLUDES)
 
-    parent_interface = interface.parent
-    if parent_interface:
-        header_includes.update(v8_types.includes_for_interface(parent_interface))
+    if interface.is_partial:
+        # A partial interface definition cannot specify that the interface inherits
+        # from another interface. Inheritance must be specified on the original interface
+        # definition.
+        parent_interface = None

[bashi, 2014/10/15 05:29:23]

nit: you can omit this.

+        is_audio_buffer = False

[bashi, 2014/10/15 05:29:23]

unused?

[tasak, 2014/10/15 11:24:18]

Done.

+        is_document = False
+        is_event_target = False

[bashi, 2014/10/15 05:29:23]

nit: you can omit assigning False.

[tasak, 2014/10/15 11:24:18]

I saw the following errors:
    'is_document': is_document,
UnboundLocalError: local variable 'is_document' referenced before assignment
    'is_event_target': is_event_target,
UnboundLocalError: local variable 'is_event_target' referenced before assignment
    'parent_interface': parent_interface,
UnboundLocalError: local variable 'parent_interface' referenced before
assignment

[bashi, 2014/10/17 02:23:24]

BTW, shouldn't we raise an exception rather than assigning false?

+        # partial interface needs the definition of its original interface.
+        includes.add('bindings/core/v8/V8%s.h' % interface.name)
+    else:
+        parent_interface = interface.parent
+        if parent_interface:
+            header_includes.update(v8_types.includes_for_interface(parent_interface))
+        is_document = inherits_interface(interface.name, 'Document')
+        is_event_target = inherits_interface(interface.name, 'EventTarget')
+
     extended_attributes = interface.extended_attributes
 
     # [ActiveDOMObject]
@@ -95,7 +109,11 @@ def interface_context(interface):
 
     # [Iterable]
     iterator_method = None
-    if 'Iterable' in extended_attributes:
+    # FIXME: support Iterable in partial interfaces. However, we don't
+    # need to support iterator overloads between interface and
+    # partial interface definitions.
+    # http://heycam.github.io/webidl/#idl-overloading
+    if 'Iterable' in extended_attributes and not interface.is_partial:
         iterator_operation = IdlOperation(interface.idl_name)
         iterator_operation.name = 'iterator'
         iterator_operation.idl_type = IdlType('Iterator')
@@ -142,9 +160,13 @@ def interface_context(interface):
 
     wrapper_class_id = ('NodeClassId' if inherits_interface(interface.name, 'Node') else 'ObjectClassId')
 
+    v8_class_name = v8_utilities.v8_class_name(interface)
+    cpp_class_name = cpp_name(interface)
+
     context = {
         'conditional_string': conditional_string(interface),  # [Conditional]
-        'cpp_class': cpp_name(interface),
+        'cpp_class': cpp_class_name,
+        'actual_cpp_class': cpp_class_name + 'Partial' if interface.is_partial else cpp_class_name,
         'gc_type': this_gc_type,
         # FIXME: Remove 'EventTarget' special handling, http://crbug.com/383699
         'has_access_check_callbacks': (is_check_security and
@@ -159,7 +181,8 @@ def interface_context(interface):
         'is_active_dom_object': is_active_dom_object,
         'is_check_security': is_check_security,
         'is_dependent_lifetime': is_dependent_lifetime,
-        'is_event_target': inherits_interface(interface.name, 'EventTarget'),
+        'is_document': is_document,
+        'is_event_target': is_event_target,
         'is_exception': interface.is_exception,
         'is_node': inherits_interface(interface.name, 'Node'),
         'is_script_wrappable': is_script_wrappable,
@@ -169,6 +192,8 @@ def interface_context(interface):
                 is_active_dom_object or
                 is_dependent_lifetime)
             else 'Independent',
+        'is_partial': interface.is_partial,
+        'has_partial_interface': len(interface.partial_interfaces) > 0,
         'measure_as': v8_utilities.measure_as(interface),  # [MeasureAs]
         'parent_interface': parent_interface,
         'pass_cpp_type': cpp_template_type(
@@ -177,53 +202,13 @@ def interface_context(interface):
         'reachable_node_function': reachable_node_function,
         'runtime_enabled_function': runtime_enabled_function_name(interface),  # [RuntimeEnabled]
         'set_wrapper_reference_to_list': set_wrapper_reference_to_list,
-        'v8_class': v8_utilities.v8_class_name(interface),
+        'v8_class': v8_class_name,
+        'actual_v8_class': v8_class_name + 'Partial' if interface.is_partial else v8_class_name,

[bashi, 2014/10/15 05:29:23]

nit: alphabetical order. (I'd like to call this |v8_partial_class| though)

[tasak, 2014/10/15 11:24:18]

Done.
I would like to call. However, if a given interface is not partial, we can use
|v8_partial_class| in jinja templates?

[bashi, 2014/10/17 02:23:24]

Acknowledged.

         'wrapper_class_id': wrapper_class_id,
     }
 
     # Constructors
-    constructors = [constructor_context(interface, constructor)
-                    for constructor in interface.constructors
-                    # FIXME: shouldn't put named constructors with constructors
-                    # (currently needed for Perl compatibility)
-                    # Handle named constructors separately
-                    if constructor.name == 'Constructor']
-    if len(constructors) > 1:
-        context['constructor_overloads'] = overloads_context(constructors)
-
-    # [CustomConstructor]
-    custom_constructors = [{  # Only needed for computing interface length
-        'number_of_required_arguments':
-            number_of_required_arguments(constructor),
-    } for constructor in interface.custom_constructors]
-
-    # [EventConstructor]
-    has_event_constructor = 'EventConstructor' in extended_attributes
-    any_type_attributes = [attribute for attribute in interface.attributes
-                           if attribute.idl_type.name == 'Any']
-    if has_event_constructor:
-        includes.add('bindings/core/v8/Dictionary.h')
-        if any_type_attributes:
-            includes.add('bindings/core/v8/SerializedScriptValue.h')
-
-    # [NamedConstructor]
-    named_constructor = named_constructor_context(interface)
-
-    if (constructors or custom_constructors or has_event_constructor or
-        named_constructor):
-        includes.add('bindings/core/v8/V8ObjectConstructor.h')
-        includes.add('core/frame/LocalDOMWindow.h')
-
-    context.update({
-        'any_type_attributes': any_type_attributes,
-        'constructors': constructors,
-        'has_custom_constructor': bool(custom_constructors),
-        'has_event_constructor': has_event_constructor,
-        'interface_length':
-            interface_length(interface, constructors + custom_constructors),
-        'is_constructor_raises_exception': extended_attributes.get('RaisesException') == 'Constructor',  # [RaisesException=Constructor]
-        'named_constructor': named_constructor,
-    })
+    update_interface_constructor_context(interface, context)
 
     constants = [constant_context(constant) for constant in interface.constants]
 
@@ -271,10 +256,27 @@ def interface_context(interface):
     })
 
     # Methods
-    methods = [v8_methods.method_context(interface, method)
-               for method in interface.operations
-               if method.name]  # Skip anonymous special operations (methods)
-    compute_method_overloads_context(methods)
+    methods = []
+    if interface.original_interface:
+        for operation in interface.original_interface.operations:
+            if not operation.name:
+                continue
+            method = v8_methods.method_context(interface, operation)

[bashi, 2014/10/15 05:29:23]

How about adding an optional argument |is_visible|, of which default value is
True, to method_context()?
This way, we can write this like below:

methods.extends([v8_methods.method_context(interface, method, is_visible=False)
    for method in interface.original_interface.operations])

[tasak, 2014/10/15 11:24:19]

Done.

+            method['visible'] = False
+            methods.append(method)
+    methods.extend([v8_methods.method_context(interface, method)
+                    for method in interface.operations
+                    if method.name])  # Skip anonymous special operations (methods)
+    if interface.partial_interfaces:
+        assert len(interface.partial_interfaces) == len(set(interface.partial_interfaces))
+        for partial_interface in interface.partial_interfaces:
+            for operation in partial_interface.operations:
+                if not operation.name:
+                    continue
+                partial_method = v8_methods.method_context(interface, operation)
+                partial_method['visible'] = False
+                methods.append(partial_method)
+    compute_method_overloads_context(interface, methods)
 
     # Stringifier
     if interface.stringifier:
@@ -300,11 +302,18 @@ def interface_context(interface):
 
         if 'overloads' in method:
             overloads = method['overloads']
+            if not overloads['visible']:
+                continue
+            # original interface will register instead of partial interface.
+            if overloads['has_partial_overloads'] and interface.is_partial:
+                continue
             per_context_enabled_function = overloads['per_context_enabled_function_all']
             conditionally_exposed_function = overloads['exposed_test_all']
             runtime_enabled_function = overloads['runtime_enabled_function_all']
             has_custom_registration = overloads['has_custom_registration_all']
         else:
+            if not method['visible']:
+                continue
             per_context_enabled_function = method['per_context_enabled_function']
             conditionally_exposed_function = method['exposed_test']
             runtime_enabled_function = method['runtime_enabled_function']
@@ -359,6 +368,56 @@ def interface_context(interface):
     return context
 
 
+def update_interface_constructor_context(interface, context):

[bashi, 2014/10/15 05:29:23]

Question: is there any change except for factoring out as a method? It's
somewhat difficult to check what has been changed without diff..

[tasak, 2014/10/15 11:24:19]

Done.

+    constructors = [constructor_context(interface, constructor)
+                    for constructor in interface.constructors
+                    # FIXME: shouldn't put named constructors with constructors
+                    # (currently needed for Perl compatibility)
+                    # Handle named constructors separately
+                    if constructor.name == 'Constructor']
+    if len(constructors) > 1:
+        context['constructor_overloads'] = overloads_context(interface, constructors)
+
+    # [CustomConstructor]
+    custom_constructors = [{  # Only needed for computing interface length
+        'number_of_required_arguments':
+            number_of_required_arguments(constructor),
+    } for constructor in interface.custom_constructors]
+
+    # [EventConstructor]
+    has_event_constructor = 'EventConstructor' in interface.extended_attributes
+    # [NamedConstructor]
+    named_constructor = named_constructor_context(interface)
+
+    if constructors or custom_constructors or has_event_constructor or named_constructor:
+        if interface.is_partial:
+            raise Exception('[Constructor] and [NamedConstructor] MUST NOT be'
+                            ' specified on partial interface definitions:'
+                            '%s' % interface.name)
+
+        includes.add('bindings/core/v8/V8ObjectConstructor.h')
+        includes.add('core/frame/LocalDOMWindow.h')
+
+    any_type_attributes = [attribute for attribute in interface.attributes
+                           if attribute.idl_type.name == 'Any']
+    if has_event_constructor:
+        includes.add('bindings/core/v8/Dictionary.h')
+        if any_type_attributes:
+            includes.add('bindings/core/v8/SerializedScriptValue.h')
+
+    context.update({
+        'any_type_attributes': any_type_attributes,
+        'constructors': constructors,
+        'has_custom_constructor': bool(custom_constructors),
+        'has_event_constructor': has_event_constructor,
+        'interface_length':
+            interface_length(interface, constructors + custom_constructors),
+        'is_constructor_raises_exception':
+            interface.extended_attributes.get('RaisesException') == 'Constructor',  # [RaisesException=Constructor]
+        'named_constructor': named_constructor,
+    })
+
+
 # [DeprecateAs], [Reflect], [RuntimeEnabled]
 def constant_context(constant):
     extended_attributes = constant.extended_attributes
@@ -379,16 +438,16 @@ def constant_context(constant):
 # Overloads
 ################################################################################
 
-def compute_method_overloads_context(methods):
+def compute_method_overloads_context(interface, methods):
     # Regular methods
-    compute_method_overloads_context_by_type([method for method in methods
-                                              if not method['is_static']])
+    compute_method_overloads_context_by_type(
+        interface, [method for method in methods if not method['is_static']])
     # Static methods
-    compute_method_overloads_context_by_type([method for method in methods
-                                              if method['is_static']])
+    compute_method_overloads_context_by_type(
+        interface, [method for method in methods if method['is_static']])
 
 
-def compute_method_overloads_context_by_type(methods):
+def compute_method_overloads_context_by_type(interface, methods):
     """Computes |method.overload*| template values.
 
     Called separately for static and non-static (regular) methods,
@@ -402,7 +461,7 @@ def compute_method_overloads_context_by_type(methods):
     for name, overloads in method_overloads_by_name(methods):
         # Resolution function is generated after last overloaded function;
         # package necessary information into |method.overloads| for that method.
-        overloads[-1]['overloads'] = overloads_context(overloads)
+        overloads[-1]['overloads'] = overloads_context(interface, overloads)
         overloads[-1]['overloads']['name'] = name
 
 
@@ -420,12 +479,13 @@ def method_overloads_by_name(methods):
     return sort_and_groupby(overloaded_methods, itemgetter('name'))
 
 
-def overloads_context(overloads):
+def overloads_context(interface, overloads):
     """Returns |overloads| template values for a single name.
 
     Sets |method.overload_index| in place for |method| in |overloads|
     and returns dict of overall overload template values.
     """
+    # partial overloads breaks this assumption.

[bashi, 2014/10/15 05:29:23]

Should we remove or change the assert then?

[tasak, 2014/10/15 11:24:19]

Removed the comment. This assert is correct now.

     assert len(overloads) > 1  # only apply to overloaded names
     for index, method in enumerate(overloads, 1):
         method['overload_index'] = index
@@ -466,6 +526,14 @@ def overloads_context(overloads):
         raise ValueError('Overloads of %s have conflicting Promise/non-Promise types'
                          % (name))
 
+    overloads_visibles = set([method.get('visible', True) for method in overloads])

[bashi, 2014/10/15 05:29:23]

Use method['visible'] instead of get()? It seems that |method| always has
'visible'.

[tasak, 2014/10/15 11:24:19]

|method| always has. However, |constructor| never has.

constructors = [constructor_context ...]
if len(constructors) > 1:
        context['constructor_overloads'] = overloads_context(interface,
constructors)

So we should modify constructor_context?

[bashi, 2014/10/17 02:23:24]

It will help make code simple, IMO.

+    if len(overloads_visibles) > 1:
+        overloads_visible = True
+        has_partial_overloads = True
+    else:
+        overloads_visible = overloads_visibles.pop()
+        has_partial_overloads = False
+
     return {
         'deprecate_all_as': common_value(overloads, 'deprecate_as'),  # [DeprecateAs]
         'exposed_test_all': common_value(overloads, 'exposed_test'),  # [Exposed]
@@ -483,6 +551,8 @@ def overloads_context(overloads):
             # sequence of possible lengths, otherwise invalid length means
             # "not enough arguments".
             if lengths[-1] - lengths[0] != len(lengths) - 1 else None,
+        'visible': overloads_visible,
+        'has_partial_overloads': has_partial_overloads,
     }