Tablet-like power button behavior on Convertible/Tablet ChromeOS devices

ash/wm/power_button_controller.cc

Index: ash/wm/power_button_controller.cc
diff --git a/ash/wm/power_button_controller.cc b/ash/wm/power_button_controller.cc
index 92ca9c12aabcd64eeb6996e10f6ab273b57e803c..e47880a967382f7132f7dae3d392fb5a2478284c 100644
--- a/ash/wm/power_button_controller.cc
+++ b/ash/wm/power_button_controller.cc
@@ -28,6 +28,19 @@
 
 namespace ash {
 
+namespace {
+// 1s timer before starting shutdown animation. If power button released event
+// is not received before timeout, |controller_| will take the duty to start
+// shutdown animation.
+const int kTabletPreStartShutdownAnimationTimeoutMs = 1000;

[Daniel Erat, 2016/11/04 16:12:52]

// Amount of time the power button must be held to start the
// pre-shutdown animation when in tablet mode.
constexpr int kTabletShutdownTimeoutMs = 1000;

[Qiang(Joe) Xu, 2016/11/04 19:29:51]

Done.

+

[Daniel Erat, 2016/11/04 16:12:52]

either remove blank line here or add a blank line after "namespace {" to match

[Qiang(Joe) Xu, 2016/11/04 19:29:51]

Done.

+}  // namespace
+
+PowerButtonController::TestApi::TestApi(PowerButtonController* controller)
+    : controller_(controller) {}
+
+PowerButtonController::TestApi::~TestApi() {}
+
 PowerButtonController::PowerButtonController(LockStateController* controller)
     : power_button_down_(false),
       lock_button_down_(false),
@@ -46,13 +59,17 @@ PowerButtonController::PowerButtonController(LockStateController* controller)
 #else
       enable_quick_lock_(false),
 #endif
-      controller_(controller) {
+      controller_(controller),
+      power_button_down_while_screen_off_(false),
+      backlights_forced_off_(false) {
 #if defined(OS_CHROMEOS)
   chromeos::DBusThreadManager::Get()->GetPowerManagerClient()->AddObserver(
       this);
   Shell::GetInstance()->display_configurator()->AddObserver(this);
 #endif
   Shell::GetInstance()->PrependPreTargetHandler(this);
+
+  GetInitialBacklightsForcedOff();
 }
 
 PowerButtonController::~PowerButtonController() {
@@ -76,16 +93,27 @@ void PowerButtonController::OnPowerButtonEvent(
   if (controller_->ShutdownRequested())
     return;
 
+  bool should_take_screenshot =
+      down && volume_down_pressed_ && IsTabletModeActive();
+
+  if (down) {
+    power_button_down_while_screen_off_ =
+        backlights_forced_off_ || brightness_is_zero_;

[Daniel Erat, 2016/11/04 16:12:52]

after updating tests, you can change this to just be:

  power_button_down_while_screen_off_ =
      down ? brightness_is_zero_ : false;

, right? (so you also clear the old state on button-up)

[Qiang(Joe) Xu, 2016/11/04 19:29:51]

We cannot clear the state on button-up here because it is needed in
OnTabletPowerButtonEvent. It will cause clearing the state before using.

if (down)
  power_button_down_while_screen_off_ = brightness_is_zero_;

And later clear it after it is used in if-else.

+  }
+
+  if (!has_legacy_power_button_ && !should_take_screenshot &&
+      IsTabletModeSupported()) {
+    OnTabletPowerButtonEvent(down, timestamp);
+    return;
+  }
+
   // Avoid starting the lock/shutdown sequence if the power button is pressed
   // while the screen is off (http://crbug.com/128451), unless an external
   // display is still on (http://crosbug.com/p/24912).
   if (brightness_is_zero_ && !internal_display_off_and_external_display_on_)
     return;
 
-  if (volume_down_pressed_ && down &&
-      WmShell::Get()
-          ->maximize_mode_controller()
-          ->IsMaximizeModeWindowManagerEnabled()) {
+  if (should_take_screenshot) {
     SystemTray* system_tray = Shell::GetInstance()->GetPrimarySystemTray();
     if (system_tray && system_tray->GetTrayAudio())
       system_tray->GetTrayAudio()->HideDetailedView(false);
@@ -127,10 +155,7 @@ void PowerButtonController::OnPowerButtonEvent(
 
       if (session_state_delegate->CanLockScreen() &&
           !session_state_delegate->IsUserSessionBlocked()) {
-        if (WmShell::Get()
-                ->maximize_mode_controller()
-                ->IsMaximizeModeWindowManagerEnabled() &&
-            enable_quick_lock_)
+        if (IsTabletModeActive() && enable_quick_lock_)

[Daniel Erat, 2016/11/04 16:12:52]

enable_quick_lock_ looks like it's true when --ash-enable-touch-view is set. is
that still relevant? i'm not sure that this code will ever be executed now --
won't we call OnTabletPowerButtonEvent() earlier when we see that
IsTabletModeSupported() return true?

[Qiang(Joe) Xu, 2016/11/04 19:29:51]

mm.. yes, do we still need quick lock here? I think power button pressed will
directly cause the screen off on tablet mode. The "quick lock" probably should
be deleted. I don't have much context with "quick lock", just thinking from the
name that it may be no longer needed. Tom is OOO today. I may check with him
later.

           controller_->StartLockAnimationAndLockImmediately(true);
         else
           controller_->StartLockAnimation(true);
@@ -170,6 +195,12 @@ void PowerButtonController::OnLockButtonEvent(
 }
 
 void PowerButtonController::OnKeyEvent(ui::KeyEvent* event) {
+  if (IsTabletModeSupported() && !IsTabletModeActive() &&

[Daniel Erat, 2016/11/04 16:12:52]

this can just be:

  if (!IsTabletModeActive() && backlights_forced_off_) {

, right? tablet mode can't be active if it isn't supported. :-)

[Qiang(Joe) Xu, 2016/11/04 19:29:51]

yes, done.

+      backlights_forced_off_) {
+    SetBacklightsForcedOff(false);
+    return;
+  }
+
   if (event->key_code() == ui::VKEY_VOLUME_DOWN) {
     volume_down_pressed_ = event->type() == ui::ET_KEY_PRESSED;
 #if defined(OS_CHROMEOS)
@@ -183,6 +214,13 @@ void PowerButtonController::OnKeyEvent(ui::KeyEvent* event) {
   }
 }
 
+void PowerButtonController::OnMouseEvent(ui::MouseEvent* event) {
+  if (IsTabletModeSupported() && !IsTabletModeActive() &&

[Daniel Erat, 2016/11/04 16:12:52]

same comment here

[Qiang(Joe) Xu, 2016/11/04 19:29:51]

Done.

+      backlights_forced_off_) {
+    SetBacklightsForcedOff(false);
+  }
+}
+
 #if defined(OS_CHROMEOS)
 void PowerButtonController::OnDisplayModeChanged(
     const ui::DisplayConfigurator::DisplayStateList& display_states) {
@@ -207,4 +245,98 @@ void PowerButtonController::PowerButtonEventReceived(
 }
 #endif  // defined(OS_CHROMEOS)
 
+void PowerButtonController::OnTabletPowerButtonEvent(
+    bool down,
+    const base::TimeTicks& timestamp) {
+  // When screen is off, SetBacklightsForcedOff(false) takes action on power
+  // button pressed.
+  // When screen is on, SetBacklightsForcedOff(true) takes action when power
+  // button is released before the |tablet_pre_start_shutdown_animation_timer_|
+  // is timeout.
+  if (down) {

[Daniel Erat, 2016/11/04 16:12:52]

i think that this could be changed to:

  if (down) {
    SetBacklightsForcedOff(false);
    StartTabletPowerButtonTimer();
  } else {

, since SetBacklightsForcedOff is a no-op if the state didn't change.

[Qiang(Joe) Xu, 2016/11/04 19:29:51]

Done.

+    if (backlights_forced_off_ || brightness_is_zero_)
+      SetBacklightsForcedOff(false);
+    StartTabletPreStartShutdownAnimationTimer();
+  } else {
+    if (power_button_down_while_screen_off_) {

[Daniel Erat, 2016/11/04 16:12:52]

seems simpler to move the timer check to the outer level:

  if (tablet_power_button_timer_.IsRunning()) {
    tablet_power_button_timer_.Stop();
    if (!power_button_down_while_screen_off_) {
      SetBacklightsForcedOff(true);
      LockScreenIfRequired();
    }
  }

[Qiang(Joe) Xu, 2016/11/04 19:29:51]

done, yes, it is clear.

+      if (tablet_pre_start_shutdown_animation_timer_.IsRunning())
+        tablet_pre_start_shutdown_animation_timer_.Stop();
+      power_button_down_while_screen_off_ = false;

[Daniel Erat, 2016/11/04 16:12:52]

i'd remove this line; it's better to only set the member from a single place
(i.e. OnPowerButtonEvent)

[Qiang(Joe) Xu, 2016/11/04 19:29:51]

See the comment OnPowerButtonEvent. If you don't like reset the status here, we
probably can create a Scoped Object that gets created on
power_button_down_while_screen_off_ and reset here.

+    } else {
+      if (tablet_pre_start_shutdown_animation_timer_.IsRunning()) {
+        tablet_pre_start_shutdown_animation_timer_.Stop();
+        SetBacklightsForcedOff(true);
+        LockScreenIfRequired();
+      }
+    }
+    // When power button is released, Cancel Shutdown animation whenever it is
+    // still cancellable.
+    if (controller_->CanCancelShutdownAnimation())
+      controller_->CancelShutdownAnimation();
+  }
+}
+
+void PowerButtonController::SetBacklightsForcedOff(bool forced_off) {
+  if (backlights_forced_off_ == forced_off)
+    return;
+
+#if defined(OS_CHROMEOS)
+  chromeos::DBusThreadManager::Get()
+      ->GetPowerManagerClient()
+      ->SetBacklightsForcedOff(forced_off);
+  backlights_forced_off_ = forced_off;
+#endif
+}
+
+void PowerButtonController::GetInitialBacklightsForcedOff() {
+#if defined(OS_CHROMEOS)
+  chromeos::DBusThreadManager::Get()
+      ->GetPowerManagerClient()
+      ->GetBacklightsForcedOff(
+          base::Bind(&PowerButtonController::HandleInitialBacklightsForcedOff,
+                     base::Unretained(this)));
+#endif
+}
+
+void PowerButtonController::HandleInitialBacklightsForcedOff(
+    bool is_forced_off) {
+  backlights_forced_off_ = is_forced_off;
+}
+
+void PowerButtonController::StartTabletPreStartShutdownAnimationTimer() {
+  tablet_pre_start_shutdown_animation_timer_.Start(
+      FROM_HERE, base::TimeDelta::FromMilliseconds(
+                     kTabletPreStartShutdownAnimationTimeoutMs),
+      this, &PowerButtonController::OnTabletPreStartShutdownAnimationTimeout);
+}
+
+void PowerButtonController::OnTabletPreStartShutdownAnimationTimeout() {
+  controller_->StartShutdownAnimation();
+}
+
+void PowerButtonController::LockScreenIfRequired() {
+  SessionStateDelegate* session_state_delegate =
+      WmShell::Get()->GetSessionStateDelegate();
+  if (session_state_delegate->ShouldLockScreenAutomatically() &&
+      session_state_delegate->CanLockScreen() &&
+      !session_state_delegate->IsUserSessionBlocked() &&
+      !controller_->LockRequested()) {
+    session_state_delegate->LockScreen();
+  }
+}
+
+bool PowerButtonController::IsTabletModeSupported() const {
+  MaximizeModeController* maximize_mode_controller =
+      WmShell::Get()->maximize_mode_controller();
+  return maximize_mode_controller &&
+         maximize_mode_controller->CanEnterMaximizeMode();
+}
+
+bool PowerButtonController::IsTabletModeActive() const {
+  MaximizeModeController* maximize_mode_controller =
+      WmShell::Get()->maximize_mode_controller();
+  return maximize_mode_controller &&
+         maximize_mode_controller->IsMaximizeModeWindowManagerEnabled();
+}
+
 }  // namespace ash