Replaced BackgroundAnimator with ShelfBackgroundAnimator.

Apply Shelf/Dock opacity animations for the Material Design style.

The Shelf animates between the 3 ShelfBackgroundType's using 2
different animations; one uses BackgroundAnimator timers and
View::OnPaint() and the other uses a Layer to animate the
SHELF_BACKGROUND_MAXIMIZED opaque background.

This change centralizes the animation logic so they all stay in sync
and applies the opacity animations to the material design style
Shelf/Dock.

Eventually the ShelfBackgroundAnimator will be enhanced to animate
between colors instead of just alpha values.

Follow-up plans:
 - Use a single instance of the ShelfBackgroundAnimator.
 - Remove the |opaque_background_| from ShelfWidget and
   DockedBackgroundWidget.
 - Collapse the multiple BackgroundAnimators owned by the
   ShelfBackgroundAnimator into a single SlideAnimation.
 - Animate the MD AppList and Overflow buttons.

BUG=607037
TEST=ShelfBackgroundAnimator(NonMD|MD)Test.*

Committed: https://crrev.com/f34d76ca0084f12c0d01f18bd9549e300b8d2ada
Cr-Commit-Position: refs/heads/master@{#408390}

[bruthig, 2016-06-09 21:51:25]

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

[bruthig, 2016-06-09 21:51:26]

James, can you take a look from a design perspective before I invest too much
time in this approach?  I just want to make sure it won't conflict with any
Shelf refactoring you're currently doing, or if you have any other issues with
the approach.

Keep in mind the code is still buggy and I haven't removed the
opaque_background() Layers yet but I've left TODO(bruthig) breadcrumbs near some
parts that will be removed.

Thanks! :)

[James Cook, 2016-06-09 23:40:37]

I'm OK with the overall approach. It's a bit of a bummer that there's another
layer of abstraction between the individual UI elements and the underlying
animator. OTOH, consolidating all the shelf stuff in one place seems beneficial,
especially if you can collapse the two sorts of animations.

I would prefer this code get implemented in //ash/common/shelf.

https://codereview.chromium.org/2053113002/diff/1/ash/common/wm/background_an...
File ash/common/wm/background_animator.h (right):

https://codereview.chromium.org/2053113002/diff/1/ash/common/wm/background_an...
ash/common/wm/background_animator.h:28: virtual void
BackgroundAnimationEnded(bool successfully) {}
nit: It feels a little weird that the other function is pure-virtual and this
one isn't.

https://codereview.chromium.org/2053113002/diff/1/ash/shelf/shelf_background_...
File ash/shelf/shelf_background_animator.cc (right):

https://codereview.chromium.org/2053113002/diff/1/ash/shelf/shelf_background_...
ash/shelf/shelf_background_animator.cc:94: for (auto delegate : delegates)
nit: don't use raw "auto" in ranged-for loops. Prefer the actual type, or use
auto* or auto& if you must.

https://codereview.chromium.org/2053113002/diff/1/ash/shelf/shelf_background_...
File ash/shelf/shelf_background_animator.h (right):

https://codereview.chromium.org/2053113002/diff/1/ash/shelf/shelf_background_...
ash/shelf/shelf_background_animator.h:15: 
This will need a good class comment.

https://codereview.chromium.org/2053113002/diff/1/ash/shelf/shelf_background_...
ash/shelf/shelf_background_animator.h:16: class ASH_EXPORT
ShelfBackgroundAnimator : public ShelfLayoutManagerObserver,
I think it would be better if this class lived in //ash/common/shelf. I'm in the
process of migrating shelf code there.

This class could derive from WmShelfObserver, and you would need to extend
WmShelfObserver to have an OnBackgroundUpdated() method, but that shouldn't be
too hard.

https://codereview.chromium.org/2053113002/diff/1/ash/shelf/shelf_background_...
File ash/shelf/shelf_background_animator_delegate.h (right):

https://codereview.chromium.org/2053113002/diff/1/ash/shelf/shelf_background_...
ash/shelf/shelf_background_animator_delegate.h:18:
DISALLOW_COPY_AND_ASSIGN(ShelfBackgroundAnimatorDelegate);
Do you need this, given that there's a pure-virtual method?

[bruthig, 2016-06-14 15:42:16]

Description was changed from

==========
Replaced BackgroundAnimator with ShelfBackgroundAnimator.

*** NOTE: This is still a WIP and is posted for design review only.

The Shelf animates between the 3 ShelfBackgroundType's using 2
different animations; one uses BackgroundAnimator timers and
View::OnPaint() and the other uses a Layer to animate the
SHELF_BACKGROUND_MAXIMIZED opaque background which is overkill.

This change will replace the Layer animation with a more powerful
ShelfBackgroundAnimator that is composed of multiple
BackgroundAnimators.

The long term goal is to be able to animate the Shelf bar AND the
background of the Shelf buttons (which are no longer asset based)
from one common spot.

Eventually the ShelfBackgroundAnimator will be enhanced to animate
between colours instead of just alpha values.

The dimmer affect can also be adopted into the ShelfBackgroundAnimator

BUG=607037

TEST=TODO
==========

to

==========
Apply Shelf/Dock opacity animations for the Material Design style.

The Shelf animates between the 3 ShelfBackgroundType's using 2
different animations; one uses BackgroundAnimator timers and
View::OnPaint() and the other uses a Layer to animate the
SHELF_BACKGROUND_MAXIMIZED opaque background.

This change centralizes the animation logic so they all stay in sync
and applies the opacity animations to the material design style
Shelf/Dock.

Eventually the ShelfBackgroundAnimator will be enhanced to animate
between colors instead of just alpha values.

Follow-up plans:
 - Use a single instance of the ShelfBackgroundAnimator.
 - Remove the |opaque_background_| from ShelfWidget and
   DockedBackgroundWidget.
 - Collapse the multiple BackgroundAnimators owned by the
   ShelfBackgroundAnimator into a single SlideAnimation.

BUG=607037

TEST=ShelfBackgroundAnimator(NonMD|MD)Test.*
==========

[bruthig, 2016-06-14 16:40:13]

Description was changed from

==========
Apply Shelf/Dock opacity animations for the Material Design style.

The Shelf animates between the 3 ShelfBackgroundType's using 2
different animations; one uses BackgroundAnimator timers and
View::OnPaint() and the other uses a Layer to animate the
SHELF_BACKGROUND_MAXIMIZED opaque background.

This change centralizes the animation logic so they all stay in sync
and applies the opacity animations to the material design style
Shelf/Dock.

Eventually the ShelfBackgroundAnimator will be enhanced to animate
between colors instead of just alpha values.

Follow-up plans:
 - Use a single instance of the ShelfBackgroundAnimator.
 - Remove the |opaque_background_| from ShelfWidget and
   DockedBackgroundWidget.
 - Collapse the multiple BackgroundAnimators owned by the
   ShelfBackgroundAnimator into a single SlideAnimation.

BUG=607037

TEST=ShelfBackgroundAnimator(NonMD|MD)Test.*
==========

to

==========
Apply Shelf/Dock opacity animations for the Material Design style.

The Shelf animates between the 3 ShelfBackgroundType's using 2
different animations; one uses BackgroundAnimator timers and
View::OnPaint() and the other uses a Layer to animate the
SHELF_BACKGROUND_MAXIMIZED opaque background.

This change centralizes the animation logic so they all stay in sync
and applies the opacity animations to the material design style
Shelf/Dock.

