Force unitless data-expected attributes

Force unitless data-expected attributes

When playing around with fast/css/calc-rounding.html
I found that using px was making the attribute value
irrelevant, because it resulted in NaN and skipped the
actual check. So also report an error when the attribute
value results in NaN and remove existing px usage for
data-expected attributes in the layout tests.

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

[rwlbuis, 2015-01-30 14:14:47]

rob.buis@samsung.com changed reviewers:
+ robhogan@chromium.org, robhogan@gmail.com

[rwlbuis, 2015-01-30 14:14:48]

@robhogan IIRC
fast/replaced/percentage-height-when-height-specified-by-top-bottom.html was
your test.
I made this CL because I found out that using px in the data-expected attributes
seem to make it invalid, or at least it does not seem to matter when you change
the value before the px. I stumbled on this when I played around with my test
fast/css/calc-rounding.html.
Since your test is the only one failing when px is removed, it seems it is the
only one with a different actual result than the expectations. So I am asking
you to check whether this means the original problem is not fixed, or the new
result is actually correct and the test can just be updated to expect that.
Thanks!

[rwlbuis, 2015-01-30 16:11:55]

On 2015/01/30 14:14:48, rwlbuis wrote:
> @robhogan IIRC
> fast/replaced/percentage-height-when-height-specified-by-top-bottom.html was
> your test.
> I made this CL because I found out that using px in the data-expected
attributes
> seem to make it invalid, or at least it does not seem to matter when you
change
> the value before the px. I stumbled on this when I played around with my test
> fast/css/calc-rounding.html.
> Since your test is the only one failing when px is removed, it seems it is the
> only one with a different actual result than the expectations. So I am asking
> you to check whether this means the original problem is not fixed, or the new
> result is actually correct and the test can just be updated to expect that.
> Thanks!

@robhogan actually the test shows up fine visually. I suspect it is a timing
problem so I now changed the test to call checkLayout in the body onload, let's
see if that makes it green.

[rwlbuis, 2015-01-30 18:20:09]

rob.buis@samsung.com changed reviewers:
+ eae@chromium.org, jchaffraix@chromium.org

[rwlbuis, 2015-01-30 18:20:09]

PTAL

[Julien - ping for review, 2015-01-31 07:12:55]

This change will make the tests relevant again but it doesn't cure the
underlying issue.

The fact that we have a silent error is bad and we should just fix it (either by
making it explicit or by fixing check-layout.js to accept such input), else
we're doomed to repeat the same mistakes.

https://codereview.chromium.org/888743002/diff/20001/LayoutTests/fast/css-gri...
File LayoutTests/fast/css-grid-layout/grid-item-named-grid-line-resolution.html
(right):

https://codereview.chromium.org/888743002/diff/20001/LayoutTests/fast/css-gri...
LayoutTests/fast/css-grid-layout/grid-item-named-grid-line-resolution.html:192:
<div class="sizedToGridArea" style="grid-column: 5 / span C;"
data-offset-x="100px" data-expected-width="50"></div>
Isn't data-offset-* plagued with the same issue?

https://codereview.chromium.org/888743002/diff/20001/LayoutTests/fast/replace...
File
LayoutTests/fast/replaced/percentage-height-when-height-specified-by-top-bottom.html
(left):

https://codereview.chromium.org/888743002/diff/20001/LayoutTests/fast/replace...
LayoutTests/fast/replaced/percentage-height-when-height-specified-by-top-bottom.html:19:
</script>
I don't think that makes a difference in the test.

[rwlbuis, 2015-02-02 01:56:34]

Patchset #3 (id:40001) has been deleted

[rwlbuis, 2015-02-02 12:03:11]

On 2015/01/31 07:12:55, Julien Chaffraix - CET wrote:
> This change will make the tests relevant again but it doesn't cure the
> underlying issue.
> 
> The fact that we have a silent error is bad and we should just fix it (either
by
> making it explicit or by fixing check-layout.js to accept such input), else
> we're doomed to repeat the same mistakes.

You are right, I now test for NaN and if so report an error.

