[css-grid] Isolate intrinsic size computation from layout

[css-grid] Isolate intrinsic size computation from layout

This is the last patch of the items placement data refactoring. By using
a different Grid instance in computeIntrinsicLogicalWidths we effectively
isolate the intrinsic size computation from the layout. They are now using
different data structures so they don't interfere each other.

This also means that we no longer reuse the placement of items done in the
intrinsic size computation. That shouldn't be a big issue because we do save
the position of items between layouts.

Last but not least, this patch finally removes the ugly const_cast's we had
in computeIntrinsicLogicalWidths() as we no longer modify the internal state
of LayoutGrid.

BUG=627812, 666688
Committed: https://crrev.com/b34fbbccb46eae5f12faec3ea00ed3c3bfb2692b
Cr-Commit-Position: refs/heads/master@{#440755}

[svillar, 2016-12-20 08:54:33]

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

[svillar, 2016-12-20 08:54:33]

Sending out

[svillar, 2016-12-20 09:10:56]

Description was changed from

==========
[css-grid] Isolate intrinsic size computation from layout

This is the last patch of the items placement data refactoring. By using
a different Grid instance in computeIntrinsicLogicalWidths we effectively
isolate the intrinsic size computation from the layout. They are now using
different data structures so they don't interfere each other.

This also means that we no longer reuse the placement of items done in the
intrinsic size computation. That shouldn't be a big issue because we do save
the position of items between layouts.

Last but not least, this patch finally removes the ugly const_cast's we had
in computeIntrinsicLogicalWidths() as we no longer modify the internal state
of LayoutGrid.

BUG=627812
==========

to

==========
[css-grid] Isolate intrinsic size computation from layout

This is the last patch of the items placement data refactoring. By using
a different Grid instance in computeIntrinsicLogicalWidths we effectively
isolate the intrinsic size computation from the layout. They are now using
different data structures so they don't interfere each other.

This also means that we no longer reuse the placement of items done in the
intrinsic size computation. That shouldn't be a big issue because we do save
the position of items between layouts.

Last but not least, this patch finally removes the ugly const_cast's we had
in computeIntrinsicLogicalWidths() as we no longer modify the internal state
of LayoutGrid.

BUG=627812,666688
==========

[jfernandez, 2016-12-20 15:57:55]

LGTM

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

https://codereview.chromium.org/2589143002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:804: placeItemsOnGrid(grid,
IntrinsicSizeComputation);
What do you think about creating the new Grid instantiate inside
placeItemsOnGrid ? Is there any other way where the Grid instance is created ?

https://codereview.chromium.org/2589143002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2092:
grid.setAutoRepeatTracks(autoRepeatRows, autoRepeatColumns);
Since we are going to get the autoRepeatRows/Columns anyway, why not just call
the the setter method and let it decide whether to sets also the flag for
needsItemsPlacement ?

[svillar, 2016-12-20 16:21:44]

Thanks for the quick review.

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

https://codereview.chromium.org/2589143002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:804: placeItemsOnGrid(grid,
IntrinsicSizeComputation);
On 2016/12/20 15:57:55, jfernandez wrote:
> What do you think about creating the new Grid instantiate inside
> placeItemsOnGrid ? Is there any other way where the Grid instance is created ?

We cannot move it inside the method because for the case of layout we don't need
to create anything, we just use m_grid.

https://codereview.chromium.org/2589143002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2092:
grid.setAutoRepeatTracks(autoRepeatRows, autoRepeatColumns);
On 2016/12/20 15:57:55, jfernandez wrote:
> Since we are going to get the autoRepeatRows/Columns anyway, why not just call
> the the setter method and let it decide whether to sets also the flag for
> needsItemsPlacement ?

Because I don't want to add any placement behavior to the Grid class. The Grid
class is just a grid representation and the methods to manipulate it. The items'
placement algorithm should be part of LayoutGrid as it is general enough to work
against any kind of grid representation.

[svillar, 2016-12-20 16:44:26]

The CQ bit was checked by svillar@igalia.com

[commit-bot: I haz the power, 2016-12-20 16:45:01]

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-12-20 18:29:43]

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

[commit-bot: I haz the power, 2016-12-20 18:29:45]

Failed to apply patch for third_party/WebKit/LayoutTests/TestExpectations:
While running git apply --index -p1;
  error: patch failed: third_party/WebKit/LayoutTests/TestExpectations:1956
  error: third_party/WebKit/LayoutTests/TestExpectations: patch does not apply

Patch:       third_party/WebKit/LayoutTests/TestExpectations
Index: third_party/WebKit/LayoutTests/TestExpectations
diff --git a/third_party/WebKit/LayoutTests/TestExpectations
b/third_party/WebKit/LayoutTests/TestExpectations
index
d8092bb2e3e2e3c5d01a11ad2aed40121b2e2d43..ff6e81e635427290049bc9f5d8c589e914ed851e
100644
--- a/third_party/WebKit/LayoutTests/TestExpectations
+++ b/third_party/WebKit/LayoutTests/TestExpectations
@@ -1956,10 +1956,6 @@ crbug.com/673109 [ Linux Win ]
virtual/scalefactor150/fast/hidpi/static/popup-me
 crbug.com/673109 [ Linux Win Mac ]
