ambient: turn off ambient mode when screen is off

* Unified the logic around when to turn ambient mode off around
  screen off. This replaces the lid state, suspend state handlers.
* Fixed a crash during shutdown when image download is scheduled

Bug: b:162597832
Change-Id: Ie64e5464c37acda3e0fa638c9343e78c4dc0c17d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2333540Reviewed-by: Meilin Wang <meilinw@chromium.org>
Reviewed-by: Tao Wu <wutao@chromium.org>
Commit-Queue: Xiaohui Chen <xiaohuic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#795278}

ash/ambient/ambient_controller.cc

@@ -33,6 +33,7 @@
 #include "chromeos/assistant/buildflags.h"
 #include "chromeos/constants/chromeos_features.h"
 #include "chromeos/dbus/power/power_manager_client.h"
+#include "chromeos/dbus/power_manager/backlight.pb.h"
 #include "chromeos/dbus/power_manager/idle.pb.h"
 #include "chromeos/services/assistant/public/cpp/assistant_service.h"
 #include "components/prefs/pref_registry_simple.h"
@@ -91,26 +92,14 @@ bool IsChargerConnected() {
          PowerStatus::Get()->IsLinePowerConnected();
 }
 
-bool IsUiShown(AmbientUiVisibility visibility) {
-  return visibility == AmbientUiVisibility::kShown;
-}
-
 bool IsUiHidden(AmbientUiVisibility visibility) {
   return visibility == AmbientUiVisibility::kHidden;
 }
 
-bool IsUiClosed(AmbientUiVisibility visibility) {
-  return visibility == AmbientUiVisibility::kClosed;
-}
-
 bool IsLockScreenUi(AmbientUiMode mode) {
   return mode == AmbientUiMode::kLockScreenUi;
 }
 
