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..ebdd5fa5255153c32e842b0b4d1885d093c19f9d 100644
--- a/ash/wm/power_button_controller.cc
+++ b/ash/wm/power_button_controller.cc
@@ -22,12 +22,25 @@
 #include "ui/wm/core/compound_event_filter.h"
 
 #if defined(OS_CHROMEOS)
+#include "ash/wm/tablet_power_state_delegate_chromeos.h"
 #include "chromeos/audio/cras_audio_handler.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #endif
 
 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;
+}  // namespace
+
+PowerButtonController::TestApi::TestApi(PowerButtonController* controller)
+    : controller_(controller) {}
+
+PowerButtonController::TestApi::~TestApi() {}
+
 PowerButtonController::PowerButtonController(LockStateController* controller)
     : power_button_down_(false),
       lock_button_down_(false),
@@ -46,16 +59,30 @@ PowerButtonController::PowerButtonController(LockStateController* controller)
 #else
       enable_quick_lock_(false),
 #endif
-      controller_(controller) {
+      controller_(controller),
+      pressed_on_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);
+#if defined(OS_CHROMEOS)
+  tablet_power_state_delegate_ =
+      base::MakeUnique<TabletPowerStateDelegateChromeos>();

[Daniel Erat, 2016/11/03 21:28:08]

i don't understand the reason for this delegate.

delegates are usually used to do dependency injection -- for example, if ash/
weren't able to depend on chromeos/, then ash would define an abstract
DelegateInterface, and then chrome/ (which can depend on both ash/ and
chromeos/) could define an implementation of DelegateInterface that calls into
chromeos/ and then instantiate it and pass it into
ash::PowerButtonController.[1]

you don't have that dependency problem here, though, and the delegate
implementation actually lives inside of ash/ and is directly instantiated by
this class.

i think that you would have less code, and simpler code, if you got rid of the
delegate and just made PowerButtonController call the PowerManagerClient methods
directly.

1. note that we're moving away from this delegate-implemented-by-chrome pattern
due to mus+ash, though.

[Qiang(Joe) Xu, 2016/11/04 01:14:03]

done, thanks. I was thinking using |delegate_| initialized or not to check
OS_CHROMEOS env. But it looks like I am not in the right way. I agree with
removing it. Thanks for explaining!