>
https://codereview.chromium.org/888743002/diff/20001/LayoutTests/fast/css-gri...
> File
LayoutTests/fast/css-grid-layout/grid-item-named-grid-line-resolution.html
> (right):
> 
>
https://codereview.chromium.org/888743002/diff/20001/LayoutTests/fast/css-gri...
>
LayoutTests/fast/css-grid-layout/grid-item-named-grid-line-resolution.html:192:
> <div class="sizedToGridArea" style="grid-column: 5 / span C;"
> data-offset-x="100px" data-expected-width="50"></div>
> Isn't data-offset-* plagued with the same issue?

Right, fixed in latest patch set.

>
https://codereview.chromium.org/888743002/diff/20001/LayoutTests/fast/replace...
> File
>
LayoutTests/fast/replaced/percentage-height-when-height-specified-by-top-bottom.html
> (left):
> 
>
https://codereview.chromium.org/888743002/diff/20001/LayoutTests/fast/replace...
>
LayoutTests/fast/replaced/percentage-height-when-height-specified-by-top-bottom.html:19:
> </script>
> I don't think that makes a difference in the test.

I need to check, it failed on some bots without this change.

[rwlbuis, 2015-02-02 17:07:02]

On 2015/01/31 07:12:55, Julien Chaffraix - CET wrote:
>
https://codereview.chromium.org/888743002/diff/20001/LayoutTests/fast/replace...
> File
>
LayoutTests/fast/replaced/percentage-height-when-height-specified-by-top-bottom.html
> (left):
> 
>
https://codereview.chromium.org/888743002/diff/20001/LayoutTests/fast/replace...
>
LayoutTests/fast/replaced/percentage-height-when-height-specified-by-top-bottom.html:19:
> </script>
> I don't think that makes a difference in the test.

See patch set 4 and 5, it does seem needed. This was hidden before because 'px'
was used.

[Julien - ping for review, 2015-02-03 02:42:53]

lgtm

https://codereview.chromium.org/888743002/diff/100001/LayoutTests/fast/css-gr...
File LayoutTests/fast/css-grid-layout/grid-align.html (right):

https://codereview.chromium.org/888743002/diff/100001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/grid-align.html:139: <div class="cell
alignSelfSelfEnd secondRowSecondColumn" data-offset-x="80" data-offset-y="360"
data-expected-width="20" data-expected-height="40"></div>
Nice catch!

https://codereview.chromium.org/888743002/diff/100001/LayoutTests/resources/c...
File LayoutTests/resources/check-layout.js (right):

https://codereview.chromium.org/888743002/diff/100001/LayoutTests/resources/c...
LayoutTests/resources/check-layout.js:98: }
Should we have testing for that in the harness? (I don't think we have any kind
of testing for check-layout.js but I would be supportive of adding a lot!

[rwlbuis, 2015-02-03 16:58:49]

On 2015/02/03 02:42:53, Julien Chaffraix - AEST wrote:
>
https://codereview.chromium.org/888743002/diff/100001/LayoutTests/resources/c...
> File LayoutTests/resources/check-layout.js (right):
> 
>
https://codereview.chromium.org/888743002/diff/100001/LayoutTests/resources/c...
> LayoutTests/resources/check-layout.js:98: }
> Should we have testing for that in the harness? (I don't think we have any
kind
> of testing for check-layout.js but I would be supportive of adding a lot!

I can do a follow-up patch for that if you tell me how to test. Is it simply
another layout
test where all the data-expected attributes use "px"?

[rwlbuis, 2015-02-03 16:59:37]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2015-02-03 17:00:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/888743002/100001

[commit-bot: I haz the power, 2015-02-03 18:17:11]

Committed patchset #5 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189429

[Julien - ping for review, 2015-02-04 20:28:04]

On 2015/02/03 at 16:58:49, rob.buis wrote:
> On 2015/02/03 02:42:53, Julien Chaffraix - AEST wrote:
> >
https://codereview.chromium.org/888743002/diff/100001/LayoutTests/resources/c...
> > File LayoutTests/resources/check-layout.js (right):
> > 
> >
https://codereview.chromium.org/888743002/diff/100001/LayoutTests/resources/c...
> > LayoutTests/resources/check-layout.js:98: }
> > Should we have testing for that in the harness? (I don't think we have any
kind
> > of testing for check-layout.js but I would be supportive of adding a lot!
> 
> I can do a follow-up patch for that if you tell me how to test. Is it simply
another layout
> test where all the data-expected attributes use "px"?

We have some JS testing framework (qUnit) that we can use to make sure we
correctly handle these cases.