New issue
Advanced search Search tips

Issue 627812 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 79180
issue 662016
issue 666688



Sign in to add a comment

[css-grid] Intrinsic width computation should not alter the internal state of LayoutGrid

Project Member Reported by svil...@igalia.com, Jul 13 2016

Issue description

The track sizing algorithm is used to compute the LayoutGrid intrinsic width. This means that we have to modify some attributes of the layout object. That's the reason why we need to use some ugly const_cast inside that method.

The idea is to make the track sizing algorithm work against some object (SizingData perhaps?). That would allow us not to modify the internal state of the layout object while still reusing all the track sizing logic to compute intrinsic widths.
 

Comment 1 by svil...@igalia.com, Jul 13 2016

Status: Assigned (was: Untriaged)

Comment 2 by r...@igalia.com, Jul 13 2016

Blocking: 79180
Components: Blink>Layout>Grid
Owner: svil...@igalia.com
Project Member

Comment 3 by bugdroid1@chromium.org, Jul 14 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ae5148ba4a2c164b809d59c4bca390e77b5124c5

commit ae5148ba4a2c164b809d59c4bca390e77b5124c5
Author: svillar <svillar@igalia.com>
Date: Thu Jul 14 06:33:03 2016

[css-grid] Constify track sizing algorithm

All the methods used to run the track sizing algorithm should not modify the
state of LayoutGrid. We can safely const-ify all of them and remove the ugly
const_cast in computeIntrinsicLogicalWidths().

No new tests needed as there is no change in behavior.

BUG= 627812 

