ScrollableAreaSet should be moved from Page to FrameView

https://bugs.webkit.org/show_bug.cgi?id=62762

Reviewed by Beth Dakin.

Source/WebCore:

It makes more sense for the set of scrollable areas to be per frame view instead of per page;
scrollable areas are associated with a containing frame view and their lifecycle follows the lifecycle of the
frame view much more closely. This could even fix a bunch of crashes where a scrollable area outlived its containing page.

* WebCore.exp.in:
Replace the Page member functions with FrameView member functions instead.

* page/EventHandler.cpp:
(WebCore::EventHandler::mouseMoved):
Check if the frame view contains the given layer.

(WebCore::EventHandler::updateMouseEventTargetNode):
Ditto.

* page/FocusController.cpp:
(WebCore::contentAreaDidShowOrHide):
Add helper function.

(WebCore::FocusController::setContainingWindowIsVisible):
Call contentAreaDidShowOrHide for the main frame view, and for all scrollable areas
inside all subframe views.

* page/FrameView.cpp:
(WebCore::FrameView::FrameView):
Use early returns to make the code more clear. Also, don't add the scrollable area to the set here.

(WebCore::FrameView::~FrameView):
Don't remove the scrollable area here.

(WebCore::FrameView::zoomAnimatorTransformChanged):
m_page is gone so use m_frame->page() instead.

(WebCore::FrameView::setAnimatorsAreActive):
Call ScrollAnimator::setIsActive for all the scrollable areas in this frame view. Previously we used to do
this for all scrollable areas on the page, but since setAnimatorsAreActive will be called for each document,
this will be done implicitly.

(WebCore::FrameView::notifyPageThatContentAreaWillPaint):
Call ScrollableArea::contentDidPaint for this frame view and all its immediate scrollable areas. Previously, we used
to do this for all scrollable areas on the page, but we only need to do it for this frame view.

(WebCore::FrameView::scrollAnimatorEnabled):
Get the page from m_frame since m_page is gone.

(WebCore::FrameView::addScrollableArea):
(WebCore::FrameView::removeScrollableArea):
(WebCore::FrameView::containsScrollableArea):
Move these member functions here from Page.

(WebCore::FrameView::addChild):
If we are adding a frame view, add it to the scrollable area set.

(WebCore::FrameView::removeChild):
If we are removing a frame view, remove it from the scrollable area set.

* page/FrameView.h:
Move the member function declarations and the scrollable area set member variable here from Page.

* page/Page.cpp:
(WebCore::Page::~Page):
Don't call disconnectPage on the scrollable areas anymore.

* platform/ScrollView.h:
(ScrollView):
Make addChild and removeChild virtual.

* platform/ScrollableArea.h:
Remove disconnectFromPage.

* rendering/RenderLayer.cpp:
(WebCore::RenderLayer::RenderLayer):
(WebCore::RenderLayer::~RenderLayer):
(WebCore::RenderLayer::styleChanged):
The frame view now keeps track of the scrollable areas.

* rendering/RenderLayer.h:
Remove the page member variable and disconnectFromPage.

* rendering/RenderListBox.cpp:
(WebCore::RenderListBox::RenderListBox):
(WebCore::RenderListBox::~RenderListBox):
The frame view now keeps track of the scrollable areas.

* rendering/RenderListBox.h:
Remove the page member variable and disconnectFromPage.

Source/WebKit/chromium:

Update for changes to WebCore now that the scrollable area set is kept per frame view.

* src/ScrollbarGroup.cpp:
(WebKit::ScrollbarGroup::ScrollbarGroup):
(WebKit::ScrollbarGroup::~ScrollbarGroup):
* src/ScrollbarGroup.h:
(WebCore):
(ScrollbarGroup):
* src/WebPluginContainerImpl.cpp:
(WebKit::WebPluginContainerImpl::scrollbarGroup):

Source/WebKit2:

* WebProcess/Plugins/PDF/BuiltInPDFView.cpp:
(WebKit::BuiltInPDFView::initialize):
Call FrameView::addScrollableArea instead.

(WebKit::BuiltInPDFView::destroy):
Call FrameView::removeScrollableArea instead.

* WebProcess/Plugins/PDF/BuiltInPDFView.h:
Remove disconnectFromPage since it no longer exists on ScrollableArea.

Canonical link: https://commits.webkit.org/94894@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@106977 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog

