Add common test configs for properties that take lengths

Add common test configs for properties that take lengths.

This is a follow-up CL to https://codereview.chromium.org/2371673002, to make testing of length-accepting properties more complete.

To do this, I added "Length" as a typedom_types for each property in CSSProperties.in
While I was at it, I also added the valid keywords for those properties.

BUG=545318
Review-Url: https://codereview.chromium.org/2583063002
Cr-Commit-Position: refs/heads/master@{#455687}
Committed: https://chromium.googlesource.com/chromium/src/+/4d77a388ceb8b2742c2ee4f0d2c8cc09a0aab4c6

[meade_UTC10, 2017-01-05 06:43:04]

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

[commit-bot: I haz the power, 2017-01-05 06:43:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[meade_UTC10, 2017-01-05 06:47:10]

Description was changed from

==========
Add common test configs for properties that take lengths

BUG=545318
==========

to

==========
Add common test configs for properties that take lengths.

This is a follow-up CL to https://codereview.chromium.org/2371673002, to make
testing of length-accepting properties more complete.

To do this, I added "Length" as a typedom_types for each property in
CSSProperties.in
While I was at it, I also added the valid keywords for those properties.

BUG=545318
==========

[meade_UTC10, 2017-01-05 06:47:45]

meade@chromium.org changed reviewers:
+ sashab@chromium.org

[meade_UTC10, 2017-01-05 06:47:46]

This is a follow-up CL to https://codereview.chromium.org/2371673002, where I
add tests for the rest of the properties that take lengths. PTAL and let me know
if you have some thoughts on my way of pulling out the common logic - I think
this is OK, but there might be something better (or at least better naming)?

Thanks!

[sashab, 2017-01-05 22:09:31]

Not sure if I like the idea of a 'common configs' file, that sounds like a utils
file which usually spells bad news.

Maybe you could turn your method into more of an API, and put the common configs
in the API, e.g.

c = new Config('foo-prop', template='length')

or

c = new Config('foo-prop', ConfigFromTemplate('length'))

or

c = config_templates.length.copy()
c.foo = ...

Have a think :)

https://codereview.chromium.org/2583063002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/typedcssom/inlinestyle/properties/common-configs.js
(right):

https://codereview.chromium.org/2583063002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/typedcssom/inlinestyle/properties/common-configs.js:2:
* @fileoverview Generates config objects that are common between multiple
What is @fileoverview? Do we use this anywhere else

https://codereview.chromium.org/2583063002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/typedcssom/inlinestyle/properties/common-configs.js:6:
function lengthConfig(propertyName) {
Maybe createConfigForLengthProperty

https://codereview.chromium.org/2583063002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/typedcssom/inlinestyle/properties/width.html
(right):

https://codereview.chromium.org/2583063002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/typedcssom/inlinestyle/properties/width.html:8:
let config = lengthPercentConfig('width');
To help with naming, maybe try think about what you want this file to look like,
e.g.:

let config = new Config('width')
config.type = 'length'
config.validKeywords = ['a', 'b', 'c']
runTypedOMTests(config)

What are you going for? :)

[meade_UTC10, 2017-01-16 04:26:15]

On 2017/01/05 22:09:31, sashab wrote:
> Not sure if I like the idea of a 'common configs' file, that sounds like a
utils
> file which usually spells bad news.
> 
> Maybe you could turn your method into more of an API, and put the common
configs
> in the API, e.g.
> 
> c = new Config('foo-prop', template='length')
> 
> or
> 
> c = new Config('foo-prop', ConfigFromTemplate('length'))
> 
> or
> 
> c = config_templates.length.copy()
> c.foo = ...
> 
> Have a think :)
> 
>
https://codereview.chromium.org/2583063002/diff/80001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/typedcssom/inlinestyle/properties/common-configs.js
> (right):
> 
>
https://codereview.chromium.org/2583063002/diff/80001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/typedcssom/inlinestyle/properties/common-configs.js:2:
> * @fileoverview Generates config objects that are common between multiple
> What is @fileoverview? Do we use this anywhere else
> 
>
https://codereview.chromium.org/2583063002/diff/80001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/typedcssom/inlinestyle/properties/common-configs.js:6:
> function lengthConfig(propertyName) {
> Maybe createConfigForLengthProperty
> 
>
https://codereview.chromium.org/2583063002/diff/80001/third_party/WebKit/Layo...
> File
third_party/WebKit/LayoutTests/typedcssom/inlinestyle/properties/width.html
> (right):
> 
>
https://codereview.chromium.org/2583063002/diff/80001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/typedcssom/inlinestyle/properties/width.html:8:
> let config = lengthPercentConfig('width');
> To help with naming, maybe try think about what you want this file to look
like,
> e.g.:
> 
> let config = new Config('width')
> config.type = 'length'
> config.validKeywords = ['a', 'b', 'c']
> runTypedOMTests(config)
> 
> What are you going for? :)

Hmm, I don't think new Config gets us much, as I expect the majority of configs
to look more similar to animation-direction (which only supports keywords):
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/typedcsso...

But I changed it to config-templates.js, and added a lengthConfig and a
lengthPercentConfig. This makes each file look like this:

let config = Object.assign({}, config_templates.lengthPercentConfig);
config['property'] = 'left';
config['validKeywords'] = ['auto'];
runInlineStylePropertyMapTests(config);


