[CSS Grid Layout] Add layout test for anonymous grid items

[CSS Grid Layout] Add layout test for anonymous grid items

Add a layout test to prevent regressions supporting anonymous grid
items.

TEST=fast/css-grid-layout/anonymous-grid-items.html

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

[Julien - ping for review, 2014-03-01 01:04:42]

I must be missing something but I have a hard time understanding what type of
regression(s) this test is intended to protect us from.

Also note that you don't check for multiple lines being put into the same
anonymous grid item as you only have a single line in your test.

[Manuel Rego, 2014-03-03 09:21:19]

On 2014/03/01 01:04:42, Julien Chaffraix - PST wrote:
> I must be missing something but I have a hard time understanding what type of
> regression(s) this test is intended to protect us from.

In case someone tries to access to the DOM node of a grid item, this test will
prevent him to do it and will help to not forget about the anonymous case.

For example, in flexbox we have a test fast/flexbox/crash-anonymous-box.html
checking it (the related bug was
https://bugs.webkit.org/show_bug.cgi?id=112958).

> Also note that you don't check for multiple lines being put into the same
> anonymous grid item as you only have a single line in your test.

Added more cases with several lines and <BR>.

[Julien - ping for review, 2014-03-05 16:14:02]

lgtm, I don't think we have anything to cover anonymous grid items so this adds
a bit of coverage. I don't think it's much but I won't stop a good willed effort
to increase our coverage of grid.

The test case you're linking to is meant to check a crash so if we follow the
same example, we could have a text-only test.

https://codereview.chromium.org/181183005/diff/10001/LayoutTests/fast/css-gri...
File LayoutTests/fast/css-grid-layout/anonymous-grid-items-expected.html
(right):

https://codereview.chromium.org/181183005/diff/10001/LayoutTests/fast/css-gri...
LayoutTests/fast/css-grid-layout/anonymous-grid-items-expected.html:5: <style
type="text/css">
Unneeded type attribute.

https://codereview.chromium.org/181183005/diff/10001/LayoutTests/fast/css-gri...
LayoutTests/fast/css-grid-layout/anonymous-grid-items-expected.html:22:
<div>anonymous item anonymous item anonymous item</div>
Does that really match the test case? (it seems to be missing some new lines).

https://codereview.chromium.org/181183005/diff/10001/LayoutTests/fast/css-gri...
File LayoutTests/fast/css-grid-layout/anonymous-grid-items.html (right):

https://codereview.chromium.org/181183005/diff/10001/LayoutTests/fast/css-gri...
LayoutTests/fast/css-grid-layout/anonymous-grid-items.html:26: 
Those are a single RenderText as it's a single text-run AFAICT.

If you want to cover several renderers folded into the same anonymous grid item,
you can do it as follows:
<i>anonymous item</i>
<i>another anonymous items</i>

(basically use several inline elements)

[Manuel Rego, 2014-03-06 09:05:56]

On 2014/03/05 16:14:02, Julien Chaffraix - PST wrote:
> lgtm, I don't think we have anything to cover anonymous grid items so this
adds
> a bit of coverage. I don't think it's much but I won't stop a good willed
effort
> to increase our coverage of grid.

I've tried to access DOM node from the painting methods and there are not
tests checking this.

> The test case you're linking to is meant to check a crash so if we follow the
> same example, we could have a text-only test.

I agree that something like that could be enough. I'm not sure about a
text-only test, because of in that case painting code is not checked.

I prefer a simple ref-test checking that anonymous items don't crash.

>
https://codereview.chromium.org/181183005/diff/10001/LayoutTests/fast/css-gri...
> File LayoutTests/fast/css-grid-layout/anonymous-grid-items-expected.html
> (right):
> 
>
https://codereview.chromium.org/181183005/diff/10001/LayoutTests/fast/css-gri...
> LayoutTests/fast/css-grid-layout/anonymous-grid-items-expected.html:5: <style
> type="text/css">
> Unneeded type attribute.

Ok.

>
https://codereview.chromium.org/181183005/diff/10001/LayoutTests/fast/css-gri...
> LayoutTests/fast/css-grid-layout/anonymous-grid-items-expected.html:22:
> <div>anonymous item anonymous item anonymous item</div>
> Does that really match the test case? (it seems to be missing some new lines).

As it was a ref-test the final result was the same, so it was passing, anyway
I'm going to remove all these cases and left only 1 simple anonymous grid item.

>
https://codereview.chromium.org/181183005/diff/10001/LayoutTests/fast/css-gri...
> File LayoutTests/fast/css-grid-layout/anonymous-grid-items.html (right):
> 
>
https://codereview.chromium.org/181183005/diff/10001/LayoutTests/fast/css-gri...
> LayoutTests/fast/css-grid-layout/anonymous-grid-items.html:26: 
> Those are a single RenderText as it's a single text-run AFAICT.
> 
> If you want to cover several renderers folded into the same anonymous grid
item,
> you can do it as follows:
> <i>anonymous item</i>
> <i>another anonymous items</i>
> 
> (basically use several inline elements)

Yes, you're right. However, I think that all these cases are already checked
by other tests, so we can keep it simple for grid.

Thanks for the review, I've just uploaded a new patch for a ref-test with a
single
anonymous grid item.

[Julien - ping for review, 2014-03-06 17:44:02]

lgtm

[commit-bot: I haz the power, 2014-03-06 17:45:17]

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

[commit-bot: I haz the power, 2014-03-06 18:03:07]

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

[commit-bot: I haz the power, 2014-03-06 18:03:08]

Try jobs failed on following builders: win_blink_rel

[Manuel Rego, 2014-03-06 20:10:29]

The CQ bit was checked by rego@igalia.com

[commit-bot: I haz the power, 2014-03-06 20:12:49]

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

[commit-bot: I haz the power, 2014-03-06 20:28:11]

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

[commit-bot: I haz the power, 2014-03-06 20:28:12]

Try jobs failed on following builders: win_blink_rel

[Manuel Rego, 2014-03-06 22:14:24]

The CQ bit was checked by rego@igalia.com

[commit-bot: I haz the power, 2014-03-06 22:15:09]

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

[commit-bot: I haz the power, 2014-03-06 22:15:28]

Change committed as 168680