Add callbacks to ScriptCustomElementDefinition

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 879fac4ecdfd1b38f444b0883ebaad761c1bde21..443eea7211cec7db7e301fd032aba32fa827be26 100644
--- a/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
+++ b/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
@@ -4,6 +4,7 @@
 
 #include "bindings/core/v8/ScriptCustomElementDefinition.h"
 
+#include "bindings/core/v8/ScriptCustomElementDefinitionBuilder.h"
 #include "bindings/core/v8/ScriptState.h"
 #include "bindings/core/v8/V8Binding.h"
 #include "bindings/core/v8/V8CustomElementsRegistry.h"
@@ -74,50 +75,69 @@ ScriptCustomElementDefinition* ScriptCustomElementDefinition::forConstructor(
     return static_cast<ScriptCustomElementDefinition*>(definition);
 }
 
+static void keepAliveIfNotEmpty(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 ScriptCustomElementDefinitionBuilder& builder)
 {
     ScriptCustomElementDefinition* definition =
-        new ScriptCustomElementDefinition(
-            scriptState,
-            descriptor,
-            constructor,
-            prototype);
+        new ScriptCustomElementDefinition(descriptor, builder);
 
     // Add a constructor -> name mapping to the registry.
+    ScriptState* scriptState = builder.m_scriptState.get();
     v8::Local<v8::Value> nameValue =
         v8String(scriptState->isolate(), descriptor.name());
     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));
+        ensureCustomElementsRegistryMap(scriptState, builder.m_registry);
+    v8CallOrCrash(map->Set(scriptState->context(), builder.m_constructor, nameValue));
+
+    // 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);

[yosin_UTC9, 2016/06/02 01:33:37]

I'll add "whenDefinedPromise" here, so JS side value holding and C++ cache
should be described in one place.

+    v8CallOrCrash(array->Set(scriptState->context(), 0, builder.m_prototype));

[dominicc (has gone to gerrit), 2016/06/02 00:31:27]

Why not use keepAliveIfNotEmpty for m_prototype too? It won't be empty but
superfluous test is probably worth it to keep things consistent.

[kojii, 2016/06/02 03:18:08]

Done.

+#define V(index, name) \

[yosin_UTC9, 2016/06/02 01:33:38]

We should have FOR_EACH_XXX() and it should be used in other places, e.g. member
variable declaration and so on.
Also, we should avoid macro expansion to generate code for one time usage.
FOR_EACH_XXX() is a pattern utilized in v8, dart and Blink too.

V8_HIDDEN_VALUES()
https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So...

In v8: 
https://code.google.com/p/chromium/codesearch#search/&q=FOR_EACH%20file:%5Esr...

[kojii, 2016/06/02 03:18:08]

I know, but I don't like it, esp. when the list is only 4. Let's worry about it
when the list gets long enough to overcome the cost of readability.

[yosin_UTC9, 2016/06/02 04:30:31]

Since list is used for parameter of ctor, it might be 4 until spec is changed.
But, size of Array soon to be 5, for |promiseWhenDefined|.
Per isolation of CL's, I'm OK to rewrite this code when I add
|promiseWhenDefined|.

+    keepAliveIfNotEmpty(array, index, builder.m_##name, definition->m_##name, scriptState)
+    V(1, connectedCallback);
+    V(2, disconnectedCallback);
+    V(3, attributeChangedCallback);
+#undef V
+    v8CallOrCrash(map->Set(scriptState->context(), nameValue, array));
 
     return definition;
 }
 
 ScriptCustomElementDefinition::ScriptCustomElementDefinition(
-    ScriptState* scriptState,
     const CustomElementDescriptor& descriptor,
-    const v8::Local<v8::Object>& constructor,
-    const v8::Local<v8::Object>& prototype)
+    const ScriptCustomElementDefinitionBuilder& builder)
     : CustomElementDefinition(descriptor)
-    , m_scriptState(scriptState)
-    , m_constructor(scriptState->isolate(), constructor)
-    , m_prototype(scriptState->isolate(), prototype)
+    , m_scriptState(builder.m_scriptState)
+    , m_constructor(builder.m_scriptState->isolate(), builder.m_constructor)

[dominicc (has gone to gerrit), 2016/06/02 00:31:27]

I guess you could just use m_scriptState at this point but either way is OK.

[kojii, 2016/06/02 03:18:08]

This was reverted, see comments in ScriptCustomElementDefinitionBuilder.h.

+    , m_prototype(builder.m_scriptState->isolate(), builder.m_prototype)
 {
     // These objects are kept alive by references from the
     // CustomElementsRegistry wrapper set up by
     // ScriptCustomElementDefinition::create.
     m_constructor.setPhantom();
     m_prototype.setPhantom();
+
+    if (builder.m_observedAttributes.isEmpty())
+        return;
+    m_observedAttributes.reserveCapacityForSize(builder.m_observedAttributes.size());
+    for (const auto& attribute : builder.m_observedAttributes)
+        m_observedAttributes.add(attribute);
 }
 
 v8::Local<v8::Object> ScriptCustomElementDefinition::constructor() const