Changeset 235830 in webkit

Timestamp:

Sep 8, 2018, 1:19:22 PM (7 years ago)

Author:

Simon Fraser

Message:

Clean up code related to Document node removal
​https://bugs.webkit.org/show_bug.cgi?id=189452

Reviewed by Wenson Hsieh.

Replace the "amongChildrenOnly" boolean argument with an enum for clarity.

Rename the remove*OfSubtree functions, because that naming is very unclear.
Instead, use adjust*OnNodeRemoval which better describes what the code does.

• dom/Document.cpp:

(WebCore::isNodeInSubtree):
(WebCore::Document::adjustFocusedNodeOnNodeRemoval):
(WebCore::Document::nodeChildrenWillBeRemoved):
(WebCore::Document::nodeWillBeRemoved):
(WebCore::Document::adjustFocusNavigationNodeOnNodeRemoval):
(WebCore::Document::adjustFullScreenElementOnNodeRemoval):
(WebCore::Document::removeFocusedNodeOfSubtree): Deleted.
(WebCore::Document::removeFocusNavigationNodeOfSubtree): Deleted.
(WebCore::Document::removeFullScreenElementOfSubtree): Deleted.

• dom/Document.h:
• dom/Element.cpp:

(WebCore::Element::removeShadowRoot):

• loader/FrameLoader.cpp:

(WebCore::FrameLoader::clear):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• dom/Document.cpp (modified) (7 diffs)
• dom/Document.h (modified) (3 diffs)
• dom/Element.cpp (modified) (1 diff)
• loader/FrameLoader.cpp (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2018-09-08  Simon Fraser  <simon.fraser@apple.com>
+
+        Clean up code related to Document node removal
+        https://bugs.webkit.org/show_bug.cgi?id=189452
+
+        Reviewed by Wenson Hsieh.
+
+        Replace the "amongChildrenOnly" boolean argument with an enum for clarity.
+        
+        Rename the remove*OfSubtree functions, because that naming is very unclear.
+        Instead, use adjust*OnNodeRemoval which better describes what the code does.
+
+        * dom/Document.cpp:
+        (WebCore::isNodeInSubtree):
+        (WebCore::Document::adjustFocusedNodeOnNodeRemoval):
+        (WebCore::Document::nodeChildrenWillBeRemoved):
+        (WebCore::Document::nodeWillBeRemoved):
+        (WebCore::Document::adjustFocusNavigationNodeOnNodeRemoval):
+        (WebCore::Document::adjustFullScreenElementOnNodeRemoval):
+        (WebCore::Document::removeFocusedNodeOfSubtree): Deleted.
+        (WebCore::Document::removeFocusNavigationNodeOfSubtree): Deleted.
+        (WebCore::Document::removeFullScreenElementOfSubtree): Deleted.
+        * dom/Document.h:
+        * dom/Element.cpp:
+        (WebCore::Element::removeShadowRoot):
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::clear):
+
 2018-09-08  Yusuke Suzuki  <yusukesuzuki@slowstart.org>
 

trunk/Source/WebCore/dom/Document.cpp

@@ -3825 +3825 @@
 }
 