The only trouble is copying looks kinda weird in js... but unless the template
is used more than once in a single test file, it might be ok to just write

let config = config_templates.lengthPercentConfig;
config['property'] = 'left';
config['validKeywords'] = ['auto'];
runInlineStylePropertyMapTests(config);


WDYT?

[meade_UTC10, 2017-01-16 04:28:08]

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

[commit-bot: I haz the power, 2017-01-16 04:28:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[meade_UTC10, 2017-01-16 04:30:47]

https://codereview.chromium.org/2583063002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/typedcssom/inlinestyle/properties/common-configs.js
(right):

https://codereview.chromium.org/2583063002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/typedcssom/inlinestyle/properties/common-configs.js:2:
* @fileoverview Generates config objects that are common between multiple
On 2017/01/05 22:09:31, sashab wrote:
> What is @fileoverview? Do we use this anywhere else

Haha, it's used in Google3 to autogenerate documentation. It's supposed to be a
1-line summary of the file. Except for the other one that I added, it's only
used in one other place in LayoutTests , lol. Removed.

https://codereview.chromium.org/2583063002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/typedcssom/inlinestyle/properties/common-configs.js:6:
function lengthConfig(propertyName) {
On 2017/01/05 22:09:31, sashab wrote:
> Maybe createConfigForLengthProperty

Changed to config_templates.lengthConfig as per discussion.

https://codereview.chromium.org/2583063002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/typedcssom/inlinestyle/properties/width.html
(right):

https://codereview.chromium.org/2583063002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/typedcssom/inlinestyle/properties/width.html:8:
let config = lengthPercentConfig('width');
On 2017/01/05 22:09:31, sashab wrote:
> To help with naming, maybe try think about what you want this file to look
like,
> e.g.:
> 
> let config = new Config('width')
> config.type = 'length'
> config.validKeywords = ['a', 'b', 'c']
> runTypedOMTests(config)
> 
> What are you going for? :)

Changed to the following:

let config = Object.assign({}, config_templates.lengthPercentConfig);
config['property'] = 'left';
config['validKeywords'] = ['auto'];
runInlineStylePropertyMapTests(config);

[sashab, 2017-01-16 04:48:03]

Looks much better, thanks :)

Just one small thing - what is Object.assign? Does that do the shallow copy?
Don't really like the idea of having that in all the files... Can we add that as
a global method maybe

So the first line can be config.copyConfig(config) or whatever :)

[commit-bot: I haz the power, 2017-01-16 05:51:22]

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

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

Dry run: This issue passed the CQ dry run.

[meade_UTC10, 2017-03-08 04:03:25]

On 2017/01/16 04:48:03, sashab wrote:
> Looks much better, thanks :)
> 
> Just one small thing - what is Object.assign? Does that do the shallow copy?
> Don't really like the idea of having that in all the files... Can we add that
as
> a global method maybe
> 
> So the first line can be config.copyConfig(config) or whatever :)

Sorry for letting this sit so long...

Object.assign is how you copy things in Javascript - it does do a shallow copy.
I don't know why it's named weird...
Added a helper function so you don't have to see it in the test files.

[meade_UTC10, 2017-03-08 04:12:48]

meade@chromium.org changed reviewers:
+ nainar@chromium.org

[meade_UTC10, 2017-03-08 04:12:48]

FYI for Naina

[meade_UTC10, 2017-03-08 04:12:59]

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

[commit-bot: I haz the power, 2017-03-08 04:13:12]

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-03-08 04:14:32]

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

[commit-bot: I haz the power, 2017-03-08 04:14:33]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[meade_UTC10, 2017-03-08 04:18:54]

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

[commit-bot: I haz the power, 2017-03-08 04:19:18]

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-03-08 04:20:54]

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

[commit-bot: I haz the power, 2017-03-08 04:20:56]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[nainar, 2017-03-08 07:49:40]

lgtm

[meade_UTC10, 2017-03-09 05:32:33]

The CQ bit was checked by meade@chromium.org

[commit-bot: I haz the power, 2017-03-09 05:32:55]

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-03-09 07:20:07]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1489037553396590,
"parent_rev": "532d98e5dc972e8e267bac9523f114cd2117702e", "commit_rev":
"4d77a388ceb8b2742c2ee4f0d2c8cc09a0aab4c6"}

[commit-bot: I haz the power, 2017-03-09 07:21:14]

Description was changed from

==========
Add common test configs for properties that take lengths.

This is a follow-up CL to https://codereview.chromium.org/2371673002, to make
testing of length-accepting properties more complete.

To do this, I added "Length" as a typedom_types for each property in
CSSProperties.in
While I was at it, I also added the valid keywords for those properties.

BUG=545318
==========

to

==========
Add common test configs for properties that take lengths.

This is a follow-up CL to https://codereview.chromium.org/2371673002, to make
testing of length-accepting properties more complete.

To do this, I added "Length" as a typedom_types for each property in
CSSProperties.in
While I was at it, I also added the valid keywords for those properties.

BUG=545318

Review-Url: https://codereview.chromium.org/2583063002
Cr-Commit-Position: refs/heads/master@{#455687}
Committed:
https://chromium.googlesource.com/chromium/src/+/4d77a388ceb8b2742c2ee4f0d2c8...
==========

[commit-bot: I haz the power, 2017-03-09 07:21:15]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/4d77a388ceb8b2742c2ee4f0d2c8...