Bug 166883 – [css-grid] Make the grid sizing data persistent through layouts

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Javier Fernandez]

Until now we were clearing the grid sizing data structures after the grid's layout logic is completed. We were doing so to reduce the memory pressure, since we were not using such data after the layout. 

However, there are certain operations that may need to access such data, like baseline position of a grid container under an inline formatting context. There are other operations that can be optimized by using this data, instead of computing it again, assuming this data is still valid after the layout.

[Javier Fernandez]

Created attachment 298456 [details]
Patch

[Darin Adler]

Comment on attachment 298456 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=298456&action=review

> Source/WebCore/rendering/RenderBox.cpp:60
> +#if ENABLE(CSS_GRID_LAYOUT)
> +#include "RenderGrid.h"
> +#endif

Please put conditional includes into a separate paragraph, rather than trying to sort them in with the unconditional includes. I believe the WebKit coding guidelines call for this explicitly.

> Source/WebCore/rendering/RenderBox.cpp:471
> +#if ENABLE(CSS_GRID_LAYOUT)

Tiny coding style thought: When there’s a long function or multiple functions surrounded by #if, or really any whole function as opposed to some code inside a function, I prefer to leave blank lines before and after the function inside the #if/#endif, otherwise the #if seems like part of the function rather than a guard for the conditional code.

> Source/WebCore/rendering/RenderBox.cpp:472
> +void RenderBox::updateGridPositionAfterStyleChange(const RenderStyle& style,  const RenderStyle* oldStyle)

Extra space here after the comma.

> Source/WebCore/rendering/RenderBox.cpp:474
> +    if (!oldStyle || !parent() || !parent()->isRenderGrid())

Typically we like to write tests like this one using the is<> function, which can handle null checks for you too.

    if (!oldStyle || !is<RenderGrid>(parent()))

> Source/WebCore/rendering/RenderBox.h:657
> +    void updateGridPositionAfterStyleChange(const RenderStyle&,  const RenderStyle* oldStyle);

Extra space here after comma.

> Source/WebCore/rendering/RenderGrid.cpp:-1956
> -void RenderGrid::clearGrid()

Patch removes this function, but does not remove the declaration of it from the class definition!

> Source/WebCore/rendering/RenderGrid.h:34
> +#include "RenderObject.h"

This include is not needed.

> Source/WebCore/rendering/RenderGrid.h:64
> +    void addChild(RenderObject* newChild, RenderObject* beforeChild = 0) final;
> +    void removeChild(RenderObject&) final;

Can these be private instead of public? When we override functions we never need them to be public unless we actually want to call them on a RenderGrid pointer rather than calling them polymorphically.

Also, please use nullptr instead of 0 for the null pointer.

Also, we may not need the default argument for the beforeChild argument if this function is only called polymorphically. There is no need to repeat the default argument simply to override the function.

> Source/WebCore/rendering/RenderGrid.h:68
> +    bool explicitGridDidResize(const RenderStyle&) const;
> +    bool namedGridLinesDefinitionDidChange(const RenderStyle&) const;

Please make these private.

[Manuel Rego Casasnovas]

Comment on attachment 298456 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=298456&action=review

The patch LGTM to me too, just a few comments inline.

Also one question, have you run the perftests? I guess this patch should show improvements there.

> Source/WebCore/ChangeLog:30
> +        (WebCore::RenderGrid::addChild): Make the grid data as dirty.

Did you mean "Mark" instead of "Make"?
The same happens in the next lines.

BTW do we have tests for these add/remove operations?

> Source/WebCore/rendering/RenderBox.cpp:482
> +    if (oldStyle->gridItemColumnStart() == style.gridItemColumnStart()
> +        && oldStyle->gridItemColumnEnd() == style.gridItemColumnEnd()
> +        && oldStyle->gridItemRowStart() == style.gridItemRowStart()
> +        && oldStyle->gridItemRowEnd() == style.gridItemRowEnd()
> +        && oldStyle->order() == style.order()
> +        && oldStyle->hasOutOfFlowPosition() == style.hasOutOfFlowPosition())

I see you added a test for "order", but what happens with the other things.
Do we have any test checking that you can dynamically change "grid-column-start" and the rest?

[Javier Fernandez]

Comment on attachment 298456 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=298456&action=review

>> Source/WebCore/ChangeLog:30
>> +        (WebCore::RenderGrid::addChild): Make the grid data as dirty.
> 
> Did you mean "Mark" instead of "Make"?
> The same happens in the next lines.
> 
> BTW do we have tests for these add/remove operations?

I'll rephrase the ChangeLog comments.

I don't think we have tests for add/remove, so I'll add them. Thanks for pointing it out.

>> Source/WebCore/rendering/RenderBox.cpp:482
>> +        && oldStyle->hasOutOfFlowPosition() == style.hasOutOfFlowPosition())
> 
> I see you added a test for "order", but what happens with the other things.
> Do we have any test checking that you can dynamically change "grid-column-start" and the rest?

Yes, we do have tests to handle style changes on those properties.

[Javier Fernandez]

Created attachment 298693 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 298693 [details]
Patch

Clearing flags on attachment: 298693

Committed r210669: <http://trac.webkit.org/changeset/210669>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.