Bug 165065 - [css-grid] Move attributes from RenderGrid to the new Grid class
Summary: [css-grid] Move attributes from RenderGrid to the new Grid class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sergio Villar Senin
URL:
Keywords:
Depends on:
Blocks: 165007
  Show dependency treegraph
 
Reported: 2016-11-24 08:30 PST by Sergio Villar Senin
Modified: 2016-11-28 03:29 PST (History)
10 users (show)

See Also:


Attachments
Patch (17.88 KB, patch)
2016-11-24 08:37 PST, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (17.32 KB, patch)
2016-11-25 08:19 PST, Sergio Villar Senin
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>