@@ -1,3 +1,96 @@
+2012-02-06  Anders Carlsson  <andersca@apple.com>
+
+        ScrollableAreaSet should be moved from Page to FrameView
+        https://bugs.webkit.org/show_bug.cgi?id=62762
+
+        Reviewed by Beth Dakin.
+
+        It makes more sense for the set of scrollable areas to be per frame view instead of per page;
+        scrollable areas are associated with a containing frame view and their lifecycle follows the lifecycle of the
+        frame view much more closely. This could even fix a bunch of crashes where a scrollable area outlived its containing page.
+
+        * WebCore.exp.in:
+        Replace the Page member functions with FrameView member functions instead.
+
+        * page/EventHandler.cpp:
+        (WebCore::EventHandler::mouseMoved):
+        Check if the frame view contains the given layer.
+
+        (WebCore::EventHandler::updateMouseEventTargetNode):
+        Ditto.
+
+        * page/FocusController.cpp:
+        (WebCore::contentAreaDidShowOrHide):
+        Add helper function.
+
+        (WebCore::FocusController::setContainingWindowIsVisible):
+        Call contentAreaDidShowOrHide for the main frame view, and for all scrollable areas
+        inside all subframe views.
+
+        * page/FrameView.cpp:
+        (WebCore::FrameView::FrameView):
+        Use early returns to make the code more clear. Also, don't add the scrollable area to the set here.
+
+        (WebCore::FrameView::~FrameView):
+        Don't remove the scrollable area here.
+
+        (WebCore::FrameView::zoomAnimatorTransformChanged):
+        m_page is gone so use m_frame->page() instead.
+
+        (WebCore::FrameView::setAnimatorsAreActive):
+        Call ScrollAnimator::setIsActive for all the scrollable areas in this frame view. Previously we used to do
+        this for all scrollable areas on the page, but since setAnimatorsAreActive will be called for each document,
+        this will be done implicitly.
+
+        (WebCore::FrameView::notifyPageThatContentAreaWillPaint):
+        Call ScrollableArea::contentDidPaint for this frame view and all its immediate scrollable areas. Previously, we used
+        to do this for all scrollable areas on the page, but we only need to do it for this frame view.
+
+        (WebCore::FrameView::scrollAnimatorEnabled):
+        Get the page from m_frame since m_page is gone.
+
+        (WebCore::FrameView::addScrollableArea):
+        (WebCore::FrameView::removeScrollableArea):
+        (WebCore::FrameView::containsScrollableArea):
+        Move these member functions here from Page.
+
+        (WebCore::FrameView::addChild):
+        If we are adding a frame view, add it to the scrollable area set.
+
+        (WebCore::FrameView::removeChild):
+        If we are removing a frame view, remove it from the scrollable area set.
+
+        * page/FrameView.h:
+        Move the member function declarations and the scrollable area set member variable here from Page.
+
+        * page/Page.cpp:
+        (WebCore::Page::~Page):
+        Don't call disconnectPage on the scrollable areas anymore.
+
+        * platform/ScrollView.h:
+        (ScrollView):
+        Make addChild and removeChild virtual.
+
+        * platform/ScrollableArea.h:
+        Remove disconnectFromPage.
+
+        * rendering/RenderLayer.cpp:
+        (WebCore::RenderLayer::RenderLayer):
+        (WebCore::RenderLayer::~RenderLayer):
+        (WebCore::RenderLayer::styleChanged):
+        The frame view now keeps track of the scrollable areas.
+
+        * rendering/RenderLayer.h:
+        Remove the page member variable and disconnectFromPage.
+
+        * rendering/RenderListBox.cpp:
+        (WebCore::RenderListBox::RenderListBox):
+        (WebCore::RenderListBox::~RenderListBox):
+        The frame view now keeps track of the scrollable areas.
+
+        * rendering/RenderListBox.h:
+        Remove the page member variable and disconnectFromPage.
+
 2012-02-07  Matthew Delaney  <mdelaney@apple.com>
 
         Remove redundant checks in CanvasRenderingContext2D.cpp

Source/WebCore/WebCore.exp.in

@@ -764,12 +764,10 @@ __ZN7WebCore4Page15addSchedulePairEN3WTF10PassRefPtrINS_12SchedulePairEEE
 __ZN7WebCore4Page15didMoveOnscreenEv
 __ZN7WebCore4Page16setCanStartMediaEb
 __ZN7WebCore4Page16setDefersLoadingEb
