Remove top controls clipping adjustment for document.rootScroller

third_party/WebKit/Source/core/frame/FrameView.cpp

Index: third_party/WebKit/Source/core/frame/FrameView.cpp
diff --git a/third_party/WebKit/Source/core/frame/FrameView.cpp b/third_party/WebKit/Source/core/frame/FrameView.cpp
index a81c4447504eac6862e5716524517fc60d42c3dc..b18d8775da1e5a6dc2459865cc3df4d55a7ab60f 100644
--- a/third_party/WebKit/Source/core/frame/FrameView.cpp
+++ b/third_party/WebKit/Source/core/frame/FrameView.cpp
@@ -1628,20 +1628,20 @@ void FrameView::viewportSizeChanged(bool widthChanged, bool heightChanged) {
   DCHECK(widthChanged || heightChanged);
   DCHECK(m_frame->page());
 
+  bool rootLayerScrollingEnabled =
+      RuntimeEnabledFeatures::rootLayerScrollingEnabled();
+
   if (LayoutViewItem layoutView = this->layoutViewItem()) {
-    if (layoutView.usesCompositing())
-      layoutView.compositor()->frameViewDidChangeSize();
+    if (layoutView.usesCompositing()) {
+      if (rootLayerScrollingEnabled)
+        layoutView.layer()->setNeedsCompositingInputsUpdate();

[chrishtr, 2017/03/16 18:46:57]

This is new since last time you landed - a new dependency on RLS?

[bokan, 2017/03/16 18:59:48]

So I'm hitting a DCHECK in RLS tests without this:

'[FATAL:FindPropertiesNeedingUpdate.h(160)] Check failed:
*m_originalProperties->overflowClip() == *objectProperties->overflowClip().
Property was updated without the layout object ("LayoutBlockFlow (positioned)
DIV id='stream-view' class='view'") needing a paint property update.

Either this check was added in that time or RLS started doing something new. It
seems that we're changing the clip rect on the layer without telling paint about
it (I believe the root layer's overflow clip uses the rect supplied from the
FrameView but we're missing some kind of invalidation). I suspect this may not
be the right fix?

[Xianzhu, 2017/03/16 19:16:54]

I'm trying to understand the error, and have some questions:

- I know viewport size affects LayoutView's overflow clip with RLS, but how does
it affect overflow clip of a normal LayoutBlockFlow? Which test is it?

- This question is about "viewport size affects LayutView's overflow clip", : Is
LayoutView::overflowClipRect() equivalent to LayoutBox::overflowClipRect() with
RLS? If not, why is the difference?

[bokan, 2017/03/16 20:11:09]

It shouldn't affect a regular LayoutBlockFlow at all, but perhaps I'm
misunderstanding - URL bar affects only the PaintLayerCompositor/LayoutView. In
LayoutView::overflowClipRect we use the viewRect which is really the FrameView's
visibleContentRect. That changes as the URL bar hides/shows. I think for non-RLS
we don't clip LayoutView at all which is why this doesn't trigger for non-RLS -
we never seem to read LayoutView::overflowClipRect.

These tests fail without this line:
   
All/ParameterizedWebFrameTest.SetForceZeroLayoutHeightWorksWithWrapContentMode/1
    All/VisualViewportTest.InvalidateLayoutViewWhenDocumentSmallerThanView/1 
    All/VisualViewportTest.ResizeNonCompositedAndFixedBackground/1 
    All/VisualViewportTest.ResizeNonFixedBackgroundNoLayoutOrInvalidation/1 
I believe not. The reason being that LayoutBox::overflowClipRect is basically
equivalent to the border box. The two will be the same while the URL bar is
showing. When the URL bar is hidden, we don't resize the ICB so the LayoutView's
border box remains unchanged while it's overflowClipRect will match the
FrameView visibleContentRect which is now expanded by the hidden URL bar.

[Xianzhu, 2017/03/21 16:59:56]

Did these tests all fail with error messages similar to the one you pasted? If
yes, it seems that viewport resize does affect a normal LayoutBlockFlow's
overflowClip. Can you make clear how this happened, and how
setNeedsCompositingInputsUpdate() fixes it?

[Xianzhu, 2017/03/21 17:26:41]

I just tried the patch and found that all the failures are about LayoutView's
paint property, not a "normal LayoutBlockFlow" like in the pasted error message.
I think my questions are resolved.

I think we should put setNeedsPaintPropertyUpdate() here, in addition to
setNeedsCompositingInputsUpdate(). The latter may imply the former for SPv1 but
may not for SPv2.

[bokan, 2017/03/23 15:09:57]

Yeah, sorry, I think I may have pasted that DCHECK from a separate failure that
I was having in a followup patch that I thought was the same.

I've added setNeedsPaintPropertyUpdate. Should it be guarded by
slimmingPaintInvalidationEnabled()?

[Xianzhu, 2017/03/23 15:39:16]

It's better to be guarded to be consistent with other places.

[bokan, 2017/03/23 16:01:03]

Done.

+      else
+        layoutView.compositor()->frameViewDidChangeSize();
+    }
   }
 
-  // Ensure the root scroller compositing layers update geometry in response to
-  // the URL bar resizing.
-  if (m_frame->isMainFrame())
-    m_frame->page()->globalRootScrollerController().mainFrameViewResized();
-
   showOverlayScrollbars();
 
-  bool rootLayerScrollingEnabled =
-      RuntimeEnabledFeatures::rootLayerScrollingEnabled();
   if (rootLayerScrollingEnabled) {
     // The background must be repainted when the FrameView is resized, even if
     // the initial containing block does not change (so we can't rely on layout