Unify and change logout/sleep/lock shortcuts

Unify and change logout/sleep/lock shortcuts

This CL implements an exit warning and requires the user to hold
the exit shortcut for a while, before actually exiting. This is
to prevent accidental exits due shortcut mistakes.

Expert user can exit quickly with a double press.

BUG=225903
R=sky@chromium.org
TEST=manual
out/Debug/ash_unittests --gtest_filter='AcceleratorControllerTest.ExitWarningHandlerTest*'

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

[sky, 2013-05-13 15:40:32]

You need some test coverage too.

https://codereview.chromium.org/14587007/diff/9001/ash/accelerators/accelerat...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/14587007/diff/9001/ash/accelerators/accelerat...
ash/accelerators/accelerator_controller.cc:373: exit_warning_handler_(new
ExitWarningHandler) {
If the lifetime of ExitWarningHandler is that of AcceleratorController then
there is no point in scoped_ptr.

https://codereview.chromium.org/14587007/diff/9001/ash/accelerators/exit_warn...
File ash/accelerators/exit_warning_handler.cc (right):

https://codereview.chromium.org/14587007/diff/9001/ash/accelerators/exit_warn...
ash/accelerators/exit_warning_handler.cc:39: width_ = text_width_ + 100;
Where does the 100 come from? Are you sure you want that for all locales?

https://codereview.chromium.org/14587007/diff/9001/ash/accelerators/exit_warn...
ash/accelerators/exit_warning_handler.cc:56: base::string16 text_;
Any reason you're not using a views::Label* to draw this?

https://codereview.chromium.org/14587007/diff/9001/ash/accelerators/exit_warn...
ash/accelerators/exit_warning_handler.cc:61: };
DISALLOW_...

https://codereview.chromium.org/14587007/diff/9001/ash/accelerators/exit_warn...
File ash/accelerators/exit_warning_handler.h (right):

https://codereview.chromium.org/14587007/diff/9001/ash/accelerators/exit_warn...
ash/accelerators/exit_warning_handler.h:5: #ifndef ASH_EXIT_WARNING_HANDLER_H_
You missed the accelerators here, eg ASH_ACCELERATORS...

https://codereview.chromium.org/14587007/diff/9001/ash/accelerators/exit_warn...
ash/accelerators/exit_warning_handler.h:22: // for immediate exit. The press,
release and press must happend within
happend -> happen

https://codereview.chromium.org/14587007/diff/9001/ash/accelerators/exit_warn...
ash/accelerators/exit_warning_handler.h:72: 
nit: no newline here.

https://codereview.chromium.org/14587007/diff/9001/ash/accelerators/exit_warn...
ash/accelerators/exit_warning_handler.h:77: void HandleExitKey(bool press);
Document what press is.

https://codereview.chromium.org/14587007/diff/9001/ash/accelerators/exit_warn...
ash/accelerators/exit_warning_handler.h:80: void Timer1Action();
These need a description.

https://codereview.chromium.org/14587007/diff/9001/ash/accelerators/exit_warn...
ash/accelerators/exit_warning_handler.h:89: enum State {
enums should be first in section.

https://codereview.chromium.org/14587007/diff/9001/ash/accelerators/exit_warn...
ash/accelerators/exit_warning_handler.h:99: views::Widget* widget_;
Document ownership.

https://codereview.chromium.org/14587007/diff/9001/ash/accelerators/exit_warn...
ash/accelerators/exit_warning_handler.h:102: };
DISALLOW_...

[sschmitz, 2013-05-14 16:49:10]

