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

ash/system/chromeos/power/tablet_power_button_controller_unittest.cc

+// Copyright 2016 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/system/chromeos/power/tablet_power_button_controller.h"
+
+#include <memory>
+
+#include "ash/common/ash_switches.h"
+#include "ash/common/session/session_state_delegate.h"
+#include "ash/common/wm/maximize_mode/maximize_mode_controller.h"
+#include "ash/common/wm_shell.h"
+#include "ash/shell.h"
+#include "ash/test/ash_test_base.h"
+#include "ash/test/lock_state_controller_test_api.h"
+#include "ash/wm/lock_state_controller.h"
+#include "base/command_line.h"
+#include "base/test/simple_test_tick_clock.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/fake_power_manager_client.h"
+#include "ui/events/test/event_generator.h"
+
+namespace ash {
+namespace test {
+
+namespace {
+
+// A non-zero brightness used for test.
+constexpr int kNonZeroBrightnessForTest = 10;

[Daniel Erat, 2016/11/11 05:40:50]

nit: drop the "ForTest" here?

[Qiang(Joe) Xu, 2016/11/11 18:19:30]

Done.

+
+void CheckBoolEquals(bool result, bool expected) {
+  EXPECT_EQ(result, expected);
+}
+
+}  // namespace
+
+class TabletPowerButtonControllerTest : public AshTestBase {
+ public:
+  TabletPowerButtonControllerTest() {}
+  ~TabletPowerButtonControllerTest() override {}
+
+  void SetUp() override {
+    // This also initializes DBusThreadManager.
+    std::unique_ptr<chromeos::DBusThreadManagerSetter> dbus_setter =
+        chromeos::DBusThreadManager::GetSetterForTesting();
+    power_manager_client_ = new chromeos::FakePowerManagerClient();
+    dbus_setter->SetPowerManagerClient(base::WrapUnique(power_manager_client_));
+    base::CommandLine::ForCurrentProcess()->AppendSwitch(
+        switches::kAshEnableTouchViewTesting);
+    AshTestBase::SetUp();
+
+    lock_state_controller_ = Shell::GetInstance()->lock_state_controller();
+    tablet_controller_ = Shell::GetInstance()->tablet_power_button_controller();
+    test_api_ = base::MakeUnique<TabletPowerButtonController::TestApi>(
+        tablet_controller_);
+    generator_ = &AshTestBase::GetEventGenerator();
+    power_manager_client_->SendBrightnessChanged(kNonZeroBrightnessForTest,
+                                                 false);
+    CheckBacklightsForcedOff(false);
+  }
+
+  void TearDown() override {
+    generator_ = nullptr;
+    AshTestBase::TearDown();
+    chromeos::DBusThreadManager::Shutdown();
+  }
+
+ protected:
+  void PressPowerButton() {
+    tablet_controller_->OnPowerButtonEvent(true, base::TimeTicks::Now());
+  }
+
+  void ReleasePowerButton() {
+    tablet_controller_->OnPowerButtonEvent(false, base::TimeTicks::Now());
+  }
+
+  void SystemUnlocks() {
+    lock_state_controller_->OnLockStateChanged(false);
+    WmShell::Get()->GetSessionStateDelegate()->UnlockScreen();
+  }
+
+  void Initialize(LoginStatus status) {
+    lock_state_controller_->OnLoginStateChanged(status);
+    SetUserLoggedIn(status != LoginStatus::NOT_LOGGED_IN);
+    lock_state_controller_->OnLockStateChanged(false);
+  }
+
+  void EnableMaximizeMode(bool enabled) {
+    WmShell::Get()->maximize_mode_controller()->EnableMaximizeModeWindowManager(
+        enabled);
+  }
+
+  void CheckLockedState(bool expected_locked) {
+    EXPECT_EQ(WmShell::Get()->GetSessionStateDelegate()->IsScreenLocked(),
+              expected_locked);
+  }
+
+  void CheckBacklightsForcedOff(bool expected_forced_off) {
+    power_manager_client_->GetBacklightsForcedOff(
+        base::Bind(&CheckBoolEquals, expected_forced_off));

[Daniel Erat, 2016/11/11 05:40:51]

does this callback ever run? i don't see the message loop getting run anywhere,
so i suspect it might not.

it'd probably be better to just do something like:

  void CopyResult(bool* dest, bool src) {
    *dest = src;
  }

  bool GetBacklightsForcedOff() WARN_UNUSED_RESULT {
    bool forced_off = false;
    power_manager_client_->GetBacklightsForcedOff(
        base::Bind(&CopyResult, base::Unretained(&forced_off)));
    base::RunLoop().RunUntilIdle();
    return forced_off;
  }

  EXPECT_TRUE(GetBacklightsForcedOff());

[Qiang(Joe) Xu, 2016/11/11 18:19:31]

Done.

+  }
+
+  // Ownership is passed on to chromeos::DBusThreadManager.
+  chromeos::FakePowerManagerClient* power_manager_client_;

[Daniel Erat, 2016/11/11 05:40:50]

nit: add a blank line after this line so it's clearer that the previous comment
only applies to this member

[Qiang(Joe) Xu, 2016/11/11 18:19:30]

Done.

+  LockStateController* lock_state_controller_;      // Not owned.
+  TabletPowerButtonController* tablet_controller_;  // Not owned.
+  std::unique_ptr<TabletPowerButtonController::TestApi> test_api_;
+  ui::test::EventGenerator* generator_ = nullptr;
+
+  DISALLOW_COPY_AND_ASSIGN(TabletPowerButtonControllerTest);
+};
+
+TEST_F(TabletPowerButtonControllerTest, LockScreenIfRequired) {
+  Initialize(LoginStatus::USER);
+  SetShouldLockScreenAutomatically(true);
+  CheckLockedState(false);
+
+  // On User logged in status, power-button-press-release should lock screen if
+  // should lock screen automatically.
+  PressPowerButton();
+  ReleasePowerButton();
+  CheckLockedState(true);
+
+  // On locked state, power-button-press-release should do nothing.
+  PressPowerButton();
+  ReleasePowerButton();
+  CheckLockedState(true);
+
+  // Unlock the sceen.
+  SystemUnlocks();
+  CheckLockedState(false);
+
+  // power-button-press-release should not lock the screen if should not lock
+  // screen automatically.
+  SetShouldLockScreenAutomatically(false);
+  PressPowerButton();
+  ReleasePowerButton();
+  CheckLockedState(false);
+}
+
+// This test is to test the power button released is done before shutdown
+// is timeout.

[Daniel Erat, 2016/11/11 05:40:50]

nit: "Tests that shutdown is cancelled if the power button is released quickly."

[Qiang(Joe) Xu, 2016/11/11 18:19:31]

I changed to "Tests that shutdown animation is not started if the power button
is released quickly".

+TEST_F(TabletPowerButtonControllerTest,
+       ReleasePowerButtonBeforeStartingShutdownTimer) {

[Daniel Erat, 2016/11/11 05:40:51]

this should be "BeforeStartingShutdownAnimation", right? the timer is already
started. (maybe it's referring to the animation timer, but please make the name
of this test consistent with the name of the next one.)

[Qiang(Joe) Xu, 2016/11/11 18:19:30]

yes, it should be "BeforeStartingShutdownAnimation"

+  PressPowerButton();
+  EXPECT_TRUE(test_api_->ShutdownTimerIsRunning());
+  CheckBacklightsForcedOff(false);
+  ReleasePowerButton();
+  power_manager_client_->SendBrightnessChanged(0, true);
+  EXPECT_FALSE(test_api_->ShutdownTimerIsRunning());
+  CheckBacklightsForcedOff(true);
+
+  PressPowerButton();
+  power_manager_client_->SendBrightnessChanged(kNonZeroBrightnessForTest, true);
+  EXPECT_TRUE(test_api_->ShutdownTimerIsRunning());
+  CheckBacklightsForcedOff(false);
+  ReleasePowerButton();
+  EXPECT_FALSE(test_api_->ShutdownTimerIsRunning());
+  CheckBacklightsForcedOff(false);
+}
+
+// This test is to test the power button released is done after shutdown timer
+// is timeout, where it will start shutdown animation timer.
+TEST_F(TabletPowerButtonControllerTest,
+       ReleasePowerButtonDuringShutdownAnimation) {
+  // Initializes LockStateControllerTestApi to observe shutdown animation timer.
+  std::unique_ptr<LockStateControllerTestApi> lock_state_test_api =
+      base::MakeUnique<LockStateControllerTestApi>(lock_state_controller_);
+
+  PressPowerButton();
+  test_api_->TriggerShutdownTimeout();
+  ReleasePowerButton();
+  CheckBacklightsForcedOff(false);
+
+  // Set backlights forced off to true for the next test.
+  PressPowerButton();

[Daniel Erat, 2016/11/11 05:40:50]

i don't understand what this part of the test is doing. isn't the system already
shutting down at this point due to the earlier TriggerShutdownTimeout call?

[Qiang(Joe) Xu, 2016/11/11 18:19:30]

I missed two lines in the earlier tests.

I think "TriggerShutdownTimeout()" is misleading. The earlier test is to test
cancelling/reverting shutdown animation when backlights is not forced off. The
later test is to test cancelling/reverting shutdown animation when backlights is
forced off. It is actually triggering the one second shutdown_timer_ timeout and
start shutdown animation timer. But the one second timer is called
|shutdown_timer_| in TabletPowerButtonController for short.

Do we really want |shutdown_timer_| for short description? It sounds like
causing misleading here.

+  ReleasePowerButton();
+  power_manager_client_->SendBrightnessChanged(0, true);
+  CheckBacklightsForcedOff(true);
+
+  PressPowerButton();
+  power_manager_client_->SendBrightnessChanged(kNonZeroBrightnessForTest, true);
+  CheckBacklightsForcedOff(false);
+  test_api_->TriggerShutdownTimeout();
+  EXPECT_TRUE(lock_state_test_api->shutdown_timer_is_running());
+  ReleasePowerButton();
+  EXPECT_FALSE(lock_state_test_api->shutdown_timer_is_running());
+  CheckBacklightsForcedOff(false);
+}
+
+// Test tapping power button when screen is idle off.
+TEST_F(TabletPowerButtonControllerTest, TappingPowerButtonWhenScreenIsIdleOff) {
+  power_manager_client_->SendBrightnessChanged(0, false);
+  PressPowerButton();
+  power_manager_client_->SendBrightnessChanged(kNonZeroBrightnessForTest, true);
+  CheckBacklightsForcedOff(false);
+  ReleasePowerButton();

[Daniel Erat, 2016/11/11 05:40:50]

you should also check that the backlights are still not forced off now, right?

[Qiang(Joe) Xu, 2016/11/11 18:19:31]

Done.

+}
+
+// Test tapping power button when device is suspended.
+TEST_F(TabletPowerButtonControllerTest, TappingPowerButtonWhenSuspended) {
+  base::SimpleTestTickClock* tick_clock = new base::SimpleTestTickClock;
+  // |tick_clock| owned by |tablet_controller_|.
+  tablet_controller_->SetTickClockForTesting(
+      std::unique_ptr<base::TickClock>(tick_clock));
+
+  power_manager_client_->SendSuspendImminent();
+  power_manager_client_->SendBrightnessChanged(0, false);
+  // There is a power button pressed here, but PowerButtonEvent is sent later.
+  power_manager_client_->SendSuspendDone();
+  power_manager_client_->SendBrightnessChanged(kNonZeroBrightnessForTest,
+                                               false);
+
+  // Test PowerButtonEvent is sent with a delay.
+  tick_clock->Advance(base::TimeDelta::FromMilliseconds(500));
+  power_manager_client_->SendPowerButtonEvent(true, tick_clock->NowTicks());
+  power_manager_client_->SendPowerButtonEvent(false, tick_clock->NowTicks());
+  CheckBacklightsForcedOff(false);
+}
+
+// For convertible device working on laptop mode, test keyboard/mouse event
+// when screen is off.
+TEST_F(TabletPowerButtonControllerTest, ConvertibleOnLaptopMode) {
+  EnableMaximizeMode(false);
+
+  // KeyEvent should SetBacklightsForcedOff(false).
+  PressPowerButton();
+  ReleasePowerButton();
+  power_manager_client_->SendBrightnessChanged(0, true);
+  CheckBacklightsForcedOff(true);
+  generator_->PressKey(ui::VKEY_L, ui::EF_NONE);
+  power_manager_client_->SendBrightnessChanged(kNonZeroBrightnessForTest, true);
+  CheckBacklightsForcedOff(false);
+
+  // Regular mouse event should SetBacklightsForcedOff(false).
+  PressPowerButton();
+  ReleasePowerButton();
+  power_manager_client_->SendBrightnessChanged(0, true);
+  CheckBacklightsForcedOff(true);
+  generator_->MoveMouseBy(1, 1);
+  power_manager_client_->SendBrightnessChanged(kNonZeroBrightnessForTest, true);
+  CheckBacklightsForcedOff(false);
+
+  // Touchpad mouse event should SetBacklightsForcedOff(false).
+  PressPowerButton();
+  ReleasePowerButton();
+  power_manager_client_->SendBrightnessChanged(0, true);
+  CheckBacklightsForcedOff(true);
+  generator_->PressTouch();
+  power_manager_client_->SendBrightnessChanged(kNonZeroBrightnessForTest, true);
+  CheckBacklightsForcedOff(false);
+
+  // Stylus mouse event should not SetBacklightsForcedOff(false).
+  PressPowerButton();
+  ReleasePowerButton();
+  power_manager_client_->SendBrightnessChanged(0, true);
+  CheckBacklightsForcedOff(true);
+  generator_->EnterPenPointerMode();
+  generator_->MoveMouseBy(1, 1);
+  CheckBacklightsForcedOff(true);
+  generator_->ExitPenPointerMode();
+}
+
+// For convertible device working on tablet mode, keyboard/mouse event should
+// not SetBacklightsForcedOff(false) when screen is off.
+TEST_F(TabletPowerButtonControllerTest, ConvertibleOnMaximizeMode) {
+  EnableMaximizeMode(true);
+
+  PressPowerButton();
+  ReleasePowerButton();
+  power_manager_client_->SendBrightnessChanged(0, true);
+  CheckBacklightsForcedOff(true);
+  generator_->PressKey(ui::VKEY_L, ui::EF_NONE);
+  CheckBacklightsForcedOff(true);
+
+  generator_->MoveMouseBy(1, 1);
+  CheckBacklightsForcedOff(true);
+
+  generator_->PressTouch();
+  CheckBacklightsForcedOff(true);
+
+  generator_->EnterPenPointerMode();
+  generator_->MoveMouseBy(1, 1);
+  CheckBacklightsForcedOff(true);
+  generator_->ExitPenPointerMode();
+}
+
+}  // namespace test
+}  // namespace ash