Add callbacks to ScriptCustomElementDefinition

This patch adds callbacks and observedAttributes to
ScriptCustomElementDefinition, as defined in the Element Definition
spec[1].

[1] https://html.spec.whatwg.org/multipage/scripting.html#element-definition

BUG=594918
TEST=imported/wpt/custom-elements/custom-elements-registry/define.html

Review-Url: https://codereview.chromium.org/2024073002
Cr-Commit-Position: refs/heads/master@{#398243}

third_party/WebKit/LayoutTests/imported/wpt/custom-elements/custom-elements-registry/define-expected.txt

@@ -73,53 +73,27 @@ FAIL If extends is spacer, should throw a NotSupportedError assert_throws: funct
 FAIL If extends is elementnametobeunknownelement, should throw a NotSupportedError assert_throws: function "() =>  {
         customElements.define('test-define-extend-' + name, class {}, { extends: name });
       }" did not throw
-FAIL If constructor.observedAttributes throws, should rethrow assert_throws: function "() => {
-      customElements.define('test-define-observedattributes-rethrow', C);
-    }" did not throw
+PASS If constructor.observedAttributes throws, should rethrow 
 PASS If constructor.prototype throws, should rethrow 
 PASS If Type(constructor.prototype) is undefined, should throw a TypeError 
 PASS If Type(constructor.prototype) is string, should throw a TypeError 
-FAIL If constructor.prototype.connectedCallback throws, should rethrow assert_throws: function "() => {
-        customElements.define(`test-define-${name.toLowerCase()}-rethrow`, C);
-      }" did not throw
+PASS If constructor.prototype.connectedCallback throws, should rethrow 
 PASS If constructor.prototype.connectedCallback is undefined, should succeed 
 PASS If constructor.prototype.connectedCallback is function, should succeed 
-FAIL If constructor.prototype.connectedCallback is null, should throw a TypeError assert_throws: function "() => {
-            customElements.define(`test-define-${name.toLowerCase()}-${data.name}`, C);
-          }" did not throw
-FAIL If constructor.prototype.connectedCallback is object, should throw a TypeError assert_throws: function "() => {
-            customElements.define(`test-define-${name.toLowerCase()}-${data.name}`, C);
-          }" did not throw
-FAIL If constructor.prototype.connectedCallback is integer, should throw a TypeError assert_throws: function "() => {
-            customElements.define(`test-define-${name.toLowerCase()}-${data.name}`, C);
-          }" did not throw
-FAIL If constructor.prototype.disconnectedCallback throws, should rethrow assert_throws: function "() => {
-        customElements.define(`test-define-${name.toLowerCase()}-rethrow`, C);
-      }" did not throw
+PASS If constructor.prototype.connectedCallback is null, should throw a TypeError 
+PASS If constructor.prototype.connectedCallback is object, should throw a TypeError 
+PASS If constructor.prototype.connectedCallback is integer, should throw a TypeError 
+PASS If constructor.prototype.disconnectedCallback throws, should rethrow 
 PASS If constructor.prototype.disconnectedCallback is undefined, should succeed 
 PASS If constructor.prototype.disconnectedCallback is function, should succeed 
-FAIL If constructor.prototype.disconnectedCallback is null, should throw a TypeError assert_throws: function "() => {
-            customElements.define(`test-define-${name.toLowerCase()}-${data.name}`, C);
-          }" did not throw
-FAIL If constructor.prototype.disconnectedCallback is object, should throw a TypeError assert_throws: function "() => {
-            customElements.define(`test-define-${name.toLowerCase()}-${data.name}`, C);
-          }" did not throw
-FAIL If constructor.prototype.disconnectedCallback is integer, should throw a TypeError assert_throws: function "() => {
-            customElements.define(`test-define-${name.toLowerCase()}-${data.name}`, C);
-          }" did not throw
-FAIL If constructor.prototype.attributeChangedCallback throws, should rethrow assert_throws: function "() => {
-        customElements.define(`test-define-${name.toLowerCase()}-rethrow`, C);
-      }" did not throw
+PASS If constructor.prototype.disconnectedCallback is null, should throw a TypeError 
+PASS If constructor.prototype.disconnectedCallback is object, should throw a TypeError 
+PASS If constructor.prototype.disconnectedCallback is integer, should throw a TypeError 
+PASS If constructor.prototype.attributeChangedCallback throws, should rethrow 
 PASS If constructor.prototype.attributeChangedCallback is undefined, should succeed 
 PASS If constructor.prototype.attributeChangedCallback is function, should succeed 