https://codereview.chromium.org/14587007/diff/9001/ash/accelerators/accelerat...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/14587007/diff/9001/ash/accelerators/accelerat...
ash/accelerators/accelerator_controller.cc:373: exit_warning_handler_(new
ExitWarningHandler) {
On 2013/05/13 15:40:32, sky wrote:
> If the lifetime of ExitWarningHandler is that of AcceleratorController then
> there is no point in scoped_ptr.

Done.

https://codereview.chromium.org/14587007/diff/9001/ash/accelerators/exit_warn...
File ash/accelerators/exit_warning_handler.cc (right):

https://codereview.chromium.org/14587007/diff/9001/ash/accelerators/exit_warn...
ash/accelerators/exit_warning_handler.cc:39: width_ = text_width_ + 100;
On 2013/05/13 15:40:32, sky wrote:
> Where does the 100 come from? Are you sure you want that for all locales?

These are margins around the text when the whole text length has been computed.
This works for any text length and, I presume, therefore any language. I created
constants for them.

https://codereview.chromium.org/14587007/diff/9001/ash/accelerators/exit_warn...
ash/accelerators/exit_warning_handler.cc:56: base::string16 text_;
On 2013/05/13 15:40:32, sky wrote:
> Any reason you're not using a views::Label* to draw this?

Using label.
Done.

https://codereview.chromium.org/14587007/diff/9001/ash/accelerators/exit_warn...
ash/accelerators/exit_warning_handler.cc:61: };
On 2013/05/13 15:40:32, sky wrote:
> DISALLOW_...

Done.

https://codereview.chromium.org/14587007/diff/9001/ash/accelerators/exit_warn...
File ash/accelerators/exit_warning_handler.h (right):

https://codereview.chromium.org/14587007/diff/9001/ash/accelerators/exit_warn...
ash/accelerators/exit_warning_handler.h:5: #ifndef ASH_EXIT_WARNING_HANDLER_H_
On 2013/05/13 15:40:32, sky wrote:
> You missed the accelerators here, eg ASH_ACCELERATORS...

Done.

https://codereview.chromium.org/14587007/diff/9001/ash/accelerators/exit_warn...
ash/accelerators/exit_warning_handler.h:22: // for immediate exit. The press,
release and press must happend within
On 2013/05/13 15:40:32, sky wrote:
> happend -> happen

Done.

https://codereview.chromium.org/14587007/diff/9001/ash/accelerators/exit_warn...
ash/accelerators/exit_warning_handler.h:72: 
On 2013/05/13 15:40:32, sky wrote:
> nit: no newline here.

Done.

https://codereview.chromium.org/14587007/diff/9001/ash/accelerators/exit_warn...
ash/accelerators/exit_warning_handler.h:77: void HandleExitKey(bool press);
On 2013/05/13 15:40:32, sky wrote:
> Document what press is.

Done.

https://codereview.chromium.org/14587007/diff/9001/ash/accelerators/exit_warn...
ash/accelerators/exit_warning_handler.h:80: void Timer1Action();
On 2013/05/13 15:40:32, sky wrote:
> These need a description.

Done.

https://codereview.chromium.org/14587007/diff/9001/ash/accelerators/exit_warn...
ash/accelerators/exit_warning_handler.h:89: enum State {
On 2013/05/13 15:40:32, sky wrote:
> enums should be first in section.

Done.

https://codereview.chromium.org/14587007/diff/9001/ash/accelerators/exit_warn...
ash/accelerators/exit_warning_handler.h:99: views::Widget* widget_;
On 2013/05/13 15:40:32, sky wrote:
> Document ownership.

Done.

https://codereview.chromium.org/14587007/diff/9001/ash/accelerators/exit_warn...
ash/accelerators/exit_warning_handler.h:102: };
On 2013/05/13 15:40:32, sky wrote:
> DISALLOW_...

Done.

[sky, 2013-05-14 17:10:05]

https://codereview.chromium.org/14587007/diff/9001/ash/accelerators/exit_warn...
File ash/accelerators/exit_warning_handler.h (right):

https://codereview.chromium.org/14587007/diff/9001/ash/accelerators/exit_warn...
ash/accelerators/exit_warning_handler.h:77: void HandleExitKey(bool press);
On 2013/05/14 16:49:11, sschmitz wrote:
> On 2013/05/13 15:40:32, sky wrote:
> > Document what press is.
> 
> Done.

It still isn't clear that |press| is true on a press and false on release.

https://codereview.chromium.org/14587007/diff/34001/ash/accelerators/accelera...
File ash/accelerators/accelerator_controller_unittest.cc (right):

https://codereview.chromium.org/14587007/diff/34001/ash/accelerators/accelera...
ash/accelerators/accelerator_controller_unittest.cc:411:
ExitWarningHandlerTestDoublePress();
The typical pattern is to put the assertions in your test both, not bounce to a
method that has the assertions.

https://codereview.chromium.org/14587007/diff/34001/ash/accelerators/exit_war...
File ash/accelerators/exit_warning_handler.cc (right):

