Implement custom element construction and some 'define' checks

third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp

Index: third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp
diff --git a/third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp b/third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp
index f22190cea9e0545388421215b5afaafbf2006b51..27f52803b8f9cd6cae80eb79ed97bb03a0d416c4 100644
--- a/third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp
+++ b/third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp
@@ -4,26 +4,163 @@
 
 #include "core/dom/custom/CustomElementsRegistry.h"
 
+// TODO(dominicc): Stop including Document.h when
+// v0CustomElementIsDefined has been removed.
+#include "bindings/core/v8/DOMWrapperWorld.h"
 #include "bindings/core/v8/ExceptionState.h"
 #include "bindings/core/v8/ScriptState.h"
+#include "bindings/core/v8/ScriptValue.h"
+#include "bindings/core/v8/V8Binding.h"
+#include "core/dom/Document.h"
 #include "core/dom/ElementRegistrationOptions.h"
+#include "core/dom/ExceptionCode.h"
+#include "core/dom/custom/CustomElement.h"
+#include "core/dom/custom/CustomElementDefinition.h"
+#include "core/dom/custom/V0CustomElementRegistrationContext.h"
+#include "core/dom/custom/V0CustomElementRegistry.h"
 
 namespace blink {
 
-CustomElementsRegistry::CustomElementsRegistry()
+CustomElementsRegistry* CustomElementsRegistry::create(
+    ScriptState* scriptState)
 {
+    if (!scriptState)

[haraken, 2016/05/10 09:04:22]

This shouldn't happen.

Also what you really need to check is:

  if (!scriptState->contextIsValid())
    return;

but it shouldn't happen either. Even if it happens it will hit the
ASSERT(scriptState->contextIsValid()) in ScriptState::Scope.

In summary, you can remove this check.

[dominicc (has gone to gerrit), 2016/05/11 02:31:49]

Hmm, what happens if a script removes its own window; is the context still
valid?

+        return nullptr;
+    ScriptState::Scope scope(scriptState);

[haraken, 2016/05/10 09:04:22]

BTW, CustomElementsRegistry::create is called only by JS via the binding layer,
right? Then it's already guaranteed that you're in a correct ScriptState. So you
don't need to enter the ScriptState::Scope.

[dominicc (has gone to gerrit), 2016/05/11 02:31:49]

This object is special: it sets up a map (NativeWeakMap) of constructors to
element names. That needs to live as long as an the main world can run script.
So I wanted to create that object in the main world. If I do this in the calling
ScriptState, if an extension called window.customElements and then the extension
isolated world was killed, what would happen when the main world manipulated the
map?

+    DCHECK(DOMWrapperWorld::current(scriptState->isolate()).isMainWorld());

[haraken, 2016/05/10 09:04:22]

Use scriptState->world().

+    return new CustomElementsRegistry(scriptState->isolate());
 }
 
+CustomElementsRegistry::CustomElementsRegistry(v8::Isolate* isolate)
+    : m_constructorIdMap(isolate, v8::NativeWeakMap::New(isolate))
+{
+}
+
+static bool v0CustomElementIsDefined(
+    v8::Isolate* isolate,
+    const AtomicString& name)
+{
+    Document* doc = toDocument(currentExecutionContext(isolate));

[haraken, 2016/05/10 09:04:22]

Can we pass in a ScriptState to v0CustomElementIsDefined?

currentXXX methods have a risk of looking up a wrong thing unless you're careful
about all the call sites. To mitigate the risk, it's better to explicitly pass
in a ScriptState that should be used.

+    V0CustomElementRegistrationContext* v0Context = doc->registrationContext();
+    return v0Context && v0Context->registry().nameIsDefined(name);
+}
+
+// http://w3c.github.io/webcomponents/spec/custom/#dfn-element-definition
 void CustomElementsRegistry::define(ScriptState* scriptState,
     const AtomicString& name, const ScriptValue& constructor,
     const ElementRegistrationOptions& options, ExceptionState& exceptionState)
 {
-    // TODO(kojii): Element definition process not implemented yet.
-    // http://w3c.github.io/webcomponents/spec/custom/#dfn-element-definition
+    if (!DOMWrapperWorld::current(scriptState->isolate()).isMainWorld()) {

[haraken, 2016/05/10 09:04:22]

Use scriptState->world().

+        exceptionState.throwDOMException(
+            NotSupportedError,
+            "defining custom elements from extensions is not supported");
+        return;
+    }
+    v8::Isolate* isolate = scriptState->isolate();
+    v8::Local<v8::Context> context = scriptState->context();
+
+    // TODO(dominicc): Make this check for constructors and not just
+    // functions when
+    // https://bugs.chromium.org/p/v8/issues/detail?id=4993 is fixed.
+    v8::Local<v8::Value> constructorValue = constructor.v8Value();
+    if (!constructorValue->IsFunction()) {
+        exceptionState.throwTypeError(
+            "constructor argument is not a constructor");
+        return;
+    }
+    v8::Local<v8::Object> constructorObject =
+        constructorValue.As<v8::Object>();
+
+    // Raise an exception if the name is not valid.
+    if (!CustomElement::isValidName(name)) {
+        exceptionState.throwSyntaxError(
+            "\"" + name + "\" is not a valid custom element name");
+        return;
+    }
+
+    // Raise an exception if the name is already in use.
+    if (nameIsDefined(name) || v0CustomElementIsDefined(isolate, name)) {
+        exceptionState.throwDOMException(
+            NotSupportedError,
+            "this name has already been used with this registry");
+        return;
+    }
+
+    // Raise an exception if the constructor is already registered.
+    if (definitionForConstructor(context, constructorObject)) {
+        exceptionState.throwDOMException(
+            NotSupportedError,
+            "this constructor has already been used with this registry");
+        return;
+    }
+
+    // TODO(dominicc): Implement steps:
+    // 5: localName
+    // 6-7: extends processing
+    // 8-9: observed attributes caching
+
+    v8::TryCatch tryCatch(isolate);

[haraken, 2016/05/10 09:04:22]

What is this TryCatch for?

[dominicc (has gone to gerrit), 2016/05/11 02:31:49]

I need all of the early returns to be terminating or throwing an exception. With

if (x) {
  exceptionState.throwX()
  return;
}

above it is easy to be confident that happens; but below if retrieving the
prototype fails I wanted to check that it is due to an exception.

+    v8::MaybeLocal<v8::Value> maybePrototype = constructorObject->Get(

[haraken, 2016/05/10 09:04:22]

Avoid using MaybeLocal handles by using macros in V8BindingMacros.h.

+        context,
+        v8AtomicString(isolate, "prototype"));
+    v8::Local<v8::Value> prototypeValue;
+    if (!maybePrototype.ToLocal(&prototypeValue)) {
+        DCHECK(tryCatch.HasCaught());
+        tryCatch.ReThrow();
+        return;
+    }
+    if (!prototypeValue->IsObject()) {
+        DCHECK(!tryCatch.HasCaught());
+        exceptionState.throwTypeError("constructor prototype is not an object");
+        return;
+    }
+    v8::Local<v8::Object> prototype = prototypeValue.As<v8::Object>();
+
+    // TODO(dominicc): Implement steps:
+    // 12-13: connected callback
+    // 14-15: disconnected callback
+    // 16-17: attribute changed callback
+
+    size_t id = m_definitions.size();
+    m_definitions.append(new CustomElementDefinition(name, isolate, prototype));
+    m_constructorIdMap.Get(isolate)->Set(

[haraken, 2016/05/10 09:04:22]

Why is it okay to make the map a weak map? As far as I understand correctly,
this weak map is caching the constructor to avoid the overhead of instantiating
the constructor every time -- right?

If the constructor is exposed to JS, a user can set an expando on the
constructor object. If the map is weak, I'm afraid that the expand can be lost.

[dominicc (has gone to gerrit), 2016/05/11 02:31:49]

Custom elements "v1" is a bit different: the constructor is supplied by the
author, not generated by Blink.

This map is used in the HTMLElement constructor to, given a constructor (via
new.target), find the definition to use for it. So we can look up things like
the tag name.

A WeakMap is a good object to use for this, instead of a private symbol, because
the constructor could have been frozen.
This does not happen because CustomElementsRegistry::visitDOMWrapper keeps the
constructor alive. It adds references to prototypes, which have a strong
reference to the constructor via the 'constructor' property.

[haraken, 2016/05/11 06:25:28]

If you're expecting that the constructor is kept alive in some other way, how
about just using a strong reference here as well? I don't see a benefit of using
a weak map.

(Note that this is the only place in v8 bindings that uses v8::NativeWeakMap.)

+        constructorObject,
+        v8::Integer::NewFromUnsigned(isolate, id));
+    m_names.add(name);
+
+    // TODO(dominicc): Implement steps:
+    // 20: when-defined promise processing
+    DCHECK(!tryCatch.HasCaught() || tryCatch.HasTerminated());
+}
+
+CustomElementDefinition* CustomElementsRegistry::definitionForConstructor(
+    v8::Handle<v8::Context> context,

[haraken, 2016/05/10 09:04:22]

I'd prefer passing in a ScriptState instead of a v8::Context.

+    v8::Handle<v8::Value> constructor)
+{
+    v8::Local<v8::Value> entry =
+        m_constructorIdMap.Get(context->GetIsolate())->Get(constructor);
+    if (!entry->IsUint32())
+        return nullptr;
+    return m_definitions[entry->Uint32Value(context).FromJust()];

[haraken, 2016/05/10 09:04:22]

Use a v8Call macro in V8BindingMacros.h.

+}
+
+bool CustomElementsRegistry::nameIsDefined(const AtomicString& name) const
+{
+    return m_names.contains(name);
+}
+
+void CustomElementsRegistry::setWrapperReferences(
+    v8::Isolate* isolate,
+    const v8::Persistent<v8::Object>& wrapper) const
+{
+    for (const auto& def : m_definitions) {
+        def->setReference(isolate, wrapper);
+    }
 }
 
 DEFINE_TRACE(CustomElementsRegistry)
 {
+    visitor->trace(m_definitions);
 }
 
 } // namespace blink