Review-Url: https://codereview.chromium.org/2150533003
Cr-Commit-Position: refs/heads/master@{#405441}

[modify] https://crrev.com/ae5148ba4a2c164b809d59c4bca390e77b5124c5/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
[modify] https://crrev.com/ae5148ba4a2c164b809d59c4bca390e77b5124c5/third_party/WebKit/Source/core/layout/LayoutGrid.h

Comment 4 by svil...@igalia.com, Nov 14 2016

Blocking: 662016

Comment 5 by r...@igalia.com, Nov 18 2016

Blocking: 666688
Project Member

Comment 6 by bugdroid1@chromium.org, Nov 18 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b9e7d1d1cc0bcef66b23b320d5c645c97bf4fc64

commit b9e7d1d1cc0bcef66b23b320d5c645c97bf4fc64
Author: rego <rego@igalia.com>
Date: Fri Nov 18 12:31:52 2016

Revert of [css-grid] Remove a duplicated auto repeat computation for intrinsic sizes (patchset #4 id:60001 of https://codereview.chromium.org/2287533002/ )

Reason for revert: Caused crash on Mac. See http://crbug.com/662016

We're reverting only the code change as we want to keep the tests,
that will be fixed after the refactoring happening in  bug #627812 
is done.
So, we need to mark 2 tests as failure in TestExpectations file.

BUG=662016, 666688 , 627812 

Original issue's description:
> [css-grid] Remove a duplicated auto repeat computation for intrinsic sizes
>
> After crrev.com/414446 we can safely remove the temporary fix
> (crrev.com/405529) we had to avoid crashes and recursive calls when
> computing the auto repeat tracks count for grids with intrinsic sizes.
>
> This change unveiled a couple of bugs in the computation of auto repeat
> tracks for intrinsic sizes. The first one is that we were not considering the
> existence of definite minimum sizes. In those cases, instead of just returning 1
> repetition, we should return the minimum number of repetitions that fulfill the
> minimum size requirement.
>
> The second bug was that grids were not properly recomputing the number of
> auto repeat tracks when the min|max-content contributions of grid items
> changed (whenever the grid container had an intrinsic width).
>
> One Mozilla test had to be updated too because it was wrong.
> BUG= 621517 , 633474 
>
> Review-Url: https://codereview.chromium.org/2287533002
> Cr-Commit-Position: refs/heads/master@{#425958}

Review-Url: https://codereview.chromium.org/2510393002
Cr-Commit-Position: refs/heads/master@{#433183}

[modify] https://crrev.com/b9e7d1d1cc0bcef66b23b320d5c645c97bf4fc64/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/b9e7d1d1cc0bcef66b23b320d5c645c97bf4fc64/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
[modify] https://crrev.com/b9e7d1d1cc0bcef66b23b320d5c645c97bf4fc64/third_party/WebKit/Source/core/layout/LayoutGrid.h

Project Member

Comment 7 by bugdroid1@chromium.org, Nov 21 2016

Labels: merge-merged-2924
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ab661a19616ff4cd7ff522cef71354971c4d5694

commit ab661a19616ff4cd7ff522cef71354971c4d5694
Author: Manuel Rego Casasnovas <rego@igalia.com>
Date: Mon Nov 21 11:38:41 2016

Revert of [css-grid] Remove a duplicated auto repeat computation for intrinsic sizes (patchset #4 id:60001 of https://codereview.chromium.org/2287533002/ )

Reason for revert: Caused crash on Mac. See http://crbug.com/662016

We're reverting only the code change as we want to keep the tests,
that will be fixed after the refactoring happening in  bug #627812 
is done.
So, we need to mark 2 tests as failure in TestExpectations file.

BUG=662016, 666688 , 627812 

Original issue's description:
> [css-grid] Remove a duplicated auto repeat computation for intrinsic sizes
>
> After crrev.com/414446 we can safely remove the temporary fix
> (crrev.com/405529) we had to avoid crashes and recursive calls when
> computing the auto repeat tracks count for grids with intrinsic sizes.
>
> This change unveiled a couple of bugs in the computation of auto repeat
> tracks for intrinsic sizes. The first one is that we were not considering the
> existence of definite minimum sizes. In those cases, instead of just returning 1
> repetition, we should return the minimum number of repetitions that fulfill the
> minimum size requirement.
>
> The second bug was that grids were not properly recomputing the number of
> auto repeat tracks when the min|max-content contributions of grid items
> changed (whenever the grid container had an intrinsic width).
>
> One Mozilla test had to be updated too because it was wrong.
> BUG= 621517 , 633474 
>
> Review-Url: https://codereview.chromium.org/2287533002
> Cr-Commit-Position: refs/heads/master@{#425958}

Review-Url: https://codereview.chromium.org/2510393002
Cr-Commit-Position: refs/heads/master@{#433183}
(cherry picked from commit b9e7d1d1cc0bcef66b23b320d5c645c97bf4fc64)

Review URL: https://codereview.chromium.org/2521573002 .

Cr-Commit-Position: refs/branch-heads/2924@{#24}
Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}

[modify] https://crrev.com/ab661a19616ff4cd7ff522cef71354971c4d5694/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/ab661a19616ff4cd7ff522cef71354971c4d5694/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
[modify] https://crrev.com/ab661a19616ff4cd7ff522cef71354971c4d5694/third_party/WebKit/Source/core/layout/LayoutGrid.h

Project Member

Comment 8 by bugdroid1@chromium.org, Nov 22 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/70a22ecc29b1197a37f860b88fd0dd56b2e9cd2d

commit 70a22ecc29b1197a37f860b88fd0dd56b2e9cd2d
Author: svillar <svillar@igalia.com>
Date: Tue Nov 22 15:47:06 2016

[css-grid] Isolate internal grid size from the actual grid size

LayoutGrid has an internal representation of a grid used to place grid
items, compute grid positions, run the track sizing algorithm etc. That data
structure normally has exactly the same size as the actual grid specified
using the grid-template-xxx properties (or any other shorthand). But in some
cases, like for example when the grid is empty, the internal data structure
does not really match the actual grid. In the particular case of empty grids
no memory allocations are done to create a grid representation as it is
not needed.

This is the first required step of the process of isolating the data used by
the grid track sizing algorithm from the actual internal state of the
LayoutGrid object.

BUG= 627812 

Review-Url: https://codereview.chromium.org/2521553002
Cr-Commit-Position: refs/heads/master@{#433870}

[modify] https://crrev.com/70a22ecc29b1197a37f860b88fd0dd56b2e9cd2d/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
[modify] https://crrev.com/70a22ecc29b1197a37f860b88fd0dd56b2e9cd2d/third_party/WebKit/Source/core/layout/LayoutGrid.h

Project Member

Comment 9 by bugdroid1@chromium.org, Nov 22 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2c3a3d0ac5362c2d07040a821bb3a22f701d58b6

commit 2c3a3d0ac5362c2d07040a821bb3a22f701d58b6
Author: svillar <svillar@igalia.com>
Date: Tue Nov 22 18:36:12 2016

[css-grid] Convert GridRepresentation into an actual class

So far grids are represented as Vectors of Vectors. There are a couple of
issues associated to that decision. First or all, the source code in
LayoutGrid assumes the existence of that data structure, meaning that we
cannot eventually change it without changing a lot of code. Apart from the
coupling there is another issue, LayoutGrid is full of methods to access and
manipulate that data structure.

Instead, it'd be much better to have a Grid class encapsulating both the
data structures and the methods required to access/manipulate it. Note that
follow-up patches will move more data and procedures into this new class
from the LayoutGrid code.

BUG= 627812 

Review-Url: https://codereview.chromium.org/2522503002
Cr-Commit-Position: refs/heads/master@{#433922}

[modify] https://crrev.com/2c3a3d0ac5362c2d07040a821bb3a22f701d58b6/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
[modify] https://crrev.com/2c3a3d0ac5362c2d07040a821bb3a22f701d58b6/third_party/WebKit/Source/core/layout/LayoutGrid.h

Project Member

Comment 10 by bugdroid1@chromium.org, Nov 23 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a93f35505a7db3243628ea9a7ea24093a8f21d6a

commit a93f35505a7db3243628ea9a7ea24093a8f21d6a
Author: svillar <svillar@igalia.com>
Date: Wed Nov 23 20:53:17 2016

[css-grid] Move LayoutGrid attributes to Grid class

crrev.com/2522503002 added a new class called Grid. This is the first patch
of a series of two that moves several attributes found in LayoutGrid that
really belong to that new class.

Apart from that this is adding a couple of new helper functions that will
decouple the existence of in-flow items from the actual data structures
storing that information.

Last but not least, the Grid::insert() method does not only insert the item
in the m_grid data structure, but also stores the GridArea associated to
that item.

BUG= 627812 

Review-Url: https://codereview.chromium.org/2524963002
Cr-Commit-Position: refs/heads/master@{#434243}

[modify] https://crrev.com/a93f35505a7db3243628ea9a7ea24093a8f21d6a/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
[modify] https://crrev.com/a93f35505a7db3243628ea9a7ea24093a8f21d6a/third_party/WebKit/Source/core/layout/LayoutGrid.h

Project Member

Comment 11 by bugdroid1@chromium.org, Nov 25 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bf7e60b4260824122d9522220919766cd58458c7

commit bf7e60b4260824122d9522220919766cd58458c7
Author: svillar <svillar@igalia.com>
Date: Fri Nov 25 15:41:31 2016

[css-grid] Move more LayoutGrid attributes to Grid class

Moved the remaining attributes of LayoutGrid to Grid class. In particular
m_autoRepeat{Columns|Rows}, m_autoRepeatEmpty{Columns|Rows} and
m_orderIterator. All of them are now private attributes of the Grid class so
clients should use the getters/setters provided by this CL.

This change allows to definitely remove the grid{Column|Row}Count() methods
from LayoutGrid. These two became Grid::numTracks() which returns the number
of tracks in the data structure used to represent the grid (currently
a matrix). Contrary to that, LayoutGrid::numTracks() returns the actual size
of the grid.

BUG= 627812 

Review-Url: https://codereview.chromium.org/2525363002
Cr-Commit-Position: refs/heads/master@{#434510}

[modify] https://crrev.com/bf7e60b4260824122d9522220919766cd58458c7/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
[modify] https://crrev.com/bf7e60b4260824122d9522220919766cd58458c7/third_party/WebKit/Source/core/layout/LayoutGrid.h

Project Member

Comment 12 by bugdroid1@chromium.org, Nov 28 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9b31974d98b5e1c03990d68129e6debca1e37cd6

commit 9b31974d98b5e1c03990d68129e6debca1e37cd6
Author: svillar <svillar@igalia.com>
Date: Mon Nov 28 12:35:29 2016

[css-grid] Cleanup LayoutGrid::Grid

This is a cleanup of some recent additions to the Grid class, basically some
renames and an API change. The hasInFlowGridItems was renamed to
hasGridItems because by definition all grid items are in flow children of
the grid container. Apart from that, in order to remove an extra attribute
for the Grid::insert method, a new method called
setHasAnyOrthogonalGridItem was added to track the existence of orthogonal
items.

BUG= 627812 

Review-Url: https://codereview.chromium.org/2526383004
Cr-Commit-Position: refs/heads/master@{#434641}

[modify] https://crrev.com/9b31974d98b5e1c03990d68129e6debca1e37cd6/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
[modify] https://crrev.com/9b31974d98b5e1c03990d68129e6debca1e37cd6/third_party/WebKit/Source/core/layout/LayoutGrid.h

Project Member

Comment 13 by bugdroid1@chromium.org, Nov 29 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7fe7ce47eb086b9329cd5116a5d09493dac3c97e

commit 7fe7ce47eb086b9329cd5116a5d09493dac3c97e
Author: svillar <svillar@igalia.com>
Date: Tue Nov 29 22:54:57 2016

[css-grid] Pass Grid as argument to the items' placements methods

In order to constify computeIntrinsicLogicalWidths() it is required to
constify placeItemsOnGrid() first, which is the base method of the grid
items' positioning logic. The first step is to constify all the methods
invoked by the latter, which basically means to pass the Grid as argument to
all of them instead of directly using the m_grid attribute from LayoutGrid.

This is the first patch of the two that will decouple the grid items'
placement from the LayoutGrid internal state.

BUG= 627812 

Review-Url: https://codereview.chromium.org/2536703002
Cr-Commit-Position: refs/heads/master@{#435099}

[modify] https://crrev.com/7fe7ce47eb086b9329cd5116a5d09493dac3c97e/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
[modify] https://crrev.com/7fe7ce47eb086b9329cd5116a5d09493dac3c97e/third_party/WebKit/Source/core/layout/LayoutGrid.h

Project Member

Comment 14 by bugdroid1@chromium.org, Dec 7 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b8f9db3a8e2aff0307f677c3b29efc0162a80b5f

commit b8f9db3a8e2aff0307f677c3b29efc0162a80b5f
Author: svillar <svillar@igalia.com>
Date: Wed Dec 07 10:47:43 2016

[css-grid] Pass Grid as argument to more methods & remove m_gridIsDirty

This is the follow up of crrev.com/2536703002, three more methods get the
Grid passed as argument. The remaining methods will access the Grid through
GridSizingData, but that will be another patch.

Apart from that, m_gridIsDirty was removed because it was always too
confusing. It was replaced by Grid's m_needsItemsPlacement which is much
more concise. The dirtyGrid() call was indeed only forcing a replacement of
the grid items.

BUG= 627812 

Review-Url: https://codereview.chromium.org/2541003003
Cr-Commit-Position: refs/heads/master@{#436910}

[modify] https://crrev.com/b8f9db3a8e2aff0307f677c3b29efc0162a80b5f/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
[modify] https://crrev.com/b8f9db3a8e2aff0307f677c3b29efc0162a80b5f/third_party/WebKit/Source/core/layout/LayoutGrid.h

Project Member

Comment 15 by bugdroid1@chromium.org, Dec 15 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/12848de612f70226c03762ae1b7e4bcbc6d2b4a4

commit 12848de612f70226c03762ae1b7e4bcbc6d2b4a4
Author: svillar <svillar@igalia.com>
Date: Thu Dec 15 13:11:43 2016

[css-grid] Move Grid into GridSizingData

The grid track sizing algorithm has been using the m_grid attribute from
LayoutGrid to compute the sizes of the tracks unconditionally. However the
goal is to make it work against a generic instance of the Grid class, so
that the intrinsic size computation and the layout processes could be
effectively decoupled.

Instead of passing the Grid as a new argument to all the track sizing
algorithm methods we leverage the existence of GridSizingData which is
already passed to all of them. This data structure holds from now on
a reference to the Grid instance so that the track sizing algorithm could
use it.

BUG= 627812 

Review-Url: https://codereview.chromium.org/2561073002
Cr-Commit-Position: refs/heads/master@{#438815}

[modify] https://crrev.com/12848de612f70226c03762ae1b7e4bcbc6d2b4a4/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
[modify] https://crrev.com/12848de612f70226c03762ae1b7e4bcbc6d2b4a4/third_party/WebKit/Source/core/layout/LayoutGrid.h

Project Member

Comment 16 by bugdroid1@chromium.org, Dec 27 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b34fbbccb46eae5f12faec3ea00ed3c3bfb2692b

commit b34fbbccb46eae5f12faec3ea00ed3c3bfb2692b
Author: svillar <svillar@igalia.com>
Date: Tue Dec 27 15:12:58 2016

[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
Cr-Commit-Position: refs/heads/master@{#440755}

[modify] https://crrev.com/b34fbbccb46eae5f12faec3ea00ed3c3bfb2692b/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/b34fbbccb46eae5f12faec3ea00ed3c3bfb2692b/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-change-intrinsic-size-with-auto-repeat-tracks.html
[modify] https://crrev.com/b34fbbccb46eae5f12faec3ea00ed3c3bfb2692b/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
[modify] https://crrev.com/b34fbbccb46eae5f12faec3ea00ed3c3bfb2692b/third_party/WebKit/Source/core/layout/LayoutGrid.h

Status: Fixed (was: Assigned)
Closing this

Sign in to add a comment