ash : Decouple power button controller and session state controller.

ash : Decouple power button controller and session state controller.
Prepare code to introduce two versions of SessionStateController later.

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

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

[Daniel Erat, 2012-10-09 17:28:55]

https://codereview.chromium.org/11091023/diff/1/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/11091023/diff/1/ash/shell.cc#newcode49
ash/shell.cc:49: #include "ash/wm/session_state_controller.h"
keep this list alphabetized

https://codereview.chromium.org/11091023/diff/1/ash/wm/power_button_controller.h
File ash/wm/power_button_controller.h (right):

https://codereview.chromium.org/11091023/diff/1/ash/wm/power_button_controlle...
ash/wm/power_button_controller.h:9: #include "ash/shell_observer.h"
remove this if it isn't needed

https://codereview.chromium.org/11091023/diff/1/ash/wm/power_button_controlle...
ash/wm/power_button_controller.h:10: #include "ash/wm/session_state_animator.h"
remove this if it isn't needed

https://codereview.chromium.org/11091023/diff/1/ash/wm/power_button_controlle...
ash/wm/power_button_controller.h:12: #include "base/memory/scoped_ptr.h"
remove this if it isn't needed

https://codereview.chromium.org/11091023/diff/1/ash/wm/power_button_controlle...
ash/wm/power_button_controller.h:14: #include "base/timer.h"
remove this if it isn't needed

https://codereview.chromium.org/11091023/diff/1/ash/wm/power_button_controlle...
ash/wm/power_button_controller.h:15: #include "ui/aura/root_window_observer.h"
remove this if it isn't needed

