Bug 131732

Summary: [CSS Grid Layout] Introduce an explicit type for resolved grid positions
Product: WebKit Reporter: Manuel Rego Casasnovas <rego>
Component: CSSAssignee: Manuel Rego Casasnovas <rego>
Status: RESOLVED FIXED    
Severity: Normal CC: allan.jensen, benjamin, buildbot, bunhere, clopez, commit-queue, darin, dino, enrica, esprehn+autocc, glenn, gyuyoung.kim, hyatt, jfernandez, kling, kondapallykalyan, macpherson, menard, rakuco, rniwa, sergio, simon.fraser, stavila, svillar, thorton, zalan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 60731    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing. none

Description Manuel Rego Casasnovas 2014-04-16 07:40:05 PDT
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/).
Comment 1 Manuel Rego Casasnovas 2014-04-16 07:46:55 PDT
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.
Comment 2 Manuel Rego Casasnovas 2014-04-16 08:41:18 PDT
Created attachment 229444 [details]
Patch

Fix Mac build.
Comment 3 Manuel Rego Casasnovas 2014-04-23 03:47:12 PDT
Created attachment 229973 [details]
Patch
Comment 4 Build Bot 2014-04-23 04:58:28 PDT
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
Comment 5 Build Bot 2014-04-23 04:58:34 PDT
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
Comment 6 Manuel Rego Casasnovas 2014-04-24 14:51:48 PDT
Created attachment 230109 [details]
Patch

Update ChangeLog once bug #131733 has been fixed.
Comment 7 Manuel Rego Casasnovas 2014-05-22 14:50:28 PDT
Ping reviewers. Thanks.
Comment 8 Benjamin Poulain 2014-05-22 15:26:33 PDT
Hum, using integers for positioning is suspicious. Maybe Zalan will have an opinion about this.
Comment 9 zalan 2014-05-22 16:11:09 PDT
(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)
Comment 10 Manuel Rego Casasnovas 2014-05-23 02:31:20 PDT
(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.
Comment 11 Manuel Rego Casasnovas 2014-05-26 04:20:25 PDT
Created attachment 232067 [details]
Patch

Rebased patch.
Comment 12 Manuel Rego Casasnovas 2014-06-04 14:35:51 PDT
Ping reviewers. Thanks.
Comment 13 Manuel Rego Casasnovas 2014-06-12 04:28:13 PDT
Created attachment 232944 [details]
Patch

Rebased patch.
Comment 14 Sergio Villar Senin 2014-06-12 07:05:22 PDT
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
Comment 15 Manuel Rego Casasnovas 2014-06-12 14:42:59 PDT
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.
Comment 16 Manuel Rego Casasnovas 2014-06-12 14:51:03 PDT
Created attachment 232989 [details]
Patch
Comment 17 Sergio Villar Senin 2014-06-13 04:44:42 PDT
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.
Comment 18 Manuel Rego Casasnovas 2014-06-13 05:06:08 PDT
Created attachment 233041 [details]
Patch for landing.
Comment 19 WebKit Commit Bot 2014-06-13 06:10:31 PDT
Comment on attachment 233041 [details]
Patch for landing.

Clearing flags on attachment: 233041

Committed r169934: <http://trac.webkit.org/changeset/169934>
Comment 20 WebKit Commit Bot 2014-06-13 06:10:44 PDT
All reviewed patches have been landed.  Closing bug.