virtual/scalefactor200/fast/hidpi/static/popup-menu-with-scrollbar-appearance.html
[ NeedsRebaseline ]
 crbug.com/673109 [ Linux Win ]
virtual/scalefactor200withzoom/fast/hidpi/static/popup-menu-with-scrollbar-appearance.html
[ NeedsRebaseline ]
 
-# Due to a temporal revert these tests are failing:
-crbug.com/666688
fast/css-grid-layout/grid-change-intrinsic-size-with-auto-repeat-tracks.html [
Failure ]
-crbug.com/666688
fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-001.html [ Failure ]
-
 # TODO(chrishall): this is a temporary mediation step as part of the P0 issue
crbug.com/657646
 # this is not meant to be here for more than a few days (from 2016-11-03 SYD)
 crbug.com/657646 [ Win ] fast/text/font-weight.html [ Failure Pass ]

[svillar, 2016-12-27 13:57:30]

The CQ bit was checked by svillar@igalia.com

[svillar, 2016-12-27 13:57:30]

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

[commit-bot: I haz the power, 2016-12-27 13:57:36]

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-12-27 15:13:01]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1482847050181890,
"parent_rev": "8319bffa3836a3149d19154b4d119d2568004cf6", "commit_rev":
"98176a81c31b3404bf5f31837e4d07548de83071"}

[commit-bot: I haz the power, 2016-12-27 15:13:30]

Description was changed from

==========
[css-grid] Isolate intrinsic size computation from layout

This is the last patch of the items placement data refactoring. By using
a different Grid instance in computeIntrinsicLogicalWidths we effectively
isolate the intrinsic size computation from the layout. They are now using
different data structures so they don't interfere each other.

This also means that we no longer reuse the placement of items done in the
intrinsic size computation. That shouldn't be a big issue because we do save
the position of items between layouts.

Last but not least, this patch finally removes the ugly const_cast's we had
in computeIntrinsicLogicalWidths() as we no longer modify the internal state
of LayoutGrid.

BUG=627812,666688
==========

to

==========
[css-grid] Isolate intrinsic size computation from layout

This is the last patch of the items placement data refactoring. By using
a different Grid instance in computeIntrinsicLogicalWidths we effectively
isolate the intrinsic size computation from the layout. They are now using
different data structures so they don't interfere each other.

This also means that we no longer reuse the placement of items done in the
intrinsic size computation. That shouldn't be a big issue because we do save
the position of items between layouts.

Last but not least, this patch finally removes the ugly const_cast's we had
in computeIntrinsicLogicalWidths() as we no longer modify the internal state
of LayoutGrid.

BUG=627812,666688

Review-Url: https://codereview.chromium.org/2589143002
==========

[commit-bot: I haz the power, 2016-12-27 15:13:31]

Committed patchset #2 (id:20001)

[huangs, 2016-12-27 16:55:29]

New failures in builders:
- Linux Tests (dbg)(1)
- Android Tests
- WebKit Android (Nexus4)
are cropping up after this CL. Might need to revert.

[huangs, 2016-12-27 19:31:37]

huangs@chromium.org changed reviewers:
+ huangs@chromium.org

[huangs, 2016-12-27 19:31:38]

The errors cleared up by themselves. No need to revert.

[commit-bot: I haz the power, 2017-01-02 15:46:13]

Description was changed from

==========
[css-grid] Isolate intrinsic size computation from layout

This is the last patch of the items placement data refactoring. By using
a different Grid instance in computeIntrinsicLogicalWidths we effectively
isolate the intrinsic size computation from the layout. They are now using
different data structures so they don't interfere each other.

This also means that we no longer reuse the placement of items done in the
intrinsic size computation. That shouldn't be a big issue because we do save
the position of items between layouts.

Last but not least, this patch finally removes the ugly const_cast's we had
in computeIntrinsicLogicalWidths() as we no longer modify the internal state
of LayoutGrid.

BUG=627812,666688

Review-Url: https://codereview.chromium.org/2589143002
==========

to

==========
[css-grid] Isolate intrinsic size computation from layout

This is the last patch of the items placement data refactoring. By using
a different Grid instance in computeIntrinsicLogicalWidths we effectively
isolate the intrinsic size computation from the layout. They are now using
different data structures so they don't interfere each other.

This also means that we no longer reuse the placement of items done in the
intrinsic size computation. That shouldn't be a big issue because we do save
the position of items between layouts.

Last but not least, this patch finally removes the ugly const_cast's we had
in computeIntrinsicLogicalWidths() as we no longer modify the internal state
of LayoutGrid.

BUG=627812,666688

Committed: https://crrev.com/b34fbbccb46eae5f12faec3ea00ed3c3bfb2692b
Cr-Commit-Position: refs/heads/master@{#440755}
==========

[commit-bot: I haz the power, 2017-01-02 15:46:14]

Patchset 2 (id:??) landed as
https://crrev.com/b34fbbccb46eae5f12faec3ea00ed3c3bfb2692b
Cr-Commit-Position: refs/heads/master@{#440755}