Bug 132332 – [CSS Grid Layout] Grid optimization keeping track of its status

• Component: CSS
• Status: ASSIGNED

[Manuel Rego Casasnovas]

The following patch is a port of Blink https://chromiumcodereview.appspot.com/21205004

It's making grid layout perftest around 29% faster.

[Manuel Rego Casasnovas]

Created attachment 230366 [details]
Patch

[Manuel Rego Casasnovas]

Created attachment 232075 [details]
Patch

Rebased patch.

[Manuel Rego Casasnovas]

Created attachment 232095 [details]
Patch

Fix build.

[Manuel Rego Casasnovas]

Created attachment 232421 [details]
Patch

Upload rebased patch after r169385.

[Radu Stavila]

Comment on attachment 232421 [details]
Patch

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

> Source/WebCore/rendering/RenderGrid.cpp:1201
> +    // We could avoid to dirty the grid if the new child has definite positions and grid auto-flow property is none.

The original patch from blink contained some additional logic here, it didn't always just dirty the grid. Is there a reason you removed that?

> Source/WebCore/rendering/RenderGrid.cpp:1219
> +    return nextSibling;

I think it would be clearer and better for future modifications to have a single return point. E.g.:

RenderObject* nextSibling = RenderBlock::removeChild(child);
if (!girdIsDirty())
   dirtyGrid();

return nextSibling;

[Manuel Rego Casasnovas]

Comment on attachment 232421 [details]
Patch

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

I'm uploading a new patch changing RenderGrid::removeChild() as suggested.

>> Source/WebCore/rendering/RenderGrid.cpp:1201
>> +    // We could avoid to dirty the grid if the new child has definite positions and grid auto-flow property is none.
> 
> The original patch from blink contained some additional logic here, it didn't always just dirty the grid. Is there a reason you removed that?

Basically the patch in Blink was not right, as it was introducing some bugs some of them due to the extra logic added here.

I've plans to avoid marking the grid as dirty always, but that will be done in a later step.

>> Source/WebCore/rendering/RenderGrid.cpp:1219
>> +    return nextSibling;
> 
> I think it would be clearer and better for future modifications to have a single return point. E.g.:
> 
> RenderObject* nextSibling = RenderBlock::removeChild(child);
> if (!girdIsDirty())
>    dirtyGrid();
> 
> return nextSibling;

Nice suggestion, I've modified it.

[Manuel Rego Casasnovas]

Created attachment 232436 [details]
Patch

[Radu Stavila]

Comment on attachment 232421 [details]
Patch

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

>>> Source/WebCore/rendering/RenderGrid.cpp:1201
>>> +    // We could avoid to dirty the grid if the new child has definite positions and grid auto-flow property is none.
>> 
>> The original patch from blink contained some additional logic here, it didn't always just dirty the grid. Is there a reason you removed that?
> 
> Basically the patch in Blink was not right, as it was introducing some bugs some of them due to the extra logic added here.
> 
> I've plans to avoid marking the grid as dirty always, but that will be done in a later step.

Ok, sounds good.

[Manuel Rego Casasnovas]

Created attachment 233300 [details]
Patch

New version of the patch including a new change from Blink (https://codereview.chromium.org/302083005).

[Manuel Rego Casasnovas]

Comment on attachment 233300 [details]
Patch

This is causing some security issues in Blink, so until it gets stabilized we won't integrate it on WebKit. Probably a new rebase would be needed but that time.