Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2524963002:
[css-grid] Move LayoutGrid attributes to Grid class (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 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
Committed: https://crrev.com/a93f35505a7db3243628ea9a7ea24093a8f21d6a
Cr-Commit-Position: refs/heads/master@{#434243}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Patch for landing #Patch Set 3 : Patch for landing v2 #
Messages
Total messages: 18 (8 generated)
svillar@igalia.com changed reviewers: + cbiesinger@chromium.org, eae@chromium.org, jfernandez@igalia.com, rego@igalia.com
LGTM, but please review the comments before landing. https://codereview.chromium.org/2524963002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2524963002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:72: m_gridItemArea.set(&child, area); You can call setGridItemArea() just in case something related to that changes in the future. https://codereview.chromium.org/2524963002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2051: m_grid.setGridItemArea(*child, area); I'm wondering if this line is really needed. Now we're setting it on insert(), that's why you move it inside the if. But we also do insert in the methods for auto-placed items, so probably that would be enough. https://codereview.chromium.org/2524963002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.h (right): https://codereview.chromium.org/2524963002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.h:358: bool hasInFlowGridItems() const { return !m_gridItemArea.isEmpty(); } Really cool you're adding this method! https://codereview.chromium.org/2524963002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.h:361: GridArea gridItemArea(const LayoutBox& item) const; I think you can omit the variable name here. https://codereview.chromium.org/2524963002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.h:362: void setGridItemArea(const LayoutBox& item, GridArea); Ditto. https://codereview.chromium.org/2524963002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.h:364: size_t gridItemPaintOrder(const LayoutBox& item) const; Ditto. https://codereview.chromium.org/2524963002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.h:365: void setGridItemPaintOrder(const LayoutBox& item, size_t order); Ditto.
Thanks for the review! https://codereview.chromium.org/2524963002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2524963002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:72: m_gridItemArea.set(&child, area); On 2016/11/23 13:30:36, Manuel Rego wrote: > You can call setGridItemArea() just in case something > related to that changes in the future. OK https://codereview.chromium.org/2524963002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2051: m_grid.setGridItemArea(*child, area); On 2016/11/23 13:30:36, Manuel Rego wrote: > I'm wondering if this line is really needed. > > Now we're setting it on insert(), that's why you move it inside the if. > But we also do insert in the methods for auto-placed items, > so probably that would be enough. Yes it's mandatory to do this here before the calls to placeSpecifiedMajorAxisItemsOnGrid/placeAutoMajorAxisItemsOnGrid because assume the GridArea was already inserted. https://codereview.chromium.org/2524963002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.h (right): https://codereview.chromium.org/2524963002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.h:361: GridArea gridItemArea(const LayoutBox& item) const; On 2016/11/23 13:30:36, Manuel Rego wrote: > I think you can omit the variable name here. The style checker didn't complain so I left them. LayoutBox does not say much so perhaps having the "item" word is not a bad thing to have.
The CQ bit was checked by svillar@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from rego@igalia.com Link to the patchset: https://codereview.chromium.org/2524963002/#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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
https://codereview.chromium.org/2524963002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2524963002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2051: m_grid.setGridItemArea(*child, area); On 2016/11/23 14:10:30, svillar wrote: > On 2016/11/23 13:30:36, Manuel Rego wrote: > > I'm wondering if this line is really needed. > > > > Now we're setting it on insert(), that's why you move it inside the if. > > But we also do insert in the methods for auto-placed items, > > so probably that would be enough. > > Yes it's mandatory to do this here before the calls to > placeSpecifiedMajorAxisItemsOnGrid/placeAutoMajorAxisItemsOnGrid because assume > the GridArea was already inserted. True, I didn't realize about that. Thanks!
The CQ bit was checked by svillar@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from rego@igalia.com Link to the patchset: https://codereview.chromium.org/2524963002/#ps40001 (title: "Patch for landing v2")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1479920957604770,
"parent_rev": "6a6ae5a590774a0d2619017c9c9cfc2b3b892b1d", "commit_rev":
"d24fe3201a542818c1319d42eb59e1ffb2676ddd"}
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [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 ========== to ========== [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 Committed: https://crrev.com/a93f35505a7db3243628ea9a7ea24093a8f21d6a Cr-Commit-Position: refs/heads/master@{#434243} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a93f35505a7db3243628ea9a7ea24093a8f21d6a Cr-Commit-Position: refs/heads/master@{#434243} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
