[CSS Grid Layout] Avoid mark grid as dirty always when removing a child

[CSS Grid Layout] Avoid mark grid as dirty always when removing a child

Current code always mark the grid as dirty in RenderGrid::removeChild().

If the grid auto flow property is different to none, the grid has to be
marked as dirty when a child removed. Just in case, the grid contains
auto-placed items, that might need to be re-positioned.

Otherwise, we can simple remove the child from the grid cells where it
is present and update the coordinates cache accordingly.

BUG=248151
TEST=fast/css-grid-layout/grid-item-removal-auto-placement-update.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170484

[Julien - ping for review, 2014-03-31 18:36:50]

lgtm

https://codereview.chromium.org/211053002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/211053002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:227: if (style()->gridAutoFlow() !=
AutoFlowNone) {
Note that if you don't have any grid item that are auto positioned you could
avoid dirtying the grid too. Probably worth a comment / FIXME but it seems to me
that it's a corner case that we shouldn't care about (for now).

https://codereview.chromium.org/211053002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:234: GridCoordinate coordinate =
m_gridItemCoordinate.get(childBox);
You could avoid the 2 hash lookup by using m_gridItemCoordinate.take(childBox)
(or find() but the code will be a bit less nice).

[Manuel Rego, 2014-03-31 19:59:38]

Thanks for the review.

On 2014/03/31 18:36:50, Julien Chaffraix - PST wrote:
>
https://codereview.chromium.org/211053002/diff/1/Source/core/rendering/Render...
> Source/core/rendering/RenderGrid.cpp:227: if (style()->gridAutoFlow() !=
> AutoFlowNone) {
> Note that if you don't have any grid item that are auto positioned you could
> avoid dirtying the grid too. Probably worth a comment / FIXME but it seems to
me
> that it's a corner case that we shouldn't care about (for now).

If you have set auto-flow property it's usually because you have
plans to use auto positioned items, so I think we can forget about
this particular case right now.

>
https://codereview.chromium.org/211053002/diff/1/Source/core/rendering/Render...
> Source/core/rendering/RenderGrid.cpp:234: GridCoordinate coordinate =
> m_gridItemCoordinate.get(childBox);
> You could avoid the 2 hash lookup by using m_gridItemCoordinate.take(childBox)
> (or find() but the code will be a bit less nice).

Yes, nice suggestion.

[Manuel Rego, 2014-03-31 19:59:43]

The CQ bit was checked by rego@igalia.com

[commit-bot: I haz the power, 2014-03-31 19:59:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rego@igalia.com/211053002/20001

[commit-bot: I haz the power, 2014-03-31 23:51:02]

Change committed as 170484