CC Animation: Move files from cc_blink to Source/platform/animation

CC Animation: Move files from cc_blink to Source/platform/animation

Disclaimer: This CL is only about moving files and sections of code.

We:

- Remove Web prefixes on files/classes and add Compositor prefix where necessary.

- Move exported/WebTransformKeyframe.cpp and exported/WebFilterKeyframe.cpp
to animation/ to group them with their headers.

- Move CompositorFilterOperations not to animation/ but to graphics/ folder,
because it's a property of WebLayer.

- Introduce CompositorFactory singleton (works as Platform) to create CC objects
in blink.
All animation methods are moved from CompositorSupport to CompositorFactory.
Other methods are also expected to be moved there.

- Decouple WebLayer and WebCompositorAnimation interfaces
by using cc::Animation class.
It allows us to make WebCompositorAnimation non-public.
WebLayer animation dependencies will go away when we erase the old animation system
(it's already disabled). http://crbug.com/575041

- Move static asserts from content/child/assert_matching_enums.cc to
Source/platform/WebCompositorAnimation.cpp (so the enum becomes non-public).

What's next (separate CLs):
- Remove virtual functions (when gmock-based tests erased)
- Remove redundant enum declarations (using alias).
- Erase the animation delegate adapter.

BUG=577016
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/26f3c5504044531a5772be2009dab72d94ade0cd
Cr-Commit-Position: refs/heads/master@{#374792}

[loyso (OOO), 2016-01-21 03:50:45]

Description was changed from

==========
CC Animations: Move files from cc_blink to Source/platform/animation

Work In Progress (demo).

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

to

==========
CC Animations: Move files from cc_blink to Source/platform/animation

Work In Progress (demo).

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

[loyso (OOO), 2016-01-21 03:52:34]

Description was changed from

==========
CC Animations: Move files from cc_blink to Source/platform/animation

Work In Progress (demo).

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

to

==========
CC Animations: Move files from cc_blink to Source/platform/animation

Work In Progress (demo).

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

[loyso (OOO), 2016-01-21 03:52:35]

loyso@chromium.org changed reviewers:
+ ajuma@chromium.org, dstockwell@chromium.org, shans@chromium.org,
vollick@chromium.org

[loyso (OOO), 2016-01-21 03:52:54]

loyso@chromium.org changed reviewers:
+ piman@chromium.org

[loyso (OOO), 2016-01-21 03:53:26]

loyso@chromium.org changed reviewers:
+ esprehn@chromium.org

[loyso (OOO), 2016-01-21 03:55:08]

Please, review the concept (naming, design, grouping).
WIP.

https://codereview.chromium.org/1616653002/diff/1/cc/blink/web_animation_impl.cc
File cc/blink/web_animation_impl.cc (right):

https://codereview.chromium.org/1616653002/diff/1/cc/blink/web_animation_impl...
cc/blink/web_animation_impl.cc:14: #include
"third_party/WebKit/Source/platform/animation/WebTransformAnimationCurve.h"
This is a viloation of includes. All the dependent cc_blink files have to be
moved. (WIP)

[loyso (OOO), 2016-01-21 03:59:02]

Description was changed from

==========
CC Animations: Move files from cc_blink to Source/platform/animation

Work In Progress (demo).

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

to

==========
CC Animations: Move files from cc_blink to Source/platform/animation

Work In Progress (demo).

This demo patchset move files from cc_blink to Source/platform/animation (as an
example).

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

[loyso (OOO), 2016-01-21 03:59:12]

Description was changed from

==========
CC Animations: Move files from cc_blink to Source/platform/animation

Work In Progress (demo).

This demo patchset move files from cc_blink to Source/platform/animation (as an
example).

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

to

==========
CC Animations: Move files from cc_blink to Source/platform/animation

Work In Progress (demo).

This demo patchset moves files from cc_blink to Source/platform/animation (as an
example).

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

[loyso (OOO), 2016-01-21 03:59:22]

https://codereview.chromium.org/1616653002/diff/1/cc/blink/web_layer_impl.cc
File cc/blink/web_layer_impl.cc (right):

https://codereview.chromium.org/1616653002/diff/1/cc/blink/web_layer_impl.cc#...
cc/blink/web_layer_impl.cc:29: #include
"third_party/WebKit/Source/platform/animation/WebFilterOperations.h"
WIP

[loyso (OOO), 2016-01-21 05:53:37]

Next step is to port the rest:

You added one or more #includes that violate checkdeps rules.
  cc/blink/web_animation_impl.cc
      Illegal include:
"third_party/WebKit/Source/platform/animation/WebCompositorAnimationCurve.h"
    Because of no rule applying. \
  cc/blink/web_animation_impl.cc
      Illegal include:
"third_party/WebKit/Source/platform/animation/WebFilterAnimationCurve.h"
    Because of no rule applying. \
  cc/blink/web_animation_impl.cc
      Illegal include:
"third_party/WebKit/Source/platform/animation/WebFloatAnimationCurve.h"
    Because of no rule applying. \
  cc/blink/web_animation_impl.cc
      Illegal include:
"third_party/WebKit/Source/platform/animation/WebScrollOffsetAnimationCurve.h"
    Because of no rule applying. \
  cc/blink/web_animation_impl.cc
      Illegal include:
"third_party/WebKit/Source/platform/animation/WebTransformAnimationCurve.h"
    Because of no rule applying. \
  cc/blink/web_animation_unittest.cc
      Illegal include:
"third_party/WebKit/Source/platform/animation/WebFloatAnimationCurve.h"
    Because of no rule applying. \
  cc/blink/web_display_item_list_impl.cc
      Illegal include:
"third_party/WebKit/Source/platform/animation/WebFilterOperations.h"
    Because of no rule applying. \
  cc/blink/web_layer_impl.cc
      Illegal include:
"third_party/WebKit/Source/platform/animation/WebFilterOperations.h"
    Because of no rule applying.

Porting web_animation_impl.cc will pull web_animation_player_impl.cc and
web_animation_timeline_impl.cc and so on...

[loyso (OOO), 2016-01-21 05:57:07]

Description was changed from

==========
CC Animations: Move files from cc_blink to Source/platform/animation

Work In Progress (demo).

This demo patchset moves files from cc_blink to Source/platform/animation (as an
example).

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

to

==========
CC Blink: Move files from cc_blink to Source/platform/animation

Work In Progress (demo).

This demo patchset moves files from cc_blink to Source/platform/animation (as an
example).

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

[loyso (OOO), 2016-01-21 05:57:21]

Description was changed from

==========
CC Blink: Move files from cc_blink to Source/platform/animation

Work In Progress (demo).

This demo patchset moves files from cc_blink to Source/platform/animation (as an
example).

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

to

==========
CC Blink: Move files from cc_blink to Source/platform/

Work In Progress (demo).

This demo patchset moves files from cc_blink to Source/platform/animation (as an
example).

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

[loyso (OOO), 2016-01-21 05:58:20]

Trybots: It passes all the tests locally. Rebase needed.

[loyso (OOO), 2016-01-21 06:03:02]

https://codereview.chromium.org/1616653002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/CompositorFactory.h (right):

https://codereview.chromium.org/1616653002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/CompositorFactory.h:22: class PLATFORM_EXPORT
CompositorFactory {
We will be able to delete CompositorFactory if we get rid of
CompositorAnimationsTest.cpp and CompositorAnimationsImpl.cpp.

[loyso (OOO), 2016-01-22 03:13:27]

loyso@chromium.org changed reviewers:
+ jbroman@chromium.org
- piman@chromium.org, shans@chromium.org

[loyso (OOO), 2016-01-22 05:26:44]

To do next:

You added one or more #includes that violate checkdeps rules.
  cc/blink/web_compositor_animation_player_impl.cc
      Illegal include:
"third_party/WebKit/Source/platform/animation/WebCompositorAnimation.h"
    Because of no rule applying. \
  cc/blink/web_display_item_list_impl.cc
      Illegal include:
"third_party/WebKit/Source/platform/animation/WebFilterOperations.h"
    Because of no rule applying. \
  cc/blink/web_layer_impl.cc
      Illegal include:
"third_party/WebKit/Source/platform/animation/WebCompositorAnimation.h"
    Because of no rule applying. \
  cc/blink/web_layer_impl.cc
      Illegal include:
"third_party/WebKit/Source/platform/animation/WebFilterOperations.h"
    Because of no rule applying. \
  cc/blink/web_layer_impl.h
      Illegal include:
"third_party/WebKit/Source/platform/animation/WebCompositorAnimation.h"
    Because of no rule applying. \
  content/child/assert_matching_enums.cc
      Illegal include:
"third_party/WebKit/Source/platform/animation/WebCompositorAnimation.h"
    Because of no rule applying.

[loyso (OOO), 2016-01-25 06:25:05]

Description was changed from

==========
CC Blink: Move files from cc_blink to Source/platform/

Work In Progress (demo).

This demo patchset moves files from cc_blink to Source/platform/animation (as an
example).

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

to

==========
CC Animation: Move files from cc_blink to Source/platform/animation

Work In Progress (demo).

This demo patchset moves files from cc_blink to Source/platform/animation (as an
example).

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

[loyso (OOO), 2016-01-25 06:30:00]

We need to leave WebCompositorAnimation, WebFilterOperations and
WebToCCAnimationDelegateAdapter interfaces in public/platform, because
WebLayerImpl uses them.
We will be able to land this CL after that. Stay tuned.

[loyso (OOO), 2016-01-25 06:33:04]

loyso@chromium.org changed reviewers:
+ piman@chromium.org

[loyso (OOO), 2016-01-25 06:33:05]

piman@chromium.org: Please review changes in

[piman, 2016-01-25 17:30:25]

lgtm

[loyso (OOO), 2016-01-27 07:06:48]

platform and cc_blink people, PTAL!
It's ready to land.

[Ian Vollick, 2016-01-27 13:53:17]

On 2016/01/27 07:06:48, loyso wrote:
> platform and cc_blink people, PTAL!
> It's ready to land.

lgtm with nit.

[Ian Vollick, 2016-01-27 13:53:27]

https://codereview.chromium.org/1616653002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/animation/WebCompositorAnimation.cpp
(right):

https://codereview.chromium.org/1616653002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/animation/WebCompositorAnimation.cpp:38:
cc::Animation::SCROLL_OFFSET);
When I did similar refactoring for compositor worker, Dana had a suggestion that
made it possible to avoid parallel enums and these static asserts.
https://codereview.chromium.org/1599673002/

[loyso (OOO), 2016-01-27 23:46:27]

https://codereview.chromium.org/1616653002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/animation/WebCompositorAnimation.cpp
(right):

https://codereview.chromium.org/1616653002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/animation/WebCompositorAnimation.cpp:38:
cc::Animation::SCROLL_OFFSET);
On 2016/01/27 13:53:27, vollick wrote:
> When I did similar refactoring for compositor worker, Dana had a suggestion
that
> made it possible to avoid parallel enums and these static asserts.
> https://codereview.chromium.org/1599673002/
Nice trick, thanks. And this CL is about moving code (I've taken these static
asserts from content/child/assert_matching_enums.cc)
I want to do that in a subsequent separate CL (and many other things like:
Remove Web prefix)

[loyso (OOO), 2016-01-28 00:22:47]

Description was changed from

==========
CC Animation: Move files from cc_blink to Source/platform/animation

Work In Progress (demo).

This demo patchset moves files from cc_blink to Source/platform/animation (as an
example).

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

to

==========
CC Animation: Move files from cc_blink to Source/platform/animation

- We introduce CompositorFactory singleton (works as Platform) to create CC
objects in blink. All animation methods are moved from CompositorSupport to
CompositorFactory. Other methods are also expected to be moved there.

- We decouple WebLayer and WebCompositorAnimation interfaces by using
cc::Animation class. It allows us to make WebCompositorAnimation to be non
public. WebLayer animation dependencies will go away when we erase the old
animation system (it's already disabled). crbug.com/575041 

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

[loyso (OOO), 2016-01-28 00:25:11]

Description was changed from

==========
CC Animation: Move files from cc_blink to Source/platform/animation

- We introduce CompositorFactory singleton (works as Platform) to create CC
objects in blink. All animation methods are moved from CompositorSupport to
CompositorFactory. Other methods are also expected to be moved there.

- We decouple WebLayer and WebCompositorAnimation interfaces by using
cc::Animation class. It allows us to make WebCompositorAnimation to be non
public. WebLayer animation dependencies will go away when we erase the old
animation system (it's already disabled). crbug.com/575041 

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

to

==========
CC Animation: Move files from cc_blink to Source/platform/animation

- We introduce CompositorFactory singleton (works as Platform) to create CC
objects in blink. All animation methods are moved from CompositorSupport to
CompositorFactory. Other methods are also expected to be moved there.

- We decouple WebLayer and WebCompositorAnimation interfaces by using
cc::Animation class. It allows us to make WebCompositorAnimation non-public.
WebLayer animation dependencies will go away when we erase the old animation
system (it's already disabled). http://crbug.com/575041 

What's next (separate CLs):
- Remove Web prefixes on files
- Remove virtual functions (when gmock-based erased)
- Remove redundant enum declarations.


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

[loyso (OOO), 2016-01-28 00:26:31]

Description was changed from

==========
CC Animation: Move files from cc_blink to Source/platform/animation

- We introduce CompositorFactory singleton (works as Platform) to create CC
objects in blink. All animation methods are moved from CompositorSupport to
CompositorFactory. Other methods are also expected to be moved there.

- We decouple WebLayer and WebCompositorAnimation interfaces by using
cc::Animation class. It allows us to make WebCompositorAnimation non-public.
WebLayer animation dependencies will go away when we erase the old animation
system (it's already disabled). http://crbug.com/575041 

What's next (separate CLs):
- Remove Web prefixes on files
- Remove virtual functions (when gmock-based erased)
- Remove redundant enum declarations.


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

to

==========
CC Animation: Move files from cc_blink to Source/platform/animation

- We introduce CompositorFactory singleton (works as Platform) to create CC
objects in blink. All animation methods are moved from CompositorSupport to
CompositorFactory. Other methods are also expected to be moved there.

- We decouple WebLayer and WebCompositorAnimation interfaces by using
cc::Animation class. It allows us to make WebCompositorAnimation non-public.
WebLayer animation dependencies will go away when we erase the old animation
system (it's already disabled). http://crbug.com/575041 

What's next (separate CLs):
- Remove Web prefixes on files
- Remove virtual functions (when gmock-based erased)
- Remove redundant enum declarations.
- Erase the animation delegate adapter.

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

[loyso (OOO), 2016-01-28 00:27:39]

Description was changed from

==========
CC Animation: Move files from cc_blink to Source/platform/animation

- We introduce CompositorFactory singleton (works as Platform) to create CC
objects in blink. All animation methods are moved from CompositorSupport to
CompositorFactory. Other methods are also expected to be moved there.

- We decouple WebLayer and WebCompositorAnimation interfaces by using
cc::Animation class. It allows us to make WebCompositorAnimation non-public.
WebLayer animation dependencies will go away when we erase the old animation
system (it's already disabled). http://crbug.com/575041 

What's next (separate CLs):
- Remove Web prefixes on files
- Remove virtual functions (when gmock-based erased)
- Remove redundant enum declarations.
- Erase the animation delegate adapter.

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

to

==========
CC Animation: Move files from cc_blink to Source/platform/animation

- We introduce CompositorFactory singleton (works as Platform) to create CC
objects in blink. All animation methods are moved from CompositorSupport to
CompositorFactory. Other methods are also expected to be moved there.

- We decouple WebLayer and WebCompositorAnimation interfaces by using
cc::Animation class. It allows us to make WebCompositorAnimation non-public.
WebLayer animation dependencies will go away when we erase the old animation
system (it's already disabled). http://crbug.com/575041 

What's next (separate CLs):
- Remove Web prefixes on files
- Remove virtual functions (when gmock-based tests erased)
- Remove redundant enum declarations.
- Erase the animation delegate adapter.

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

[loyso (OOO), 2016-01-28 00:28:11]

Description was changed from

==========
CC Animation: Move files from cc_blink to Source/platform/animation

- We introduce CompositorFactory singleton (works as Platform) to create CC
objects in blink. All animation methods are moved from CompositorSupport to
CompositorFactory. Other methods are also expected to be moved there.

- We decouple WebLayer and WebCompositorAnimation interfaces by using
cc::Animation class. It allows us to make WebCompositorAnimation non-public.
WebLayer animation dependencies will go away when we erase the old animation
system (it's already disabled). http://crbug.com/575041 

What's next (separate CLs):
- Remove Web prefixes on files
- Remove virtual functions (when gmock-based tests erased)
- Remove redundant enum declarations.
- Erase the animation delegate adapter.

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

to

==========
CC Animation: Move files from cc_blink to Source/platform/animation

Disclaimer: This CL is only about moving files and sections of code.

- We introduce CompositorFactory singleton (works as Platform) to create CC
objects in blink. All animation methods are moved from CompositorSupport to
CompositorFactory. Other methods are also expected to be moved there.

- We decouple WebLayer and WebCompositorAnimation interfaces by using
cc::Animation class. It allows us to make WebCompositorAnimation non-public.
WebLayer animation dependencies will go away when we erase the old animation
system (it's already disabled). http://crbug.com/575041 

What's next (separate CLs):
- Remove Web prefixes on files
- Remove virtual functions (when gmock-based tests erased)
- Remove redundant enum declarations.
- Erase the animation delegate adapter.

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

[loyso (OOO), 2016-01-28 00:29:34]

Description was changed from

==========
CC Animation: Move files from cc_blink to Source/platform/animation

Disclaimer: This CL is only about moving files and sections of code.

- We introduce CompositorFactory singleton (works as Platform) to create CC
objects in blink. All animation methods are moved from CompositorSupport to
CompositorFactory. Other methods are also expected to be moved there.

- We decouple WebLayer and WebCompositorAnimation interfaces by using
cc::Animation class. It allows us to make WebCompositorAnimation non-public.
WebLayer animation dependencies will go away when we erase the old animation
system (it's already disabled). http://crbug.com/575041 

What's next (separate CLs):
- Remove Web prefixes on files
- Remove virtual functions (when gmock-based tests erased)
- Remove redundant enum declarations.
- Erase the animation delegate adapter.

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

to

==========
CC Animation: Move files from cc_blink to Source/platform/animation

Disclaimer: This CL is only about moving files and sections of code.

- We introduce CompositorFactory singleton (works as Platform) to create CC
objects in blink. All animation methods are moved from CompositorSupport to
CompositorFactory. Other methods are also expected to be moved there.

- We decouple WebLayer and WebCompositorAnimation interfaces by using
cc::Animation class. It allows us to make WebCompositorAnimation non-public.
WebLayer animation dependencies will go away when we erase the old animation
system (it's already disabled). http://crbug.com/575041 

- We move static asserts from content/child/assert_matching_enums.cc to
Source/platform/WebCompositorAnimation.cpp (so the enum becomes non-public)

What's next (separate CLs):
- Remove Web prefixes on files
- Remove virtual functions (when gmock-based tests erased)
- Remove redundant enum declarations.
- Erase the animation delegate adapter.

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

[esprehn, 2016-01-28 03:40:40]

This uses cc types in the web/ layer which isn't allowed, you need to push that
code down into platform/

This is looking really good though.

https://codereview.chromium.org/1616653002/diff/180001/cc/blink/web_to_cc_ani...
File cc/blink/web_to_cc_animation_delegate_adapter.cc (left):

https://codereview.chromium.org/1616653002/diff/180001/cc/blink/web_to_cc_ani...
cc/blink/web_to_cc_animation_delegate_adapter.cc:21: #if
WEB_COMPOSITOR_ANIMATION_DELEGATE_TAKES_GROUP
woah crazy ifdefs, what is this?

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/animation/WebCompositorAnimation.cpp
(right):

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/animation/WebCompositorAnimation.cpp:24:
#define STATIC_ASSERT_MATCHING_ENUM(expected, actual) \
we don't have a macro for this already?

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/animation/WebCompositorAnimation.cpp:48:
animationId = AnimationIdProvider::NextAnimationId();
animationId || ...; works too I think?

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/animation/WebCompositorAnimation.cpp:50:
groupId = AnimationIdProvider::NextGroupId();
ditto

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/animation/WebCompositorAnimation.cpp:84: {
move braces up for consistency

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/animation/WebCompositorAnimation.cpp:210:
break;
we should make our enums parallel and just cast here

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/animation/WebCompositorAnimation.cpp:231:
m_animation->set_needs_synchronized_start_time(true);
why do both of these need to call set_needs_synchronized_start_time?

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/animation/WebCompositorAnimation.h
(right):

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/animation/WebCompositorAnimation.h:47:
virtual int id();
why are these virtual now? I don't think you want any of this virtual. And I
think you can make them all inline too.

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/animation/WebCompositorAnimationCurve.cpp
(right):

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/animation/WebCompositorAnimationCurve.cpp:11:
scoped_ptr<cc::TimingFunction>
WebCompositorAnimationCurve::CreateTimingFunction(TimingFunctionType type)
lowercase

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/animation/WebCompositorAnimationPlayer.h
(right):

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/animation/WebCompositorAnimationPlayer.h:38:
virtual void detachLayer();
does this all need to be virtual still?

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/animation/WebCompositorAnimationTimeline.h
(right):

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/animation/WebCompositorAnimationTimeline.h:28:
virtual void playerAttached(const WebCompositorAnimationPlayerClient&);
why virtual?

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/animation/WebFloatAnimationCurve.cpp
(right):

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/animation/WebFloatAnimationCurve.cpp:77:
double y2)
I really hate this style, it leaves one arg way far on the edge and the others
here. :(

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/animation/WebScrollOffsetAnimationCurve.cpp
(right):

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/animation/WebScrollOffsetAnimationCurve.cpp:78:
m_curve->UpdateTarget(time, gfx::ScrollOffset(newTarget.x, newTarget.y));
we can probably add WebFoo to gfx::Foo conversion operators too (in a different
patch)

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/animation/WebScrollOffsetAnimationCurve.h
(right):

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/animation/WebScrollOffsetAnimationCurve.h:28:
WebScrollOffsetAnimationCurve(blink::WebFloatPoint, TimingFunctionType,
ScrollDurationBehavior);
no need to put blink:: on stuff in here

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/animation/WebTransformOperations.cpp
(right):

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/animation/WebTransformOperations.cpp:71:
WebTransformOperations::~WebTransformOperations()
maybe move this up with the constructor above?

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/LinkHighlightImpl.cpp (right):

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/LinkHighlightImpl.cpp:314:
m_contentLayer->layer()->addAnimation(animation->releaseCCAnimation());
the web layer isn't really supposed to use cc types.

[loyso (OOO), 2016-01-28 07:09:56]

On 2016/01/28 03:40:40, esprehn wrote:
> This uses cc types in the web/ layer which isn't allowed, you need to push
that
> code down into platform/
Nice! Let's continue tomorrow.

[loyso (OOO), 2016-01-28 07:10:09]

https://codereview.chromium.org/1616653002/diff/180001/cc/blink/web_to_cc_ani...
File cc/blink/web_to_cc_animation_delegate_adapter.cc (left):

https://codereview.chromium.org/1616653002/diff/180001/cc/blink/web_to_cc_ani...
cc/blink/web_to_cc_animation_delegate_adapter.cc:21: #if
WEB_COMPOSITOR_ANIMATION_DELEGATE_TAKES_GROUP
On 2016/01/28 03:40:39, esprehn wrote:
> woah crazy ifdefs, what is this?
Leftovers from old 3-side patches.

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/animation/WebCompositorAnimation.cpp
(right):

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/animation/WebCompositorAnimation.cpp:24:
#define STATIC_ASSERT_MATCHING_ENUM(expected, actual) \
On 2016/01/28 03:40:39, esprehn wrote:
> we don't have a macro for this already?
No, we don't. I have moved this code 'as-is' from assert_matching_enums.cc.
The whole point of this CL to eliminate these checks completely (get rid of
redundant enums in next CLs).

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/animation/WebCompositorAnimation.cpp:48:
animationId = AnimationIdProvider::NextAnimationId();
On 2016/01/28 03:40:39, esprehn wrote:
> animationId || ...; works too I think?
It would work and it's the same in terms of generated asm code. I think that
'if' is more readable.
This code was moved 'as-is' from web_compositor_animation_impl.cc.

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/animation/WebCompositorAnimation.cpp:84: {
On 2016/01/28 03:40:39, esprehn wrote:
> move braces up for consistency

Done.

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/animation/WebCompositorAnimation.cpp:210:
break;
On 2016/01/28 03:40:40, esprehn wrote:
> we should make our enums parallel and just cast here
Yeah, that's supposed to be the next step (a separate CL)

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/animation/WebCompositorAnimation.cpp:231:
m_animation->set_needs_synchronized_start_time(true);
On 2016/01/28 03:40:39, esprehn wrote:
> why do both of these need to call set_needs_synchronized_start_time?

'passAnimation' was copied 'as-is'. I want to preserve the behavior.
releaseCCAnimation is an intermediate code and marked for deletion (soon). See
WebLayer::addAnimation and WebLayer vs. WebCompositorAnimation decoupling
discussion.

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/animation/WebCompositorAnimation.h
(right):

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/animation/WebCompositorAnimation.h:47:
virtual int id();
On 2016/01/28 03:40:40, esprehn wrote:
> why are these virtual now? I don't think you want any of this virtual. And I
> think you can make them all inline too.
We have random gmock classes (not for all the public interfaces). As per the
disclaimer, I would like to delete virtuals in a separate CL.

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/animation/WebCompositorAnimationCurve.cpp
(right):

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/animation/WebCompositorAnimationCurve.cpp:11:
scoped_ptr<cc::TimingFunction>
WebCompositorAnimationCurve::CreateTimingFunction(TimingFunctionType type)
On 2016/01/28 03:40:40, esprehn wrote:
> lowercase

Done.

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/animation/WebCompositorAnimationTimeline.h
(right):

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/animation/WebCompositorAnimationTimeline.h:28:
virtual void playerAttached(const WebCompositorAnimationPlayerClient&);
On 2016/01/28 03:40:40, esprehn wrote:
> why virtual?
Copied 'as-is' and we have random mocks everywhere. I would like to deal with
virtuals and mocks in a separate CL.

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/animation/WebFloatAnimationCurve.cpp
(right):

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/animation/WebFloatAnimationCurve.cpp:77:
double y2)
On 2016/01/28 03:40:40, esprehn wrote:
> I really hate this style, it leaves one arg way far on the edge and the others
> here. :(

Done.

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/animation/WebScrollOffsetAnimationCurve.cpp
(right):

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/animation/WebScrollOffsetAnimationCurve.cpp:78:
m_curve->UpdateTarget(time, gfx::ScrollOffset(newTarget.x, newTarget.y));
On 2016/01/28 03:40:40, esprehn wrote:
> we can probably add WebFoo to gfx::Foo conversion operators too (in a
different
> patch)

Acknowledged.

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/animation/WebScrollOffsetAnimationCurve.h
(right):

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/animation/WebScrollOffsetAnimationCurve.h:28:
WebScrollOffsetAnimationCurve(blink::WebFloatPoint, TimingFunctionType,
ScrollDurationBehavior);
On 2016/01/28 03:40:40, esprehn wrote:
> no need to put blink:: on stuff in here

Done.

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/animation/WebTransformOperations.cpp
(right):

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/animation/WebTransformOperations.cpp:71:
WebTransformOperations::~WebTransformOperations()
On 2016/01/28 03:40:40, esprehn wrote:
> maybe move this up with the constructor above?

Done.

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/LinkHighlightImpl.cpp (right):

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/LinkHighlightImpl.cpp:314:
m_contentLayer->layer()->addAnimation(animation->releaseCCAnimation());
On 2016/01/28 03:40:40, esprehn wrote:
> the web layer isn't really supposed to use cc types.
This is intermediate state of things (see http://crbug.com/575041).
WebLayer::addAnimation and WebCompositorAnimation::releaseCCAnimation are marked
as deprecated and will be deleted on the 26th of February, 2016.

[jbroman, 2016-01-28 15:27:23]

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/animation/WebCompositorAnimation.cpp
(right):

https://codereview.chromium.org/1616653002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/animation/WebCompositorAnimation.cpp:48:
animationId = AnimationIdProvider::NextAnimationId();
On 2016/01/28 at 07:10:08, loyso wrote:
> On 2016/01/28 03:40:39, esprehn wrote:
> > animationId || ...; works too I think?
> It would work and it's the same in terms of generated asm code. I think that
'if' is more readable.
> This code was moved 'as-is' from web_compositor_animation_impl.cc.

Drive-by: no it wouldn't (unlike JavaScript, in C++ the default || operator acts
only on booleans). "0 || 4" evaluates to 1 (true), and does not produce a type
error.

[loyso (OOO), 2016-01-29 04:03:04]

Description was changed from

==========
CC Animation: Move files from cc_blink to Source/platform/animation

Disclaimer: This CL is only about moving files and sections of code.

- We introduce CompositorFactory singleton (works as Platform) to create CC
objects in blink. All animation methods are moved from CompositorSupport to
CompositorFactory. Other methods are also expected to be moved there.

- We decouple WebLayer and WebCompositorAnimation interfaces by using
cc::Animation class. It allows us to make WebCompositorAnimation non-public.
WebLayer animation dependencies will go away when we erase the old animation
system (it's already disabled). http://crbug.com/575041 

- We move static asserts from content/child/assert_matching_enums.cc to
Source/platform/WebCompositorAnimation.cpp (so the enum becomes non-public)

What's next (separate CLs):
- Remove Web prefixes on files
- Remove virtual functions (when gmock-based tests erased)
- Remove redundant enum declarations.
- Erase the animation delegate adapter.

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

to

==========
CC Animation: Move files from cc_blink to Source/platform/animation

Disclaimer: This CL is only about moving files and sections of code.

- We remove Web prefixes on files/classes and add Compositor prefix where
necessary

- We move exported/WebTransformKeyframe.cpp and exported/WebFilterKeyframe.cpp
to animation/ to group them with their headers.

- We move CompositorFilterOperations not to animation/ but to graphics/ folder,
because it's a property of WebLayer.

- We introduce CompositorFactory singleton (works as Platform) to create CC
objects in blink. All animation methods are moved from CompositorSupport to
CompositorFactory. Other methods are also expected to be moved there.

- We decouple WebLayer and WebCompositorAnimation interfaces by using
cc::Animation class. It allows us to make WebCompositorAnimation non-public.
WebLayer animation dependencies will go away when we erase the old animation
system (it's already disabled). http://crbug.com/575041 

- We move static asserts from content/child/assert_matching_enums.cc to
Source/platform/WebCompositorAnimation.cpp (so the enum becomes non-public)

What's next (separate CLs):
- Remove virtual functions (when gmock-based tests erased)
- Remove redundant enum declarations.
- Erase the animation delegate adapter.

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

[loyso (OOO), 2016-01-29 04:05:07]

Description was changed from

==========
CC Animation: Move files from cc_blink to Source/platform/animation

Disclaimer: This CL is only about moving files and sections of code.

- We remove Web prefixes on files/classes and add Compositor prefix where
necessary

- We move exported/WebTransformKeyframe.cpp and exported/WebFilterKeyframe.cpp
to animation/ to group them with their headers.

- We move CompositorFilterOperations not to animation/ but to graphics/ folder,
because it's a property of WebLayer.

- We introduce CompositorFactory singleton (works as Platform) to create CC
objects in blink. All animation methods are moved from CompositorSupport to
CompositorFactory. Other methods are also expected to be moved there.

- We decouple WebLayer and WebCompositorAnimation interfaces by using
cc::Animation class. It allows us to make WebCompositorAnimation non-public.
WebLayer animation dependencies will go away when we erase the old animation
system (it's already disabled). http://crbug.com/575041 

- We move static asserts from content/child/assert_matching_enums.cc to
Source/platform/WebCompositorAnimation.cpp (so the enum becomes non-public)

What's next (separate CLs):
- Remove virtual functions (when gmock-based tests erased)
- Remove redundant enum declarations.
- Erase the animation delegate adapter.

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

to

==========
CC Animation: Move files from cc_blink to Source/platform/animation

Disclaimer: This CL is only about moving files and sections of code.

We:

- Remove Web prefixes on files/classes and add Compositor prefix where necessary

- Move exported/WebTransformKeyframe.cpp and exported/WebFilterKeyframe.cpp to
animation/ to group them with their headers.

- Move CompositorFilterOperations not to animation/ but to graphics/ folder,
because it's a property of WebLayer.

- Introduce CompositorFactory singleton (works as Platform) to create CC objects
in blink. All animation methods are moved from CompositorSupport to
CompositorFactory. Other methods are also expected to be moved there.

- Decouple WebLayer and WebCompositorAnimation interfaces by using cc::Animation
class. It allows us to make WebCompositorAnimation non-public. WebLayer
animation dependencies will go away when we erase the old animation system (it's
already disabled). http://crbug.com/575041 

- Move static asserts from content/child/assert_matching_enums.cc to
Source/platform/WebCompositorAnimation.cpp (so the enum becomes non-public)

What's next (separate CLs):
- Remove virtual functions (when gmock-based tests erased)
- Remove redundant enum declarations (using alias).
- Erase the animation delegate adapter.

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

[loyso (OOO), 2016-01-29 05:52:46]

I have updated the description.

jbroman@, dstockwell@ - PTAL!

[jbroman, 2016-01-29 15:29:18]

A few comments (I've mostly looked at platform/ outside of platform/animation/
so far).

https://codereview.chromium.org/1616653002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/CompositorFactory.cpp (right):

https://codereview.chromium.org/1616653002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/CompositorFactory.cpp:80: void
CompositorFactory::initializeForTesting(CompositorFactory* factory)
Passing ownership through a raw pointer = :(.

https://codereview.chromium.org/1616653002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/CompositorFactory.cpp:88: delete s_factory;
Why is it necessary to shut down this factory?

https://codereview.chromium.org/1616653002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/CompositorFactory.cpp:94: ASSERT(s_factory);
How about something like:

// Does not assume ownership.
// Reset by passing nullptr after each test, before destroying the mock factory.
void CompositorFactory::setForTesting(CompositorFactory* factory)
{
    s_factoryForTesting = factory;
}

CompositorFactory& CompositorFactory::current()
{
    if (s_factoryForTesting)
        return s_factoryForTesting;
    DEFINE_STATIC_LOCAL(CompositorFactoryImpl, defaultFactory, ());
    return defaultFactory;
}

https://codereview.chromium.org/1616653002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/CompositorFactory.h (right):

https://codereview.chromium.org/1616653002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/CompositorFactory.h:8: #include
"base/memory/scoped_ptr.h"
This file doesn't seem to use scoped_ptr.

https://codereview.chromium.org/1616653002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/CompositorFactory.h:39: WebFloatPoint
targetValue,
Prefer FloatPoint to WebFloatPoint (here and elsewhere) now that this is no
longer part of the Blink public API.

https://codereview.chromium.org/1616653002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/exported/Platform.cpp (right):

https://codereview.chromium.org/1616653002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/exported/Platform.cpp:54:
CompositorFactory::initializeDefault();
Since this factory is no longer exposed outside Blink, it's a little odd to have
Platform setting it up.

https://codereview.chromium.org/1616653002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CompositorFilterOperations.h
(right):

https://codereview.chromium.org/1616653002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorFilterOperations.h:39:
class PLATFORM_EXPORT CompositorFilterOperations {
Is this virtual purely for the mock used in tests?

If so, I'd think it would be nicer to have this be non-virtual, and instead have
the tests query the actual data that was stored, like:

ASSERT_EQ(FilterOperation::createGrayscaleFilter(0.5),
outOps.asFilterOperations().at(0));

(That might be better as a follow-up patch, though.)

https://codereview.chromium.org/1616653002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorFilterOperations.h:55:
virtual void appendDropShadowFilter(WebPoint offset, float stdDeviation,
WebColor);
IntPoint, Color

https://codereview.chromium.org/1616653002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorFilterOperations.h:56:
virtual void appendColorMatrixFilter(SkScalar matrix[20]);
Is this called anywhere? I was wondering what we stored this as in Blink before
calling this, but I can't actually find any call sites of
appendColorMatrixFilter.

[loyso (OOO), 2016-02-01 05:31:33]

https://codereview.chromium.org/1616653002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/CompositorFactory.cpp (right):

https://codereview.chromium.org/1616653002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/CompositorFactory.cpp:80: void
CompositorFactory::initializeForTesting(CompositorFactory* factory)
On 2016/01/29 15:29:18, jbroman wrote:
> Passing ownership through a raw pointer = :(.
Actually, I have CompositorFactoryMock-type pointer owned by test fixture. But I
can change this to OwnPtr. Done.

https://codereview.chromium.org/1616653002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/CompositorFactory.cpp:88: delete s_factory;
On 2016/01/29 15:29:18, jbroman wrote:
> Why is it necessary to shut down this factory?
In this CL I want to preserve the behavior of CompositorSupport object.

https://codereview.chromium.org/1616653002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/CompositorFactory.cpp:94: ASSERT(s_factory);
On 2016/01/29 15:29:18, jbroman wrote:
> How about something like:
> 
> // Does not assume ownership.
> // Reset by passing nullptr after each test, before destroying the mock
factory.
> void CompositorFactory::setForTesting(CompositorFactory* factory)
> {
>     s_factoryForTesting = factory;
> }
> 
> CompositorFactory& CompositorFactory::current()
> {
>     if (s_factoryForTesting)
>         return s_factoryForTesting;
>     DEFINE_STATIC_LOCAL(CompositorFactoryImpl, defaultFactory, ());
>     return defaultFactory;
> }
Technically, this is an extra conditional branch in assembly code.
In general, this would be the Meyer's lazy initialized singleton with known
drawbacks.

I would like to keep following "the scope" idiom (so we know the init-time
explicitly and we can shutdown the subsystem). It starts to make sense if you
have non-trivial initialization and shutdown procedures. See WebLayer factory
and WebLayerImpl::SetLayerSettings, for example.

FTR, it looks like our blink (compositor animation?) tests are rather chaotic.
We call Platform::initialize multiple times without calling Platform::shutdown.
I would like to solve this problem as a separate CL.

In total, yes. We can go with STATIC_LOCAL in this case. No objections. Would be
nice to have a design policy for singletons+factories+TDD (IoC containers? :) )

https://codereview.chromium.org/1616653002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/CompositorFactory.h (right):

https://codereview.chromium.org/1616653002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/CompositorFactory.h:8: #include
"base/memory/scoped_ptr.h"
On 2016/01/29 15:29:18, jbroman wrote:
> This file doesn't seem to use scoped_ptr.
That is a garbage from my initial attempt to specify ownership in
initializeForTesting.

https://codereview.chromium.org/1616653002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/CompositorFactory.h:39: WebFloatPoint
targetValue,
On 2016/01/29 15:29:18, jbroman wrote:
> Prefer FloatPoint to WebFloatPoint (here and elsewhere) now that this is no
> longer part of the Blink public API.

Done.

https://codereview.chromium.org/1616653002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/animation/CompositorFilterKeyframe.h
(left):

https://codereview.chromium.org/1616653002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/animation/CompositorFilterKeyframe.h:38: #if
INSIDE_BLINK
This is internal blink header now. Do I need to erase INSIDE_BLINK (as always
'true') in this CL?

https://codereview.chromium.org/1616653002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/exported/Platform.cpp (right):

https://codereview.chromium.org/1616653002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/exported/Platform.cpp:54:
CompositorFactory::initializeDefault();
On 2016/01/29 15:29:18, jbroman wrote:
> Since this factory is no longer exposed outside Blink, it's a little odd to
have
> Platform setting it up.
It's still a part of the platform, though. Nevertheless, I want to preserve the
existing behavior in this CL (so we can enhance it later). It should match
CompositorSupport for now.

https://codereview.chromium.org/1616653002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CompositorFilterOperations.h
(right):

https://codereview.chromium.org/1616653002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorFilterOperations.h:39:
class PLATFORM_EXPORT CompositorFilterOperations {
On 2016/01/29 15:29:18, jbroman wrote:
> Is this virtual purely for the mock used in tests?
Yup.

> If so, I'd think it would be nicer to have this be non-virtual, and instead
have
> the tests query the actual data that was stored, like:
> 
> ASSERT_EQ(FilterOperation::createGrayscaleFilter(0.5),
> outOps.asFilterOperations().at(0));
> (That might be better as a follow-up patch, though.)

Yes, that was one of the intentions - to get rid of virtuals and inline where
critical. This CL is just a beginning.

https://codereview.chromium.org/1616653002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorFilterOperations.h:55:
virtual void appendDropShadowFilter(WebPoint offset, float stdDeviation,
WebColor);
On 2016/01/29 15:29:18, jbroman wrote:
> IntPoint, Color

Done.

https://codereview.chromium.org/1616653002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorFilterOperations.h:56:
virtual void appendColorMatrixFilter(SkScalar matrix[20]);
On 2016/01/29 15:29:18, jbroman wrote:
> Is this called anywhere? I was wondering what we stored this as in Blink
before
> calling this, but I can't actually find any call sites of
> appendColorMatrixFilter.
Maybe we wanted it to be implemented as accelerated animation. Should I erase it
in this CL?

[loyso (OOO), 2016-02-01 06:33:04]

DISALLOW_COPY_AND_ASSIGN -> WTF_MAKE_NONCOPYABLE

https://codereview.chromium.org/1616653002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/animation/CompositorFilterKeyframe.h
(left):

https://codereview.chromium.org/1616653002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/animation/CompositorFilterKeyframe.h:38: #if
INSIDE_BLINK
On 2016/02/01 05:31:32, loyso wrote:
> This is internal blink header now. Do I need to erase INSIDE_BLINK (as always
> 'true') in this CL?

Done.

[loyso (OOO), 2016-02-01 06:41:04]

Description was changed from

==========
CC Animation: Move files from cc_blink to Source/platform/animation

Disclaimer: This CL is only about moving files and sections of code.

We:

- Remove Web prefixes on files/classes and add Compositor prefix where necessary

- Move exported/WebTransformKeyframe.cpp and exported/WebFilterKeyframe.cpp to
animation/ to group them with their headers.

- Move CompositorFilterOperations not to animation/ but to graphics/ folder,
because it's a property of WebLayer.

- Introduce CompositorFactory singleton (works as Platform) to create CC objects
in blink. All animation methods are moved from CompositorSupport to
CompositorFactory. Other methods are also expected to be moved there.

- Decouple WebLayer and WebCompositorAnimation interfaces by using cc::Animation
class. It allows us to make WebCompositorAnimation non-public. WebLayer
animation dependencies will go away when we erase the old animation system (it's
already disabled). http://crbug.com/575041 

- Move static asserts from content/child/assert_matching_enums.cc to
Source/platform/WebCompositorAnimation.cpp (so the enum becomes non-public)

What's next (separate CLs):
- Remove virtual functions (when gmock-based tests erased)
- Remove redundant enum declarations (using alias).
- Erase the animation delegate adapter.

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

to

==========
CC Animation: Move files from cc_blink to Source/platform/animation

Disclaimer: This CL is only about moving files and sections of code.

We:

- Remove Web prefixes on files/classes and add Compositor prefix where
necessary.

- Move exported/WebTransformKeyframe.cpp and exported/WebFilterKeyframe.cpp
to animation/ to group them with their headers.

- Move CompositorFilterOperations not to animation/ but to graphics/ folder,
because it's a property of WebLayer.

- Introduce CompositorFactory singleton (works as Platform) to create CC objects
in blink. 
All animation methods are moved from CompositorSupport to CompositorFactory.
Other methods are also expected to be moved there.

- Decouple WebLayer and WebCompositorAnimation interfaces
by using cc::Animation class. 
It allows us to make WebCompositorAnimation non-public. 
WebLayer animation dependencies will go away when we erase the old animation
system
(it's already disabled). http://crbug.com/575041 

- Move static asserts from content/child/assert_matching_enums.cc to
Source/platform/WebCompositorAnimation.cpp (so the enum becomes non-public).

What's next (separate CLs):
- Remove virtual functions (when gmock-based tests erased)
- Remove redundant enum declarations (using alias).
- Erase the animation delegate adapter.

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

[jbroman, 2016-02-01 15:44:37]

Thanks for your patience (and thanks for doing this big chunk of work). A few
more thoughts.

https://codereview.chromium.org/1616653002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/CompositorFactory.cpp (right):

https://codereview.chromium.org/1616653002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/CompositorFactory.cpp:80: void
CompositorFactory::initializeForTesting(CompositorFactory* factory)
On 2016/02/01 at 05:31:32, loyso wrote:
> On 2016/01/29 15:29:18, jbroman wrote:
> > Passing ownership through a raw pointer = :(.
> Actually, I have CompositorFactoryMock-type pointer owned by test fixture. But
I can change this to OwnPtr. Done.

The delete calls make this an owning pointer, and it's confusing to have two
(exclusively) owning pointers at once. (If the test fixture owned it, I'd expect
the static pointer not to manage deletion.)

This looks fine now.

https://codereview.chromium.org/1616653002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/CompositorFactory.cpp:94: ASSERT(s_factory);
On 2016/02/01 at 05:31:32, loyso wrote:
> On 2016/01/29 15:29:18, jbroman wrote:
> > How about something like:
> > 
> > // Does not assume ownership.
> > // Reset by passing nullptr after each test, before destroying the mock
factory.
> > void CompositorFactory::setForTesting(CompositorFactory* factory)
> > {
> >     s_factoryForTesting = factory;
> > }
> > 
> > CompositorFactory& CompositorFactory::current()
> > {
> >     if (s_factoryForTesting)
> >         return s_factoryForTesting;
> >     DEFINE_STATIC_LOCAL(CompositorFactoryImpl, defaultFactory, ());
> >     return defaultFactory;
> > }
> Technically, this is an extra conditional branch in assembly code.

I'm not too worried about that (there's a lot of much more expensive stuff going
on at the same time), and I'd expect a branch predictor to be pretty good at
"this branch always goes right" if it did get hot.

> In general, this would be the Meyer's lazy initialized singleton with known
drawbacks.

Yeah, it's just a common pattern in Blink to have singletons using
DEFINE_STATIC_LOCAL.

> I would like to keep following "the scope" idiom (so we know the init-time
explicitly and we can shutdown the subsystem). It starts to make sense if you
have non-trivial initialization and shutdown procedures. See WebLayer factory
and WebLayerImpl::SetLayerSettings, for example.
> 
> FTR, it looks like our blink (compositor animation?) tests are rather chaotic.
We call Platform::initialize multiple times without calling Platform::shutdown.
I would like to solve this problem as a separate CL.

Ouch. With that in mind, addressing separately seems OK.

> In total, yes. We can go with STATIC_LOCAL in this case. No objections. Would
be nice to have a design policy for singletons+factories+TDD (IoC containers? :)
)

Unit testing is still fairly new in Blink, so there's not a lot of precedent for
unit testing infrastructure in general.

https://codereview.chromium.org/1616653002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CompositorFilterOperations.h
(right):

https://codereview.chromium.org/1616653002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorFilterOperations.h:56:
virtual void appendColorMatrixFilter(SkScalar matrix[20]);
On 2016/02/01 at 05:31:32, loyso wrote:
> On 2016/01/29 15:29:18, jbroman wrote:
> > Is this called anywhere? I was wondering what we stored this as in Blink
before
> > calling this, but I can't actually find any call sites of
> > appendColorMatrixFilter.
> Maybe we wanted it to be implemented as accelerated animation. Should I erase
it in this CL?

OK to leave it for now (since this is mostly a code-movement CL), but it would
be nice to remove dead code at some point. :)

https://codereview.chromium.org/1616653002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/CompositorFactory.h (right):

https://codereview.chromium.org/1616653002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/CompositorFactory.h:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
nit: This file probably belongs in platform/graphics/.

https://codereview.chromium.org/1616653002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/CompositorFactory.h:34: virtual
CompositorFilterAnimationCurve* createFilterAnimationCurve() { return nullptr; }
Please return smart pointers (i.e. PassOwnPtr<T>) from all factory methods
(which we can do now that this is inside Blink).

(You may need to include more headers or move default implementations to the
source file.)

https://codereview.chromium.org/1616653002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/CompositorFactory.h:57: }
super-nit: closing namespace comment please:

} // namespace blink

https://codereview.chromium.org/1616653002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CompositorFilterOperations.cpp
(right):

https://codereview.chromium.org/1616653002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorFilterOperations.cpp:8:
#include "third_party/skia/include/core/SkScalar.h"
nit: this include is redundant with the one in the header

https://codereview.chromium.org/1616653002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CompositorFilterOperations.h
(right):

https://codereview.chromium.org/1616653002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorFilterOperations.h:29:
#include "SkImageFilter.h"
use full include paths to Skia, like
"third_party/skia/include/core/SkImageFilter.h" (and re-sort)

(For SkImageFilter, you can probably get away with just a forward declaration,
but in this case I don't think it's a big deal either way.)

https://codereview.chromium.org/1616653002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/DEPS (left):

https://codereview.chromium.org/1616653002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/DEPS:1: include_rules = [
We could allow these in all of platform, but I'd prefer leaving these here and
adding an additional rule permitting "+cc", "-cc/blink" in platform/animation/.

That way we can more easily track which parts of platform/ are using the
compositor (and both animation and graphics seem reasonable).

https://codereview.chromium.org/1616653002/diff/280001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/filters/SkiaImageFilterBuilder.cpp
(right):

https://codereview.chromium.org/1616653002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/filters/SkiaImageFilterBuilder.cpp:160:
filters->appendDropShadowFilter(IntPoint(drop.x(), drop.y()),
drop.stdDeviation(), drop.color());
nit: drop.location() is already IntPoint(drop.x(), drop.y())

https://codereview.chromium.org/1616653002/diff/280001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp
(right):

https://codereview.chromium.org/1616653002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp:7:
#include "cc/animation/animation_player.h"
Are cc includes needed here?

[dstockwell, 2016-02-02 11:57:33]

The updates in core (mostly trivial for the changed class names) and the overall
plan lgtm.

[loyso (OOO), 2016-02-04 05:49:29]

https://codereview.chromium.org/1616653002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/CompositorFactory.h (right):

https://codereview.chromium.org/1616653002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/CompositorFactory.h:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
On 2016/02/01 15:44:36, jbroman wrote:
> nit: This file probably belongs in platform/graphics/.

Done.

https://codereview.chromium.org/1616653002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/CompositorFactory.h:34: virtual
CompositorFilterAnimationCurve* createFilterAnimationCurve() { return nullptr; }
On 2016/02/01 15:44:36, jbroman wrote:
> Please return smart pointers (i.e. PassOwnPtr<T>) from all factory methods
> (which we can do now that this is inside Blink).
> 
> (You may need to include more headers or move default implementations to the
> source file.)

I doesn't work with gmock overrides.
See the dependent CL as a demo: https://codereview.chromium.org/1663963004

    class CompositorFactoryMock : public CompositorFactory {
    public:
        MOCK_METHOD4(createAnimation, PassOwnPtr<CompositorAnimation>(const
CompositorAnimationCurve& curve, CompositorAnimation::TargetProperty target, int
groupId, int animationId));
        MOCK_METHOD0(createFloatAnimationCurve,
PassOwnPtr<CompositorFloatAnimationCurve>());
        MOCK_METHOD0(createAnimationPlayer,
PassOwnPtr<CompositorAnimationPlayer>());
        MOCK_METHOD0(createAnimationTimeline,
PassOwnPtr<CompositorAnimationTimeline>());
    };

    EXPECT_CALL(*m_mockCompositorFactory, createAnimationTimeline())
        .WillOnce(Return(adoptPtr(new WebCompositorAnimationTimelineMock())));

../../testing/gmock/include/gmock/gmock-actions.h:539:43: error: call to deleted
function 'operator new'
  explicit ReturnAction(R value) : value_(new R(internal::move(value))) {}
                                          ^~~
../../testing/gmock/include/gmock/gmock-actions.h:1062:10: note: in
instantiation of member function
'testing::internal::ReturnAction<WTF::PassOwnPtr<blink::WebCompositorAnimationTimelineMock>
>::ReturnAction' requested here
  return internal::ReturnAction<R>(internal::move(value));
         ^
../../third_party/WebKit/Source/core/animation/CompositorAnimationsTest.cpp:114:27:
note: in instantiation of function template specialization
'testing::Return<WTF::PassOwnPtr<blink::WebCompositorAnimationTimelineMock> >'
requested here
                .WillOnce(Return(adoptPtr(new
WebCompositorAnimationTimelineMock())));
                          ^
../../third_party/WebKit/Source/wtf/PassOwnPtr.h:42:5: note: candidate function
has been explicitly deleted
    DISALLOW_NEW();

Looks like gmock ReturnAction wants to allocate PassOwnPtr on a heap:

explicit ReturnAction(R value) : value_(new R(internal::move(value))) {}

which is disallowed:

template <typename T> class PassOwnPtr {
    DISALLOW_NEW();

Any ideas how to fix it?

https://codereview.chromium.org/1616653002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/CompositorFactory.h:57: }
On 2016/02/01 15:44:36, jbroman wrote:
> super-nit: closing namespace comment please:
> 
> } // namespace blink

Done.

https://codereview.chromium.org/1616653002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CompositorFilterOperations.cpp
(right):

https://codereview.chromium.org/1616653002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorFilterOperations.cpp:8:
#include "third_party/skia/include/core/SkScalar.h"
On 2016/02/01 15:44:36, jbroman wrote:
> nit: this include is redundant with the one in the header

Done.

https://codereview.chromium.org/1616653002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CompositorFilterOperations.h
(right):

https://codereview.chromium.org/1616653002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorFilterOperations.h:29:
#include "SkImageFilter.h"
On 2016/02/01 15:44:36, jbroman wrote:
> use full include paths to Skia, like
> "third_party/skia/include/core/SkImageFilter.h" (and re-sort)
> 
> (For SkImageFilter, you can probably get away with just a forward declaration,
> but in this case I don't think it's a big deal either way.)

Done.

https://codereview.chromium.org/1616653002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/DEPS (left):

https://codereview.chromium.org/1616653002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/DEPS:1: include_rules = [
On 2016/02/01 15:44:36, jbroman wrote:
> We could allow these in all of platform, but I'd prefer leaving these here and
> adding an additional rule permitting "+cc", "-cc/blink" in
platform/animation/.
> 
> That way we can more easily track which parts of platform/ are using the
> compositor (and both animation and graphics seem reasonable).

I have relaxed the DEPS as per discussion with esprehn@.
But I agree, keeping it strictly scoped is better for the code in transition.
Done.

https://codereview.chromium.org/1616653002/diff/280001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/filters/SkiaImageFilterBuilder.cpp
(right):

https://codereview.chromium.org/1616653002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/filters/SkiaImageFilterBuilder.cpp:160:
filters->appendDropShadowFilter(IntPoint(drop.x(), drop.y()),
drop.stdDeviation(), drop.color());
On 2016/02/01 15:44:36, jbroman wrote:
> nit: drop.location() is already IntPoint(drop.x(), drop.y())

Done.

https://codereview.chromium.org/1616653002/diff/280001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp
(right):

https://codereview.chromium.org/1616653002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp:7:
#include "cc/animation/animation_player.h"
On 2016/02/01 15:44:36, jbroman wrote:
> Are cc includes needed here?
I wanted to use forward declaration of cc::AnimationPlayer/Timeline for
blink::CompositorAnimationPlayer/CompositorAnimationTimeline (surprisingly,
clang and gcc tolerate scoped_refptr for a forward-declared class with intrusive
refcounted base). MSVC doesn't tolerate that, so it forces one to include CC's
AnimationPlayer header if you include CompositorAnimationPlayer.h.

That's weird. Let's include full headers then. Must work ok.

[jbroman, 2016-02-04 15:17:55]

platform/ (minus platform/animation/) lgtm

https://codereview.chromium.org/1616653002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/CompositorFactory.h (right):

https://codereview.chromium.org/1616653002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/CompositorFactory.h:34: virtual
CompositorFilterAnimationCurve* createFilterAnimationCurve() { return nullptr; }
On 2016/02/04 at 05:49:29, loyso wrote:
> On 2016/02/01 15:44:36, jbroman wrote:
> > Please return smart pointers (i.e. PassOwnPtr<T>) from all factory methods
> > (which we can do now that this is inside Blink).
> > 
> > (You may need to include more headers or move default implementations to the
> > source file.)
> 
> I doesn't work with gmock overrides.
> See the dependent CL as a demo: https://codereview.chromium.org/1663963004
> 
>     class CompositorFactoryMock : public CompositorFactory {
>     public:
>         MOCK_METHOD4(createAnimation, PassOwnPtr<CompositorAnimation>(const
CompositorAnimationCurve& curve, CompositorAnimation::TargetProperty target, int
groupId, int animationId));
>         MOCK_METHOD0(createFloatAnimationCurve,
PassOwnPtr<CompositorFloatAnimationCurve>());
>         MOCK_METHOD0(createAnimationPlayer,
PassOwnPtr<CompositorAnimationPlayer>());
>         MOCK_METHOD0(createAnimationTimeline,
PassOwnPtr<CompositorAnimationTimeline>());
>     };
> 
>     EXPECT_CALL(*m_mockCompositorFactory, createAnimationTimeline())
>         .WillOnce(Return(adoptPtr(new WebCompositorAnimationTimelineMock())));
> 
> ../../testing/gmock/include/gmock/gmock-actions.h:539:43: error: call to
deleted function 'operator new'
>   explicit ReturnAction(R value) : value_(new R(internal::move(value))) {}
>                                           ^~~
> ../../testing/gmock/include/gmock/gmock-actions.h:1062:10: note: in
instantiation of member function
'testing::internal::ReturnAction<WTF::PassOwnPtr<blink::WebCompositorAnimationTimelineMock>
>::ReturnAction' requested here
>   return internal::ReturnAction<R>(internal::move(value));
>          ^
>
../../third_party/WebKit/Source/core/animation/CompositorAnimationsTest.cpp:114:27:
note: in instantiation of function template specialization
'testing::Return<WTF::PassOwnPtr<blink::WebCompositorAnimationTimelineMock> >'
requested here
>                 .WillOnce(Return(adoptPtr(new
WebCompositorAnimationTimelineMock())));
>                           ^
> ../../third_party/WebKit/Source/wtf/PassOwnPtr.h:42:5: note: candidate
function has been explicitly deleted
>     DISALLOW_NEW();
> 
> Looks like gmock ReturnAction wants to allocate PassOwnPtr on a heap:
> 
> explicit ReturnAction(R value) : value_(new R(internal::move(value))) {}
> 
> which is disallowed:
> 
> template <typename T> class PassOwnPtr {
>     DISALLOW_NEW();
> 
> Any ideas how to fix it?

:( I hadn't realized someone had added that annotation to PassOwnPtr. IMHO, the
fix is probably to remove the DISALLOW_NEW annotation. I think it's entirely
reasonable to want to mock a function that returns a PassOwnPtr, and it's
reasonable for gmock to want to move the object to the heap as part of that.
That will need a wtf owner, though, so dealing with that separately SGTM.

[loyso (OOO), 2016-02-05 03:16:47]

esprehn@, PTAL! Final approval needed.

[loyso (OOO), 2016-02-08 00:48:00]

loyso@chromium.org changed reviewers:
+ aelias@chromium.org

[loyso (OOO), 2016-02-08 00:48:58]

aelias@, Source/web/ review needed! Thanks!

[aelias_OOO_until_Jul13, 2016-02-08 22:00:30]

Source/web lgtm

[esprehn, 2016-02-10 04:56:54]

Epic patch is epic!
http://i.imgur.com/nIIlchK.gif

This patch is so big it's hard to really review it deeply, but I think it's
probably okay. A good first step in tearing down the walls. I'm sure we'll
improve and simplify  as we go. :)

lgtm

https://codereview.chromium.org/1616653002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/animation/CompositorAnimation.h (right):

https://codereview.chromium.org/1616653002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/animation/CompositorAnimation.h:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
2016?

[loyso (OOO), 2016-02-10 22:52:09]

The CQ bit was checked by loyso@chromium.org

[loyso (OOO), 2016-02-10 22:52:09]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
vollick@chromium.org, dstockwell@chromium.org, jbroman@chromium.org,
aelias@chromium.org, esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/1616653002/#ps380001
(title: "Rebase. Fix copyrights and years.")

[commit-bot: I haz the power, 2016-02-10 22:55:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1616653002/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1616653002/380001

[commit-bot: I haz the power, 2016-02-10 23:54:41]

Description was changed from

==========
CC Animation: Move files from cc_blink to Source/platform/animation

Disclaimer: This CL is only about moving files and sections of code.

We:

- Remove Web prefixes on files/classes and add Compositor prefix where
necessary.

- Move exported/WebTransformKeyframe.cpp and exported/WebFilterKeyframe.cpp
to animation/ to group them with their headers.

- Move CompositorFilterOperations not to animation/ but to graphics/ folder,
because it's a property of WebLayer.

- Introduce CompositorFactory singleton (works as Platform) to create CC objects
in blink. 
All animation methods are moved from CompositorSupport to CompositorFactory.
Other methods are also expected to be moved there.

- Decouple WebLayer and WebCompositorAnimation interfaces
by using cc::Animation class. 
It allows us to make WebCompositorAnimation non-public. 
WebLayer animation dependencies will go away when we erase the old animation
system
(it's already disabled). http://crbug.com/575041 

- Move static asserts from content/child/assert_matching_enums.cc to
Source/platform/WebCompositorAnimation.cpp (so the enum becomes non-public).

What's next (separate CLs):
- Remove virtual functions (when gmock-based tests erased)
- Remove redundant enum declarations (using alias).
- Erase the animation delegate adapter.

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

to

==========
CC Animation: Move files from cc_blink to Source/platform/animation

Disclaimer: This CL is only about moving files and sections of code.

We:

- Remove Web prefixes on files/classes and add Compositor prefix where
necessary.

- Move exported/WebTransformKeyframe.cpp and exported/WebFilterKeyframe.cpp
to animation/ to group them with their headers.

- Move CompositorFilterOperations not to animation/ but to graphics/ folder,
because it's a property of WebLayer.

- Introduce CompositorFactory singleton (works as Platform) to create CC objects
in blink. 
All animation methods are moved from CompositorSupport to CompositorFactory.
Other methods are also expected to be moved there.

- Decouple WebLayer and WebCompositorAnimation interfaces
by using cc::Animation class. 
It allows us to make WebCompositorAnimation non-public. 
WebLayer animation dependencies will go away when we erase the old animation
system
(it's already disabled). http://crbug.com/575041 

- Move static asserts from content/child/assert_matching_enums.cc to
Source/platform/WebCompositorAnimation.cpp (so the enum becomes non-public).

What's next (separate CLs):
- Remove virtual functions (when gmock-based tests erased)
- Remove redundant enum declarations (using alias).
- Erase the animation delegate adapter.

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

[commit-bot: I haz the power, 2016-02-10 23:54:43]

Committed patchset #20 (id:380001)

[commit-bot: I haz the power, 2016-02-16 22:32:52]

Description was changed from

==========
CC Animation: Move files from cc_blink to Source/platform/animation

Disclaimer: This CL is only about moving files and sections of code.

We:

- Remove Web prefixes on files/classes and add Compositor prefix where
necessary.

- Move exported/WebTransformKeyframe.cpp and exported/WebFilterKeyframe.cpp
to animation/ to group them with their headers.

- Move CompositorFilterOperations not to animation/ but to graphics/ folder,
because it's a property of WebLayer.

- Introduce CompositorFactory singleton (works as Platform) to create CC objects
in blink. 
All animation methods are moved from CompositorSupport to CompositorFactory.
Other methods are also expected to be moved there.

- Decouple WebLayer and WebCompositorAnimation interfaces
by using cc::Animation class. 
It allows us to make WebCompositorAnimation non-public. 
WebLayer animation dependencies will go away when we erase the old animation
system
(it's already disabled). http://crbug.com/575041 

- Move static asserts from content/child/assert_matching_enums.cc to
Source/platform/WebCompositorAnimation.cpp (so the enum becomes non-public).

What's next (separate CLs):
- Remove virtual functions (when gmock-based tests erased)
- Remove redundant enum declarations (using alias).
- Erase the animation delegate adapter.

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

to

==========
CC Animation: Move files from cc_blink to Source/platform/animation

Disclaimer: This CL is only about moving files and sections of code.

We:

- Remove Web prefixes on files/classes and add Compositor prefix where
necessary.

- Move exported/WebTransformKeyframe.cpp and exported/WebFilterKeyframe.cpp
to animation/ to group them with their headers.

- Move CompositorFilterOperations not to animation/ but to graphics/ folder,
because it's a property of WebLayer.

- Introduce CompositorFactory singleton (works as Platform) to create CC objects
in blink.
All animation methods are moved from CompositorSupport to CompositorFactory.
Other methods are also expected to be moved there.

- Decouple WebLayer and WebCompositorAnimation interfaces
by using cc::Animation class.
It allows us to make WebCompositorAnimation non-public.
WebLayer animation dependencies will go away when we erase the old animation
system
(it's already disabled). http://crbug.com/575041

- Move static asserts from content/child/assert_matching_enums.cc to
Source/platform/WebCompositorAnimation.cpp (so the enum becomes non-public).

What's next (separate CLs):
- Remove virtual functions (when gmock-based tests erased)
- Remove redundant enum declarations (using alias).
- Erase the animation delegate adapter.

BUG=577016
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/26f3c5504044531a5772be2009dab72d94ade0cd
Cr-Commit-Position: refs/heads/master@{#374792}
==========

[commit-bot: I haz the power, 2016-02-16 22:32:54]

Patchset 20 (id:??) landed as
https://crrev.com/26f3c5504044531a5772be2009dab72d94ade0cd
Cr-Commit-Position: refs/heads/master@{#374792}