IDL: reuse more code between CG for methods and constructors

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 ae5bfbd527bd6b238a2662ce2f2303b54adba2ed..2890b9c16bb3a390e8031192f58910f6ba121ec1 100644
--- a/Source/bindings/templates/methods.cpp
+++ b/Source/bindings/templates/methods.cpp
@@ -489,12 +489,12 @@ static void constructor{{constructor.overload_index}}(const v8::FunctionCallback
     {{generate_arguments(constructor) | indent}}
     {% endif %}
     {% if is_constructor_call_with_execution_context %}
-    ExecutionContext* context = currentExecutionContext(isolate);
+    ExecutionContext* scriptContext = currentExecutionContext(isolate);

[Jens Widell, 2014/06/17 10:40:20]

Renamed to make [CallWith] handling more uniform; it's named scriptContext in
methods already.

This rename, and the DocumentPtr*/Document& tweaking for named constructors, is
the only CG changes caused by this patch.

[haraken, 2014/06/17 11:28:05]

scriptContext sounds unnatural. Shall we rename it to exeuctionContext or
context in other places as well?

[haraken, 2014/06/17 12:38:53]

Let's use |executionContext| consistently.

[Jens Widell, 2014/06/17 12:53:16]

Done.

     {% endif %}
     {% if is_constructor_call_with_document %}
     Document& document = *toDocument(currentExecutionContext(isolate));
     {% endif %}
-    {{constructor.cpp_type}} impl = {{cpp_class}}::create({{constructor.argument_list | join(', ')}});
+    {{constructor.cpp_type}} impl = {{constructor.cpp_value}};
     {% if is_constructor_raises_exception %}
     if (exceptionState.throwIfNeeded())
         return;
@@ -535,12 +535,13 @@ static void {{v8_class}}ConstructorCallback(const v8::FunctionCallbackInfo<v8::V
         return;
     }
 
-    Document* document = currentDOMWindow(isolate)->document();
-    ASSERT(document);
+    Document* documentPtr = currentDOMWindow(isolate)->document();

[Jens Widell, 2014/06/17 10:40:20]

Since this code is here, unconditionally, CallWith=Document is still partially
implied for named constructors. It would be nicer if it was all or nothing;
either making this code conditional, or really making it fully implied (and
revert the IDL file changes from this CL.)

[Jens Widell, 2014/06/17 12:53:16]

This code uses 'currentDOMWindow(isolate)->document()' and regular constructors
use 'toDocument(currentExecutionContext(isolate))'. Are those two different in
any way?

[haraken, 2014/06/17 12:57:39]

The issue I want to address is just that we have both
[ConstructorCallWith=ExecutionContext] and [ConstructorCallWith=Document], and
both [CallWith=ExecutionContext] and [CallWith=Document]. Given that
ExecutionContext is the same thing as Document, I want to remove either of them.
(I haven't looked at the generate code in details; so I'm not sure which should
be removed.)

+    ASSERT(documentPtr);
+    Document& document = *documentPtr;

[Jens Widell, 2014/06/17 10:40:20]

Introduced to make [CallWith] handling more uniform. It's a reference in methods
already.

[haraken, 2014/06/17 11:28:05]

Given that we're planning to remove CallWith=Document, shall we keep this code
as is?

[Jens Widell, 2014/06/17 12:29:41]

Yeah, maybe. Would you prefer I upload a patch that does that first?

You're talking about replacing all uses of Document with ExecutionContext and
let the C++ implementation fetch the document instead, here? As is talked about
in a FIXME in IDLExtendedAttributes.txt?

[haraken, 2014/06/17 12:38:53]

Yes. A follow-up CL is fine :)

[Jens Widell, 2014/06/17 13:31:02]

Hmm. Replacing ConstructorCallWith=Document with ExecutionContext is not really
as nice as I would have hoped. There's a bunch of them where the ::create()
method is also used by other C++ code, and in that case passing in a Document is
the natural thing, I think.

If we're adding a bunch of create(ExecutionContext*, ...) functions that just
extract the document and call a create(Document&, ...) function, I wonder if it
doesn't make more sense to have that as functionality in the script/generated
code instead.

And there are constructors that are called with an ExecutionContext* today where
a Document& makes no sense, e.g. FileReader() which is usable in worker
contexts, so dropping ConstructorCallWith=ExecutionContext instead doesn't seem
viable.

 
     // Make sure the document is added to the DOM Node map. Otherwise, the {{cpp_class}} instance
     // may end up being the only node in the map and get garbage-collected prematurely.
-    toV8(document, info.Holder(), isolate);
+    toV8(documentPtr, info.Holder(), isolate);
 
     {% if constructor.has_exception_state %}
     ExceptionState exceptionState(ExceptionState::ConstructionContext, "{{interface_name}}", info.Holder(), isolate);
@@ -554,7 +555,7 @@ static void {{v8_class}}ConstructorCallback(const v8::FunctionCallbackInfo<v8::V
     {% if constructor.arguments %}
     {{generate_arguments(constructor) | indent}}
     {% endif %}
-    {{constructor.cpp_type}} impl = {{cpp_class}}::createForJSConstructor({{constructor.argument_list | join(', ')}});
+    {{constructor.cpp_type}} impl = {{constructor.cpp_value}};
     {% if is_constructor_raises_exception %}
     if (exceptionState.throwIfNeeded())
         return;