-__ZN7WebCore4Page17addScrollableAreaEPNS_14ScrollableAreaE
 __ZN7WebCore4Page17willMoveOffscreenEv
 __ZN7WebCore4Page18removeSchedulePairEN3WTF10PassRefPtrINS_12SchedulePairEEE
 __ZN7WebCore4Page18setPageScaleFactorEfRKNS_8IntPointE
 __ZN7WebCore4Page19visitedStateChangedEPNS_9PageGroupEy
-__ZN7WebCore4Page20removeScrollableAreaEPNS_14ScrollableAreaE
 __ZN7WebCore4Page20setDeviceScaleFactorEf
 __ZN7WebCore4Page20unmarkAllTextMatchesEv
 __ZN7WebCore4Page21markAllMatchesForTextERKN3WTF6StringEjbj
@@ -1046,12 +1044,14 @@ __ZN7WebCore9FrameView14setNeedsLayoutEv
 __ZN7WebCore9FrameView14setTransparentEb
 __ZN7WebCore9FrameView15setMarginHeightEi
 __ZN7WebCore9FrameView16setPaintBehaviorEj
+__ZN7WebCore9FrameView17addScrollableAreaEPNS_14ScrollableAreaE
 __ZN7WebCore9FrameView17paintControlTintsEv
 __ZN7WebCore9FrameView17setScrollPositionERKNS_8IntPointE
 __ZN7WebCore9FrameView17setTracksRepaintsEb
 __ZN7WebCore9FrameView18updateControlTintsEv
 __ZN7WebCore9FrameView19scrollElementToRectEPNS_7ElementERKNS_7IntRectE
 __ZN7WebCore9FrameView20enterCompositingModeEv
+__ZN7WebCore9FrameView20removeScrollableAreaEPNS_14ScrollableAreaE
 __ZN7WebCore9FrameView21flushDeferredRepaintsEv
 __ZN7WebCore9FrameView22setBaseBackgroundColorERKNS_5ColorE
 __ZN7WebCore9FrameView23updateCanHaveScrollbarsEv

Source/WebCore/page/EventHandler.cpp

@@ -1656,8 +1656,10 @@ bool EventHandler::mouseMoved(const PlatformMouseEvent& event)
         return result;
 
     if (RenderLayer* layer = layerForNode(hoveredNode.innerNode())) {
-        if (page->containsScrollableArea(layer))
-            layer->mouseMovedInContentArea();
+        if (FrameView* frameView = m_frame->view()) {
+            if (frameView->containsScrollableArea(layer))
+                layer->mouseMovedInContentArea();
+        }
     }
 
     if (FrameView* frameView = m_frame->view())
@@ -2117,10 +2119,14 @@ void EventHandler::updateMouseEventTargetNode(Node* targetNode, const PlatformMo
             }
         } else if (page && (layerForLastNode && (!layerForNodeUnderMouse || layerForNodeUnderMouse != layerForLastNode))) {
             // The mouse has moved between layers.
-            if (page->containsScrollableArea(layerForLastNode))
-                layerForLastNode->mouseExitedContentArea();
+            if (Frame* frame = m_lastNodeUnderMouse->document()->frame()) {
+                if (FrameView* frameView = frame->view()) {
+                    if (frameView->containsScrollableArea(layerForLastNode))
+                        layerForLastNode->mouseExitedContentArea();
+                }
+            }
         }
