Update layer size from LayoutBox::UpdateAfterLayout

Update layer size from LayoutBox::UpdateAfterLayout

Previously, layer size was set during a post-layout call to
PaintLayer::UpdateLayerPositions.  With this change,
layer size is updated during layout, as soon as the new size is
available (after its owning LayoutBox has finished layout).

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

Review-Url: https://codereview.chromium.org/2854213004
Cr-Commit-Position: refs/heads/master@{#472387}
Committed: https://chromium.googlesource.com/chromium/src/+/2088149da693bad956c1b2f2fc4e58fa0eedbab8

[szager1, 2017-05-03 11:15:50]

Description was changed from

==========
Update layer size from LayoutBox::UpdateAfterLayout

Previously, layer size was set during a post-layout call to
PaintLayer::UpdateLayerPositions.  With this change,
layer size is updated during layout, as soon as the new size is
available (after its owning LayoutBox has finished layout).

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

to

==========
Update layer size from LayoutBox::UpdateAfterLayout

Previously, layer size was set during a post-layout call to
PaintLayer::UpdateLayerPositions.  With this change,
layer size is updated during layout, as soon as the new size is
available (after its owning LayoutBox has finished layout).

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

[szager1, 2017-05-03 11:15:55]

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

[commit-bot: I haz the power, 2017-05-03 11:16:19]

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 12:33:16]

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

[commit-bot: I haz the power, 2017-05-03 12:33:16]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[szager1, 2017-05-05 13:23:09]

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

[commit-bot: I haz the power, 2017-05-05 13:23:18]

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-05 14:24:35]

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

[commit-bot: I haz the power, 2017-05-05 14:24:35]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

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

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

[commit-bot: I haz the power, 2017-05-08 16:39:14]

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:59:02]

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

[commit-bot: I haz the power, 2017-05-08 16:59:02]

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-11 16:30:33]

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

[szager1, 2017-05-11 16:30:57]

Description was changed from

==========
Update layer size from LayoutBox::UpdateAfterLayout

Previously, layer size was set during a post-layout call to
PaintLayer::UpdateLayerPositions.  With this change,
layer size is updated during layout, as soon as the new size is
available (after its owning LayoutBox has finished layout).

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

to

==========
Update layer size from LayoutBox::UpdateAfterLayout

Previously, layer size was set during a post-layout call to
PaintLayer::UpdateLayerPositions.  With this change,
layer size is updated during layout, as soon as the new size is
available (after its owning LayoutBox has finished layout).

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

[szager1, 2017-05-11 16:30:57]

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

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

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-11 16:33:04]

This CL is the coup de grâce.  It clears up all kinds of issues for root layer
scrolling.

[commit-bot: I haz the power, 2017-05-11 18:38:28]

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

[commit-bot: I haz the power, 2017-05-11 18:38:29]

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-12 13:21:50]

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

[commit-bot: I haz the power, 2017-05-12 13:22:15]

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-12 15:14:59]

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

[commit-bot: I haz the power, 2017-05-12 15:15:00]

Dry run: This issue passed the CQ dry run.

[bokan, 2017-05-12 15:58:18]

https://codereview.chromium.org/2854213004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/2854213004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:664:
child.SetLocationAndUpdateOverflowControlsIfNeeded(new_location);
Should we not do this for the SetLogicalTopForChild method below too?

https://codereview.chromium.org/2854213004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/2854213004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:334: // FIXME: We
should call a specialized versions of this function.
The FIXME should be moved down since it refers to
UpdateLayerPositionsAfterLayout (do you know what it means? what specialised
versions?)

https://codereview.chromium.org/2854213004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/2854213004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:792: //
LayoutBox::UpdateAfterLayout, but LayoutInlines will still need to update
This may be a naive question as I'm a layout-outsider, but what makes inlines
special here? Why can't they call UpdateSizeAndScrollingAfterLayout after they
layout/get their size in the same way a LayoutBox does? I noticed that
LayoutInline::UpdateLayout is a NOTREACHED so I'm guessing it's special somehow;
how does an inline get its size?

https://codereview.chromium.org/2854213004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:877:
scrollable_area_->VisibleSizeChanged();
Any reason not to put this in UpdateSize().

[szager1, 2017-05-12 19:12:13]

https://codereview.chromium.org/2854213004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/2854213004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:664:
child.SetLocationAndUpdateOverflowControlsIfNeeded(new_location);
On 2017/05/12 15:58:17, bokan wrote:
> Should we not do this for the SetLogicalTopForChild method below too?

No, because SetLogicalTopForChild is called *before* the child runs layout (as
opposed to this method, which is called *after* the child runs layout).

https://codereview.chromium.org/2854213004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/2854213004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:334: // FIXME: We
should call a specialized versions of this function.
On 2017/05/12 15:58:18, bokan wrote:
> The FIXME should be moved down since it refers to
> UpdateLayerPositionsAfterLayout (do you know what it means? what specialised
> versions?)

The comment was added here:

https://codereview.chromium.org/443773002/

I'm not sure whether it refers to the fact that in this case, it's not being
called after layout.  Or maybe it's because UpdateLayerPositionsAfterLayout is
recursive, but there's no need to recurse here (because a brand-new layer won't
have any child layers).  I'm not really sure.

