Update WebView/FrameView size from LayoutView::UpdateAfterLayout

Update WebView/FrameView size from LayoutView::UpdateAfterLayout

Previously, this was set *after* layout had completed, via
FrameView::AdjustViewSize and WebViewImpl::LayoutUpdated.

The layout size of the full document must be known before the
WebView and FrameView can be resized, because the document size may
affect the minimum page scale factor, which will affect the
FrameView's size.  The earliest that the full document's layout size
is known is after the LayoutView completes its block layout.

This patch updates the FrameView size in
LayoutView::UpdateAfterLayout, which is after the LayoutView has
finished block layout, but still in time for the LayoutView to use
the new sizes when updating its scrollbars (when root layer
scrolling is enabled).

The new test expectations are just paint invalidation reasons, which
are expected; and one case (resize-iframe-text.html) where the existing
invalidation geometry was too big, and this patch results in a smaller,
correct set of invalidations.

BUG=701575
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2860433002
Cr-Commit-Position: refs/heads/master@{#471275}
Committed: https://chromium.googlesource.com/chromium/src/+/54a03d79877c3cacd79e37e33ec715abb3460bd7

[szager1, 2017-05-02 14:00:54]

Description was changed from

==========
Update WebView/FrameView size from LayoutView::UpdateAfterLayout

Previously, this was set *after* layout had completed, via
FrameView::AdjustViewSizeAndLayout and WebViewImpl::LayoutUpdated.

The layout size of the full document must be known before the
WebView and FrameView can be resized, because the document size may
affect the minimum page scale factor, which will affect the
FrameView's size.  The earliest that the full document's layout size
is known is after the LayoutView completes its block layout.

This patch updates the FrameView size in
LayoutView::UpdateAfterLayout, which is after the LayoutView has
finished block layout, but still in time for the LayoutView to use
the new sizes when updating its scrollbars (when root layer
scrolling is enabled).

BUG=701575
==========

to

==========
Update WebView/FrameView size from LayoutView::UpdateAfterLayout

Previously, this was set *after* layout had completed, via
FrameView::AdjustViewSizeAndLayout and WebViewImpl::LayoutUpdated.

The layout size of the full document must be known before the
WebView and FrameView can be resized, because the document size may
affect the minimum page scale factor, which will affect the
FrameView's size.  The earliest that the full document's layout size
is known is after the LayoutView completes its block layout.

This patch updates the FrameView size in
LayoutView::UpdateAfterLayout, which is after the LayoutView has
finished block layout, but still in time for the LayoutView to use
the new sizes when updating its scrollbars (when root layer
scrolling is enabled).

BUG=701575
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[szager1, 2017-05-02 14:01:20]

The CQ bit was checked by szager@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 14:01:36]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[szager1, 2017-05-02 14:18:39]

Description was changed from

==========
Update WebView/FrameView size from LayoutView::UpdateAfterLayout

Previously, this was set *after* layout had completed, via
FrameView::AdjustViewSizeAndLayout and WebViewImpl::LayoutUpdated.

The layout size of the full document must be known before the
WebView and FrameView can be resized, because the document size may
affect the minimum page scale factor, which will affect the
FrameView's size.  The earliest that the full document's layout size
is known is after the LayoutView completes its block layout.

This patch updates the FrameView size in
LayoutView::UpdateAfterLayout, which is after the LayoutView has
finished block layout, but still in time for the LayoutView to use
the new sizes when updating its scrollbars (when root layer
scrolling is enabled).

BUG=701575
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Update WebView/FrameView size from LayoutView::UpdateAfterLayout

Previously, this was set *after* layout had completed, via
FrameView::AdjustViewSizeAndLayout and WebViewImpl::LayoutUpdated.

The layout size of the full document must be known before the
WebView and FrameView can be resized, because the document size may
affect the minimum page scale factor, which will affect the
FrameView's size.  The earliest that the full document's layout size
is known is after the LayoutView completes its block layout.

This patch updates the FrameView size in
LayoutView::UpdateAfterLayout, which is after the LayoutView has
finished block layout, but still in time for the LayoutView to use
the new sizes when updating its scrollbars (when root layer
scrolling is enabled).

BUG=701575
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[commit-bot: I haz the power, 2017-05-02 15:29:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-02 15:29:54]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[szager1, 2017-05-03 09:31:11]