Eventually the ShelfBackgroundAnimator will be enhanced to animate
between colors instead of just alpha values.

Follow-up plans:
 - Use a single instance of the ShelfBackgroundAnimator.
 - Remove the |opaque_background_| from ShelfWidget and
   DockedBackgroundWidget.
 - Collapse the multiple BackgroundAnimators owned by the
   ShelfBackgroundAnimator into a single SlideAnimation.
 - Animate the MD AppList and Overflow buttons.

BUG=607037

TEST=ShelfBackgroundAnimator(NonMD|MD)Test.*
==========

[bruthig, 2016-06-14 16:40:44]

James, Can you please take another look?

https://codereview.chromium.org/2053113002/diff/1/ash/common/wm/background_an...
File ash/common/wm/background_animator.h (right):

https://codereview.chromium.org/2053113002/diff/1/ash/common/wm/background_an...
ash/common/wm/background_animator.h:28: virtual void
BackgroundAnimationEnded(bool successfully) {}
On 2016/06/09 23:40:37, James Cook wrote:
> nit: It feels a little weird that the other function is pure-virtual and this
> one isn't.

Done. Agreed, this was a short cut to get it up for design review.

https://codereview.chromium.org/2053113002/diff/1/ash/shelf/shelf_background_...
File ash/shelf/shelf_background_animator.cc (right):

https://codereview.chromium.org/2053113002/diff/1/ash/shelf/shelf_background_...
ash/shelf/shelf_background_animator.cc:94: for (auto delegate : delegates)
On 2016/06/09 23:40:37, James Cook wrote:
> nit: don't use raw "auto" in ranged-for loops. Prefer the actual type, or use
> auto* or auto& if you must.

Done.

https://codereview.chromium.org/2053113002/diff/1/ash/shelf/shelf_background_...
File ash/shelf/shelf_background_animator.h (right):

https://codereview.chromium.org/2053113002/diff/1/ash/shelf/shelf_background_...
ash/shelf/shelf_background_animator.h:15: 
On 2016/06/09 23:40:37, James Cook wrote:
> This will need a good class comment.

Acknowledged.

https://codereview.chromium.org/2053113002/diff/1/ash/shelf/shelf_background_...
ash/shelf/shelf_background_animator.h:16: class ASH_EXPORT
ShelfBackgroundAnimator : public ShelfLayoutManagerObserver,
On 2016/06/09 23:40:37, James Cook wrote:
> I think it would be better if this class lived in //ash/common/shelf. I'm in
the
> process of migrating shelf code there.
> 
> This class could derive from WmShelfObserver, and you would need to extend
> WmShelfObserver to have an OnBackgroundUpdated() method, but that shouldn't be
> too hard.

Done.

[James Cook, 2016-06-14 17:50:02]

Overall approach seems fine.

https://codereview.chromium.org/2053113002/diff/40001/ash/ash.gyp
File ash/ash.gyp (right):

https://codereview.chromium.org/2053113002/diff/40001/ash/ash.gyp#newcode795
ash/ash.gyp:795:
'common/material_design/test/material_design_controller_test_api.cc',
nit: Sort includes.

Also, I would prefer this either go in //ash/common/test (to mirror //ash/test)
or in //ash/common/material_design (to sit next to the file under test).

https://codereview.chromium.org/2053113002/diff/40001/ash/aura/wm_shelf_aura.cc
File ash/aura/wm_shelf_aura.cc (right):

https://codereview.chromium.org/2053113002/diff/40001/ash/aura/wm_shelf_aura....
ash/aura/wm_shelf_aura.cc:111: if (background_type == GetBackgroundType())
This might change when the observer fires compared to the behavior in
ShelfLayoutManager::UpdateShelfBackground(), affecting
DockedWindowLayoutManager. Are you sure this is safe?

https://codereview.chromium.org/2053113002/diff/40001/ash/aura/wm_shelf_aura.h
File ash/aura/wm_shelf_aura.h (right):

https://codereview.chromium.org/2053113002/diff/40001/ash/aura/wm_shelf_aura....
ash/aura/wm_shelf_aura.h:38: void AddObserver(WmShelfObserver* observer)
override;
Why do these need to be public?

https://codereview.chromium.org/2053113002/diff/40001/ash/common/material_des...
File ash/common/material_design/test/material_design_controller_test_api.h
(right):

https://codereview.chromium.org/2053113002/diff/40001/ash/common/material_des...
ash/common/material_design/test/material_design_controller_test_api.h:5: #ifndef
ASH_TEST_MATERIAL_DESIGN_CONTROLLER_TEST_API_H_
update include guards (or use tools/git/move_source_file.py, which does it for
you)

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

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator.cc:24: :
target_background_type_(SHELF_BACKGROUND_DEFAULT),
optional: You could init these in the header

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator.cc:53: if (has_animated_once_ &&
target_background_type_ == background_type) {
I don't understand how this matches the comment. It looks like you're skipping
repeated requests for animation to the same type.

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator.cc:55: }
nit: no braces

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator.cc:95: void
ShelfBackgroundAnimator::OnBackgroundUpdated(
Per earlier comment about notifications - are you sure it's OK not to call this
if the background types match? What if it's the very first call?

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

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator.h:31: //    3. Shelf and Dock assets
for the SHELF_BACKGROUND_OVERLAP state.
So for material design, we don't have the overlap state? Or we do, but we don't
use assets for it? You might clarify that below.

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator.h:34: //    2. Opaque overlay for THE
SHELF_BACKGROUND_MAXIMIZED state.
nit: THE -> the

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator.h:41: ShelfBackgroundType
target_background_type() const {
#include "ash/common/shelf/shelf_types.h" for ShelfBackgroundType

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator.h:92: int
previous_opaque_background_alpha_;
For my edification: Do we prefer ints for alpha or floats? I seem to recall
seeing sky@ land something recently that changed ints to floats in
ui/views/widget.h.

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator.h:104:
std::unique_ptr<BackgroundAnimator> opaque_background_animator_;
nit: Comment which things are animated with this animator.

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator.h:113: // true if the existing
BackgroundAnimators can be re-used to animate to the
nit: true -> True and false -> False below

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator.h:126: };
Thanks for nice documentation through this class.

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_background_animator_observer.h (right):

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator_observer.h:7: #include
"base/callback.h"
not needed, also no blank line above

Also, includes go insider header guards

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

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator_unittest.cc:44: 
nit: no blank lines needed

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator_unittest.cc:51: :
opaque_background_alpha_(0),
optional: initialize in class declaration

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator_unittest.cc:76: virtual
~ShelfBackgroundAnimatorTestApi();
doesn't need virtual

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator_unittest.cc:105:
ShelfBackgroundAnimatorTestApi::~ShelfBackgroundAnimatorTestApi() {}
optional: feel free to inline little methods like this and the constructor

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator_unittest.cc:137: int
expected_translucent_alpha_;
other reviewers may disagree, but I'm generally OK with members like these in
tests being protected, so you don't have to provide the getter method

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator_unittest.cc:143: :
expected_translucent_alpha_(0) {}
optional: init in class decl above

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator_unittest.cc:215: return
MaterialDesignController::MATERIAL_EXPERIMENTAL;
So the changes in this CL are tied to MATERIAL_EXPERIMENTAL, not
MATERIAL_NORMAL?

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator_unittest.cc:287: }  // namespace ash
Nice test suite.

