Update WebView/FrameView size from LayoutView::UpdateAfterLayout

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

 /*
  * Copyright (C) 1998, 1999 Torben Weis <weis@kde.org>
  *                     1999 Lars Knoll <knoll@kde.org>
  *                     1999 Antti Koivisto <koivisto@kde.org>
  *                     2000 Dirk Mueller <mueller@kde.org>
  * Copyright (C) 2004, 2005, 2006, 2007, 2008 Apple Inc. All rights reserved.
  *           (C) 2006 Graham Dennis (graham.dennis@gmail.com)
  *           (C) 2006 Alexey Proskuryakov (ap@nypop.com)
  * Copyright (C) 2009 Google Inc. All rights reserved.
  *
@@ skipping 536 matching lines @@
 }
 
 void FrameView::SetFrameRect(const IntRect& frame_rect) {
   if (frame_rect == frame_rect_)
     return;
 
   const bool width_changed = frame_rect_.Width() != frame_rect.Width();
   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();
 
   UpdateParentScrollableAreaSet();
 
   if (RuntimeEnabledFeatures::slimmingPaintInvalidationEnabled() &&
       !RuntimeEnabledFeatures::rootLayerScrollingEnabled()) {
     // The overflow clip property depends on the frame size and the pre
     // translation property depends on the frame location.
     SetNeedsPaintPropertyUpdate();
@@ skipping 130 matching lines @@
 
 Scrollbar* FrameView::CreateScrollbar(ScrollbarOrientation orientation) {
   return scrollbar_manager_.CreateScrollbar(orientation);
 }
 
 void FrameView::SetContentsSize(const IntSize& size) {
   if (size == ContentsSize())
     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
   // restoreScrollPositionAndViewState when reload
   ScrollToFragmentAnchor();
   GetFrame().Loader().RestoreScrollPositionAndViewState();
 }
 
 void FrameView::AdjustViewSize() {
   if (suppress_adjust_view_size_)
     return;
 
   LayoutViewItem layout_view_item = this->GetLayoutViewItem();
   if (layout_view_item.IsNull())
     return;
 
   ASSERT(frame_->View() == this);
 
   const IntRect rect = layout_view_item.DocumentRect();
   const IntSize& size = rect.Size();
 
   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);
 }
 
 void FrameView::AdjustViewSizeAndLayout() {
   AdjustViewSize();
   if (NeedsLayout()) {
     AutoReset<bool> suppress_adjust_view_size(&suppress_adjust_view_size_,
                                               true);
@@ skipping 117 matching lines @@
   bool has_horizontal_scrollbar = HorizontalScrollbar();
   bool has_vertical_scrollbar = VerticalScrollbar();
   if (has_horizontal_scrollbar != should_have_horizontal_scrollbar ||
       has_vertical_scrollbar != should_have_vertical_scrollbar) {
     SetNeedsLayout();
     return;
   }
 
   AdjustViewSize();
   UpdateScrollbarGeometry();
+  SetNeedsPaintPropertyUpdate();
 
   if (ScrollOriginChanged())
     SetNeedsLayout();
 }
 
 bool FrameView::UsesCompositedScrolling() const {
   LayoutViewItem layout_view = this->GetLayoutViewItem();
   if (layout_view.IsNull())
     return false;
   if (frame_->GetSettings() &&
@@ skipping 403 matching lines @@
         else if (root_layout_object &&
                  root_layout_object->StretchesToViewport())
           root_layout_object->SetChildNeedsLayout();
       }
     }
 
     TRACE_EVENT_OBJECT_SNAPSHOT_WITH_ID(
         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.
   CheckDoesNotNeedLayout();
 
   frame_timing_requests_dirty_ = true;
 
   // FIXME: Could find the common ancestor layer of all dirty subtrees and mark
   // from there. crbug.com/462719
   GetLayoutViewItem().EnclosingLayer()->UpdateLayerPositionsAfterLayout();
@@ skipping 339 matching lines @@
         if (RuntimeEnabledFeatures::slimmingPaintInvalidationEnabled())
           SetNeedsPaintPropertyUpdate();
       } else {
         layout_view.Compositor()->FrameViewDidChangeSize();
       }
     }
   }
 
   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()) {
     if (GetLayoutView()->Style()->HasFixedBackgroundImage()) {
       // In the case where we don't change layout size from top control resizes,
       // we wont perform a layout. If we have a fixed background image however,
       // the background layer needs to get resized so we should request a layout
       // explicitly.
       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
       // above.
       LayoutViewItem lvi = GetLayoutViewItem();
       DCHECK(!lvi.IsNull());
       if (lvi.DocumentRect().Height() < lvi.ViewRect().Height())
         lvi.SetShouldDoFullPaintInvalidation();
     }
   }
@@ skipping 450 matching lines @@
   // frame().
   if (!GetFrame().View())
     return;
 
   Element* custom_scrollbar_element = nullptr;
 
   bool uses_overlay_scrollbars =
       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();
 
   if (!GetLayoutViewItem().IsNull() && GetLayoutViewItem().UsesCompositing()) {
     GetLayoutViewItem().Compositor()->FrameViewScrollbarsExistenceDidChange();
 
     if (!uses_overlay_scrollbars)
       GetLayoutViewItem().Compositor()->FrameViewDidChangeSize();
   }
 }
@@ skipping 1405 matching lines @@
       if (!horizontal_writing_mode)
         overflow = overflow.TransposedRect();
       AdjustViewSizeAndLayout();
       // This is how we clip in case we overflow again.
       layout_view->ClearLayoutOverflow();
       layout_view->AddLayoutOverflow(overflow);
       return;
     }
   }
 
+  if (TextAutosizer* text_autosizer = frame_->GetDocument()->GetTextAutosizer())
+    text_autosizer->UpdatePageInfo();
   AdjustViewSizeAndLayout();
 }
 
 IntRect FrameView::ConvertFromLayoutItem(
     const LayoutItem& layout_item,
     const IntRect& layout_object_rect) const {
   // Convert from page ("absolute") to FrameView coordinates.
   LayoutRect rect = EnclosingLayoutRect(
       layout_item.LocalToAbsoluteQuad(FloatRect(layout_object_rect))
           .BoundingBox());
@@ skipping 693 matching lines @@
   if (!scrollbar_existence_changed)
     return false;
 
   scrollbar_manager_.SetHasHorizontalScrollbar(new_has_horizontal_scrollbar);
   scrollbar_manager_.SetHasVerticalScrollbar(new_has_vertical_scrollbar);
 
   if (scrollbars_suppressed_)
     return true;
 
   if (!HasOverlayScrollbars())
-    ContentsResized();
+    SetNeedsLayout();
   ScrollbarExistenceDidChange();
   return true;
 }
 
 bool FrameView::NeedsScrollbarReconstruction() const {
   Scrollbar* scrollbar = HorizontalScrollbar();
   if (!scrollbar)
     scrollbar = VerticalScrollbar();
   if (!scrollbar) {
     // We have no scrollbar to reconstruct.
@@ skipping 1057 matching lines @@
 void FrameView::SetAnimationHost(
     std::unique_ptr<CompositorAnimationHost> host) {
   animation_host_ = std::move(host);
 }
 
 LayoutUnit FrameView::CaretWidth() const {
   return LayoutUnit(GetChromeClient()->WindowToViewportScalar(1));
 }
 
 }  // namespace blink