[webkit-changes] [261028] trunk/Source/WebCore

Fri, 01 May 2020 15:17:22 -0700

Title: [261028] trunk/Source/WebCore
Revision
261028
Author
[email protected]
Date
2020-05-01 15:16:19 -0700 (Fri, 01 May 2020)

Log 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):

Modified Paths

Diff

Modified: trunk/Source/WebCore/ChangeLog (261027 => 261028)


--- trunk/Source/WebCore/ChangeLog	2020-05-01 22:08:10 UTC (rev 261027)
+++ trunk/Source/WebCore/ChangeLog	2020-05-01 22:16:19 UTC (rev 261028)
@@ -1,3 +1,40 @@
+2020-05-01  Daniel Bates  <[email protected]>
+
+        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  <[email protected]>
 
         Unreviewed build fix after r260962.

Modified: trunk/Source/WebCore/dom/Document.cpp (261027 => 261028)


--- trunk/Source/WebCore/dom/Document.cpp	2020-05-01 22:08:10 UTC (rev 261027)
+++ trunk/Source/WebCore/dom/Document.cpp	2020-05-01 22:16:19 UTC (rev 261028)
@@ -1512,23 +1512,23 @@
         return nullptr;
 
     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);
 }
 
 bool Document::isBodyPotentiallyScrollable(HTMLBodyElement& body)

Modified: trunk/Source/WebCore/dom/EventPath.cpp (261027 => 261028)


--- trunk/Source/WebCore/dom/EventPath.cpp	2020-05-01 22:08:10 UTC (rev 261027)
+++ trunk/Source/WebCore/dom/EventPath.cpp	2020-05-01 22:16:19 UTC (rev 261028)
@@ -451,7 +451,7 @@
     if (!m_retargetedRelatedNode)
         return;
     ASSERT(!currentTarget.isClosedShadowHidden(*m_retargetedRelatedNode));
-    ASSERT(m_retargetedRelatedNode == &currentTarget.treeScope().retargetToScope(m_relatedNode));
+    ASSERT(m_retargetedRelatedNode == currentTarget.treeScope().retargetToScope(m_relatedNode).ptr());
 }
 
 #endif // ASSERT_ENABLED

Modified: trunk/Source/WebCore/dom/TreeScope.cpp (261027 => 261028)


--- trunk/Source/WebCore/dom/TreeScope.cpp	2020-05-01 22:08:10 UTC (rev 261027)
+++ trunk/Source/WebCore/dom/TreeScope.cpp	2020-05-01 22:16:19 UTC (rev 261028)
@@ -180,7 +180,7 @@
 }
 
 
-Node& TreeScope::retargetToScope(Node& node) const
+Ref<Node> TreeScope::retargetToScope(Node& node) const
 {
     auto& scope = node.treeScope();
     if (LIKELY(this == &scope || !node.isInShadowTree()))
@@ -196,8 +196,8 @@
     for (auto* currentScope = this; currentScope; currentScope = currentScope->parentTreeScope())
         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;
         --j;
@@ -207,7 +207,7 @@
     if (nodeIsInOuterTreeScope)
         return node;
 
-    ShadowRoot& shadowRootInLowestCommonTreeScope = downcast<ShadowRoot>(nodeTreeScopes[i - 1]->rootNode());
+    auto& shadowRootInLowestCommonTreeScope = downcast<ShadowRoot>(nodeTreeScopes[i - 1]->rootNode());
     return *shadowRootInLowestCommonTreeScope.host();
 }
 
@@ -349,7 +349,7 @@
     return WTF::nullopt;
 }
 
-Node* TreeScope::nodeFromPoint(const LayoutPoint& clientPoint, LayoutPoint* localPoint)
+RefPtr<Node> TreeScope::nodeFromPoint(const LayoutPoint& clientPoint, LayoutPoint* localPoint)
 {
     auto absolutePoint = absolutePointIfNotClipped(documentScope(), clientPoint);
     if (!absolutePoint)
@@ -368,19 +368,19 @@
     if (!document.hasLivingRenderTree())
         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);
+        node = retargetToScope(*node);
     }
 
-    return downcast<Element>(node);
+    return static_pointer_cast<Element>(node);
 }
 
 Vector<RefPtr<Element>> TreeScope::elementsFromPoint(double clientX, double clientY)
@@ -399,15 +399,16 @@
     HitTestResult result { absolutePoint.value() };
     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);
         }
 
         if (!node)
