ambient: handle suspend case better

Previously ambient mode will trigger lockscreen on screen dimm and then
rely on lock screen idle to show screen saver. This works in some cases,
but in suspend case the time between screen dim and cpu suspend is too
short. Screen saver was not able to enguage. The reason we have the set
up is because the screen saver was sharing the same window constainer as
the lock screen, it cannot show before lock screen is ready.

Now screen saver has its own window container, we changed the flow to
show ambient mode immediately when idle. This will prevent cpu suspend
because screen saver will take a wake lock if charging. Screen saver
will still lock the screen in the back if user reference indicated
lockscreen after wake.

Bug: b:169442907
Test: unitests and manual tests
Change-Id: I1a5df2c58c976bba492c709e0a61dd5a6314d084
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2435334Reviewed-by: Tao Wu <wutao@chromium.org>
Commit-Queue: Xiaohui Chen <xiaohuic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812397}

ash/ambient/ambient_constants.h

@@ -28,6 +28,9 @@ constexpr base::TimeDelta kPhotoRefreshInterval =
 constexpr base::TimeDelta kWeatherRefreshInterval =
     base::TimeDelta::FromMinutes(5);
 
+// The delay between ambient mode starts and enabling lock screen.
+constexpr base::TimeDelta kLockScreenDelay = base::TimeDelta::FromSeconds(5);
+
 // The batch size of topics to fetch in one request.
 // Magic number 2 is based on experiments that no curation on Google Photos.
 constexpr int kTopicsBatchSize = 2;

ash/ambient/ambient_controller.cc

@@ -96,10 +96,6 @@ bool IsUiHidden(AmbientUiVisibility visibility) {
   return visibility == AmbientUiVisibility::kHidden;
 }
 
