Request/Release wake lock when power state changed.

This is a follow-up CL of the original:
https://chromium-review.googlesource.com/c/chromium/src/+/2253363/

Upon shown, wake lock will only be requested when the battery is
charging, and also be release when the charger is disconnected later.

Bug: b/159382866
Test: covered by unittests.
Change-Id: I62d94d645a67c07c2310b6d1a7ee8220a24c77f0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2259773
Commit-Queue: Meilin Wang <meilinw@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#782060}

ash/ambient/ambient_controller.cc

@@ -22,6 +22,7 @@
 #include "ash/public/cpp/shell_window_ids.h"
 #include "ash/session/session_controller_impl.h"
 #include "ash/shell.h"
+#include "ash/system/power/power_status.h"
 #include "base/bind_helpers.h"
 #include "base/memory/weak_ptr.h"
 #include "base/time/time.h"
@@ -167,12 +168,23 @@ void AmbientController::OnAmbientUiVisibilityChanged(
       // This will be no-op if the view is already visible.
       container_view_->SetVisible(true);
 
-      AcquireWakeLock();
+      if (PowerStatus::Get()->IsBatteryCharging()) {
+        // Requires wake lock to prevent display from sleeping when the battery
+        // is charging.
+        AcquireWakeLock();
+      }
+      // Observes the |PowerStatus| on the battery charging status change for
+      // the current ambient session.
+      if (!power_status_observer_.IsObserving(PowerStatus::Get())) {
+        power_status_observer_.Add(PowerStatus::Get());
+      }
+
       StartRefreshingImages();
       break;
     case AmbientUiVisibility::kHidden:
       container_view_->GetWidget()->Hide();
 
+      // Has no effect if |wake_lock_| has already been released.
       ReleaseWakeLock();
       StopRefreshingImages();
 
@@ -241,6 +253,19 @@ void AmbientController::OnLockStateChanged(bool locked) {
   }
 }
 
