ash: Extract animator from PowerButtonController

Refactoring part 1 : extract animator

BUG=138171, 139461
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=159690

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=159896

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=160161

[Daniel Erat, 2012-08-30 18:05:24]

Haven't gone through it all yet, but here's a comment.

https://chromiumcodereview.appspot.com/10914016/diff/1/ash/wm/session_state_a...
File ash/wm/session_state_animator.h (right):

https://chromiumcodereview.appspot.com/10914016/diff/1/ash/wm/session_state_a...
ash/wm/session_state_animator.h:42: enum AnimationContainerGroup {
This is too generally-named to go in the toplevel ash namespace.  Can these
enums be moved inside of SessionStateAnimator, or do lines wrap when you do
that?

[Denis Kuznetsov (DE-MUC), 2012-08-30 18:17:48]

https://chromiumcodereview.appspot.com/10914016/diff/1/ash/wm/session_state_a...
File ash/wm/session_state_animator.h (right):

https://chromiumcodereview.appspot.com/10914016/diff/1/ash/wm/session_state_a...
ash/wm/session_state_animator.h:42: enum AnimationContainerGroup {
On 2012/08/30 18:05:24, Daniel Erat wrote:
> This is too generally-named to go in the toplevel ash namespace.  Can these
> enums be moved inside of SessionStateAnimator, or do lines wrap when you do
> that?
Yes, they seem to be too long.
Would StateAnimationContainerGroup be a good replacement?

[Daniel Erat, 2012-08-30 18:28:17]

https://chromiumcodereview.appspot.com/10914016/diff/1/ash/wm/session_state_a...
File ash/wm/session_state_animator.h (right):

https://chromiumcodereview.appspot.com/10914016/diff/1/ash/wm/session_state_a...
ash/wm/session_state_animator.h:42: enum AnimationContainerGroup {
On 2012/08/30 18:17:48, Denis Kuznetsov wrote:
> On 2012/08/30 18:05:24, Daniel Erat wrote:
> > This is too generally-named to go in the toplevel ash namespace.  Can these
> > enums be moved inside of SessionStateAnimator, or do lines wrap when you do
> > that?
> Yes, they seem to be too long.
> Would StateAnimationContainerGroup be a good replacement?

I think that it's really best to keep these enums within the class.  Maybe the
enum names can be shortened.

[Daniel Erat, 2012-08-30 21:10:10]

https://chromiumcodereview.appspot.com/10914016/diff/1/ash/wm/session_state_a...
File ash/wm/session_state_animator.h (right):

https://chromiumcodereview.appspot.com/10914016/diff/1/ash/wm/session_state_a...
ash/wm/session_state_animator.h:42: enum AnimationContainerGroup {
On 2012/08/30 18:28:17, Daniel Erat wrote:
> On 2012/08/30 18:17:48, Denis Kuznetsov wrote:
> > On 2012/08/30 18:05:24, Daniel Erat wrote:
> > > This is too generally-named to go in the toplevel ash namespace.  Can
these
> > > enums be moved inside of SessionStateAnimator, or do lines wrap when you
do
> > > that?
> > Yes, they seem to be too long.
> > Would StateAnimationContainerGroup be a good replacement?
> 
> I think that it's really best to keep these enums within the class.  Maybe the
> enum names can be shortened.

Alternately, I'd be fine moving the enums inside the class and keeping the
longer names but adding "using" directives in power_button_controller.cc so you
don't need to specify the class name over and over there.  My main concern is
not seeing generic names creep into the ash namespace in headers, which then get
included by other headers, and so on.

[Denis Kuznetsov (DE-MUC), 2012-08-31 16:02:37]

https://chromiumcodereview.appspot.com/10914016/diff/1/ash/wm/session_state_a...
File ash/wm/session_state_animator.h (right):

https://chromiumcodereview.appspot.com/10914016/diff/1/ash/wm/session_state_a...
ash/wm/session_state_animator.h:42: enum AnimationContainerGroup {
On 2012/08/30 21:10:10, Daniel Erat wrote:
> On 2012/08/30 18:28:17, Daniel Erat wrote:
> > On 2012/08/30 18:17:48, Denis Kuznetsov wrote:
> > > On 2012/08/30 18:05:24, Daniel Erat wrote:
> > > > This is too generally-named to go in the toplevel ash namespace.  Can
> these
> > > > enums be moved inside of SessionStateAnimator, or do lines wrap when you
> do
> > > > that?
> > > Yes, they seem to be too long.
> > > Would StateAnimationContainerGroup be a good replacement?
> > 
> > I think that it's really best to keep these enums within the class.  Maybe
the
> > enum names can be shortened.
> 
> Alternately, I'd be fine moving the enums inside the class and keeping the
> longer names but adding "using" directives in power_button_controller.cc so
you
> don't need to specify the class name over and over there.  My main concern is
> not seeing generic names creep into the ash namespace in headers, which then
get
> included by other headers, and so on.

Done.

[Daniel Erat, 2012-08-31 17:02:12]

https://chromiumcodereview.appspot.com/10914016/diff/4002/ash/wm/power_button...
File ash/wm/power_button_controller.cc (right):

https://chromiumcodereview.appspot.com/10914016/diff/4002/ash/wm/power_button...
ash/wm/power_button_controller.cc:21: #include "ui/compositor/layer.h"
please go through all of these headers and remove the ones that are no longer
needed (which is probably most of them, i bet)

https://chromiumcodereview.appspot.com/10914016/diff/4002/ash/wm/power_button...
File ash/wm/power_button_controller.h (right):

https://chromiumcodereview.appspot.com/10914016/diff/4002/ash/wm/power_button...
ash/wm/power_button_controller.h:121: SessionStateAnimator::TestApi*
animator_api_;  // owned
any reason not to use scoped_ptr instead?

https://chromiumcodereview.appspot.com/10914016/diff/4002/ash/wm/power_button...
ash/wm/power_button_controller.h:233: SessionStateAnimator* animator_;
use scoped_ptr.  in general, please use it every time you have an opportunity to

https://chromiumcodereview.appspot.com/10914016/diff/4002/ash/wm/session_stat...
File ash/wm/session_state_animator.cc (right):

https://chromiumcodereview.appspot.com/10914016/diff/4002/ash/wm/session_stat...
ash/wm/session_state_animator.cc:201: aura::Window::Windows* containers) {
nit: fix indenting

https://chromiumcodereview.appspot.com/10914016/diff/4002/ash/wm/session_stat...
ash/wm/session_state_animator.cc:296: void
SessionStateAnimator::DropBackgroundLayer() {
both this method and HideBackgroundLayer drop the background layer; give them
better names:

DropBackgroundLayerImmediately()
DropBackgroundLayerAfterDelay()

also document in the header that the AfterDelay() method waits for the
UNDO_SLOW_CLOSE animation's delay

https://chromiumcodereview.appspot.com/10914016/diff/4002/ash/wm/session_stat...
File ash/wm/session_state_animator.h (right):

https://chromiumcodereview.appspot.com/10914016/diff/4002/ash/wm/session_stat...
ash/wm/session_state_animator.h:28: // Displays onscreen animations for session
state changes (lock/unlock,sign out,
nit: add space after "lock/unlock,"

https://chromiumcodereview.appspot.com/10914016/diff/4002/ash/wm/session_stat...
ash/wm/session_state_animator.h:33: // Exposed here for
TestApi::ContainerGroupIsAnimated().
nit: remove this "Exposed here for" comment; it's no longer accurate (since
PowerButtonController also uses these)

https://chromiumcodereview.appspot.com/10914016/diff/4002/ash/wm/session_stat...
ash/wm/session_state_animator.h:42: // Exposed here for
TestApi::ContainerGroupIsAnimated().
ditto

https://chromiumcodereview.appspot.com/10914016/diff/4002/ash/wm/session_stat...
ash/wm/session_state_animator.h:87: // aura::RootWindowObserver overrides:
nit: overridden methods should come after all others in the "public" section

https://chromiumcodereview.appspot.com/10914016/diff/4002/ash/wm/session_stat...
ash/wm/session_state_animator.h:98: AnimationType type);
nit: add a blank line between a method's signature and the following method's
comments

https://chromiumcodereview.appspot.com/10914016/diff/4002/ash/wm/session_stat...
ash/wm/session_state_animator.h:103: 
nit: remove all of these blank lines

[tfarina, 2012-09-01 11:02:30]

http://codereview.chromium.org/10914016/diff/4002/ash/wm/session_state_animat...
File ash/wm/session_state_animator.h (right):

http://codereview.chromium.org/10914016/diff/4002/ash/wm/session_state_animat...
ash/wm/session_state_animator.h:62: void trigger_hide_background_layer_timeout()
{
This looks like more complex than a simple getter and setter to be inlined in
the header file. Could you move the implementation of this class to source file
instead?

[Denis Kuznetsov (DE-MUC), 2012-09-28 11:52:43]

Continue after 3-weeks vacation.

http://codereview.chromium.org/10914016/diff/4002/ash/wm/power_button_control...
File ash/wm/power_button_controller.cc (right):

http://codereview.chromium.org/10914016/diff/4002/ash/wm/power_button_control...
ash/wm/power_button_controller.cc:21: #include "ui/compositor/layer.h"
On 2012/08/31 17:02:12, Daniel Erat wrote:
> please go through all of these headers and remove the ones that are no longer
> needed (which is probably most of them, i bet)

Done.

http://codereview.chromium.org/10914016/diff/4002/ash/wm/power_button_control...
File ash/wm/power_button_controller.h (right):

http://codereview.chromium.org/10914016/diff/4002/ash/wm/power_button_control...
ash/wm/power_button_controller.h:121: SessionStateAnimator::TestApi*
animator_api_;  // owned
On 2012/08/31 17:02:12, Daniel Erat wrote:
> any reason not to use scoped_ptr instead?

Done.

http://codereview.chromium.org/10914016/diff/4002/ash/wm/power_button_control...
ash/wm/power_button_controller.h:233: SessionStateAnimator* animator_;
On 2012/08/31 17:02:12, Daniel Erat wrote:
> use scoped_ptr.  in general, please use it every time you have an opportunity
to

Done.

http://codereview.chromium.org/10914016/diff/4002/ash/wm/session_state_animat...
File ash/wm/session_state_animator.cc (right):

http://codereview.chromium.org/10914016/diff/4002/ash/wm/session_state_animat...
ash/wm/session_state_animator.cc:201: aura::Window::Windows* containers) {
On 2012/08/31 17:02:12, Daniel Erat wrote:
> nit: fix indenting

Done.

http://codereview.chromium.org/10914016/diff/4002/ash/wm/session_state_animat...
ash/wm/session_state_animator.cc:296: void
SessionStateAnimator::DropBackgroundLayer() {
On 2012/08/31 17:02:12, Daniel Erat wrote:
> both this method and HideBackgroundLayer drop the background layer; give them
> better names:
> 
> DropBackgroundLayerImmediately()
> DropBackgroundLayerAfterDelay()
> 
> also document in the header that the AfterDelay() method waits for the
> UNDO_SLOW_CLOSE animation's delay

Done.

http://codereview.chromium.org/10914016/diff/4002/ash/wm/session_state_animat...
File ash/wm/session_state_animator.h (right):

http://codereview.chromium.org/10914016/diff/4002/ash/wm/session_state_animat...
ash/wm/session_state_animator.h:28: // Displays onscreen animations for session
state changes (lock/unlock,sign out,
On 2012/08/31 17:02:12, Daniel Erat wrote:
> nit: add space after "lock/unlock,"

Done.

http://codereview.chromium.org/10914016/diff/4002/ash/wm/session_state_animat...
ash/wm/session_state_animator.h:33: // Exposed here for
TestApi::ContainerGroupIsAnimated().
On 2012/08/31 17:02:12, Daniel Erat wrote:
> nit: remove this "Exposed here for" comment; it's no longer accurate (since
> PowerButtonController also uses these)

Done.

http://codereview.chromium.org/10914016/diff/4002/ash/wm/session_state_animat...
ash/wm/session_state_animator.h:42: // Exposed here for
TestApi::ContainerGroupIsAnimated().
On 2012/08/31 17:02:12, Daniel Erat wrote:
> ditto

Done.

http://codereview.chromium.org/10914016/diff/4002/ash/wm/session_state_animat...
ash/wm/session_state_animator.h:62: void trigger_hide_background_layer_timeout()
{
On 2012/09/01 11:02:30, tfarina wrote:
> This looks like more complex than a simple getter and setter to be inlined in
> the header file. Could you move the implementation of this class to source
file
> instead?
Done, but there is a  bunch of similar methods in PowerButtonController's
TestAPI, should I rename/move them also?

http://codereview.chromium.org/10914016/diff/4002/ash/wm/session_state_animat...
ash/wm/session_state_animator.h:87: // aura::RootWindowObserver overrides:
On 2012/08/31 17:02:12, Daniel Erat wrote:
> nit: overridden methods should come after all others in the "public" section

Done.

http://codereview.chromium.org/10914016/diff/4002/ash/wm/session_state_animat...
ash/wm/session_state_animator.h:98: AnimationType type);
On 2012/08/31 17:02:12, Daniel Erat wrote:
> nit: add a blank line between a method's signature and the following method's
> comments

Done.

http://codereview.chromium.org/10914016/diff/4002/ash/wm/session_state_animat...
ash/wm/session_state_animator.h:103: 
On 2012/08/31 17:02:12, Daniel Erat wrote:
> nit: remove all of these blank lines

Done.

[Daniel Erat, 2012-09-28 15:00:43]

LGTM after some nits are addressed

http://codereview.chromium.org/10914016/diff/8001/ash/wm/session_state_animat...
File ash/wm/session_state_animator.h (right):

http://codereview.chromium.org/10914016/diff/8001/ash/wm/session_state_animat...
ash/wm/session_state_animator.h:25: namespace ash {
nit: can this go inside of ash::internal?

http://codereview.chromium.org/10914016/diff/8001/ash/wm/session_state_animat...
ash/wm/session_state_animator.h:29: class ASH_EXPORT SessionStateAnimator :
public aura::RootWindowObserver {
nit: is ASH_EXPORT actually needed here?  (i'm not sure one way or the other;
maybe it is for tests)

http://codereview.chromium.org/10914016/diff/8001/ash/wm/session_state_animat...
ash/wm/session_state_animator.h:94: static int GetAllContainersMask();
nit: instead of static methods, this and GetAllLockScreenContainersMask() seem
like they can just be static constant variables, e.g. kAllContainersMask and
kAllLockScreenContainersMask

http://codereview.chromium.org/10914016/diff/8001/ash/wm/session_state_animat...
ash/wm/session_state_animator.h:121: const gfx::Size& old_size) OVERRIDE;
nit: add a blank line after this

[Daniel Erat, 2012-09-28 15:01:42]

Can you improve this change's description, too?  Something like "ash: Extract
animator from PowerButtonController" would be better.

[Nikita (slow), 2012-09-28 19:39:07]

Please don't commit this CL for now.
I have a bug that I would like to fix first:
http://code.google.com/p/chromium/issues/detail?id=150900

Otherwise this re-factoring would complicate merge into M23.

[Nikita (slow), 2012-09-28 20:34:54]

My CL is in progress:
http://codereview.chromium.org/10990118/

[commit-bot: I haz the power, 2012-10-02 12:59:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/antrim@chromium.org/10914016/18001

[Nikita (slow), 2012-10-02 13:29:52]

lgtm
please run cros_x86 / win_aura trybots

CQ doesn't include those

[commit-bot: I haz the power, 2012-10-02 15:33:31]

Change committed as 159690

[commit-bot: I haz the power, 2012-10-03 12:30:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/antrim@chromium.org/10914016/37002

[commit-bot: I haz the power, 2012-10-03 15:40:28]

Change committed as 159896

[commit-bot: I haz the power, 2012-10-04 15:22:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/antrim@chromium.org/10914016/24003

[commit-bot: I haz the power, 2012-10-04 17:43:10]

Change committed as 160161