The CQ bit was checked by szager@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-03 09:31:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[szager1, 2017-05-03 09:33:16]

Description was changed from

==========
Update WebView/FrameView size from LayoutView::UpdateAfterLayout

Previously, this was set *after* layout had completed, via
FrameView::AdjustViewSizeAndLayout and WebViewImpl::LayoutUpdated.

The layout size of the full document must be known before the
WebView and FrameView can be resized, because the document size may
affect the minimum page scale factor, which will affect the
FrameView's size.  The earliest that the full document's layout size
is known is after the LayoutView completes its block layout.

This patch updates the FrameView size in
LayoutView::UpdateAfterLayout, which is after the LayoutView has
finished block layout, but still in time for the LayoutView to use
the new sizes when updating its scrollbars (when root layer
scrolling is enabled).

BUG=701575
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Update WebView/FrameView size from LayoutView::UpdateAfterLayout

Previously, this was set *after* layout had completed, via
FrameView::AdjustViewSize and WebViewImpl::LayoutUpdated.

The layout size of the full document must be known before the
WebView and FrameView can be resized, because the document size may
affect the minimum page scale factor, which will affect the
FrameView's size.  The earliest that the full document's layout size
is known is after the LayoutView completes its block layout.

This patch updates the FrameView size in
LayoutView::UpdateAfterLayout, which is after the LayoutView has
finished block layout, but still in time for the LayoutView to use
the new sizes when updating its scrollbars (when root layer
scrolling is enabled).

BUG=701575
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[commit-bot: I haz the power, 2017-05-03 10:56:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-03 10:56:43]

Dry run: Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[szager1, 2017-05-03 15:32:33]

The CQ bit was checked by szager@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-03 15:32:56]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 16:36:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-03 16:36:41]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[szager1, 2017-05-04 14:55:02]

The CQ bit was checked by szager@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 14:55:11]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-04 16:44:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-04 16:44:50]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[szager1, 2017-05-05 12:28:00]

The CQ bit was checked by szager@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 12:28:10]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[szager1, 2017-05-05 14:06:18]

Description was changed from

==========
Update WebView/FrameView size from LayoutView::UpdateAfterLayout

Previously, this was set *after* layout had completed, via
FrameView::AdjustViewSize and WebViewImpl::LayoutUpdated.

The layout size of the full document must be known before the
WebView and FrameView can be resized, because the document size may
affect the minimum page scale factor, which will affect the
FrameView's size.  The earliest that the full document's layout size
is known is after the LayoutView completes its block layout.

This patch updates the FrameView size in
LayoutView::UpdateAfterLayout, which is after the LayoutView has
finished block layout, but still in time for the LayoutView to use
the new sizes when updating its scrollbars (when root layer
scrolling is enabled).

BUG=701575
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Update WebView/FrameView size from LayoutView::UpdateAfterLayout

Previously, this was set *after* layout had completed, via
FrameView::AdjustViewSize and WebViewImpl::LayoutUpdated.

The layout size of the full document must be known before the
WebView and FrameView can be resized, because the document size may
affect the minimum page scale factor, which will affect the
FrameView's size.  The earliest that the full document's layout size
is known is after the LayoutView completes its block layout.

This patch updates the FrameView size in
LayoutView::UpdateAfterLayout, which is after the LayoutView has
finished block layout, but still in time for the LayoutView to use
the new sizes when updating its scrollbars (when root layer
scrolling is enabled).

The new test expectations are just paint invalidation reasons, which are
expected; and one case where the existing invalidation geometry was too big, and
this patch results in a smaller, correct set of invalidations.

BUG=701575
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[szager1, 2017-05-05 14:07:19]

Description was changed from

==========
Update WebView/FrameView size from LayoutView::UpdateAfterLayout

Previously, this was set *after* layout had completed, via
FrameView::AdjustViewSize and WebViewImpl::LayoutUpdated.

The layout size of the full document must be known before the
WebView and FrameView can be resized, because the document size may
affect the minimum page scale factor, which will affect the
FrameView's size.  The earliest that the full document's layout size
is known is after the LayoutView completes its block layout.

This patch updates the FrameView size in
LayoutView::UpdateAfterLayout, which is after the LayoutView has
finished block layout, but still in time for the LayoutView to use
the new sizes when updating its scrollbars (when root layer
scrolling is enabled).

