Cancel partial screenshot UI when lock happens.

Cancel partial screenshot UI when lock happens.


BUG=122901
TEST=manually, aura_shell_unittests passed

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=133164

[sky, 2012-04-13 15:43:47]

I don't like tying these two classes together. Could you add a
notification/observer so that the partial screen shot code can listen cancel
itself when the screen is locked?

[Jun Mukai, 2012-04-16 05:33:53]

Added methods to ShellObserver.  Not sure this is the right class to be added
though...

[sky, 2012-04-16 14:47:44]

http://codereview.chromium.org/10008074/diff/6001/ash/shell_observer.h
File ash/shell_observer.h (right):

http://codereview.chromium.org/10008074/diff/6001/ash/shell_observer.h#newcode19
ash/shell_observer.h:19: virtual void OnLoginStateChange(bool logged_in, bool
is_guest) {}
This should be past tense: OnLoginStateChanged

http://codereview.chromium.org/10008074/diff/6001/ash/shell_observer.h#newcode22
ash/shell_observer.h:22: virtual void OnExit() {}
Can this match the notification name: OnAppTerminating.

http://codereview.chromium.org/10008074/diff/6001/ash/shell_observer.h#newcode26
ash/shell_observer.h:26: virtual void OnLockStateChange(bool locked) {}
Paste tense too: OnLockStateChanged.

[Jun Mukai, 2012-04-17 03:34:39]

http://codereview.chromium.org/10008074/diff/6001/ash/shell_observer.h
File ash/shell_observer.h (right):

http://codereview.chromium.org/10008074/diff/6001/ash/shell_observer.h#newcode19
ash/shell_observer.h:19: virtual void OnLoginStateChange(bool logged_in, bool
is_guest) {}
On 2012/04/16 14:47:44, sky wrote:
> This should be past tense: OnLoginStateChanged

Done.

http://codereview.chromium.org/10008074/diff/6001/ash/shell_observer.h#newcode22
ash/shell_observer.h:22: virtual void OnExit() {}
On 2012/04/16 14:47:44, sky wrote:
> Can this match the notification name: OnAppTerminating.

Done.

http://codereview.chromium.org/10008074/diff/6001/ash/shell_observer.h#newcode26
ash/shell_observer.h:26: virtual void OnLockStateChange(bool locked) {}
On 2012/04/16 14:47:44, sky wrote:
> Paste tense too: OnLockStateChanged.

Done.

[Jun Mukai, 2012-04-18 12:52:04]

PTAL

[sky, 2012-04-18 14:41:11]

http://codereview.chromium.org/10008074/diff/9001/ash/shell_observer.h
File ash/shell_observer.h (right):

http://codereview.chromium.org/10008074/diff/9001/ash/shell_observer.h#newcode19
ash/shell_observer.h:19: virtual void OnLoginStateChanged(bool logged_in, bool
is_guest) {}
Are logged_in and is_guest mutually exclusive? Wouldn't an enum be better here?

[Jun Mukai, 2012-04-18 14:54:28]

On 2012/04/18 14:41:11, sky wrote:
> http://codereview.chromium.org/10008074/diff/9001/ash/shell_observer.h
> File ash/shell_observer.h (right):
> 
>
http://codereview.chromium.org/10008074/diff/9001/ash/shell_observer.h#newcode19
> ash/shell_observer.h:19: virtual void OnLoginStateChanged(bool logged_in, bool
> is_guest) {}
> Are logged_in and is_guest mutually exclusive? Wouldn't an enum be better
here?

This respects the one in power_button_controller.h which was introduced by
derat.
derat, what do you think?  I'm thinking to switch to enum unless you have
objections.

[Jun Mukai, 2012-04-18 15:41:00]

uploaded patchset5, which uses an enum for OnLoginStateChanged.

http://codereview.chromium.org/10008074/diff/9001/ash/shell_observer.h
File ash/shell_observer.h (right):

http://codereview.chromium.org/10008074/diff/9001/ash/shell_observer.h#newcode19
ash/shell_observer.h:19: virtual void OnLoginStateChanged(bool logged_in, bool
is_guest) {}
On 2012/04/18 14:41:12, sky wrote:
> Are logged_in and is_guest mutually exclusive? Wouldn't an enum be better
here?

Done.

[sky, 2012-04-18 16:03:31]

http://codereview.chromium.org/10008074/diff/14001/ash/shell.h
File ash/shell.h (right):

