[css-grid] Intrinsic width computation should not alter the internal state of LayoutGrid |
||||||
Issue descriptionThe 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.
,
Jul 13 2016
,
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
,
Nov 14 2016
,
Nov 18 2016
,
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
,
Nov 21 2016
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
,
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
,
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
,
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
,
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
,
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
,
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
,
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
,
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
,
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
,
Jan 9 2017
Closing this |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by svil...@igalia.com
, Jul 13 2016