Changeset 261028 in webkit

Timestamp:

May 1, 2020 3:16:19 PM (4 years ago)

Author:

dbates@webkit.org

Message:

Change HitTestResult::NodeSet from set of RefPtrs to set of Refs
​https://bugs.webkit.org/show_bug.cgi?id=211306

Reviewed by Simon Fraser.

HitTestResult::listBasedTestResult() never returns a set with nullptrs in it.
So, change the set declaration from ListHashSet<RefPtr<Node>> to ListHashSet<Ref<Node>>.
This way people are not tempted to unnecessarily null check the nodes in the set.

As I made this change to TreeScope::elementsFromPoint() I noticed that retargetToScope(),
which is called by it, returned a Node&. So, I changed it to return a Ref<Node>. That
required me to fix up caretRangeFromPoint(), which lead me to fix up nodeFromPoint() as well.

• dom/Document.cpp:

(WebCore::Document::caretRangeFromPoint):

• dom/EventPath.cpp:

(WebCore::RelatedNodeRetargeter::checkConsistency):

• dom/TreeScope.cpp:

(WebCore::TreeScope::retargetToScope const):
(WebCore::TreeScope::nodeFromPoint):
(WebCore::TreeScope::elementFromPoint):
(WebCore::TreeScope::elementsFromPoint):

• dom/TreeScope.h:
• page/Page.cpp:

(WebCore::Page::editableElementsInRect const):

• rendering/HitTestResult.cpp:

(WebCore::appendToNodeSet):
(WebCore::HitTestResult::HitTestResult):
(WebCore::HitTestResult::operator=):
(WebCore::HitTestResult::addNodeToListBasedTestResultCommon):
(WebCore::HitTestResult::append):

• rendering/HitTestResult.h:
• testing/Internals.cpp:

(WebCore::Internals::nodesFromRect const):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• dom/Document.cpp (modified) (1 diff)
• dom/EventPath.cpp (modified) (1 diff)
• dom/TreeScope.cpp (modified) (7 diffs)
• dom/TreeScope.h (modified) (2 diffs)
• page/Page.cpp (modified) (1 diff)
• rendering/HitTestResult.cpp (modified) (5 diffs)
• rendering/HitTestResult.h (modified) (1 diff)
• testing/Internals.cpp (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2020-05-01  Daniel Bates  <dabates@apple.com>
+
+        Change HitTestResult::NodeSet from set of RefPtrs to set of Refs
+        https://bugs.webkit.org/show_bug.cgi?id=211306
+
+        Reviewed by Simon Fraser.
+
+        HitTestResult::listBasedTestResult() never returns a set with nullptrs in it.
+        So, change the set declaration from ListHashSet<RefPtr<Node>> to ListHashSet<Ref<Node>>.
+        This way people are not tempted to unnecessarily null check the nodes in the set.
+
+        As I made this change to TreeScope::elementsFromPoint() I noticed that retargetToScope(),
+        which is called by it, returned a Node&. So, I changed it to return a Ref<Node>. That
+        required me to fix up caretRangeFromPoint(), which lead me to fix up nodeFromPoint() as well.
+
+        * dom/Document.cpp:
+        (WebCore::Document::caretRangeFromPoint):
+        * dom/EventPath.cpp:
+        (WebCore::RelatedNodeRetargeter::checkConsistency):
+        * dom/TreeScope.cpp:
+        (WebCore::TreeScope::retargetToScope const):
+        (WebCore::TreeScope::nodeFromPoint):
+        (WebCore::TreeScope::elementFromPoint):
+        (WebCore::TreeScope::elementsFromPoint):
+        * dom/TreeScope.h:
+        * page/Page.cpp:
+        (WebCore::Page::editableElementsInRect const):
+        * rendering/HitTestResult.cpp:
+        (WebCore::appendToNodeSet):
+        (WebCore::HitTestResult::HitTestResult):
+        (WebCore::HitTestResult::operator=):
+        (WebCore::HitTestResult::addNodeToListBasedTestResultCommon):
+        (WebCore::HitTestResult::append):
+        * rendering/HitTestResult.h:
+        * testing/Internals.cpp:
+        (WebCore::Internals::nodesFromRect const):
+
 2020-05-01  Chris Dumez  <cdumez@apple.com>
 

trunk/Source/WebCore/dom/Document.cpp

@@ -1513 +1513 @@
 
     LayoutPoint localPoint;
-    Node* node = nodeFromPoint(clientPoint, &localPoint);
+    auto node = nodeFromPoint(clientPoint, &localPoint);
     if (!node)
         return nullptr;
 
-    RenderObject* renderer = node->renderer();
+    auto* renderer = node->renderer();
     if (!renderer)
         return nullptr;
-    Position rangeCompliantPosition = renderer->positionForPoint(localPoint).parentAnchoredEquivalent();
+    auto rangeCompliantPosition = renderer->positionForPoint(localPoint).parentAnchoredEquivalent();
     if (rangeCompliantPosition.isNull())
         return nullptr;
 
-    unsigned offset = rangeCompliantPosition.offsetInContainerNode();
-    node = &retargetToScope(*rangeCompliantPosition.containerNode());
+    auto offset = rangeCompliantPosition.offsetInContainerNode();
+    node = retargetToScope(*rangeCompliantPosition.containerNode());
     if (node != rangeCompliantPosition.containerNode())
         offset = 0;
 
-    return Range::create(*this, node, offset, node, offset);
+    return Range::create(*this, node.get(), offset, node.get(), offset);
 }
 