-FAIL If constructor.prototype.attributeChangedCallback is null, should throw a TypeError assert_throws: function "() => {
-            customElements.define(`test-define-${name.toLowerCase()}-${data.name}`, C);
-          }" did not throw
-FAIL If constructor.prototype.attributeChangedCallback is object, should throw a TypeError assert_throws: function "() => {
-            customElements.define(`test-define-${name.toLowerCase()}-${data.name}`, C);
-          }" did not throw
-FAIL If constructor.prototype.attributeChangedCallback is integer, should throw a TypeError assert_throws: function "() => {
-            customElements.define(`test-define-${name.toLowerCase()}-${data.name}`, C);
-          }" did not throw
+PASS If constructor.prototype.attributeChangedCallback is null, should throw a TypeError 
+PASS If constructor.prototype.attributeChangedCallback is object, should throw a TypeError 
+PASS If constructor.prototype.attributeChangedCallback is integer, should throw a TypeError 
 Harness: the test ran to completion.
 

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

@@ -80,19 +80,41 @@ ScriptCustomElementDefinition* ScriptCustomElementDefinition::forConstructor(
     return static_cast<ScriptCustomElementDefinition*>(definition);
 }
 
+static void keepAlive(v8::Local<v8::Array>& array, uint32_t index,
+    const v8::Local<v8::Object>& value,
+    ScopedPersistent<v8::Object>& persistent,
+    ScriptState* scriptState)
+{
+    if (value.IsEmpty())
+        return;
+
+    v8CallOrCrash(array->Set(scriptState->context(), index, value));
+
+    persistent.set(scriptState->isolate(), value);
+    persistent.setPhantom();
+}
+
 ScriptCustomElementDefinition* ScriptCustomElementDefinition::create(
     ScriptState* scriptState,
     CustomElementsRegistry* registry,
     const CustomElementDescriptor& descriptor,
     const v8::Local<v8::Object>& constructor,
-    const v8::Local<v8::Object>& prototype)
+    const v8::Local<v8::Object>& prototype,
+    const v8::Local<v8::Object>& connectedCallback,
+    const v8::Local<v8::Object>& disconnectedCallback,
+    const v8::Local<v8::Object>& attributeChangedCallback,
+    const HashSet<AtomicString>& observedAttributes)
 {
     ScriptCustomElementDefinition* definition =
         new ScriptCustomElementDefinition(
             scriptState,
             descriptor,
             constructor,
-            prototype);
+            prototype,
+            connectedCallback,
+            disconnectedCallback,
+            attributeChangedCallback,
+            observedAttributes);
 
     // Add a constructor -> name mapping to the registry.
     v8::Local<v8::Value> nameValue =
@@ -100,11 +122,16 @@ ScriptCustomElementDefinition* ScriptCustomElementDefinition::create(
     v8::Local<v8::Map> map =
         ensureCustomElementsRegistryMap(scriptState, registry);
     v8CallOrCrash(map->Set(scriptState->context(), constructor, nameValue));
-    // We add the prototype here to keep it alive; we make it a value
-    // not a key so authors cannot return another constructor as a
-    // prototype to overwrite a constructor in this map. We use the
-    // name because it is unique per-registry.
-    v8CallOrCrash(map->Set(scriptState->context(), nameValue, prototype));
+    definition->m_constructor.setPhantom();
+
+    // We add the prototype and callbacks here to keep them alive. We use the
+    // name as the key because it is unique per-registry.
+    v8::Local<v8::Array> array = v8::Array::New(scriptState->isolate(), 4);
+    keepAlive(array, 0, prototype, definition->m_prototype, scriptState);
+    keepAlive(array, 1, connectedCallback, definition->m_connectedCallback, scriptState);
+    keepAlive(array, 2, disconnectedCallback, definition->m_disconnectedCallback, scriptState);
+    keepAlive(array, 3, attributeChangedCallback, definition->m_attributeChangedCallback, scriptState);
+    v8CallOrCrash(map->Set(scriptState->context(), nameValue, array));
 
     return definition;
 }
