Attachment 349258 Details for Bug 189452 – Patch

[patch] Patch

bug-189452-20180908123556.patch (text/plain), 9.12 KB, created by Simon Fraser (smfr) on 2018-09-08 12:35:56 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Simon Fraser (smfr)

Created: 2018-09-08 12:35:56 PDT

Size: 9.12 KB

patch

obsolete

>Subversion Revision: 235826
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index dfff4175f9ef5ca37a1147f611d1244cb5c85a80..7d089eaa9ef9f8e7ce6e52fb67c408f464c587e2 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -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 NOBODY (OOPS!).
>+
>+        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-07  Fujii Hironori  <Hironori.Fujii@sony.com>
> 
>         [Win][Clang] exceptionShouldTerminateProgram of StructuredExceptionHandlerSuppressor.cpp should take DWORD
>diff --git a/Source/WebCore/dom/Document.cpp b/Source/WebCore/dom/Document.cpp
>index a9ab459d47023e08dfd6ad1f8169f8efe9a5ab36..609892bdc943ee5eafededc39b5cf3d5ab340ac5 100644
>--- a/Source/WebCore/dom/Document.cpp
>+++ b/Source/WebCore/dom/Document.cpp
>@@ -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);
>diff --git a/Source/WebCore/dom/Document.h b/Source/WebCore/dom/Document.h
>index b64973d3d5fcb308ba5b81939367db9b83689887..cd1a9630fc494d1c44423e238d8ebe687dacf15a 100644
>--- a/Source/WebCore/dom/Document.h
>+++ b/Source/WebCore/dom/Document.h
>@@ -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);
> 
>diff --git a/Source/WebCore/dom/Element.cpp b/Source/WebCore/dom/Element.cpp
>index 14e3c076391e87504caf2d6d6187ef4f61d03797..eef4971f208e72e27176126a252cfc51af224244 100644
>--- a/Source/WebCore/dom/Element.cpp
>+++ b/Source/WebCore/dom/Element.cpp
>@@ -2017,7 +2017,7 @@ void Element::removeShadowRoot()
>         return;
> 
>     InspectorInstrumentation::willPopShadowRoot(*this, *oldRoot);
>-    document().removeFocusedNodeOfSubtree(*oldRoot);
>+    document().adjustFocusedNodeOnNodeRemoval(*oldRoot);
> 
>     ASSERT(!oldRoot->renderer());
> 
>diff --git a/Source/WebCore/loader/FrameLoader.cpp b/Source/WebCore/loader/FrameLoader.cpp
>index c85fcc1ebafea02678b1f684077beaf0b38a10e2..d7318b110f22d91073de02295206cc304c195605 100644
>--- a/Source/WebCore/loader/FrameLoader.cpp
>+++ b/Source/WebCore/loader/FrameLoader.cpp
>@@ -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.

Flags:

wenson_hsieh: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 189452: 349258