The new test expectations are just paint invalidation reasons, which are
expected; and one case where the existing invalidation geometry was too big, and
this patch results in a smaller, correct set of invalidations.

BUG=701575
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Update WebView/FrameView size from LayoutView::UpdateAfterLayout

Previously, this was set *after* layout had completed, via
FrameView::AdjustViewSize and WebViewImpl::LayoutUpdated.

The layout size of the full document must be known before the
WebView and FrameView can be resized, because the document size may
affect the minimum page scale factor, which will affect the
FrameView's size.  The earliest that the full document's layout size
is known is after the LayoutView completes its block layout.

This patch updates the FrameView size in
LayoutView::UpdateAfterLayout, which is after the LayoutView has
finished block layout, but still in time for the LayoutView to use
the new sizes when updating its scrollbars (when root layer
scrolling is enabled).

The new test expectations are just paint invalidation reasons, which are
expected; and one case (resize-iframe-text.html) where the existing invalidation
geometry was too big, and this patch results in a smaller, correct set of
invalidations.

BUG=701575
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[commit-bot: I haz the power, 2017-05-05 14:07:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-05 14:07:28]

Dry run: This issue passed the CQ dry run.

[szager1, 2017-05-05 14:07:40]

Description was changed from

==========
Update WebView/FrameView size from LayoutView::UpdateAfterLayout

Previously, this was set *after* layout had completed, via
FrameView::AdjustViewSize and WebViewImpl::LayoutUpdated.

The layout size of the full document must be known before the
WebView and FrameView can be resized, because the document size may
affect the minimum page scale factor, which will affect the
FrameView's size.  The earliest that the full document's layout size
is known is after the LayoutView completes its block layout.

This patch updates the FrameView size in
LayoutView::UpdateAfterLayout, which is after the LayoutView has
finished block layout, but still in time for the LayoutView to use
the new sizes when updating its scrollbars (when root layer
scrolling is enabled).

The new test expectations are just paint invalidation reasons, which are
expected; and one case (resize-iframe-text.html) where the existing invalidation
geometry was too big, and this patch results in a smaller, correct set of
invalidations.

BUG=701575
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Update WebView/FrameView size from LayoutView::UpdateAfterLayout

Previously, this was set *after* layout had completed, via
FrameView::AdjustViewSize and WebViewImpl::LayoutUpdated.

The layout size of the full document must be known before the
WebView and FrameView can be resized, because the document size may
affect the minimum page scale factor, which will affect the
FrameView's size.  The earliest that the full document's layout size
is known is after the LayoutView completes its block layout.

This patch updates the FrameView size in
LayoutView::UpdateAfterLayout, which is after the LayoutView has
finished block layout, but still in time for the LayoutView to use
the new sizes when updating its scrollbars (when root layer
scrolling is enabled).

The new test expectations are just paint invalidation reasons, which
are expected; and one case (resize-iframe-text.html) where the existing
invalidation geometry was too big, and this patch results in a smaller,
correct set of invalidations.

BUG=701575
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[szager1, 2017-05-05 14:08:17]

szager@chromium.org changed reviewers:
+ cbiesinger@chromium.org, wangxianzhu@chromium.org

[szager1, 2017-05-05 14:09:38]

cbiesinger: Sorry for this, but skobes and bokan are both OOO, so I'm sending it
to you.

+wangxianzhu for the invalidation test baselines.

[szager1, 2017-05-05 14:12:12]

szager@chromium.org changed reviewers:
+ chrishtr@chromium.org

[szager1, 2017-05-05 14:12:53]

+chrishtr, who has some background on this from a meeting about a month ago.

[cbiesinger, 2017-05-05 18:20:01]

I think this mostly makes sense but I'm not sure I fully understand it. See
questions below.

https://codereview.chromium.org/2860433002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2860433002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:889:
SetNeedsPaintPropertyUpdate();
Why is this now newly needed?

https://codereview.chromium.org/2860433002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutView.cpp (right):

https://codereview.chromium.org/2860433002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutView.cpp:718:
LayoutBlock::UpdateAfterLayout();
Shouldn't this be LayoutBlockFlow::?

https://codereview.chromium.org/2860433002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/FramePainter.cpp (right):

https://codereview.chromium.org/2860433002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FramePainter.cpp:219: if
(corner_rect.IsEmpty())
This seems fine but why is that necessary as part of this change?