https://codereview.chromium.org/11091023/diff/1/ash/wm/power_button_controlle...
ash/wm/power_button_controller.h:37: class ASH_EXPORT PowerButtonController :
public aura::RootWindowObserver {
do you still need RootWindowObserver?

https://codereview.chromium.org/11091023/diff/1/ash/wm/session_state_controll...
File ash/wm/session_state_controller.h (right):

https://codereview.chromium.org/11091023/diff/1/ash/wm/session_state_controll...
ash/wm/session_state_controller.h:6: #define ASH_WM_SESSION_STATE_CONTROLLER_H_
is there any new code in this file and session_state_controller.cc, or is it
just code moved from PowerButtonController with s/PowerButton/SessionState/?

https://codereview.chromium.org/11091023/diff/1/chrome/browser/chromeos/chrom...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/11091023/diff/1/chrome/browser/chromeos/chrom...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:51: #include
"chrome/browser/chromeos/power/session_state_controller_delegate_chromeos.h"
is this include needed, or can you just delete it?

[Nikita (slow), 2012-10-09 18:30:05]

https://codereview.chromium.org/11091023/diff/1/ash/wm/power_button_controlle...
File ash/wm/power_button_controller.cc (right):

https://codereview.chromium.org/11091023/diff/1/ash/wm/power_button_controlle...
ash/wm/power_button_controller.cc:53: if (controller_->IsEligibleForLock()) {
nit: Drop {} here now that each block is exactly 1 line.

https://codereview.chromium.org/11091023/diff/1/ash/wm/power_button_controlle...
ash/wm/power_button_controller.cc:92: if (controller_->IsLocked() ||
nit: Fit condition on one line.

https://codereview.chromium.org/11091023/diff/1/ash/wm/power_button_controlle...
ash/wm/power_button_controller.cc:96: if (down) {
nit: Please drop {} here.

https://codereview.chromium.org/11091023/diff/1/ash/wm/power_button_controller.h
File ash/wm/power_button_controller.h (right):

https://codereview.chromium.org/11091023/diff/1/ash/wm/power_button_controlle...
ash/wm/power_button_controller.h:26: 
nit: drop extra empty line

https://codereview.chromium.org/11091023/diff/1/ash/wm/power_button_controlle...
ash/wm/power_button_controller.h:40:
PowerButtonController(SessionStateController* controller);
Add explicit
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Explicit_Const...

https://codereview.chromium.org/11091023/diff/1/ash/wm/power_button_controlle...
File ash/wm/power_button_controller_unittest.cc (right):

https://codereview.chromium.org/11091023/diff/1/ash/wm/power_button_controlle...
ash/wm/power_button_controller_unittest.cc:7: #include
"ash/wm/session_state_animator.h"
nit: Sort

[Denis Kuznetsov (DE-MUC), 2012-10-11 11:37:12]

https://codereview.chromium.org/11091023/diff/1/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/11091023/diff/1/ash/shell.cc#newcode49
ash/shell.cc:49: #include "ash/wm/session_state_controller.h"
On 2012/10/09 17:28:55, Daniel Erat wrote:
> keep this list alphabetized

Done.

https://codereview.chromium.org/11091023/diff/1/ash/wm/power_button_controlle...
File ash/wm/power_button_controller.cc (right):

https://codereview.chromium.org/11091023/diff/1/ash/wm/power_button_controlle...
ash/wm/power_button_controller.cc:53: if (controller_->IsEligibleForLock()) {
On 2012/10/09 18:30:06, Nikita Kostylev wrote:
> nit: Drop {} here now that each block is exactly 1 line.

Done.

https://codereview.chromium.org/11091023/diff/1/ash/wm/power_button_controlle...
ash/wm/power_button_controller.cc:92: if (controller_->IsLocked() ||
On 2012/10/09 18:30:06, Nikita Kostylev wrote:
> nit: Fit condition on one line.

Done.

https://codereview.chromium.org/11091023/diff/1/ash/wm/power_button_controlle...
ash/wm/power_button_controller.cc:96: if (down) {
On 2012/10/09 18:30:06, Nikita Kostylev wrote:
> nit: Please drop {} here.

Done.

https://codereview.chromium.org/11091023/diff/1/ash/wm/power_button_controller.h
File ash/wm/power_button_controller.h (right):

https://codereview.chromium.org/11091023/diff/1/ash/wm/power_button_controlle...
ash/wm/power_button_controller.h:9: #include "ash/shell_observer.h"
On 2012/10/09 17:28:55, Daniel Erat wrote:
> remove this if it isn't needed

Done.

https://codereview.chromium.org/11091023/diff/1/ash/wm/power_button_controlle...
ash/wm/power_button_controller.h:10: #include "ash/wm/session_state_animator.h"
On 2012/10/09 17:28:55, Daniel Erat wrote:
> remove this if it isn't needed

Done.

https://codereview.chromium.org/11091023/diff/1/ash/wm/power_button_controlle...
ash/wm/power_button_controller.h:12: #include "base/memory/scoped_ptr.h"
On 2012/10/09 17:28:55, Daniel Erat wrote:
> remove this if it isn't needed

Done.

https://codereview.chromium.org/11091023/diff/1/ash/wm/power_button_controlle...
ash/wm/power_button_controller.h:14: #include "base/timer.h"
On 2012/10/09 17:28:55, Daniel Erat wrote:
> remove this if it isn't needed

Done.

https://codereview.chromium.org/11091023/diff/1/ash/wm/power_button_controlle...
ash/wm/power_button_controller.h:15: #include "ui/aura/root_window_observer.h"
On 2012/10/09 17:28:55, Daniel Erat wrote:
> remove this if it isn't needed

Done.

https://codereview.chromium.org/11091023/diff/1/ash/wm/power_button_controlle...
ash/wm/power_button_controller.h:26: 
On 2012/10/09 18:30:06, Nikita Kostylev wrote:
> nit: drop extra empty line

Done.

https://codereview.chromium.org/11091023/diff/1/ash/wm/power_button_controlle...
ash/wm/power_button_controller.h:37: class ASH_EXPORT PowerButtonController :
public aura::RootWindowObserver {
On 2012/10/09 17:28:55, Daniel Erat wrote:
> do you still need RootWindowObserver?

Done.

https://codereview.chromium.org/11091023/diff/1/ash/wm/power_button_controlle...
ash/wm/power_button_controller.h:40:
PowerButtonController(SessionStateController* controller);
On 2012/10/09 18:30:06, Nikita Kostylev wrote:
> Add explicit
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Explicit_Const...

Done.

https://codereview.chromium.org/11091023/diff/1/ash/wm/power_button_controlle...
File ash/wm/power_button_controller_unittest.cc (right):

https://codereview.chromium.org/11091023/diff/1/ash/wm/power_button_controlle...
ash/wm/power_button_controller_unittest.cc:7: #include
"ash/wm/session_state_animator.h"
On 2012/10/09 18:30:06, Nikita Kostylev wrote:
> nit: Sort

Done.

https://codereview.chromium.org/11091023/diff/1/ash/wm/session_state_controll...
File ash/wm/session_state_controller.h (right):

https://codereview.chromium.org/11091023/diff/1/ash/wm/session_state_controll...
ash/wm/session_state_controller.h:6: #define ASH_WM_SESSION_STATE_CONTROLLER_H_
Only code moved around, with only small change - addition of
shutdown_after_lock_ flag.

https://codereview.chromium.org/11091023/diff/1/chrome/browser/chromeos/chrom...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/11091023/diff/1/chrome/browser/chromeos/chrom...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:51: #include
"chrome/browser/chromeos/power/session_state_controller_delegate_chromeos.h"
On 2012/10/09 17:28:55, Daniel Erat wrote:
> is this include needed, or can you just delete it?

Done.

[Daniel Erat, 2012-10-11 15:08:30]

here are some early comments

https://codereview.chromium.org/11091023/diff/7003/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/11091023/diff/7003/ash/shell.h#newcode70
ash/shell.h:70: class SessionStateController;
nit: fix alphabetization

https://codereview.chromium.org/11091023/diff/7003/ash/wm/session_state_contr...
File ash/wm/session_state_controller.h (right):

https://codereview.chromium.org/11091023/diff/7003/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:32: namespace internal {
why do you need all of these in the header instead of in an anonymous namespace
in the .cc file?  some of these names, like kSlowCloseAnimMs, seem too generic
to expose publicly in the internal namespace.

https://codereview.chromium.org/11091023/diff/7003/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:144: virtual bool IsEligibleForLock();
can this and many other similar methods be const?

why are all of these methods virtual?

https://codereview.chromium.org/11091023/diff/7003/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:161: virtual void
StartLockAnimationButLockImmediately();
nit: s/But/And/

https://codereview.chromium.org/11091023/diff/7003/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:163: // Returns true if we have requested
system to lock, but havan't recieved
nit: s/havan't/haven't/

https://codereview.chromium.org/11091023/diff/7003/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:168: virtual bool IsLocking();
i think that the IsTryingToLock() vs. IsLocking() vs. CanCancelLock()
distinction is confusing.

how about 

CanCancelLock() -> InLockTimeout()
IsTryingToLock() -> LockRequested()
StartCancellableLock() -> StartLockTimeout()
CancelLock() -> CancelLockTimeout()
remove IsLocking()

?

https://codereview.chromium.org/11091023/diff/7003/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:180: // parameters. Seems to be some bug, but
refactoring should keep all bugs.
add a TODO to remove this; put the explanation there

https://codereview.chromium.org/11091023/diff/7003/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:184: virtual bool CanCancelShutdown();
i recommend the same thing here:

CanCancelShutdown() -> InShutdownTimeout()
CancelShutdown() -> CancelShutdownTimeout()

this makes it clearer from the title that we're just canceling a pre-shutdown
timeout rather than somehow actually canceling shutdown.

https://codereview.chromium.org/11091023/diff/7003/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:189: 
nit: delete extra blank line

https://codereview.chromium.org/11091023/diff/7003/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:196: 
nit: delete extra blank line

https://codereview.chromium.org/11091023/diff/7003/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:199: const aura::RootWindow* root) OVERRIDE;
nit: fix indenting

https://codereview.chromium.org/11091023/diff/7003/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:211: 
nit: delete extra blank line

https://codereview.chromium.org/11091023/diff/7003/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:215: // Timer stuff.
nit: delete unnecessary comment

https://codereview.chromium.org/11091023/diff/7003/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:228: // Calls StartCancellableShutdown().
nit: add blank line before this one

https://codereview.chromium.org/11091023/diff/7003/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:233: // Calls RequestShutdownImpl();
nit: add blank line before this one

[Denis Kuznetsov (DE-MUC), 2012-10-11 16:14:38]

https://codereview.chromium.org/11091023/diff/7003/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/11091023/diff/7003/ash/shell.h#newcode70
ash/shell.h:70: class SessionStateController;
On 2012/10/11 15:08:30, Daniel Erat wrote:
> nit: fix alphabetization

Done.

https://codereview.chromium.org/11091023/diff/7003/ash/wm/session_state_contr...
File ash/wm/session_state_controller.h (right):

https://codereview.chromium.org/11091023/diff/7003/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:32: namespace internal {
I will have two implementations that would reuse these constants.

https://codereview.chromium.org/11091023/diff/7003/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:144: virtual bool IsEligibleForLock();
On 2012/10/11 15:08:30, Daniel Erat wrote:
> can this and many other similar methods be const?
> 
> why are all of these methods virtual?
Same as above - there would be two actual implementations of
session_state_controller, and these would become =0.

https://codereview.chromium.org/11091023/diff/7003/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:161: virtual void
StartLockAnimationButLockImmediately();
On 2012/10/11 15:08:30, Daniel Erat wrote:
> nit: s/But/And/

Done.

https://codereview.chromium.org/11091023/diff/7003/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:163: // Returns true if we have requested
system to lock, but havan't recieved
On 2012/10/11 15:08:30, Daniel Erat wrote:
> nit: s/havan't/haven't/

Done.

https://codereview.chromium.org/11091023/diff/7003/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:168: virtual bool IsLocking();
On 2012/10/11 15:08:30, Daniel Erat wrote:
> i think that the IsTryingToLock() vs. IsLocking() vs. CanCancelLock()
> distinction is confusing.
> 
> how about 
> 
> CanCancelLock() -> InLockTimeout()
> IsTryingToLock() -> LockRequested()
> StartCancellableLock() -> StartLockTimeout()
> CancelLock() -> CancelLockTimeout()
> remove IsLocking()
> 
> ?

Done.

https://codereview.chromium.org/11091023/diff/7003/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:180: // parameters. Seems to be some bug, but
refactoring should keep all bugs.
On 2012/10/11 15:08:30, Daniel Erat wrote:
> add a TODO to remove this; put the explanation there

Done.

https://codereview.chromium.org/11091023/diff/7003/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:184: virtual bool CanCancelShutdown();
On 2012/10/11 15:08:30, Daniel Erat wrote:
> i recommend the same thing here:
> 
> CanCancelShutdown() -> InShutdownTimeout()
> CancelShutdown() -> CancelShutdownTimeout()
> 
> this makes it clearer from the title that we're just canceling a pre-shutdown
> timeout rather than somehow actually canceling shutdown.

Done.

https://codereview.chromium.org/11091023/diff/7003/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:189: 
On 2012/10/11 15:08:30, Daniel Erat wrote:
> nit: delete extra blank line

Done.

https://codereview.chromium.org/11091023/diff/7003/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:196: 
On 2012/10/11 15:08:30, Daniel Erat wrote:
> nit: delete extra blank line

Done.

https://codereview.chromium.org/11091023/diff/7003/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:199: const aura::RootWindow* root) OVERRIDE;
On 2012/10/11 15:08:30, Daniel Erat wrote:
> nit: fix indenting

Done.

https://codereview.chromium.org/11091023/diff/7003/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:211: 
On 2012/10/11 15:08:30, Daniel Erat wrote:
> nit: delete extra blank line

Done.

https://codereview.chromium.org/11091023/diff/7003/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:215: // Timer stuff.
On 2012/10/11 15:08:30, Daniel Erat wrote:
> nit: delete unnecessary comment

Done.

https://codereview.chromium.org/11091023/diff/7003/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:228: // Calls StartCancellableShutdown().
On 2012/10/11 15:08:30, Daniel Erat wrote:
> nit: add blank line before this one

Done.

https://codereview.chromium.org/11091023/diff/7003/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:233: // Calls RequestShutdownImpl();
On 2012/10/11 15:08:30, Daniel Erat wrote:
> nit: add blank line before this one

Done.

[Daniel Erat, 2012-10-11 16:16:42]

https://codereview.chromium.org/11091023/diff/7003/ash/wm/session_state_contr...
File ash/wm/session_state_controller.h (right):

https://codereview.chromium.org/11091023/diff/7003/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:32: namespace internal {
On 2012/10/11 16:14:38, Denis Kuznetsov wrote:
> I will have two implementations that would reuse these constants.

please make them be protected static members of the SessionStateController
class, then.

[Daniel Erat, 2012-10-11 16:26:56]

https://codereview.chromium.org/11091023/diff/9002/ash/wm/session_state_contr...
File ash/wm/session_state_controller.cc (right):

https://codereview.chromium.org/11091023/diff/9002/ash/wm/session_state_contr...
ash/wm/session_state_controller.cc:231: 
nit: remove extra blank line

https://codereview.chromium.org/11091023/diff/9002/ash/wm/session_state_contr...
ash/wm/session_state_controller.cc:275: // ----------- Timer stuff.
nit: delete this

https://codereview.chromium.org/11091023/diff/9002/ash/wm/session_state_contr...
File ash/wm/session_state_controller.h (right):

https://codereview.chromium.org/11091023/diff/9002/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:150: // After locking and
|kLockToShutdownTimeoutMs| StartCancellableShutdown()
s/StartCancellableShutdown/StartShutdownAnimation/, right?

https://codereview.chromium.org/11091023/diff/9002/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:168: virtual bool IsShuttingDown();
ShutdownRequested()?

https://codereview.chromium.org/11091023/diff/9002/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:178: //ToDo(antrim): remove this, animations
should actually be the same.
nit: should be:

// TODO(antrim): ...

https://codereview.chromium.org/11091023/diff/9002/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:191: virtual void RequestShutdown();
do you really need both this and ShutdownImmediately()?  they seem to do
essentially the same thing

https://codereview.chromium.org/11091023/diff/9002/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:193: 
nit: delete extra blank line

https://codereview.chromium.org/11091023/diff/9002/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:222: // Calls StartCancellableShutdown().
nit: add blank line before this line

[Denis Kuznetsov (DE-MUC), 2012-10-15 11:49:58]

https://codereview.chromium.org/11091023/diff/9002/ash/wm/session_state_contr...
File ash/wm/session_state_controller.cc (right):

https://codereview.chromium.org/11091023/diff/9002/ash/wm/session_state_contr...
ash/wm/session_state_controller.cc:231: 
On 2012/10/11 16:26:56, Daniel Erat wrote:
> nit: remove extra blank line

Done.

https://codereview.chromium.org/11091023/diff/9002/ash/wm/session_state_contr...
ash/wm/session_state_controller.cc:275: // ----------- Timer stuff.
On 2012/10/11 16:26:56, Daniel Erat wrote:
> nit: delete this

Done.

https://codereview.chromium.org/11091023/diff/9002/ash/wm/session_state_contr...
File ash/wm/session_state_controller.h (right):

https://codereview.chromium.org/11091023/diff/9002/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:150: // After locking and
|kLockToShutdownTimeoutMs| StartCancellableShutdown()
On 2012/10/11 16:26:56, Daniel Erat wrote:
> s/StartCancellableShutdown/StartShutdownAnimation/, right?

Done.

https://codereview.chromium.org/11091023/diff/9002/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:168: virtual bool IsShuttingDown();
On 2012/10/11 16:26:56, Daniel Erat wrote:
> ShutdownRequested()?

Done.

https://codereview.chromium.org/11091023/diff/9002/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:178: //ToDo(antrim): remove this, animations
should actually be the same.
On 2012/10/11 16:26:56, Daniel Erat wrote:
> nit: should be:
> 
> // TODO(antrim): ...

Done.

https://codereview.chromium.org/11091023/diff/9002/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:191: virtual void RequestShutdown();
On 2012/10/11 16:26:56, Daniel Erat wrote:
> do you really need both this and ShutdownImmediately()?  they seem to do
> essentially the same thing
Good catch, I've missed it.

https://codereview.chromium.org/11091023/diff/9002/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:193: 
On 2012/10/11 16:26:56, Daniel Erat wrote:
> nit: delete extra blank line

Done.

https://codereview.chromium.org/11091023/diff/9002/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:222: // Calls StartCancellableShutdown().
On 2012/10/11 16:26:56, Daniel Erat wrote:
> nit: add blank line before this line

Done.

[Daniel Erat, 2012-10-15 13:24:47]

LGTM with a few more comments

https://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_cont...
File ash/wm/session_state_controller.cc (right):

https://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_cont...
ash/wm/session_state_controller.cc:73: if (shutting_down_ || (login_status_ ==
user::LOGGED_IN_LOCKED) == locked)
nit: i think you can change "(login_status_ == user::LOGGED_IN_LOCKED)" to
IsLocked()

https://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_cont...
ash/wm/session_state_controller.cc:156: return LoggedInAsNonGuest() &&
!IsLocked();
should this also have && !LockRequested()?

https://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_cont...
ash/wm/session_state_controller.cc:345: delegate->Exit();
should there be a return within this "if (delegate)" block?  calling Exit() and
then RequestShutdown() immediately afterwards seems incorrect

https://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_cont...
File ash/wm/session_state_controller.h (right):

https://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_cont...
ash/wm/session_state_controller.h:149: // will be called unless CancelShutdown()
is called.
nit: this is a TODO at best rather than something that should happen in this
change, but i wonder if it'd be cleaner if this method didn't automatically
start the shutdown animation, and the caller had to call StartShutdownAnimation
explicitly instead.  opinions?

https://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_cont...
ash/wm/session_state_controller.h:155: // Starts usual lock animation, but lock
immediately.
nit: s/lock/locks/

also describe whether StartLockAnimation will be called automatically as in
StartLockAnimation or not

https://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_cont...
ash/wm/session_state_controller.h:158: // Returns true if we have requested
system to lock, but haven't recieved
nit: s/recieved/received/

https://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_cont...
ash/wm/session_state_controller.h:235: // Original login status during locked. 
LOGGED_IN_NONE if it's not locked.
nit: s/during locked/from before we locked/

[Nikita (slow), 2012-10-16 12:43:45]

lgtm with few comments.

http://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_contr...
File ash/wm/session_state_controller.cc (right):

http://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_contr...
ash/wm/session_state_controller.cc:19: 
nit: drop empty line.

http://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_contr...
ash/wm/session_state_controller.cc:31: 
Please add these:

/////////////////////////////////////////////////////////
// SessionStateController, public:

/////////////////////////////////////////////////////////
// SessionStateController::ShellObserver:

/////////////////////////////////////////////////////////
// SessionStateController, private:

etc.

See shell.cc for example

http://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_contr...
ash/wm/session_state_controller.cc:270: if (login_status_ ==
user::LOGGED_IN_NONE)
Could just use

return (login_status_ != user::LOGGED_IN_NONE) && 
       (login_status_ != user::LOGGED_IN_GUEST);

http://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_contr...
File ash/wm/session_state_controller.h (right):

http://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:138: delegate_.reset(delegate);
That's not a trivial setter as

  void reset(C* p = NULL) {
    if (p != ptr_) {
      enum { type_must_be_complete = sizeof(C) };
      delete ptr_;
      ptr_ = p;
    }
  }

Please move implementation to .cc and rename to SetDelegate

http://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:189: virtual void
OnRootWindowHostCloseRequested(
Place these overrides first. Then other public methods.
Or as an alternative you should place these into protected: section.

http://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:193: virtual void
OnLoginStateChanged(user::LoginStatus status) OVERRIDE;
Same with these.

http://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:200: bool LoggedInAsNonGuest() const;
nit: IsLoggedInAsNonGuest()

http://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:206: void StartLockTimer();
Just for the record (I'm not asking to fix this in this class though as you have
another CL depending on it): when creating new class,  it is nice to order all
methods alphabetically in each section (public/protected/private).

http://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:236: user::LoginStatus unlocked_login_status_;
I find having these two login_status_ /  unlocked_login_status_  quite
confusing.

I think you should simplify that by having just login_status_ to track logged in
/ not logged in / as guest / etc. and you need just boolean variable locked_.

I know this has been part of existing code but since you're doing a refactoring
now's the right time to simplify this.

[Daniel Erat, 2012-10-16 14:04:16]

comments about comments

http://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_contr...
File ash/wm/session_state_controller.cc (right):

http://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_contr...
ash/wm/session_state_controller.cc:31: 
On 2012/10/16 12:43:45, Nikita Kostylev wrote:
> Please add these:
> 
> /////////////////////////////////////////////////////////
> // SessionStateController, public:
> 
> /////////////////////////////////////////////////////////
> // SessionStateController::ShellObserver:
> 
> /////////////////////////////////////////////////////////
> // SessionStateController, private:
> 
> etc.
> 
> See shell.cc for example

I'm actually curious: what's the point of putting comments like these in the .cc
file?  The header is the canonical source of this information, so this just
feels like duplication.  Doesn't everyone just use search if they're looking for
a particular method within the .cc file?

http://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_contr...
File ash/wm/session_state_controller.h (right):

http://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:189: virtual void
OnRootWindowHostCloseRequested(
On 2012/10/16 12:43:45, Nikita Kostylev wrote:
> Place these overrides first. Then other public methods.
> Or as an alternative you should place these into protected: section.

I think that overrides usually come after non-overridden method in each section,
not before.  I can't find this in the style guide, though.  Please check
surrounding code, but I'm pretty sure that what's currently here in this change
is preferred.

http://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_contr...
ash/wm/session_state_controller.h:206: void StartLockTimer();
On 2012/10/16 12:43:45, Nikita Kostylev wrote:
> Just for the record (I'm not asking to fix this in this class though as you
have
> another CL depending on it): when creating new class,  it is nice to order all
> methods alphabetically in each section (public/protected/private).

I need to disagree with this one too -- I find logical grouping (e.g. lock
methods, then shutdown methods, etc.) more intuitive.  I haven't noticed
alphabetical method ordering anywhere else in Chrome.

[Denis Kuznetsov (DE-MUC), 2012-10-16 14:24:53]

https://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_cont...
File ash/wm/session_state_controller.cc (right):

https://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_cont...
ash/wm/session_state_controller.cc:19: 
On 2012/10/16 12:43:45, Nikita Kostylev wrote:
> nit: drop empty line.

Done.

https://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_cont...
ash/wm/session_state_controller.cc:73: if (shutting_down_ || (login_status_ ==
user::LOGGED_IN_LOCKED) == locked)
On 2012/10/15 13:24:47, Daniel Erat wrote:
> nit: i think you can change "(login_status_ == user::LOGGED_IN_LOCKED)" to
> IsLocked()

Done.

https://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_cont...
ash/wm/session_state_controller.cc:156: return LoggedInAsNonGuest() &&
!IsLocked();
On 2012/10/15 13:24:47, Daniel Erat wrote:
> should this also have && !LockRequested()?
It seems logical, I will add this, but I thought about this CL as about
refactoring (just  moving code around, without changes).

https://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_cont...
ash/wm/session_state_controller.cc:270: if (login_status_ ==
user::LOGGED_IN_NONE)
On 2012/10/16 12:43:45, Nikita Kostylev wrote:
> Could just use
> 
> return (login_status_ != user::LOGGED_IN_NONE) && 
>        (login_status_ != user::LOGGED_IN_GUEST);

Done.

https://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_cont...
ash/wm/session_state_controller.cc:345: delegate->Exit();
On 2012/10/15 13:24:47, Daniel Erat wrote:
> should there be a return within this "if (delegate)" block?  calling Exit()
and
> then RequestShutdown() immediately afterwards seems incorrect

Sounds reasonable.

https://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_cont...
File ash/wm/session_state_controller.h (right):

https://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_cont...
ash/wm/session_state_controller.h:138: delegate_.reset(delegate);
On 2012/10/16 12:43:45, Nikita Kostylev wrote:
> That's not a trivial setter as
> 
>   void reset(C* p = NULL) {
>     if (p != ptr_) {
>       enum { type_must_be_complete = sizeof(C) };
>       delete ptr_;
>       ptr_ = p;
>     }
>   }
> 
> Please move implementation to .cc and rename to SetDelegate

Done.

https://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_cont...
ash/wm/session_state_controller.h:149: // will be called unless CancelShutdown()
is called.
On 2012/10/15 13:24:47, Daniel Erat wrote:
> nit: this is a TODO at best rather than something that should happen in this
> change, but i wonder if it'd be cleaner if this method didn't automatically
> start the shutdown animation, and the caller had to call
StartShutdownAnimation
> explicitly instead.  opinions?
I'd like to have PowerButtonController as stateless as possible - it's just a
source of events for SessionStateController.
If StartShutdownAnimation should be called from outside, then 
PowerButtonController should keep it's own timer, that have to be matched with
animation (lock animation time + a bit of extra delay time). If by some reason
we would have two different lock animation times for two implementaions, code
would become much more coupled (e.g. there might appear something like
GetLockAnimationDuration() method).

https://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_cont...
ash/wm/session_state_controller.h:155: // Starts usual lock animation, but lock
immediately.
On 2012/10/15 13:24:47, Daniel Erat wrote:
> nit: s/lock/locks/
> 
> also describe whether StartLockAnimation will be called automatically as in
> StartLockAnimation or not

Done.

https://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_cont...
ash/wm/session_state_controller.h:155: // Starts usual lock animation, but lock
immediately.
On 2012/10/15 13:24:47, Daniel Erat wrote:
> nit: s/lock/locks/
> 
> also describe whether StartLockAnimation will be called automatically as in
> StartLockAnimation or not

Done.

https://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_cont...
ash/wm/session_state_controller.h:158: // Returns true if we have requested
system to lock, but haven't recieved
On 2012/10/15 13:24:47, Daniel Erat wrote:
> nit: s/recieved/received/

Done.

https://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_cont...
ash/wm/session_state_controller.h:189: virtual void
OnRootWindowHostCloseRequested(
On 2012/10/16 12:43:45, Nikita Kostylev wrote:
> Place these overrides first. Then other public methods.
> Or as an alternative you should place these into protected: section.

Done.

https://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_cont...
ash/wm/session_state_controller.h:193: virtual void
OnLoginStateChanged(user::LoginStatus status) OVERRIDE;
On 2012/10/16 12:43:45, Nikita Kostylev wrote:
> Same with these.

Done.

https://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_cont...
ash/wm/session_state_controller.h:200: bool LoggedInAsNonGuest() const;
On 2012/10/16 12:43:45, Nikita Kostylev wrote:
> nit: IsLoggedInAsNonGuest()

Done.

https://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_cont...
ash/wm/session_state_controller.h:206: void StartLockTimer();
On 2012/10/16 12:43:45, Nikita Kostylev wrote:
> Just for the record (I'm not asking to fix this in this class though as you
have
> another CL depending on it): when creating new class,  it is nice to order all
> methods alphabetically in each section (public/protected/private).

https://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_cont...
ash/wm/session_state_controller.h:235: // Original login status during locked. 
LOGGED_IN_NONE if it's not locked.
On 2012/10/15 13:24:47, Daniel Erat wrote:
> nit: s/during locked/from before we locked/

Done.

https://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_cont...
ash/wm/session_state_controller.h:236: user::LoginStatus unlocked_login_status_;
On 2012/10/16 12:43:45, Nikita Kostylev wrote:
> I find having these two login_status_ /  unlocked_login_status_  quite
> confusing. think you should simplify that by having just login_status_ to
track logged in
> / not logged in / as guest / etc. and you need just boolean variable locked_.
> 
> I know this has been part of existing code but since you're doing a
refactoring
> now's the right time to simplify this.

Will think about fixing it in next CL.

[Nikita (slow), 2012-10-16 14:29:01]

still lgtm

More of these seems to be not a strict style guide but a personal preference -
see more inline.

On 2012/10/16 14:04:16, Daniel Erat wrote:
> comments about comments
> 
>
http://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_contr...
> File ash/wm/session_state_controller.cc (right):
> 
>
http://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_contr...
> ash/wm/session_state_controller.cc:31: 
> On 2012/10/16 12:43:45, Nikita Kostylev wrote:
> > Please add these:
> > 
> > /////////////////////////////////////////////////////////
> > // SessionStateController, public:
> > 
> > /////////////////////////////////////////////////////////
> > // SessionStateController::ShellObserver:
> > 
> > /////////////////////////////////////////////////////////
> > // SessionStateController, private:
> > 
> > etc.
> > 
> > See shell.cc for example
> 
> I'm actually curious: what's the point of putting comments like these in the
.cc
> file?  The header is the canonical source of this information, so this just
> feels like duplication.  Doesn't everyone just use search if they're looking
for
> a particular method within the .cc file?

This seems to be more of the personal preference but I've seen this multiple
times over the Chromium codebase.
For me it makes it .cc file a bit structured.

> 
>
http://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_contr...
> File ash/wm/session_state_controller.h (right):
> 
>
http://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_contr...
> ash/wm/session_state_controller.h:189: virtual void
> OnRootWindowHostCloseRequested(
> On 2012/10/16 12:43:45, Nikita Kostylev wrote:
> > Place these overrides first. Then other public methods.
> > Or as an alternative you should place these into protected: section.
> 
> I think that overrides usually come after non-overridden method in each
section,
> not before.  I can't find this in the style guide, though.  Please check
> surrounding code, but I'm pretty sure that what's currently here in this
change
> is preferred.

You're right, that's more clear and my comment should be ignored then.

> 
>
http://codereview.chromium.org/11091023/diff/14001/ash/wm/session_state_contr...
> ash/wm/session_state_controller.h:206: void StartLockTimer();
> On 2012/10/16 12:43:45, Nikita Kostylev wrote:
> > Just for the record (I'm not asking to fix this in this class though as you
> have
> > another CL depending on it): when creating new class,  it is nice to order
all
> > methods alphabetically in each section (public/protected/private).
> 
> I need to disagree with this one too -- I find logical grouping (e.g. lock
> methods, then shutdown methods, etc.) more intuitive.  I haven't noticed
> alphabetical method ordering anywhere else in Chrome.

It seems to be that we're migrating to having a logical grouping but previously
most of the header files I've come across had methods sorted and some reviewers
specifically asked for that. I'm ok with leaving as is and actually grouping the
way they are.
Just for record, current order is:

IsEligibleForLock();
IsLocked();

StartLockAnimation();
StartShutdownAnimation();
StartLockAnimationAndLockImmediately();

LockRequested();
ShutdownRequested();

CanCancelLockAnimation();
CancelLockAnimation();
CancelLockWithOtherAnimation()
CanCancelShutdownAnimation()
CancelShutdownAnimation();

OnStartingLock();
RequestShutdown();

[commit-bot: I haz the power, 2012-10-16 14:29:02]

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

[commit-bot: I haz the power, 2012-10-16 14:29:28]

Presubmit check for 11091023-27001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 1.9s to calculate.

[sky, 2012-10-16 14:32:36]

LGTM

[commit-bot: I haz the power, 2012-10-16 14:33:32]

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

[commit-bot: I haz the power, 2012-10-16 16:27:57]

Change committed as 162140

[Nikita (slow), 2012-10-16 19:27:19]

Last patchset has linux_chromeos trybot run but that was done via CQ so
ash_unittests were not executed.

Previous full linux_chromeos trybot run was done in patchset 3.

Please make sure that you do a full run of these bots before commit:

linux_chromeos
linux_chromeos_clang
cros_x86

[Daniel Erat, 2012-10-16 23:54:19]

On 2012/10/16 19:27:19, Nikita Kostylev wrote:
> Last patchset has linux_chromeos trybot run but that was done via CQ so
> ash_unittests were not executed.
> 
> Previous full linux_chromeos trybot run was done in patchset 3.
> 
> Please make sure that you do a full run of these bots before commit:
> 
> linux_chromeos
> linux_chromeos_clang
> cros_x86

I think that this has been reverted now, but please make sure that you're
testing this code on a live device.  In a build at r162150, when I hold the
power button until the machine locks and then let go of it, it shuts down
unconditionally.

[commit-bot: I haz the power, 2012-10-17 13:59:14]

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

[Daniel Erat, 2012-10-17 14:36:38]

Please do not commit this without addressing the issue that I pointed
out over email yesterday!

On Wed, Oct 17, 2012 at 6:59 AM,  <commit-bot@chromium.org> wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/antrim@chromium.org/11091023/33002
>
>
> https://chromiumcodereview.appspot.com/11091023/

[commit-bot: I haz the power, 2012-10-18 07:08:12]

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

[commit-bot: I haz the power, 2012-10-18 08:58:15]

Change committed as 162675