Restructure type tracking in StyleValues to work better with new numeric types

Restructure type tracking in StyleValues to work better with new numeric types.

The enum CSSStyleValue::StyleValueType is now used to track the
"strong type" 
(https://drafts.css-houdini.org/css-typed-om/#cssnumericvalue-strong-type)
of each style value.

Reviewers, please add thoughts on putting "ContainsPercent" on 
CSSStyleValue. I couldn't think of a good way to express 
"CSSStyleValues can be <a non-percent type> *and* a percent at 
the same time (e.g. calc(3px + 2%)). However this is only used
on CSSNumericValue subclasses.

Additional notes:
- User border-*-width for testing setting CSSUnitValues as lengths.
- border-*-width was previously marked as supports_percent, but it
actually does not 
(https://developer.mozilla.org/en/docs/Web/CSS/border-bottom-width)
- There is now a list of valid values for typedom_types in 
CSSProperties.json5
- supports_percentage is now implied by including "Percent" in 
typedom_types



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

[meade_UTC10, 2017-05-26 06:47:25]

Description was changed from

==========
Restructure type tracking in StyleValues to work better with new numeric types

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

to

==========
Restructure type tracking in StyleValues to work better with new numeric types.

The enum CSSStyleValue::StyleValueType is now used to track the
"strong type" 
(https://drafts.css-houdini.org/css-typed-om/#cssnumericvalue-strong-type)
of each style value.

Reviewers, please add thoughts on putting "ContainsPercent" on 
CSSStyleValue. I couldn't think of a good way to express 
"CSSStyleValues can be <a non-percent type> *and* a percent at 
the same time (e.g. calc(3px + 2%)).

Additional notes:
- User border-*-width for testing setting CSSUnitValues as lengths.
- border-*-width was previously marked as supports_percent, but it
actually does not 
(https://developer.mozilla.org/en/docs/Web/CSS/border-bottom-width)
- There is now a list of valid values for typedom_types in 
CSSProperties.json5
- supports_percentage is now implied by including "Percent" in 
typedom_types



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

[meade_UTC10, 2017-05-26 06:48:11]

Description was changed from

==========
Restructure type tracking in StyleValues to work better with new numeric types.

The enum CSSStyleValue::StyleValueType is now used to track the
"strong type" 
(https://drafts.css-houdini.org/css-typed-om/#cssnumericvalue-strong-type)
of each style value.

Reviewers, please add thoughts on putting "ContainsPercent" on 
CSSStyleValue. I couldn't think of a good way to express 
"CSSStyleValues can be <a non-percent type> *and* a percent at 
the same time (e.g. calc(3px + 2%)).

Additional notes:
- User border-*-width for testing setting CSSUnitValues as lengths.
- border-*-width was previously marked as supports_percent, but it
actually does not 
(https://developer.mozilla.org/en/docs/Web/CSS/border-bottom-width)
- There is now a list of valid values for typedom_types in 
CSSProperties.json5
- supports_percentage is now implied by including "Percent" in 
typedom_types



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

to

==========
Restructure type tracking in StyleValues to work better with new numeric types.

The enum CSSStyleValue::StyleValueType is now used to track the
"strong type" 
(https://drafts.css-houdini.org/css-typed-om/#cssnumericvalue-strong-type)
of each style value.

Reviewers, please add thoughts on putting "ContainsPercent" on 
CSSStyleValue. I couldn't think of a good way to express 
"CSSStyleValues can be <a non-percent type> *and* a percent at 
the same time (e.g. calc(3px + 2%)). However this is only used
on CSSNumericValue subclasses.

Additional notes:
- User border-*-width for testing setting CSSUnitValues as lengths.
- border-*-width was previously marked as supports_percent, but it
actually does not 
(https://developer.mozilla.org/en/docs/Web/CSS/border-bottom-width)
- There is now a list of valid values for typedom_types in 
CSSProperties.json5
- supports_percentage is now implied by including "Percent" in 
typedom_types



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

[meade_UTC10, 2017-05-26 06:48:36]

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

[meade_UTC10, 2017-05-26 06:48:37]

Hi Darren! Here's a fun one for you to do as first pass review. PTAL :)

[shend, 2017-05-29 23:42:08]

lgtm after bots pass :)

https://codereview.chromium.org/2903413002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/make_css_property_metadata.py
(right):

https://codereview.chromium.org/2903413002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/make_css_property_metadata.py:23:
self.generate_css_property_metadata_cpp
nit: I think it'll look better on the same line.

https://codereview.chromium.org/2903413002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/make_css_property_metadata.py:23:
self.generate_css_property_metadata_cpp
nit: I think it'll look better on the same line.

https://codereview.chromium.org/2903413002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSProperties.json5 (right):

https://codereview.chromium.org/2903413002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSProperties.json5:152: "Percent",
I like this way of handling %s.

https://codereview.chromium.org/2903413002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSProperties.json5:158: "Image"],
nit: '],' on the next line.

https://codereview.chromium.org/2903413002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSProperties.json5:890: typedom_types:
["Length"],
I would've though this is ["Length", "Percent"]? Does Length imply Percent?

https://codereview.chromium.org/2903413002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/cssom/CSSStyleValue.h (right):

https://codereview.chromium.org/2903413002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/CSSStyleValue.h:55: virtual bool
ContainsPercent() const { return false; }
I don't this is that bad. The only alternative I can think of is putting this in
CSSNumericValue, but it means that where you have:

   value.ContainsPercent()

you need to replace with:

   value.GetType() == <blah> && ((const
CSSNumericValue&)value).ContainsPercent()

I don't think it's worth it.

[meade_UTC10, 2017-05-30 04:11:05]

https://codereview.chromium.org/2903413002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/make_css_property_metadata.py
(right):

https://codereview.chromium.org/2903413002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/make_css_property_metadata.py:23:
self.generate_css_property_metadata_cpp
On 2017/05/29 23:42:08, shend wrote:
> nit: I think it'll look better on the same line.

The style checker was complaining about the line being too long if I did that :/

https://codereview.chromium.org/2903413002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSProperties.json5 (right):

https://codereview.chromium.org/2903413002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSProperties.json5:152: "Percent",
On 2017/05/29 23:42:08, shend wrote:
> I like this way of handling %s.

Acknowledged.

https://codereview.chromium.org/2903413002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSProperties.json5:158: "Image"],
On 2017/05/29 23:42:08, shend wrote:
> nit: '],' on the next line.

Done.

https://codereview.chromium.org/2903413002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSProperties.json5:890: typedom_types:
["Length"],
On 2017/05/29 23:42:08, shend wrote:
> I would've though this is ["Length", "Percent"]? Does Length imply Percent?

Length does not imply percent, but as per the CL description, border-*-width
actually doesn't support percentages... the previous declaration is a mistake
>_> 

https://developer.mozilla.org/en/docs/Web/CSS/border-top-width
in devtools:
>> $0.style.borderTopWidth 
""
>> $0.style.borderTopWidth = "5%"
"5%"
>> $0.style.borderTopWidth 
""

https://codereview.chromium.org/2903413002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/cssom/CSSStyleValue.h (right):

https://codereview.chromium.org/2903413002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/CSSStyleValue.h:55: virtual bool
ContainsPercent() const { return false; }
On 2017/05/29 23:42:08, shend wrote:
> I don't this is that bad. The only alternative I can think of is putting this
in
> CSSNumericValue, but it means that where you have:
> 
>    value.ContainsPercent()
> 
> you need to replace with:
> 
>    value.GetType() == <blah> && ((const
> CSSNumericValue&)value).ContainsPercent()
> 
> I don't think it's worth it.

Yeah, the alternative isn't thaaat bad, but it's a bit of a pain, and requires
more code. I also think this is OK.

[meade_UTC10, 2017-05-30 07:08:56]

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

[meade_UTC10, 2017-05-30 07:08:57]

+nainar to sanity check also, thanks shend and nainar!

[nainar, 2017-05-30 08:17:26]

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

[commit-bot: I haz the power, 2017-05-30 08:17:38]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nainar, 2017-05-30 08:19:18]

meade@, 

Please split this up into 2 CLs.

https://codereview.chromium.org/2903413002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/CSSStyleValue.h (right):

https://codereview.chromium.org/2903413002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/CSSStyleValue.h:52: virtual
~CSSStyleValue() {}
why has this code moved?

https://codereview.chromium.org/2903413002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/CSSUnitValue.h (right):

https://codereview.chromium.org/2903413002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/CSSUnitValue.h:34: String type() const;
This change sounds like it belongs in a separate CL

https://codereview.chromium.org/2903413002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/CSSUnitValue.idl (right):

https://codereview.chromium.org/2903413002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/CSSUnitValue.idl:16: readonly attribute
DOMString type;
This really should be a separate change.

[commit-bot: I haz the power, 2017-05-30 10:24:48]

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

[commit-bot: I haz the power, 2017-05-30 10:24:49]

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_...)

[meade_UTC10, 2017-06-02 04:43:38]

I split out a really little one in https://codereview.chromium.org/2921663004/
if that's what you mean.

https://codereview.chromium.org/2903413002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/CSSStyleValue.h (right):

https://codereview.chromium.org/2903413002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/CSSStyleValue.h:52: virtual
~CSSStyleValue() {}
On 2017/05/30 08:19:17, nainar wrote:
> why has this code moved?

To match the style guide:

"Within each section, generally prefer grouping similar kinds of declarations
together, and generally prefer the following order: types, constants, factory
functions, constructors, assignment operators, destructor, all other methods,
data members."
https://google.github.io/styleguide/cppguide.html#Declaration_Order

https://codereview.chromium.org/2903413002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/CSSUnitValue.h (right):

https://codereview.chromium.org/2903413002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/CSSUnitValue.h:34: String type() const;
On 2017/05/30 08:19:17, nainar wrote:
> This change sounds like it belongs in a separate CL 

Uploaded https://codereview.chromium.org/2921663004/

https://codereview.chromium.org/2903413002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/CSSUnitValue.idl (right):

https://codereview.chromium.org/2903413002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/CSSUnitValue.idl:16: readonly attribute
DOMString type;
On 2017/05/30 08:19:18, nainar wrote:
> This really should be a separate change. 

Done.

[meade_UTC10, 2017-06-02 04:48:32]

On 2017/06/02 04:43:38, meade_UTC10 wrote:
> I split out a really little one in https://codereview.chromium.org/2921663004/
> if that's what you mean.
> 
>
https://codereview.chromium.org/2903413002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/css/cssom/CSSStyleValue.h (right):
> 
>
https://codereview.chromium.org/2903413002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/css/cssom/CSSStyleValue.h:52: virtual
> ~CSSStyleValue() {}
> On 2017/05/30 08:19:17, nainar wrote:
> > why has this code moved?
> 
> To match the style guide:
> 
> "Within each section, generally prefer grouping similar kinds of declarations
> together, and generally prefer the following order: types, constants, factory
> functions, constructors, assignment operators, destructor, all other methods,
> data members."
> https://google.github.io/styleguide/cppguide.html#Declaration_Order
> 
>
https://codereview.chromium.org/2903413002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/css/cssom/CSSUnitValue.h (right):
> 
>
https://codereview.chromium.org/2903413002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/css/cssom/CSSUnitValue.h:34: String type()
const;
> On 2017/05/30 08:19:17, nainar wrote:
> > This change sounds like it belongs in a separate CL 
> 
> Uploaded https://codereview.chromium.org/2921663004/
> 
>
https://codereview.chromium.org/2903413002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/css/cssom/CSSUnitValue.idl (right):
> 
>
https://codereview.chromium.org/2903413002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/css/cssom/CSSUnitValue.idl:16: readonly
attribute
> DOMString type;
> On 2017/05/30 08:19:18, nainar wrote:
> > This really should be a separate change. 
> 
> Done.

oops, I mean https://codereview.chromium.org/2919943002/

[meade_UTC10, 2017-06-02 04:52:10]

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

[commit-bot: I haz the power, 2017-06-02 04:52:34]

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-06-02 06:01:40]

The CQ bit was checked by meade@chromium.org

[meade_UTC10, 2017-06-02 06:01:40]

The patchset sent to the CQ was uploaded after l-g-t-m from shend@chromium.org
Link to the patchset: https://codereview.chromium.org/2903413002/#ps140001
(title: "Remove spurious file")

[nainar, 2017-06-02 06:01:40]

yup lgtm

[commit-bot: I haz the power, 2017-06-02 06:01:58]

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

[commit-bot: I haz the power, 2017-06-02 06:01:59]

This CL has an open dependency (Issue 2919943002 Patch 1). Please resolve the
dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[meade_UTC10, 2017-06-05 01:28:01]

The CQ bit was checked by meade@chromium.org

[meade_UTC10, 2017-06-05 01:28:02]

The patchset sent to the CQ was uploaded after l-g-t-m from nainar@chromium.org,
shend@chromium.org
Link to the patchset: https://codereview.chromium.org/2903413002/#ps160001
(title: "rebase")

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

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-06-05 03:03:57]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1496626081184530,
"parent_rev": "c94383db1e3b0e200f22286e140d0542433acbf7", "commit_rev":
"ee27a36a85422f591853031122a77a5d01ecc68d"}

[commit-bot: I haz the power, 2017-06-05 03:04:09]

Description was changed from

==========
Restructure type tracking in StyleValues to work better with new numeric types.

The enum CSSStyleValue::StyleValueType is now used to track the
"strong type" 
(https://drafts.css-houdini.org/css-typed-om/#cssnumericvalue-strong-type)
of each style value.

Reviewers, please add thoughts on putting "ContainsPercent" on 
CSSStyleValue. I couldn't think of a good way to express 
"CSSStyleValues can be <a non-percent type> *and* a percent at 
the same time (e.g. calc(3px + 2%)). However this is only used
on CSSNumericValue subclasses.

Additional notes:
- User border-*-width for testing setting CSSUnitValues as lengths.
- border-*-width was previously marked as supports_percent, but it
actually does not 
(https://developer.mozilla.org/en/docs/Web/CSS/border-bottom-width)
- There is now a list of valid values for typedom_types in 
CSSProperties.json5
- supports_percentage is now implied by including "Percent" in 
typedom_types



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

to

==========
Restructure type tracking in StyleValues to work better with new numeric types.

The enum CSSStyleValue::StyleValueType is now used to track the
"strong type" 
(https://drafts.css-houdini.org/css-typed-om/#cssnumericvalue-strong-type)
of each style value.

Reviewers, please add thoughts on putting "ContainsPercent" on 
CSSStyleValue. I couldn't think of a good way to express 
"CSSStyleValues can be <a non-percent type> *and* a percent at 
the same time (e.g. calc(3px + 2%)). However this is only used
on CSSNumericValue subclasses.

Additional notes:
- User border-*-width for testing setting CSSUnitValues as lengths.
- border-*-width was previously marked as supports_percent, but it
actually does not 
(https://developer.mozilla.org/en/docs/Web/CSS/border-bottom-width)
- There is now a list of valid values for typedom_types in 
CSSProperties.json5
- supports_percentage is now implied by including "Percent" in 
typedom_types



BUG=545318

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

[commit-bot: I haz the power, 2017-06-05 03:04:11]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/ee27a36a85422f591853031122a7...