https://codereview.chromium.org/2860433002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2860433002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:3681: if (!client_ ||
!frame->IsLocalRoot())
I don't understand this change?

You're still calling UpdateLayout, so why don't you need the CanUpdateLayout
condition anymore?

I assume you replaced IsMainFrame with IsLocalRoot because local frames get
updated in UpdateAfterLayout now but remote ones don't?

[szager1, 2017-05-08 16:27:01]

The CQ bit was checked by szager@chromium.org to run a CQ dry run

[szager1, 2017-05-08 16:27:06]

https://codereview.chromium.org/2860433002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2860433002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:889:
SetNeedsPaintPropertyUpdate();
On 2017/05/05 18:20:01, cbiesinger wrote:
> Why is this now newly needed?

Here's the test failure if this line is omitted:

https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel...

https://codereview.chromium.org/2860433002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutView.cpp (right):

https://codereview.chromium.org/2860433002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutView.cpp:718:
LayoutBlock::UpdateAfterLayout();
On 2017/05/05 18:20:01, cbiesinger wrote:
> Shouldn't this be LayoutBlockFlow::?

LayoutBlockFlow doesn't have an override for UpdateAfterLayout, but it's
probably better to make the change you suggest, so I did.

https://codereview.chromium.org/2860433002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/FramePainter.cpp (right):

https://codereview.chromium.org/2860433002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FramePainter.cpp:219: if
(corner_rect.IsEmpty())
On 2017/05/05 18:20:01, cbiesinger wrote:
> This seems fine but why is that necessary as part of this change?

This is a mistake, I had another CL to fix this issue; reverted this diff.

https://codereview.chromium.org/2860433002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2860433002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:3681: if (!client_ ||
!frame->IsLocalRoot())
On 2017/05/05 18:20:01, cbiesinger wrote:
> I don't understand this change?
> 
> You're still calling UpdateLayout, so why don't you need the CanUpdateLayout
> condition anymore?
> 
> I assume you replaced IsMainFrame with IsLocalRoot because local frames get
> updated in UpdateAfterLayout now but remote ones don't?

I asked bokan and dcheng about this: IsMainFrame() is not exactly right.  This
code should run for any frame that might have a VisualViewport, and any local
root frame can have a VisualViewport.

The CanUpdateLayout call is obsolete; all overrides to that method that could
conceivably get called here are implemented as 'return true;'

[commit-bot: I haz the power, 2017-05-08 16:27:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-08 16:54:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-08 16:54:47]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[szager1, 2017-05-10 15:07:02]

ping

[szager1, 2017-05-10 15:22:43]

szager@chromium.org changed reviewers:
+ bokan@chromium.org

[szager1, 2017-05-10 15:22:43]

+bokan, you're probably best positioned to review this with skobes out.

[bokan, 2017-05-10 21:30:08]

Thanks, I think this is making things more sane. Haven't looked over the test
yet but I've got some questions and comments first.

https://codereview.chromium.org/2860433002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (left):

https://codereview.chromium.org/2860433002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:561: // discover the deeper
cause of this. http://crbug.com/607987.
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?

https://codereview.chromium.org/2860433002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:1683:
lvi.SetShouldDoFullPaintInvalidation();
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?

https://codereview.chromium.org/2860433002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2860433002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:557: needs_scrollbars_update_
|= width_changed || height_changed;
This change is subtle, is this maybe what fixes the old TODO?

https://codereview.chromium.org/2860433002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:566:
lv->GetScrollableArea()->ClampScrollOffsetAfterOverflowChange();
Why do we need to special case the main frame here? Won't this get called from
LayoutView::UpdateAfterLayout()?