@@ -113,17 +140,16 @@ ScriptCustomElementDefinition::ScriptCustomElementDefinition(
     ScriptState* scriptState,
     const CustomElementDescriptor& descriptor,
     const v8::Local<v8::Object>& constructor,
-    const v8::Local<v8::Object>& prototype)
+    const v8::Local<v8::Object>& prototype,
+    const v8::Local<v8::Object>& connectedCallback,
+    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_prototype(scriptState->isolate(), prototype)
+    , m_observedAttributes(observedAttributes)
 {
-    // These objects are kept alive by references from the
-    // CustomElementsRegistry wrapper set up by
-    // ScriptCustomElementDefinition::create.
-    m_constructor.setPhantom();
-    m_prototype.setPhantom();
 }
 
 // https://html.spec.whatwg.org/multipage/scripting.html#upgrades

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

@@ -10,8 +10,11 @@
 #include "core/CoreExport.h"
 #include "core/dom/custom/CustomElementDefinition.h"
 #include "v8.h"
+#include "wtf/HashSet.h"
 #include "wtf/Noncopyable.h"
 #include "wtf/RefPtr.h"
+#include "wtf/text/AtomicString.h"
+#include "wtf/text/AtomicStringHash.h"
 
 namespace blink {
 
@@ -32,7 +35,11 @@ public:
         CustomElementsRegistry*,
         const CustomElementDescriptor&,
         const v8::Local<v8::Object>& constructor,
-        const v8::Local<v8::Object>& prototype);
+        const v8::Local<v8::Object>& prototype,
+        const v8::Local<v8::Object>& connectedCallback,
+        const v8::Local<v8::Object>& disconnectedCallback,
+        const v8::Local<v8::Object>& attributeChangedCallback,
+        const HashSet<AtomicString>& observedAttributes);
 
     virtual ~ScriptCustomElementDefinition() = default;
 
@@ -44,7 +51,11 @@ private:
         ScriptState*,
         const CustomElementDescriptor&,
         const v8::Local<v8::Object>& constructor,
-        const v8::Local<v8::Object>& prototype);
+        const v8::Local<v8::Object>& prototype,
+        const v8::Local<v8::Object>& connectedCallback,
+        const v8::Local<v8::Object>& disconnectedCallback,
+        const v8::Local<v8::Object>& attributeChangedCallback,
+        const HashSet<AtomicString>& observedAttributes);
 
     // Implementations of |CustomElementDefinition|
     ScriptValue getConstructorForScript() final;
@@ -53,6 +64,10 @@ private:
     RefPtr<ScriptState> m_scriptState;
     ScopedPersistent<v8::Object> m_constructor;
     ScopedPersistent<v8::Object> m_prototype;
+    ScopedPersistent<v8::Object> m_connectedCallback;
+    ScopedPersistent<v8::Object> m_disconnectedCallback;
+    ScopedPersistent<v8::Object> m_attributeChangedCallback;
+    HashSet<AtomicString> m_observedAttributes;
 };
 
 } // namespace blink

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

@@ -81,17 +81,26 @@ bool ScriptCustomElementDefinitionBuilder::checkConstructorNotRegistered()
     return true;
 }
 
