CC Animation: Expose TargetProperty enum to be aliased in Blink Platform.

CC Animation: Expose TargetProperty enum to be aliased in Blink Platform.

AN UNFINISHED DEMO.

AnimationTargetProperty is intended to be used as an alias in
Source/platform/animation/CompositorAnimationTargetProperty.h

It demonstrates all the problems with strongly typed enums:
1) ostream support needed (logging):
DCHECK(animation->target_property() != AnimationTargetProperty::SCROLL_OFFSET) to work.
Pros: it will be logged with its string value instead of integer value for old-style enums.

2) hash support needed.
(See the usage in LayerAnimationController)
(C++14 addresses this with DR2148)

3) array indexing: static_casts or helper functions.
(See the problems in animation_timelines_test_common.*)

Consider an alternative approach:
https://codereview.chromium.org/1700653002


BUG=577016
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

[loyso (OOO), 2016-02-15 06:35:04]

Description was changed from

==========
CC Animation: Expose TargetProperty enum to be aliased in Blink Platform.

BUG=577016
==========

to

==========
CC Animation: Expose TargetProperty enum to be aliased in Blink Platform.

BUG=577016
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[loyso (OOO), 2016-02-15 06:39:54]

Description was changed from

==========
CC Animation: Expose TargetProperty enum to be aliased in Blink Platform.

BUG=577016
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
CC Animation: Expose TargetProperty enum to be aliased in Blink Platform.

AN UNFINISHED DEMO.

AnimationTargetProperty is intended to be used as an alias in
Source/platform/animation/CompositorAnimationTargetProperty.h

It demonstrates all the problems with strongly typed enums:
1) ostream support needed
2) hash support needed
3) array indexing: static_casts or helper functions.

Consider an alternative approach:
https://codereview.chromium.org/1700653002


BUG=577016
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[loyso (OOO), 2016-02-15 06:39:54]

loyso@chromium.org changed reviewers:
+ ajuma@chromium.org, danakj@chromium.org, esprehn@chromium.org,
jbroman@chromium.org, vollick@chromium.org

[loyso (OOO), 2016-02-15 06:40:37]

PTAL! Which approach is better?

https://codereview.chromium.org/1698813002/diff/1/cc/animation/animation_targ...
File cc/animation/animation_target_property.h (right):

https://codereview.chromium.org/1698813002/diff/1/cc/animation/animation_targ...
cc/animation/animation_target_property.h:39: }
We need some common traits (a macro) to work with strongly-typed enums and
implement all these helpers.

[loyso (OOO), 2016-02-15 06:45:55]

Description was changed from

==========
CC Animation: Expose TargetProperty enum to be aliased in Blink Platform.

AN UNFINISHED DEMO.

AnimationTargetProperty is intended to be used as an alias in
Source/platform/animation/CompositorAnimationTargetProperty.h

It demonstrates all the problems with strongly typed enums:
1) ostream support needed
2) hash support needed
3) array indexing: static_casts or helper functions.

Consider an alternative approach:
https://codereview.chromium.org/1700653002


BUG=577016
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
CC Animation: Expose TargetProperty enum to be aliased in Blink Platform.

AN UNFINISHED DEMO.

AnimationTargetProperty is intended to be used as an alias in
Source/platform/animation/CompositorAnimationTargetProperty.h

It demonstrates all the problems with strongly typed enums:
1) ostream support needed
2) hash support needed (See the usage in LayerAnimationController)
3) array indexing: static_casts or helper functions. (See the problems in
animation_timelines_test_common.*)

Consider an alternative approach:
https://codereview.chromium.org/1700653002


BUG=577016
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[loyso (OOO), 2016-02-15 06:47:15]

Description was changed from

==========
CC Animation: Expose TargetProperty enum to be aliased in Blink Platform.

AN UNFINISHED DEMO.

AnimationTargetProperty is intended to be used as an alias in
Source/platform/animation/CompositorAnimationTargetProperty.h

It demonstrates all the problems with strongly typed enums:
1) ostream support needed
2) hash support needed (See the usage in LayerAnimationController)
3) array indexing: static_casts or helper functions. (See the problems in
animation_timelines_test_common.*)

Consider an alternative approach:
https://codereview.chromium.org/1700653002


BUG=577016
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
CC Animation: Expose TargetProperty enum to be aliased in Blink Platform.

AN UNFINISHED DEMO.

AnimationTargetProperty is intended to be used as an alias in
Source/platform/animation/CompositorAnimationTargetProperty.h

