[css-grid] Fix crash removing positioned grid item

[css-grid] Fix crash removing positioned grid item

When we add or remove a positioned item we don't need to mark
the grid as dirty, because positioned items do not affect the layout
of the grid at all.

This was causing a crash when a positioned item was removed
after a layout. As after the positioned item was removed,
the method LayoutGrid::layoutBlock() was not called,
so when the grid was repainted we got a crash.

BUG=700383
TEST=fast/css-grid-layout/grid-crash-remove-positioned-item.html

Review-Url: https://codereview.chromium.org/2748983003
Cr-Commit-Position: refs/heads/master@{#457061}
Committed: https://chromium.googlesource.com/chromium/src/+/eb7634d5a91a495af9495164ffb2e5f1f91f7e09

[Manuel Rego, 2017-03-15 09:05:04]

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

[jfernandez, 2017-03-15 09:26:36]

lgtm

https://codereview.chromium.org/2748983003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-crash-remove-positioned-item-expected.txt
(right):

https://codereview.chromium.org/2748983003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-crash-remove-positioned-item-expected.txt:5:
item
Do we really need the "item" text ? isn't the test valid as well if the regular
grid item is empty ?

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

https://codereview.chromium.org/2748983003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:74: if
(!newChild->isOutOfFlowPositioned()) {
I think it'd be useful to add comment explaining why we don't need to mark the
grid as dirty in this case.

[svillar, 2017-03-15 10:03:54]

lgtm too

https://codereview.chromium.org/2748983003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-crash-remove-positioned-item.html
(right):

https://codereview.chromium.org/2748983003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-crash-remove-positioned-item.html:14:
document.getElementById("grid").removeChild(document.getElementById("abspositem"));
Don't we need a test for the case of adding an abspos item?

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

https://codereview.chromium.org/2748983003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:78: }
Better to early return

if (newChild->isOutOfFlowPositioned)
   return;

https://codereview.chromium.org/2748983003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:88: }
Ditto.

[Manuel Rego, 2017-03-15 10:59:26]

Thanks for the quick reviews!

https://codereview.chromium.org/2748983003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-crash-remove-positioned-item-expected.txt
(right):

https://codereview.chromium.org/2748983003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-crash-remove-positioned-item-expected.txt:5:
item
On 2017/03/15 09:26:36, jfernandez wrote:
> Do we really need the "item" text ? isn't the test valid as well if the
regular
> grid item is empty ?

Yeah, otherwise the repaint is not triggered, so it doesn't crash.
I've added a comment on the test about that.

https://codereview.chromium.org/2748983003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-crash-remove-positioned-item.html
(right):

https://codereview.chromium.org/2748983003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-crash-remove-positioned-item.html:14:
document.getElementById("grid").removeChild(document.getElementById("abspositem"));
On 2017/03/15 10:03:54, svillar wrote:
> Don't we need a test for the case of adding an abspos item?

Actually that part was not crashing before,
but as I'm modifying the code, I'll add and remove the child
in the test.

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

https://codereview.chromium.org/2748983003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:74: if
(!newChild->isOutOfFlowPositioned()) {
On 2017/03/15 09:26:36, jfernandez wrote:
> I think it'd be useful to add comment explaining why we don't need to mark the
> grid as dirty in this case.

Done.

https://codereview.chromium.org/2748983003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:78: }
On 2017/03/15 10:03:54, svillar wrote:
> Better to early return
> 
> if (newChild->isOutOfFlowPositioned)
>    return;

Acknowledged.

https://codereview.chromium.org/2748983003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:88: }
On 2017/03/15 10:03:54, svillar wrote:
> Ditto.

Acknowledged.

[Manuel Rego, 2017-03-15 10:59:39]

The CQ bit was checked by rego@igalia.com

[Manuel Rego, 2017-03-15 10:59:40]

The patchset sent to the CQ was uploaded after l-g-t-m from svillar@igalia.com,
jfernandez@igalia.com
Link to the patchset: https://codereview.chromium.org/2748983003/#ps20001
(title: "Early return, improved test and more comments")

[commit-bot: I haz the power, 2017-03-15 10:59:55]

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, 2017-03-15 12:45:58]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1489575579893000,
"parent_rev": "5c7a83e9da26f8621d4b17196c22c75fb1dca45f", "commit_rev":
"eb7634d5a91a495af9495164ffb2e5f1f91f7e09"}

[commit-bot: I haz the power, 2017-03-15 12:46:42]

Description was changed from

==========
[css-grid] Fix crash removing positioned grid item

When we add or remove a positioned item we don't need to mark
the grid as dirty, because positioned items do not affect the layout
of the grid at all.

This was causing a crash when a positioned item was removed
after a layout. As after the positioned item was removed,
the method LayoutGrid::layoutBlock() was not called,
so when the grid was repainted we got a crash.

BUG=700383
TEST=fast/css-grid-layout/grid-crash-remove-positioned-item.html
==========

to

==========
[css-grid] Fix crash removing positioned grid item

When we add or remove a positioned item we don't need to mark
the grid as dirty, because positioned items do not affect the layout
of the grid at all.

This was causing a crash when a positioned item was removed
after a layout. As after the positioned item was removed,
the method LayoutGrid::layoutBlock() was not called,
so when the grid was repainted we got a crash.

BUG=700383
TEST=fast/css-grid-layout/grid-crash-remove-positioned-item.html

Review-Url: https://codereview.chromium.org/2748983003
Cr-Commit-Position: refs/heads/master@{#457061}
Committed:
https://chromium.googlesource.com/chromium/src/+/eb7634d5a91a495af9495164ffb2...
==========

[commit-bot: I haz the power, 2017-03-15 12:46:43]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/eb7634d5a91a495af9495164ffb2...