Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2561073002:
[css-grid] Move Grid into GridSizingData (Closed)
|
Created:
4 years ago by svillar Modified:
4 years ago CC:
chromium-reviews, jfernandez, szager+layoutwatch_chromium.org, zoltan1, svillar, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, Manuel Rego, leviw+renderwatch, jchaffraix+rendering, blink-reviews, eae+blinkwatch Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[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
Committed: https://crrev.com/12848de612f70226c03762ae1b7e4bcbc6d2b4a4
Cr-Commit-Position: refs/heads/master@{#438815}
Patch Set 1 #
Total comments: 18
Patch Set 2 : Patch for landing #
Messages
Total messages: 19 (9 generated)
Description was changed from ========== [css-grid] Move Grid into SizingData 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 ========== to ========== [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 ==========
svillar@igalia.com changed reviewers: + cbiesinger@chromium.org, eae@chromium.org, jfernandez@igalia.com, rego@igalia.com
Beh, I've just noticed that I hadn't sent it out for review
https://codereview.chromium.org/2561073002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2561073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:468: Grid& m_grid; Why a reference here ? Didn't we want to have independent Grid data between intrinsic size and layout ? https://codereview.chromium.org/2561073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:556: guttersSize(sizingData.grid(), ForRows, 0, sizingData.rowTracks.size(), We could perhaps join grid and sizingOperation arguments and just use sizingData. https://codereview.chromium.org/2561073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:569: availableSpace - guttersSize(sizingData.grid(), direction, 0, Ditto. https://codereview.chromium.org/2561073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:816: const_cast<Grid&>(m_grid)); umm, wouldn't be a good idea to get rid of this const_cast ? I remember that it was one of the reasons of this code refactoring. https://codereview.chromium.org/2561073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:983: Grid& grid = sizingData.grid(); const Grid& grid ? https://codereview.chromium.org/2561073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1531: Grid& grid = sizingData.grid(); const ? https://codereview.chromium.org/2561073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2441: DCHECK(!m_grid.needsItemsPlacement()); Perhaps too late for this suggestion, and not very related to the goal of this patch, but I think it'd be better/clearer to assert that "items placement" has been done already, instead of this negative clause. https://codereview.chromium.org/2561073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2718: DCHECK(!m_grid.needsItemsPlacement()); ditto https://codereview.chromium.org/2561073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2848: Grid& grid = sizingData.grid(); const ? https://codereview.chromium.org/2561073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:3610: DCHECK(!m_grid.needsItemsPlacement()); ditto. https://codereview.chromium.org/2561073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:3628: DCHECK(!grid.needsItemsPlacement()); Ditto.
lgtm
Thanks for the review! https://codereview.chromium.org/2561073002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2561073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:468: Grid& m_grid; On 2016/12/14 10:41:55, jfernandez wrote: > Why a reference here ? Didn't we want to have independent Grid data between > intrinsic size and layout ? A reference because it cannot be null. https://codereview.chromium.org/2561073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:556: guttersSize(sizingData.grid(), ForRows, 0, sizingData.rowTracks.size(), On 2016/12/14 10:41:55, jfernandez wrote: > We could perhaps join grid and sizingOperation arguments and just use > sizingData. Yes I thought about that but there are usages of guttersSize() in functions that do not receive a GridSizingData. https://codereview.chromium.org/2561073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:816: const_cast<Grid&>(m_grid)); On 2016/12/14 10:41:54, jfernandez wrote: > umm, wouldn't be a good idea to get rid of this const_cast ? I remember that it > was one of the reasons of this code refactoring. Yes it was one of the reasons. The problem is that we cannot do it now. You could only remove it when instead of passing m_grid, we'd be passing another instance of Grid. https://codereview.chromium.org/2561073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:983: Grid& grid = sizingData.grid(); On 2016/12/14 10:41:55, jfernandez wrote: > const Grid& grid ? Acknowledged. https://codereview.chromium.org/2561073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1531: Grid& grid = sizingData.grid(); On 2016/12/14 10:41:55, jfernandez wrote: > const ? Acknowledged. https://codereview.chromium.org/2561073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2441: DCHECK(!m_grid.needsItemsPlacement()); On 2016/12/14 10:41:55, jfernandez wrote: > Perhaps too late for this suggestion, and not very related to the goal of this > patch, but I think it'd be better/clearer to assert that "items placement" has > been done already, instead of this negative clause. OK we could change it later if needed. https://codereview.chromium.org/2561073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2848: Grid& grid = sizingData.grid(); On 2016/12/14 10:41:55, jfernandez wrote: > const ? Acknowledged.
The CQ bit was checked by svillar@igalia.com
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/2561073002/#ps20001 (title: "Patch for landing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
LGTM
The CQ bit was checked by svillar@igalia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1481799183140880,
"parent_rev": "0b1976d889797d0854d4b89d0c5395ed0941dc8b", "commit_rev":
"671e85630c0bd3491b78f73290a070ecbe82aead"}
Message was sent while issue was closed.
Description was changed from ========== [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 ========== to ========== [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 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [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 ========== to ========== [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 Committed: https://crrev.com/12848de612f70226c03762ae1b7e4bcbc6d2b4a4 Cr-Commit-Position: refs/heads/master@{#438815} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/12848de612f70226c03762ae1b7e4bcbc6d2b4a4 Cr-Commit-Position: refs/heads/master@{#438815} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