It demonstrates all the problems with strongly typed enums:
1) ostream support needed (logging):

DCHECK(animation->target_property() != AnimationTargetProperty::SCROLL_OFFSET)
to work.

2) hash support needed (See the usage in LayerAnimationController)
3) array indexing: static_casts or helper functions. (See the problems in
animation_timelines_test_common.*)

Consider an alternative approach:
https://codereview.chromium.org/1700653002


BUG=577016
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[loyso (OOO), 2016-02-15 06:47:46]

Description was changed from

==========
CC Animation: Expose TargetProperty enum to be aliased in Blink Platform.

AN UNFINISHED DEMO.

AnimationTargetProperty is intended to be used as an alias in
Source/platform/animation/CompositorAnimationTargetProperty.h

It demonstrates all the problems with strongly typed enums:
1) ostream support needed (logging):

DCHECK(animation->target_property() != AnimationTargetProperty::SCROLL_OFFSET)
to work.

2) hash support needed (See the usage in LayerAnimationController)
3) array indexing: static_casts or helper functions. (See the problems in
animation_timelines_test_common.*)

Consider an alternative approach:
https://codereview.chromium.org/1700653002


BUG=577016
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
CC Animation: Expose TargetProperty enum to be aliased in Blink Platform.

AN UNFINISHED DEMO.

AnimationTargetProperty is intended to be used as an alias in
Source/platform/animation/CompositorAnimationTargetProperty.h

It demonstrates all the problems with strongly typed enums:
1) ostream support needed (logging):
DCHECK(animation->target_property() != AnimationTargetProperty::SCROLL_OFFSET)
to work.

2) hash support needed.
(See the usage in LayerAnimationController)

3) array indexing: static_casts or helper functions.
(See the problems in animation_timelines_test_common.*)

Consider an alternative approach:
https://codereview.chromium.org/1700653002


BUG=577016
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[loyso (OOO), 2016-02-15 22:52:48]

Any suggestions/opinions please?

[loyso (OOO), 2016-02-16 06:14:18]

Description was changed from

==========
CC Animation: Expose TargetProperty enum to be aliased in Blink Platform.

AN UNFINISHED DEMO.

AnimationTargetProperty is intended to be used as an alias in
Source/platform/animation/CompositorAnimationTargetProperty.h

It demonstrates all the problems with strongly typed enums:
1) ostream support needed (logging):
DCHECK(animation->target_property() != AnimationTargetProperty::SCROLL_OFFSET)
to work.

2) hash support needed.
(See the usage in LayerAnimationController)

3) array indexing: static_casts or helper functions.
(See the problems in animation_timelines_test_common.*)

Consider an alternative approach:
https://codereview.chromium.org/1700653002


BUG=577016
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
CC Animation: Expose TargetProperty enum to be aliased in Blink Platform.

AN UNFINISHED DEMO.

AnimationTargetProperty is intended to be used as an alias in
Source/platform/animation/CompositorAnimationTargetProperty.h

It demonstrates all the problems with strongly typed enums:
1) ostream support needed (logging):
DCHECK(animation->target_property() != AnimationTargetProperty::SCROLL_OFFSET)
to work.

2) hash support needed.
(See the usage in LayerAnimationController)
(C++14 addresses this with DR2148)

3) array indexing: static_casts or helper functions.
(See the problems in animation_timelines_test_common.*)

Consider an alternative approach:
https://codereview.chromium.org/1700653002


BUG=577016
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[loyso (OOO), 2016-02-16 06:18:44]

Description was changed from

==========
CC Animation: Expose TargetProperty enum to be aliased in Blink Platform.

AN UNFINISHED DEMO.

AnimationTargetProperty is intended to be used as an alias in
Source/platform/animation/CompositorAnimationTargetProperty.h

It demonstrates all the problems with strongly typed enums:
1) ostream support needed (logging):
DCHECK(animation->target_property() != AnimationTargetProperty::SCROLL_OFFSET)
to work.

2) hash support needed.
(See the usage in LayerAnimationController)
(C++14 addresses this with DR2148)

3) array indexing: static_casts or helper functions.
(See the problems in animation_timelines_test_common.*)

Consider an alternative approach:
https://codereview.chromium.org/1700653002


BUG=577016
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
CC Animation: Expose TargetProperty enum to be aliased in Blink Platform.

AN UNFINISHED DEMO.

AnimationTargetProperty is intended to be used as an alias in
Source/platform/animation/CompositorAnimationTargetProperty.h