https://codereview.chromium.org/2860433002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:750: if (ScrollOrigin() !=
origin) {
Nit: remove braces

https://codereview.chromium.org/2860433002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:1323:
MarkViewportConstrainedObjectsForLayout(
Will ViewportConstrainedObjects still get laid out if the frame rect doesn't
change but scrollbar existence does?

https://codereview.chromium.org/2860433002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2860433002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:3649: void
WebViewImpl::ResizeFrameView(WebLocalFrameImpl* webframe) {
It seems UpdateICBAndResizeViewport is the only caller, just inline this at the
callsite and remove the method.

https://codereview.chromium.org/2860433002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:3681: if (!client_ ||
!frame->IsLocalRoot())
Can you remind me and point me to the place we had this discussion? A local root
(I think) will have a VisualViewport but only nominally (it's effectively non
operational) and I think this logic doesn't apply in a subframe.

https://codereview.chromium.org/2860433002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:3687: view->UpdateLayout();
I think this was only needed because of the ResizeAfterLayout call you removed
(we shouldn't need layout right after completing layout, right?). If that's
true, could you try removing it?

[szager1, 2017-05-11 12:50:55]

https://codereview.chromium.org/2860433002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (left):

https://codereview.chromium.org/2860433002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:561: // discover the deeper
cause of this. http://crbug.com/607987.
On 2017/05/10 21:30:07, bokan wrote:
> 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?

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

https://codereview.chromium.org/2860433002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:1683:
lvi.SetShouldDoFullPaintInvalidation();
On 2017/05/10 21:30:07, bokan wrote:
> 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?

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

https://codereview.chromium.org/2860433002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2860433002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:557: needs_scrollbars_update_
|= width_changed || height_changed;
On 2017/05/10 21:30:07, bokan wrote:
> This change is subtle, is this maybe what fixes the old TODO?

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.

https://codereview.chromium.org/2860433002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:566:
lv->GetScrollableArea()->ClampScrollOffsetAfterOverflowChange();
On 2017/05/10 21:30:07, bokan wrote:
> Why do we need to special case the main frame here? Won't this get called from
> LayoutView::UpdateAfterLayout()?

Without this, I get a unit test failure

VisualViewportTest.TestResizeAtFullyScrolledPreservesViewportLocation

https://codereview.chromium.org/2860433002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:750: if (ScrollOrigin() !=
origin) {
On 2017/05/10 21:30:07, bokan wrote:
> Nit: remove braces

Done.

https://codereview.chromium.org/2860433002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:1323:
MarkViewportConstrainedObjectsForLayout(
On 2017/05/10 21:30:07, bokan wrote:
> Will ViewportConstrainedObjects still get laid out if the frame rect doesn't
> change but scrollbar existence does?

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.

https://codereview.chromium.org/2860433002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2860433002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:3649: void
WebViewImpl::ResizeFrameView(WebLocalFrameImpl* webframe) {
On 2017/05/10 21:30:07, bokan wrote:
> It seems UpdateICBAndResizeViewport is the only caller, just inline this at
the
> callsite and remove the method.

It's also called from ResizeAfterLayout

https://codereview.chromium.org/2860433002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:3681: if (!client_ ||
!frame->IsLocalRoot())
On 2017/05/10 21:30:07, bokan wrote:
> Can you remind me and point me to the place we had this discussion? A local
root
> (I think) will have a VisualViewport but only nominally (it's effectively non
> operational) and I think this logic doesn't apply in a subframe.

I think it was over IM, I don't have any record of it.  If you think this is
wrong, I can remove it.

https://codereview.chromium.org/2860433002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:3687: view->UpdateLayout();
On 2017/05/10 21:30:07, bokan wrote:
> I think this was only needed because of the ResizeAfterLayout call you removed
> (we shouldn't need layout right after completing layout, right?). If that's
> true, could you try removing it?

Good catch, this is indeed OK to remove (and I did).

[bokan, 2017-05-11 16:51:09]

https://codereview.chromium.org/2860433002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (left):

https://codereview.chromium.org/2860433002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:1683:
lvi.SetShouldDoFullPaintInvalidation();
On 2017/05/11 12:50:54, szager1 wrote:
> On 2017/05/10 21:30:07, bokan wrote:
> > 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?
> 
> The background paint invalidation issue was fixed separately (see the bug), I
> believe this code was already obsolete.

Acknowledged.

https://codereview.chromium.org/2860433002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2860433002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:557: needs_scrollbars_update_
|= width_changed || height_changed;
On 2017/05/11 12:50:54, szager1 wrote:
> On 2017/05/10 21:30:07, bokan wrote:
> > This change is subtle, is this maybe what fixes the old TODO?
> 
> 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.

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

https://codereview.chromium.org/2860433002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:566:
lv->GetScrollableArea()->ClampScrollOffsetAfterOverflowChange();
On 2017/05/11 12:50:54, szager1 wrote:
> On 2017/05/10 21:30:07, bokan wrote:
> > Why do we need to special case the main frame here? Won't this get called
from
> > LayoutView::UpdateAfterLayout()?
> 
> Without this, I get a unit test failure
> 
> VisualViewportTest.TestResizeAtFullyScrolledPreservesViewportLocation

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?

https://codereview.chromium.org/2860433002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:1323:
MarkViewportConstrainedObjectsForLayout(
On 2017/05/11 12:50:54, szager1 wrote:
> On 2017/05/10 21:30:07, bokan wrote:
> > Will ViewportConstrainedObjects still get laid out if the frame rect doesn't
> > change but scrollbar existence does?
> 
> 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.

Got it, thanks.

https://codereview.chromium.org/2860433002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2860433002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:3649: void
WebViewImpl::ResizeFrameView(WebLocalFrameImpl* webframe) {
On 2017/05/11 12:50:54, szager1 wrote:
> On 2017/05/10 21:30:07, bokan wrote:
> > It seems UpdateICBAndResizeViewport is the only caller, just inline this at
> the
> > callsite and remove the method.
> 
> It's also called from ResizeAfterLayout

Ah, right, failed at codesearch...

Regardless, we're calling it for the main frame in both cases (assuming you
change IsLocalRoot back to IsMainFrame) so I think you can just inline the call
in both places and remove this.

https://codereview.chromium.org/2860433002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:3681: if (!client_ ||
!frame->IsLocalRoot())
On 2017/05/11 12:50:55, szager1 wrote:
> On 2017/05/10 21:30:07, bokan wrote:
> > Can you remind me and point me to the place we had this discussion? A local
> root
> > (I think) will have a VisualViewport but only nominally (it's effectively
non
> > operational) and I think this logic doesn't apply in a subframe.
> 
> I think it was over IM, I don't have any record of it.  If you think this is
> wrong, I can remove it.

Yah, I don't remember the context of that conversation but I think in this case
we should be using IsMainFrame

https://codereview.chromium.org/2860433002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/paint/invalidation/window-resize-frameset-expected.txt
(left):

https://codereview.chromium.org/2860433002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/paint/invalidation/window-resize-frameset-expected.txt:44:
"object": "VerticalScrollbar",
Why is it ok that we no longer invalidate scrollbars after the resize?

[szager1, 2017-05-11 20:11:56]

The CQ bit was checked by szager@chromium.org to run a CQ dry run

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

https://codereview.chromium.org/2860433002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2860433002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:566:
lv->GetScrollableArea()->ClampScrollOffsetAfterOverflowChange();
On 2017/05/11 16:51:08, bokan wrote:
> On 2017/05/11 12:50:54, szager1 wrote:
> > On 2017/05/10 21:30:07, bokan wrote:
> > > Why do we need to special case the main frame here? Won't this get called
> from
> > > LayoutView::UpdateAfterLayout()?
> > 
> > Without this, I get a unit test failure
> > 
> > VisualViewportTest.TestResizeAtFullyScrolledPreservesViewportLocation
> 
> 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?

Done.

https://codereview.chromium.org/2860433002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2860433002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:3649: void
WebViewImpl::ResizeFrameView(WebLocalFrameImpl* webframe) {
On 2017/05/11 16:51:08, bokan wrote:
> On 2017/05/11 12:50:54, szager1 wrote:
> > On 2017/05/10 21:30:07, bokan wrote:
> > > It seems UpdateICBAndResizeViewport is the only caller, just inline this
at
> > the
> > > callsite and remove the method.
> > 
> > It's also called from ResizeAfterLayout
> 
> Ah, right, failed at codesearch...
> 
> Regardless, we're calling it for the main frame in both cases (assuming you
> change IsLocalRoot back to IsMainFrame) so I think you can just inline the
call
> in both places and remove this.

Done.

[commit-bot: I haz the power, 2017-05-11 20:12:42]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bokan, 2017-05-11 22:15:06]

thanks, lgtm once you change IsLocalRoot back to IsMainFrame. But I'd like
someone from paint to verify that the changes to invalidation expectations
(particularly the scrollbar one) are expected since I don't know that area well.

[commit-bot: I haz the power, 2017-05-11 23:09:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-11 23:09:29]

Dry run: This issue passed the CQ dry run.

[szager1, 2017-05-12 06:15:13]

From previous conversations with wangxianzhu, the invalidation reason diffs are
fine, as long as the invalidation rects match.  He also looked at the one test
that has an invalidation rect diff, and concluded that the new result is
correct.

https://codereview.chromium.org/2860433002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/paint/invalidation/window-resize-frameset-expected.txt
(left):

https://codereview.chromium.org/2860433002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/paint/invalidation/window-resize-frameset-expected.txt:44:
"object": "VerticalScrollbar",
On 2017/05/11 16:51:09, bokan wrote:
> Why is it ok that we no longer invalidate scrollbars after the resize?

We don't invalidate the scrollbar because the scrollbar doesn't exist at the
time invalidation runs.  Previously, FrameView would do multiple passes of
creating, then destroying, scrollbars.  This patch eliminates some of the extra
passes, so that by the time invalidation runs, the scrollbars are in their final
state (i.e., they don't exist).

The important thing is that the invalidation rects match up, and they do.

[szager1, 2017-05-12 06:18:19]

The CQ bit was checked by szager@chromium.org

[szager1, 2017-05-12 06:18:19]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org
Link to the patchset: https://codereview.chromium.org/2860433002/#ps200001
(title: "s/IsLocalRoot/IsMainFrame/")

[commit-bot: I haz the power, 2017-05-12 06:18:38]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-12 07:27:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-12 07:27:11]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[szager1, 2017-05-12 07:45:49]

The CQ bit was checked by szager@chromium.org

[commit-bot: I haz the power, 2017-05-12 07:46:11]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-12 08:47:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-12 08:47:16]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[szager1, 2017-05-12 09:34:40]

The CQ bit was checked by szager@chromium.org

[commit-bot: I haz the power, 2017-05-12 09:34:57]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-12 11:09:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-12 11:09:54]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[szager1, 2017-05-12 11:21:43]

The CQ bit was checked by szager@chromium.org

[commit-bot: I haz the power, 2017-05-12 11:21:54]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-12 11:45:03]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1494588103277800,
"parent_rev": "bb9147ed072a94397e8d448ba7445c8f214230f3", "commit_rev":
"54a03d79877c3cacd79e37e33ec715abb3460bd7"}