+#endif
+
+  base::ThreadTaskRunnerHandle::Get()->PostTask(

[Daniel Erat, 2016/11/03 21:28:08]

what's the reason for posting this instead of just calling
GetBacklightsForcedOff directly within the c'tor? note that the method usually
makes an asynchronous method call to powerd. if the stub implementation runs the
callback synchronously instead, please feel free to update it to be async so it
matches the real implementation.

[Qiang(Joe) Xu, 2016/11/04 01:14:04]

I think just GetInitialBacklightsForcedOff should be fine. It is called from
PowerButtonController and dbus manager should be ready. Thanks!

+      FROM_HERE,
+      base::Bind(&PowerButtonController::GetInitialBacklightsForcedOff,
+                 base::Unretained(this)));
 }
 
 PowerButtonController::~PowerButtonController() {
+#if defined(OS_CHROMEOS)
+  tablet_power_state_delegate_.reset();
+#endif
   Shell::GetInstance()->RemovePreTargetHandler(this);
 #if defined(OS_CHROMEOS)
   Shell::GetInstance()->display_configurator()->RemoveObserver(this);
@@ -76,16 +103,19 @@ void PowerButtonController::OnPowerButtonEvent(
   if (controller_->ShutdownRequested())
     return;
 
+  if (!has_legacy_power_button_ && !MaybeTakeScreenshot(down) &&

[Daniel Erat, 2016/11/03 21:28:07]

instead of calling this helper method twice, why not just remove it and inline
it above this?

bool should_take_screenshot =
    down && volume_down_pressed_ &&
    WmShell::Get()
        ->maximize_mode_controller()
        ->IsMaximizeModeWindowManagerEnabled();

[Qiang(Joe) Xu, 2016/11/04 01:14:04]

Done.

+      WmShell::Get()->maximize_mode_controller()->CanEnterMaximizeMode()) {
+    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 (MaybeTakeScreenshot(down)) {
     SystemTray* system_tray = Shell::GetInstance()->GetPrimarySystemTray();
     if (system_tray && system_tray->GetTrayAudio())
       system_tray->GetTrayAudio()->HideDetailedView(false);
@@ -170,6 +200,7 @@ void PowerButtonController::OnLockButtonEvent(
 }
 
 void PowerButtonController::OnKeyEvent(ui::KeyEvent* event) {
+  WakeOnLaptopMode();
   if (event->key_code() == ui::VKEY_VOLUME_DOWN) {
     volume_down_pressed_ = event->type() == ui::ET_KEY_PRESSED;
 #if defined(OS_CHROMEOS)
@@ -183,6 +214,10 @@ void PowerButtonController::OnKeyEvent(ui::KeyEvent* event) {
   }
 }
 
+void PowerButtonController::OnMouseEvent(ui::MouseEvent* event) {
+  WakeOnLaptopMode();
+}
+
 #if defined(OS_CHROMEOS)
 void PowerButtonController::OnDisplayModeChanged(
     const ui::DisplayConfigurator::DisplayStateList& display_states) {
@@ -207,4 +242,105 @@ void PowerButtonController::PowerButtonEventReceived(
 }
 #endif  // defined(OS_CHROMEOS)
 
+void PowerButtonController::OnTabletPowerButtonEvent(
+    bool down,
+    const base::TimeTicks& timestamp) {
+  if (!tablet_power_state_delegate_)
+    return;
+
+  // 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) {
+    if (backlights_forced_off_ || brightness_is_zero_) {
+      pressed_on_screen_off_ = true;

[Daniel Erat, 2016/11/03 21:28:07]

it seems a bit safer to set this unconditionally here instead of only setting it
to true:

  power_button_down_while_screen_off_ = brightness_is_zero_;
  if (backlights_forced_off_)
    SetBacklightsForcedOff(false);

it would also be better to set this from OnPowerButtonEvent instead of
OnTabletPowerButtonEvent so it'll also have the correct state when we aren't in
tablet mode.

(if backlights_forced_off_ is true, won't brightness_is_zero_ also be true?)

[Qiang(Joe) Xu, 2016/11/04 01:14:03]

done. If backlights_forced_off_is true, brightness_is_zero_ should be true.
However, in test, that is not true.

So there are two options:
(1) leave backlights_forced_off_ || brightness_is_zero_ here
(2) leave brightness_is_zero_ here, and do some work in tests so that when
SetBacklightsForcedOff is called, BrightnessChanged can be called between zero
and non-zero.

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

i think that 2 is much better, since it ensures that the tests are simulating
the real behavior instead of testing something that should never happen. :-)

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

Done.

+      SetBacklightsForcedOff(false);
+    }
+    StartTabletPreStartShutdownAnimationTimer();
+  } else {
+    if (pressed_on_screen_off_) {
+      if (tablet_pre_start_shutdown_animation_timer_.IsRunning())
+        tablet_pre_start_shutdown_animation_timer_.Stop();
+      pressed_on_screen_off_ = false;
+    } 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 (!tablet_power_state_delegate_ || backlights_forced_off_ == forced_off)
+    return;
+
+  tablet_power_state_delegate_->SetBacklightsForcedOff(forced_off);
+  backlights_forced_off_ = forced_off;
+}
+
+void PowerButtonController::GetInitialBacklightsForcedOff() {
+  if (!tablet_power_state_delegate_)
+    return;
+
+  tablet_power_state_delegate_->GetBacklightsForcedOff(
+      base::Bind(&PowerButtonController::HandleInitialBacklightsForcedOff,
+                 base::Unretained(this)));
+}
+
+void PowerButtonController::HandleInitialBacklightsForcedOff(
+    bool is_forced_off) {
+  backlights_forced_off_ = is_forced_off;
+  if (is_forced_off)
+    SetBacklightsForcedOff(false);

[Daniel Erat, 2016/11/03 21:28:07]

i don't understand why this call is needed. the backlights are already forced
off in this case.

[Qiang(Joe) Xu, 2016/11/04 01:14:03]

I thought when constructed, user should face non-forced-off state. But I agree
they are not needed. We should follow the actual initialization state.

+}
+
+void PowerButtonController::StartTabletPreStartShutdownAnimationTimer() {
+  tablet_pre_start_shutdown_animation_timer_.Stop();

[Daniel Erat, 2016/11/03 21:28:08]

you don't need this call to Stop(); Start() already restarts the timer if it's
already running.

[Qiang(Joe) Xu, 2016/11/04 01:14:03]

Done.

+  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();
+  }
+}
+
+void PowerButtonController::WakeOnLaptopMode() {

[Daniel Erat, 2016/11/03 21:28:07]

using the word "wake" here seems a bit inaccurate; it usually refers to
returning from the S3 (suspended) power state. if this method becomes shorter
(see below), i'd just inline it within the places that call it.

[Qiang(Joe) Xu, 2016/11/04 01:14:03]

Done.

+  MaximizeModeController* maximize_mode_controller =
+      WmShell::Get()->maximize_mode_controller();
+  if (maximize_mode_controller &&
+      maximize_mode_controller->CanEnterMaximizeMode() &&
+      !maximize_mode_controller->IsMaximizeModeWindowManagerEnabled() &&

[Daniel Erat, 2016/11/03 21:28:07]

i find all of these references to MaximizeModeController a bit confusing. how
about extracting them to two private helper methods?

  bool IsTabletModeActive() const;
  bool IsTabletModeSupported() const;

i think that'll make the rest of the code much clearer. this would turn into
IsTabletModeSupported() && !IsTabletModeActive(), for example.

[Qiang(Joe) Xu, 2016/11/04 01:14:03]

Done.

+      (backlights_forced_off_ || brightness_is_zero_)) {

[Daniel Erat, 2016/11/03 21:28:07]

i don't understand the brightness_is_zero_ check here. if the backlights aren't
forced off but the brightness is zero (e.g. because the user reduced it to 0%),
why are you calling this? calling SetBacklightsForcedOff(false) won't do
anything if they already aren't forced off.

[Qiang(Joe) Xu, 2016/11/04 01:14:03]

Yes, brightness_is_zero_ is not needed. Thanks!

+    SetBacklightsForcedOff(false);
+  }
+}
+
+bool PowerButtonController::MaybeTakeScreenshot(bool power_button_pressed) {
+  return volume_down_pressed_ && power_button_pressed &&
+         WmShell::Get()
+             ->maximize_mode_controller()
+             ->IsMaximizeModeWindowManagerEnabled();
+}
+
 }  // namespace ash