Custom Elements: Check Definition in createElement, Create Customized Built-in Elements Sync

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

Index: third_party/WebKit/Source/core/dom/custom/CustomElement.cpp
diff --git a/third_party/WebKit/Source/core/dom/custom/CustomElement.cpp b/third_party/WebKit/Source/core/dom/custom/CustomElement.cpp
index c86abfe261a64bc17246a54fb299ed9729dde3cd..3216d4d443a388c97edb34b1872179e623a19cca 100644
--- a/third_party/WebKit/Source/core/dom/custom/CustomElement.cpp
+++ b/third_party/WebKit/Source/core/dom/custom/CustomElement.cpp
@@ -4,6 +4,8 @@
 
 #include "core/dom/custom/CustomElement.h"
 
+#include "core/HTMLElementFactory.h"
+#include "core/HTMLElementTypeHelpers.h"
 #include "core/dom/Document.h"
 #include "core/dom/QualifiedName.h"
 #include "core/dom/custom/CEReactionsScope.h"
@@ -79,21 +81,38 @@ static CustomElementDefinition* definitionFor(
 
 HTMLElement* CustomElement::createCustomElementSync(
     Document& document,
-    const AtomicString& localName) {
+    const AtomicString& localName,
+    const AtomicString& is) {
   return createCustomElementSync(
       document,
-      QualifiedName(nullAtom, localName, HTMLNames::xhtmlNamespaceURI));
+      QualifiedName(nullAtom, localName, HTMLNames::xhtmlNamespaceURI), is);
 }
 
 HTMLElement* CustomElement::createCustomElementSync(
     Document& document,
-    const QualifiedName& tagName) {
-  DCHECK(shouldCreateCustomElement(tagName));
-  if (CustomElementDefinition* definition = definitionFor(
-          document,
-          CustomElementDescriptor(tagName.localName(), tagName.localName())))
-    return definition->createElementSync(document, tagName);
-  return createUndefinedElement(document, tagName);
+    const QualifiedName& tagName,
+    const AtomicString& is) {
+  const AtomicString& name = is.isNull() ? tagName.localName() : is;
+  DCHECK(shouldCreateCustomElement(name));
+  const CustomElementDescriptor& desc =

[dominicc (has gone to gerrit), 2016/11/07 01:59:00]

I think you could just make this

const CustomElementDescriptor desc(name, tagName.localName());

Hmm, do you think it is awkward to have createElement take tag name/is, but have
CustomElementDescriptor take custom element name/tag name? Maybe we should swap
the parameters to CustomElementDescriptor. Don't worry about it in this
changelist though.

+      CustomElementDescriptor(name, tagName.localName());
+  CustomElementDefinition* definition = definitionFor(document, desc);
+  HTMLElement* element;
+
+  if (definition && desc.isAutonomous()) {
+    // 6. If definition is non-null and we have an autonomous custom element

[dominicc (has gone to gerrit), 2016/11/07 01:59:00]

Add a link to the spec this is implementing to give these comments context.

+    element = definition->createElementSync(document, tagName);
+    element->setAttribute(HTMLNames::isAttr, is);

[dominicc (has gone to gerrit), 2016/11/07 01:59:00]

Hmm, this branch doesn't upgrade. When do these get upgraded?

+  } else if (definition) {
+    // 5. If definition is non-null and we have a customized built-in element
+    element = createUndefinedElement(document, tagName);
+    element->setAttribute(HTMLNames::isAttr, name);
+    definition->upgrade(element);
+  } else {
+    // 7. Otherwise
+    element = createUndefinedElement(document, tagName);
+  }
+  return element;
 }
 
 HTMLElement* CustomElement::createCustomElementAsync(
@@ -113,10 +132,14 @@ HTMLElement* CustomElement::createCustomElementAsync(
   return createUndefinedElement(document, tagName);
 }
 
+// Create a HTMLElement
 HTMLElement* CustomElement::createUndefinedElement(
     Document& document,
     const QualifiedName& tagName) {
-  DCHECK(shouldCreateCustomElement(tagName));
+  int builtin = htmlElementTypeForTag(tagName.localName()) !=

[dominicc (has gone to gerrit), 2016/11/07 01:59:00]

int -> bool

Maybe add a word or two to give this a slightly more descriptive name? Because
it checks the runtime flag as well as the tag name, maybe shouldCreateBuiltin or
something?

+                    HTMLElementType::kHTMLUnknownElement &&
+                RuntimeEnabledFeatures::customElementsBuiltinEnabled();
+  DCHECK(shouldCreateCustomElement(tagName) || builtin);
 
   HTMLElement* element;
   if (V0CustomElement::isValidName(tagName.localName()) &&
@@ -125,6 +148,9 @@ HTMLElement* CustomElement::createUndefinedElement(
         document, tagName);
     SECURITY_DCHECK(v0element->isHTMLElement());
     element = toHTMLElement(v0element);
+  } else if (builtin) {
+    element = HTMLElementFactory::createHTMLElement(
+        tagName.localName(), document, 0, CreatedByCreateElement);

[dominicc (has gone to gerrit), 2016/11/07 01:59:00]

There's lots of code running around passing 0 for null, but IIRC we write
nullptr now for null pointers.

See https://www.chromium.org/blink/coding-style#TOC-Null-false-and-0

   } else {
     element = HTMLElement::create(tagName, document);

[dominicc (has gone to gerrit), 2016/11/07 01:59:00]

We're going to end up in a situation where some people are using
document.register and customElements.define in the same page for a while, and we
need them to interact cleanly until we can remove document.registerEntirely.

Does this affect the tests in LayoutTests/fast/dom/custom? I'm not sure how
these code paths interact at this point.

   }