Bug 166883 - [css-grid] Make the grid sizing data persistent through layouts
Summary: [css-grid] Make the grid sizing data persistent through layouts
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Javier Fernandez
URL:
Keywords:
Depends on:
Blocks: 165063
  Show dependency treegraph
 
Reported: 2017-01-10 02:26 PST by Javier Fernandez
Modified: 2017-01-12 10:42 PST (History)
12 users (show)

See Also:


Attachments
Patch (22.25 KB, patch)
2017-01-10 04:00 PST, Javier Fernandez
no flags Details | Formatted Diff | Diff
Patch (26.66 KB, patch)
2017-01-12 10:03 PST, Javier Fernandez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Javier Fernandez 2017-01-10 02:26:46 PST
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.
Comment 1 Javier Fernandez 2017-01-10 04:00:49 PST
Created attachment 298456 [details]
Patch
Comment 2 Darin Adler 2017-01-10 21:18:29 PST
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.
Comment 3 Manuel Rego Casasnovas - OOO (back 1st Sep) 2017-01-11 06:31:05 PST
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?
Comment 4 Javier Fernandez 2017-01-12 04:59:01 PST
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.
Comment 5 Javier Fernandez 2017-01-12 10:03:53 PST
Created attachment 298693 [details]
Patch
Comment 6 WebKit Commit Bot 2017-01-12 10:42:33 PST
Comment on attachment 298693 [details]
Patch

Clearing flags on attachment: 298693

Committed r210669: <http://trac.webkit.org/changeset/210669>
Comment 7 WebKit Commit Bot 2017-01-12 10:42:38 PST
All reviewed patches have been landed.  Closing bug.