[css-grid] Preemptively store intrinsic sizes during layout

[css-grid] Preemptively store intrinsic sizes during layout

Computing intrinsic sizes after the layout process (as it happens in Mac
with the calls to content::RenderViewImpl::didUpdateLayout()) or when using
the grid as a flex item in some cases, is very problematic, because the
intrinsic size computation did change the override sizes of the items.

One potential solution would be to mark the grid as needs layout after the
preferred widths computation but that is forbidden by the engine (also it
would require an additional layout). There is a better way to do this based
on the fact that the preferred widths computation is actually part of the
track sizing algorithm.

We could store the intrinsic sizes of the grid returned by the track sizing
algorithm after computing the column sizes in the event of the intrinsic
sizes being dirty. Note that this does not affect the other cases where
preferred widths are required as we already execute
computeIntrinsicLogicalWidths() as part of updateLogicalWidths() if
required.

BUG=708159
Review-Url: https://codereview.chromium.org/2834473003
Cr-Commit-Position: refs/heads/master@{#466668}
Committed: https://chromium.googlesource.com/chromium/src/+/41d53a7316548ae2147de1fbb0cb9d1fda4ed3fa

[svillar, 2017-04-21 14:37:25]

svillar@igalia.com changed reviewers:
+ eae@chromium.org, jfernandez@igalia.com, rego@igalia.com

[svillar, 2017-04-21 14:37:25]

Sending out for review

[jfernandez, 2017-04-21 14:48:03]

https://codereview.chromium.org/2834473003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2834473003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:284: // algorithm always
compute them as part of its execution. Apart from saving
What do you mean with this sentence ?

https://codereview.chromium.org/2834473003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:285: // an algorithm
execution, it fixes the use case of computing the preferred
Why we save an algorithm execution with this change ?

[svillar, 2017-04-21 14:51:24]

https://codereview.chromium.org/2834473003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2834473003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:284: // algorithm always
compute them as part of its execution. Apart from saving
On 2017/04/21 14:48:02, jfernandez wrote:
> What do you mean with this sentence ?

In order to compute the intrinsic sizes we have to call the track sizing
algorithm. The algorithm always computes the intrinsic sizes because it's just
an intermediate result of its execution, is not avoidable and it does not
require any extra work.

https://codereview.chromium.org/2834473003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:285: // an algorithm
execution, it fixes the use case of computing the preferred
On 2017/04/21 14:48:02, jfernandez wrote:
> Why we save an algorithm execution with this change ?

Because we wouldn't have to call the algorithm later if
minPreferredLogicalWidth() is called because they'd be already computed. I can
rephrase it if you want.

[jfernandez, 2017-04-24 08:33:17]

lgtm

[Manuel Rego, 2017-04-24 09:08:23]

Really nice idea to solve this complex issue.
LGTM!

https://codereview.chromium.org/2834473003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/preferred-width-computed-after-layout.html
(right):

https://codereview.chromium.org/2834473003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/preferred-width-computed-after-layout.html:21:
<p>This test checks that preferred widths computations triggered after layout
for flex items do not cause grid containers to shrink. The test PASS if you see
a 10px black square inside a white box inside a gray box.</p>
Nit: As this is a testharness.js test, you'll actually see some PASS messages.
Also I guess you'll see 2 boxes, not only one.
I'd just remove this last sentence, but feel free to update it accordingly.

https://codereview.chromium.org/2834473003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp (left):

https://codereview.chromium.org/2834473003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp:440: return;
Really nice we're getting rid of this ugly code!

[svillar, 2017-04-24 16:18:30]

The CQ bit was checked by svillar@igalia.com

[svillar, 2017-04-24 16:18:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
jfernandez@igalia.com, rego@igalia.com
Link to the patchset: https://codereview.chromium.org/2834473003/#ps60001
(title: "More tests")

[commit-bot: I haz the power, 2017-04-24 16:19:04]

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-04-24 17:21:03]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493050710607020,
"parent_rev": "d818557130efc3469ada7c3e03d9f4c308da6226", "commit_rev":
"41d53a7316548ae2147de1fbb0cb9d1fda4ed3fa"}

[commit-bot: I haz the power, 2017-04-24 17:21:17]

Description was changed from

==========
[css-grid] Preemptively store intrinsic sizes during layout

Computing intrinsic sizes after the layout process (as it happens in Mac
with the calls to content::RenderViewImpl::didUpdateLayout()) or when using
the grid as a flex item in some cases, is very problematic, because the
intrinsic size computation did change the override sizes of the items.

One potential solution would be to mark the grid as needs layout after the
preferred widths computation but that is forbidden by the engine (also it
would require an additional layout). There is a better way to do this based
on the fact that the preferred widths computation is actually part of the
track sizing algorithm.

We could store the intrinsic sizes of the grid returned by the track sizing
algorithm after computing the column sizes in the event of the intrinsic
sizes being dirty. Note that this does not affect the other cases where
preferred widths are required as we already execute
computeIntrinsicLogicalWidths() as part of updateLogicalWidths() if
required.

BUG=708159
==========

to

==========
[css-grid] Preemptively store intrinsic sizes during layout

Computing intrinsic sizes after the layout process (as it happens in Mac
with the calls to content::RenderViewImpl::didUpdateLayout()) or when using
the grid as a flex item in some cases, is very problematic, because the
intrinsic size computation did change the override sizes of the items.

One potential solution would be to mark the grid as needs layout after the
preferred widths computation but that is forbidden by the engine (also it
would require an additional layout). There is a better way to do this based
on the fact that the preferred widths computation is actually part of the
track sizing algorithm.

We could store the intrinsic sizes of the grid returned by the track sizing
algorithm after computing the column sizes in the event of the intrinsic
sizes being dirty. Note that this does not affect the other cases where
preferred widths are required as we already execute
computeIntrinsicLogicalWidths() as part of updateLogicalWidths() if
required.

BUG=708159

Review-Url: https://codereview.chromium.org/2834473003
Cr-Commit-Position: refs/heads/master@{#466668}
Committed:
https://chromium.googlesource.com/chromium/src/+/41d53a7316548ae2147de1fbb0cb...
==========

[commit-bot: I haz the power, 2017-04-24 17:21:18]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/41d53a7316548ae2147de1fbb0cb...