[CSS Grid Layout] Ignore positioned block item added after inline

[CSS Grid Layout] Ignore positioned block item added after inline

Right now if you add a positioned block item after an inline item, the
positioned block item is inserted inside the anonymous block wrapping
the inline item.

This was causing an assert in RenderGrid::addChildToIndexesMap() while
navigating the sibling boxes, as its previous sibling is an inline.

As the positioned block is now not direct child of the grid, we can
ignore it, as it's not considered a grid item. The grid item would
be the anonymous block. So the proper check is added in
RenderGrid::addChild to do it.

BUG=401463
TEST=fast/css-grid-layout/grid-add-positioned-block-item-after-inline-item.html

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

[Manuel Rego, 2014-09-03 16:49:28]

Ping reviewers. Thanks!

[svillar, 2014-09-04 07:51:26]

On 2014/09/03 16:49:28, Manuel Rego wrote:
> Ping reviewers. Thanks!

Rego do we need this after https://codereview.chromium.org/488163002/ ?

[svillar, 2014-09-04 09:36:50]

On 2014/09/04 07:51:26, svillar wrote:
> On 2014/09/03 16:49:28, Manuel Rego wrote:
> > Ping reviewers. Thanks!
> 
> Rego do we need this after https://codereview.chromium.org/488163002/ ?

It isn't failing for me with current master. I guess the problem has been fixed.

[Manuel Rego, 2014-09-04 15:34:52]

On 2014/09/04 09:36:50, svillar wrote:
> On 2014/09/04 07:51:26, svillar wrote:
> > Rego do we need this after https://codereview.chromium.org/488163002/ ?
> 
> It isn't failing for me with current master. I guess the problem has been
fixed.

The test was not failing in current master. But I've uploaded a new one that is
failing.

The new one is forcing a layout and then adding a positioned item. As the only
item in the grid is a text and wrapped by an anonymous item, the anonymous
item is reused to insert the positioned item and the crash appears.
Check the bug for more information.

So we still need this patch to avoid this kind of issues.

[svillar, 2014-09-08 08:59:16]

On 2014/09/04 15:34:52, Manuel Rego wrote:
> On 2014/09/04 09:36:50, svillar wrote:
> > On 2014/09/04 07:51:26, svillar wrote:
> > > Rego do we need this after https://codereview.chromium.org/488163002/ ?
> > 
> > It isn't failing for me with current master. I guess the problem has been
> fixed.
> 
> The test was not failing in current master. But I've uploaded a new one that
is
> failing.
> 
> The new one is forcing a layout and then adding a positioned item. As the only
> item in the grid is a text and wrapped by an anonymous item, the anonymous
> item is reused to insert the positioned item and the crash appears.
> Check the bug for more information.
> 
> So we still need this patch to avoid this kind of issues.

IMO the change makes sense.

It's pretty unfortunate how the ::addChild() method is looking though. It's
getting overly complicated, full of special cases. At some point we should
reevaluate if this is the direction we want to follow.

[Julien - ping for review, 2014-09-08 23:13:17]

On 2014/09/08 at 08:59:16, svillar wrote:
> On 2014/09/04 15:34:52, Manuel Rego wrote:
> > On 2014/09/04 09:36:50, svillar wrote:
> > > On 2014/09/04 07:51:26, svillar wrote:
> > > > Rego do we need this after https://codereview.chromium.org/488163002/ ?
> > > 
> > > It isn't failing for me with current master. I guess the problem has been
> > fixed.
> > 
> > The test was not failing in current master. But I've uploaded a new one that
is
> > failing.
> > 
> > The new one is forcing a layout and then adding a positioned item. As the
only
> > item in the grid is a text and wrapped by an anonymous item, the anonymous
> > item is reused to insert the positioned item and the crash appears.
> > Check the bug for more information.
> > 
> > So we still need this patch to avoid this kind of issues.
> 
> IMO the change makes sense.
> 
> It's pretty unfortunate how the ::addChild() method is looking though. It's
getting overly complicated, full of special cases. At some point we should
reevaluate if this is the direction we want to follow.

100% agreed, however it's a more general direction I would say. Currently
addChild() will generate the anonymous wrappers so we could end up inserting
*another* renderer under the parent than the one passed in as a parameter.

[Julien - ping for review, 2014-09-08 23:13:51]

lgtm

https://codereview.chromium.org/450783002/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/450783002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:222: // If the new child has been inserted
inside an existent anonymous block, we can simple ignore it as the anonymous
s/simple/simply/

[Manuel Rego, 2014-09-09 10:03:58]

https://codereview.chromium.org/450783002/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/450783002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:222: // If the new child has been inserted
inside an existent anonymous block, we can simple ignore it as the anonymous
On 2014/09/08 23:13:51, Julien Chaffraix - PST wrote:
> s/simple/simply/

Done.

[Manuel Rego, 2014-09-09 10:13:40]

The CQ bit was checked by rego@igalia.com

[commit-bot: I haz the power, 2014-09-09 10:14:43]

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

[commit-bot: I haz the power, 2014-09-09 11:17:59]

Committed patchset #3 (id:40001) as 181633