[ash-md] Remove the number of animators used for the Shelf animations.

[ash-md] Remove the number of animators used for the Shelf animations.

Previously the ShelfBackgroundAnimator was using 2 BackgroundAnimators, this CL
replaces (and removes) the BackgroundAnimator with a single SlideAnimation.

BUG=595010
TEST=ash_unittests

Review-Url: https://codereview.chromium.org/2679333002
Cr-Commit-Position: refs/heads/master@{#450016}
Committed: https://chromium.googlesource.com/chromium/src/+/ed35a0fe3dbb990c4782489c3982de49fa6f9059

[bruthig, 2017-02-07 21:36:00]

Description was changed from

==========
[ash-md] Remove the number of animators used for the Shelf animations.

Previously the ShelfBackgroundAnimator was using 2 BackgroundAnimators, this CL
replaces (and removes) the BackgroundAnimator with a single SlideAnimation.

BUG=595010
TEST=ash_unittests, gfx_unittests
==========

to

==========
[ash-md] Remove the number of animators used for the Shelf animations.

***NOTE: This CL depends on: https://codereview.chromium.org/2680113002/

Previously the ShelfBackgroundAnimator was using 2 BackgroundAnimators, this CL
replaces (and removes) the BackgroundAnimator with a single SlideAnimation.

BUG=595010
TEST=ash_unittests, gfx_unittests
==========

[bruthig, 2017-02-07 21:36:48]

bruthig@chromium.org changed reviewers:
+ jamescook@chromium.org, sky@chromium.org

[bruthig, 2017-02-07 21:36:48]

James, can you please look at ash/* changes?

Scott, can you please look at ui/* changes?

[James Cook, 2017-02-08 00:31:28]

https://codereview.chromium.org/2679333002/diff/1/ash/BUILD.gn
File ash/BUILD.gn (left):

https://codereview.chromium.org/2679333002/diff/1/ash/BUILD.gn#oldcode469
ash/BUILD.gn:469: "common/wm/background_animator.h",
Was this file actually deleted in this CL?

https://codereview.chromium.org/2679333002/diff/1/ash/common/shelf/shelf_back...
File ash/common/shelf/shelf_background_animator.cc (right):

https://codereview.chromium.org/2679333002/diff/1/ash/common/shelf/shelf_back...
ash/common/shelf/shelf_background_animator.cc:17: const int
ShelfBackgroundAnimator::kMaxAlpha = 255;
Can this be initialized in header?

https://codereview.chromium.org/2679333002/diff/1/ash/common/shelf/shelf_back...
ash/common/shelf/shelf_background_animator.cc:109: if (animator_->IsShowing()) {
You're checking for null animator_ on line 103. Can animator_ ever be null here?
If not, a comment somewhere would be nice.

https://codereview.chromium.org/2679333002/diff/1/ash/common/shelf/shelf_back...
File ash/common/shelf/shelf_background_animator.h (right):

https://codereview.chromium.org/2679333002/diff/1/ash/common/shelf/shelf_back...
ash/common/shelf/shelf_background_animator.h:88: struct AnimationValues {
nit: if all the members are private, why not class instead of struct?

https://codereview.chromium.org/2679333002/diff/1/ash/common/shelf/shelf_cons...
File ash/common/shelf/shelf_constants.cc (right):

https://codereview.chromium.org/2679333002/diff/1/ash/common/shelf/shelf_cons...
ash/common/shelf/shelf_constants.cc:24: const int kShelfTranslucentAlpha = 204;
Isn't 204 the value for NON_MATERIAL?

https://codereview.chromium.org/2679333002/diff/1/ash/common/shelf/shelf_layo...
File ash/common/shelf/shelf_layout_manager.h (right):

https://codereview.chromium.org/2679333002/diff/1/ash/common/shelf/shelf_layo...
ash/common/shelf/shelf_layout_manager.h:244: void
MaybeUpdateShelfBackground(gfx::AnimationChangeType change_type);
#include animation_change_type.h?

https://codereview.chromium.org/2679333002/diff/1/ash/common/shelf/shelf_widg...
File ash/common/shelf/shelf_widget.h (right):

https://codereview.chromium.org/2679333002/diff/1/ash/common/shelf/shelf_widg...
ash/common/shelf/shelf_widget.h:49: gfx::AnimationChangeType change_type);
#include for this?

or you could use enum class which would let you forward declare it

https://codereview.chromium.org/2679333002/diff/1/ui/gfx/animation/slide_anim...
File ui/gfx/animation/slide_animation.cc (right):

https://codereview.chromium.org/2679333002/diff/1/ui/gfx/animation/slide_anim...
ui/gfx/animation/slide_animation.cc:53: delegate()->AnimationProgressed(this);
Do you know offhand if any of Chrome's other users of SlideAnimation depend on
duration 0?

[sky, 2017-02-08 03:02:56]

https://codereview.chromium.org/2679333002/diff/1/ui/gfx/animation/slide_anim...
File ui/gfx/animation/slide_animation.h (right):

https://codereview.chromium.org/2679333002/diff/1/ui/gfx/animation/slide_anim...
ui/gfx/animation/slide_animation.h:61: void ShowImmediately();
Can you motivate why you want these? I would prefer client code deal with this
situation rather than complicating the animation code. By that I mean if the
client code knows to call this, then presumably the client can take whatever
action completely immediately would.

[bruthig, 2017-02-10 15:32:25]

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

[commit-bot: I haz the power, 2017-02-10 15:32:48]

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, 2017-02-10 17:16:28]

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

[commit-bot: I haz the power, 2017-02-10 17:16:29]

Dry run: This issue passed the CQ dry run.

[bruthig, 2017-02-10 18:49:10]

Scott, James, I've addressed your comments and re-worked the CL a little bit.

James, can you look at:
- ash/*

Scott, can you look at:
- ui/*

https://codereview.chromium.org/2679333002/diff/1/ash/BUILD.gn
File ash/BUILD.gn (left):

https://codereview.chromium.org/2679333002/diff/1/ash/BUILD.gn#oldcode469
ash/BUILD.gn:469: "common/wm/background_animator.h",
On 2017/02/08 00:31:27, James Cook wrote:
> Was this file actually deleted in this CL?

Whoops, don't know how that happened.  Good catch, thx!

https://codereview.chromium.org/2679333002/diff/1/ash/common/shelf/shelf_back...
File ash/common/shelf/shelf_background_animator.cc (right):

https://codereview.chromium.org/2679333002/diff/1/ash/common/shelf/shelf_back...
ash/common/shelf/shelf_background_animator.cc:17: const int
ShelfBackgroundAnimator::kMaxAlpha = 255;
On 2017/02/08 00:31:27, James Cook wrote:
> Can this be initialized in header?

Done.

https://codereview.chromium.org/2679333002/diff/1/ash/common/shelf/shelf_back...
ash/common/shelf/shelf_background_animator.cc:109: if (animator_->IsShowing()) {
On 2017/02/08 00:31:27, James Cook wrote:
> You're checking for null animator_ on line 103. Can animator_ ever be null
here?
> If not, a comment somewhere would be nice.

Comment added, WDYT?

https://codereview.chromium.org/2679333002/diff/1/ash/common/shelf/shelf_back...
File ash/common/shelf/shelf_background_animator.h (right):

https://codereview.chromium.org/2679333002/diff/1/ash/common/shelf/shelf_back...
ash/common/shelf/shelf_background_animator.h:88: struct AnimationValues {
On 2017/02/08 00:31:27, James Cook wrote:
> nit: if all the members are private, why not class instead of struct?

Done.

https://codereview.chromium.org/2679333002/diff/1/ash/common/shelf/shelf_cons...
File ash/common/shelf/shelf_constants.cc (right):

https://codereview.chromium.org/2679333002/diff/1/ash/common/shelf/shelf_cons...
ash/common/shelf/shelf_constants.cc:24: const int kShelfTranslucentAlpha = 204;
On 2017/02/08 00:31:27, James Cook wrote:
> Isn't 204 the value for NON_MATERIAL?

Good catch.  This was moved to pre-requisite CL.

https://codereview.chromium.org/2679333002/diff/1/ash/common/shelf/shelf_layo...
File ash/common/shelf/shelf_layout_manager.h (right):

https://codereview.chromium.org/2679333002/diff/1/ash/common/shelf/shelf_layo...
ash/common/shelf/shelf_layout_manager.h:244: void
MaybeUpdateShelfBackground(gfx::AnimationChangeType change_type);
On 2017/02/08 00:31:28, James Cook wrote:
> #include animation_change_type.h?

Forward declared instead.

https://codereview.chromium.org/2679333002/diff/1/ash/common/shelf/shelf_widg...
File ash/common/shelf/shelf_widget.h (right):

https://codereview.chromium.org/2679333002/diff/1/ash/common/shelf/shelf_widg...
ash/common/shelf/shelf_widget.h:49: gfx::AnimationChangeType change_type);
On 2017/02/08 00:31:28, James Cook wrote:
> #include for this?
> 
> or you could use enum class which would let you forward declare it

Neat, I didn't know you could forward declare enum classes.

https://codereview.chromium.org/2679333002/diff/1/ui/gfx/animation/slide_anim...
File ui/gfx/animation/slide_animation.cc (right):

https://codereview.chromium.org/2679333002/diff/1/ui/gfx/animation/slide_anim...
ui/gfx/animation/slide_animation.cc:53: delegate()->AnimationProgressed(this);
On 2017/02/08 00:31:28, James Cook wrote:
> Do you know offhand if any of Chrome's other users of SlideAnimation depend on
> duration 0?

I did a quick audit and it appears there might be 3 cases where a slide duration
of 0 may be used:
- ImmersiveFullscreenController
- ScrollAnimator
- OmniboxResultView::OnBoundsChanged()

Oddly enough there is a BoundsAnimatorDisabler in shelf_view.cc that uses a
duration of 1.  Seems like a work around for some undesirable behaviour when the
duration is 0.

IMO the SlideAnimation doesn't support this case well and it should probably be
fixed or removed.

https://codereview.chromium.org/2679333002/diff/1/ui/gfx/animation/slide_anim...
File ui/gfx/animation/slide_animation.h (right):

https://codereview.chromium.org/2679333002/diff/1/ui/gfx/animation/slide_anim...
ui/gfx/animation/slide_animation.h:61: void ShowImmediately();
On 2017/02/08 03:02:56, sky wrote:
> Can you motivate why you want these? I would prefer client code deal with this
> situation rather than complicating the animation code. By that I mean if the
> client code knows to call this, then presumably the client can take whatever
> action completely immediately would.

Done.

[sky, 2017-02-10 20:54:16]

https://codereview.chromium.org/2679333002/diff/60001/ui/gfx/animation/animat...
File ui/gfx/animation/animation_change_type.h (right):

https://codereview.chromium.org/2679333002/diff/60001/ui/gfx/animation/animat...
ui/gfx/animation/animation_change_type.h:10: enum class AnimationChangeType {
ANIMATE, IMMEDIATE };
This isn't used in this package at all, so I would rather you keep in the code
that uses it; ash.

[James Cook, 2017-02-10 21:17:34]

LGTM with nits

https://codereview.chromium.org/2679333002/diff/60001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_background_animator.cc (right):

https://codereview.chromium.org/2679333002/diff/60001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator.cc:145: return animator_ &&
previous_background_type_ == background_type &&
optional: early exit above if animator_ is null?

https://codereview.chromium.org/2679333002/diff/60001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_background_animator.h (right):

https://codereview.chromium.org/2679333002/diff/60001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator.h:17: #include
"ui/gfx/animation/tween.h"
needed?

https://codereview.chromium.org/2679333002/diff/60001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator.h:89: class AnimationValues {
nit: I would provide empty constructor and destructor in .cc file, then either
DISALLOW_COPY_AND_ASSIGN or explicitly do copy constructors = default.

https://codereview.chromium.org/2679333002/diff/60001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_background_animator_unittest.cc (right):

https://codereview.chromium.org/2679333002/diff/60001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator_unittest.cc:221: EXPECT_NE(nullptr,
test_api_->animator());
nit: It's pretty common to EXPECT_TRUE(pointer) and EXPECT_FALSE(pointer). I
find EXPECT_TRUE(pointer) easier to read than the backwards NE nullptr.

https://codereview.chromium.org/2679333002/diff/60001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator_unittest.cc:227:
AnimatorDestroyedWhenChangingBackgroundImmedidately) {
nit: Immediately

https://codereview.chromium.org/2679333002/diff/60001/ash/common/shelf/wm_she...
File ash/common/shelf/wm_shelf_observer.h (right):

https://codereview.chromium.org/2679333002/diff/60001/ash/common/shelf/wm_she...
ash/common/shelf/wm_shelf_observer.h:18: gfx::AnimationChangeType change_type)
{}
optional: forward declare

[bruthig, 2017-02-10 21:54:18]

Description was changed from

==========
[ash-md] Remove the number of animators used for the Shelf animations.

***NOTE: This CL depends on: https://codereview.chromium.org/2680113002/

Previously the ShelfBackgroundAnimator was using 2 BackgroundAnimators, this CL
replaces (and removes) the BackgroundAnimator with a single SlideAnimation.

BUG=595010
TEST=ash_unittests, gfx_unittests
==========

to

==========
[ash-md] Remove the number of animators used for the Shelf animations.

***NOTE: This CL depends on: https://codereview.chromium.org/2680113002/

Previously the ShelfBackgroundAnimator was using 2 BackgroundAnimators, this CL
replaces (and removes) the BackgroundAnimator with a single SlideAnimation.

BUG=595010
TEST=ash_unittests
==========

[bruthig, 2017-02-10 21:54:28]

Description was changed from

==========
[ash-md] Remove the number of animators used for the Shelf animations.

***NOTE: This CL depends on: https://codereview.chromium.org/2680113002/

Previously the ShelfBackgroundAnimator was using 2 BackgroundAnimators, this CL
replaces (and removes) the BackgroundAnimator with a single SlideAnimation.

BUG=595010
TEST=ash_unittests
==========

to

==========
[ash-md] Remove the number of animators used for the Shelf animations.

Previously the ShelfBackgroundAnimator was using 2 BackgroundAnimators, this CL
replaces (and removes) the BackgroundAnimator with a single SlideAnimation.

BUG=595010
TEST=ash_unittests
==========

[bruthig, 2017-02-10 22:44:56]

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

[commit-bot: I haz the power, 2017-02-10 22:45:12]

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

[bruthig, 2017-02-10 22:45:36]

Scott, can you take another look at:
- ui/*
- ash/animation/animation_change_type.h

https://codereview.chromium.org/2679333002/diff/60001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_background_animator.cc (right):

https://codereview.chromium.org/2679333002/diff/60001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator.cc:145: return animator_ &&
previous_background_type_ == background_type &&
On 2017/02/10 21:17:34, James Cook wrote:
> optional: early exit above if animator_ is null?

Good idea, done.

https://codereview.chromium.org/2679333002/diff/60001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_background_animator.h (right):

https://codereview.chromium.org/2679333002/diff/60001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator.h:17: #include
"ui/gfx/animation/tween.h"
On 2017/02/10 21:17:34, James Cook wrote:
> needed?

Nope, removed.

https://codereview.chromium.org/2679333002/diff/60001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator.h:89: class AnimationValues {
On 2017/02/10 21:17:34, James Cook wrote:
> nit: I would provide empty constructor and destructor in .cc file, then either
> DISALLOW_COPY_AND_ASSIGN or explicitly do copy constructors = default.

Done.

https://codereview.chromium.org/2679333002/diff/60001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_background_animator_unittest.cc (right):

https://codereview.chromium.org/2679333002/diff/60001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator_unittest.cc:221: EXPECT_NE(nullptr,
test_api_->animator());
On 2017/02/10 21:17:34, James Cook wrote:
> nit: It's pretty common to EXPECT_TRUE(pointer) and EXPECT_FALSE(pointer). I
> find EXPECT_TRUE(pointer) easier to read than the backwards NE nullptr.

Done.

https://codereview.chromium.org/2679333002/diff/60001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator_unittest.cc:227:
AnimatorDestroyedWhenChangingBackgroundImmedidately) {
On 2017/02/10 21:17:34, James Cook wrote:
> nit: Immediately

Done.

https://codereview.chromium.org/2679333002/diff/60001/ash/common/shelf/wm_she...
File ash/common/shelf/wm_shelf_observer.h (right):

https://codereview.chromium.org/2679333002/diff/60001/ash/common/shelf/wm_she...
ash/common/shelf/wm_shelf_observer.h:18: gfx::AnimationChangeType change_type)
{}
On 2017/02/10 21:17:34, James Cook wrote:
> optional: forward declare

Done.

https://codereview.chromium.org/2679333002/diff/60001/ui/gfx/animation/animat...
File ui/gfx/animation/animation_change_type.h (right):

https://codereview.chromium.org/2679333002/diff/60001/ui/gfx/animation/animat...
ui/gfx/animation/animation_change_type.h:10: enum class AnimationChangeType {
ANIMATE, IMMEDIATE };
On 2017/02/10 20:54:16, sky wrote:
> This isn't used in this package at all, so I would rather you keep in the code
> that uses it; ash.

Done.

[commit-bot: I haz the power, 2017-02-10 23:51:24]

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

[commit-bot: I haz the power, 2017-02-10 23:51:24]

Dry run: This issue passed the CQ dry run.

[sky, 2017-02-13 16:30:19]

LGTM

[bruthig, 2017-02-13 17:03:10]

The CQ bit was checked by bruthig@chromium.org

[bruthig, 2017-02-13 17:03:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2679333002/#ps80001
(title: "Addressed comments from patch set 4.")

[commit-bot: I haz the power, 2017-02-13 17:03:57]

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

[commit-bot: I haz the power, 2017-02-13 18:22:49]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487005390369450,
"parent_rev": "42e2bf191c71187a227fa5c76194b372052b049b", "commit_rev":
"ed35a0fe3dbb990c4782489c3982de49fa6f9059"}

[commit-bot: I haz the power, 2017-02-13 18:23:28]

Description was changed from

==========
[ash-md] Remove the number of animators used for the Shelf animations.

Previously the ShelfBackgroundAnimator was using 2 BackgroundAnimators, this CL
replaces (and removes) the BackgroundAnimator with a single SlideAnimation.

BUG=595010
TEST=ash_unittests
==========

to

==========
[ash-md] Remove the number of animators used for the Shelf animations.

Previously the ShelfBackgroundAnimator was using 2 BackgroundAnimators, this CL
replaces (and removes) the BackgroundAnimator with a single SlideAnimation.

BUG=595010
TEST=ash_unittests

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

[commit-bot: I haz the power, 2017-02-13 18:23:30]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/ed35a0fe3dbb990c4782489c3982...