Widget's position change should not initiate layout, only when its size changes.
authorzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 6 Nov 2013 07:31:52 +0000 (07:31 +0000)
committerzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 6 Nov 2013 07:31:52 +0000 (07:31 +0000)
https://bugs.webkit.org/show_bug.cgi?id=123860

Reviewed by Andreas Kling.

RenderWidgets initiate unnecessary layouts while scrolling when they are embedded to
overflow:scroll containers. Scroll position change doesn't dirty the render tree
so it should not trigger layout either.

.:

* ManualTests/layouts-on-renderwidgets-while-scrolling.html: Added.

Source/WebCore:

Manual test added. Unfortunately we can't test against the number of layouts yet.

* rendering/RenderWidget.cpp:
(WebCore::RenderWidget::setWidgetGeometry):
(WebCore::RenderWidget::updateWidgetGeometry):
(WebCore::RenderWidget::updateWidgetPosition):

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

ChangeLog
ManualTests/layouts-on-renderwidgets-while-scrolling.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderWidget.cpp

index 2a941ca..01065b9 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,16 @@
+2013-11-05  Zalan Bujtas  <zalan@apple.com>
+
+        Widget's position change should not initiate layout, only when its size changes.
+        https://bugs.webkit.org/show_bug.cgi?id=123860
+
+        Reviewed by Andreas Kling.
+
+        RenderWidgets initiate unnecessary layouts while scrolling when they are embedded to
+        overflow:scroll containers. Scroll position change doesn't dirty the render tree
+        so it should not trigger layout either.
+
+        * ManualTests/layouts-on-renderwidgets-while-scrolling.html: Added.
+
 2013-11-05  Éva Balázsfalvi  <balazsfalvi.eva@stud.u-szeged.hu>
 
         Remove leftover Qt related things from WebKitMacros.cmake
diff --git a/ManualTests/layouts-on-renderwidgets-while-scrolling.html b/ManualTests/layouts-on-renderwidgets-while-scrolling.html
new file mode 100644 (file)
index 0000000..acebca9
--- /dev/null
@@ -0,0 +1,26 @@
+<!doctype html>
+<html>
+<head>
+  <style>
+    html, body {
+      height: 100%;
+    }
+    
+    .container {
+      height: 100%;
+      overflow: scroll;
+    }
+    iframe {
+      display: block;
+      width: 200px;
+      height: 600px;
+    }
+  </style>
+</head>
+<body>
+  <div class="container">
+    <iframe src="data:text/html,Scroll this out of the view."></iframe>
+    <iframe src="data:text/html,Stop here and inspect if layouts got initiated, while scolling in the Web Inspector."></iframe>
+  </div>
+</body>
+</html>
index 846d286..cd08e65 100644 (file)
@@ -1,3 +1,21 @@
+2013-11-05  Zalan Bujtas  <zalan@apple.com>
+
+        Widget's position change should not initiate layout, only when its size changes.
+        https://bugs.webkit.org/show_bug.cgi?id=123860
+
+        Reviewed by Andreas Kling.
+
+        RenderWidgets initiate unnecessary layouts while scrolling when they are embedded to
+        overflow:scroll containers. Scroll position change doesn't dirty the render tree
+        so it should not trigger layout either.
+
+        Manual test added. Unfortunately we can't test against the number of layouts yet.
+
+        * rendering/RenderWidget.cpp:
+        (WebCore::RenderWidget::setWidgetGeometry):
+        (WebCore::RenderWidget::updateWidgetGeometry):
+        (WebCore::RenderWidget::updateWidgetPosition):
+
 2013-11-05  Ryosuke Niwa  <rniwa@webkit.org>
 
         Use-after-free in SliderThumbElement::dragFrom
index 9495efb..d965c18 100644 (file)
@@ -118,9 +118,10 @@ static inline IntRect roundedIntRect(const LayoutRect& rect)
 bool RenderWidget::setWidgetGeometry(const LayoutRect& frame)
 {
     IntRect clipRect = roundedIntRect(enclosingLayer()->childrenClipRect());
-    IntRect newFrame = roundedIntRect(frame);
+    IntRect newFrameRect = roundedIntRect(frame);
+    IntRect oldFrameRect = m_widget->frameRect();
     bool clipChanged = m_clipRect != clipRect;
-    bool boundsChanged = m_widget->frameRect() != newFrame;
+    bool boundsChanged = oldFrameRect != newFrameRect;
 
     if (!boundsChanged && !clipChanged)
         return false;
@@ -130,7 +131,7 @@ bool RenderWidget::setWidgetGeometry(const LayoutRect& frame)
     WeakPtr<RenderWidget> weakThis = createWeakPtr();
     // These calls *may* cause this renderer to disappear from underneath...
     if (boundsChanged)
-        m_widget->setFrameRect(newFrame);
+        m_widget->setFrameRect(newFrameRect);
     else if (clipChanged)
         m_widget->clipRectChanged();
     // ...so we follow up with a sanity check.
@@ -138,19 +139,18 @@ bool RenderWidget::setWidgetGeometry(const LayoutRect& frame)
         return true;
 
 #if USE(ACCELERATED_COMPOSITING)
-    if (hasLayer() && layer()->isComposited())
+    if (boundsChanged && hasLayer() && layer()->isComposited())
         layer()->backing()->updateAfterWidgetResize();
 #endif
-    
-    return boundsChanged;
+    return oldFrameRect.size() != newFrameRect.size();
 }
 
 bool RenderWidget::updateWidgetGeometry()
 {
-    LayoutRect contentBox = contentBoxRect();
     if (!m_widget->transformsAffectFrameRect())
         return setWidgetGeometry(absoluteContentBox());
 
+    LayoutRect contentBox = contentBoxRect();
     LayoutRect absoluteContentBox(localToAbsoluteQuad(FloatQuad(contentBox)).boundingBox());
     if (m_widget->isFrameView()) {
         contentBox.setLocation(absoluteContentBox.location());
@@ -316,18 +316,16 @@ void RenderWidget::updateWidgetPosition()
         return;
 
     WeakPtr<RenderWidget> weakThis = createWeakPtr();
-
-    bool boundsChanged = updateWidgetGeometry();
-
+    bool widgetSizeChanged = updateWidgetGeometry();
     if (!weakThis)
         return;
 
-    // if the frame bounds got changed, or if view needs layout (possibly indicating
-    // content size is wrong) we have to do a layout to set the right widget size
-    if (m_widget && m_widget->isFrameView()) {
+    // if the frame size got changed, or if view needs layout (possibly indicating
+    // content size is wrong) we have to do a layout to set the right widget size.
+    if (m_widget->isFrameView()) {
         FrameView* frameView = toFrameView(m_widget.get());
         // Check the frame's page to make sure that the frame isn't in the process of being destroyed.
-        if ((boundsChanged || frameView->needsLayout()) && frameView->frame().page())
+        if ((widgetSizeChanged || frameView->needsLayout()) && frameView->frame().page())
             frameView->layout();
     }
 }