https://codereview.chromium.org/2053113002/diff/40001/ash/common/wm/backgroun...
File ash/common/wm/background_animator.h (right):

https://codereview.chromium.org/2053113002/diff/40001/ash/common/wm/backgroun...
ash/common/wm/background_animator.h:12: #include "ui/gfx/animation/tween.h"
do you need this?

https://codereview.chromium.org/2053113002/diff/40001/ash/common/wm/backgroun...
ash/common/wm/background_animator.h:29: virtual void
BackgroundAnimationEnded(BackgroundAnimator* animator,
optional: Consider making this Ended vs. Canceled to match
gfx::AnimationDelegate below.

https://codereview.chromium.org/2053113002/diff/40001/ash/common/wm/dock/dock...
File ash/common/wm/dock/docked_window_layout_manager.cc (right):

https://codereview.chromium.org/2053113002/diff/40001/ash/common/wm/dock/dock...
ash/common/wm/dock/docked_window_layout_manager.cc:71:
manager_->shelf()->RemoveObserver(this);
background_animator_.RemoveObserver?

https://codereview.chromium.org/2053113002/diff/40001/ash/common/wm/dock/dock...
ash/common/wm/dock/docked_window_layout_manager.cc:120: const double kMaxAlpha =
255.0;
float?

https://codereview.chromium.org/2053113002/diff/40001/ash/shelf/shelf_widget.cc
File ash/shelf/shelf_widget.cc (right):

https://codereview.chromium.org/2053113002/diff/40001/ash/shelf/shelf_widget....
ash/shelf/shelf_widget.cc:375: int asset_background_alpha_;
Hooray for more explicit variable names!

https://codereview.chromium.org/2053113002/diff/40001/ash/shelf/shelf_widget....
ash/shelf/shelf_widget.cc:376: // TODO(bruthig): Remove opaque_background_.
Please file a bug for this and reference it here.

https://codereview.chromium.org/2053113002/diff/40001/ash/shelf/shelf_widget....
ash/shelf/shelf_widget.cc:568: const double kMaxAlpha = 255.0;
could be a float

https://codereview.chromium.org/2053113002/diff/40001/ash/shelf/shelf_widget....
ash/shelf/shelf_widget.cc:605: background_animator_.AddObserver(delegate_view_);
Does this need to be done here, or could it happen around line 582, nearer to
where delegate_view_ is constructed?

Where is this observer removed?

https://codereview.chromium.org/2053113002/diff/40001/ash/shelf/shelf_widget....
ash/shelf/shelf_widget.cc:735:
wm_shelf_aura->AddObserver(&background_animator_);
Given that the items being animated are built in the ShelfWidget constructor,
maybe the observer should be added there?

If so, you could pass in WmShelf there. WmShelf exists for the lifetime of the
RootWindowController.

Also, I think the observer should be removed.

https://codereview.chromium.org/2053113002/diff/40001/ash/shelf/shelf_widget.h
File ash/shelf/shelf_widget.h (right):

https://codereview.chromium.org/2053113002/diff/40001/ash/shelf/shelf_widget....
ash/shelf/shelf_widget.h:12: #include
"ash/common/shelf/shelf_background_animator_observer.h"
Do you need this include?

https://codereview.chromium.org/2053113002/diff/40001/ash/system/tray/tray_ba...
File ash/system/tray/tray_background_view.cc (left):

https://codereview.chromium.org/2053113002/diff/40001/ash/system/tray/tray_ba...
ash/system/tray/tray_background_view.cc:382: // The animator should never fire
when the alternate shelf layout is used.
Hooray for eliminating old "alternate shelf layout" code!

[bruthig, 2016-07-26 19:49:28]

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

[commit-bot: I haz the power, 2016-07-26 19:49:53]

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

[bruthig, 2016-07-26 19:50:03]

James, can you PTAL?

https://codereview.chromium.org/2053113002/diff/40001/ash/ash.gyp
File ash/ash.gyp (right):

https://codereview.chromium.org/2053113002/diff/40001/ash/ash.gyp#newcode795
ash/ash.gyp:795:
'common/material_design/test/material_design_controller_test_api.cc',
On 2016/06/14 17:50:00, James Cook wrote:
> nit: Sort includes.
> 
> Also, I would prefer this either go in //ash/common/test (to mirror
//ash/test)
> or in //ash/common/material_design (to sit next to the file under test).

Moved to //ash/common/test

https://codereview.chromium.org/2053113002/diff/40001/ash/aura/wm_shelf_aura.cc
File ash/aura/wm_shelf_aura.cc (right):

https://codereview.chromium.org/2053113002/diff/40001/ash/aura/wm_shelf_aura....
ash/aura/wm_shelf_aura.cc:111: if (background_type == GetBackgroundType())
On 2016/06/14 17:50:00, James Cook wrote:
> This might change when the observer fires compared to the behavior in
> ShelfLayoutManager::UpdateShelfBackground(), affecting
> DockedWindowLayoutManager. Are you sure this is safe?

It most definitely changes when the WmShelfObserver is notified ;)  Before this
change the WmShelfObserver's could get spammed with notifications even when the
|background_type| didn't change.  (e.g.
ShelfLayoutManager::SetWindowOverlapsShelf() gets called for each mouse drag
event)  This is most definitely sub-optimal.

I've renamed WmShelfObserver::OnBackgroundUpdated() as OnBackgroundChanged() to
be more clear that it actually changed.  And the only two overrides of this are
in
ShelfBackgroundAnimator and DockedBackgroundWidget which only need to know if
the background type actually changed.

IMO the ShelfLayoutManager should only notify it's observers when the background
type has changed as well.  But this is not a straight forward change and it's
unclear if some observers require the current spammy behavior. However, I could
add a TODO if you'd like?

https://codereview.chromium.org/2053113002/diff/40001/ash/aura/wm_shelf_aura.h
File ash/aura/wm_shelf_aura.h (right):

https://codereview.chromium.org/2053113002/diff/40001/ash/aura/wm_shelf_aura....
ash/aura/wm_shelf_aura.h:38: void AddObserver(WmShelfObserver* observer)
override;
On 2016/06/14 17:50:00, James Cook wrote:
> Why do these need to be public?

Reverted.

https://codereview.chromium.org/2053113002/diff/40001/ash/common/material_des...
File ash/common/material_design/test/material_design_controller_test_api.h
(right):

https://codereview.chromium.org/2053113002/diff/40001/ash/common/material_des...
ash/common/material_design/test/material_design_controller_test_api.h:5: #ifndef
ASH_TEST_MATERIAL_DESIGN_CONTROLLER_TEST_API_H_
On 2016/06/14 17:50:00, James Cook wrote:
> update include guards (or use tools/git/move_source_file.py, which does it for
> you)

Done.

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

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator.cc:24: :
target_background_type_(SHELF_BACKGROUND_DEFAULT),
On 2016/06/14 17:50:00, James Cook wrote:
> optional: You could init these in the header

Done.

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator.cc:53: if (has_animated_once_ &&
target_background_type_ == background_type) {
On 2016/06/14 17:50:01, James Cook wrote:
> I don't understand how this matches the comment. It looks like you're skipping
> repeated requests for animation to the same type.

Updated, PTAL.

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator.cc:55: }
On 2016/06/14 17:50:01, James Cook wrote:
> nit: no braces

