[css-grid] Refactor intrinsic size computation code

[css-grid] Refactor intrinsic size computation code

The code which computes the intrinsic logical width of the grid container
and the rows sizes with indefinite height is exactly the same but operating
in different axis. Refactored so that we don't have to write the
initialization and tear down code twice.

Committed: https://crrev.com/8ccb563f3b1ce1435f97a65a6a396559febf7e89
Cr-Commit-Position: refs/heads/master@{#419158}

[svillar, 2016-09-15 18:33:47]

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

[jfernandez, 2016-09-15 21:47:28]

Nice refactoring. The tracks and intrinsic size computation is much clearer now.

LGTM

https://codereview.chromium.org/2347613003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2347613003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:408: void
LayoutGrid::computeTrackSizesForDefiniteSize(GridTrackSizingDirection direction,
GridSizingData& sizingData, LayoutUnit availableSpace)
Any reason why this method is not declared 'const' ?

https://codereview.chromium.org/2347613003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:618:
sizingData.freeSpace(ForRows) = LayoutUnit();
Shouldn't we use 'direction' here, instead of 'ForRows' ?

https://codereview.chromium.org/2347613003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1402: bool
LayoutGrid::tracksAreWiderThanMinTrackBreadth(GridTrackSizingDirection
direction, GridSizingData& sizingData) const
sizingData should be const here.

[eae, 2016-09-15 21:53:02]

Nice! LGTM.

[svillar, 2016-09-16 11:51:47]

The CQ bit was checked by svillar@igalia.com

[svillar, 2016-09-16 11:51:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
jfernandez@igalia.com, eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2347613003/#ps20001
(title: "Patch for landing")

[commit-bot: I haz the power, 2016-09-16 11:52: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, 2016-09-16 12:03:23]

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

[commit-bot: I haz the power, 2016-09-16 12:03:24]

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

[svillar, 2016-09-16 12:23:31]

https://codereview.chromium.org/2347613003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2347613003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:408: void
LayoutGrid::computeTrackSizesForDefiniteSize(GridTrackSizingDirection direction,
GridSizingData& sizingData, LayoutUnit availableSpace)
On 2016/09/15 21:47:28, jfernandez wrote:
> Any reason why this method is not declared 'const' ?

Good point, I think we can do it.

https://codereview.chromium.org/2347613003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:618:
sizingData.freeSpace(ForRows) = LayoutUnit();
On 2016/09/15 21:47:28, jfernandez wrote:
> Shouldn't we use 'direction' here, instead of 'ForRows' ? 

Uhu, right

https://codereview.chromium.org/2347613003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1402: bool
LayoutGrid::tracksAreWiderThanMinTrackBreadth(GridTrackSizingDirection
direction, GridSizingData& sizingData) const
On 2016/09/15 21:47:28, jfernandez wrote:
> sizingData should be const here.

It should but we cannot do it right now because freeSpace() is not const
(returns a reference). Perhaps in some other change.

[svillar, 2016-09-16 12:32:23]

The CQ bit was checked by svillar@igalia.com

[svillar, 2016-09-16 12:32:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
jfernandez@igalia.com, eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2347613003/#ps40001
(title: "Build fix")

[commit-bot: I haz the power, 2016-09-16 12:32:42]

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, 2016-09-16 13:45:35]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-16 13:47:53]

Description was changed from

==========
[css-grid] Refactor intrinsic size computation code

The code which computes the intrinsic logical width of the grid container
and the rows sizes with indefinite height is exactly the same but operating
in different axis. Refactored so that we don't have to write the
initialization and tear down code twice.
==========

to

==========
[css-grid] Refactor intrinsic size computation code

The code which computes the intrinsic logical width of the grid container
and the rows sizes with indefinite height is exactly the same but operating
in different axis. Refactored so that we don't have to write the
initialization and tear down code twice.

Committed: https://crrev.com/8ccb563f3b1ce1435f97a65a6a396559febf7e89
Cr-Commit-Position: refs/heads/master@{#419158}
==========

[commit-bot: I haz the power, 2016-09-16 13:47:54]

Patchset 3 (id:??) landed as
https://crrev.com/8ccb563f3b1ce1435f97a65a6a396559febf7e89
Cr-Commit-Position: refs/heads/master@{#419158}