https://codereview.chromium.org/2854213004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/2854213004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:792: //
LayoutBox::UpdateAfterLayout, but LayoutInlines will still need to update
On 2017/05/12 15:58:18, bokan wrote:
> This may be a naive question as I'm a layout-outsider, but what makes inlines
> special here? Why can't they call UpdateSizeAndScrollingAfterLayout after they
> layout/get their size in the same way a LayoutBox does? I noticed that
> LayoutInline::UpdateLayout is a NOTREACHED so I'm guessing it's special
somehow;
> how does an inline get its size?

LayoutInline's don't run their own layout; instead, their parent LayoutBlockFlow
creates a line box tree (instances of InlineFlowBox) and lays them out. 
LayoutInline gets its geometry from the geometry of its constituent
InlineFlowBox'es.

LayoutBlockFlow iterates over the line box tree when running layout, so there
isn't currently an iteration over LayoutInline's where we can call
UpdateSizeAndScrollingAfterLayout.  I could add an iteration to do this in
LayoutBlockFlow::LayoutInlineChildren, after the call to LayoutRunsAndFloats,
but I don't really think it's worth the performance cost.

https://codereview.chromium.org/2854213004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:877:
scrollable_area_->VisibleSizeChanged();
On 2017/05/12 15:58:18, bokan wrote:
> Any reason not to put this in UpdateSize().

UpdateSize is also called from LayoutBoxModelObject::StyleDidChange, but it's
not safe to call into scrollable_area_ at that point.

[szager1, 2017-05-16 18:59:20]

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

[szager1, 2017-05-16 18:59:40]

+skobes

[skobes, 2017-05-16 22:22:22]

https://codereview.chromium.org/2854213004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/2854213004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:334: // FIXME: We
should call a specialized versions of this function.
On 2017/05/12 19:12:12, szager1 wrote:
> On 2017/05/12 15:58:18, bokan wrote:
> > The FIXME should be moved down since it refers to
> > UpdateLayerPositionsAfterLayout (do you know what it means? what specialised
> > versions?)
> 
> The comment was added here:
> 
> https://codereview.chromium.org/443773002/
> 
> I'm not sure whether it refers to the fact that in this case, it's not being
> called after layout.  Or maybe it's because UpdateLayerPositionsAfterLayout is
> recursive, but there's no need to recurse here (because a brand-new layer
won't
> have any child layers).  I'm not really sure.

I'm guessing the former (not after layout).

https://codereview.chromium.org/2854213004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/2854213004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:863: size_ =
line_box.Size();
Why is did_resize not set in this branch?

[szager, 2017-05-16 23:55:45]

szager@google.com changed reviewers:
+ szager@google.com

[szager, 2017-05-16 23:55:46]

https://codereview.chromium.org/2854213004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/2854213004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:863: size_ =
line_box.Size();
On 2017/05/16 22:22:22, skobes wrote:
> Why is did_resize not set in this branch?

This preserves the existing logic.  did_resize is only used to decide whether to
call PLSA->VisibleSizeChanged().  A LayoutInline can't be scrollable, so size
changes to LayoutInline's are not interesting.

Having said that: now that UpdateSize is a separate, public method, it makes no
sense to preserve that logic, and it won't affect anything in the existing code,
so I changed it so that did_resize will be set for inlines that change size.

[skobes, 2017-05-17 00:01:02]

lgtm

[szager, 2017-05-17 00:06:01]

The CQ bit was checked by szager@google.com

[commit-bot: I haz the power, 2017-05-17 00:07:55]

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-17 02:13:13]

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

[commit-bot: I haz the power, 2017-05-17 02:13:14]

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-17 02:35:01]

The CQ bit was checked by szager@chromium.org

[commit-bot: I haz the power, 2017-05-17 02:36:50]

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-17 04:06:12]

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

[commit-bot: I haz the power, 2017-05-17 04:06:13]

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-17 07:55:47]

The CQ bit was checked by szager@chromium.org

[commit-bot: I haz the power, 2017-05-17 07:56:31]

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-17 08:39:39]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1495007747352620,
"parent_rev": "b71ba396c9975c119cee6632c8e78695c0771e69", "commit_rev":
"2088149da693bad956c1b2f2fc4e58fa0eedbab8"}

[commit-bot: I haz the power, 2017-05-17 08:39:50]

Description was changed from

==========
Update layer size from LayoutBox::UpdateAfterLayout

Previously, layer size was set during a post-layout call to
PaintLayer::UpdateLayerPositions.  With this change,
layer size is updated during layout, as soon as the new size is
available (after its owning LayoutBox has finished layout).

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

to

==========
Update layer size from LayoutBox::UpdateAfterLayout

Previously, layer size was set during a post-layout call to
PaintLayer::UpdateLayerPositions.  With this change,
layer size is updated during layout, as soon as the new size is
available (after its owning LayoutBox has finished layout).

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

Review-Url: https://codereview.chromium.org/2854213004
Cr-Commit-Position: refs/heads/master@{#472387}
Committed:
https://chromium.googlesource.com/chromium/src/+/2088149da693bad956c1b2f2fc4e...
==========

[commit-bot: I haz the power, 2017-05-17 08:39:52]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/2088149da693bad956c1b2f2fc4e...