Implement "create an element" when sync for Custom Element V1

third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp

Index: third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
diff --git a/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp b/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
index de725ff62ad418e8ad81b48123a14d602e1e0ba3..3939ae1ce6a20d5f0ce86c40c6ace644d3b58793 100644
--- a/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
+++ b/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
@@ -13,6 +13,8 @@
 #include "bindings/core/v8/V8ScriptRunner.h"
 #include "bindings/core/v8/V8ThrowException.h"
 #include "core/dom/ExceptionCode.h"
+#include "core/html/HTMLElement.h"
+#include "core/html/HTMLUnknownElement.h"
 #include "v8.h"
 #include "wtf/Allocator.h"
 
@@ -152,6 +154,61 @@ ScriptCustomElementDefinition::ScriptCustomElementDefinition(
 {
 }
 
+HTMLElement* ScriptCustomElementDefinition::createElementSync(
+    Document& document, const QualifiedName& tagName,
+    ExceptionState& exceptionState)
+{

[haraken, 2016/06/14 01:48:11]

You can add DCHECK(ScriptState::current() == m_scriptState).

[kojii, 2016/06/14 07:13:15]

Done.

+    // Create an element
+    // https://dom.spec.whatwg.org/#concept-create-element
+    // 6. If definition is non-null
+    // 6.1. If the synchronous custom elements flag is set:
+    // 6.1.2. Set result to Construct(C). Rethrow any exceptions.
+    Element* element = nullptr;
+    {
+        v8::TryCatch tryCatch(m_scriptState->isolate());
+        element = runConstructor();
+        if (tryCatch.HasCaught()) {
+            exceptionState.rethrowV8Exception(tryCatch.Exception());
+            return nullptr;
+        }
+    }
+
+    // 6.1.3. through 6.1.9.
+    checkConstructorResult(element, document, tagName, exceptionState);
+    if (exceptionState.hadException())
+        return nullptr;
+
+    DCHECK_EQ(element->getCustomElementState(), CustomElementState::Custom);
+    return toHTMLElement(element);
+}
+
+HTMLElement* ScriptCustomElementDefinition::createElementSync(
+    Document& document, const QualifiedName& tagName)
+{
+    ScriptState::Scope scope(m_scriptState.get());
+    v8::Isolate* isolate = m_scriptState->isolate();
+
+    // When invoked from "create an element for a token":
+    // https://html.spec.whatwg.org/multipage/syntax.html#create-an-element-for-the-token
+    // 7. If this step throws an exception, then report the exception, ...
+    v8::TryCatch tryCatch(isolate);
+    tryCatch.SetVerbose(true);
+    ExceptionState exceptionState(ExceptionState::ConstructionContext, "CustomElement", constructor(), isolate);
+    HTMLElement* element = createElementSync(document, tagName, exceptionState);
+    DCHECK(!tryCatch.HasCaught());
+    exceptionState.throwIfNeeded();

[haraken, 2016/06/14 01:48:11]

I still don't understand these lines.

The createElementSync at line 157 should never throw a V8 exception because it
is suppressed at the TryCatch block at line 168. Note that
exceptionState.rethrowV8Exception just sets the exception on the exceptionState
and doesn't throw the exception.

[kojii, 2016/06/14 07:13:14]

Reordered to read easier by reflecting our offline discussion.

+
+    if (tryCatch.HasCaught() || !element) {
+        // ...and let element be instead a new element that implements
+        // HTMLUnknownElement, with no attributes, namespace set to given
+        // namespace, namespace prefix set to null, custom element state
+        // "undefined", and node document set to document.
+        element = HTMLUnknownElement::create(tagName, document);
+        element->setCustomElementState(CustomElementState::Undefined);
+    }

[haraken, 2016/06/14 06:42:59]

To make the code clearer, let's write this as follows:

createElementSync(..., exceptionState);
if (exceptionState.hadException()) {
  ...;
}
v8::TryCatch tryCatch(isolate);
tryCatch.SetVerbose(true);
exceptionState.throwIfNeeded();

+    return element;
+}
+
 // https://html.spec.whatwg.org/multipage/scripting.html#upgrades
 bool ScriptCustomElementDefinition::runConstructor(Element* element)
 {
@@ -165,18 +222,11 @@ bool ScriptCustomElementDefinition::runConstructor(Element* element)
     v8::TryCatch tryCatch(isolate);
     tryCatch.SetVerbose(true);
 
-    ExecutionContext* executionContext = m_scriptState->getExecutionContext();
-    v8::Local<v8::Value> result;
-    if (!v8Call(V8ScriptRunner::callAsConstructor(
-        isolate,
-        constructor(),
-        executionContext,
-        0,
-        nullptr),
-        result))
+    Element* result = runConstructor();
+    if (!result)
         return false;
 
-    if (V8Element::toImplWithTypeCheck(isolate, result) != element) {
+    if (result != element) {
         V8ThrowException::throwException(
             V8ThrowException::createDOMException(
                 m_scriptState->isolate(),
@@ -191,6 +241,23 @@ bool ScriptCustomElementDefinition::runConstructor(Element* element)
     return true;
 }
 
+Element* ScriptCustomElementDefinition::runConstructor()
+{

[haraken, 2016/06/14 01:48:11]

You can add DCHECK(ScriptState::current() == m_scriptState).

[kojii, 2016/06/14 07:13:15]

Done.

+    v8::Isolate* isolate = m_scriptState->isolate();
+    ExecutionContext* executionContext = m_scriptState->getExecutionContext();
+    v8::Local<v8::Value> result;
+    if (!v8Call(V8ScriptRunner::callAsConstructor(
+        isolate,
+        constructor(),
+        executionContext,
+        0,
+        nullptr),
+        result)) {
+        return nullptr;
+    }
+    return V8Element::toImplWithTypeCheck(isolate, result);
+}
+
 v8::Local<v8::Object> ScriptCustomElementDefinition::constructor() const
 {
     DCHECK(!m_constructor.isEmpty());