-bool IsLockScreenUi(AmbientUiMode mode) {
-  return mode == AmbientUiMode::kLockScreenUi;
-}
-
 bool IsAmbientModeEnabled() {
   if (!AmbientClient::Get()->IsAmbientModeAllowed())
     return false;
@@ -204,7 +200,8 @@ void AmbientController::OnAmbientUiVisibilityChanged(
 
       // Record metrics on ambient mode usage.
       ambient::RecordAmbientModeActivation(
-          /*ui_mode=*/ambient_ui_model_.ui_mode(),
+          /*ui_mode=*/LockScreen::HasInstance() ? AmbientUiMode::kLockScreenUi
+                                                : AmbientUiMode::kInSessionUi,
           /*tablet_mode=*/Shell::Get()->IsInTabletMode());
 
       DCHECK(!start_time_);
@@ -319,7 +316,11 @@ void AmbientController::OnLockStateChanged(bool locked) {
     //        wait.
     RequestAccessToken(base::DoNothing(), /*may_refresh_token_on_lock=*/true);
 
-    ShowUi(AmbientUiMode::kLockScreenUi);
+    if (!IsShown()) {
+      // When lock screen starts, we don't immediately show the UI. The Ui is
+      // hidden and will show after a delay.
+      ShowHiddenUi();
+    }
   } else {
     // Ambient screen will be destroyed along with the lock screen when user
     // logs in.
@@ -374,7 +375,7 @@ void AmbientController::ScreenBrightnessChanged(
   is_screen_off_ = false;
   // If screen is back on, turn on ambient mode for lock screen.
   if (LockScreen::HasInstance())
-    ShowUi(AmbientUiMode::kLockScreenUi);
+    ShowHiddenUi();
 }
 
 void AmbientController::ScreenIdleStateChanged(
@@ -392,16 +393,7 @@ void AmbientController::ScreenIdleStateChanged(
   if (!idle_state.dimmed())
     return;
 
-  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);
-  }
+  ShowUi();
 }
 
 void AmbientController::AddAmbientViewDelegateObserver(
@@ -414,8 +406,8 @@ void AmbientController::RemoveAmbientViewDelegateObserver(
   delegate_.RemoveObserver(observer);
 }
 
-void AmbientController::ShowUi(AmbientUiMode mode) {
-  DVLOG(1) << "ShowUi: " << mode;
+void AmbientController::ShowUi() {
+  DVLOG(1) << __func__;
 
   // TODO(meilinw): move the eligibility check to the idle entry point once
   // implemented: b/149246117.
@@ -424,30 +416,24 @@ void AmbientController::ShowUi(AmbientUiMode mode) {
     return;
   }
 
-  ambient_ui_model_.SetUiMode(mode);
-  switch (mode) {
-    case AmbientUiMode::kInSessionUi:
-      ambient_ui_model_.SetUiVisibility(AmbientUiVisibility::kShown);
-      break;
-    case AmbientUiMode::kLockScreenUi:
-      ambient_ui_model_.SetUiVisibility(AmbientUiVisibility::kHidden);
-      break;
-  }
+  ambient_ui_model_.SetUiVisibility(AmbientUiVisibility::kShown);
 }
 
-void AmbientController::CloseUi() {
-  ambient_ui_model_.SetUiVisibility(AmbientUiVisibility::kClosed);
+void AmbientController::ShowHiddenUi() {
+  DVLOG(1) << __func__;
+
+  ambient_ui_model_.SetUiVisibility(AmbientUiVisibility::kHidden);
 }
 
-void AmbientController::HideLockScreenUi() {
-  DCHECK(IsLockScreenUi(ambient_ui_model_.ui_mode()));
+void AmbientController::CloseUi() {
+  DVLOG(1) << __func__;
 
-  ambient_ui_model_.SetUiVisibility(AmbientUiVisibility::kHidden);
+  ambient_ui_model_.SetUiVisibility(AmbientUiVisibility::kClosed);
 }
 
 void AmbientController::ToggleInSessionUi() {
-  if (!container_view_)
-    ShowUi(AmbientUiMode::kInSessionUi);
+  if (ambient_ui_model_.ui_visibility() == AmbientUiVisibility::kClosed)
+    ShowUi();
   else
     CloseUi();
 }
@@ -458,16 +444,12 @@ bool AmbientController::IsShown() const {
 
 void AmbientController::OnBackgroundPhotoEvents() {
   // Dismisses the ambient screen when user interacts with the background photo.
-  if (IsLockScreenUi(ambient_ui_model_.ui_mode()))
-    HideLockScreenUi();
+  if (LockScreen::HasInstance())
+    ShowHiddenUi();
   else
     CloseUi();
 }
 
-void AmbientController::UpdateUiMode(AmbientUiMode ui_mode) {
-  ambient_ui_model_.SetUiMode(ui_mode);
-}
-
 void AmbientController::AcquireWakeLock() {
   if (!wake_lock_) {
     mojo::Remote<device::mojom::WakeLockProvider> provider;
@@ -482,6 +464,17 @@ void AmbientController::AcquireWakeLock() {
   DCHECK(wake_lock_);
   wake_lock_->RequestWakeLock();
   VLOG(1) << "Acquired wake lock";
+
+  auto* session_controller = Shell::Get()->session_controller();
+  if (session_controller->CanLockScreen() &&
+      session_controller->ShouldLockScreenAutomatically()) {
+    if (!session_controller->IsScreenLocked()) {
+      delayed_lock_timer_.Start(
+          FROM_HERE, kLockScreenDelay, base::BindOnce([]() {
+            Shell::Get()->session_controller()->LockScreen();
+          }));
+    }
+  }
 }
 
 void AmbientController::ReleaseWakeLock() {
@@ -490,6 +483,8 @@ void AmbientController::ReleaseWakeLock() {
 
   wake_lock_->CancelWakeLock();
   VLOG(1) << "Released wake lock";
+
+  delayed_lock_timer_.Stop();
 }
 
 void AmbientController::CloseWidget(bool immediately) {

ash/ambient/ambient_controller.h

@@ -20,6 +20,7 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/scoped_observer.h"
+#include "base/timer/timer.h"
 #include "chromeos/dbus/power/power_manager_client.h"
 #include "components/prefs/pref_change_registrar.h"
 #include "components/prefs/pref_service.h"
@@ -74,12 +75,12 @@ class ASH_EXPORT AmbientController
   void AddAmbientViewDelegateObserver(AmbientViewDelegateObserver* observer);
   void RemoveAmbientViewDelegateObserver(AmbientViewDelegateObserver* observer);
 
-  // Invoked to show/close ambient UI in |mode|.
-  void ShowUi(AmbientUiMode mode);
+  void ShowUi();
+  // Ui will be enabled but not shown immediately. If there is no user activity
+  // Ui will be shown after a short delay.
+  void ShowHiddenUi();
   void CloseUi();
 
-  void HideLockScreenUi();
-
   void ToggleInSessionUi();
 
   // Returns true if the |container_view_| is currently visible.
@@ -88,8 +89,6 @@ class ASH_EXPORT AmbientController
   // Handles events on the background photo.
   void OnBackgroundPhotoEvents();
 
-  void UpdateUiMode(AmbientUiMode ui_mode);
-
   void RequestAccessToken(
       AmbientAccessTokenController::AccessTokenCallback callback,
       bool may_refresh_token_on_lock = false);
@@ -190,6 +189,8 @@ class ASH_EXPORT AmbientController
   // Used to record Ambient mode engagement metrics.
   base::Optional<base::Time> start_time_ = base::nullopt;
 
+  base::OneShotTimer delayed_lock_timer_;
+
   base::WeakPtrFactory<AmbientController> weak_ptr_factory_{this};
   DISALLOW_COPY_AND_ASSIGN(AmbientController);
 };

ash/ambient/test/ambient_ash_test_base.cc

@@ -35,6 +35,9 @@
 #include "ui/views/controls/label.h"
 
 namespace ash {
+namespace {
+constexpr float kFastForwardFactor = 1.0001;
+}  // namespace
 
 class TestAmbientURLLoaderImpl : public AmbientURLLoader {
  public:
@@ -141,7 +144,7 @@ void AmbientAshTestBase::SetAmbientModeEnabled(bool enabled) {
 
 void AmbientAshTestBase::ShowAmbientScreen() {
   // The widget will be destroyed in |AshTestBase::TearDown()|.
-  ambient_controller()->ShowUi(AmbientUiMode::kInSessionUi);
+  ambient_controller()->ShowUi();
   // The UI only shows when images are downloaded to avoid showing blank screen.
   FastForwardToNextImage();
   // Flush the message loop to finish all async calls.
@@ -149,12 +152,11 @@ void AmbientAshTestBase::ShowAmbientScreen() {
 }
 
 void AmbientAshTestBase::HideAmbientScreen() {
-  ambient_controller()->HideLockScreenUi();
+  ambient_controller()->ShowHiddenUi();
 }
 
 void AmbientAshTestBase::CloseAmbientScreen() {
-  ambient_controller()->ambient_ui_model()->SetUiVisibility(
-      AmbientUiVisibility::kClosed);
+  ambient_controller()->CloseUi();
 }
 
 void AmbientAshTestBase::LockScreen() {
@@ -242,11 +244,51 @@ MediaStringView* AmbientAshTestBase::GetMediaStringView() {
 
 void AmbientAshTestBase::FastForwardToInactivity() {
   task_environment()->FastForwardBy(
-      2 * AmbientController::kAutoShowWaitTimeInterval);
+      kFastForwardFactor * AmbientController::kAutoShowWaitTimeInterval);
 }
 
 void AmbientAshTestBase::FastForwardToNextImage() {
-  task_environment()->FastForwardBy(1.2 * kPhotoRefreshInterval);
+  task_environment()->FastForwardBy(kFastForwardFactor * kPhotoRefreshInterval);
+}
+
+void AmbientAshTestBase::FastForwardTiny() {
+  // `TestAmbientURLLoaderImpl` has a small delay (1ms) to fake download delay,
+  // here we fake plenty of time to download the image.
+  task_environment()->FastForwardBy(base::TimeDelta::FromMilliseconds(10));
+}
+
+void AmbientAshTestBase::FastForwardToLockScreen() {
+  task_environment()->FastForwardBy(kFastForwardFactor * kLockScreenDelay);
+}
+
+void AmbientAshTestBase::SetPowerStateCharging() {
+  power_manager::PowerSupplyProperties proto;
+  proto.set_battery_state(
+      power_manager::PowerSupplyProperties_BatteryState_CHARGING);
+  proto.set_external_power(
+      power_manager::PowerSupplyProperties_ExternalPower_AC);
+  PowerStatus::Get()->SetProtoForTesting(proto);
+  ambient_controller()->OnPowerStatusChanged();
+}
+
+void AmbientAshTestBase::SetPowerStateDischarging() {
+  power_manager::PowerSupplyProperties proto;
+  proto.set_battery_state(
+      power_manager::PowerSupplyProperties_BatteryState_DISCHARGING);
+  proto.set_external_power(
+      power_manager::PowerSupplyProperties_ExternalPower_DISCONNECTED);
+  PowerStatus::Get()->SetProtoForTesting(proto);
+  ambient_controller()->OnPowerStatusChanged();
+}
+
+void AmbientAshTestBase::SetPowerStateFull() {
+  power_manager::PowerSupplyProperties proto;
+  proto.set_battery_state(
+      power_manager::PowerSupplyProperties_BatteryState_FULL);
+  proto.set_external_power(
+      power_manager::PowerSupplyProperties_ExternalPower_AC);
+  PowerStatus::Get()->SetProtoForTesting(proto);
+  ambient_controller()->OnPowerStatusChanged();
 }
 
 void AmbientAshTestBase::FastForwardToRefreshWeather() {

ash/ambient/test/ambient_ash_test_base.h

@@ -96,9 +96,20 @@ class AmbientAshTestBase : public AshTestBase {
   // Advance the task environment timer to load the next photo.
   void FastForwardToNextImage();
 
+  // Advance the task environment timer a tiny amount. This is intended to
+  // trigger any pending async operations.
+  void FastForwardTiny();
+
   // Advance the task environment timer to load the weather info.
   void FastForwardToRefreshWeather();
 
+  // Advance the task environment timer to ambient mode lock screen delay.
+  void FastForwardToLockScreen();
+
+  void SetPowerStateCharging();
+  void SetPowerStateDischarging();
+  void SetPowerStateFull();
+
   // Returns the number of active wake locks of type |type|.
   int GetNumOfActiveWakeLocks(device::mojom::WakeLockType type);
 

ash/ambient/ui/media_string_view_unittest.cc

@@ -360,6 +360,7 @@ TEST_F(MediaStringViewTest, DoNotShowOnLockScreenIfPrefIsDisabled) {
   // Simulates Ambient Mode shown on lock-screen.
   LockScreen();
   FastForwardToInactivity();
+  FastForwardTiny();
 
   // Simulates active and playing media session.
   SimulateMediaPlaybackStateChanged(

ash/public/cpp/ambient/ambient_ui_model.cc

@@ -44,13 +44,6 @@ void AmbientUiModel::SetUiVisibility(AmbientUiVisibility visibility) {
   NotifyAmbientUiVisibilityChanged();
 }
 
-void AmbientUiModel::SetUiMode(AmbientUiMode ui_mode) {
-  if (ui_mode_ == ui_mode)
-    return;
-
-  ui_mode_ = ui_mode;
-}
-
 void AmbientUiModel::NotifyAmbientUiVisibilityChanged() {
   for (auto& observer : observers_)
     observer.OnAmbientUiVisibilityChanged(ui_visibility_);
@@ -68,4 +61,19 @@ std::ostream& operator<<(std::ostream& out, AmbientUiMode mode) {
   return out;
 }
 
+std::ostream& operator<<(std::ostream& out, AmbientUiVisibility visibility) {
+  switch (visibility) {
+    case AmbientUiVisibility::kShown:
+      out << "kShown";
+      break;
+    case AmbientUiVisibility::kHidden:
+      out << "kHidden";
+      break;
+    case AmbientUiVisibility::kClosed:
+      out << "kClosed";
+      break;
+  }
+  return out;
+}
+
 }  // namespace ash

ash/public/cpp/ambient/ambient_ui_model.h

@@ -51,18 +51,12 @@ class ASH_PUBLIC_EXPORT AmbientUiModel {
   // Updates current UI visibility and notifies all subscribers.
   void SetUiVisibility(AmbientUiVisibility visibility);
 
-  // Updates current UI mode.
-  void SetUiMode(AmbientUiMode ui_mode);
-
   AmbientUiVisibility ui_visibility() const { return ui_visibility_; }
 
-  AmbientUiMode ui_mode() const { return ui_mode_; }
-
  private:
   void NotifyAmbientUiVisibilityChanged();
 
   AmbientUiVisibility ui_visibility_ = AmbientUiVisibility::kClosed;
-  AmbientUiMode ui_mode_ = AmbientUiMode::kLockScreenUi;
 
   base::ObserverList<AmbientUiModelObserver> observers_;
 };
@@ -70,6 +64,9 @@ class ASH_PUBLIC_EXPORT AmbientUiModel {
 ASH_PUBLIC_EXPORT std::ostream& operator<<(std::ostream& out,
                                            AmbientUiMode mode);
 
+ASH_PUBLIC_EXPORT std::ostream& operator<<(std::ostream& out,
+                                           AmbientUiVisibility visibility);
+
 }  // namespace ash
 
 #endif  // ASH_PUBLIC_CPP_AMBIENT_AMBIENT_UI_MODEL_H_