Generate CSSPropertyEquality instead of using hand-updated file.

[WIP] Generate CSSPropertyEquality instead of using hand-updated file.

This patch generates CSSPropertyEquality::propertiesEqual based off CSSProperties.in.

It uses existing flags on CSSProperties.in to decide if the equality check can be auto generated, (i.e. 'svg'). If not it produces a header stub on CSSPropertyEqualityCustom.h which can be implemented by hand in CSSPropertyEqualityCustom.cpp

BUG=

[ikilpatrick, 2016-04-27 00:29:46]

Description was changed from

==========
[WIP] Generate CSSPropertyEquality instead of using hand-updated file.

BUG=
==========

to

==========
[WIP] Generate CSSPropertyEquality instead of using hand-updated file.

This patch generates CSSPropertyEquality::propertiesEqual based off
CSSProperties.in.

It uses existing flags on CSSProperties.in to decide if the equality check can
be auto generated, (i.e. 'svg'). If not it produces a header stub on
CSSPropertyEqualityCustom.h which can be implemented by hand in
CSSPropertyEqualityCustom.cpp

BUG=
==========

[ikilpatrick, 2016-04-27 00:29:48]

ikilpatrick@chromium.org changed reviewers:
+ alancutter@chromium.org

[ikilpatrick, 2016-04-27 00:32:17]

Alan, could you take an initial look at this? This isn't ready to submit, but I
just want some early feedback that I'm on the right or wrong path.

Thanks!

https://codereview.chromium.org/1910263003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSPropertyEqualityCustom.cpp (right):

https://codereview.chromium.org/1910263003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPropertyEqualityCustom.cpp:93: // TODO
test
Anything with TODO test means that I need to do a cpp test setting an inline
style and checking equality.
I.e. I'm very scared that the computed style test of these will be wrong :).

https://codereview.chromium.org/1910263003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPropertyEqualityCustom.cpp:99: return
a.transitions() == b.transitions()
transitions / animations would be auto-gen'd in the cpp.tmpl file if we had a
special mark for them, worth it?

https://codereview.chromium.org/1910263003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPropertyEqualityCustom.cpp:105: return
a.textDecorationColor() == b.textDecorationColor()
Ditto colors, a special flag for color properties, means we could auto-gen in
cpp.tmpl, worth it?

https://codereview.chromium.org/1910263003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPropertyEqualityCustom.cpp:129: return
fillLayersEqual<CSSPropertyWebkitMaskComposite>(a.maskLayers(), b.maskLayers());
Ditto fillLayers.

[alancutter (OOO until 2018), 2016-05-02 02:55:40]

Sorry for the late review. This looks reasonable to me!

https://codereview.chromium.org/1910263003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSPropertyEqualityCustom.cpp (right):

https://codereview.chromium.org/1910263003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPropertyEqualityCustom.cpp:93: // TODO
test
On 2016/04/27 at 00:32:17, ikilpatrick wrote:
> Anything with TODO test means that I need to do a cpp test setting an inline
style and checking equality.
> I.e. I'm very scared that the computed style test of these will be wrong :).

Could you test it with layout tests? I think that'd be safer to make sure you're
not missing out on any part of the style resolve process. Perhaps you need to
expose this equality check in internals until it becomes an observable
behaviour.

https://codereview.chromium.org/1910263003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPropertyEqualityCustom.cpp:99: return
a.transitions() == b.transitions()
On 2016/04/27 at 00:32:17, ikilpatrick wrote:
> transitions / animations would be auto-gen'd in the cpp.tmpl file if we had a
special mark for them, worth it?

I think it'd be cleaner to make accessors for transitionDelay and co. on
ComputedStyle so it fits the mould of every other property.

https://codereview.chromium.org/1910263003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPropertyEqualityCustom.cpp:129: return
fillLayersEqual<CSSPropertyWebkitMaskComposite>(a.maskLayers(), b.maskLayers());
On 2016/04/27 at 00:32:17, ikilpatrick wrote:
> Ditto fillLayers.

I'm not entirely for the idea of adding special flags to CSSProperties.in for
specific types, I'm sad to see this is already the case for font properties. I'd
rather see the C++ code fit into a unified interface for properties and this
custom equality handling seems fine for now.

If I had to drop the custom code I'd consider having the ComputedStyle accessors
return STACK_ALLOCATED view objects that know how to check equality (perhaps
later if use cases justify the abstraction).

[ikilpatrick, 2016-05-24 00:16:15]

ikilpatrick@chromium.org changed reviewers:
+ timloh@chromium.org

[ikilpatrick, 2016-05-24 00:16:16]

