Avoid extra scrollbar-related layouts for overlay scrollbars
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 12 Sep 2013 23:28:32 +0000 (23:28 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 12 Sep 2013 23:28:32 +0000 (23:28 +0000)
https://bugs.webkit.org/show_bug.cgi?id=121267

Source/WebCore:

Reviewed by Beth Dakin.

If ScrollView::updateScrollbars() detected that scrollbars were added
and removed, it would call contentsResized(), which calls setNeedsLayout(),
followed by visibleContentsResized() which would trigger layout. There is no
point doing this with overlay scrollbars, so avoid it by having
setHas*Scrollbar() return true if the addition/removal of a scrollbar changed
the available width.

No tests: we can't test overlay scrollbars in tests.

* page/FrameView.cpp:
(WebCore::FrameView::setContentsSize): Drive-by assertion that
checks that the unsigned m_deferSetNeedsLayouts doesn't wrap when
decremented.
* platform/ScrollView.cpp:
(WebCore::ScrollView::setHasHorizontalScrollbar): Return true if the addition/removal
changed available space.
(WebCore::ScrollView::setHasVerticalScrollbar): Ditto.
(WebCore::ScrollView::updateScrollbars): Only set sendContentResizedNotification
if available space was changed by addition/removal of scrollbars.
* platform/ScrollView.h:

Source/WebKit2:

Reviewed by Beth Dakin.

view->resize() will call setNeedsLayout() if necessary, and may already have
done layout, so the extra setNeedsLayout() here was bad.

* WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::setSize):

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

Source/WebCore/ChangeLog
Source/WebCore/page/FrameView.cpp
Source/WebCore/platform/ScrollView.cpp
Source/WebCore/platform/ScrollView.h
Source/WebKit2/ChangeLog
Source/WebKit2/WebProcess/WebPage/WebPage.cpp

index df78407..fc59e2f 100644 (file)
@@ -1,3 +1,31 @@
+2013-09-12  Simon Fraser  <simon.fraser@apple.com>
+
+        Avoid extra scrollbar-related layouts for overlay scrollbars
+        https://bugs.webkit.org/show_bug.cgi?id=121267
+
+        Reviewed by Beth Dakin.
+
+        If ScrollView::updateScrollbars() detected that scrollbars were added
+        and removed, it would call contentsResized(), which calls setNeedsLayout(),
+        followed by visibleContentsResized() which would trigger layout. There is no
+        point doing this with overlay scrollbars, so avoid it by having 
+        setHas*Scrollbar() return true if the addition/removal of a scrollbar changed
+        the available width.
+        
+        No tests: we can't test overlay scrollbars in tests.
+
+        * page/FrameView.cpp:
+        (WebCore::FrameView::setContentsSize): Drive-by assertion that
+        checks that the unsigned m_deferSetNeedsLayouts doesn't wrap when
+        decremented.
+        * platform/ScrollView.cpp:
+        (WebCore::ScrollView::setHasHorizontalScrollbar): Return true if the addition/removal
+        changed available space.
+        (WebCore::ScrollView::setHasVerticalScrollbar): Ditto.
+        (WebCore::ScrollView::updateScrollbars): Only set sendContentResizedNotification
+        if available space was changed by addition/removal of scrollbars.
+        * platform/ScrollView.h:
+
 2013-09-12  Zoltan Horvath  <zoltan@webkit.org>
 
         [CSS Shapes] Rename shapeContainingBlockHeight to shapeContainingBlockLogicalHeight
index 8065eac..ddf7bac 100644 (file)
@@ -581,6 +581,7 @@ void FrameView::setContentsSize(const IntSize& size)
 
     page->chrome().contentsSizeChanged(&frame(), size); // Notify only.
 
+    ASSERT(m_deferSetNeedsLayouts);
     m_deferSetNeedsLayouts--;
     
     if (!m_deferSetNeedsLayouts)
index 1a5c719..0813d3a 100644 (file)
@@ -84,7 +84,7 @@ void ScrollView::removeChild(Widget* child)
         platformRemoveChild(child);
 }
 
-void ScrollView::setHasHorizontalScrollbar(bool hasBar)
+bool ScrollView::setHasHorizontalScrollbar(bool hasBar)
 {
     ASSERT(!hasBar || !avoidScrollbarCreation());
     if (hasBar && !m_horizontalScrollbar) {
@@ -92,14 +92,21 @@ void ScrollView::setHasHorizontalScrollbar(bool hasBar)
         addChild(m_horizontalScrollbar.get());
         didAddScrollbar(m_horizontalScrollbar.get(), HorizontalScrollbar);
         m_horizontalScrollbar->styleChanged();
-    } else if (!hasBar && m_horizontalScrollbar) {
+        return !m_horizontalScrollbar->isOverlayScrollbar();
+    }
+    
+    if (!hasBar && m_horizontalScrollbar) {
+        bool wasOverlayScrollbar = m_horizontalScrollbar->isOverlayScrollbar();
         willRemoveScrollbar(m_horizontalScrollbar.get(), HorizontalScrollbar);
         removeChild(m_horizontalScrollbar.get());
         m_horizontalScrollbar = 0;
+        return !wasOverlayScrollbar;
     }
+
+    return false;
 }
 
