Make all LayerAnimationElement::Create*Element return unique_ptr

Also made change to LayerAnimationSequence, replace raw pointer
with unique_ptr to stress ownership transfer behavior.

And replace linked_ptr with unique_ptr in LayerAnimationSequence.

BUG=556939
Committed: https://crrev.com/bd5087e031d189ae2292b7cd05d846961ae086a5
Cr-Commit-Position: refs/heads/master@{#439326}

[Sunny, 2016-12-03 17:08:01]

Description was changed from

==========
Use unique_ptr instead of linked_ptr in layer_animation_sequence

Also remove the unnecessary use of std::unique_ptr in
ash/wm/window_animations.cc and session_state_animator_impl.cc

BUG=556939
==========

to

==========
Use unique_ptr instead of linked_ptr in layer_animation_sequence

Also remove the unnecessary use of std::unique_ptr in
ash/wm/window_animations.cc and session_state_animator_impl.cc

BUG=556939
==========

[Sunny, 2016-12-03 17:08:01]

ratsunny@gmail.com changed reviewers:
+ loyso@chromium.org, sky@chromium.org

[loyso (OOO), 2016-12-06 04:21:56]

https://codereview.chromium.org/2550933002/diff/1/ash/wm/session_state_animat...
File ash/wm/session_state_animator_impl.cc (right):

https://codereview.chromium.org/2550933002/diff/1/ash/wm/session_state_animat...
ash/wm/session_state_animator_impl.cc:257: ui::LayerAnimationSequence*
brightness_sequence =
Use auto and MakeUnique and std::move here and everywhere, please.

https://codereview.chromium.org/2550933002/diff/1/ui/compositor/layer_animati...
File ui/compositor/layer_animation_sequence.cc (right):

https://codereview.chromium.org/2550933002/diff/1/ui/compositor/layer_animati...
ui/compositor/layer_animation_sequence.cc:185: void
LayerAnimationSequence::AddElement(LayerAnimationElement* element) {
AddElement(std::unique_ptr<LayerAnimationElement> element) ?

https://codereview.chromium.org/2550933002/diff/1/ui/compositor/layer_animati...
ui/compositor/layer_animation_sequence.cc:187: elements_.emplace_back(element);
push_back(std::move(element))

[loyso (OOO), 2016-12-06 04:25:17]

https://codereview.chromium.org/2550933002/diff/1/ash/wm/session_state_animat...
File ash/wm/session_state_animator_impl.cc (right):

https://codereview.chromium.org/2550933002/diff/1/ash/wm/session_state_animat...
ash/wm/session_state_animator_impl.cc:263:
ui::LayerAnimationElement::CreateBrightnessElement(target, duration);
It's better for Create*Element methods to use move semantics as well.

[Sunny, 2016-12-06 06:21:05]

https://codereview.chromium.org/2550933002/diff/1/ash/wm/session_state_animat...
File ash/wm/session_state_animator_impl.cc (right):

https://codereview.chromium.org/2550933002/diff/1/ash/wm/session_state_animat...
ash/wm/session_state_animator_impl.cc:257: ui::LayerAnimationSequence*
brightness_sequence =
On 2016/12/06 04:21:56, loyso wrote:
> Use auto and MakeUnique and std::move here and everywhere, please.

Hi, the variable "brightness_sequence" here is only used for add one element and
then been added to std::vector<ui::LayerAnimationSequence*>, since the logic
here is simple and there is RAW pointer in vector, is it worthwhile to use
std::unique to wrap each raw pointer?

Also I've search across the chromium codebase, seems no one use
std::make_unique<>(), and it's a C++14 feature, shall we use it?

Thanks!

https://codereview.chromium.org/2550933002/diff/1/ash/wm/session_state_animat...
ash/wm/session_state_animator_impl.cc:263:
ui::LayerAnimationElement::CreateBrightnessElement(target, duration);
On 2016/12/06 04:25:17, loyso wrote:
> It's better for Create*Element methods to use move semantics as well.

Same question, is it worthwhile to wrap raw pointer with std::unique or are
there any guide in chromium team advise to use unique_ptr instead of raw pointer
everywhere?

Thanks

[loyso (OOO), 2016-12-06 06:32:35]

https://codereview.chromium.org/2550933002/diff/1/ash/wm/session_state_animat...
File ash/wm/session_state_animator_impl.cc (right):

https://codereview.chromium.org/2550933002/diff/1/ash/wm/session_state_animat...
ash/wm/session_state_animator_impl.cc:257: ui::LayerAnimationSequence*
brightness_sequence =
Thanks for working on this, Sunny!

On 2016/12/06 at 06:21:05, Sunny wrote:
> On 2016/12/06 04:21:56, loyso wrote:
> > Use auto and MakeUnique and std::move here and everywhere, please.
> Hi, the variable "brightness_sequence" here is only used for add one element
and then been added to std::vector<ui::LayerAnimationSequence*>, since the logic
here is simple and there is RAW pointer in vector, is it worthwhile to use
std::unique to wrap each raw pointer?
Yes, the type checking should be pervasive and total. It doesn't work otherwise.
https://google.github.io/styleguide/cppguide.html#Ownership_and_Smart_Pointers
Discussion:
https://groups.google.com/a/chromium.org/forum/#!topic/cxx/aT2wsBLKvzI

> Also I've search across the chromium codebase, seems no one use
std::make_unique<>(), and it's a C++14 feature, shall we use it?
Search for base::MakeUnique instead. See this discussion if interested:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/iQgMedVA8-k/u7HSa...

[loyso (OOO), 2016-12-06 06:50:53]

Yeah. You can limit this CL to move semantics for LayerAnimationElement only.
Leave LayerAnimationSequence as-is (for a separate CL by someone).

[Sunny, 2016-12-07 17:57:21]

Description was changed from

==========
Use unique_ptr instead of linked_ptr in layer_animation_sequence

Also remove the unnecessary use of std::unique_ptr in
ash/wm/window_animations.cc and session_state_animator_impl.cc

BUG=556939
==========

to

==========
Make all LayerAnimationElement::Create*Element return unique_ptr

Also made change to LayerAnimationSequence, replace raw pointer
with unique_ptr to stress ownership transfer behavior.

And replace linked_ptr with unique_ptr in LayerAnimationSequence.

BUG=556939
==========

[Sunny, 2016-12-07 17:57:41]

Description was changed from

==========
Make all LayerAnimationElement::Create*Element return unique_ptr

Also made change to LayerAnimationSequence, replace raw pointer
with unique_ptr to stress ownership transfer behavior.

And replace linked_ptr with unique_ptr in LayerAnimationSequence.

BUG=556939
==========

to

==========
Also made change to LayerAnimationSequence, replace raw pointer
with unique_ptr to stress ownership transfer behavior.

And replace linked_ptr with unique_ptr in LayerAnimationSequence.

BUG=556939
==========

[Sunny, 2016-12-07 17:59:01]

On 2016/12/06 06:50:53, loyso wrote:
> Yeah. You can limit this CL to move semantics for LayerAnimationElement only.
> Leave LayerAnimationSequence as-is (for a separate CL by someone).

Hi, loyso

Thanks for your guiding, could you take a look at my new patch?

[loyso (OOO), 2016-12-08 00:05:21]

I would use auto for Create*Element results but I know that sky@ prefers
explicitness. I leave it for sky@'s review.

Looks very good! See a couple of nits (use move instead of .release())

https://codereview.chromium.org/2550933002/diff/20001/ash/wm/window_animation...
File ash/wm/window_animations.cc (right):

https://codereview.chromium.org/2550933002/diff/20001/ash/wm/window_animation...
ash/wm/window_animations.cc:117: scale.release(), duration);
std::move(scale) ?

https://codereview.chromium.org/2550933002/diff/20001/ui/wm/core/window_anima...
File ui/wm/core/window_animations.cc (right):

https://codereview.chromium.org/2550933002/diff/20001/ui/wm/core/window_anima...
ui/wm/core/window_animations.cc:479: rotation.release(), duration);
std::move ?

