Update WebView/FrameView size from LayoutView::UpdateAfterLayout

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 cd578f3903f9ac16aa8302409e85ce03aed6351b..74e498758d7b9bd7308064e9d4b40a94d47bdf05 100644
--- a/third_party/WebKit/Source/core/frame/FrameView.cpp
+++ b/third_party/WebKit/Source/core/frame/FrameView.cpp
@@ -554,12 +554,20 @@ void FrameView::SetFrameRect(const IntRect& frame_rect) {
   const bool height_changed = frame_rect_.Height() != frame_rect.Height();
   frame_rect_ = frame_rect;
 
-  needs_scrollbars_update_ = width_changed || height_changed;
-  // TODO(wjmaclean): find out why scrollbars fail to resize for complex
-  // subframes after changing the zoom level. For now always calling
-  // updateScrollbarsIfNeeded() here fixes the issue, but it would be good to
-  // discover the deeper cause of this. http://crbug.com/607987.

[bokan, 2017/05/10 21:30:07]

Does this CL fix the linked issue? e.g. if you run 

data:text/html;charset=utf8,<iframe width="300" height="300"
src="http://slashdot.org"></iframe>

With --site-per-process and ctrl+/- a few times, do the scrollbars position
correctly?

[szager1, 2017/05/11 12:50:54]

Yes, that appears to work correctly now.  I'll add the bug number to this CL.

-  UpdateScrollbarsIfNeeded();
+  needs_scrollbars_update_ |= width_changed || height_changed;

[bokan, 2017/05/10 21:30:07]

This change is subtle, is this maybe what fixes the old TODO?

[szager1, 2017/05/11 12:50:54]

With the old code, I couldn't see a path where needs_scrollbars_update_ would be
true entering SetFrameRect.  I could have missed something, though.

[bokan, 2017/05/11 16:51:08]

Ah, you're right, UpdateScrollbarsIfNeeded below would clear this.

+
+  // If this is not the main frame, then we got here via
+  // LayoutPart::UpdateGeometryInternal.  In that case, we can't clamp the
+  // scroll offset yet, because we still need to run UpdateLayout(), so our
+  // clamping boundaries may yet change.
+  if (GetFrame().IsMainFrame()) {
+    if (RuntimeEnabledFeatures::rootLayerScrollingEnabled()) {
+      if (LayoutView* lv = GetLayoutView())
+        lv->GetScrollableArea()->ClampScrollOffsetAfterOverflowChange();

[bokan, 2017/05/10 21:30:07]

Why do we need to special case the main frame here? Won't this get called from
LayoutView::UpdateAfterLayout()?

[szager1, 2017/05/11 12:50:54]

Without this, I get a unit test failure

VisualViewportTest.TestResizeAtFullyScrolledPreservesViewportLocation

[bokan, 2017/05/11 16:51:08]

Right, because hiding top controls will change the frame rect but won't cause a
layout. Could you move this to ViewportSizeChanged and add a comment that the
main frame needs this because of the top controls case I mentioned?

[szager1, 2017/05/11 20:12:00]

Done.

+    } else {
+      AdjustScrollOffsetFromUpdateScrollbars();
+    }
+  }
 
   FrameRectsChanged();
 
@@ -710,15 +718,13 @@ void FrameView::SetContentsSize(const IntSize& size) {
     return;
 
   contents_size_ = size;
-  UpdateScrollbars();
+  needs_scrollbars_update_ = true;
   ScrollableArea::ContentsResized();
 
   Page* page = GetFrame().GetPage();
   if (!page)
     return;
 
-  UpdateParentScrollableAreaSet();
-
   page->GetChromeClient().ContentsSizeChanged(frame_.Get(), size);
 
   // Ensure the scrollToFragmentAnchor is called before
@@ -743,11 +749,6 @@ void FrameView::AdjustViewSize() {
   const IntPoint origin(-rect.X(), -rect.Y());
   if (ScrollOrigin() != origin) {

[bokan, 2017/05/10 21:30:07]

Nit: remove braces

[szager1, 2017/05/11 12:50:54]

Done.

     SetScrollOrigin(origin);
-    // setContentSize (below) also calls updateScrollbars so we can avoid
-    // updating scrollbars twice by skipping the call here when the content
-    // size does not change.
-    if (!frame_->GetDocument()->Printing() && size == ContentsSize())
-      UpdateScrollbars();
   }
 
   SetContentsSize(size);
@@ -885,6 +886,7 @@ void FrameView::RecalcOverflowAfterStyleChange() {
 
   AdjustViewSize();
   UpdateScrollbarGeometry();
+  SetNeedsPaintPropertyUpdate();
 
   if (ScrollOriginChanged())
     SetNeedsLayout();
@@ -1308,10 +1310,25 @@ void FrameView::UpdateLayout() {
         TRACE_DISABLED_BY_DEFAULT("blink.debug.layout.trees"), "LayoutTree",
         this, TracedLayoutObject::Create(*GetLayoutView(), false));
 
+    IntSize old_size(Size());
+
     PerformLayout(in_subtree_layout);
+    UpdateScrollbars();
+    UpdateParentScrollableAreaSet();
 
-    if (!in_subtree_layout && !document->Printing())
-      AdjustViewSizeAndLayout();
+    IntSize new_size(Size());
+    if (old_size != new_size) {
+      needs_scrollbars_update_ = true;
+      SetNeedsLayout();
+      MarkViewportConstrainedObjectsForLayout(

[bokan, 2017/05/10 21:30:07]

Will ViewportConstrainedObjects still get laid out if the frame rect doesn't
change but scrollbar existence does?

[szager1, 2017/05/11 12:50:54]

Yes, because UpdateScrollbars() above will recursively call into UpdateLayout,
and the viewport constrained objects will be invalidated through normal layout
because the width available to children has changed.

[bokan, 2017/05/11 16:51:08]

Got it, thanks.

+          old_size.Width() != new_size.Width(),
+          old_size.Height() != new_size.Height());
+    }
+
+    if (NeedsLayout()) {
+      AutoReset<bool> suppress(&suppress_adjust_view_size_, true);
+      UpdateLayout();
+    }
 
     ASSERT(layout_subtree_root_list_.IsEmpty());
   }  // Reset m_layoutSchedulingEnabled to its previous value.
@@ -1671,18 +1688,6 @@ void FrameView::ViewportSizeChanged(bool width_changed, bool height_changed) {
 
   ShowOverlayScrollbars();
 
-  if (root_layer_scrolling_enabled) {
-    // 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
-    // to issue the invalidation).  This is because the background fills the
-    // main GraphicsLayer, which takes the size of the layout viewport.
-    // TODO(skobes): Paint non-fixed backgrounds into the scrolling contents
-    // layer and avoid this invalidation (http://crbug.com/568847).
-    LayoutViewItem lvi = GetLayoutViewItem();
-    if (!lvi.IsNull())
-      lvi.SetShouldDoFullPaintInvalidation();

[bokan, 2017/05/10 21:30:07]

Why do we no longer need this? There's cases where the viewport size can change
and we need an invalidation but we don't get a layout. i.e. on Android, hiding
the URL bar. Is this covered in the cases below?

[szager1, 2017/05/11 12:50:54]

The background paint invalidation issue was fixed separately (see the bug), I
believe this code was already obsolete.

[bokan, 2017/05/11 16:51:08]

Acknowledged.

-  }
-
   if (RuntimeEnabledFeatures::inertTopControlsEnabled() && GetLayoutView() &&
       frame_->IsMainFrame() &&
       frame_->GetPage()->GetBrowserControls().Height()) {
@@ -1694,13 +1699,13 @@ void FrameView::ViewportSizeChanged(bool width_changed, bool height_changed) {
       PaintLayer* layer = GetLayoutView()->Layer();
       if (GetLayoutView()->Compositor()->NeedsFixedRootBackgroundLayer(layer)) {
         SetNeedsLayout();
-      } else if (!root_layer_scrolling_enabled) {
+      } else {
         // If root layer scrolls is on, we've already issued a full invalidation
         // above.
         GetLayoutView()->SetShouldDoFullPaintInvalidationOnResizeIfNeeded(
             width_changed, height_changed);
       }
-    } else if (height_changed && !root_layer_scrolling_enabled) {
+    } else if (height_changed) {
       // If the document rect doesn't fill the full view height, hiding the
       // URL bar will expose area outside the current LayoutView so we need to
       // paint additional background. If RLS is on, we've already invalidated
@@ -2171,9 +2176,6 @@ void FrameView::ScrollbarExistenceDidChange() {
       ScrollbarTheme::GetTheme().UsesOverlayScrollbars() &&
       !ShouldUseCustomScrollbars(custom_scrollbar_element);
 
-  // FIXME: this call to layout() could be called within FrameView::layout(),
-  // but before performLayout(), causing double-layout. See also
-  // crbug.com/429242.
   if (!uses_overlay_scrollbars && NeedsLayout())
     UpdateLayout();
 
@@ -3599,6 +3601,8 @@ void FrameView::ForceLayoutForPagination(const FloatSize& page_size,
     }
   }
 
+  if (TextAutosizer* text_autosizer = frame_->GetDocument()->GetTextAutosizer())
+    text_autosizer->UpdatePageInfo();
   AdjustViewSizeAndLayout();
 }
 
@@ -4312,7 +4316,7 @@ bool FrameView::AdjustScrollbarExistence(
     return true;
 
   if (!HasOverlayScrollbars())
-    ContentsResized();
+    SetNeedsLayout();
   ScrollbarExistenceDidChange();
   return true;
 }