Attachment 348392 Details for Bug 189075 – Cleanup

[patch] Cleanup

bug-189075-20180828222805.patch (text/plain), 10.66 KB, created by Ryosuke Niwa on 2018-08-28 22:28:06 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Ryosuke Niwa

Created: 2018-08-28 22:28:06 PDT

Size: 10.66 KB

patch

obsolete

>Index: Source/WebCore/ChangeLog
>===================================================================
>--- Source/WebCore/ChangeLog	(revision 235457)
>+++ Source/WebCore/ChangeLog	(working copy)
>@@ -1,3 +1,26 @@
>+2018-08-28  Ryosuke Niwa  <rniwa@webkit.org>
>+
>+        Modernize SlotAssignment
>+        https://bugs.webkit.org/show_bug.cgi?id=189075
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Modernized the code related to SlotAssignment. Namely, use HashMap<>::get instead of HashMap<>::find,
>+        and use HashMap<>::ensure instead of HashMap<>::add. Also use WeakPtr to keep track of HTMLSlotElement
>+        instead of a raw pointer.
>+
>+        * dom/SlotAssignment.cpp:
>+        (WebCore::SlotAssignment::findAssignedSlot):
>+        (WebCore::SlotAssignment::addSlotElementByName):
>+        (WebCore::SlotAssignment::removeSlotElementByName):
>+        (WebCore::SlotAssignment::didChangeSlot):
>+        (WebCore::SlotAssignment::findFirstSlotElement):
>+        (WebCore::SlotAssignment::resolveAllSlotElements):
>+        (WebCore::SlotAssignment::assignToSlot):
>+        * dom/SlotAssignment.h:
>+        (WebCore::SlotAssignment::Slot::Slot): Renamed from SlotInfo since "Info" doesn't add any value.
>+        * html/HTMLSlotElement.h:
>+
> 2018-08-28  Don Olmstead  <don.olmstead@sony.com>
> 
>         Check for null renderer in canBeScrolledIntoView
>Index: Source/WebCore/dom/SlotAssignment.cpp
>===================================================================
>--- Source/WebCore/dom/SlotAssignment.cpp	(revision 235452)
>+++ Source/WebCore/dom/SlotAssignment.cpp	(working copy)
>@@ -57,12 +57,11 @@ HTMLSlotElement* SlotAssignment::findAss
>     if (!is<Text>(node) && !is<Element>(node))
>         return nullptr;
> 
>-    auto slotName = slotNameForHostChild(node);
>-    auto it = m_slots.find(slotName);
>-    if (it == m_slots.end())
>+    auto* slot = m_slots.get(slotNameForHostChild(node));
>+    if (!slot)
>         return nullptr;
> 
>-    return findFirstSlotElement(*it->value, shadowRoot);
>+    return findFirstSlotElement(*slot, shadowRoot);
> }
> 
> void SlotAssignment::addSlotElementByName(const AtomicString& name, HTMLSlotElement& slotElement, ShadowRoot& shadowRoot)
>@@ -76,25 +75,28 @@ void SlotAssignment::addSlotElementByNam
>     shadowRoot.host()->invalidateStyleAndRenderersForSubtree();
> 
>     const AtomicString& slotName = slotNameFromAttributeValue(name);
>-    auto addResult = m_slots.add(slotName, std::unique_ptr<SlotInfo>());
>+    auto addResult = m_slots.ensure(slotName, [&slotElement] {
>+        return std::make_unique<Slot>(makeWeakPtr(slotElement));
>+    });
>     if (addResult.isNewEntry) {
>-        addResult.iterator->value = std::make_unique<SlotInfo>(slotElement);
>-        if (slotName == defaultSlotName()) // Because assignSlots doesn't collect nodes assigned to the default slot as an optimzation.
>+        // Unlike named slots, assignSlots doesn't collect nodes assigned to the default slot
>+        // to avoid always having a vector of all child nodes of a shadow host.
>+        if (slotName == defaultSlotName())
>             m_slotAssignmentsIsValid = false;
>         return;
>     }
> 
>-    auto& slotInfo = *addResult.iterator->value;
>+    auto& slot = *addResult.iterator->value;
> 
>-    if (!slotInfo.hasSlotElements())
>-        slotInfo.element = &slotElement;
>+    if (!slot.hasSlotElements())
>+        slot.element = makeWeakPtr(slotElement);
>     else {
>-        slotInfo.element = nullptr;
>+        slot.element = nullptr;
> #ifndef NDEBUG
>         m_needsToResolveSlotElements = true;
> #endif
>     }
>-    slotInfo.elementCount++;
>+    slot.elementCount++;
> }
> 
> void SlotAssignment::removeSlotElementByName(const AtomicString& name, HTMLSlotElement& slotElement, ShadowRoot& shadowRoot)
>@@ -107,33 +109,30 @@ void SlotAssignment::removeSlotElementBy
>     if (auto* host = shadowRoot.host()) // FIXME: We should be able to do a targeted reconstruction.
>         host->invalidateStyleAndRenderersForSubtree();
> 
>-    auto it = m_slots.find(slotNameFromAttributeValue(name));
>-    RELEASE_ASSERT(it != m_slots.end());
>+    auto* slot = m_slots.get(slotNameFromAttributeValue(name));
>+    RELEASE_ASSERT(slot && slot->hasSlotElements());
> 
>-    auto& slotInfo = *it->value;
>-    RELEASE_ASSERT(slotInfo.hasSlotElements());
>-
>-    slotInfo.elementCount--;
>-    if (slotInfo.element == &slotElement) {
>-        slotInfo.element = nullptr;
>+    slot->elementCount--;
>+    if (slot->element == &slotElement) {
>+        slot->element = nullptr;
> #ifndef NDEBUG
>         m_needsToResolveSlotElements = true;
> #endif
>     }
>-    ASSERT(slotInfo.element || m_needsToResolveSlotElements);
>+    ASSERT(slot->element || m_needsToResolveSlotElements);
> }
> 
> void SlotAssignment::didChangeSlot(const AtomicString& slotAttrValue, ShadowRoot& shadowRoot)
> {
>     auto& slotName = slotNameFromAttributeValue(slotAttrValue);
>-    auto it = m_slots.find(slotName);
>-    if (it == m_slots.end())
>+    auto* slot = m_slots.get(slotName);
>+    if (!slot)
>         return;
>     
>-    it->value->assignedNodes.clear();
>+    slot->assignedNodes.clear();
>     m_slotAssignmentsIsValid = false;
> 
>-    RefPtr<HTMLSlotElement> slotElement = findFirstSlotElement(*it->value, shadowRoot);
>+    auto slotElement = makeRefPtr(findFirstSlotElement(*slot, shadowRoot));
>     if (!slotElement)
>         return;
> 
>@@ -154,21 +153,20 @@ const Vector<Node*>* SlotAssignment::ass
> {
>     ASSERT(slotElement.containingShadowRoot() == &shadowRoot);
>     const AtomicString& slotName = slotNameFromAttributeValue(slotElement.attributeWithoutSynchronization(nameAttr));
>-    auto it = m_slots.find(slotName);
>-    RELEASE_ASSERT(it != m_slots.end());
>+    auto* slot = m_slots.get(slotName);
>+    RELEASE_ASSERT(slot);
> 
>-    auto& slotInfo = *it->value;
>     if (!m_slotAssignmentsIsValid)
>         assignSlots(shadowRoot);
> 
>-    if (!slotInfo.assignedNodes.size())
>+    if (slot->assignedNodes.isEmpty())
>         return nullptr;
> 
>-    RELEASE_ASSERT(slotInfo.hasSlotElements());
>-    if (slotInfo.hasDuplicatedSlotElements() && findFirstSlotElement(slotInfo, shadowRoot) != &slotElement)
>+    RELEASE_ASSERT(slot->hasSlotElements());
>+    if (slot->hasDuplicatedSlotElements() && findFirstSlotElement(*slot, shadowRoot) != &slotElement)
>         return nullptr;
> 
>-    return &slotInfo.assignedNodes;
>+    return &slot->assignedNodes;
> }
> 
> const AtomicString& SlotAssignment::slotNameForHostChild(const Node& child) const
>@@ -176,17 +174,17 @@ const AtomicString& SlotAssignment::slot
>     return slotNameFromSlotAttribute(child);
> }
> 
>-HTMLSlotElement* SlotAssignment::findFirstSlotElement(SlotInfo& slotInfo, ShadowRoot& shadowRoot)
>+HTMLSlotElement* SlotAssignment::findFirstSlotElement(Slot& slot, ShadowRoot& shadowRoot)
> {
>-    if (slotInfo.shouldResolveSlotElement())
>+    if (slot.shouldResolveSlotElement())
>         resolveAllSlotElements(shadowRoot);
> 
> #ifndef NDEBUG
>-    ASSERT(!slotInfo.element || m_slotElementsForConsistencyCheck.contains(slotInfo.element));
>-    ASSERT(!!slotInfo.element == !!slotInfo.elementCount);
>+    ASSERT(!slot.element || m_slotElementsForConsistencyCheck.contains(slot.element.get()));
>+    ASSERT(!!slot.element == !!slot.elementCount);
> #endif
> 
>-    return slotInfo.element;
>+    return slot.element.get();
> }
> 
> void SlotAssignment::resolveAllSlotElements(ShadowRoot& shadowRoot)
>@@ -204,15 +202,14 @@ void SlotAssignment::resolveAllSlotEleme
>     for (auto& slotElement : descendantsOfType<HTMLSlotElement>(shadowRoot)) {
>         auto& slotName = slotNameFromAttributeValue(slotElement.attributeWithoutSynchronization(nameAttr));
> 
>-        auto it = m_slots.find(slotName);
>-        RELEASE_ASSERT(it != m_slots.end());
>+        auto* slot = m_slots.get(slotName);
>+        RELEASE_ASSERT(slot); // slot must have been created when a slot was inserted.
> 
>-        SlotInfo& slotInfo = *it->value;
>-        bool hasSeenSlotWithSameName = !!slotInfo.element;
>+        bool hasSeenSlotWithSameName = !!slot->element;
>         if (hasSeenSlotWithSameName)
>             continue;
> 
>-        slotInfo.element = &slotElement;
>+        slot->element = makeWeakPtr(slotElement);
>         slotCount--;
>         if (!slotCount)
>             break;
>@@ -249,7 +246,9 @@ void SlotAssignment::assignToSlot(Node& 
>         return;
>     }
> 
>-    auto addResult = m_slots.add(slotName, std::make_unique<SlotInfo>());
>+    auto addResult = m_slots.ensure(slotName, [] {
>+        return std::make_unique<Slot>();
>+    });
>     addResult.iterator->value->assignedNodes.append(&child);
> }
> 
>Index: Source/WebCore/dom/SlotAssignment.h
>===================================================================
>--- Source/WebCore/dom/SlotAssignment.h	(revision 235452)
>+++ Source/WebCore/dom/SlotAssignment.h	(working copy)
>@@ -30,6 +30,7 @@
> #include <wtf/HashMap.h>
> #include <wtf/HashSet.h>
> #include <wtf/Vector.h>
>+#include <wtf/WeakPtr.h>
> #include <wtf/text/AtomicString.h>
> #include <wtf/text/AtomicStringHash.h>
> 
>@@ -60,12 +61,12 @@ public:
>     virtual void hostChildElementDidChange(const Element&, ShadowRoot&);
> 
> private:
>-    struct SlotInfo {
>+    struct Slot {
>         WTF_MAKE_FAST_ALLOCATED;
>     public:
>-        SlotInfo() { }
>-        SlotInfo(HTMLSlotElement& slotElement)
>-            : element(&slotElement)
>+        Slot() { }
>+        Slot(WeakPtr<HTMLSlotElement>&& slotElement)
>+            : element(WTFMove(slotElement))
>             , elementCount(1)
>         { }
> 
>@@ -73,20 +74,20 @@ private:
>         bool hasDuplicatedSlotElements() { return elementCount > 1; }
>         bool shouldResolveSlotElement() { return !element && elementCount; }
> 
>-        HTMLSlotElement* element { nullptr };
>+        WeakPtr<HTMLSlotElement> element;
>         unsigned elementCount { 0 };
>         Vector<Node*> assignedNodes;
>     };
>     
>     virtual const AtomicString& slotNameForHostChild(const Node&) const;
> 
>-    HTMLSlotElement* findFirstSlotElement(SlotInfo&, ShadowRoot&);
>+    HTMLSlotElement* findFirstSlotElement(Slot&, ShadowRoot&);
>     void resolveAllSlotElements(ShadowRoot&);
> 
>     void assignSlots(ShadowRoot&);
>     void assignToSlot(Node& child, const AtomicString& slotName);
> 
>-    HashMap<AtomicString, std::unique_ptr<SlotInfo>> m_slots;
>+    HashMap<AtomicString, std::unique_ptr<Slot>> m_slots;
> 
> #ifndef NDEBUG
>     HashSet<HTMLSlotElement*> m_slotElementsForConsistencyCheck;
>Index: Source/WebCore/html/HTMLSlotElement.h
>===================================================================
>--- Source/WebCore/html/HTMLSlotElement.h	(revision 235452)
>+++ Source/WebCore/html/HTMLSlotElement.h	(working copy)
>@@ -30,7 +30,7 @@
> 
> namespace WebCore {
> 
>-class HTMLSlotElement final : public HTMLElement {
>+class HTMLSlotElement final : public HTMLElement, public CanMakeWeakPtr<HTMLSlotElement> {
>     WTF_MAKE_ISO_ALLOCATED(HTMLSlotElement);
> public:
>     static Ref<HTMLSlotElement> create(const QualifiedName&, Document&);

Actions: View | Formatted Diff | Diff

Attachments on bug 189075: 348392 | 348427