Bug 131732 – [CSS Grid Layout] Introduce an explicit type for resolved grid positions

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Manuel Rego Casasnovas]

It would be nice to separate the code related to resolved positions from the rest of the code.

Also have a type for resolved positions will help to avoid mistakes mixing resolved and unresolved positions in grid code.

This has been already fixed in Blink and it'll be ported to WebKit (see https://codereview.chromium.org/166623002/).

[Manuel Rego Casasnovas]

Created attachment 229443 [details]
Patch

ChangeLog has some wrong entries generated by prepare-ChangeLog script. I'll report a bug related to that pointing to this patch as an example. Of course, before landing this should be fixed even manually if the issue is not fixed before.

[Manuel Rego Casasnovas]

Created attachment 229444 [details]
Patch

Fix Mac build.

[Manuel Rego Casasnovas]

Created attachment 229973 [details]
Patch

[Build Bot]

Comment on attachment 229973 [details]
Patch

Attachment 229973 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6170024674852864

New failing tests:
media/W3C/video/networkState/networkState_during_loadstart.html

[Build Bot]

Created attachment 229980 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Manuel Rego Casasnovas]

Created attachment 230109 [details]
Patch

Update ChangeLog once bug #131733 has been fixed.

[Manuel Rego Casasnovas]

Ping reviewers. Thanks.

[Benjamin Poulain]

Hum, using integers for positioning is suspicious. Maybe Zalan will have an opinion about this.

[zalan]

(In reply to comment #8)
> Hum, using integers for positioning is suspicious. Maybe Zalan will have an opinion about this.
These look to be grid area indexes and not pixel positions. (similar to table row/column indexes)

[Manuel Rego Casasnovas]

(In reply to comment #9)
> (In reply to comment #8)
> > Hum, using integers for positioning is suspicious. Maybe Zalan will have an opinion about this.
> These look to be grid area indexes and not pixel positions. (similar to table row/column indexes)

RenderGrid has an internal structure m_grid:
    typedef Vector<RenderBox*, 1> GridCell;
    typedef Vector<Vector<GridCell> > GridRepresentation;
    GridRepresentation m_grid;

These integers are positions inside that structure. You can check the new class GridResolvedPosition comment:
// This class represents an index into one of the dimensions of the grid array.
// Wraps a size_t integer just for the purpose of knowing what we manipulate in the grid code.

[Manuel Rego Casasnovas]

Created attachment 232067 [details]
Patch

Rebased patch.

[Manuel Rego Casasnovas]

Ping reviewers. Thanks.

[Manuel Rego Casasnovas]

Created attachment 232944 [details]
Patch

Rebased patch.

[Sergio Villar Senin]

Comment on attachment 232944 [details]
Patch

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

It's nice to slim down the size of the renderer, I was going to r+ but then I wondered what would be the impact in terms of memory usage. Could you try to run the performance tests (that use huge grids) and paste here the results?

> Source/WebCore/rendering/style/GridResolvedPosition.cpp:50
> +

Not sure it pays off to have these two as they're only used in explicitGridSizeForSide()

> Source/WebCore/rendering/style/GridResolvedPosition.h:144
> +    static size_t explicitGridSizeForSide(const RenderStyle& gridContainerStyle, GridPositionSide);

This does not make any sense to me. Let's just have the static method in the .cpp

[Manuel Rego Casasnovas]

Comment on attachment 232944 [details]
Patch

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

I've run the perftests several times in trunk and with my patch and the numbers are pretty similar:
* auto-grid-lots-of-data:
  * Time:
    * Runs trunk: 360.17 runs/s vs Runs patch: 360.10 runs/s
  * Memory:
    * JSHeap trunk: 239.05 KB vs JSHeap patch: 239.05 KB
    * Malloc trunk: 4504.09 KB vs Malloc patch: 4513.19	KB
* fixed-grid-lots-of-data:
  * Time:
    * Runs trunk: 356.52 runs/s vs Runs patch: 358.62 runs/s
  * Memory:
    * JSHeap trunk: 239.05 KB vs JSHeap patch: 239.05 KB
    * Malloc trunk: 4507.94 KB vs Malloc patch: 4506.38	KB

So it seems, the patch is not regressing.

I'm uploading a new patch with the suggested changes.

>> Source/WebCore/rendering/style/GridResolvedPosition.cpp:50
>> +
> 
> Not sure it pays off to have these two as they're only used in explicitGridSizeForSide()

And in RenderGrid::populateExplicitGridAndOrderIterator().

>> Source/WebCore/rendering/style/GridResolvedPosition.h:144
>> +    static size_t explicitGridSizeForSide(const RenderStyle& gridContainerStyle, GridPositionSide);
> 
> This does not make any sense to me. Let's just have the static method in the .cpp

Yeah, not sure why it is here :-). Removed from the .h.

[Manuel Rego Casasnovas]

Created attachment 232989 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 232989 [details]
Patch

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

OK good figures. Let's get this in. 

I have a couple of nits that should be fixed before landing. Also check the EFL build failure.

> Source/WebCore/rendering/style/GridResolvedPosition.cpp:50
> +

Let's remove these two, they don't have anything to do with a resolved position, they just access the style.

> Source/WebCore/rendering/style/GridResolvedPosition.h:87
> +

Remove the resolvedOppositePosition argument name in these four. I guess it's obvious from the function name.

[Manuel Rego Casasnovas]

Created attachment 233041 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 233041 [details]
Patch for landing.

Clearing flags on attachment: 233041

Committed r169934: <http://trac.webkit.org/changeset/169934>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.