[css-grid] Remove the 2x computation of row sizes w/ indefinite heights

[css-grid] Remove the 2x computation of row sizes w/ indefinite heights

On crrev.com/358816, among other things, we added a second pass of the track
sizing algorithm for rows in order to properly compute row sizes when the
height was indefinite. We did that in order to have a symmetrical
implementation for columns and rows, but unfortunatelly that was not
correct.

Apart from issuing incorrect results in some cases it created a huge
performance issue in the case of nested grids because we were exponentially
increasing the amount of executions of the track sizing algorithm. The attached
performance test shows a 350% improvement with the patch.

BUG=619629
Committed: https://crrev.com/7eeda9249ff7a744d713417830e4b71319879b87
Cr-Commit-Position: refs/heads/master@{#418892}

[svillar, 2016-09-14 18:51:26]

svillar@igalia.com changed reviewers:
+ cbiesinger@chromium.org, eae@chromium.org, jfernandez@igalia.com,
rego@igalia.com

[svillar, 2016-09-14 18:51:27]

Sending out for review

[svillar, 2016-09-14 18:52:10]

Description was changed from

==========
[css-grid] Remove the 2x computation of row sizes w/ indefinite heights

On crrev.com/358816, among other things, we added a second pass of the track
sizing algorithm for rows in order to properly compute row sizes when the
height was indefinite. We did that in order to have a symmetrical
implementation for columns and rows, but unfortunatelly that was not
correct.

Apart from issuing incorrect results in some cases it created a huge
performance issue in the case of nested grids because we were exponentially
increasing the amount of executions of the track sizing algorithm.

BUG=619629
==========

to

==========
[css-grid] Remove the 2x computation of row sizes w/ indefinite heights

On crrev.com/358816, among other things, we added a second pass of the track
sizing algorithm for rows in order to properly compute row sizes when the
height was indefinite. We did that in order to have a symmetrical
implementation for columns and rows, but unfortunatelly that was not
correct.

Apart from issuing incorrect results in some cases it created a huge
performance issue in the case of nested grids because we were exponentially
increasing the amount of executions of the track sizing algorithm. The attached
performance test shows a 350% improvement with the patch.

BUG=619629
==========

[jfernandez, 2016-09-14 21:54:46]

I like the change and it indeed simplifies the tracks sizing logic considerably.


LGTM

https://codereview.chromium.org/2339983002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2339983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:631:
sizingData.nextState();
It's a bit confusing/peculiar that intrinsic size computation became part of the
regular track sizing algorithm.

https://codereview.chromium.org/2339983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1893:
sizingData.sizingOperation = TrackSizing;
Why we need to do this ?  Wouldn't be better place in the layoutBlock function ?
Or perhaps after finishing the computeIntrinsicLogicalHeight logic.

[eae, 2016-09-14 22:06:38]

LGTM

[svillar, 2016-09-15 10:04:34]

Thanks for the review.

https://codereview.chromium.org/2339983002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2339983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1893:
sizingData.sizingOperation = TrackSizing;
On 2016/09/14 21:54:46, jfernandez wrote:
> Why we need to do this ?  Wouldn't be better place in the layoutBlock function
?
> Or perhaps after finishing the computeIntrinsicLogicalHeight logic. 

We need this because the layout of the items involves calling some functions
whose behavior depend on that.

We didn't need that before because we always had a call to
computeUsedBreadthOfGridTracks(), something that does not happen now.

We can do it indeed inside computeIntrinsicLogicalHeight() as well. BTW I think
I'm renaming that function because it is a bit confusing. Perhaps
computeRowSizesForIndefiniteHeight() or something more explicit.

[svillar, 2016-09-15 11:47:56]

The CQ bit was checked by svillar@igalia.com

[svillar, 2016-09-15 11:47:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
jfernandez@igalia.com, eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2339983002/#ps40001
(title: "Patch for landing")

[commit-bot: I haz the power, 2016-09-15 11:48:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-15 11:50:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-15 11:50:26]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[svillar, 2016-09-15 13:13:53]

The CQ bit was checked by svillar@igalia.com

[svillar, 2016-09-15 13:13:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
jfernandez@igalia.com, eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2339983002/#ps80001
(title: "Rebased patch for landing v2")

[commit-bot: I haz the power, 2016-09-15 13:14:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-15 14:27:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-15 14:27:28]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[svillar, 2016-09-15 14:42:29]

The CQ bit was checked by svillar@igalia.com

[commit-bot: I haz the power, 2016-09-15 14:42:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-15 17:15:32]

Description was changed from

==========
[css-grid] Remove the 2x computation of row sizes w/ indefinite heights

On crrev.com/358816, among other things, we added a second pass of the track
sizing algorithm for rows in order to properly compute row sizes when the
height was indefinite. We did that in order to have a symmetrical
implementation for columns and rows, but unfortunatelly that was not
correct.

Apart from issuing incorrect results in some cases it created a huge
performance issue in the case of nested grids because we were exponentially
increasing the amount of executions of the track sizing algorithm. The attached
performance test shows a 350% improvement with the patch.

BUG=619629
==========

to

==========
[css-grid] Remove the 2x computation of row sizes w/ indefinite heights

On crrev.com/358816, among other things, we added a second pass of the track
sizing algorithm for rows in order to properly compute row sizes when the
height was indefinite. We did that in order to have a symmetrical
implementation for columns and rows, but unfortunatelly that was not
correct.

Apart from issuing incorrect results in some cases it created a huge
performance issue in the case of nested grids because we were exponentially
increasing the amount of executions of the track sizing algorithm. The attached
performance test shows a 350% improvement with the patch.

BUG=619629
==========

[commit-bot: I haz the power, 2016-09-15 17:15:34]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-15 17:17:50]

Description was changed from

==========
[css-grid] Remove the 2x computation of row sizes w/ indefinite heights

On crrev.com/358816, among other things, we added a second pass of the track
sizing algorithm for rows in order to properly compute row sizes when the
height was indefinite. We did that in order to have a symmetrical
implementation for columns and rows, but unfortunatelly that was not
correct.

Apart from issuing incorrect results in some cases it created a huge
performance issue in the case of nested grids because we were exponentially
increasing the amount of executions of the track sizing algorithm. The attached
performance test shows a 350% improvement with the patch.

BUG=619629
==========

to

==========
[css-grid] Remove the 2x computation of row sizes w/ indefinite heights

On crrev.com/358816, among other things, we added a second pass of the track
sizing algorithm for rows in order to properly compute row sizes when the
height was indefinite. We did that in order to have a symmetrical
implementation for columns and rows, but unfortunatelly that was not
correct.

Apart from issuing incorrect results in some cases it created a huge
performance issue in the case of nested grids because we were exponentially
increasing the amount of executions of the track sizing algorithm. The attached
performance test shows a 350% improvement with the patch.

BUG=619629

Committed: https://crrev.com/7eeda9249ff7a744d713417830e4b71319879b87
Cr-Commit-Position: refs/heads/master@{#418892}
==========

[commit-bot: I haz the power, 2016-09-15 17:17:51]

Patchset 5 (id:??) landed as
https://crrev.com/7eeda9249ff7a744d713417830e4b71319879b87
Cr-Commit-Position: refs/heads/master@{#418892}

[Manuel Rego, 2016-09-16 04:57:41]

A small comment about the performance test.

https://codereview.chromium.org/2339983002/diff/80001/third_party/WebKit/Perf...
File third_party/WebKit/PerformanceTests/Layout/nested-grid.html (right):

https://codereview.chromium.org/2339983002/diff/80001/third_party/WebKit/Perf...
third_party/WebKit/PerformanceTests/Layout/nested-grid.html:43: <div
class='gridItem' style='grid-area: 1 / 1; background-color: rgb(36, 100,
135)'></div>
"gridItem" class is not defined.

Wouldn't be better set a specific size for the items,
like we do in other tests?

Otherwise all the columns and rows of this grid will have
a 0px breadth.

[svillar, 2016-09-16 11:16:28]

On 2016/09/16 04:57:41, Manuel Rego wrote:
> A small comment about the performance test.
> 
>
https://codereview.chromium.org/2339983002/diff/80001/third_party/WebKit/Perf...
> File third_party/WebKit/PerformanceTests/Layout/nested-grid.html (right):
> 
>
https://codereview.chromium.org/2339983002/diff/80001/third_party/WebKit/Perf...
> third_party/WebKit/PerformanceTests/Layout/nested-grid.html:43: <div
> class='gridItem' style='grid-area: 1 / 1; background-color: rgb(36, 100,
> 135)'></div>
> "gridItem" class is not defined.
> 
> Wouldn't be better set a specific size for the items,
> like we do in other tests?
> 
> Otherwise all the columns and rows of this grid will have
> a 0px breadth.

Tracks are all flex so the maximum does not depend on that.