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

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

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "bindings/core/v8/ScriptCustomElementDefinition.h"
 
 #include "bindings/core/v8/ScriptState.h"
 #include "bindings/core/v8/V8Binding.h"
 #include "bindings/core/v8/V8BindingMacros.h"
 #include "bindings/core/v8/V8CustomElementsRegistry.h"
 #include "bindings/core/v8/V8Element.h"
 #include "bindings/core/v8/V8HiddenValue.h"
 #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"
 
 namespace blink {
 
 // Retrieves the custom elements constructor -> name map, creating it
 // if necessary. The same map is used to keep prototypes alive.
 static v8::Local<v8::Map> ensureCustomElementsRegistryMap(
     ScriptState* scriptState,
     CustomElementsRegistry* registry)
@@ skipping 119 matching lines @@
     const v8::Local<v8::Object>& disconnectedCallback,
     const v8::Local<v8::Object>& attributeChangedCallback,
     const HashSet<AtomicString>& observedAttributes)
     : CustomElementDefinition(descriptor)
     , m_scriptState(scriptState)
     , m_constructor(scriptState->isolate(), constructor)
     , m_observedAttributes(observedAttributes)
 {
 }
 
+HTMLElement* ScriptCustomElementDefinition::createElementByRunningConstructor(
+    Document& document, const QualifiedName& tagName,
+    ExceptionState& exceptionState)
+{
+    // 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;
+    {
+        v8::TryCatch tryCatch(m_scriptState->isolate());
+        element = runConstructor();
+        if (tryCatch.HasCaught()) {
+            exceptionState.rethrowV8Exception(tryCatch.Exception());
+            return nullptr;
+        }
+    }
+
+    // 6.1.3. through 6.1.9.
+    errorForConstructorResult(element, document, tagName, exceptionState);

[dominicc (has gone to gerrit), 2016/06/10 06:13:28]

Could we call this checkConstructorResult or something, like the pattern in
CustomElementConstructorBuilder::check*. This really does the check and
schedules the exception.

[kojii, 2016/06/10 19:01:45]

Done.

+    if (exceptionState.hadException())
+        return nullptr;
+
+    DCHECK_EQ(element->getCustomElementState(), CustomElementState::Custom);
+    return toHTMLElement(element);
+}
+
+HTMLElement* ScriptCustomElementDefinition::createElementByRunningConstructor(
+    Document& document, const QualifiedName& tagName)
+{
+    // 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, 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.
+    v8::Isolate* isolate = m_scriptState->isolate();

[dominicc (has gone to gerrit), 2016/06/10 06:13:28]

When the parser calls this there will not be a handle scope on the stack.
TryCatch, ExceptionState, etc. use handles; they need a handle scope. I think
you need a ScriptState::Scope here.

I'm not sure if we need to check contextIsValid here. Let's see what haraken
says. Remember the document/window this definition comes from could be
completely unrelated to the one the parser is from.

[kojii, 2016/06/10 19:01:44]

Done.

+    v8::TryCatch tryCatch(isolate);
+    tryCatch.SetVerbose(true);
+    ExceptionState exceptionState(ExceptionState::ConstructionContext, "CustomElement", constructor(), isolate);
+
+    HTMLElement* element = createElementByRunningConstructor(document, tagName, exceptionState);
+    if (!exceptionState.throwIfNeeded() && element && !tryCatch.HasCaught())

[dominicc (has gone to gerrit), 2016/06/10 06:13:28]

exceptionState.throwIfNeeded() will set tryCatch.HasCaught(). It might be more
readable to just have

// (*)
exceptionState.throwIfNeeded();
if (element && !tryCatch.HasCaught())
  ...

In theory, tryCatch.HasCaught() should be false at (*), because runConstructor
should catch the exception and use rethrowV8Exception to transmit it, *not* V8
try-catch handlers. Could you add that assertion?

[kojii, 2016/06/10 19:01:44]

Done.

+        return element;
+
+    element = HTMLUnknownElement::create(tagName, document);
+    element->setCustomElementState(CustomElementState::Undefined);
+    return element;

[dominicc (has gone to gerrit), 2016/06/10 06:13:28]

Maybe reversing the condition and putting HTMLUnknownElement::create in the
branch will eliminate the duplication of return element.

[kojii, 2016/06/10 19:01:45]

Done.

+}
+
 // https://html.spec.whatwg.org/multipage/scripting.html#upgrades
 bool ScriptCustomElementDefinition::runConstructor(Element* element)
 {
     if (!m_scriptState->contextIsValid())
         return false;
-    ScriptState::Scope scope(m_scriptState.get());
     v8::Isolate* isolate = m_scriptState->isolate();
 
     // Step 5 says to rethrow the exception; but there is no one to
     // catch it. The side effect is to report the error.
     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(),
                 InvalidStateError,
                 "custom element constructors must call super() first and must "
                 "not return a different object",
                 constructor()),
             m_scriptState->isolate());
         return false;
     }
 
     return true;
 }
 
+Element* ScriptCustomElementDefinition::runConstructor()
+{
+    if (!m_scriptState->contextIsValid())

[dominicc (has gone to gerrit), 2016/06/10 06:13:28]

I'm not sure this is right; all the calls are from
ScriptCustomElementDefinition. The callers should have entered the scope. A
pretty sure sign is you're relying on the caller to have a TryCatch to catch
your exception from v8Call--it must have a scope to do that.

[kojii, 2016/06/10 19:01:45]

Removed and the one above revived. Started to understand what the Scope is...

+        return nullptr;
+    ScriptState::Scope scope(m_scriptState.get());
+    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());
     return m_constructor.newLocal(m_scriptState->isolate());
 }
 
 v8::Local<v8::Object> ScriptCustomElementDefinition::prototype() const
 {
     DCHECK(!m_prototype.isEmpty());
     return m_prototype.newLocal(m_scriptState->isolate());
 }
 
 // CustomElementDefinition
 ScriptValue ScriptCustomElementDefinition::getConstructorForScript()
 {
     return ScriptValue(m_scriptState.get(), constructor());
 }
 
 } // namespace blink