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.

This is a second CL on this topic (see crrev.com/200204). The first
had issues by relying on release events, which this CL does not.

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=200957

[sschmitz, 2013-05-17 00:41:31]

Scott, here the 2nd attempt... not relying on releases.

[sky, 2013-05-17 14:53:16]

https://codereview.chromium.org/14771027/diff/1/ash/accelerators/exit_warning...
File ash/accelerators/exit_warning_handler.cc (right):

https://codereview.chromium.org/14771027/diff/1/ash/accelerators/exit_warning...
ash/accelerators/exit_warning_handler.cc:89: void
ExitWarningHandler::Init(AcceleratorControllerContext* context) {
Why not pass the context into the constructor?

https://codereview.chromium.org/14771027/diff/1/ash/accelerators/exit_warning...
ash/accelerators/exit_warning_handler.cc:127: Hide();
Doesn't this mean you're effectively waiting longer than needed? Seems better to
make Hide() as soon as one of the keys that make up the accelerator is released.

https://codereview.chromium.org/14771027/diff/1/ash/accelerators/exit_warning...
File ash/accelerators/exit_warning_handler.h (right):

https://codereview.chromium.org/14771027/diff/1/ash/accelerators/exit_warning...
ash/accelerators/exit_warning_handler.h:115: AcceleratorControllerContext*
context_;
Documentation?

[sschmitz, 2013-05-17 15:53:31]

https://codereview.chromium.org/14771027/diff/1/ash/accelerators/exit_warning...
File ash/accelerators/exit_warning_handler.cc (right):

https://codereview.chromium.org/14771027/diff/1/ash/accelerators/exit_warning...
ash/accelerators/exit_warning_handler.cc:89: void
ExitWarningHandler::Init(AcceleratorControllerContext* context) {
On 2013/05/17 14:53:16, sky wrote:
> Why not pass the context into the constructor?

ExitWarningHandler and AcceleratorControllerContext are both members of
AcceleratorController. I avoided initializing one member with the address of
another in the constructor initialization list.

https://codereview.chromium.org/14771027/diff/1/ash/accelerators/exit_warning...
ash/accelerators/exit_warning_handler.cc:127: Hide();
On 2013/05/17 14:53:16, sky wrote:
> Doesn't this mean you're effectively waiting longer than needed? Seems better
to
> make Hide() as soon as one of the keys that make up the accelerator is
released.

No. This is done by design. PM did not want a short flash of the popup so the
user can't read it. The popup is meant to stay up for the duration (unless an
expert user performs a quick double press)

https://codereview.chromium.org/14771027/diff/1/ash/accelerators/exit_warning...
File ash/accelerators/exit_warning_handler.h (right):

https://codereview.chromium.org/14771027/diff/1/ash/accelerators/exit_warning...
ash/accelerators/exit_warning_handler.h:115: AcceleratorControllerContext*
context_;
On 2013/05/17 14:53:16, sky wrote:
> Documentation?

Done.

[sky, 2013-05-17 16:05:12]

On Fri, May 17, 2013 at 8:53 AM,  <sschmitz@chromium.org> wrote:
>
>
https://codereview.chromium.org/14771027/diff/1/ash/accelerators/exit_warning...
> File ash/accelerators/exit_warning_handler.cc (right):
>
>
https://codereview.chromium.org/14771027/diff/1/ash/accelerators/exit_warning...
> ash/accelerators/exit_warning_handler.cc:89: void
> ExitWarningHandler::Init(AcceleratorControllerContext* context) {
> On 2013/05/17 14:53:16, sky wrote:
>>
>> Why not pass the context into the constructor?
>
>
> ExitWarningHandler and AcceleratorControllerContext are both members of
> AcceleratorController. I avoided initializing one member with the
> address of another in the constructor initialization list.

Why? The order is known and makes the code easier to follow with less
null checks.

>
>
https://codereview.chromium.org/14771027/diff/1/ash/accelerators/exit_warning...
> ash/accelerators/exit_warning_handler.cc:127: Hide();
> On 2013/05/17 14:53:16, sky wrote:
>>
>> Doesn't this mean you're effectively waiting longer than needed? Seems
>
> better to
>>
>> make Hide() as soon as one of the keys that make up the accelerator is
>
> released.
>
> No. This is done by design. PM did not want a short flash of the popup
> so the user can't read it. The popup is meant to stay up for the
> duration (unless an expert user performs a quick double press)

Ok.

  -Scott

>
>
>
https://codereview.chromium.org/14771027/diff/1/ash/accelerators/exit_warning...
> File ash/accelerators/exit_warning_handler.h (right):
>
>
https://codereview.chromium.org/14771027/diff/1/ash/accelerators/exit_warning...
> ash/accelerators/exit_warning_handler.h:115:
> AcceleratorControllerContext* context_;
> On 2013/05/17 14:53:16, sky wrote:
>>
>> Documentation?
>
>
> Done.
>
> https://codereview.chromium.org/14771027/

[sschmitz, 2013-05-17 17:01:07]

Done: I removed Init and use the ctor to initialize the exit warning handler
with the context.

[sky, 2013-05-17 19:22:11]

LGTM

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

https://codereview.chromium.org/14771027/diff/13001/ash/accelerators/exit_war...
ash/accelerators/exit_warning_handler.h:117: ui::Accelerator accelerator_;
Document accelerator_ too.

[commit-bot: I haz the power, 2013-05-17 19:31:00]

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

[commit-bot: I haz the power, 2013-05-18 02:17:51]

Change committed as 200957