@@ -421,7 +422,7 @@
         if (node == lastNode)
             continue;
 
-        elements.append(downcast<Element>(node));
+        elements.append(static_pointer_cast<Element>(node));
         lastNode = node;
     }

Modified: trunk/Source/WebCore/dom/TreeScope.h (261027 => 261028)


--- trunk/Source/WebCore/dom/TreeScope.h	2020-05-01 22:08:10 UTC (rev 261027)
+++ trunk/Source/WebCore/dom/TreeScope.h	2020-05-01 22:16:19 UTC (rev 261028)
@@ -76,7 +76,7 @@
     static ptrdiff_t documentScopeMemoryOffset() { return OBJECT_OFFSETOF(TreeScope, m_documentScope); }
 
     // https://dom.spec.whatwg.org/#retarget
-    Node& retargetToScope(Node&) const;
+    Ref<Node> retargetToScope(Node&) const;
 
     WEBCORE_EXPORT Node* ancestorNodeInThisScope(Node*) const;
     WEBCORE_EXPORT Element* ancestorElementInThisScope(Element*) const;
@@ -123,7 +123,7 @@
         m_documentScope = document;
     }
 
-    Node* nodeFromPoint(const LayoutPoint& clientPoint, LayoutPoint* localPoint);
+    RefPtr<Node> nodeFromPoint(const LayoutPoint& clientPoint, LayoutPoint* localPoint);
 
 private:

Modified: trunk/Source/WebCore/page/Page.cpp (261027 => 261028)


--- trunk/Source/WebCore/page/Page.cpp	2020-05-01 22:08:10 UTC (rev 261027)
+++ trunk/Source/WebCore/page/Page.cpp	2020-05-01 22:16:19 UTC (rev 261028)
@@ -942,9 +942,9 @@
     auto& nodeSet = hitTestResult.listBasedTestResult();
     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));
         }
     }
     return result;

Modified: trunk/Source/WebCore/rendering/HitTestResult.cpp (261027 => 261028)


--- trunk/Source/WebCore/rendering/HitTestResult.cpp	2020-05-01 22:08:10 UTC (rev 261027)
+++ trunk/Source/WebCore/rendering/HitTestResult.cpp	2020-05-01 22:16:19 UTC (rev 261028)
@@ -54,6 +54,12 @@
 
 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;
 
 HitTestResult::HitTestResult(const LayoutPoint& point)
@@ -91,7 +97,10 @@
     , m_isOverWidget(other.isOverWidget())
 {
     // 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);
+    }
 }
 
 HitTestResult::~HitTestResult() = default;
@@ -108,7 +117,10 @@
     m_isOverWidget = other.isOverWidget();
 
     // 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;
 }
@@ -634,7 +646,7 @@
     if (request.disallowsUserAgentShadowContent() && node->isInUserAgentShadowTree())
         node = node->document().ancestorNodeInThisScope(node);
 
-    mutableListBasedTestResult().add(node);
+    mutableListBasedTestResult().add(*node);
 
     if (request.includesAllElementsUnderPoint())
         return HitTestProgress::Continue;
@@ -667,11 +679,8 @@
         m_isOverWidget = other.isOverWidget();
     }
 
-    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());
 }
 
 const HitTestResult::NodeSet& HitTestResult::listBasedTestResult() const

Modified: trunk/Source/WebCore/rendering/HitTestResult.h (261027 => 261028)


--- trunk/Source/WebCore/rendering/HitTestResult.h	2020-05-01 22:08:10 UTC (rev 261027)
+++ trunk/Source/WebCore/rendering/HitTestResult.h	2020-05-01 22:16:19 UTC (rev 261028)
@@ -40,7 +40,7 @@
 class HitTestResult {
     WTF_MAKE_FAST_ALLOCATED;
 public:
-    using NodeSet = ListHashSet<RefPtr<Node>>;
+    using NodeSet = ListHashSet<Ref<Node>>;
 
     WEBCORE_EXPORT HitTestResult();
     WEBCORE_EXPORT explicit HitTestResult(const LayoutPoint&);

Modified: trunk/Source/WebCore/testing/Internals.cpp (261027 => 261028)


--- trunk/Source/WebCore/testing/Internals.cpp	2020-05-01 22:08:10 UTC (rev 261027)
+++ trunk/Source/WebCore/testing/Internals.cpp	2020-05-01 22:16:19 UTC (rev 261028)
@@ -2227,12 +2227,7 @@
 
     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)) };
 }
 

_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to