https://codereview.chromium.org/14587007/diff/34001/ash/accelerators/exit_war...
ash/accelerators/exit_warning_handler.cc:47: label->SetBounds(0, 0, width_,
height_);
Better to remove this and set a filllayoutmanager on this. That way the size
tracks the real width/bounds.

https://codereview.chromium.org/14587007/diff/34001/ash/accelerators/exit_war...
ash/accelerators/exit_warning_handler.cc:82: }
What happens if you get here and widget_ is non-null?

https://codereview.chromium.org/14587007/diff/34001/ash/accelerators/exit_war...
File ash/accelerators/exit_warning_handler.h (right):

https://codereview.chromium.org/14587007/diff/34001/ash/accelerators/exit_war...
ash/accelerators/exit_warning_handler.h:111: void StubTimersForTest() {
stub_timers_for_test_ = true; }
Is there a reason these needs to be exposed? Can't the test directly access the
fields?

https://codereview.chromium.org/14587007/diff/34001/ash/accelerators/exit_war...
ash/accelerators/exit_warning_handler.h:119: bool stub_timers_for_test_;
Document what this is.

https://codereview.chromium.org/14587007/diff/34001/ash/accelerators/exit_war...
ash/accelerators/exit_warning_handler.h:126: #endif
You need:
#endif  // ASH_ACCELERATORS_EXIT_WARNING_HANDLER_H_

[sschmitz, 2013-05-14 21:36:42]

https://codereview.chromium.org/14587007/diff/34001/ash/accelerators/accelera...
File ash/accelerators/accelerator_controller_unittest.cc (right):

https://codereview.chromium.org/14587007/diff/34001/ash/accelerators/accelera...
ash/accelerators/accelerator_controller_unittest.cc:411:
ExitWarningHandlerTestDoublePress();
On 2013/05/14 17:10:05, sky wrote:
> The typical pattern is to put the assertions in your test both, not bounce to
a
> method that has the assertions.

Done. Due to the need of friendship to ExitWarningHandler, the test base class
provides some utility functions for the needed access.

https://codereview.chromium.org/14587007/diff/34001/ash/accelerators/exit_war...
File ash/accelerators/exit_warning_handler.cc (right):

https://codereview.chromium.org/14587007/diff/34001/ash/accelerators/exit_war...
ash/accelerators/exit_warning_handler.cc:47: label->SetBounds(0, 0, width_,
height_);
On 2013/05/14 17:10:05, sky wrote:
> Better to remove this and set a filllayoutmanager on this. That way the size
> tracks the real width/bounds.

Done.

https://codereview.chromium.org/14587007/diff/34001/ash/accelerators/exit_war...
ash/accelerators/exit_warning_handler.cc:82: }
On 2013/05/14 17:10:05, sky wrote:
> What happens if you get here and widget_ is non-null?

Added Hide(). The timers get canceled automatically in their destructors.

https://codereview.chromium.org/14587007/diff/34001/ash/accelerators/exit_war...
File ash/accelerators/exit_warning_handler.h (right):

https://codereview.chromium.org/14587007/diff/34001/ash/accelerators/exit_war...
ash/accelerators/exit_warning_handler.h:111: void StubTimersForTest() {
stub_timers_for_test_ = true; }
On 2013/05/14 17:10:05, sky wrote:
> Is there a reason these needs to be exposed? Can't the test directly access
the
> fields?

Done.

https://codereview.chromium.org/14587007/diff/34001/ash/accelerators/exit_war...
ash/accelerators/exit_warning_handler.h:119: bool stub_timers_for_test_;
On 2013/05/14 17:10:05, sky wrote:
> Document what this is.

Done.

https://codereview.chromium.org/14587007/diff/34001/ash/accelerators/exit_war...
ash/accelerators/exit_warning_handler.h:126: #endif
On 2013/05/14 17:10:05, sky wrote:
> You need:
> #endif  // ASH_ACCELERATORS_EXIT_WARNING_HANDLER_H_

Done.

[sky, 2013-05-15 00:14:58]

LGTM

[commit-bot: I haz the power, 2013-05-15 00:23:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sschmitz@chromium.org/14587007/47001

[commit-bot: I haz the power, 2013-05-15 08:48:05]

Change committed as 200204