[loyso (OOO), 2016-12-08 00:05:37]

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

[commit-bot: I haz the power, 2016-12-08 00:06:01]

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

[commit-bot: I haz the power, 2016-12-08 00:20:42]

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

[commit-bot: I haz the power, 2016-12-08 00:20:44]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sunny, 2016-12-08 15:35:44]

On 2016/12/08 00:05:21, loyso wrote:
> I would use auto for Create*Element results but I know that sky@ prefers
> explicitness. I leave it for sky@'s review.
> 
> Looks very good! See a couple of nits (use move instead of .release())
> 
>
https://codereview.chromium.org/2550933002/diff/20001/ash/wm/window_animation...
> File ash/wm/window_animations.cc (right):
> 
>
https://codereview.chromium.org/2550933002/diff/20001/ash/wm/window_animation...
> ash/wm/window_animations.cc:117: scale.release(), duration);
> std::move(scale) ?
> 
>
https://codereview.chromium.org/2550933002/diff/20001/ui/wm/core/window_anima...
> File ui/wm/core/window_animations.cc (right):
> 
>
https://codereview.chromium.org/2550933002/diff/20001/ui/wm/core/window_anima...
> ui/wm/core/window_animations.cc:479: rotation.release(), duration);
> std::move ?

Hi, loyso@

