Oilpan: improve ScrollableArea handling.

Source/platform/scroll/Scrollbar.cpp

Index: Source/platform/scroll/Scrollbar.cpp
diff --git a/Source/platform/scroll/Scrollbar.cpp b/Source/platform/scroll/Scrollbar.cpp
index 12f8fc35ad36fe271873cbabd050cf3e6591e6b2..12bd604d0bfa064be66f3a3728a54403af26b7a0 100644
--- a/Source/platform/scroll/Scrollbar.cpp
+++ b/Source/platform/scroll/Scrollbar.cpp
@@ -80,29 +80,17 @@ Scrollbar::Scrollbar(ScrollableArea* scrollableArea, ScrollbarOrientation orient
     Widget::setFrameRect(IntRect(0, 0, thickness, thickness));
 
     m_currentPos = scrollableAreaCurrentPos();
-
-#if ENABLE(OILPAN)
-    if (m_scrollableArea)
-        m_animator = m_scrollableArea->scrollAnimator();
-#endif
 }
 
 Scrollbar::~Scrollbar()

[haraken, 2015/06/29 15:20:22]

Can we add ASSERT(!m_scrollableArea) to verify that
disconnectFromScrollableArea() has been called?

[sof, 2015/06/29 15:59:42]

It doesn't hold in all cases, and I didn't see the value of enforcing it, so I
didn't in the end. Look at the WIP CL for versions that did.

[haraken, 2015/06/30 10:30:35]

It doesn't hold in non-oilpan or/and oilpan? In oilpan, it's ok it doesn't hold
because disconnectFromScrollableArea() is just clearing the back pointer (which
is a Member). In non-oilpan, it's not ok if it doesn't hold -- it would imply
m_scrollableArea can be a stale pointer.

[sof, 2015/06/30 11:34:08]

It holds in neither setting; with non-Oilpan, the cases where it doesn't the
lifetimes of the scrollbars are such that isn't an issue.

https://codereview.chromium.org/1209293004/#ps260001 did this & had the assert.
cf. FrameViewAutoSize and WebFrameTest changes, in particular. i.e., it looked
artificial to have the explicit disconnects when dealing with local RefPtr<>s.

 {
-    stopTimerIfNeeded();
-
     m_theme->unregisterScrollbar(this);
+}
 
-#if ENABLE(OILPAN)
-    if (!m_animator)
-        return;
-
-    ASSERT(m_scrollableArea);
-    if (m_orientation == VerticalScrollbar)
-        m_animator->willRemoveVerticalScrollbar(this);
-    else
-        m_animator->willRemoveHorizontalScrollbar(this);

[haraken, 2015/06/29 15:20:22]

Help me understand: Who calls these willRemoveScrollbar after this CL?

[sof, 2015/06/29 15:59:41]

The ScrollableAreas will now enforce this, as they did previously really. This
code actually had no effect for the problematic Mac scroll animators -- the
theme unregistration caused their implementations of willRemove*() to leave
early & making them be no-ops, in effect.

The RefPtr<> may have served a purpose initially with render objects and frames
being on/off-heap; unclear. It had no purpose now, I realized.

-#endif
+DEFINE_TRACE(Scrollbar)
+{
+    visitor->trace(m_scrollableArea);
+    Widget::trace(visitor);
 }
 
 void Scrollbar::setFrameRect(const IntRect& frameRect)
@@ -156,9 +144,6 @@ void Scrollbar::offsetDidChange()
 void Scrollbar::disconnectFromScrollableArea()

[haraken, 2015/06/29 15:20:22]

In oilpan, do we need to call Scrollbar::disconnectFromScrollableArea()? It
seems that it is just clearing a back pointer. In oilpan, the clear wouldn't be
needed since the back pointer is a Member.

[sof, 2015/06/29 15:59:41]

yes, I think we can get rid of this step once we switch over.

 {
     m_scrollableArea = nullptr;

[sof, 2015/06/30 11:34:08]

Made clearing conditional on !ENABLE(OILPAN) + added a TODO.

-#if ENABLE(OILPAN)
-    m_animator = nullptr;
-#endif
 }
 
 void Scrollbar::setProportion(int visibleSize, int totalSize)