It demonstrates all the problems with strongly typed enums:
1) ostream support needed (logging):
DCHECK(animation->target_property() != AnimationTargetProperty::SCROLL_OFFSET)
to work.
Pros: it will be logged with its string value instead of integer value for
old-style enums.

2) hash support needed.
(See the usage in LayerAnimationController)
(C++14 addresses this with DR2148)

3) array indexing: static_casts or helper functions.
(See the problems in animation_timelines_test_common.*)

Consider an alternative approach:
https://codereview.chromium.org/1700653002


BUG=577016
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[jbroman, 2016-02-16 15:56:43]

This (and the comparison to the other) seem to me to be largely an issue for cc
owners (I don't know that the difference really affects Blink much). I do have
one minor comment.

(Apologies for the delayed reply; yesterday was a holiday for both US and
Canada, which is why we were all unresponsive.)

https://codereview.chromium.org/1698813002/diff/1/cc/animation/animation_targ...
File cc/animation/animation_target_property.h (right):

https://codereview.chromium.org/1698813002/diff/1/cc/animation/animation_targ...
cc/animation/animation_target_property.h:35: inline size_t
hash<cc::AnimationTargetProperty>::operator()(
http://chromium-cpp.appspot.com/ and the Google C++ style guide say we shouldn't
specialize std::hash.

[ajuma, 2016-02-16 18:13:53]

On 2016/02/15 06:40:37, loyso wrote:
> PTAL! Which approach is better?

I weakly prefer the approach in this CL.

[loyso (OOO), 2016-02-16 23:20:28]

On 2016/02/16 15:56:43, jbroman wrote:
> This (and the comparison to the other) seem to me to be largely an issue for
cc
> owners (I don't know that the difference really affects Blink much).
Well, it's straightforward. We want to use this enum as:

namespace blink {
using CompositorAnimationTargetProperty = cc::AnimationTargetProperty;
}

so we write CompositorAnimationTargetProperty::SCROLL_OFFSET or
CompositorAnimationTargetProperty::Enum in blink...

> http://chromium-cpp.appspot.com/ and the Google C++ style guide say we
shouldn't
> specialize std::hash.
Ok. I'll try a variant with a custom hasher for strongly typed enum.

[loyso (OOO), 2016-02-17 00:22:58]

On 2016/02/16 15:56:43, jbroman wrote:
> http://chromium-cpp.appspot.com/ and the Google C++ style guide say we
shouldn't
> specialize std::hash.
Done.

[loyso (OOO), 2016-02-17 00:23:06]

https://codereview.chromium.org/1698813002/diff/20001/cc/animation/animation_...
File cc/animation/animation_target_property.h (right):

https://codereview.chromium.org/1698813002/diff/20001/cc/animation/animation_...
cc/animation/animation_target_property.h:30: struct EnumClassHash {
This can be shared somewhere in base/

[esprehn, 2016-02-17 00:40:11]

https://codereview.chromium.org/1698813002/diff/20001/cc/animation/layer_anim...
File cc/animation/layer_animation_controller.h (right):

https://codereview.chromium.org/1698813002/diff/20001/cc/animation/layer_anim...
cc/animation/layer_animation_controller.h:179:
std::unordered_set<AnimationTargetProperty, EnumClassHash>;
This could just be a bitset right? there's like 5 properties?

[loyso (OOO), 2016-02-17 00:51:16]

On 2016/02/17 00:40:11, esprehn wrote:
> std::unordered_set<AnimationTargetProperty, EnumClassHash>;
> This could just be a bitset right? there's like 5 properties?
In this particular case, yes. I'm rather interested in general approach to
cc/blink enums sharing.

[loyso (OOO), 2016-02-17 05:23:50]

On 2016/02/17 00:40:11, esprehn wrote:
> std::unordered_set<AnimationTargetProperty, EnumClassHash>;
> This could just be a bitset right? there's like 5 properties?
p.s. std::bit_set would require a compile-time size and integer subscripts
(indexes). So explicit casts to int would be needed.
So my current understanding is that we should use strongly-typed enum only when
no any conversions to int needed.
In this case I would prefer a CL with 'Enum' approach (so we can: have an
implicit conversion to integers, refer to it with 'using', add member conversion
functions)

[loyso (OOO), 2016-02-17 05:25:58]

loyso@chromium.org changed reviewers:
+ enne@chromium.org
- jbroman@chromium.org

[loyso (OOO), 2016-02-21 22:56:29]

Closing this issue in favor for https://codereview.chromium.org/1700653002/