-void ScrollView::setHasVerticalScrollbar(bool hasBar)
+bool ScrollView::setHasVerticalScrollbar(bool hasBar)
 {
     ASSERT(!hasBar || !avoidScrollbarCreation());
     if (hasBar && !m_verticalScrollbar) {
@@ -107,11 +114,18 @@ void ScrollView::setHasVerticalScrollbar(bool hasBar)
         addChild(m_verticalScrollbar.get());
         didAddScrollbar(m_verticalScrollbar.get(), VerticalScrollbar);
         m_verticalScrollbar->styleChanged();
-    } else if (!hasBar && m_verticalScrollbar) {
+        return !m_verticalScrollbar->isOverlayScrollbar();
+    }
+    
+    if (!hasBar && m_verticalScrollbar) {
+        bool wasOverlayScrollbar = m_verticalScrollbar->isOverlayScrollbar();
         willRemoveScrollbar(m_verticalScrollbar.get(), VerticalScrollbar);
         removeChild(m_verticalScrollbar.get());
         m_verticalScrollbar = 0;
+        return !wasOverlayScrollbar;
     }
+
+    return false;
 }
 
 #if !PLATFORM(GTK)
@@ -542,8 +556,8 @@ void ScrollView::updateScrollbars(const IntSize& desiredOffset)
                 ScrollableArea::setScrollOrigin(IntPoint(scrollOrigin().x(), scrollOrigin().y() - m_horizontalScrollbar->height()));
             if (m_horizontalScrollbar)
                 m_horizontalScrollbar->invalidate();
-            setHasHorizontalScrollbar(newHasHorizontalScrollbar);
-            sendContentResizedNotification = true;
+
+            sendContentResizedNotification = setHasHorizontalScrollbar(newHasHorizontalScrollbar);
         }
 
         if (hasVerticalScrollbar != newHasVerticalScrollbar && (hasVerticalScrollbar || !avoidScrollbarCreation())) {
@@ -551,8 +565,8 @@ void ScrollView::updateScrollbars(const IntSize& desiredOffset)
                 ScrollableArea::setScrollOrigin(IntPoint(scrollOrigin().x() - m_verticalScrollbar->width(), scrollOrigin().y()));
             if (m_verticalScrollbar)
                 m_verticalScrollbar->invalidate();
-            setHasVerticalScrollbar(newHasVerticalScrollbar);
-            sendContentResizedNotification = true;
+
+            sendContentResizedNotification = setHasVerticalScrollbar(newHasVerticalScrollbar);
         }
 
         if (sendContentResizedNotification && m_updateScrollbarsPass < cMaxUpdateScrollbarsPass) {
index 880554e..8de04fb 100644 (file)
@@ -315,8 +315,9 @@ protected:
     virtual void fixedLayoutSizeChanged();
 
     // These functions are used to create/destroy scrollbars.
-    void setHasHorizontalScrollbar(bool);
-    void setHasVerticalScrollbar(bool);
+    // They return true if the space taken up by the scrollbar changed.
+    bool setHasHorizontalScrollbar(bool);
+    bool setHasVerticalScrollbar(bool);
 
     virtual void updateScrollCorner();
     virtual void invalidateScrollCornerRect(const IntRect&) OVERRIDE;
index 77518df..425e5fe 100644 (file)
@@ -1,3 +1,16 @@
+2013-09-12  Simon Fraser  <simon.fraser@apple.com>
+
+        Avoid extra scrollbar-related layouts for overlay scrollbars
+        https://bugs.webkit.org/show_bug.cgi?id=121267
+
+        Reviewed by Beth Dakin.
+        
+        view->resize() will call setNeedsLayout() if necessary, and may already have
+        done layout, so the extra setNeedsLayout() here was bad.
+
+        * WebProcess/WebPage/WebPage.cpp:
+        (WebKit::WebPage::setSize):
+
 2013-09-12  Andre Moreira Magalhaes   <andre.magalhaes@collabora.co.uk>
 
         Web Inspector: Do not try to parse incomplete HTTP requests
index 9277540..a862c33 100644 (file)
@@ -1068,7 +1068,6 @@ void WebPage::setSize(const WebCore::IntSize& viewSize)
 
     FrameView* view = m_page->mainFrame().view();
     view->resize(viewSize);
-    view->setNeedsLayout();
     m_drawingArea->setNeedsDisplay();
     
     m_viewSize = viewSize;