-        
+
         if (m_nodeUnderMouse && (!m_lastNodeUnderMouse || m_lastNodeUnderMouse->document() != m_frame->document())) {
             // The mouse has moved between frames.
             if (Frame* frame = m_nodeUnderMouse->document()->frame()) {
@@ -2129,10 +2135,14 @@ void EventHandler::updateMouseEventTargetNode(Node* targetNode, const PlatformMo
             }
         } else if (page && (layerForNodeUnderMouse && (!layerForLastNode || layerForNodeUnderMouse != layerForLastNode))) {
             // The mouse has moved between layers.
-            if (page->containsScrollableArea(layerForNodeUnderMouse))
-                layerForNodeUnderMouse->mouseEnteredContentArea();
+            if (Frame* frame = m_nodeUnderMouse->document()->frame()) {
+                if (FrameView* frameView = frame->view()) {
+                    if (frameView->containsScrollableArea(layerForNodeUnderMouse))
+                        layerForNodeUnderMouse->mouseEnteredContentArea();
+                }
+            }
         }
-        
+
         if (m_lastNodeUnderMouse && m_lastNodeUnderMouse->document() != m_frame->document()) {
             m_lastNodeUnderMouse = 0;
             m_lastScrollbarUnderMouse = 0;

Source/WebCore/page/FocusController.cpp

@@ -582,6 +582,14 @@ void FocusController::setActive(bool active)
         dispatchEventsOnWindowAndFocusedNode(m_focusedFrame->document(), active);
 }
 
+static void contentAreaDidShowOrHide(ScrollableArea* scrollableArea, bool didShow)
+{
+    if (didShow)
+        scrollableArea->contentAreaDidShow();
+    else
+        scrollableArea->contentAreaDidHide();
+}
+
 void FocusController::setContainingWindowIsVisible(bool containingWindowIsVisible)
 {
     if (m_containingWindowIsVisible == containingWindowIsVisible)
@@ -593,13 +601,22 @@ void FocusController::setContainingWindowIsVisible(bool containingWindowIsVisibl
     if (!view)
         return;
 
-    if (const HashSet<ScrollableArea*>* scrollableAreas = m_page->scrollableAreaSet()) {
-        HashSet<ScrollableArea*>::const_iterator end = scrollableAreas->end(); 
-        for (HashSet<ScrollableArea*>::const_iterator it = scrollableAreas->begin(); it != end; ++it) {
-            if (!containingWindowIsVisible)
-                (*it)->contentAreaDidHide();
-            else
-                (*it)->contentAreaDidShow();
+    contentAreaDidShowOrHide(view, containingWindowIsVisible);
+
+    for (Frame* frame = m_page->mainFrame(); frame; frame = frame->tree()->traverseNext()) {
+        FrameView* frameView = frame->view();
+        if (!frameView)
+            continue;
+
+        const HashSet<ScrollableArea*>* scrollableAreas = frameView->scrollableAreas();
+        if (!scrollableAreas)
+            continue;
+
+        for (HashSet<ScrollableArea*>::const_iterator it = scrollableAreas->begin(), end = scrollableAreas->end(); it != end; ++it) {
+            ScrollableArea* scrollableArea = *it;
+            ASSERT(scrollableArea->isOnActivePage());
+
+            contentAreaDidShowOrHide(scrollableArea, containingWindowIsVisible);
         }
     }
 }

Source/WebCore/page/FrameView.cpp

@@ -152,16 +152,17 @@ FrameView::FrameView(Frame* frame)
 {
     init();
 
-    if (m_frame) {
-        if (Page* page = m_frame->page()) {
-            m_page = page;
-            m_page->addScrollableArea(this);
+    // FIXME: Can m_frame ever be null here?
+    if (!m_frame)
+        return;
 
-            if (m_frame == m_page->mainFrame()) {
-                ScrollableArea::setVerticalScrollElasticity(ScrollElasticityAllowed);
-                ScrollableArea::setHorizontalScrollElasticity(ScrollElasticityAllowed);
-            }
-        }
+    Page* page = m_frame->page();
+    if (!page)
+        return;
+
+    if (m_frame == page->mainFrame()) {
+        ScrollableArea::setVerticalScrollElasticity(ScrollElasticityAllowed);
+        ScrollableArea::setHorizontalScrollElasticity(ScrollElasticityAllowed);
     }
 }
 
@@ -202,9 +203,6 @@ FrameView::~FrameView()
     ASSERT(!m_scrollCorner);
     ASSERT(m_actionScheduler->isEmpty());
 
-    if (m_page)
-        m_page->removeScrollableArea(this);
-
     if (m_frame) {
         ASSERT(m_frame->view() != this || !m_frame->contentRenderer());
         RenderPart* renderer = m_frame->ownerRenderer();
@@ -1242,8 +1240,8 @@ void FrameView::removeWidgetToUpdate(RenderEmbeddedObject* object)
 void FrameView::zoomAnimatorTransformChanged(float scale, float x, float y, ZoomAnimationState state)
 {
     if (state == ZoomAnimationFinishing) {
-        m_page->setPageScaleFactor(m_page->pageScaleFactor() * scale,
-                                   IntPoint(scale * scrollX() - x, scale * scrollY() - y));
+        if (Page* page = m_frame->page())
+            page->setPageScaleFactor(page->pageScaleFactor() * scale, IntPoint(scale * scrollX() - x, scale * scrollY() - y));
         scrollAnimator()->resetZoom();
     }
 
@@ -2575,18 +2573,16 @@ void FrameView::setAnimatorsAreActive()
     if (!page)
         return;
 
-    const HashSet<ScrollableArea*>* scrollableAreas = page->scrollableAreaSet();
-    if (!scrollableAreas)
+    scrollAnimator()->setIsActive();
+
+    if (!m_scrollableAreas)
         return;
 
-    HashSet<ScrollableArea*>::const_iterator end = scrollableAreas->end(); 
-    for (HashSet<ScrollableArea*>::const_iterator it = scrollableAreas->begin(); it != end; ++it) {
-        // FIXME: This extra check to make sure the ScrollableArea is on an active page needs 
-        // to be here as long as the ScrollableArea HashSet lives on Page. But it should really be
-        // moved to the top-level Document or a similar class that really represents a single 
-        // web page. https://bugs.webkit.org/show_bug.cgi?id=62762
-        if ((*it)->isOnActivePage())
-            (*it)->scrollAnimator()->setIsActive();
+    for (HashSet<ScrollableArea*>::const_iterator it = m_scrollableAreas->begin(), end = m_scrollableAreas->end(); it != end; ++it) {
+        ScrollableArea* scrollableArea = *it;
+
+        ASSERT(scrollableArea->isOnActivePage());
+        scrollableArea->scrollAnimator()->setIsActive();
     }
 }
 
@@ -2596,21 +2592,26 @@ void FrameView::notifyPageThatContentAreaWillPaint() const
     if (!page)
         return;
 
-    const HashSet<ScrollableArea*>* scrollableAreas = page->scrollableAreaSet();
-    if (!scrollableAreas)
+    contentAreaWillPaint();
+
+    if (!m_scrollableAreas)
         return;
 
-    HashSet<ScrollableArea*>::const_iterator end = scrollableAreas->end(); 
-    for (HashSet<ScrollableArea*>::const_iterator it = scrollableAreas->begin(); it != end; ++it)
-        (*it)->contentAreaWillPaint();
+    for (HashSet<ScrollableArea*>::const_iterator it = m_scrollableAreas->begin(), end = m_scrollableAreas->end(); it != end; ++it) {
+        ScrollableArea* scrollableArea = *it;
+
+        ASSERT(scrollableArea->isOnActivePage());
+        scrollableArea->contentAreaWillPaint();
+    }
 }
 
 bool FrameView::scrollAnimatorEnabled() const
 {
 #if ENABLE(SMOOTH_SCROLLING)
-    if (m_page && m_page->settings())
-        return m_page->settings()->scrollAnimatorEnabled();
+    if (Page* page = m_frame->page())
+        return page->settings()->scrollAnimatorEnabled();
 #endif
+
     return false;
 }
 
@@ -3262,6 +3263,43 @@ void FrameView::setTracksRepaints(bool trackRepaints)
     m_isTrackingRepaints = trackRepaints;
 }
 
+void FrameView::addScrollableArea(ScrollableArea* scrollableArea)
+{
+    if (!m_scrollableAreas)
+        m_scrollableAreas = adoptPtr(new ScrollableAreaSet);
+    m_scrollableAreas->add(scrollableArea);
+}
+
+void FrameView::removeScrollableArea(ScrollableArea* scrollableArea)
+{
+    if (!m_scrollableAreas)
+        return;
+    m_scrollableAreas->remove(scrollableArea);
+}
+
+bool FrameView::containsScrollableArea(ScrollableArea* scrollableArea) const
+{
+    if (!m_scrollableAreas)
+        return false;
+    return m_scrollableAreas->contains(scrollableArea);
+}
+
+void FrameView::addChild(PassRefPtr<Widget> widget)
+{
+    if (widget->isFrameView())
+        addScrollableArea(static_cast<FrameView*>(widget.get()));
+
+    ScrollView::addChild(widget);
+}
+
+void FrameView::removeChild(Widget* widget)
+{
+    if (widget->isFrameView())
+        removeScrollableArea(static_cast<FrameView*>(widget));
+
+    ScrollView::removeChild(widget);
+}
+
 bool FrameView::isVerticalDocument() const
 {
     RenderView* root = rootRenderer(this);