-static bool isNodeInSubtree(Node& node, Node& container, bool amongChildrenOnly)
-{
-    if (amongChildrenOnly)
+static bool isNodeInSubtree(Node& node, Node& container, Document::NodeRemoval nodeRemoval)
+{
+    if (nodeRemoval == Document::NodeRemoval::ChildrenOfNode)
         return node.isDescendantOf(container);
-    else
-        return &node == &container || node.isDescendantOf(container);
-}
-
-void Document::removeFocusedNodeOfSubtree(Node& node, bool amongChildrenOnly)
+
+    return &node == &container || node.isDescendantOf(container);
+}
+
+void Document::adjustFocusedNodeOnNodeRemoval(Node& node, NodeRemoval nodeRemoval)
 {
     if (!m_focusedElement || pageCacheState() != NotInPageCache) // If the document is in the page cache, then we don't need to clear out the focused node.
@@ -3841 +3841 @@
     if (!focusedElement)
         return;
-    
-    if (isNodeInSubtree(*focusedElement, node, amongChildrenOnly)) {
+
+    if (isNodeInSubtree(*focusedElement, node, nodeRemoval)) {
         // FIXME: We should avoid synchronously updating the style inside setFocusedElement.
         // FIXME: Object elements should avoid loading a frame synchronously in a post style recalc callback.
@@ -4202 +4202 @@
     ASSERT(!ScriptDisallowedScope::InMainThread::isScriptAllowed());
 
-    removeFocusedNodeOfSubtree(container, true /* amongChildrenOnly */);
-    removeFocusNavigationNodeOfSubtree(container, true /* amongChildrenOnly */);
+    adjustFocusedNodeOnNodeRemoval(container, NodeRemoval::ChildrenOfNode);
+    adjustFocusNavigationNodeOnNodeRemoval(container, NodeRemoval::ChildrenOfNode);
 
 #if ENABLE(FULLSCREEN_API)
-    removeFullScreenElementOfSubtree(container, true /* amongChildrenOnly */);
+    adjustFullScreenElementOnNodeRemoval(container, NodeRemoval::ChildrenOfNode);
 #endif
 
@@ -4235 +4235 @@
     ASSERT(!ScriptDisallowedScope::InMainThread::isScriptAllowed());
 
-    removeFocusedNodeOfSubtree(node);
-    removeFocusNavigationNodeOfSubtree(node);
+    adjustFocusedNodeOnNodeRemoval(node);
+    adjustFocusNavigationNodeOnNodeRemoval(node);
 
 #if ENABLE(FULLSCREEN_API)
-    removeFullScreenElementOfSubtree(node);
+    adjustFullScreenElementOnNodeRemoval(node);
 #endif
 
@@ -4263 +4263 @@
 }
 
-void Document::removeFocusNavigationNodeOfSubtree(Node& node, bool amongChildrenOnly)
+void Document::adjustFocusNavigationNodeOnNodeRemoval(Node& node, NodeRemoval nodeRemoval)
 {
     if (!m_focusNavigationStartingNode)
         return;
 
-    if (isNodeInSubtree(*m_focusNavigationStartingNode, node, amongChildrenOnly)) {
-        m_focusNavigationStartingNode = amongChildrenOnly ? &node : fallbackFocusNavigationStartingNodeAfterRemoval(node);
+    if (isNodeInSubtree(*m_focusNavigationStartingNode, node, nodeRemoval)) {
+        m_focusNavigationStartingNode = (nodeRemoval == NodeRemoval::ChildrenOfNode) ? &node : fallbackFocusNavigationStartingNodeAfterRemoval(node);
         m_focusNavigationStartingNodeIsRemoved = true;
     }
@@ -6467 +6467 @@
 }
 
-void Document::removeFullScreenElementOfSubtree(Node& node, bool amongChildrenOnly)
+void Document::adjustFullScreenElementOnNodeRemoval(Node& node, NodeRemoval nodeRemoval)
 {
     if (!m_fullScreenElement)
@@ -6473 +6473 @@
     
     bool elementInSubtree = false;
-    if (amongChildrenOnly)
+    if (nodeRemoval == NodeRemoval::ChildrenOfNode)
         elementInSubtree = m_fullScreenElement->isDescendantOf(node);
     else

trunk/Source/WebCore/dom/Document.h

@@ -758 +758 @@
     Element* focusNavigationStartingNode(FocusDirection) const;
 
-    void removeFocusedNodeOfSubtree(Node&, bool amongChildrenOnly = false);
+    enum class NodeRemoval { Node, ChildrenOfNode };
+    void adjustFocusedNodeOnNodeRemoval(Node&, NodeRemoval = NodeRemoval::Node);
+    void adjustFocusNavigationNodeOnNodeRemoval(Node&, NodeRemoval = NodeRemoval::Node);
+
     void hoveredElementDidDetach(Element*);
     void elementInActiveChainDidDetach(Element*);
@@ -803 +806 @@
     // nodeWillBeRemoved is only safe when removing one node at a time.
     void nodeWillBeRemoved(Node&);
-    void removeFocusNavigationNodeOfSubtree(Node&, bool amongChildrenOnly = false);
+
     enum class AcceptChildOperation { Replace, InsertOrAdd };
     bool canAcceptChild(const Node& newChild, const Node* refChild, AcceptChildOperation) const;
@@ -1178 +1181 @@
     bool fullScreenIsAllowedForElement(Element*) const;
     void fullScreenElementRemoved();
-    void removeFullScreenElementOfSubtree(Node&, bool amongChildrenOnly = false);
+    void adjustFullScreenElementOnNodeRemoval(Node&, NodeRemoval = NodeRemoval::Node);
+
     WEBCORE_EXPORT bool isAnimatingFullScreen() const;
     WEBCORE_EXPORT void setAnimatingFullScreen(bool);

trunk/Source/WebCore/dom/Element.cpp

@@ -2018 +2018 @@
 
     InspectorInstrumentation::willPopShadowRoot(*this, *oldRoot);
-    document().removeFocusedNodeOfSubtree(*oldRoot);
+    document().adjustFocusedNodeOnNodeRemoval(*oldRoot);
 
     ASSERT(!oldRoot->renderer());

trunk/Source/WebCore/loader/FrameLoader.cpp

@@ -632 +632 @@
         m_frame.document()->prepareForDestruction();
         if (hadLivingRenderTree)
-            m_frame.document()->removeFocusedNodeOfSubtree(*m_frame.document());
+            m_frame.document()->adjustFocusedNodeOnNodeRemoval(*m_frame.document());
     }