RenderScrollbar: Map of scrollbar parts should use RenderPtr.
authorakling@apple.com <akling@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 2 Jan 2014 00:52:55 +0000 (00:52 +0000)
committerakling@apple.com <akling@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 2 Jan 2014 00:52:55 +0000 (00:52 +0000)
<https://webkit.org/b/126367>

Turn RenderScrollbar::m_parts into HashMap of RenderPtrs. This makes
renderer destruction automatic and lets us remove some code.

Reviewed by Antti Koivisto.

* rendering/RenderPtr.h:

    Add HashTraits for RenderPtr so we can use them as values in
    WTF hash tables.

* rendering/RenderScrollbar.h:
* rendering/RenderScrollbar.cpp:
(WebCore::RenderScrollbar::~RenderScrollbar):
(WebCore::RenderScrollbar::setParent):
(WebCore::RenderScrollbar::updateScrollbarParts):
(WebCore::RenderScrollbar::updateScrollbarPart):

    Remove now-unneeded kludges of logic to manually delete scrollbar
    part renderers in various scenarios.

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

Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderPtr.h
Source/WebCore/rendering/RenderScrollbar.cpp
Source/WebCore/rendering/RenderScrollbar.h

index e6075b0d73bc1d16853a754a85b00c8e26e20be9..d3dc2dbb145f3fa7d24c29c4e525dde46d5f84fd 100644 (file)
@@ -1,3 +1,28 @@
+2014-01-01  Andreas Kling  <akling@apple.com>
+
+        RenderScrollbar: Map of scrollbar parts should use RenderPtr.
+        <https://webkit.org/b/126367>
+
+        Turn RenderScrollbar::m_parts into HashMap of RenderPtrs. This makes
+        renderer destruction automatic and lets us remove some code.
+
+        Reviewed by Antti Koivisto.
+
+        * rendering/RenderPtr.h:
+
+            Add HashTraits for RenderPtr so we can use them as values in
+            WTF hash tables.
+
+        * rendering/RenderScrollbar.h:
+        * rendering/RenderScrollbar.cpp:
+        (WebCore::RenderScrollbar::~RenderScrollbar):
+        (WebCore::RenderScrollbar::setParent):
+        (WebCore::RenderScrollbar::updateScrollbarParts):
+        (WebCore::RenderScrollbar::updateScrollbarPart):
+
+            Remove now-unneeded kludges of logic to manually delete scrollbar
+            part renderers in various scenarios.
+
 2014-01-01  Antti Koivisto  <antti@apple.com>
 
         Remove reattachRenderTree
index 35944450d3087836fe26dfff50025234fc964a39..0723eeb8a565129f41c347bb83a988de070e4a8f 100644 (file)
 #ifndef RenderPtr_h
 #define RenderPtr_h
 
-#include <wtf/Assertions.h>
 #include <algorithm>
 #include <cstddef>
 #include <memory>
+#include <wtf/Assertions.h>
+#include <wtf/HashTraits.h>
 
 namespace WebCore {
 
@@ -159,4 +160,17 @@ createRenderer(Args&&... args)
 
 } // namespace WebCore
 
+namespace WTF {
+
+template<typename T> struct HashTraits<WebCore::RenderPtr<T>> : SimpleClassHashTraits<WebCore::RenderPtr<T>> {
+    typedef std::nullptr_t EmptyValueType;
+    static EmptyValueType emptyValue() { return nullptr; }
+
+    typedef T* PeekType;
+    static T* peek(const WebCore::RenderPtr<T>& value) { return value.get(); }
+    static T* peek(std::nullptr_t) { return nullptr; }
+};
+
+} // namespace WTF
+
 #endif // RenderPtr_h
