Bug 165065 – [css-grid] Move attributes from RenderGrid to the new Grid class

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sergio Villar Senin]

[css-grid] Move attributes from RenderGrid to the new Grid class

[Sergio Villar Senin]

Created attachment 295414 [details]
Patch

[Javier Fernandez]

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

[Michael Catanzaro]

(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.

[Michael Catanzaro]

(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.

[Sergio Villar Senin]

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.

[Javier Fernandez]

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.

[Sergio Villar Senin]

Created attachment 295425 [details]
Patch

[Sergio Villar Senin]

Committed r208995: <http://trac.webkit.org/changeset/208995>