IDL: Generate all constructor callbacks with the same template macro

Source/bindings/templates/methods.cpp

Index: Source/bindings/templates/methods.cpp
diff --git a/Source/bindings/templates/methods.cpp b/Source/bindings/templates/methods.cpp
index 598506269c938201bd0d63a2271c22b8b373d6da..ec4a33cc859a3a336c3b8093973fbb6e9a80acb3 100644
--- a/Source/bindings/templates/methods.cpp
+++ b/Source/bindings/templates/methods.cpp
@@ -482,17 +482,31 @@ static void {{method.name}}OriginSafeMethodGetterCallback{{world_suffix}}(v8::Lo
 
 {##############################################################################}
 {% macro generate_constructor(constructor) %}
-static void constructor{{constructor.overload_index}}(const v8::FunctionCallbackInfo<v8::Value>& info)
+{% set name = '%sConstructorCallback' % v8_class
+              if constructor.is_named_constructor else
+              'constructor%s' % (constructor.overload_index or '') %}
+static void {{name}}(const v8::FunctionCallbackInfo<v8::Value>& info)
 {
     v8::Isolate* isolate = info.GetIsolate();
+    {% if constructor.is_named_constructor %}
+    if (!info.IsConstructCall()) {
+        throwTypeError(ExceptionMessages::constructorNotCallableAsFunction("{{constructor.name}}"), isolate);
+        return;
+    }
+
+    if (ConstructorMode::current(isolate) == ConstructorMode::WrapExistingObject) {
+        v8SetReturnValue(info, info.Holder());
+        return;
+    }
+    {% endif %}

[haraken, 2014/06/25 09:43:11]

This block is fine in this CL, but we might want to merge the block with
constructorCallback in interface.cpp. (We should add the TRACE_EVENT macro and
UserCounter to the named constructor. If we can share the block with
constructorCallback, the issue will be resolved.)

[Jens Widell, 2014/06/25 10:55:37]

Ah, I was wondering why these things applied only to named constructors.

I'll sort it out in a follow-up.

     {% if constructor.has_exception_state %}
     ExceptionState exceptionState(ExceptionState::ConstructionContext, "{{interface_name}}", info.Holder(), isolate);
     {% endif %}
     {# Overloaded constructors have length checked during overload resolution #}
-    {% if interface_length and not constructor.overload_index %}
+    {% if constructor.number_of_required_arguments and not constructor.overload_index %}

[Jens Widell, 2014/06/25 07:50:42]

Using 'constructor.number_of_required_arguments' instead of 'interface_length'
made a difference for named constructors with non-zero required arguments, since
'interface_length' only takes regular constructors into account.

In practice, there are named constructors with non-zero required arguments
outside of the bindings tests.

[haraken, 2014/06/25 09:43:11]

What is the spec saying?

interface.cpp is still using interface_length, but is it OK?

[Jens Widell, 2014/06/25 10:55:37]

The key here is that regular constructors (or custom constructors, or event
constructors) and 'interface_length' all apply to the interface object.

Named constructors is a separate case, and their number of required arguments
affects the length property of a different object. (I don't think that object's
length property is ever actually set, so it's probably effectively hard-coded
zero. Good thing no real named constructors have any required arguments.)

And also, 'interface_length' is specifically "Function.length for the interface
object". That's the number of required arguments of the shortest overload, and
was thus always technically incorrect to use for arity checks for overloaded
regular constructors. Each constructor's 'number_of_required_arguments' is the
right value for the arity check. But the arity check is of course disabled here
for overloads, since it's performed implicitly by the overload resolution, so
this incorrectness was inconsequential.

     {# FIXME: remove UNLIKELY: constructors are expensive, so no difference. #}
-    if (UNLIKELY(info.Length() < {{interface_length}})) {
-        {{throw_minimum_arity_type_error(constructor, interface_length)}};
+    if (UNLIKELY(info.Length() < {{constructor.number_of_required_arguments}})) {
+        {{throw_minimum_arity_type_error(constructor, constructor.number_of_required_arguments)}};
         return;
     }
     {% endif %}
@@ -529,43 +543,3 @@ V8DOMWrapper::associateObjectWithWrapper<{{v8_class}}>(impl.release(), &{{constr
 {% endif %}
 v8SetReturnValue(info, wrapper);
 {% endmacro %}
-
-
-{##############################################################################}
-{% macro named_constructor_callback(constructor) %}
-static void {{v8_class}}ConstructorCallback(const v8::FunctionCallbackInfo<v8::Value>& info)
-{
-    v8::Isolate* isolate = info.GetIsolate();
-    if (!info.IsConstructCall()) {
-        throwTypeError(ExceptionMessages::constructorNotCallableAsFunction("{{constructor.name}}"), isolate);
-        return;
-    }
-
-    if (ConstructorMode::current(isolate) == ConstructorMode::WrapExistingObject) {
-        v8SetReturnValue(info, info.Holder());
-        return;
-    }
-
-    Document& document = *toDocument(currentExecutionContext(isolate));
-
-    {% if constructor.has_exception_state %}
-    ExceptionState exceptionState(ExceptionState::ConstructionContext, "{{interface_name}}", info.Holder(), isolate);
-    {% endif %}
-    {% if constructor.number_of_required_arguments %}
-    if (UNLIKELY(info.Length() < {{constructor.number_of_required_arguments}})) {
-        {{throw_minimum_arity_type_error(constructor, constructor.number_of_required_arguments)}};
-        return;
-    }
-    {% endif %}
-    {% if constructor.arguments %}
-    {{generate_arguments(constructor) | indent}}
-    {% endif %}
-    {{constructor.cpp_type}} impl = {{constructor.cpp_value}};
-    {% if is_constructor_raises_exception %}
-    if (exceptionState.throwIfNeeded())
-        return;
-    {% endif %}
-
-    {{generate_constructor_wrapper(constructor) | indent}}
-}
-{% endmacro %}