http://codereview.chromium.org/10008074/diff/14001/ash/shell.h#newcode100
ash/shell.h:100: enum LoginStatus {
Move this into it's own file so that folks don't need to include shell.h to
access it.

http://codereview.chromium.org/10008074/diff/14001/ash/wm/power_button_contro...
File ash/wm/power_button_controller.cc (right):

http://codereview.chromium.org/10008074/diff/14001/ash/wm/power_button_contro...
ash/wm/power_button_controller.cc:294: logged_in_ = (status !=
Shell::NOT_LOGGED_IN);
Can logged_in_ and is_guest_ be converted to LoginStatus too?

[Daniel Erat, 2012-04-18 16:50:42]

Thanks for cleaning this up!  I'm fine with it going in after Scott is happy.

http://codereview.chromium.org/10008074/diff/14001/ash/shell.h
File ash/shell.h (right):

http://codereview.chromium.org/10008074/diff/14001/ash/shell.h#newcode102
ash/shell.h:102: LOGGED_IN,
It'd probably be clearer to name this LOGGED_IN_AS_USER or something similar. 
Otherwise, someone could see the LOGGED_IN value without being aware of
LOGGED_IN_AS_GUEST and figure that LOGGED_IN covered the guest case as well.

[sadrul, 2012-04-18 16:53:20]

http://codereview.chromium.org/10008074/diff/14001/ash/shell.h
File ash/shell.h (right):

http://codereview.chromium.org/10008074/diff/14001/ash/shell.h#newcode104
ash/shell.h:104: };
Can you please use ash::user::LoginStatus instead? (in
ash/system/user/login_status.h)

[Jun Mukai, 2012-04-19 06:19:25]

http://codereview.chromium.org/10008074/diff/14001/ash/shell.h
File ash/shell.h (right):

http://codereview.chromium.org/10008074/diff/14001/ash/shell.h#newcode100
ash/shell.h:100: enum LoginStatus {
On 2012/04/18 16:03:31, sky wrote:
> Move this into it's own file so that folks don't need to include shell.h to
> access it.

used system/user/login_status instead, as sadrul suggested.

http://codereview.chromium.org/10008074/diff/14001/ash/shell.h#newcode104
ash/shell.h:104: };
On 2012/04/18 16:53:20, sadrul wrote:
> Can you please use ash::user::LoginStatus instead? (in
> ash/system/user/login_status.h)

Done.

http://codereview.chromium.org/10008074/diff/14001/ash/wm/power_button_contro...
File ash/wm/power_button_controller.cc (right):

http://codereview.chromium.org/10008074/diff/14001/ash/wm/power_button_contro...
ash/wm/power_button_controller.cc:294: logged_in_ = (status !=
Shell::NOT_LOGGED_IN);
On 2012/04/18 16:03:31, sky wrote:
> Can logged_in_ and is_guest_ be converted to LoginStatus too?

Done.

[sky, 2012-04-19 15:43:29]

LGTM

http://codereview.chromium.org/10008074/diff/20010/ash/wm/power_button_contro...
File ash/wm/power_button_controller.cc (right):

http://codereview.chromium.org/10008074/diff/20010/ash/wm/power_button_contro...
ash/wm/power_button_controller.cc:472: DCHECK(login_status_ !=
user::LOGGED_IN_LOCKED);
DCHECK_NE

http://codereview.chromium.org/10008074/diff/20010/ash/wm/power_button_contro...
ash/wm/power_button_controller.cc:480: DCHECK(login_status_ ==
user::LOGGED_IN_LOCKED);
DCHECK_EQ

http://codereview.chromium.org/10008074/diff/20010/chrome/browser/chromeos/po...
File chrome/browser/chromeos/power/power_button_observer.cc (right):

http://codereview.chromium.org/10008074/diff/20010/chrome/browser/chromeos/po...
chrome/browser/chromeos/power/power_button_observer.cc:66:
ash::user::LoginStatus login_status = user->is_guest() ?
this is nearly identical to 43-48. Refactor into a common function. (it can be
in an anonmyous namespace in this file if you want, or if there is some more
central place put it there).

[Jun Mukai, 2012-04-20 07:27:33]

http://codereview.chromium.org/10008074/diff/20010/ash/wm/power_button_contro...
File ash/wm/power_button_controller.cc (right):

http://codereview.chromium.org/10008074/diff/20010/ash/wm/power_button_contro...
ash/wm/power_button_controller.cc:472: DCHECK(login_status_ !=
user::LOGGED_IN_LOCKED);
On 2012/04/19 15:43:29, sky wrote:
> DCHECK_NE

Done.

http://codereview.chromium.org/10008074/diff/20010/ash/wm/power_button_contro...
ash/wm/power_button_controller.cc:480: DCHECK(login_status_ ==
user::LOGGED_IN_LOCKED);
On 2012/04/19 15:43:29, sky wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/10008074/diff/20010/chrome/browser/chromeos/po...
File chrome/browser/chromeos/power/power_button_observer.cc (right):

http://codereview.chromium.org/10008074/diff/20010/chrome/browser/chromeos/po...
chrome/browser/chromeos/power/power_button_observer.cc:66:
ash::user::LoginStatus login_status = user->is_guest() ?
On 2012/04/19 15:43:29, sky wrote:
> this is nearly identical to 43-48. Refactor into a common function. (it can be
> in an anonmyous namespace in this file if you want, or if there is some more
> central place put it there).

Done.