Sorry for the delay here Alan, CSS F2F + other things came up. :)
+timloh as well.

Tim: Really not tied to anything in this patch, so if you have thoughts on a
better way to do it, lmk.

https://codereview.chromium.org/1910263003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSPropertyEqualityCustom.cpp (right):

https://codereview.chromium.org/1910263003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPropertyEqualityCustom.cpp:93: // TODO
test
On 2016/05/02 02:55:40, alancutter wrote:
> On 2016/04/27 at 00:32:17, ikilpatrick wrote:
> > Anything with TODO test means that I need to do a cpp test setting an inline
> style and checking equality.
> > I.e. I'm very scared that the computed style test of these will be wrong :).
> 
> Could you test it with layout tests? I think that'd be safer to make sure
you're
> not missing out on any part of the style resolve process. Perhaps you need to
> expose this equality check in internals until it becomes an observable
> behaviour.

Done.

https://codereview.chromium.org/1910263003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPropertyEqualityCustom.cpp:99: return
a.transitions() == b.transitions()
On 2016/05/02 02:55:40, alancutter wrote:
> On 2016/04/27 at 00:32:17, ikilpatrick wrote:
> > transitions / animations would be auto-gen'd in the cpp.tmpl file if we had
a
> special mark for them, worth it?
> 
> I think it'd be cleaner to make accessors for transitionDelay and co. on
> ComputedStyle so it fits the mould of every other property.

I had a look at this, we already need a separate flag to determine if it's an
animation/transition in order to not trigger the custom code path (or is there
something else I can use?). I can revert this if you like, and return to this
hand-written version.

Completely up to you ^-^.

https://codereview.chromium.org/1910263003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPropertyEqualityCustom.cpp:129: return
fillLayersEqual<CSSPropertyWebkitMaskComposite>(a.maskLayers(), b.maskLayers());
On 2016/05/02 02:55:40, alancutter wrote:
> On 2016/04/27 at 00:32:17, ikilpatrick wrote:
> > Ditto fillLayers.
> 
> I'm not entirely for the idea of adding special flags to CSSProperties.in for
> specific types, I'm sad to see this is already the case for font properties.
I'd
> rather see the C++ code fit into a unified interface for properties and this
> custom equality handling seems fine for now.
> 
> If I had to drop the custom code I'd consider having the ComputedStyle
accessors
> return STACK_ALLOCATED view objects that know how to check equality (perhaps
> later if use cases justify the abstraction).

Acknowledged.

[Timothy Loh, 2016-05-24 04:38:20]

Can we start by doing what's just enough for the properties currently supported
for transitions? This patch is pretty big right now :x

https://codereview.chromium.org/1910263003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/BUILD.gn (right):

https://codereview.chromium.org/1910263003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/BUILD.gn:661: # "CSSOMTypes" in
make_core_generated from GYP.
too much copy-paste

https://codereview.chromium.org/1910263003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSPropertyEqualityCustom.cpp (right):

https://codereview.chromium.org/1910263003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPropertyEqualityCustom.cpp:42: break;
missing breaks on subsequent cases

[Timothy Loh, 2016-05-24 05:38:35]

Patch notes needs some more context as to why we're doing this.

https://codereview.chromium.org/1910263003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/css_properties.py (right):

https://codereview.chromium.org/1910263003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/css_properties.py:19: 'animation':
False,
I'm not really a fan of having these as arguments in the .in file as they don't
seem to add anything over what's already in the property name.

https://codereview.chromium.org/1910263003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/make_css_property_equality.py
(right):

https://codereview.chromium.org/1910263003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/make_css_property_equality.py:29:
upper_camel = property['upper_camel_name']
probably any of the stuff copied from other files should just move up into
css_properties.py

https://codereview.chromium.org/1910263003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSProperties.in (right):

https://codereview.chromium.org/1910263003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSProperties.in:79: // custom_all is
equivalent to setting the other three flags
worth mentioning here?

https://codereview.chromium.org/1910263003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSPropertyEqualityCustom.cpp (right):

https://codereview.chromium.org/1910263003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPropertyEqualityCustom.cpp:23: switch
(property) {
Maybe it's nicer to pass a getter as the template argument instead of a
property?

https://codereview.chromium.org/1910263003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/1910263003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.h:135: friend class
CSSPropertyEqualityCustom; // Used by CSS animations. We can't allow them to
animate based off visited colors.
do we need to friend both of these?

https://codereview.chromium.org/1910263003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/DataEquivalency.h (right):

https://codereview.chromium.org/1910263003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/DataEquivalency.h:53: bool
dataEquivalent(const T a, const T b)
const T&?