Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2536703002:
[css-grid] Pass Grid as argument to the items' placements methods (Closed)
|
Created:
4 years ago by svillar Modified:
4 years ago CC:
chromium-reviews, jfernandez, szager+layoutwatch_chromium.org, zoltan1, 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] 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
Committed: https://crrev.com/7fe7ce47eb086b9329cd5116a5d09493dac3c97e
Cr-Commit-Position: refs/heads/master@{#435099}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Rebased patch for landing #
Messages
Total messages: 21 (9 generated)
svillar@igalia.com changed reviewers: + cbiesinger@chromium.org, eae@chromium.org, jfernandez@igalia.com, rego@igalia.com
If this change is kind of a intermediate step, I guess it LGTM, but I like to know the future plans because passing a Grid copy instead of using directly the m_grid attribute seems a bit artificial.
On 2016/11/29 11:23:48, jfernandez wrote: > If this change is kind of a intermediate step, I guess it LGTM, but I like to > know the future plans because passing a Grid copy instead of using directly the > m_grid attribute seems a bit artificial. I don't understand this very well. We're passing a reference so there is no copy of the Grid structure there. Secondly, this is indeed an intermediate step because we're still using m_grid everywhere, even the methods that receive Grid as argument are actually working against m_grid. Finally, the future plans are to use an unique Grid instance for the intrinsic size computation. That way all the data generated by intrinsic size computation won't interfere with the layout process. On a second step, we could decide to keep part of the cached data generated by intrinsic size computation to be reused during layout process if needed.
On 2016/11/29 11:42:30, svillar wrote: > On 2016/11/29 11:23:48, jfernandez wrote: > > If this change is kind of a intermediate step, I guess it LGTM, but I like to > > know the future plans because passing a Grid copy instead of using directly > the > > m_grid attribute seems a bit artificial. > > I don't understand this very well. We're passing a reference so there is no copy > of the Grid structure there. > > Secondly, this is indeed an intermediate step because we're still using m_grid > everywhere, even the methods that receive Grid as argument are actually working > against m_grid. > > Finally, the future plans are to use an unique Grid instance for the intrinsic > size computation. That way all the data generated by intrinsic size computation > won't interfere with the layout process. On a second step, we could decide to > keep part of the cached data generated by intrinsic size computation to be > reused during layout process if needed. Just to make this crystal clear the plan is the following. The first step is to pass Grid as argument to all the items' placement methods. That would allow us to use m_grid for the Layout process, and a different new instance for the intrinsic size computation. Something like this Layout: ------- placeItemsOnGrid(m_grid) Intrinsic size computation -------------------------- Grid grid; placeItemsOnGrid(grid);
LGTM too. https://codereview.chromium.org/2536703002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2536703002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2062: m_grid.setAutoRepeatTracks(autoRepeatRows, autoRepeatColumns); It still uses m_grid here, I guess it's just becuase it's the first patch.
https://codereview.chromium.org/2536703002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2536703002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2062: m_grid.setAutoRepeatTracks(autoRepeatRows, autoRepeatColumns); On 2016/11/29 13:03:20, Manuel Rego wrote: > It still uses m_grid here, I guess it's just becuase it's the first patch. Yeah, this patch is just changing some calls inside placeItemsOnGrid()
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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, rego@igalia.com Link to the patchset: https://codereview.chromium.org/2536703002/#ps20001 (title: "Rebased 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 svillar@igalia.com
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": 1480446920667860,
"parent_rev": "bf9e9aa6fad833acfc3abaa5e847ffcff13d643a", "commit_rev":
"4f14d1114ad78111e3b684b4ce4842d3509d4c2c"}
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] 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 ========== to ========== [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 Committed: https://crrev.com/7fe7ce47eb086b9329cd5116a5d09493dac3c97e Cr-Commit-Position: refs/heads/master@{#435099} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/7fe7ce47eb086b9329cd5116a5d09493dac3c97e Cr-Commit-Position: refs/heads/master@{#435099} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