index cae14bae1181969521348ea4b32a712058509117..4bd887f8f5cb53e03f7f629b2b19a6d1a8fbccb9 100644 (file)
@@ -68,15 +68,6 @@ RenderScrollbar::RenderScrollbar(ScrollableArea* scrollableArea, ScrollbarOrient
 
 RenderScrollbar::~RenderScrollbar()
 {
-    if (!m_parts.isEmpty()) {
-        // When a scrollbar is detached from its parent (causing all parts removal) and 
-        // ready to be destroyed, its destruction can be delayed because of RefPtr
-        // maintained in other classes such as EventHandler (m_lastScrollbarUnderMouse).
-        // Meanwhile, we can have a call to updateScrollbarPart which recreates the 
-        // scrollbar part. So, we need to destroy these parts since we don't want them
-        // to call on a destroyed scrollbar. See webkit bug 68009.
-        updateScrollbarParts(true);
-    }
 }
 
 RenderBox* RenderScrollbar::owningRenderer() const
@@ -92,10 +83,8 @@ RenderBox* RenderScrollbar::owningRenderer() const
 void RenderScrollbar::setParent(ScrollView* parent)
 {
     Scrollbar::setParent(parent);
-    if (!parent) {
-        // Destroy all of the scrollbar's RenderBoxes.
-        updateScrollbarParts(true);
-    }
+    if (!parent)
+        m_parts.clear();
 }
 
 void RenderScrollbar::setEnabled(bool e)
@@ -163,21 +152,18 @@ PassRefPtr<RenderStyle> RenderScrollbar::getScrollbarPseudoStyle(ScrollbarPart p
     return result;
 }
 
-void RenderScrollbar::updateScrollbarParts(bool destroy)
+void RenderScrollbar::updateScrollbarParts()
 {
-    updateScrollbarPart(ScrollbarBGPart, destroy);
-    updateScrollbarPart(BackButtonStartPart, destroy);
-    updateScrollbarPart(ForwardButtonStartPart, destroy);
-    updateScrollbarPart(BackTrackPart, destroy);
-    updateScrollbarPart(ThumbPart, destroy);
-    updateScrollbarPart(ForwardTrackPart, destroy);
-    updateScrollbarPart(BackButtonEndPart, destroy);
-    updateScrollbarPart(ForwardButtonEndPart, destroy);
-    updateScrollbarPart(TrackBGPart, destroy);
+    updateScrollbarPart(ScrollbarBGPart);
+    updateScrollbarPart(BackButtonStartPart);
+    updateScrollbarPart(ForwardButtonStartPart);
+    updateScrollbarPart(BackTrackPart);
+    updateScrollbarPart(ThumbPart);
+    updateScrollbarPart(ForwardTrackPart);
+    updateScrollbarPart(BackButtonEndPart);
+    updateScrollbarPart(ForwardButtonEndPart);
+    updateScrollbarPart(TrackBGPart);
     
-    if (destroy)
-        return;
-
     // See if the scrollbar's thickness changed.  If so, we need to mark our owning object as needing a layout.
     bool isHorizontal = orientation() == HorizontalScrollbar;    
     int oldThickness = isHorizontal ? height() : width();
@@ -220,12 +206,12 @@ static PseudoId pseudoForScrollbarPart(ScrollbarPart part)
     return SCROLLBAR;
 }
 
-void RenderScrollbar::updateScrollbarPart(ScrollbarPart partType, bool destroy)
+void RenderScrollbar::updateScrollbarPart(ScrollbarPart partType)
 {
     if (partType == NoPart)
         return;
 
-    RefPtr<RenderStyle> partStyle = destroy ? nullptr : getScrollbarPseudoStyle(partType, pseudoForScrollbarPart(partType));
+    RefPtr<RenderStyle> partStyle = getScrollbarPseudoStyle(partType, pseudoForScrollbarPart(partType));
     bool needRenderer = partStyle && partStyle->display() != NONE;
 
     if (needRenderer && partStyle->display() != BLOCK) {
@@ -251,17 +237,16 @@ void RenderScrollbar::updateScrollbarPart(ScrollbarPart partType, bool destroy)
         }
     }
 
-    if (needRenderer) {
-        RenderScrollbarPart*& partRendererSlot = m_parts.add(partType, nullptr).iterator->value;
-        if (partRendererSlot)
-            partRendererSlot->setStyle(partStyle.releaseNonNull());
-        else {
-            partRendererSlot = new RenderScrollbarPart(owningRenderer()->document(), partStyle.releaseNonNull(), this, partType);
-            partRendererSlot->initializeStyle();
-        }
-    } else {
-        if (RenderScrollbarPart* partRenderer = m_parts.take(partType))
-            partRenderer->destroy();
+    if (!needRenderer) {
+        m_parts.remove(partType);
+        return;
+    }
+
+    if (auto& partRendererSlot = m_parts.add(partType, nullptr).iterator->value)
+        partRendererSlot->setStyle(partStyle.releaseNonNull());
+    else {
+        partRendererSlot = createRenderer<RenderScrollbarPart>(owningRenderer()->document(), partStyle.releaseNonNull(), this, partType);
+        partRendererSlot->initializeStyle();
     }
 }
 
index 548d59d1d9780e94d1af134f14dec464a60a6ac9..c108b8a98c10bdf77d92d307e352414240ac0feb 100644 (file)
@@ -26,6 +26,7 @@
 #ifndef RenderScrollbar_h
 #define RenderScrollbar_h
 
+#include "RenderPtr.h"
 #include "RenderStyleConstants.h"
 #include "Scrollbar.h"
 #include <wtf/HashMap.h>
@@ -75,9 +76,9 @@ private:
 
     virtual bool isCustomScrollbar() const OVERRIDE { return true; }
 
-    void updateScrollbarParts(bool destroy = false);
+    void updateScrollbarParts();
 
-    void updateScrollbarPart(ScrollbarPart, bool destroy = false);
+    void updateScrollbarPart(ScrollbarPart);
 
     // This Scrollbar(Widget) may outlive the DOM which created it (during tear down),
     // so we keep a reference to the Element which caused this custom scrollbar creation.
@@ -86,7 +87,7 @@ private:
     RefPtr<Element> m_ownerElement;
 
     Frame* m_owningFrame;
-    HashMap<unsigned, RenderScrollbarPart*> m_parts;
+    HashMap<unsigned, RenderPtr<RenderScrollbarPart>> m_parts;
 };
 
 inline RenderScrollbar* toRenderScrollbar(ScrollbarThemeClient* scrollbar)