trunk/Source/WebCore/dom/EventPath.cpp

@@ -452 +452 @@
         return;
     ASSERT(!currentTarget.isClosedShadowHidden(*m_retargetedRelatedNode));
-    ASSERT(m_retargetedRelatedNode == &currentTarget.treeScope().retargetToScope(m_relatedNode));
+    ASSERT(m_retargetedRelatedNode == currentTarget.treeScope().retargetToScope(m_relatedNode).ptr());
 }
 

trunk/Source/WebCore/dom/TreeScope.cpp

@@ -181 +181 @@
 
 
-Node& TreeScope::retargetToScope(Node& node) const
+Ref<Node> TreeScope::retargetToScope(Node& node) const
 {
     auto& scope = node.treeScope();
@@ -197 +197 @@
         ancestorScopes.append(currentScope);
 
-    size_t i = nodeTreeScopes.size();
-    size_t j = ancestorScopes.size();
+    auto i = nodeTreeScopes.size();
+    auto j = ancestorScopes.size();
     while (i > 0 && j > 0 && nodeTreeScopes[i - 1] == ancestorScopes[j - 1]) {
         --i;
@@ -208 +208 @@
         return node;
 
-    ShadowRoot& shadowRootInLowestCommonTreeScope = downcast<ShadowRoot>(nodeTreeScopes[i - 1]->rootNode());
+    auto& shadowRootInLowestCommonTreeScope = downcast<ShadowRoot>(nodeTreeScopes[i - 1]->rootNode());
     return *shadowRootInLowestCommonTreeScope.host();
 }
@@ -350 +350 @@
 }
 
-Node* TreeScope::nodeFromPoint(const LayoutPoint& clientPoint, LayoutPoint* localPoint)
+RefPtr<Node> TreeScope::nodeFromPoint(const LayoutPoint& clientPoint, LayoutPoint* localPoint)
 {
     auto absolutePoint = absolutePointIfNotClipped(documentScope(), clientPoint);
@@ -369 +369 @@
         return nullptr;
 
-    Node* node = nodeFromPoint(LayoutPoint(clientX, clientY), nullptr);
+    auto node = nodeFromPoint(LayoutPoint { clientX, clientY }, nullptr);
     if (!node)
         return nullptr;
 
-    node = &retargetToScope(*node);
+    node = retargetToScope(*node);
     while (!is<Element>(*node)) {
         node = node->parentInComposedTree();
         if (!node)
             break;
-        node = &retargetToScope(*node);
-    }
-
-    return downcast<Element>(node);
+        node = retargetToScope(*node);
+    }
+
+    return static_pointer_cast<Element>(node);
 }
 
@@ -400 +400 @@
     documentScope().hitTest(hitType, result);
 