Done.

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator.cc:95: void
ShelfBackgroundAnimator::OnBackgroundUpdated(
On 2016/06/14 17:50:01, James Cook wrote:
> Per earlier comment about notifications - are you sure it's OK not to call
this
> if the background types match? What if it's the very first call?

Yeah, this is ok, the ShelfBackgroundAnimator only cares when the
|background_type| actually changes.  The ShelfBackgroundAnimator is now
initialized to the ShelfLayoutManager's BackgroundType in the ShelfWidget ctor.

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

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator.h:31: //    3. Shelf and Dock assets
for the SHELF_BACKGROUND_OVERLAP state.
On 2016/06/14 17:50:01, James Cook wrote:
> So for material design, we don't have the overlap state? Or we do, but we
don't
> use assets for it? You might clarify that below.

Attempted to clarify in comment.  WDYT?

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator.h:34: //    2. Opaque overlay for THE
SHELF_BACKGROUND_MAXIMIZED state.
On 2016/06/14 17:50:01, James Cook wrote:
> nit: THE -> the

Done.

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator.h:41: ShelfBackgroundType
target_background_type() const {
On 2016/06/14 17:50:01, James Cook wrote:
> #include "ash/common/shelf/shelf_types.h" for ShelfBackgroundType

Done.

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator.h:92: int
previous_opaque_background_alpha_;
On 2016/06/14 17:50:01, James Cook wrote:
> For my edification: Do we prefer ints for alpha or floats? I seem to recall
> seeing sky@ land something recently that changed ints to floats in
> ui/views/widget.h.
> 

I haven't dealt much in the opacity/alpha space but this is my take.  Alpha
values are ints in the inclusive [0, 255] range whereas opacities are float
values in the inclusive range [0.0f, 1.0f].  I am not aware of any 'preferred'
choice but I'm all for it if one exists.  As for this change I tried to limit
the delta and just used what BackgroundAnimator was already using.

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator.h:104:
std::unique_ptr<BackgroundAnimator> opaque_background_animator_;
On 2016/06/14 17:50:01, James Cook wrote:
> nit: Comment which things are animated with this animator.

Done.

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator.h:113: // true if the existing
BackgroundAnimators can be re-used to animate to the
On 2016/06/14 17:50:01, James Cook wrote:
> nit: true -> True and false -> False below

Done.

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator.h:126: };
On 2016/06/14 17:50:01, James Cook wrote:
> Thanks for nice documentation through this class.

:)

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_background_animator_observer.h (right):

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator_observer.h:7: #include
"base/callback.h"
On 2016/06/14 17:50:01, James Cook wrote:
> not needed, also no blank line above
> 
> Also, includes go insider header guards

Done.

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

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator_unittest.cc:44: 
On 2016/06/14 17:50:02, James Cook wrote:
> nit: no blank lines needed

Done.

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator_unittest.cc:51: :
opaque_background_alpha_(0),
On 2016/06/14 17:50:01, James Cook wrote:
> optional: initialize in class declaration

Done.

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator_unittest.cc:76: virtual
~ShelfBackgroundAnimatorTestApi();
On 2016/06/14 17:50:01, James Cook wrote:
> doesn't need virtual

Done.

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator_unittest.cc:105:
ShelfBackgroundAnimatorTestApi::~ShelfBackgroundAnimatorTestApi() {}
On 2016/06/14 17:50:01, James Cook wrote:
> optional: feel free to inline little methods like this and the constructor

Done.

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator_unittest.cc:137: int
expected_translucent_alpha_;
On 2016/06/14 17:50:01, James Cook wrote:
> other reviewers may disagree, but I'm generally OK with members like these in
> tests being protected, so you don't have to provide the getter method

Normally I would use protected members as well but chose the getter here to
enforce this as a read only value in tests.

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator_unittest.cc:143: :
expected_translucent_alpha_(0) {}
On 2016/06/14 17:50:01, James Cook wrote:
> optional: init in class decl above

Done.

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator_unittest.cc:215: return
MaterialDesignController::MATERIAL_EXPERIMENTAL;
On 2016/06/14 17:50:02, James Cook wrote:
> So the changes in this CL are tied to MATERIAL_EXPERIMENTAL, not
> MATERIAL_NORMAL?

Correct, but more accurately this CL is tied to the return value of
ash::MaterialDesignController::IsShelfMaterial() which is currently true for
MATERIAL_EXPERIMENTAL but we are hoping to change this to MATERIAL_NORMAL soon. 
i.e. We're hoping to enable this for M54.

https://codereview.chromium.org/2053113002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator_unittest.cc:287: }  // namespace ash
On 2016/06/14 17:50:01, James Cook wrote:
> Nice test suite.

:D

https://codereview.chromium.org/2053113002/diff/40001/ash/common/wm/backgroun...
File ash/common/wm/background_animator.h (right):

https://codereview.chromium.org/2053113002/diff/40001/ash/common/wm/backgroun...
ash/common/wm/background_animator.h:12: #include "ui/gfx/animation/tween.h"
On 2016/06/14 17:50:02, James Cook wrote:
> do you need this?

Whoops, removed.

