Clean up code related to Document node removal
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 8 Sep 2018 20:19:22 +0000 (20:19 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 8 Sep 2018 20:19:22 +0000 (20:19 +0000)
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):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@235830 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/dom/Document.cpp
Source/WebCore/dom/Document.h
Source/WebCore/dom/Element.cpp
Source/WebCore/loader/FrameLoader.cpp

index cf8159a..c5a3807 100644 (file)
@@ -1,3 +1,31 @@
+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>
 
         [CSSJIT] Use lshiftPtr instead of mul32
index a9ab459..609892b 100644 (file)
@@ -3824,15 +3824,15 @@ void Document::pageMutedStateDidChange()
         audioProducer->pageMutedStateDidChange();
 }
 
-static bool isNodeInSubtree(Node& node, Node& container, bool amongChildrenOnly)
+static bool isNodeInSubtree(Node& node, Node& container, Document::NodeRemoval nodeRemoval)
 {
-    if (amongChildrenOnly)
+    if (nodeRemoval == Document::NodeRemoval::ChildrenOfNode)
         return node.isDescendantOf(container);
-    else
-        return &node == &container || node.isDescendantOf(container);
+
+    return &node == &container || node.isDescendantOf(container);
 }
 
-void Document::removeFocusedNodeOfSubtree(Node& node, bool amongChildrenOnly)
+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.
         return;
@@ -3840,8 +3840,8 @@ void Document::removeFocusedNodeOfSubtree(Node& node, bool amongChildrenOnly)
     Element* focusedElement = node.treeScope().focusedElementInScope();
     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.
         SubframeLoadingDisabler disabler(is<ContainerNode>(node) ? &downcast<ContainerNode>(node) : nullptr);
@@ -4201,11 +4201,11 @@ void Document::nodeChildrenWillBeRemoved(ContainerNode& container)
 {
     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
 
     for (auto* range : m_ranges)
@@ -4234,11 +4234,11 @@ void Document::nodeWillBeRemoved(Node& node)
 {
     ASSERT(!ScriptDisallowedScope::InMainThread::isScriptAllowed());
 
-    removeFocusedNodeOfSubtree(node);
-    removeFocusNavigationNodeOfSubtree(node);
+    adjustFocusedNodeOnNodeRemoval(node);
+    adjustFocusNavigationNodeOnNodeRemoval(node);
 
 #if ENABLE(FULLSCREEN_API)
-    removeFullScreenElementOfSubtree(node);
+    adjustFullScreenElementOnNodeRemoval(node);
 #endif
 
     for (auto* it : m_nodeIterators)
@@ -4262,13 +4262,13 @@ static Node* fallbackFocusNavigationStartingNodeAfterRemoval(Node& node)
     return node.previousSibling() ? node.previousSibling() : node.parentNode();
 }
 
-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;
     }
 }
@@ -6466,13 +6466,13 @@ void Document::fullScreenElementRemoved()
     webkitCancelFullScreen();
 }
 
-void Document::removeFullScreenElementOfSubtree(Node& node, bool amongChildrenOnly)
+void Document::adjustFullScreenElementOnNodeRemoval(Node& node, NodeRemoval nodeRemoval)
 {
     if (!m_fullScreenElement)
         return;
     
     bool elementInSubtree = false;
-    if (amongChildrenOnly)
+    if (nodeRemoval == NodeRemoval::ChildrenOfNode)
         elementInSubtree = m_fullScreenElement->isDescendantOf(node);
     else
         elementInSubtree = (m_fullScreenElement == &node) || m_fullScreenElement->isDescendantOf(node);
index b64973d..cd1a963 100644 (file)
@@ -757,7 +757,10 @@ public:
     void setFocusNavigationStartingNode(Node*);
     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*);
 
@@ -802,7 +805,7 @@ public:
     void nodeChildrenWillBeRemoved(ContainerNode&);
     // 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;
 
@@ -1177,7 +1180,8 @@ public:
     void dispatchFullScreenChangeEvents();
     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);
 
index 14e3c07..eef4971 100644 (file)
@@ -2017,7 +2017,7 @@ void Element::removeShadowRoot()
         return;
 
     InspectorInstrumentation::willPopShadowRoot(*this, *oldRoot);
-    document().removeFocusedNodeOfSubtree(*oldRoot);
+    document().adjustFocusedNodeOnNodeRemoval(*oldRoot);
 
     ASSERT(!oldRoot->renderer());
 
index c85fcc1..d7318b1 100644 (file)
@@ -631,7 +631,7 @@ void FrameLoader::clear(Document* newDocument, bool clearWindowProperties, bool
         bool hadLivingRenderTree = m_frame.document()->hasLivingRenderTree();
         m_frame.document()->prepareForDestruction();
         if (hadLivingRenderTree)
-            m_frame.document()->removeFocusedNodeOfSubtree(*m_frame.document());
+            m_frame.document()->adjustFocusedNodeOnNodeRemoval(*m_frame.document());
     }
 
     // Do this after detaching the document so that the unload event works.