-    Node* lastNode = nullptr;
-    for (const auto& listBasedNode : result.listBasedTestResult()) {
-        Node* node = listBasedNode.get();
-        node = &retargetToScope(*node);
-        while (!is<Element>(*node)) {
+    RefPtr<Node> lastNode;
+    auto& nodeSet = result.listBasedTestResult();
+    elements.reserveInitialCapacity(nodeSet.size());
+    for (auto& listBasedNode : nodeSet) {
+        RefPtr<Node> node = retargetToScope(listBasedNode);
+        while (!is<Element>(node)) {
             node = node->parentInComposedTree();
             if (!node)
                 break;
-            node = &retargetToScope(*node);
+            node = retargetToScope(*node);
         }
 
@@ -422 +423 @@
             continue;
 
-        elements.append(downcast<Element>(node));
+        elements.append(static_pointer_cast<Element>(node));
         lastNode = node;
     }

trunk/Source/WebCore/dom/TreeScope.h

@@ -77 +77 @@
 
     // https://dom.spec.whatwg.org/#retarget
-    Node& retargetToScope(Node&) const;
+    Ref<Node> retargetToScope(Node&) const;
 
     WEBCORE_EXPORT Node* ancestorNodeInThisScope(Node*) const;
@@ -124 +124 @@
     }
 
-    Node* nodeFromPoint(const LayoutPoint& clientPoint, LayoutPoint* localPoint);
+    RefPtr<Node> nodeFromPoint(const LayoutPoint& clientPoint, LayoutPoint* localPoint);
 
 private:

trunk/Source/WebCore/page/Page.cpp

@@ -943 +943 @@
     result.reserveInitialCapacity(nodeSet.size());
     for (auto& node : nodeSet) {
-        if (is<Element>(node) && isEditableTextInputElement(downcast<Element>(*node))) {
-            ASSERT(searchRectInRootViewCoordinates.intersects(downcast<Element>(*node).clientRect()));
-            result.uncheckedAppend(downcast<Element>(*node));
+        if (is<Element>(node) && isEditableTextInputElement(downcast<Element>(node.get()))) {
+            ASSERT(searchRectInRootViewCoordinates.intersects(downcast<Element>(node.get()).clientRect()));
+            result.uncheckedAppend(static_reference_cast<Element>(node));
         }
     }

trunk/Source/WebCore/rendering/HitTestResult.cpp

@@ -55 +55 @@
 using namespace HTMLNames;
 
+static inline void appendToNodeSet(const HitTestResult::NodeSet& source, HitTestResult::NodeSet& destination)
+{
+    for (auto& node : source)
+        destination.add(node.copyRef());
+}
+
 HitTestResult::HitTestResult() = default;
 
@@ -92 +98 @@
 {
     // Only copy the NodeSet in case of list hit test.
-    m_listBasedTestResult = other.m_listBasedTestResult ? makeUnique<NodeSet>(*other.m_listBasedTestResult) : nullptr;
+    if (other.m_listBasedTestResult) {
+        m_listBasedTestResult = makeUnique<NodeSet>();
+        appendToNodeSet(*other.m_listBasedTestResult, *m_listBasedTestResult);
+    }
 }
 
@@ -109 +118 @@
 
     // Only copy the NodeSet in case of list hit test.
-    m_listBasedTestResult = other.m_listBasedTestResult ? makeUnique<NodeSet>(*other.m_listBasedTestResult) : nullptr;
+    if (other.m_listBasedTestResult) {
+        m_listBasedTestResult = makeUnique<NodeSet>();
+        appendToNodeSet(*other.m_listBasedTestResult, *m_listBasedTestResult);
+    }
 
     return *this;
@@ -635 +647 @@
         node = node->document().ancestorNodeInThisScope(node);
 
-    mutableListBasedTestResult().add(node);
+    mutableListBasedTestResult().add(*node);
 
     if (request.includesAllElementsUnderPoint())
@@ -668 +680 @@
     }
 
-    if (other.m_listBasedTestResult) {
-        NodeSet& set = mutableListBasedTestResult();
-        for (const auto& node : *other.m_listBasedTestResult)
-            set.add(node.get());
-    }
+    if (other.m_listBasedTestResult)
+        appendToNodeSet(*other.m_listBasedTestResult, mutableListBasedTestResult());
 }
 

trunk/Source/WebCore/rendering/HitTestResult.h

@@ -41 +41 @@
     WTF_MAKE_FAST_ALLOCATED;
 public:
-    using NodeSet = ListHashSet<RefPtr<Node>>;
+    using NodeSet = ListHashSet<Ref<Node>>;
 
     WEBCORE_EXPORT HitTestResult();

trunk/Source/WebCore/testing/Internals.cpp

@@ -2228 +2228 @@
     HitTestResult result(point, topPadding, rightPadding, bottomPadding, leftPadding);
     document.hitTest(request, result);
-    const HitTestResult::NodeSet& nodeSet = result.listBasedTestResult();
-    Vector<Ref<Node>> matches;
-    matches.reserveInitialCapacity(nodeSet.size());
-    for (auto& node : nodeSet)
-        matches.uncheckedAppend(*node);
-
+    auto matches = WTF::map(result.listBasedTestResult(), [](const auto& node) { return node.copyRef(); });
     return RefPtr<NodeList> { StaticNodeList::create(WTFMove(matches)) };
 }