https://codereview.chromium.org/2053113002/diff/40001/ash/common/wm/backgroun...
ash/common/wm/background_animator.h:29: virtual void
BackgroundAnimationEnded(BackgroundAnimator* animator,
On 2016/06/14 17:50:02, James Cook wrote:
> optional: Consider making this Ended vs. Canceled to match
> gfx::AnimationDelegate below.

Canceled isn't actually needed so it has been removed.

https://codereview.chromium.org/2053113002/diff/40001/ash/common/wm/dock/dock...
File ash/common/wm/dock/docked_window_layout_manager.cc (right):

https://codereview.chromium.org/2053113002/diff/40001/ash/common/wm/dock/dock...
ash/common/wm/dock/docked_window_layout_manager.cc:71:
manager_->shelf()->RemoveObserver(this);
On 2016/06/14 17:50:02, James Cook wrote:
> background_animator_.RemoveObserver?

Done.

https://codereview.chromium.org/2053113002/diff/40001/ash/common/wm/dock/dock...
ash/common/wm/dock/docked_window_layout_manager.cc:120: const double kMaxAlpha =
255.0;
On 2016/06/14 17:50:02, James Cook wrote:
> float?

Done.

https://codereview.chromium.org/2053113002/diff/40001/ash/shelf/shelf_widget.cc
File ash/shelf/shelf_widget.cc (right):

https://codereview.chromium.org/2053113002/diff/40001/ash/shelf/shelf_widget....
ash/shelf/shelf_widget.cc:376: // TODO(bruthig): Remove opaque_background_.
On 2016/06/14 17:50:02, James Cook wrote:
> Please file a bug for this and reference it here.

Done.

https://codereview.chromium.org/2053113002/diff/40001/ash/shelf/shelf_widget....
ash/shelf/shelf_widget.cc:568: const double kMaxAlpha = 255.0;
On 2016/06/14 17:50:02, James Cook wrote:
> could be a float

Done.

https://codereview.chromium.org/2053113002/diff/40001/ash/shelf/shelf_widget....
ash/shelf/shelf_widget.cc:605: background_animator_.AddObserver(delegate_view_);
On 2016/06/14 17:50:02, James Cook wrote:
> Does this need to be done here, or could it happen around line 582, nearer to
> where delegate_view_ is constructed?
> 
> Where is this observer removed?

Done.

https://codereview.chromium.org/2053113002/diff/40001/ash/shelf/shelf_widget....
ash/shelf/shelf_widget.cc:735:
wm_shelf_aura->AddObserver(&background_animator_);
On 2016/06/14 17:50:02, James Cook wrote:
> Given that the items being animated are built in the ShelfWidget constructor,
> maybe the observer should be added there?
> 
> If so, you could pass in WmShelf there. WmShelf exists for the lifetime of the
> RootWindowController.
> 
> Also, I think the observer should be removed.

Done.

https://codereview.chromium.org/2053113002/diff/40001/ash/shelf/shelf_widget.h
File ash/shelf/shelf_widget.h (right):

https://codereview.chromium.org/2053113002/diff/40001/ash/shelf/shelf_widget....
ash/shelf/shelf_widget.h:12: #include
"ash/common/shelf/shelf_background_animator_observer.h"
On 2016/06/14 17:50:02, James Cook wrote:
> Do you need this include?

Removed.

[commit-bot: I haz the power, 2016-07-26 20:51:11]

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

[commit-bot: I haz the power, 2016-07-26 20:51:12]

Dry run: This issue passed the CQ dry run.

[James Cook, 2016-07-27 00:30:39]

https://codereview.chromium.org/2053113002/diff/40001/ash/aura/wm_shelf_aura.cc
File ash/aura/wm_shelf_aura.cc (right):

https://codereview.chromium.org/2053113002/diff/40001/ash/aura/wm_shelf_aura....
ash/aura/wm_shelf_aura.cc:111: if (background_type == GetBackgroundType())
On 2016/07/26 19:50:01, bruthig wrote:
> On 2016/06/14 17:50:00, James Cook wrote:
> > This might change when the observer fires compared to the behavior in
> > ShelfLayoutManager::UpdateShelfBackground(), affecting
> > DockedWindowLayoutManager. Are you sure this is safe?
> 
> It most definitely changes when the WmShelfObserver is notified ;)  Before
this
> change the WmShelfObserver's could get spammed with notifications even when
the
> |background_type| didn't change.  (e.g.
> ShelfLayoutManager::SetWindowOverlapsShelf() gets called for each mouse drag
> event)  This is most definitely sub-optimal.
> 
> I've renamed WmShelfObserver::OnBackgroundUpdated() as OnBackgroundChanged()
to
> be more clear that it actually changed.  And the only two overrides of this
are
> in
> ShelfBackgroundAnimator and DockedBackgroundWidget which only need to know if
> the background type actually changed.
> 
> IMO the ShelfLayoutManager should only notify it's observers when the
background
> type has changed as well.  But this is not a straight forward change and it's
> unclear if some observers require the current spammy behavior. However, I
could
> add a TODO if you'd like?

Yes, please TODO it.

https://codereview.chromium.org/2053113002/diff/240001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_background_animator.cc (right):

https://codereview.chromium.org/2053113002/diff/240001/ash/common/shelf/shelf...
ash/common/shelf/shelf_background_animator.cc:12: #include
"ui/gfx/animation/tween.h"
needed?

https://codereview.chromium.org/2053113002/diff/240001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_background_animator.h (right):

https://codereview.chromium.org/2053113002/diff/240001/ash/common/shelf/shelf...
ash/common/shelf/shelf_background_animator.h:101: // TODO(bruthig): Replace all
BackgroundAnimators with a single
super-nit: Put the TODO under the other comment, here and below

https://codereview.chromium.org/2053113002/diff/240001/ash/common/shelf/shelf...
ash/common/shelf/shelf_background_animator.h:120: // Tracks how many animators
complete successfully since the last
nit: complete -> completed?

https://codereview.chromium.org/2053113002/diff/240001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_background_animator_observer.h (right):

https://codereview.chromium.org/2053113002/diff/240001/ash/common/shelf/shelf...
ash/common/shelf/shelf_background_animator_observer.h:27:
ShelfBackgroundAnimatorObserver() {}
nit: not needed

https://codereview.chromium.org/2053113002/diff/240001/ash/common/shelf/shelf...
ash/common/shelf/shelf_background_animator_observer.h:31:
DISALLOW_COPY_AND_ASSIGN(ShelfBackgroundAnimatorObserver);
nit: not needed

https://codereview.chromium.org/2053113002/diff/240001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_background_animator_unittest.cc (right):

https://codereview.chromium.org/2053113002/diff/240001/ash/common/shelf/shelf...
ash/common/shelf/shelf_background_animator_unittest.cc:19: namespace test {
nit: tests themselves don't go in the test namespace, just test support code in
other files. Sadly, this is currently inconsistent across ash.

https://codereview.chromium.org/2053113002/diff/240001/ash/common/shelf/shelf...
ash/common/shelf/shelf_background_animator_unittest.cc:101: class
ShelfBackgroundAnimatorTestBase : public testing::Test {
nit: You might comment that this isn't a parameterized test (like other MD
tests) because the behavior is so different between MD and non-MD.

https://codereview.chromium.org/2053113002/diff/240001/ash/common/shelf/shelf...
ash/common/shelf/shelf_background_animator_unittest.cc:260: 
optional nit: I think you could remove some of the blank lines in this function.

https://codereview.chromium.org/2053113002/diff/240001/ash/common/shelf/shelf...
ash/common/shelf/shelf_background_animator_unittest.cc:280:
SetAlphaValuesOnObserver(kInvalidAlpha);
nit: it's kind of weird to use an invalid alpha here. You could just use 111 or
something that obviously visually matches below, but is a valid value.

https://codereview.chromium.org/2053113002/diff/240001/ash/common/shelf/shelf...
ash/common/shelf/shelf_background_animator_unittest.cc:408: }
Nice test suite.

https://codereview.chromium.org/2053113002/diff/240001/ash/common/shelf/wm_sh...
File ash/common/shelf/wm_shelf_observer.h (right):

