CSS Transforms 2: Web Platform Tests for translate,rotate,scale parsing

CSS Transforms 2: Web Platform Tests for translate,rotate,scale parsing

These are the first Web Platform Tests for the Individual Transform
Properties.

Spec:
https://drafts.csswg.org/css-transforms-2/#individual-transforms

BUG=722696
Review-Url: https://codereview.chromium.org/2882153004
Cr-Commit-Position: refs/heads/master@{#473775}
Committed: https://chromium.googlesource.com/chromium/src/+/c2dd1a0ce5ba82339906ec3f43bb0c34f1b1743d

[Eric Willigers, 2017-05-16 04:27:52]

ericwilligers@chromium.org changed reviewers:
+ suzyh@chromium.org

[Eric Willigers, 2017-05-16 04:38:08]

The CQ bit was checked by ericwilligers@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-16 04:38:36]

Dry run: 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-05-16 05:27:58]

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

[commit-bot: I haz the power, 2017-05-16 05:27:58]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[suzyh_UTC10 (ex-contributor), 2017-05-19 05:41:01]

On 2017/05/16 at 04:27:52, ericwilligers wrote:
> 

Hi Eric,

I'm sorry for the delay in reviewing this CL. I've given it a first pass and am
in the progress of reading through the WPT contribution guidelines in order to
inform my review. I hope to be able to send my review comments to you on Monday.

[suzyh_UTC10 (ex-contributor), 2017-05-22 01:11:45]

https://codereview.chromium.org/2882153004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/css/css-transforms-2/resources/testcommon.js
(right):

https://codereview.chromium.org/2882153004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/css/css-transforms-2/resources/testcommon.js:1:
'use strict';
Consider naming this file something like testcommon-parsing.js or
parsing-testcommon.js and/or rename the functions to make it obvious that they
are specifically related to parsing.

https://codereview.chromium.org/2882153004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/css/css-transforms-2/resources/testcommon.js:3:
function assert_valid_value(property, value) {
I find it slightly surprising that a function named 'assert_...' actually
creates tests, where the testharness format is of tests containing calls to
assert_* functions. Perhaps test_valid_value or test_valid_value_for_parsing?

https://codereview.chromium.org/2882153004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/css/css-transforms-2/resources/testcommon.js:10:
}, "e.style['" + property + "'] = " + stringifiedValue + " should set the
value");
Is 'sets the value' exactly synonymous with 'parses as a valid value', or is
this test doing something subtly different from testing parsing?

https://codereview.chromium.org/2882153004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/css/css-transforms-2/resources/testcommon.js:16:
}, "Serialization should round-trip after setting e.style['" + property + "'] =
" + stringifiedValue);
Are we likely to want to test that X is a valid value, but it serializes
differently, so would come out differently here?

https://codereview.chromium.org/2882153004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/css/css-transforms-2/resources/testcommon.js:26:
}, "e.style['" + property + "'] = " + stringifiedValue + " should not set the
value");
I find this test description (and to a lesser extent, the equivalent one under
assert_valid_value) difficult to understand. That is, if I accidentally made
this test fail, I'm not sure I'd know what to do as a result of reading this
description. I think my main concern is that I would come across this error and
think "should not set the value of what?"

https://codereview.chromium.org/2882153004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/css/css-transforms-2/rotate-parsing-invalid.html
(right):

https://codereview.chromium.org/2882153004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/css/css-transforms-2/rotate-parsing-invalid.html:1:
<!DOCTYPE html>
Optional: Consider adding some structure to this test suite, putting these files
in a 'parsing' subdirectory, or some other structure that makes sense relative
to the spec.
http://web-platform-tests.org/writing-tests/general-guidelines.html#file-path...

https://codereview.chromium.org/2882153004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/css/css-transforms-2/rotate-parsing-invalid.html:6:
<link rel="author" title="Eric Willigers"
href="mailto:ericwilligers@google.com">
Use @chromium instead?

https://codereview.chromium.org/2882153004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/css/css-transforms-2/rotate-parsing-invalid.html:8:
<meta name="assert" content="rotate value: none | <number>{3}? <angle>">
Consider making this assertion a full sentence (and likewise in other files).
See guidelines at
http://web-platform-tests.org/writing-tests/css-metadata.html#test-assertions

[suzyh_UTC10 (ex-contributor), 2017-05-22 01:41:16]

https://codereview.chromium.org/2882153004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/css/css-transforms-2/rotate-parsing-invalid.html
(right):

https://codereview.chromium.org/2882153004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/css/css-transforms-2/rotate-parsing-invalid.html:7:
<link rel="help"
href="https://drafts.csswg.org/css-transforms-2/#propdef-rotate">
Oh yes, is the editor's draft the correct spec link to include here?

[Eric Willigers, 2017-05-22 07:12:31]

Patchset #3 (id:40001) has been deleted

[Eric Willigers, 2017-05-22 07:13:40]

https://codereview.chromium.org/2882153004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/css/css-transforms-2/resources/testcommon.js
(right):

https://codereview.chromium.org/2882153004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/css/css-transforms-2/resources/testcommon.js:1:
'use strict';
On 2017/05/22 01:11:45, suzyh_UTC10 wrote:
> Consider naming this file something like testcommon-parsing.js or
> parsing-testcommon.js and/or rename the functions to make it obvious that they
> are specifically related to parsing.

