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

ash/wm/power_button_controller.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ash/wm/power_button_controller.h"
 
 #include "ash/common/accelerators/accelerator_controller.h"
 #include "ash/common/ash_switches.h"
 #include "ash/common/session/session_state_delegate.h"
 #include "ash/common/system/audio/tray_audio.h"
@@ skipping 10 matching lines @@
 #include "ui/events/event_handler.h"
 #include "ui/wm/core/compound_event_filter.h"
 
 #if defined(OS_CHROMEOS)
 #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;

[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),
       volume_down_pressed_(false),
 #if defined(OS_CHROMEOS)
       volume_percent_before_screenshot_(0),
 #endif
       brightness_is_zero_(false),
       internal_display_off_and_external_display_on_(false),
       has_legacy_power_button_(
           base::CommandLine::ForCurrentProcess()->HasSwitch(
               switches::kAuraLegacyPowerButton)),
 #if defined(OS_CHROMEOS)
       enable_quick_lock_(base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kAshEnableTouchView)),
 #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() {
   Shell::GetInstance()->RemovePreTargetHandler(this);
 #if defined(OS_CHROMEOS)
   Shell::GetInstance()->display_configurator()->RemoveObserver(this);
   chromeos::DBusThreadManager::Get()->GetPowerManagerClient()->RemoveObserver(
       this);
 #endif
 }
 
 void PowerButtonController::OnScreenBrightnessChanged(double percent) {
   brightness_is_zero_ = percent <= 0.001;
 }
 
 void PowerButtonController::OnPowerButtonEvent(
     bool down,
     const base::TimeTicks& timestamp) {
   power_button_down_ = down;
 
   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);
 
     WmShell::Get()->accelerator_controller()->PerformActionIfEnabled(
         TAKE_SCREENSHOT);
 
 #if defined(OS_CHROMEOS)
     // Restore volume.
     chromeos::CrasAudioHandler* audio_handler =
@@ skipping 21 matching lines @@
       }
     }
   } else {  // !has_legacy_power_button_
     if (down) {
       // If we already have a pending request to lock the screen, wait.
       if (controller_->LockRequested())
         return;
 
       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);
       } else {
         controller_->StartShutdownAnimation();
       }
     } else {  // Button is up.
       if (controller_->CanCancelLockAnimation())
         controller_->CancelLockAnimation();
       else if (controller_->CanCancelShutdownAnimation())
@@ skipping 19 matching lines @@
   if (power_button_down_)
     return;
 
   if (down)
     controller_->StartLockAnimation(false);
   else
     controller_->CancelLockAnimation();
 }
 
 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)
     if (!event->is_repeat()) {
       chromeos::CrasAudioHandler* audio_handler =
           chromeos::CrasAudioHandler::Get();
       volume_percent_before_screenshot_ =
           audio_handler->GetOutputVolumePercent();
     }
 #endif
   }
 }
 
+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) {
   bool internal_display_off = false;
   bool external_display_on = false;
   for (const ui::DisplaySnapshot* display : display_states) {
     if (display->type() == ui::DISPLAY_CONNECTION_TYPE_INTERNAL) {
       if (!display->current_mode())
         internal_display_off = true;
     } else if (display->current_mode()) {
       external_display_on = true;
     }
   }
   internal_display_off_and_external_display_on_ =
       internal_display_off && external_display_on;
 }
 
 void PowerButtonController::PowerButtonEventReceived(
     bool down,
     const base::TimeTicks& timestamp) {
   OnPowerButtonEvent(down, timestamp);
 }
 #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