Changeset 155660 in webkit

Timestamp:

Sep 12, 2013 4:28:32 PM (11 years ago)

Author:

Simon Fraser

Message:

Avoid extra scrollbar-related layouts for overlay scrollbars
​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):

Location:

trunk/Source

Files:

• WebCore/ChangeLog (modified) (1 diff)
• WebCore/page/FrameView.cpp (modified) (1 diff)
• WebCore/platform/ScrollView.cpp (modified) (5 diffs)
• WebCore/platform/ScrollView.h (modified) (1 diff)
• WebKit2/ChangeLog (modified) (1 diff)
• WebKit2/WebProcess/WebPage/WebPage.cpp (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/page/FrameView.cpp

@@ -582 +582 @@
     page->chrome().contentsSizeChanged(&frame(), size); // Notify only.
 
+    ASSERT(m_deferSetNeedsLayouts);
     m_deferSetNeedsLayouts--;
     

trunk/Source/WebCore/platform/ScrollView.cpp

@@ -85 +85 @@
 }
 
-void ScrollView::setHasHorizontalScrollbar(bool hasBar)
+bool ScrollView::setHasHorizontalScrollbar(bool hasBar)
 {
     ASSERT(!hasBar || !avoidScrollbarCreation());
@@ -93 +93 @@
         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;
-    }
-}
-
-void ScrollView::setHasVerticalScrollbar(bool hasBar)
+        return !wasOverlayScrollbar;
+    }
+
+    return false;
+}
+
+bool ScrollView::setHasVerticalScrollbar(bool hasBar)
 {
     ASSERT(!hasBar || !avoidScrollbarCreation());
@@ -108 +115 @@
         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;
 }
 
@@ -543 +557 @@
             if (m_horizontalScrollbar)
                 m_horizontalScrollbar->invalidate();
-            setHasHorizontalScrollbar(newHasHorizontalScrollbar);
-            sendContentResizedNotification = true;
+
+            sendContentResizedNotification = setHasHorizontalScrollbar(newHasHorizontalScrollbar);
         }
 
@@ -552 +566 @@
             if (m_verticalScrollbar)
                 m_verticalScrollbar->invalidate();
-            setHasVerticalScrollbar(newHasVerticalScrollbar);
-            sendContentResizedNotification = true;
+
+            sendContentResizedNotification = setHasVerticalScrollbar(newHasVerticalScrollbar);
         }
 

trunk/Source/WebCore/platform/ScrollView.h

@@ -316 +316 @@
 
     // 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();

trunk/Source/WebKit2/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebKit2/WebProcess/WebPage/WebPage.cpp

@@ -1069 +1069 @@
     FrameView* view = m_page->mainFrame().view();
     view->resize(viewSize);
-    view->setNeedsLayout();
     m_drawingArea->setNeedsDisplay();