-bool ScriptCustomElementDefinitionBuilder::checkPrototype()
+bool ScriptCustomElementDefinitionBuilder::valueForName(
+    const v8::Local<v8::Object>& object, const String& name,
+    v8::Local<v8::Value>& value) const
 {
     v8::Isolate* isolate = m_scriptState->isolate();
     v8::Local<v8::Context> context = m_scriptState->context();
-    v8::Local<v8::String> prototypeString =
-        v8AtomicString(isolate, "prototype");
-    v8::Local<v8::Value> prototypeValue;
-    if (!v8Call(
-        m_constructor->Get(context, prototypeString), prototypeValue)) {
+    v8::Local<v8::String> nameString = v8String(isolate, name);
+    v8::TryCatch tryCatch(isolate);
+    if (!v8Call(object->Get(context, nameString), value, tryCatch)) {
+        m_exceptionState.rethrowV8Exception(tryCatch.Exception());
         return false;
     }
+    return true;
+}
+
+bool ScriptCustomElementDefinitionBuilder::checkPrototype()
+{
+    v8::Local<v8::Value> prototypeValue;
+    if (!valueForName(m_constructor, "prototype", prototypeValue))
+        return false;
     if (!prototypeValue->IsObject()) {
         m_exceptionState.throwTypeError(
             "constructor prototype is not an object");
@@ -103,6 +112,62 @@ bool ScriptCustomElementDefinitionBuilder::checkPrototype()
     return true;
 }
 
+bool ScriptCustomElementDefinitionBuilder::callableForName(const String& name,
+    v8::Local<v8::Object>& callback) const
+{
+    v8::Local<v8::Value> value;
+    if (!valueForName(m_prototype, name, value))
+        return false;
+    // "undefined" means "omitted", so return true.
+    if (value->IsUndefined())
+        return true;
+    if (!value->IsObject()) {
+        m_exceptionState.throwTypeError(
+            String::format("\"%s\" is not an object", name.ascii().data()));
+        return false;
+    }
+    callback = value.As<v8::Object>();
+    if (!callback->IsCallable()) {
+        m_exceptionState.throwTypeError(
+            String::format("\"%s\" is not callable", name.ascii().data()));
+        return false;
+    }
+    return true;
+}
+
+bool ScriptCustomElementDefinitionBuilder::retrieveObservedAttributes()
+{
+    const String kObservedAttributes = "observedAttributes";
+    v8::Local<v8::Value> observedAttributesValue;
+    if (!valueForName(m_constructor, kObservedAttributes, observedAttributesValue))
+        return false;
+    if (observedAttributesValue->IsUndefined())
+        return true;
+    Vector<AtomicString> list = toImplArray<Vector<AtomicString>>(
+        observedAttributesValue, 0, m_scriptState->isolate(), m_exceptionState);
+    if (m_exceptionState.hadException())
+        return false;
+    if (list.isEmpty())
+        return true;
+    m_observedAttributes.reserveCapacityForSize(list.size());
+    for (const auto& attribute : list)
+        m_observedAttributes.add(attribute);
+    return true;
+}
+
+bool ScriptCustomElementDefinitionBuilder::rememberOriginalProperties()
+{
+    // Spec requires to use values of these properties at the point
+    // CustomElementDefinition is built, even if JS changes them afterwards.
+    const String kConnectedCallback = "connectedCallback";
+    const String kDisconnectedCallback = "disconnectedCallback";
+    const String kAttributeChangedCallback = "attributeChangedCallback";
+    return retrieveObservedAttributes()
+        && callableForName(kConnectedCallback, m_connectedCallback)
+        && callableForName(kDisconnectedCallback, m_disconnectedCallback)
+        && callableForName(kAttributeChangedCallback, m_attributeChangedCallback);
+}
+
 CustomElementDefinition* ScriptCustomElementDefinitionBuilder::build(
     const CustomElementDescriptor& descriptor)
 {
@@ -111,7 +176,11 @@ CustomElementDefinition* ScriptCustomElementDefinitionBuilder::build(
         m_registry,
         descriptor,
         m_constructor,
-        m_prototype);
+        m_prototype,
+        m_connectedCallback,
+        m_disconnectedCallback,
+        m_attributeChangedCallback,
+        m_observedAttributes);
 }
 
 } // namespace blink

third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.h

@@ -10,8 +10,11 @@
 #include "platform/heap/Handle.h"
 #include "v8.h"
 #include "wtf/Allocator.h"
