Bug 165065

Summary: [css-grid] Move attributes from RenderGrid to the new Grid class
Product: WebKit Reporter: Sergio Villar Senin <svillar>
Component: New BugsAssignee: 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 Flags
Patch
none
Patch darin: review+

Description Sergio Villar Senin 2016-11-24 08:30:11 PST
[css-grid] Move attributes from RenderGrid to the new Grid class
Comment 1 Sergio Villar Senin 2016-11-24 08:37:13 PST
Created attachment 295414 [details]
Patch
Comment 2 Javier Fernandez 2016-11-24 09:43:37 PST
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
Comment 3 Michael Catanzaro 2016-11-24 11:54:57 PST
(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.
Comment 4 Michael Catanzaro 2016-11-24 12:16:43 PST
(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 5 Sergio Villar Senin 2016-11-25 06:56:00 PST
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 6 Javier Fernandez 2016-11-25 07:31:41 PST
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.
Comment 7 Sergio Villar Senin 2016-11-25 08:19:21 PST
Created attachment 295425 [details]
Patch
Comment 8 Sergio Villar Senin 2016-11-28 03:29:06 PST
Committed r208995: <http://trac.webkit.org/changeset/208995>