[commit-bot: I haz the power, 2017-05-12 11:45:16]

Description was changed from

==========
Update WebView/FrameView size from LayoutView::UpdateAfterLayout

Previously, this was set *after* layout had completed, via
FrameView::AdjustViewSize and WebViewImpl::LayoutUpdated.

The layout size of the full document must be known before the
WebView and FrameView can be resized, because the document size may
affect the minimum page scale factor, which will affect the
FrameView's size.  The earliest that the full document's layout size
is known is after the LayoutView completes its block layout.

This patch updates the FrameView size in
LayoutView::UpdateAfterLayout, which is after the LayoutView has
finished block layout, but still in time for the LayoutView to use
the new sizes when updating its scrollbars (when root layer
scrolling is enabled).

The new test expectations are just paint invalidation reasons, which
are expected; and one case (resize-iframe-text.html) where the existing
invalidation geometry was too big, and this patch results in a smaller,
correct set of invalidations.

BUG=701575
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Update WebView/FrameView size from LayoutView::UpdateAfterLayout

Previously, this was set *after* layout had completed, via
FrameView::AdjustViewSize and WebViewImpl::LayoutUpdated.

The layout size of the full document must be known before the
WebView and FrameView can be resized, because the document size may
affect the minimum page scale factor, which will affect the
FrameView's size.  The earliest that the full document's layout size
is known is after the LayoutView completes its block layout.

This patch updates the FrameView size in
LayoutView::UpdateAfterLayout, which is after the LayoutView has
finished block layout, but still in time for the LayoutView to use
the new sizes when updating its scrollbars (when root layer
scrolling is enabled).

The new test expectations are just paint invalidation reasons, which
are expected; and one case (resize-iframe-text.html) where the existing
invalidation geometry was too big, and this patch results in a smaller,
correct set of invalidations.

BUG=701575
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2860433002
Cr-Commit-Position: refs/heads/master@{#471275}
Committed:
https://chromium.googlesource.com/chromium/src/+/54a03d79877c3cacd79e37e33ec7...
==========

[commit-bot: I haz the power, 2017-05-12 11:45:17]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/54a03d79877c3cacd79e37e33ec7...