https://codereview.chromium.org/2053113002/diff/240001/ash/common/shelf/wm_sh...
ash/common/shelf/wm_shelf_observer.h:17: virtual void
OnBackgroundChanged(ShelfBackgroundType background_type,
Please add a comment about the semantics of this method.

Or maybe call it OnBackgroundTypeChanged(), which will strongly imply the first
parameter changes.

https://codereview.chromium.org/2053113002/diff/240001/ash/common/system/stat...
File ash/common/system/status_area_widget.cc (right):

https://codereview.chromium.org/2053113002/diff/240001/ash/common/system/stat...
ash/common/system/status_area_widget.cc:163:
logout_button_tray_->UpdateShelfItemBackground(alpha);
ime_menu_tray_ too?

Also, can the order here match SchedulePaint() above?

https://codereview.chromium.org/2053113002/diff/240001/ash/common/system/tray...
File ash/common/system/tray/tray_background_view.cc (right):

https://codereview.chromium.org/2053113002/diff/240001/ash/common/system/tray...
ash/common/system/tray/tray_background_view.cc:504: SchedulePaint();
This is probably fine, but does it need to SchedulePaint() if there's no
background?

https://codereview.chromium.org/2053113002/diff/240001/ash/common/system/tray...
File ash/common/system/tray/tray_background_view.h (right):

https://codereview.chromium.org/2053113002/diff/240001/ash/common/system/tray...
ash/common/system/tray/tray_background_view.h:14: #include
"ash/common/wm/background_animator.h"
still needed?

https://codereview.chromium.org/2053113002/diff/240001/ash/shelf/shelf.h
File ash/shelf/shelf.h (right):

https://codereview.chromium.org/2053113002/diff/240001/ash/shelf/shelf.h#newc...
ash/shelf/shelf.h:53: class ASH_EXPORT Shelf : public
ShelfBackgroundAnimatorObserver {
Can the ShelfBackgroundAnimatorObserver live somewhere other than on Shelf?
Maybe on ShelfView, or ShelfWidget, or failing that WmShelf? I'm trying to get
rid of Shelf.

https://codereview.chromium.org/2053113002/diff/240001/ash/shelf/shelf_widget.cc
File ash/shelf/shelf_widget.cc (right):

https://codereview.chromium.org/2053113002/diff/240001/ash/shelf/shelf_widget...
ash/shelf/shelf_widget.cc:577:
static_cast<WmShelf*>(wm_shelf_aura_)->AddObserver(&background_animator_);
Could the background animator take a WmShelf as a constructor parameter, then
add/remove itself as an observer?

WmShelf is long-lived, should outlive ShelfWidget, ShelfView, etc.

https://codereview.chromium.org/2053113002/diff/240001/ash/test/ash_md_test_b...
File ash/test/ash_md_test_base.h (right):

https://codereview.chromium.org/2053113002/diff/240001/ash/test/ash_md_test_b...
ash/test/ash_md_test_base.h:9: #include
"ash/common/test/material_design_controller_test_api.h"
Could this be forward declared?

[bruthig, 2016-07-27 17:04:52]

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

[bruthig, 2016-07-27 17:05:45]

James, can you take another look?  I tried broke the changes down into 3
different patch sets.  Hopefully it's easier to follow the changes that way.

https://codereview.chromium.org/2053113002/diff/240001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_background_animator.cc (right):

https://codereview.chromium.org/2053113002/diff/240001/ash/common/shelf/shelf...
ash/common/shelf/shelf_background_animator.cc:12: #include
"ui/gfx/animation/tween.h"
On 2016/07/27 00:30:38, James Cook wrote:
> needed?

Removed.

https://codereview.chromium.org/2053113002/diff/240001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_background_animator.h (right):

https://codereview.chromium.org/2053113002/diff/240001/ash/common/shelf/shelf...
ash/common/shelf/shelf_background_animator.h:101: // TODO(bruthig): Replace all
BackgroundAnimators with a single
On 2016/07/27 00:30:38, James Cook wrote:
> super-nit: Put the TODO under the other comment, here and below

Done.

https://codereview.chromium.org/2053113002/diff/240001/ash/common/shelf/shelf...
ash/common/shelf/shelf_background_animator.h:120: // Tracks how many animators
complete successfully since the last
On 2016/07/27 00:30:38, James Cook wrote:
> nit: complete -> completed?

Done.

https://codereview.chromium.org/2053113002/diff/240001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_background_animator_observer.h (right):

https://codereview.chromium.org/2053113002/diff/240001/ash/common/shelf/shelf...
ash/common/shelf/shelf_background_animator_observer.h:27:
ShelfBackgroundAnimatorObserver() {}
On 2016/07/27 00:30:38, James Cook wrote:
> nit: not needed

Done.

https://codereview.chromium.org/2053113002/diff/240001/ash/common/shelf/shelf...
ash/common/shelf/shelf_background_animator_observer.h:31:
DISALLOW_COPY_AND_ASSIGN(ShelfBackgroundAnimatorObserver);
On 2016/07/27 00:30:38, James Cook wrote:
> nit: not needed

Done.

https://codereview.chromium.org/2053113002/diff/240001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_background_animator_unittest.cc (right):

https://codereview.chromium.org/2053113002/diff/240001/ash/common/shelf/shelf...
ash/common/shelf/shelf_background_animator_unittest.cc:19: namespace test {
On 2016/07/27 00:30:39, James Cook wrote:
> nit: tests themselves don't go in the test namespace, just test support code
in
> other files. Sadly, this is currently inconsistent across ash.

Is it ok to scope the ShelfBackgroundAnimatorTestApi test support class in this
file in the test namespace or should I move it to it's own file?

https://codereview.chromium.org/2053113002/diff/240001/ash/common/shelf/shelf...
ash/common/shelf/shelf_background_animator_unittest.cc:101: class
ShelfBackgroundAnimatorTestBase : public testing::Test {
On 2016/07/27 00:30:38, James Cook wrote:
> nit: You might comment that this isn't a parameterized test (like other MD
> tests) because the behavior is so different between MD and non-MD.

Done.

https://codereview.chromium.org/2053113002/diff/240001/ash/common/shelf/shelf...
ash/common/shelf/shelf_background_animator_unittest.cc:260: 
On 2016/07/27 00:30:39, James Cook wrote:
> optional nit: I think you could remove some of the blank lines in this
function.

Done.

https://codereview.chromium.org/2053113002/diff/240001/ash/common/shelf/shelf...
ash/common/shelf/shelf_background_animator_unittest.cc:280:
SetAlphaValuesOnObserver(kInvalidAlpha);
On 2016/07/27 00:30:39, James Cook wrote:
> nit: it's kind of weird to use an invalid alpha here. You could just use 111
or
> something that obviously visually matches below, but is a valid value.

Done, renamed kInvalidAlpha to kDummyAlpha and used a value of 111.

https://codereview.chromium.org/2053113002/diff/240001/ash/common/shelf/shelf...
ash/common/shelf/shelf_background_animator_unittest.cc:408: }
On 2016/07/27 00:30:38, James Cook wrote:
> Nice test suite.

:)

https://codereview.chromium.org/2053113002/diff/240001/ash/common/shelf/wm_sh...
File ash/common/shelf/wm_shelf_observer.h (right):

https://codereview.chromium.org/2053113002/diff/240001/ash/common/shelf/wm_sh...
ash/common/shelf/wm_shelf_observer.h:17: virtual void
OnBackgroundChanged(ShelfBackgroundType background_type,
On 2016/07/27 00:30:39, James Cook wrote:
> Please add a comment about the semantics of this method.
> 
> Or maybe call it OnBackgroundTypeChanged(), which will strongly imply the
first
> parameter changes.

Done. Opted for the later suggestion.

https://codereview.chromium.org/2053113002/diff/240001/ash/common/system/stat...
File ash/common/system/status_area_widget.cc (right):

https://codereview.chromium.org/2053113002/diff/240001/ash/common/system/stat...
ash/common/system/status_area_widget.cc:163:
logout_button_tray_->UpdateShelfItemBackground(alpha);
On 2016/07/27 00:30:39, James Cook wrote:
> ime_menu_tray_ too?
> 
> Also, can the order here match SchedulePaint() above?

Done.

https://codereview.chromium.org/2053113002/diff/240001/ash/common/system/tray...
File ash/common/system/tray/tray_background_view.cc (right):

https://codereview.chromium.org/2053113002/diff/240001/ash/common/system/tray...
ash/common/system/tray/tray_background_view.cc:504: SchedulePaint();
On 2016/07/27 00:30:39, James Cook wrote:
> This is probably fine, but does it need to SchedulePaint() if there's no
> background?

Nope, changed.

https://codereview.chromium.org/2053113002/diff/240001/ash/common/system/tray...
File ash/common/system/tray/tray_background_view.h (right):

https://codereview.chromium.org/2053113002/diff/240001/ash/common/system/tray...
ash/common/system/tray/tray_background_view.h:14: #include
"ash/common/wm/background_animator.h"
On 2016/07/27 00:30:39, James Cook wrote:
> still needed?

Removed.

https://codereview.chromium.org/2053113002/diff/240001/ash/shelf/shelf.h
File ash/shelf/shelf.h (right):

https://codereview.chromium.org/2053113002/diff/240001/ash/shelf/shelf.h#newc...
ash/shelf/shelf.h:53: class ASH_EXPORT Shelf : public
ShelfBackgroundAnimatorObserver {
On 2016/07/27 00:30:39, James Cook wrote:
> Can the ShelfBackgroundAnimatorObserver live somewhere other than on Shelf?
> Maybe on ShelfView, or ShelfWidget, or failing that WmShelf? I'm trying to get
> rid of Shelf.

I made the ShelfView extend the ShelfBackgroundAnimatorObserver and passed the
ShelfBackgroundAnimator owned by the ShelfWidget to the ShelfView so that the
ShelfView can add/remove itself as an observer.

PTAL

https://codereview.chromium.org/2053113002/diff/240001/ash/shelf/shelf_widget.cc
File ash/shelf/shelf_widget.cc (right):

https://codereview.chromium.org/2053113002/diff/240001/ash/shelf/shelf_widget...
ash/shelf/shelf_widget.cc:577:
static_cast<WmShelf*>(wm_shelf_aura_)->AddObserver(&background_animator_);
On 2016/07/27 00:30:39, James Cook wrote:
> Could the background animator take a WmShelf as a constructor parameter, then
> add/remove itself as an observer?
> 
> WmShelf is long-lived, should outlive ShelfWidget, ShelfView, etc.

I've added a WmShelf param to ShelfBackgroundAnimator, however I made it handle
null values because the DockedWindowLayoutManager observes the WmShelf and
passes a different ShelfBackgroundType depending on whether the widget is
visible or not.

PTAL

https://codereview.chromium.org/2053113002/diff/240001/ash/test/ash_md_test_b...
File ash/test/ash_md_test_base.h (right):

https://codereview.chromium.org/2053113002/diff/240001/ash/test/ash_md_test_b...
ash/test/ash_md_test_base.h:9: #include
"ash/common/test/material_design_controller_test_api.h"
On 2016/07/27 00:30:39, James Cook wrote:
> Could this be forward declared?

Done.

[commit-bot: I haz the power, 2016-07-27 17:05:54]

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

[James Cook, 2016-07-27 17:33:10]

LGTM with nits. Hooray!

https://codereview.chromium.org/2053113002/diff/300001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_background_animator_unittest.cc (right):

https://codereview.chromium.org/2053113002/diff/300001/ash/common/shelf/shelf...
ash/common/shelf/shelf_background_animator_unittest.cc:69: namespace test {
nit: Remove. Generally we only do this for test support code in its own file.

https://codereview.chromium.org/2053113002/diff/300001/ash/shelf/shelf.h
File ash/shelf/shelf.h (right):

https://codereview.chromium.org/2053113002/diff/300001/ash/shelf/shelf.h#newc...
ash/shelf/shelf.h:61: virtual ~Shelf();
Does Shelf have any subclasses? If not, remove virtual.

[commit-bot: I haz the power, 2016-07-27 17:55:29]

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

[commit-bot: I haz the power, 2016-07-27 17:55:30]

Dry run: This issue passed the CQ dry run.

[bruthig, 2016-07-27 18:01:45]

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

[bruthig, 2016-07-27 18:01:47]

sky@, can you take a quick look at the TODO I added in
ui/gfx/animation/animation_container.h ?

https://codereview.chromium.org/2053113002/diff/300001/ash/shelf/shelf.h
File ash/shelf/shelf.h (right):

https://codereview.chromium.org/2053113002/diff/300001/ash/shelf/shelf.h#newc...
ash/shelf/shelf.h:61: virtual ~Shelf();
On 2016/07/27 17:33:10, James Cook wrote:
> Does Shelf have any subclasses? If not, remove virtual.

Done.

[bruthig, 2016-07-27 18:07:51]

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

[commit-bot: I haz the power, 2016-07-27 18:08:12]

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

[bruthig, 2016-07-27 18:09:00]

https://codereview.chromium.org/2053113002/diff/300001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_background_animator_unittest.cc (right):

https://codereview.chromium.org/2053113002/diff/300001/ash/common/shelf/shelf...
ash/common/shelf/shelf_background_animator_unittest.cc:69: namespace test {
On 2016/07/27 17:33:10, James Cook wrote:
> nit: Remove. Generally we only do this for test support code in its own file.

Done.

[commit-bot: I haz the power, 2016-07-27 19:11:21]

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

[commit-bot: I haz the power, 2016-07-27 19:11:22]

Dry run: This issue passed the CQ dry run.

[sky, 2016-07-27 22:06:14]

https://codereview.chromium.org/2053113002/diff/320001/ui/gfx/animation/anima...
File ui/gfx/animation/animation_container.h (right):

https://codereview.chromium.org/2053113002/diff/320001/ui/gfx/animation/anima...
ui/gfx/animation/animation_container.h:30: // TODO(bruthig): Enhance to use the
same clock source as |timer_| so that tests
I'm not particularly familiar with all the task runner code. Is the clock source
readily available from said code?

[bruthig, 2016-07-28 13:42:03]

https://codereview.chromium.org/2053113002/diff/320001/ui/gfx/animation/anima...
File ui/gfx/animation/animation_container.h (right):

https://codereview.chromium.org/2053113002/diff/320001/ui/gfx/animation/anima...
ui/gfx/animation/animation_container.h:30: // TODO(bruthig): Enhance to use the
same clock source as |timer_| so that tests
On 2016/07/27 22:06:14, sky wrote:
> I'm not particularly familiar with all the task runner code. Is the clock
source
> readily available from said code?

FYI I've removed the TODO from this CL so as to not hold up the review.

It's not immediately obvious how to consolidate these 2 clock sources.  You can
readily get the clock from the TestMockTimeTaskRunner but the tricky part is
injecting that into the AnimationContainer::RepeatingTimer instance.  Anyway,
perhaps we can move further discussions over to the bug:
https://crbug.com/630684

[bruthig, 2016-07-28 13:42:10]

The CQ bit was checked by bruthig@chromium.org

[bruthig, 2016-07-28 13:42:11]

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/2053113002/#ps340001
(title: "Removed TODO from animation_container.h.")

[commit-bot: I haz the power, 2016-07-28 13:42:29]

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-07-28 13:44:14]

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

[commit-bot: I haz the power, 2016-07-28 13:44:16]

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-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[bruthig, 2016-07-28 14:02:00]

The CQ bit was checked by bruthig@chromium.org

[bruthig, 2016-07-28 14:02:00]

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/2053113002/#ps360001
(title: "Merge branch 'master' into animate_shelf_chip_backgrounds")

[commit-bot: I haz the power, 2016-07-28 14:02:17]

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-07-28 14:50:19]

Description was changed from

==========
Apply Shelf/Dock opacity animations for the Material Design style.

The Shelf animates between the 3 ShelfBackgroundType's using 2
different animations; one uses BackgroundAnimator timers and
View::OnPaint() and the other uses a Layer to animate the
SHELF_BACKGROUND_MAXIMIZED opaque background.

This change centralizes the animation logic so they all stay in sync
and applies the opacity animations to the material design style
Shelf/Dock.

Eventually the ShelfBackgroundAnimator will be enhanced to animate
between colors instead of just alpha values.

Follow-up plans:
 - Use a single instance of the ShelfBackgroundAnimator.
 - Remove the |opaque_background_| from ShelfWidget and
   DockedBackgroundWidget.
 - Collapse the multiple BackgroundAnimators owned by the
   ShelfBackgroundAnimator into a single SlideAnimation.
 - Animate the MD AppList and Overflow buttons.

BUG=607037

TEST=ShelfBackgroundAnimator(NonMD|MD)Test.*
==========

to

==========
Apply Shelf/Dock opacity animations for the Material Design style.

The Shelf animates between the 3 ShelfBackgroundType's using 2
different animations; one uses BackgroundAnimator timers and
View::OnPaint() and the other uses a Layer to animate the
SHELF_BACKGROUND_MAXIMIZED opaque background.

This change centralizes the animation logic so they all stay in sync
and applies the opacity animations to the material design style
Shelf/Dock.

Eventually the ShelfBackgroundAnimator will be enhanced to animate
between colors instead of just alpha values.

Follow-up plans:
 - Use a single instance of the ShelfBackgroundAnimator.
 - Remove the |opaque_background_| from ShelfWidget and
   DockedBackgroundWidget.
 - Collapse the multiple BackgroundAnimators owned by the
   ShelfBackgroundAnimator into a single SlideAnimation.
 - Animate the MD AppList and Overflow buttons.

BUG=607037

TEST=ShelfBackgroundAnimator(NonMD|MD)Test.*
==========

[commit-bot: I haz the power, 2016-07-28 14:50:20]

Committed patchset #19 (id:360001)

[commit-bot: I haz the power, 2016-07-28 14:52:35]

Description was changed from

==========
Apply Shelf/Dock opacity animations for the Material Design style.

The Shelf animates between the 3 ShelfBackgroundType's using 2
different animations; one uses BackgroundAnimator timers and
View::OnPaint() and the other uses a Layer to animate the
SHELF_BACKGROUND_MAXIMIZED opaque background.

This change centralizes the animation logic so they all stay in sync
and applies the opacity animations to the material design style
Shelf/Dock.

Eventually the ShelfBackgroundAnimator will be enhanced to animate
between colors instead of just alpha values.

Follow-up plans:
 - Use a single instance of the ShelfBackgroundAnimator.
 - Remove the |opaque_background_| from ShelfWidget and
   DockedBackgroundWidget.
 - Collapse the multiple BackgroundAnimators owned by the
   ShelfBackgroundAnimator into a single SlideAnimation.
 - Animate the MD AppList and Overflow buttons.

BUG=607037

TEST=ShelfBackgroundAnimator(NonMD|MD)Test.*
==========

to

==========
Apply Shelf/Dock opacity animations for the Material Design style.

The Shelf animates between the 3 ShelfBackgroundType's using 2
different animations; one uses BackgroundAnimator timers and
View::OnPaint() and the other uses a Layer to animate the
SHELF_BACKGROUND_MAXIMIZED opaque background.

This change centralizes the animation logic so they all stay in sync
and applies the opacity animations to the material design style
Shelf/Dock.

Eventually the ShelfBackgroundAnimator will be enhanced to animate
between colors instead of just alpha values.

Follow-up plans:
 - Use a single instance of the ShelfBackgroundAnimator.
 - Remove the |opaque_background_| from ShelfWidget and
   DockedBackgroundWidget.
 - Collapse the multiple BackgroundAnimators owned by the
   ShelfBackgroundAnimator into a single SlideAnimation.
 - Animate the MD AppList and Overflow buttons.

BUG=607037

TEST=ShelfBackgroundAnimator(NonMD|MD)Test.*

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

[commit-bot: I haz the power, 2016-07-28 14:52:37]

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

[tapted, 2016-07-29 00:37:50]

A revert of this CL (patchset #19 id:360001) has been created in
https://codereview.chromium.org/2191153002/ by tapted@chromium.org.

The reason for reverting is: Many ASAN failures in ash_unittests and others on
ChromeOS builders

link:
https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2...

errors like

==3029==ERROR: AddressSanitizer: heap-use-after-free on address 0x6030000b9800
at pc 0x0000127a38fa bp 0x7fff10ad4a50 sp 0x7fff10ad4a48
READ of size 8 at 0x6030000b9800 thread T0
    #0 0x127a38f9 in
__find<__gnu_cxx::__normal_iterator<ash::ShelfBackgroundAnimatorObserver **,
std::vector<ash::ShelfBackgroundAnimatorObserver *,
std::allocator<ash::ShelfBackgroundAnimatorObserver *> > >,
ash::ShelfBackgroundAnimatorObserver *>
../../include/c++/4.6/bits/stl_algo.h:190:8
    #1 0x127a38f9 in
find<__gnu_cxx::__normal_iterator<ash::ShelfBackgroundAnimatorObserver **,
std::vector<ash::ShelfBackgroundAnimatorObserver *,
std::allocator<ash::ShelfBackgroundAnimatorObserver *> > >,
ash::ShelfBackgroundAnimatorObserver *>
../../include/c++/4.6/bits/stl_algo.h:4402
    #2 0x127a38f9 in
base::ObserverListBase<ash::ShelfBackgroundAnimatorObserver>::RemoveObserver(ash::ShelfBackgroundAnimatorObserver*)
base/observer_list.h:174
    #3 0x127033f2 in ash::ShelfView::~ShelfView() ash/shelf/shelf_view.cc:408:27
    #4 0x12703a5d in ash::ShelfView::~ShelfView() ash/shelf/shelf_view.cc:404:25
    #5 0xcf2b9f1 in views::View::~View() ui/views/view.cc:134:7
    #6 0x127191a4 in ~DelegateView ash/shelf/shelf_widget.cc:407:44
    #7 0x127191a4 in non-virtual thunk to
ash::ShelfWidget::DelegateView::~DelegateView() ash/shelf/shelf_widget.cc:407
    #8 0xcf2dd8d in operator() ../../include/c++/4.6/bits/unique_ptr.h:63:2
    #9 0xcf2dd8d in reset ../../include/c++/4.6/bits/unique_ptr.h:245
    #10 0xcf2dd8d in ~unique_ptr ../../include/c++/4.6/bits/unique_ptr.h:169
    #11 0xcf2dd8d in views::View::DoRemoveChildView(views::View*, bool, bool,
bool, views::View*) ui/views/view.cc:1847
...
0x6030000b9800 is located 0 bytes inside of 32-byte region
[0x6030000b9800,0x6030000b9820)
freed by thread T0 here:
    #0 0xa83edb in operator delete(void*)
(/b/swarm_slave/w/ireuBjON/out/Release/unit_tests+0xa83edb)
    #1 0x127a2c17 in deallocate ../../include/c++/4.6/ext/new_allocator.h:98:9
    #2 0x127a2c17 in _M_deallocate ../../include/c++/4.6/bits/stl_vector.h:156
    #3 0x127a2c17 in ~_Vector_base ../../include/c++/4.6/bits/stl_vector.h:142
    #4 0x127a2c17 in ~vector ../../include/c++/4.6/bits/stl_vector.h:351
    #5 0x127a2c17 in ~ObserverListBase base/observer_list.h:69
    #6 0x127a2c17 in ~ObserverList base/observer_list.h:229
    #7 0x127a2c17 in ash::ShelfBackgroundAnimator::~ShelfBackgroundAnimator()
ash/common/shelf/shelf_background_animator.cc:37
.