Unify and change logout/sleep/lock shortcuts

ash/accelerators/exit_warning_handler.cc

Index: ash/accelerators/exit_warning_handler.cc
diff --git a/ash/accelerators/exit_warning_handler.cc b/ash/accelerators/exit_warning_handler.cc
index a285560ee9cc85dabd2f255cd1ad05ad0d7eee68..684a53fc8e132d86ee0987c8b42c3d82de5e49e6 100644
--- a/ash/accelerators/exit_warning_handler.cc
+++ b/ash/accelerators/exit_warning_handler.cc
@@ -4,6 +4,7 @@
 
 #include "ash/accelerators/exit_warning_handler.h"
 
+#include "ash/accelerators/accelerator_controller.h"
 #include "ash/shell.h"
 #include "ash/shell_delegate.h"
 #include "ash/shell_window_ids.h"
@@ -26,7 +27,7 @@ namespace ash {
 namespace {
 
 const int64 kDoublePressTimeOutMilliseconds = 300;
-const int64 kHoldTimeOutMilliseconds = 1700;
+const int64 kHoldTimeOutMilliseconds = 1000;
 const SkColor kForegroundColor = 0xFFFFFFFF;
 const SkColor kBackgroundColor = 0xE0808080;
 const int kHorizontalMarginAroundText = 100;
@@ -74,9 +75,10 @@ class ExitWarningWidgetDelegateView : public views::WidgetDelegateView {
 } // namespace
 
 ExitWarningHandler::ExitWarningHandler()
-  : state_(IDLE),
-    widget_(NULL),
-    stub_timers_for_test_(false) {
+    : context_(NULL),
+      state_(IDLE),
+      widget_(NULL),
+      stub_timers_for_test_(false) {
 }
 
 ExitWarningHandler::~ExitWarningHandler() {
@@ -84,30 +86,28 @@ ExitWarningHandler::~ExitWarningHandler() {
   Hide();
 }
 
-void ExitWarningHandler::HandleExitKey(bool press) {
+void ExitWarningHandler::Init(AcceleratorControllerContext* context) {

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

Why not pass the context into the constructor?

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

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

+  context_ = context;
+}
+
+void ExitWarningHandler::HandleAccelerator() {
+  if (!context_)
+    return;
   switch (state_) {
     case IDLE:
-      if (press) {
-        state_ = WAIT_FOR_QUICK_RELEASE;
-        Show();
-        StartTimers();
-      }
-      break;
-    case WAIT_FOR_QUICK_RELEASE:
-      if (!press)
-        state_ = WAIT_FOR_DOUBLE_PRESS;
+      state_ = WAIT_FOR_DOUBLE_PRESS;
+      accelerator_ = context_->current_accelerator();
+      Show();
+      StartTimers();
       break;
     case WAIT_FOR_DOUBLE_PRESS:
-      if (press) {
-        state_ = EXITING;
-        CancelTimers();
-        Hide();
-        Shell::GetInstance()->delegate()->Exit();
-      }
+      state_ = EXITING;
+      CancelTimers();
+      Hide();
+      Shell::GetInstance()->delegate()->Exit();
       break;
     case WAIT_FOR_LONG_HOLD:
-      if (!press)
-        state_ = CANCELED;
+      state_ = CANCELED;
       break;
     case CANCELED:
     case EXITING:
@@ -119,21 +119,25 @@ void ExitWarningHandler::HandleExitKey(bool press) {
 }
 
 void ExitWarningHandler::Timer1Action() {
-  if (state_ == WAIT_FOR_QUICK_RELEASE)
+  if (state_ == WAIT_FOR_DOUBLE_PRESS)
     state_ = WAIT_FOR_LONG_HOLD;
-  else if (state_ == WAIT_FOR_DOUBLE_PRESS)
-    state_ = CANCELED;
 }
 
 void ExitWarningHandler::Timer2Action() {
+  Hide();

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

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.

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

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)

   if (state_ == CANCELED) {
     state_ = IDLE;
-    Hide();
-  }
-  else if (state_ == WAIT_FOR_LONG_HOLD) {
-    state_ = EXITING;
-    Hide();
-    Shell::GetInstance()->delegate()->Exit();
+  } else if (state_ == WAIT_FOR_LONG_HOLD) {
+    if (accelerator_ == context_->current_accelerator()) {
+      // We detect if the user has released any one of the keys that
+      // make up the shortcut by comparing "our" accelerator to the one
+      // from the current context and do not exit in that case.
+      state_ = EXITING;
+      Shell::GetInstance()->delegate()->Exit();
+    }
+    else {
+      state_ = IDLE;
+    }
   }
 }