Thanks for your advice

I changed more files to make "scale" and "rotation" movable, use explicit
declaration instead of auto,
and fix build on linux and windows (in my mind).

Build passed on mac but not tested on linux and windows. Could you take a look
and do dry run again?

Thanks in advance

[loyso (OOO), 2016-12-08 23:16:57]

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

[commit-bot: I haz the power, 2016-12-08 23:17:42]

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

[commit-bot: I haz the power, 2016-12-08 23:20:14]

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

[commit-bot: I haz the power, 2016-12-08 23:20:15]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Sunny, 2016-12-09 09:33:16]

On 2016/12/08 00:05:21, loyso wrote:
> I would use auto for Create*Element results but I know that sky@ prefers
> explicitness. I leave it for sky@'s review.
> 
> Looks very good! See a couple of nits (use move instead of .release())
> 
>
https://codereview.chromium.org/2550933002/diff/20001/ash/wm/window_animation...
> File ash/wm/window_animations.cc (right):
> 
>
https://codereview.chromium.org/2550933002/diff/20001/ash/wm/window_animation...
> ash/wm/window_animations.cc:117: scale.release(), duration);
> std::move(scale) ?
> 
>
https://codereview.chromium.org/2550933002/diff/20001/ui/wm/core/window_anima...
> File ui/wm/core/window_animations.cc (right):
> 
>
https://codereview.chromium.org/2550933002/diff/20001/ui/wm/core/window_anima...
> ui/wm/core/window_animations.cc:479: rotation.release(), duration);
> std::move ?

Sorry I use the codebase fetched a week ago to compile and test, it seems
conflict with my another patch
on file "ui/gfx/interpolated_transform.cc".

I've fixed this issue and request another dry run, thanks in advance 😂

[loyso (OOO), 2016-12-11 23:31:54]

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

[commit-bot: I haz the power, 2016-12-11 23:32:08]

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

[commit-bot: I haz the power, 2016-12-11 23:49:17]

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

[commit-bot: I haz the power, 2016-12-11 23:49:18]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sunny, 2016-12-12 10:38:43]

Hi loyso@

New patch uploaded, and that's too bad I don't have windows and linux compile
environment, so I always missing things which would be ignored on mac build,
could you give me a l-g-t-m to let me use TBR=loyso@?

Thanks

[loyso (OOO), 2016-12-12 23:23:42]

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

[commit-bot: I haz the power, 2016-12-12 23:25:17]

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

[commit-bot: I haz the power, 2016-12-13 01:38:46]

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

[commit-bot: I haz the power, 2016-12-13 01:38:47]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[loyso (OOO), 2016-12-13 02:24:50]

ui/compositor/ *animat* LGTM with a nit:

https://codereview.chromium.org/2550933002/diff/80001/ui/compositor/layer_ani...
File ui/compositor/layer_animation_element.cc (right):

https://codereview.chromium.org/2550933002/diff/80001/ui/compositor/layer_ani...
ui/compositor/layer_animation_element.cc:604: return
base::MakeUnique<ThreadedTransformTransition>(transform, duration);
Looks like this file doesn't 
#include "base/memory/ptr_util.h"
for base::MakeUnique.
Could you make sure that we include <memory> for unique_ptr and ptr_util.h for
MakeUnique?
Thanks in advance!

[loyso (OOO), 2016-12-13 02:26:54]

https://codereview.chromium.org/2550933002/diff/80001/ui/compositor/layer_ani...
File ui/compositor/layer_animation_sequence.h (left):

https://codereview.chromium.org/2550933002/diff/80001/ui/compositor/layer_ani...
ui/compositor/layer_animation_sequence.h:140: typedef
std::vector<linked_ptr<LayerAnimationElement> > Elements;
Could you erase linked_ptr include dependency? It was erased in previous
patchsets, AFAIR.

[Sunny, 2016-12-15 08:26:25]

On 2016/12/13 02:26:54, loyso wrote:
>
https://codereview.chromium.org/2550933002/diff/80001/ui/compositor/layer_ani...
> File ui/compositor/layer_animation_sequence.h (left):
> 
>
https://codereview.chromium.org/2550933002/diff/80001/ui/compositor/layer_ani...
> ui/compositor/layer_animation_sequence.h:140: typedef
> std::vector<linked_ptr<LayerAnimationElement> > Elements;
> Could you erase linked_ptr include dependency? It was erased in previous
> patchsets, AFAIR.