+void AmbientController::OnPowerStatusChanged() {
+  if (ambient_ui_model_.ui_visibility() != AmbientUiVisibility::kShown) {
+    // No action needed if ambient screen is not shown.
+    return;
+  }
+
+  if (PowerStatus::Get()->IsBatteryCharging()) {
+    AcquireWakeLock();
+  } else {
+    ReleaseWakeLock();
+  }
+}
+
 void AmbientController::AddAmbientViewDelegateObserver(
     AmbientViewDelegateObserver* observer) {
   delegate_.AddObserver(observer);
@@ -323,7 +348,8 @@ void AmbientController::AcquireWakeLock() {
 }
 
 void AmbientController::ReleaseWakeLock() {
-  DCHECK(wake_lock_);
+  if (!wake_lock_)
+    return;
 
   wake_lock_->CancelWakeLock();
   VLOG(1) << "Released wake lock";
@@ -358,6 +384,8 @@ void AmbientController::CleanUpOnClosed() {
   // Invalidates the view pointer.
   container_view_ = nullptr;
   inactivity_monitor_.reset();
+  power_status_observer_.Remove(PowerStatus::Get());
+
   // Should do nothing if the wake lock has already been released.
   ReleaseWakeLock();
 }

ash/ambient/ambient_controller.h

@@ -15,8 +15,10 @@
 #include "ash/ash_export.h"
 #include "ash/public/cpp/ambient/ambient_ui_model.h"
 #include "ash/session/session_observer.h"
+#include "ash/system/power/power_status.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "base/scoped_observer.h"
 #include "mojo/public/cpp/bindings/remote.h"
 #include "services/device/public/mojom/wake_lock.mojom.h"
 #include "ui/views/widget/widget.h"
@@ -33,7 +35,8 @@ class AmbientViewDelegateObserver;
 
 // Class to handle all ambient mode functionalities.
 class ASH_EXPORT AmbientController : public AmbientUiModelObserver,
-                                     public SessionObserver {
+                                     public SessionObserver,
+                                     public PowerStatus::Observer {
  public:
   static void RegisterProfilePrefs(PrefRegistrySimple* registry);
 
@@ -46,6 +49,9 @@ class ASH_EXPORT AmbientController : public AmbientUiModelObserver,
   // SessionObserver:
   void OnLockStateChanged(bool locked) override;
 
+  // PowerStatus::Observer:
+  void OnPowerStatusChanged() override;
+
   void AddAmbientViewDelegateObserver(AmbientViewDelegateObserver* observer);
   void RemoveAmbientViewDelegateObserver(AmbientViewDelegateObserver* observer);
 
@@ -102,11 +108,11 @@ class ASH_EXPORT AmbientController : public AmbientUiModelObserver,
   void set_backend_controller_for_testing(
       std::unique_ptr<AmbientBackendController> photo_client);
 
-  // Creates (if not created) and acquires |wake_lock_|. Called when ambient
-  // screen starts to show.
+  // Creates (if not created) and acquires |wake_lock_|. Unbalanced call
+  // without subsequently |ReleaseWakeLock| will have no effect.
   void AcquireWakeLock();
 
-  // Release |wake_lock_|. Called when ambient screen is hidden/closed.
+  // Release |wake_lock_|. Unbalanced release call will have no effect.
   void ReleaseWakeLock();
 
   AmbientPhotoController* get_ambient_photo_controller_for_testing() {
@@ -133,6 +139,9 @@ class ASH_EXPORT AmbientController : public AmbientUiModelObserver,
   // Lazily initialized on the first call of |AcquireWakeLock|.
   mojo::Remote<device::mojom::WakeLock> wake_lock_;
 
+  ScopedObserver<PowerStatus, PowerStatus::Observer> power_status_observer_{
+      this};
+
   base::WeakPtrFactory<AmbientController> weak_ptr_factory_{this};
   DISALLOW_COPY_AND_ASSIGN(AmbientController);
 };

ash/ambient/ambient_controller_unittest.cc

@@ -11,9 +11,11 @@
 #include "ash/ambient/test/ambient_ash_test_base.h"
 #include "ash/ambient/ui/ambient_container_view.h"
 #include "ash/public/cpp/ambient/ambient_ui_model.h"
+#include "ash/system/power/power_status.h"
 #include "base/run_loop.h"
 #include "base/test/bind_test_util.h"
 #include "base/time/time.h"
+#include "chromeos/dbus/power_manager/power_supply_properties.pb.h"
 
 namespace ash {
 
@@ -121,24 +123,94 @@ TEST_F(AmbientControllerTest, ShouldRefreshAccessTokenAfterFailure) {
   EXPECT_TRUE(IsAccessTokenRequestPending());
 }
 
-TEST_F(AmbientControllerTest, CheckAcquireAndReleaseWakeLock) {
-  // Simulates screen lock to show ambient mode, will result in acquiring a wake
-  // lock.
+TEST_F(AmbientControllerTest,
+       CheckAcquireAndReleaseWakeLockWhenBatteryIsCharging) {
+  // Flush the loop first to ensure the |PowerStatus| has picked up the initial
+  // status.
+  base::RunLoop().RunUntilIdle();
+
+  // Simulate a device being connected to a charger initially.
+  power_manager::PowerSupplyProperties proto;
+  proto.set_battery_state(
+      power_manager::PowerSupplyProperties_BatteryState_CHARGING);
+  PowerStatus::Get()->SetProtoForTesting(proto);
+
+  // Lock screen to start ambient mode, and flush the loop to ensure
+  // the acquire wake lock request has reached the wake lock provider.
   LockScreen();
-  // Run loop to ensure the request has reached the wake lock provider.
   base::RunLoop().RunUntilIdle();
 
-  EXPECT_TRUE(ambient_controller()->IsShown());
   EXPECT_EQ(1, GetNumOfActiveWakeLocks(
                    device::mojom::WakeLockType::kPreventDisplaySleep));
 
-  // Simulates user logs in to close ambient mode, will result in releasing the
-  // wake lock.
+  HideAmbientScreen();
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(0, GetNumOfActiveWakeLocks(
+                   device::mojom::WakeLockType::kPreventDisplaySleep));
+
+  // TODO(meilinw): refactor |AmbientAshTestBase| to make this built-in.
+  // Simulate the ambient screen being shown again.
+  ambient_controller()->OnAmbientUiVisibilityChanged(
+      AmbientUiVisibility::kShown);
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(1, GetNumOfActiveWakeLocks(
+                   device::mojom::WakeLockType::kPreventDisplaySleep));
+
+  // Unlock screen to exit ambient mode.
   UnlockScreen();
-  // Run loop to ensure the request has reached the wake lock provider.
   base::RunLoop().RunUntilIdle();
 
-  EXPECT_FALSE(ambient_controller()->IsShown());
+  EXPECT_EQ(0, GetNumOfActiveWakeLocks(
+                   device::mojom::WakeLockType::kPreventDisplaySleep));
+}
+
+TEST_F(AmbientControllerTest,
+       CheckAcquireAndReleaseWakeLockWhenBatteryChargingStateChanged) {
+  // Flush the loop first to ensure the |PowerStatus| has picked up the initial
+  // status.
+  base::RunLoop().RunUntilIdle();
+
+  // Simulate a device being disconnected with a charger initially.
+  power_manager::PowerSupplyProperties proto;
+  proto.set_battery_state(
+      power_manager::PowerSupplyProperties_BatteryState_DISCHARGING);
+  PowerStatus::Get()->SetProtoForTesting(proto);
+  // Lock screen to start ambient mode.
+  LockScreen();
+
+  // Should not acquire wake lock when device is not charging.
+  EXPECT_EQ(0, GetNumOfActiveWakeLocks(
+                   device::mojom::WakeLockType::kPreventDisplaySleep));
+
+  // Connect the device with a charger.
+  proto.set_battery_state(
+      power_manager::PowerSupplyProperties_BatteryState_CHARGING);
+  PowerStatus::Get()->SetProtoForTesting(proto);
+  // Notify the controller about the power status change, and flush the loop to
+  // ensure the wake lock request has reached the wake lock provider.
+  ambient_controller()->OnPowerStatusChanged();
+  base::RunLoop().RunUntilIdle();
+
+  // Should acquire the wake lock when battery is charging.
+  EXPECT_EQ(1, GetNumOfActiveWakeLocks(
+                   device::mojom::WakeLockType::kPreventDisplaySleep));
+
+  // Disconnects the charger again.
+  proto.set_battery_state(
+      power_manager::PowerSupplyProperties_BatteryState_DISCHARGING);
+  PowerStatus::Get()->SetProtoForTesting(proto);
+  ambient_controller()->OnPowerStatusChanged();
+  base::RunLoop().RunUntilIdle();
+
+  // Should release the wake lock when battery is not charging.
+  EXPECT_TRUE(ambient_controller()->IsShown());
+  EXPECT_EQ(0, GetNumOfActiveWakeLocks(
+                   device::mojom::WakeLockType::kPreventDisplaySleep));
+
+  // An unbalanced release should do nothing.
+  UnlockScreen();
   EXPECT_EQ(0, GetNumOfActiveWakeLocks(
                    device::mojom::WakeLockType::kPreventDisplaySleep));
 }