-bool IsInSessionUi(AmbientUiMode mode) {
-  return mode == AmbientUiMode::kInSessionUi;
-}
-
 bool IsAmbientModeEnabled() {
   if (!AmbientClient::Get()->IsAmbientModeAllowed())
     return false;
@@ -186,9 +175,6 @@ AmbientController::AmbientController() {
   DCHECK(power_manager_client);
   power_manager_client_observer_.Add(power_manager_client);
   power_manager_client->RequestStatusUpdate();
-  power_manager_client->GetSwitchStates(
-      base::BindOnce(&AmbientController::OnReceiveSwitchStates,
-                     weak_ptr_factory_.GetWeakPtr()));
 
   ambient_backend_model_observer_.Add(
       ambient_photo_controller_.ambient_backend_model());
@@ -220,10 +206,7 @@ void AmbientController::OnAmbientUiVisibilityChanged(
       break;
     case AmbientUiVisibility::kHidden:
     case AmbientUiVisibility::kClosed:
-      if (container_view_) {
-        container_view_->GetWidget()->Close();
-        container_view_ = nullptr;
-      }
+      CloseWidget(/*immediately=*/false);
 
       // TODO(wutao): This will clear the image cache currently. It will not
       // work with `kHidden` if the token has expired and ambient mode is shown
@@ -242,7 +225,7 @@ void AmbientController::OnAmbientUiVisibilityChanged(
       if (visibility == AmbientUiVisibility::kHidden) {
         // Creates the monitor and starts the auto-show timer upon hidden.
         DCHECK(!inactivity_monitor_);
-        if (LockScreen::HasInstance() && autoshow_enabled_) {
+        if (LockScreen::HasInstance()) {
           inactivity_monitor_ = std::make_unique<InactivityMonitor>(
               LockScreen::Get()->widget()->GetWeakPtr(),
               base::BindOnce(&AmbientController::OnAutoShowTimeOut,
@@ -302,40 +285,6 @@ void AmbientController::OnLockStateChanged(bool locked) {
   }
 }
 
-void AmbientController::LidEventReceived(
-    chromeos::PowerManagerClient::LidState state,
-    const base::TimeTicks& timestamp) {
-  // We still need to observe the lid closed event despite of suspend signals
-  // to release the wake lock acquired if any.
-  bool lid_closed = (state != chromeos::PowerManagerClient::LidState::OPEN);
-  if (lid_closed)
-    ReleaseWakeLock();
-}
-
-void AmbientController::SuspendImminent(
-    power_manager::SuspendImminent::Reason reason) {
-  // This is triggered when the system is about to suspend and the display will
-  // be turned off, we should handle this event by dismissing ambient (if not
-  // yet) and cancel the auto-show timer.
-  if (IsUiClosed(ambient_ui_model_.ui_visibility())) {
-    // No action needed if ambient screen has closed.
-    return;
-  }
-
-  HandleOnSuspend();
-}
-
-void AmbientController::SuspendDone(const base::TimeDelta& sleep_duration) {
-  // This is triggered when the system resumes after an earlier suspension, we
-  // should handle this event by restarting the auto-show timer if necessary.
-  if (IsUiClosed(ambient_ui_model_.ui_visibility())) {
-    // No action needed if ambient screen has closed.
-    return;
-  }
-
-  HandleOnResume();
-}
-
 void AmbientController::OnPowerStatusChanged() {
   if (ambient_ui_model_.ui_visibility() != AmbientUiVisibility::kShown) {
     // No action needed if ambient screen is not shown.
@@ -349,6 +298,38 @@ void AmbientController::OnPowerStatusChanged() {
   }
 }
 
+void AmbientController::ScreenBrightnessChanged(
+    const power_manager::BacklightBrightnessChange& change) {
+  if (!change.has_percent())
+    return;
+
+  constexpr double kMinBrightness = 0.01;
+  if (change.percent() < kMinBrightness) {
+    if (is_screen_off_)
+      return;
+    is_screen_off_ = true;
+    // If screen is off, turn everything off. This covers:
+    //   1. Manually turn screen off.
+    //   2. Clicking tablet power button.
+    //   3. Close lid.
+    // Need to specially close the widget immediately here to be able to close
+    // the UI before device goes to suspend. Otherwise when opening lid after
+    // lid closed, there may be a flash of the old window before previous
+    // closing finished.
+    CloseWidget(/*immediately=*/true);
+    CloseUi();
+    return;
+  }
+
+  // change.percent() > kMinBrightness
+  if (!is_screen_off_)
+    return;
+  is_screen_off_ = false;
+  // If screen is back on, turn on ambient mode for lock screen.
+  if (LockScreen::HasInstance())
+    ShowUi(AmbientUiMode::kLockScreenUi);
+}
+
 void AmbientController::ScreenIdleStateChanged(
     const power_manager::ScreenIdleState& idle_state) {
   if (!IsAmbientModeEnabled())
@@ -360,8 +341,10 @@ void AmbientController::ScreenIdleStateChanged(
   auto* session_controller = Shell::Get()->session_controller();
   if (session_controller->CanLockScreen() &&
       session_controller->ShouldLockScreenAutomatically()) {
+    if (!session_controller->IsScreenLocked()) {
       // TODO(b/161469136): revise this behavior after further discussion.
       Shell::Get()->session_controller()->LockScreen();
+    }
   } else {
     ShowUi(AmbientUiMode::kInSessionUi);
   }
@@ -453,41 +436,16 @@ void AmbientController::ReleaseWakeLock() {
   VLOG(1) << "Released wake lock";
 }
 
-void AmbientController::OnReceiveSwitchStates(
-    base::Optional<chromeos::PowerManagerClient::SwitchStates> switch_states) {
-  autoshow_enabled_ = (switch_states->lid_state !=
-                       chromeos::PowerManagerClient::LidState::CLOSED);
-}
-
-void AmbientController::HandleOnSuspend() {
-  // Disables auto-show and reset the timer if created.
-  autoshow_enabled_ = false;
-  inactivity_monitor_.reset();
-
-  // Has no effect if no wake lock is acquired.
-  ReleaseWakeLock();
-
-  // Dismiss ambient screen.
-  if (IsLockScreenUi(ambient_ui_model_.ui_mode())) {
-    // No-op if UI has already been hidden.
-    HideLockScreenUi();
-  } else {
-    DCHECK(IsInSessionUi(ambient_ui_model_.ui_mode()));
-    CloseUi();
-  }
-}
+void AmbientController::CloseWidget(bool immediately) {
+  if (!container_view_)
+    return;
 
-void AmbientController::HandleOnResume() {
-  DCHECK(!IsUiShown(ambient_ui_model_.ui_visibility()));
+  if (immediately)
+    container_view_->GetWidget()->CloseNow();
+  else
+    container_view_->GetWidget()->Close();
 
-  // Enables auto-show and starts the timer.
-  autoshow_enabled_ = true;
-  if (!inactivity_monitor_ && LockScreen::HasInstance()) {
-    inactivity_monitor_ = std::make_unique<InactivityMonitor>(
-        LockScreen::Get()->widget()->GetWeakPtr(),
-        base::BindOnce(&AmbientController::OnAutoShowTimeOut,
-                       weak_ptr_factory_.GetWeakPtr()));
-  }
+  container_view_ = nullptr;
 }
 
 void AmbientController::RequestAccessToken(

ash/ambient/ambient_controller.h

@@ -61,12 +61,10 @@ class ASH_EXPORT AmbientController
   void OnPowerStatusChanged() override;
 
   // chromeos::PowerManagerClient::Observer:
-  void LidEventReceived(chromeos::PowerManagerClient::LidState state,
-                        const base::TimeTicks& timestamp) override;
-  void SuspendImminent(power_manager::SuspendImminent::Reason reason) override;
-  void SuspendDone(const base::TimeDelta& sleep_duration) override;
   void ScreenIdleStateChanged(
       const power_manager::ScreenIdleState& idle_state) override;
+  void ScreenBrightnessChanged(
+      const power_manager::BacklightBrightnessChange& change) override;
 
   void AddAmbientViewDelegateObserver(AmbientViewDelegateObserver* observer);
   void RemoveAmbientViewDelegateObserver(AmbientViewDelegateObserver* observer);
@@ -131,15 +129,7 @@ class ASH_EXPORT AmbientController
   // Release |wake_lock_|. Unbalanced release call will have no effect.
   void ReleaseWakeLock();
 
-  // Updates |autoshow_enabled_| flag based on the lid state.
-  void OnReceiveSwitchStates(
-      base::Optional<chromeos::PowerManagerClient::SwitchStates> switch_states);
-
-  // Invoked to dismiss ambient when the device is suspending.
-  void HandleOnSuspend();
-
-  // Invoked to restart the auto-show timer when the device is resuming.
-  void HandleOnResume();
+  void CloseWidget(bool immediately);
 
   AmbientPhotoController* get_ambient_photo_controller_for_testing() {
     return &ambient_photo_controller_;
@@ -177,9 +167,7 @@ class ASH_EXPORT AmbientController
                  chromeos::PowerManagerClient::Observer>
       power_manager_client_observer_{this};
 
-  // Whether ambient screen will be brought up from hidden after long device
-  // inactivity.
-  bool autoshow_enabled_ = false;
+  bool is_screen_off_ = false;
 
   base::WeakPtrFactory<AmbientController> weak_ptr_factory_{this};
   DISALLOW_COPY_AND_ASSIGN(AmbientController);

ash/ambient/ambient_controller_unittest.cc

@@ -301,47 +301,13 @@ TEST_F(AmbientControllerTest, ShouldDismissAndThenComesBack) {
   EXPECT_TRUE(container_view()->GetWidget()->IsVisible());
 }
 
-TEST_F(AmbientControllerTest, UpdateUiAndWakeLockWhenSystemSuspendOrResume) {
-  // Simulate a device being connected with a charger initially to acquire a
-  // wake lock upon shown.
-  power_manager::PowerSupplyProperties proto;
-  proto.set_battery_state(
-      power_manager::PowerSupplyProperties_BatteryState_CHARGING);
-  PowerStatus::Get()->SetProtoForTesting(proto);
-
-  // Starts ambient screen.
-  ShowAmbientScreen();
-  EXPECT_TRUE(ambient_controller()->IsShown());
-  EXPECT_EQ(1, GetNumOfActiveWakeLocks(
-                   device::mojom::WakeLockType::kPreventDisplaySleep));
-
-  // Simulates the system starting to suspend by lid closed.
-  SimulateSystemSuspendAndWait(
-      power_manager::SuspendImminent_Reason_LID_CLOSED);
-
-  // System suspension should hide Ui and release the wake lock acquired
-  // previously.
-  EXPECT_FALSE(container_view());
-  EXPECT_EQ(0, GetNumOfActiveWakeLocks(
-                   device::mojom::WakeLockType::kPreventDisplaySleep));
-
-  // Simulates the system starting to resume.
-  SimulateSystemResumeAndWait();
-
-  // System resume should not invoke Ui to show up, thus no wake lock needed
-  // to acquire.
-  EXPECT_FALSE(container_view());
-  EXPECT_EQ(0, GetNumOfActiveWakeLocks(
-                   device::mojom::WakeLockType::kPreventDisplaySleep));
-}
-
 TEST_F(AmbientControllerTest,
        ShouldShowAmbientScreenWithLockscreenWhenScreenIsDimmed) {
   GetSessionControllerClient()->SetShouldLockScreenAutomatically(true);
   EXPECT_FALSE(ambient_controller()->IsShown());
 
   // Should lock the device and enter ambient mode when the screen is dimmed.
-  SetScreenDimmedAndWait(true);
+  SetScreenIdleStateAndWait(/*dimmed=*/true, /*off=*/false);
   EXPECT_TRUE(IsLocked());
   EXPECT_FALSE(ambient_controller()->IsShown());
 
@@ -358,8 +324,9 @@ TEST_F(AmbientControllerTest, ShouldShowAmbientScreenWhenScreenIsDimmed) {
   GetSessionControllerClient()->SetShouldLockScreenAutomatically(false);
   EXPECT_FALSE(ambient_controller()->IsShown());
 
-  // Should lock the device and enter ambient mode when the screen is dimmed.
-  SetScreenDimmedAndWait(true);
+  // Should not lock the device but enter ambient mode when the screen is
+  // dimmed.
+  SetScreenIdleStateAndWait(/*dimmed=*/true, /*off=*/false);
   EXPECT_FALSE(IsLocked());
 
   FastForwardToNextImage();
@@ -369,4 +336,60 @@ TEST_F(AmbientControllerTest, ShouldShowAmbientScreenWhenScreenIsDimmed) {
   CloseAmbientScreen();
 }
 
+TEST_F(AmbientControllerTest, ShouldHideAmbientScreenWhenDisplayIsOff) {
+  GetSessionControllerClient()->SetShouldLockScreenAutomatically(false);
+  EXPECT_FALSE(ambient_controller()->IsShown());
+
+  // Should not lock the device and enter ambient mode when the screen is
+  // dimmed.
+  SetScreenBrightnessAndWait(/*percent=*/50);
+  SetScreenIdleStateAndWait(/*dimmed=*/true, /*off=*/false);
+  EXPECT_FALSE(IsLocked());
+
+  FastForwardToNextImage();
+  EXPECT_TRUE(ambient_controller()->IsShown());
+
+  // Should dismiss ambient mode screen.
+  SetScreenBrightnessAndWait(/*percent=*/0);
+  SetScreenIdleStateAndWait(/*dimmed=*/true, /*off=*/true);
+  EXPECT_FALSE(ambient_controller()->IsShown());
+
+  // Screen back on again, should not have ambient screen.
+  SetScreenBrightnessAndWait(/*percent=*/50);
+  SetScreenIdleStateAndWait(/*dimmed=*/false, /*off=*/false);
+  EXPECT_FALSE(ambient_controller()->IsShown());
+}
+
+TEST_F(AmbientControllerTest,
+       ShouldHideAmbientScreenWhenDisplayIsOffThenComesBackWithLockScreen) {
+  GetSessionControllerClient()->SetShouldLockScreenAutomatically(true);
+  EXPECT_FALSE(ambient_controller()->IsShown());
+
+  // Should not lock the device and enter ambient mode when the screen is
+  // dimmed.
+  SetScreenBrightnessAndWait(/*percent=*/50);
+  SetScreenIdleStateAndWait(/*dimmed=*/true, /*off=*/false);
+  EXPECT_TRUE(IsLocked());
+
+  FastForwardToInactivity();
+  FastForwardToNextImage();
+  EXPECT_TRUE(ambient_controller()->IsShown());
+
+  // Should dismiss ambient mode screen.
+  SetScreenBrightnessAndWait(/*percent=*/0);
+  SetScreenIdleStateAndWait(/*dimmed=*/true, /*off=*/true);
+  EXPECT_FALSE(ambient_controller()->IsShown());
+
+  // Screen back on again, should not have ambient screen, but still has lock
+  // screen.
+  SetScreenBrightnessAndWait(/*percent=*/50);
+  SetScreenIdleStateAndWait(/*dimmed=*/false, /*off=*/false);
+  EXPECT_TRUE(IsLocked());
+  EXPECT_FALSE(ambient_controller()->IsShown());
+
+  FastForwardToInactivity();
+  FastForwardToNextImage();
+  EXPECT_TRUE(ambient_controller()->IsShown());
+}
+
 }  // namespace ash

ash/ambient/ambient_photo_controller.cc

@@ -69,6 +69,10 @@ using DownloadCallback = base::OnceCallback<void(const gfx::ImageSkia&)>;
 void DownloadImageFromUrl(const std::string& url, DownloadCallback callback) {
   DCHECK(!url.empty());
 
+  // During shutdown, we may not have `ImageDownloader` when reach here.
+  if (!ImageDownloader::Get())
+    return;
+
   ImageDownloader::Get()->Download(GURL(url), NO_TRAFFIC_ANNOTATION_YET,
                                    base::BindOnce(std::move(callback)));
 }

ash/ambient/test/ambient_ash_test_base.cc

@@ -149,14 +149,24 @@ void AmbientAshTestBase::SimulateSystemResumeAndWait() {
   base::RunLoop().RunUntilIdle();
 }
 
-void AmbientAshTestBase::SetScreenDimmedAndWait(bool is_screen_dimmed) {
+void AmbientAshTestBase::SetScreenIdleStateAndWait(bool is_screen_dimmed,
+                                                   bool is_off) {
   power_manager::ScreenIdleState screen_idle_state;
   screen_idle_state.set_dimmed(is_screen_dimmed);
+  screen_idle_state.set_off(is_off);
   chromeos::FakePowerManagerClient::Get()->SendScreenIdleStateChanged(
       screen_idle_state);
   base::RunLoop().RunUntilIdle();
 }
 
+void AmbientAshTestBase::SetScreenBrightnessAndWait(double percent) {
+  power_manager::BacklightBrightnessChange change;
+  change.set_percent(percent);
+
+  chromeos::FakePowerManagerClient::Get()->SendScreenBrightnessChanged(change);
+  base::RunLoop().RunUntilIdle();
+}
+
 void AmbientAshTestBase::SetPhotoViewImageSize(int width, int height) {
   auto* image_decoder = static_cast<TestAmbientImageDecoderImpl*>(
       ambient_controller()

ash/ambient/test/ambient_ash_test_base.h

@@ -59,9 +59,12 @@ class AmbientAshTestBase : public AshTestBase {
   // Wait until the event has been processed.
   void SimulateSystemResumeAndWait();
 
-  // Simulates a screen dimmed event.
+  // Simulates a screen idle state event.
   // Wait until the event has been processed.
-  void SetScreenDimmedAndWait(bool is_screen_dimmed);
+  void SetScreenIdleStateAndWait(bool is_screen_dimmed, bool is_off);
+
+  // Simulates a screen brightness changed event.
+  void SetScreenBrightnessAndWait(double percent);
 
   // Set the size of the next image that will be loaded.
   void SetPhotoViewImageSize(int width, int height);