Hi loyso@, sky@, I've added all missing includes, could you take a look?

Thanks~

[sky, 2016-12-15 16:42:18]

LGTM

https://codereview.chromium.org/2550933002/diff/100001/ui/compositor/layer_an...
File ui/compositor/layer_animation_sequence.cc (right):

https://codereview.chromium.org/2550933002/diff/100001/ui/compositor/layer_an...
ui/compositor/layer_animation_sequence.cc:189:
elements_.push_back(std::move(element));
push_back->emplace_back?

[Sunny, 2016-12-16 10:53:49]

https://codereview.chromium.org/2550933002/diff/100001/ui/compositor/layer_an...
File ui/compositor/layer_animation_sequence.cc (right):

https://codereview.chromium.org/2550933002/diff/100001/ui/compositor/layer_an...
ui/compositor/layer_animation_sequence.cc:189:
elements_.push_back(std::move(element));
On 2016/12/15 16:42:18, sky wrote:
> push_back->emplace_back?

Emm...Do you prefer emplace_back(std::move(element)) or
emplace_back(element.release())?

[danakj, 2016-12-16 14:20:31]

driveby thots

https://codereview.chromium.org/2550933002/diff/100001/ui/compositor/layer_an...
File ui/compositor/layer_animation_sequence.cc (right):

https://codereview.chromium.org/2550933002/diff/100001/ui/compositor/layer_an...
ui/compositor/layer_animation_sequence.cc:189:
elements_.push_back(std::move(element));
On 2016/12/16 10:53:49, Sunny wrote:
> On 2016/12/15 16:42:18, sky wrote:
> > push_back->emplace_back?
> 
> Emm...Do you prefer emplace_back(std::move(element)) or
> emplace_back(element.release())?

Never use release() unless you really have to. Since you have the type that is
being stored in the array already push_back should make sense here IMO.
emplace_back would be for constructing a new type from a constructor call (not a
copy/move constructor).

[sky, 2016-12-16 17:13:06]

LGTM then

[Sunny, 2016-12-17 03:46:09]

The CQ bit was checked by ratsunny@gmail.com

[Sunny, 2016-12-17 03:46:10]

The patchset sent to the CQ was uploaded after l-g-t-m from loyso@chromium.org
Link to the patchset: https://codereview.chromium.org/2550933002/#ps100001
(title: "Complete inclusion")

[commit-bot: I haz the power, 2016-12-17 03:46:23]

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

[commit-bot: I haz the power, 2016-12-17 05:19:28]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481946369925740,
"parent_rev": "02bc2dec2b2afe7def9b90de2dfaa447c5271c70", "commit_rev":
"5e6ba6896734bd23b47c38aa5c9c45c801810b05"}

[commit-bot: I haz the power, 2016-12-17 05:20:29]

Description was changed from

==========
Also made change to LayerAnimationSequence, replace raw pointer
with unique_ptr to stress ownership transfer behavior.

And replace linked_ptr with unique_ptr in LayerAnimationSequence.

BUG=556939
==========

to

==========
Also made change to LayerAnimationSequence, replace raw pointer
with unique_ptr to stress ownership transfer behavior.

And replace linked_ptr with unique_ptr in LayerAnimationSequence.

BUG=556939

Review-Url: https://codereview.chromium.org/2550933002
==========

[commit-bot: I haz the power, 2016-12-17 05:20:30]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-12-17 05:22:49]

Description was changed from

==========
Also made change to LayerAnimationSequence, replace raw pointer
with unique_ptr to stress ownership transfer behavior.

And replace linked_ptr with unique_ptr in LayerAnimationSequence.

BUG=556939

Review-Url: https://codereview.chromium.org/2550933002
==========

to

==========
Also made change to LayerAnimationSequence, replace raw pointer
with unique_ptr to stress ownership transfer behavior.

And replace linked_ptr with unique_ptr in LayerAnimationSequence.

BUG=556939

Committed: https://crrev.com/bd5087e031d189ae2292b7cd05d846961ae086a5
Cr-Commit-Position: refs/heads/master@{#439326}
==========

[commit-bot: I haz the power, 2016-12-17 05:22:50]

Patchset 6 (id:??) landed as
https://crrev.com/bd5087e031d189ae2292b7cd05d846961ae086a5
Cr-Commit-Position: refs/heads/master@{#439326}