Summary: | [css-grid] Move attributes from RenderGrid to the new Grid class | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sergio Villar Senin <svillar> | ||||||
Component: | New Bugs | Assignee: | Sergio Villar Senin <svillar> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, darin, esprehn+autocc, glenn, jfernandez, joepeck, kondapallykalyan, mcatanzaro, rego, svillar | ||||||
Priority: | P2 | ||||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 165007 | ||||||||
Attachments: |
|
Description
Sergio Villar Senin
2016-11-24 08:30:11 PST
Created attachment 295414 [details]
Patch
Comment on attachment 295414 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=295414&action=review > Source/WebCore/rendering/RenderGrid.cpp:65 > +void RenderGrid::Grid::insert(RenderBox& child, const GridArea& area, bool isOrthogonalChild) Not a huge fan of boolean arguments to change the behavior of the function. We could, perhaps, cache the grid's writing mode so we can determine inside the function whether the item is orthogonal or not. > Source/WebCore/rendering/RenderGrid.cpp:77 > + m_hasAnyOrthogonalChildren = m_hasAnyOrthogonalChildren || isOrthogonalChild; We can determine the value of m_hasAnyOrthogonalChildren as part of the LayoutGrid instance logic, perhaps just before calling the 'insert' function. > Source/WebCore/rendering/RenderGrid.cpp:1715 > + m_grid.insert(*child, { area.rows, area.columns }, isOrthogonalChild(*child)); I'd rather avoid to pass the child's orthogonality as parameter. > Source/WebCore/rendering/RenderGrid.cpp:1815 > + m_grid.insert(*autoGridItem, *emptyGridArea, isOrthogonalChild(*autoGridItem)); Ditto. > Source/WebCore/rendering/RenderGrid.cpp:1887 > + m_grid.insert(gridItem, *emptyGridArea, isOrthogonalChild(gridItem)); Ditto. > Source/WebCore/rendering/RenderGrid.h:212 > + bool hasInFlowGridItems() const { return !m_gridItemArea.isEmpty(); } I really don't get the "InFlow" term here. Gird containers are supposed to have only InFlow elements. - https://github.com/w3c/csswg-drafts/issues/639#issuecomment-256254195 (In reply to comment #2) > Not a huge fan of boolean arguments to change the behavior of the function. Yeah, we don't want to add more of these to WebKit anymore. The easiest way to avoid this is to use an enum with two values. (In reply to comment #3) > Yeah, we don't want to add more of these to WebKit anymore. The easiest way > to avoid this is to use an enum with two values. Like Carlos does in bug #164917. Comment on attachment 295414 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=295414&action=review >> Source/WebCore/rendering/RenderGrid.cpp:65 >> +void RenderGrid::Grid::insert(RenderBox& child, const GridArea& area, bool isOrthogonalChild) > > Not a huge fan of boolean arguments to change the behavior of the function. We could, perhaps, cache the grid's writing mode so we can determine inside the function whether the item is orthogonal or not. Caching it would mean listening to style changes, something that we don't want to do at Grid's level. But yeah I should not have used a boolean there. >> Source/WebCore/rendering/RenderGrid.cpp:77 >> + m_hasAnyOrthogonalChildren = m_hasAnyOrthogonalChildren || isOrthogonalChild; > > We can determine the value of m_hasAnyOrthogonalChildren as part of the LayoutGrid instance logic, perhaps just before calling the 'insert' function. Not sure what you mean. We were doing it in LayoutGrid. The point is that I now want to store it in Grid. >> Source/WebCore/rendering/RenderGrid.h:212 >> + bool hasInFlowGridItems() const { return !m_gridItemArea.isEmpty(); } > > I really don't get the "InFlow" term here. Gird containers are supposed to have only InFlow elements. > - https://github.com/w3c/csswg-drafts/issues/639#issuecomment-256254195 Right, so all the grid items are in flow children. But there are some children that might be out of flow like absolutely positioned children, but you're right that they are not technically grid items but just children of the grid container. Comment on attachment 295414 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=295414&action=review >>> Source/WebCore/rendering/RenderGrid.cpp:65 >>> +void RenderGrid::Grid::insert(RenderBox& child, const GridArea& area, bool isOrthogonalChild) >> >> Not a huge fan of boolean arguments to change the behavior of the function. We could, perhaps, cache the grid's writing mode so we can determine inside the function whether the item is orthogonal or not. > > Caching it would mean listening to style changes, something that we don't want to do at Grid's level. But yeah I should not have used a boolean there. A change in grid's writing mode will trigger the execution of the LayoutGrid layout logic, so I think we will create again the Grid internal structure after updating the cached grid's writing mode. The grid items will be inserted again, so we will check out their orthogonality again. >>> Source/WebCore/rendering/RenderGrid.cpp:77 >>> + m_hasAnyOrthogonalChildren = m_hasAnyOrthogonalChildren || isOrthogonalChild; >> >> We can determine the value of m_hasAnyOrthogonalChildren as part of the LayoutGrid instance logic, perhaps just before calling the 'insert' function. > > Not sure what you mean. We were doing it in LayoutGrid. The point is that I now want to store it in Grid. Sure, sorry. I didn't realize that this field is not part of the Grid internal class. >> Source/WebCore/rendering/RenderGrid.cpp:1715 >> + m_grid.insert(*child, { area.rows, area.columns }, isOrthogonalChild(*child)); > > I'd rather avoid to pass the child's orthogonality as parameter. Another option is to determine orthogonality and assign the value to the "anyOrthogonalItem" internal grid's attribute. Created attachment 295425 [details]
Patch
Committed r208995: <http://trac.webkit.org/changeset/208995> |