Done.

https://codereview.chromium.org/2882153004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/css/css-transforms-2/resources/testcommon.js:3:
function assert_valid_value(property, value) {
On 2017/05/22 01:11:45, suzyh_UTC10 wrote:
> I find it slightly surprising that a function named 'assert_...' actually
> creates tests, where the testharness format is of tests containing calls to
> assert_* functions. Perhaps test_valid_value or test_valid_value_for_parsing?

Done.

I also came across another option, but I don't find it any clearer:
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/...
uses the terminology generate_tests.

https://codereview.chromium.org/2882153004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/css/css-transforms-2/resources/testcommon.js:10:
}, "e.style['" + property + "'] = " + stringifiedValue + " should set the
value");
On 2017/05/22 01:11:44, suzyh_UTC10 wrote:
> Is 'sets the value' exactly synonymous with 'parses as a valid value', or is
> this test doing something subtly different from testing parsing?

I have changed the function to accept an optional third argument: the canonical
serialized form.

Two tests using the third argument have been appended to
translate-parsing-valid.html

https://codereview.chromium.org/2882153004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/css/css-transforms-2/rotate-parsing-invalid.html
(right):

https://codereview.chromium.org/2882153004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/css/css-transforms-2/rotate-parsing-invalid.html:1:
<!DOCTYPE html>
On 2017/05/22 01:11:45, suzyh_UTC10 wrote:
> Optional: Consider adding some structure to this test suite, putting these
files
> in a 'parsing' subdirectory, or some other structure that makes sense relative
> to the spec.
>
http://web-platform-tests.org/writing-tests/general-guidelines.html#file-path...

Done.

https://codereview.chromium.org/2882153004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/css/css-transforms-2/rotate-parsing-invalid.html:6:
<link rel="author" title="Eric Willigers"
href="mailto:ericwilligers@google.com">
On 2017/05/22 01:11:45, suzyh_UTC10 wrote:
> Use @chromium instead?

Done.

https://codereview.chromium.org/2882153004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/css/css-transforms-2/rotate-parsing-invalid.html:7:
<link rel="help"
href="https://drafts.csswg.org/css-transforms-2/#propdef-rotate">
On 2017/05/22 01:41:16, suzyh_UTC10 wrote:
> Oh yes, is the editor's draft the correct spec link to include here?

It is. There are no other published versions of css-transforms-2.

With CSS Transforms Module Level 1, we would have a choice, but the new
properties are not present in Level 1.

https://codereview.chromium.org/2882153004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/css/css-transforms-2/rotate-parsing-invalid.html:8:
<meta name="assert" content="rotate value: none | <number>{3}? <angle>">
On 2017/05/22 01:11:45, suzyh_UTC10 wrote:
> Consider making this assertion a full sentence (and likewise in other files).
> See guidelines at
> http://web-platform-tests.org/writing-tests/css-metadata.html#test-assertions

Done.

[suzyh_UTC10 (ex-contributor), 2017-05-22 23:28:18]

lgtm

https://codereview.chromium.org/2882153004/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/css/css-transforms-2/parsing/resources/parsing-testcommon.js
(right):

https://codereview.chromium.org/2882153004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/css/css-transforms-2/parsing/resources/parsing-testcommon.js:4:
if (arguments.length < 3)
Are there any cases for css-transforms-2 where the third argument would be
provided, or do they all always round-trip?

[suzyh_UTC10 (ex-contributor), 2017-05-22 23:29:01]

On 2017/05/22 at 23:28:18, suzyh_UTC10 wrote:
> lgtm
> 
>
https://codereview.chromium.org/2882153004/diff/60001/third_party/WebKit/Layo...
> File
third_party/WebKit/LayoutTests/external/wpt/css/css-transforms-2/parsing/resources/parsing-testcommon.js
(right):
> 
>
https://codereview.chromium.org/2882153004/diff/60001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/external/wpt/css/css-transforms-2/parsing/resources/parsing-testcommon.js:4:
if (arguments.length < 3)
> Are there any cases for css-transforms-2 where the third argument would be
provided, or do they all always round-trip?

Gah, sorry, I thought I'd deleted that question after realising that there were
some. Disregard.

[Eric Willigers, 2017-05-23 01:15:28]

The CQ bit was checked by ericwilligers@chromium.org

[commit-bot: I haz the power, 2017-05-23 01:16:04]

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-05-23 01:28:13]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495502128750030,
"parent_rev": "34b5e4d8973b34420533e465f0124504349ac72e", "commit_rev":
"c2dd1a0ce5ba82339906ec3f43bb0c34f1b1743d"}

[commit-bot: I haz the power, 2017-05-23 01:28:23]

Description was changed from

==========
CSS Transforms 2: Web Platform Tests for translate,rotate,scale parsing

These are the first Web Platform Tests for the Individual Transform
Properties.

Spec:
https://drafts.csswg.org/css-transforms-2/#individual-transforms

BUG=722696
==========

to

==========
CSS Transforms 2: Web Platform Tests for translate,rotate,scale parsing

These are the first Web Platform Tests for the Individual Transform
Properties.

Spec:
https://drafts.csswg.org/css-transforms-2/#individual-transforms

BUG=722696

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

[commit-bot: I haz the power, 2017-05-23 01:28:24]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/c2dd1a0ce5ba82339906ec3f43bb...