Upgrade in-document custom elements when an element is defined.

third_party/WebKit/Source/core/dom/custom/CustomElementsRegistryTest.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 "core/dom/custom/CustomElementsRegistry.h"
+
+#include "bindings/core/v8/ExceptionState.h"
+#include "core/dom/Document.h"
+#include "core/dom/Element.h"
+#include "core/dom/ElementRegistrationOptions.h"
+#include "core/dom/custom/CustomElementDefinition.h"
+#include "core/dom/custom/CustomElementDefinitionBuilder.h"
+#include "core/dom/custom/CustomElementDescriptor.h"
+#include "core/dom/custom/CustomElementTestHelpers.h"
+#include "core/dom/shadow/ShadowRoot.h"
+#include "core/dom/shadow/ShadowRootInit.h"
+#include "core/html/HTMLDocument.h"
+#include "core/testing/DummyPageHolder.h"
+#include "platform/ScriptForbiddenScope.h"
+#include "platform/heap/Handle.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "wtf/OwnPtr.h"
+#include "wtf/text/AtomicString.h"
+
+namespace blink {
+
+class CustomElementsRegistryTestBase : public ::testing::Test {
+public:

[yosin_UTC9, 2016/06/01 06:15:45]

No need to have public accessibility for Test class.

[dominicc (has gone to gerrit), 2016/06/01 23:30:57]

I suspect you're wrong about this; fixtures using CustomElementRegistryTest need
to cast it to ::testing::Test. Making ::testing::Test non-public only works in
the case CustomElementsRegistryTestBase has no subclasses.

Even for test fixtures with no subclasses, I do not see any particular harm in
making it public. The only benefit I can see of not doing that is if I was
worried about someone adding subclasses. It will be easy to spot the
dependencies because these are defined in .cpp.

Look at the examples in the primer; they're all public ::testing::Test:

https://github.com/google/googletest/blob/master/googletest/docs/Primer.md

Also look at the codebase; in Blink there are 160 occurrences of
'::testing::Test' and 157 of them are 'public ?::testing::Test' The remaining
three are a comment and two aliases.

[yosin_UTC9, 2016/06/02 02:02:18]

Sorry, my explanation is bad. I mean all members of derived class of
::testing::Test should be protected or private; == no public in derived class.

Note: Google coding style says all inheritance should be "public".

+    virtual Document& document() = 0;
+    virtual CustomElementsRegistry& registry() = 0;
+
+    void collectCandidates(
+        const CustomElementDescriptor& desc,
+        HeapVector<Member<Element>>* elements)
+    {
+        registry().collectCandidates(desc, elements);
+    }
+};
+
+class CustomElementsRegistryTest : public CustomElementsRegistryTestBase {
+public:

[yosin_UTC9, 2016/06/01 06:15:44]

No need to have public accessibility for Test class.

+    Document& document() override { return *m_document; }
+    CustomElementsRegistry& registry() override { return *m_registry; }
+
+protected:
+    void SetUp() override
+    {
+        CustomElementsRegistryTestBase::SetUp();
+
+        m_document = HTMLDocument::create();
+        m_document->appendChild(CreateElement("html").inDocument(m_document));
+
+        m_registry = CustomElementsRegistry::create(m_document);
+    }
+
+    void TearDown() override
+    {
+        m_document = nullptr;
+        m_registry = nullptr;
+        CustomElementsRegistryTestBase::TearDown();
+    }
+
+private:
+    Persistent<Document> m_document;
+    Persistent<CustomElementsRegistry> m_registry;
+};
+
+class CustomElementsRegistryFrameTest : public CustomElementsRegistryTestBase {
+public:

[yosin_UTC9, 2016/06/01 06:15:45]

No need to have public accessibility for Test class.

+    Document& document() override { return m_page->document(); }
+    CustomElementsRegistry& registry() override
+    {
+        return *m_page->frame().localDOMWindow()->customElements();
+    }
+    ScriptState* scriptState()
+    {
+        return ScriptState::forMainWorld(&m_page->frame());
+    }
+
+    ShadowRoot* attachShadowTo(Element* element)
+    {
+        NonThrowableExceptionState noExceptions;
+        ShadowRootInit shadowRootInit;
+        return
+            element->attachShadow(scriptState(), shadowRootInit, noExceptions);
+    }
+
+protected:
+    void SetUp() override
+    {
+        CustomElementsRegistryTestBase::SetUp();
+        m_page = DummyPageHolder::create(IntSize(1, 1));
+    }
+
+    void TearDown() override
+    {
+        m_page = nullptr;
+        CustomElementsRegistryTestBase::TearDown();
+    }
+
+private:
+    OwnPtr<DummyPageHolder> m_page;

[yosin_UTC9, 2016/06/01 06:15:45]

nit: Let's use std::unique_ptr<DummyPageHolder>

+};
+
+TEST_F(
+    CustomElementsRegistryTest,
+    collectCandidates_shouldNotIncludeElementsRemovedFromDocument)
+{
+    Element* element = CreateElement("a-a").inDocument(&document());
+    registry().addCandidate(element);
+
+    HeapVector<Member<Element>> elements;
+    collectCandidates(
+        CustomElementDescriptor("a-a", "a-a"),
+        &elements);
+
+    EXPECT_EQ(0u, elements.size())

[yosin_UTC9, 2016/06/01 06:15:45]

nit: isEmpty() is more readable.

[dominicc (has gone to gerrit), 2016/06/01 23:30:57]

True; this produces a more useful failure. With isEmpty you know it is not
empty; with size you know it should be empty but there were 7 elements.

[yosin_UTC9, 2016/06/02 02:02:18]

How about:
EXPECT_TRUE(elements.isEmpty()) << "no candidates should been found, but we have
" << elements.size();

Anyway, this is personal preference.

+        << "no candidates should have been found";
+    EXPECT_FALSE(elements.contains(element))
+        << "the out-of-document candidate should not have been found";
+}
+
+TEST_F(
+    CustomElementsRegistryTest,
+    collectCandidates_shouldNotIncludeElementsInDifferentDocument)
+{
+    Element* element = CreateElement("a-a").inDocument(&document());
+    registry().addCandidate(element);
+
+    Document* otherDocument = HTMLDocument::create();
+    otherDocument->appendChild(element);
+    EXPECT_EQ(otherDocument, element->ownerDocument())
+        << "sanity: another document should have adopted an element on append";
+
+    HeapVector<Member<Element>> elements;
+    collectCandidates(
+        CustomElementDescriptor("a-a", "a-a"),
+        &elements);
+
+    EXPECT_EQ(0u, elements.size())
+        << "no candidates should have been found";
+    EXPECT_FALSE(elements.contains(element))
+        << "the adopted-away candidate should not have been found";
+}
+
+TEST_F(
+    CustomElementsRegistryTest,
+    collectCandidates_shouldOnlyIncludeCandidatesMatchingDescriptor)
+{
+    CustomElementDescriptor descriptor("hello-world", "hello-world");
+
+    // Does not match: namespace is not HTML
+    Element* a = CreateElement("hello-world")

[yosin_UTC9, 2016/06/01 06:15:45]

Please avoid to use one letter variable name.

+        .inDocument(&document())
+        .inNamespace("data:text/date,1981-03-10");
+    // Matches
+    Element* b = CreateElement("hello-world").inDocument(&document());
+    // Does not match: local name is not hello-world
+    Element* c = CreateElement("button")
+        .inDocument(&document())
+        .withIsAttribute("hello-world");
+    document().documentElement()->appendChild(a);
+    a->appendChild(b);
+    a->appendChild(c);
+
+    registry().addCandidate(a);
+    registry().addCandidate(b);
+    registry().addCandidate(c);
+
+    HeapVector<Member<Element>> elements;
+    collectCandidates(descriptor, &elements);
+
+    EXPECT_EQ(1u, elements.size())
+        << "only one candidates should have been found";
+    EXPECT_EQ(b, elements[0])
+        << "the matching element should have been found";
+}
+
+TEST_F(CustomElementsRegistryTest, collectCandidates_oneCandidate)
+{
+    Element* element = CreateElement("a-a").inDocument(&document());
+    registry().addCandidate(element);
+    document().documentElement()->appendChild(element);
+
+    HeapVector<Member<Element>> elements;
+    collectCandidates(
+        CustomElementDescriptor("a-a", "a-a"),
+        &elements);
+
+    EXPECT_EQ(1u, elements.size())
+        << "exactly one candidate should have been found";
+    EXPECT_TRUE(elements.contains(element))
+        << "the candidate should be the element that was added";
+}
+
+TEST_F(CustomElementsRegistryTest, collectCandidates_shouldBeInDocumentOrder)
+{
+    CreateElement& factory = CreateElement("a-a").inDocument(&document());
+    Element* a = factory.withId("a");
+    Element* b = factory.withId("b");
+    Element* c = factory.withId("c");
+
+    registry().addCandidate(b);
+    registry().addCandidate(a);
+    registry().addCandidate(c);
+
+    document().documentElement()->appendChild(a);
+    a->appendChild(b);
+    document().documentElement()->appendChild(c);
+
+    HeapVector<Member<Element>> elements;
+    collectCandidates(
+        CustomElementDescriptor("a-a", "a-a"),
+        &elements);
+
+    EXPECT_EQ(a, elements[0].get());
+    EXPECT_EQ(b, elements[1].get());
+    EXPECT_EQ(c, elements[2].get());
+}
+
+class TestCustomElementDefinition : public CustomElementDefinition {
+    WTF_MAKE_NONCOPYABLE(TestCustomElementDefinition);
+public:
+    TestCustomElementDefinition(const CustomElementDescriptor& descriptor)
+        : CustomElementDefinition(descriptor)
+    {
+    }
+
+    virtual ~TestCustomElementDefinition() = default;
+
+    bool runConstructor(Element* element) override
+    {
+        if (constructionStack().isEmpty()
+            || constructionStack().last() != element)
+            return false;
+        constructionStack().last().clear();
+        return true;
+    }
+};
+
+// Classes which use trace macros cannot be local because of the
+// traceImpl template.
+class LogUpgradeDefinition : public TestCustomElementDefinition {
+    WTF_MAKE_NONCOPYABLE(LogUpgradeDefinition);
+public:
+    LogUpgradeDefinition(const CustomElementDescriptor& descriptor)
+        : TestCustomElementDefinition(descriptor)
+    {
+    }
+
+    DEFINE_INLINE_VIRTUAL_TRACE()
+    {
+        TestCustomElementDefinition::trace(visitor);
+        visitor->trace(m_element);
+    }
+
+    // TODO(dominicc): Make this class collect a vector of what's
+    // upgraded; it will be useful in more tests.
+    Member<Element> m_element;
+    uint32_t m_invocationCount;
+
+    bool runConstructor(Element* element) override
+    {
+        m_invocationCount++;
+        m_element = element;
+        return TestCustomElementDefinition::runConstructor(element);
+    }
+};
+
+class LogUpgradeBuilder final : public CustomElementDefinitionBuilder {
+    STACK_ALLOCATED();
+    WTF_MAKE_NONCOPYABLE(LogUpgradeBuilder);
+public:
+    LogUpgradeBuilder() { }
+
+    bool checkConstructorIntrinsics() override { return true; }
+    bool checkConstructorNotRegistered() override { return true; }
+    bool checkPrototype() override { return true; }
+    CustomElementDefinition* build(
+        const CustomElementDescriptor& descriptor) {
+        return new LogUpgradeDefinition(descriptor);
+    }
+};
+
+TEST_F(CustomElementsRegistryFrameTest, define_upgradesInDocumentElements)
+{
+    ScriptForbiddenScope doNotRelyOnScript;
+
+    Element* element = CreateElement("a-a").inDocument(&document());
+    document().documentElement()->appendChild(element);
+
+    LogUpgradeBuilder builder;
+    NonThrowableExceptionState shouldNotThrow;
+    registry().define(
+        "a-a",
+        builder,
+        ElementRegistrationOptions(),
+        shouldNotThrow);
+    LogUpgradeDefinition* definition =
+        static_cast<LogUpgradeDefinition*>(registry().definitionForName("a-a"));
+    EXPECT_EQ(1u, definition->m_invocationCount)
+        << "defining the element should have 'upgraded' the existing element";
+    EXPECT_EQ(element, definition->m_element)
+        << "the existing a-a element should have been upgraded";
+}
+
+// TODO(dominicc): Add tests which adjust the "is" attribute when type
+// extensions are implemented.
+
+} // namespace blink