[css-grid] Use grid area position to determine overflowing items

[css-grid] Use grid area position to determine overflowing items

Grid code keeps a list of items overflowing their grid areas so that it
could apply some painting optimizations to reduce the amount of grid items
to be painted. Even if the border box is completely contained by the
item's grid area, margins in the item could make it overflow its grid area
by altering the border box position.

The problem is not that the optimization won't work, it's worse because it will
make that item not visible as it won't be painted.

BUG=628155
Committed: https://crrev.com/a67debf9a9b78f60e7b10b5574ae4ceab49af31b
Cr-Commit-Position: refs/heads/master@{#420094}

[svillar, 2016-09-19 17:15:59]

Description was changed from

==========
[css-grid] Use margin box to determine items overflowing their grid area

Grid code keeps a list of items overflowing their grid areas so that it
could apply some painting optimizations to reduce the amount of grid items
to be painted. Margin box should be used to determine whether or not an item
overflows its grid area otherwise setting a large enough margin could easily
move an item out of its grid area. The problem is that the optimization will
make that item not visible as it won't be painted.

BUG=628155
==========

to

==========
[css-grid] Use margin box to determine items overflowing their grid area

Grid code keeps a list of items overflowing their grid areas so that it
could apply some painting optimizations to reduce the amount of grid items
to be painted. Margin box should be used to determine whether or not an item
overflows its grid area otherwise setting a large enough margin could easily
move an item out of its grid area. The problem is that the optimization will
make that item not visible as it won't be painted.

A nice side effect of this patch is that it brings a 2% improvement in 
auto-grid-lots-of-data.html and a 9% improvement in
fixed-grid-lots-of-stretched-data.html

BUG=628155
==========

[svillar, 2016-09-19 17:15:59]

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

[cbiesinger, 2016-09-19 17:25:12]

The change seems fine but I don't see where this uses the margin box? frame rect
is the border box

[jfernandez, 2016-09-19 18:01:24]

https://codereview.chromium.org/2353513003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2353513003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1934: // TODO (lajava):
Child's margins should account when evaluating whether it overflows its grid
area (http://crbug.com/628155).
Remove the TODO, please.

https://codereview.chromium.org/2353513003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1938: if
(!gridAreaRect.contains(child->frameRect()))
Using FrameRect should be the same than doing operations with height()(width(),
like in the old code. I don't understand why this is solving the issue.
Additionally, wouldn't we take into account the box-sizing property ?

[cbiesinger, 2016-09-19 18:07:04]

frameRect is always the border box rect -- box sizing is already taken into
account for it.

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[cbiesinger, 2016-09-19 18:07:05]

frameRect is always the border box rect -- box sizing is already taken into
account for it.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[svillar, 2016-09-20 11:10:18]

On 2016/09/19 17:25:12, cbiesinger wrote:
> The change seems fine but I don't see where this uses the margin box? frame
rect
> is the border box

Hmm yeah so I think the problem is that I did a bad job in the description of
the bug. The actual issue is that we were only considering the child size when
checking whether or not it overflowed the grid area. We should obviously also
consider child's location because that might alter the result. I'll try to
correct the description.

[svillar, 2016-09-20 11:12:47]

https://codereview.chromium.org/2353513003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2353513003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1934: // TODO (lajava):
Child's margins should account when evaluating whether it overflows its grid
area (http://crbug.com/628155).
On 2016/09/19 18:01:24, jfernandez wrote:
> Remove the TODO, please.

Acknowledged.

https://codereview.chromium.org/2353513003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1938: if
(!gridAreaRect.contains(child->frameRect()))
On 2016/09/19 18:01:24, jfernandez wrote:
> Using FrameRect should be the same than doing operations with
height()(width(),
> like in the old code. I don't understand why this is solving the issue.
> Additionally, wouldn't we take into account the box-sizing property ?  

The main difference are not the sizes indeed, but the location of the grid area
vs the location of the child.

[jfernandez, 2016-09-20 11:48:59]

I understand now, thanks for the explanations.
LGTM.

[svillar, 2016-09-20 12:43:17]

Description was changed from

==========
[css-grid] Use margin box to determine items overflowing their grid area

Grid code keeps a list of items overflowing their grid areas so that it
could apply some painting optimizations to reduce the amount of grid items
to be painted. Margin box should be used to determine whether or not an item
overflows its grid area otherwise setting a large enough margin could easily
move an item out of its grid area. The problem is that the optimization will
make that item not visible as it won't be painted.

A nice side effect of this patch is that it brings a 2% improvement in 
auto-grid-lots-of-data.html and a 9% improvement in
fixed-grid-lots-of-stretched-data.html

BUG=628155
==========

to

==========
[css-grid] Use grid area position to determine overflowing items

Grid code keeps a list of items overflowing their grid areas so that it
could apply some painting optimizations to reduce the amount of grid items
to be painted. Even if the border box is completely contained by the item's grid
area, margins in the item could make it overflow its grid area by altering the
border box position.

The problem is not that the optimization won't work, is worsdt because it will
make that item not visible as it won't be painted.

BUG=628155
==========

[svillar, 2016-09-20 12:43:31]

Description was changed from

==========
[css-grid] Use grid area position to determine overflowing items

Grid code keeps a list of items overflowing their grid areas so that it
could apply some painting optimizations to reduce the amount of grid items
to be painted. Even if the border box is completely contained by the item's grid
area, margins in the item could make it overflow its grid area by altering the
border box position.

The problem is not that the optimization won't work, is worsdt because it will
make that item not visible as it won't be painted.

BUG=628155
==========

to

==========
[css-grid] Use grid area position to determine overflowing items

Grid code keeps a list of items overflowing their grid areas so that it
could apply some painting optimizations to reduce the amount of grid items
to be painted. Even if the border box is completely contained by the item's grid
area, margins in the item could make it overflow its grid area by altering the
border box position.

The problem is not that the optimization won't work, is worsdt because it will
make that item not visible as it won't be painted.

BUG=628155
==========

[svillar, 2016-09-20 12:43:51]

Description was changed from

==========
[css-grid] Use grid area position to determine overflowing items

Grid code keeps a list of items overflowing their grid areas so that it
could apply some painting optimizations to reduce the amount of grid items
to be painted. Even if the border box is completely contained by the item's grid
area, margins in the item could make it overflow its grid area by altering the
border box position.

The problem is not that the optimization won't work, is worsdt because it will
make that item not visible as it won't be painted.

BUG=628155
==========

to

==========
[css-grid] Use grid area position to determine overflowing items

Grid code keeps a list of items overflowing their grid areas so that it
could apply some painting optimizations to reduce the amount of grid items
to be painted. Even if the border box is completely contained by the 
item's grid area, margins in the item could make it overflow its grid area
by altering the border box position.

The problem is not that the optimization won't work, is worsdt because it will
make that item not visible as it won't be painted.

BUG=628155
==========

[cbiesinger, 2016-09-20 12:57:26]

Description was changed from

==========
[css-grid] Use grid area position to determine overflowing items

Grid code keeps a list of items overflowing their grid areas so that it
could apply some painting optimizations to reduce the amount of grid items
to be painted. Even if the border box is completely contained by the 
item's grid area, margins in the item could make it overflow its grid area
by altering the border box position.

The problem is not that the optimization won't work, is worsdt because it will
make that item not visible as it won't be painted.

BUG=628155
==========

to

==========
[css-grid] Use grid area position to determine overflowing items

Grid code keeps a list of items overflowing their grid areas so that it
could apply some painting optimizations to reduce the amount of grid items
to be painted. Even if the border box is completely contained by the 
item's grid area, margins in the item could make it overflow its grid area
by altering the border box position.

The problem is not that the optimization won't work, it's worse because it will
make that item not visible as it won't be painted.

BUG=628155
==========

[cbiesinger, 2016-09-20 12:58:16]

thanks, lgtm

[svillar, 2016-09-20 13:39:40]

The CQ bit was checked by svillar@igalia.com

[commit-bot: I haz the power, 2016-09-20 13:39:59]

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-20 15:19:05]

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

[commit-bot: I haz the power, 2016-09-20 15:19:06]

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-21 15:15:04]

The CQ bit was checked by svillar@igalia.com

[commit-bot: I haz the power, 2016-09-21 15:15: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-21 17:29:57]

Description was changed from

==========
[css-grid] Use grid area position to determine overflowing items

Grid code keeps a list of items overflowing their grid areas so that it
could apply some painting optimizations to reduce the amount of grid items
to be painted. Even if the border box is completely contained by the 
item's grid area, margins in the item could make it overflow its grid area
by altering the border box position.

The problem is not that the optimization won't work, it's worse because it will
make that item not visible as it won't be painted.

BUG=628155
==========

to

==========
[css-grid] Use grid area position to determine overflowing items

Grid code keeps a list of items overflowing their grid areas so that it
could apply some painting optimizations to reduce the amount of grid items
to be painted. Even if the border box is completely contained by the 
item's grid area, margins in the item could make it overflow its grid area
by altering the border box position.

The problem is not that the optimization won't work, it's worse because it will
make that item not visible as it won't be painted.

BUG=628155
==========

[commit-bot: I haz the power, 2016-09-21 17:29:58]

Committed patchset #2 (id:20001)

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

Description was changed from

==========
[css-grid] Use grid area position to determine overflowing items

Grid code keeps a list of items overflowing their grid areas so that it
could apply some painting optimizations to reduce the amount of grid items
to be painted. Even if the border box is completely contained by the 
item's grid area, margins in the item could make it overflow its grid area
by altering the border box position.

The problem is not that the optimization won't work, it's worse because it will
make that item not visible as it won't be painted.

BUG=628155
==========

to

==========
[css-grid] Use grid area position to determine overflowing items

Grid code keeps a list of items overflowing their grid areas so that it
could apply some painting optimizations to reduce the amount of grid items
to be painted. Even if the border box is completely contained by the
item's grid area, margins in the item could make it overflow its grid area
by altering the border box position.

The problem is not that the optimization won't work, it's worse because it will
make that item not visible as it won't be painted.

BUG=628155

Committed: https://crrev.com/a67debf9a9b78f60e7b10b5574ae4ceab49af31b
Cr-Commit-Position: refs/heads/master@{#420094}
==========

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

Patchset 2 (id:??) landed as
https://crrev.com/a67debf9a9b78f60e7b10b5574ae4ceab49af31b
Cr-Commit-Position: refs/heads/master@{#420094}