+#include "wtf/HashSet.h"
 #include "wtf/Noncopyable.h"
 #include "wtf/RefPtr.h"
+#include "wtf/text/AtomicString.h"
+#include "wtf/text/AtomicStringHash.h"
 
 namespace blink {
 
@@ -35,6 +38,7 @@ public:
     bool checkConstructorIntrinsics() override;
     bool checkConstructorNotRegistered() override;
     bool checkPrototype() override;
+    bool rememberOriginalProperties() override;
     CustomElementDefinition* build(const CustomElementDescriptor&) override;
 
 private:
@@ -46,7 +50,15 @@ private:
     v8::Local<v8::Value> m_constructorValue;
     v8::Local<v8::Object> m_constructor;
     v8::Local<v8::Object> m_prototype;
+    v8::Local<v8::Object> m_connectedCallback;
+    v8::Local<v8::Object> m_disconnectedCallback;
+    v8::Local<v8::Object> m_attributeChangedCallback;
+    HashSet<AtomicString> m_observedAttributes;
     ExceptionState& m_exceptionState;
+
+    bool valueForName(const v8::Local<v8::Object>&, const String&, v8::Local<v8::Value>&) const;
+    bool callableForName(const String&, v8::Local<v8::Object>&) const;
+    bool retrieveObservedAttributes();
 };
 
 } // namespace blink

third_party/WebKit/Source/bindings/core/v8/V8Binding.h

@@ -774,6 +774,17 @@ struct NativeValueTraits<String> {
     }
 };
 
+template<>
+struct NativeValueTraits<AtomicString> {
+    static inline AtomicString nativeValue(v8::Isolate* isolate, v8::Local<v8::Value> value, ExceptionState& exceptionState)
+    {
+        V8StringResource<> stringValue(value);
+        if (!stringValue.prepare(exceptionState))
+            return AtomicString();
+        return stringValue;
+    }
+};
+
 template<>
 struct NativeValueTraits<int> {
     static inline int nativeValue(v8::Isolate* isolate, v8::Local<v8::Value> value, ExceptionState& exceptionState)

third_party/WebKit/Source/core/dom/custom/CustomElementDefinitionBuilder.h

@@ -43,6 +43,10 @@ public:
     // processing should not proceed.
     virtual bool checkPrototype() = 0;
 
+    // Cache properties for build to use. Return false if processing
+    // should not proceed.
+    virtual bool rememberOriginalProperties() = 0;
+
     // Produce the definition. This must produce a definition.
     virtual CustomElementDefinition* build(const CustomElementDescriptor&) = 0;
 };

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

@@ -123,7 +123,10 @@ void CustomElementsRegistry::define(
     // TODO(dominicc): Implement steps:
     // 5: localName
     // 6-7: extends processing
-    // 8-9: observed attributes caching
+
+    // 8-9: observed attributes caching is done below, together with callbacks.
+    // TODO(kojii): https://github.com/whatwg/html/issues/1373 for the ordering.
+    // When it's resolved, revisit if this code needs changes.
 
     // TODO(dominicc): Add a test where the prototype getter destroys
     // the context.
@@ -131,11 +134,14 @@ void CustomElementsRegistry::define(
     if (!builder.checkPrototype())
         return;
 
-    // TODO(dominicc): Implement steps:
+    // 8-9: observed attributes caching
     // 12-13: connected callback
     // 14-15: disconnected callback
     // 16-17: attribute changed callback
 
+    if (!builder.rememberOriginalProperties())
+        return;
+
     // TODO(dominicc): Add a test where retrieving the prototype
     // recursively calls define with the same name.
 

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

@@ -286,6 +286,7 @@ public:
     bool checkConstructorIntrinsics() override { return true; }
     bool checkConstructorNotRegistered() override { return true; }
     bool checkPrototype() override { return true; }
+    bool rememberOriginalProperties() override { return true; }
     CustomElementDefinition* build(
         const CustomElementDescriptor& descriptor) {
         return new LogUpgradeDefinition(descriptor);