Revert "ambient: handle suspend case better"

This reverts commit 29736b94.

Reason for revert: Tests are failing consistently in linux-chromeos-rel

Sample failing jobs:
[1] https://ci.chromium.org/p/chromium/builders/ci/linux-chromeos-rel/41574
[2] https://ci.chromium.org/p/chromium/builders/ci/linux-chromeos-rel/41572

Tried to reproduce locally, HasMaskLayerWithLongText fails 9 out of 10 times.

Error message: 
../../ash/ambient/ui/media_string_view_unittest.cc:285: Failure
Value of: GetMediaStringViewTextContainer()->layer()->layer_mask_layer()
  Actual: false
Expected: true

Original change's description:
> 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/+/2435334
> Reviewed-by: Tao Wu <wutao@chromium.org>
> Commit-Queue: Xiaohui Chen <xiaohuic@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#812397}

TBR=xiaohuic@chromium.org,wutao@chromium.org,cowmoo@chromium.org

Change-Id: I307c11aed04801e6705be083dcaabfa2c5099802
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: b:169442907
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2440326Reviewed-by: Jiewei Qian  <qjw@chromium.org>
Commit-Queue: Jiewei Qian  <qjw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812549}

ash/ambient/ambient_constants.h

@@ -28,9 +28,6 @@ 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,6 +96,10 @@ bool IsUiHidden(AmbientUiVisibility visibility) {
   return visibility == AmbientUiVisibility::kHidden;
 }
 
+bool IsLockScreenUi(AmbientUiMode mode) {
+  return mode == AmbientUiMode::kLockScreenUi;
+}
+
 bool IsAmbientModeEnabled() {
   if (!AmbientClient::Get()->IsAmbientModeAllowed())
     return false;
@@ -200,8 +204,7 @@ void AmbientController::OnAmbientUiVisibilityChanged(
 
       // Record metrics on ambient mode usage.
       ambient::RecordAmbientModeActivation(
-          /*ui_mode=*/LockScreen::HasInstance() ? AmbientUiMode::kLockScreenUi
-                                                : AmbientUiMode::kInSessionUi,
+          /*ui_mode=*/ambient_ui_model_.ui_mode(),
           /*tablet_mode=*/Shell::Get()->IsInTabletMode());
 
       DCHECK(!start_time_);
@@ -316,11 +319,7 @@ void AmbientController::OnLockStateChanged(bool locked) {
     //        wait.
     RequestAccessToken(base::DoNothing(), /*may_refresh_token_on_lock=*/true);
 
-    if (!IsShown()) {
-      // When lock screen starts, we don't immediately show the UI. The Ui is
-      // hidden and will show after a delay.
-      ShowHiddenUi();
-    }
+    ShowUi(AmbientUiMode::kLockScreenUi);
   } else {
     // Ambient screen will be destroyed along with the lock screen when user
     // logs in.
@@ -375,7 +374,7 @@ void AmbientController::ScreenBrightnessChanged(
   is_screen_off_ = false;
   // If screen is back on, turn on ambient mode for lock screen.
   if (LockScreen::HasInstance())
-    ShowHiddenUi();
+    ShowUi(AmbientUiMode::kLockScreenUi);
 }
 
 void AmbientController::ScreenIdleStateChanged(
@@ -393,7 +392,16 @@ void AmbientController::ScreenIdleStateChanged(
   if (!idle_state.dimmed())
     return;
 
-  ShowUi();
+  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);
+  }
 }
 
 void AmbientController::AddAmbientViewDelegateObserver(
@@ -406,8 +414,8 @@ void AmbientController::RemoveAmbientViewDelegateObserver(
   delegate_.RemoveObserver(observer);
 }
 
-void AmbientController::ShowUi() {
-  DVLOG(1) << __func__;
+void AmbientController::ShowUi(AmbientUiMode mode) {
+  DVLOG(1) << "ShowUi: " << mode;
 
   // TODO(meilinw): move the eligibility check to the idle entry point once
   // implemented: b/149246117.
@@ -416,24 +424,30 @@ void AmbientController::ShowUi() {
     return;
   }
 
-  ambient_ui_model_.SetUiVisibility(AmbientUiVisibility::kShown);
+  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;
+  }
 }
 
-void AmbientController::ShowHiddenUi() {
-  DVLOG(1) << __func__;
-
-  ambient_ui_model_.SetUiVisibility(AmbientUiVisibility::kHidden);
+void AmbientController::CloseUi() {
+  ambient_ui_model_.SetUiVisibility(AmbientUiVisibility::kClosed);
 }
 
-void AmbientController::CloseUi() {
-  DVLOG(1) << __func__;
+void AmbientController::HideLockScreenUi() {
+  DCHECK(IsLockScreenUi(ambient_ui_model_.ui_mode()));
 
-  ambient_ui_model_.SetUiVisibility(AmbientUiVisibility::kClosed);
+  ambient_ui_model_.SetUiVisibility(AmbientUiVisibility::kHidden);
 }
 
 void AmbientController::ToggleInSessionUi() {
-  if (ambient_ui_model_.ui_visibility() == AmbientUiVisibility::kClosed)
-    ShowUi();
+  if (!container_view_)
+    ShowUi(AmbientUiMode::kInSessionUi);
   else
     CloseUi();
 }
@@ -444,12 +458,16 @@ bool AmbientController::IsShown() const {
 
 void AmbientController::OnBackgroundPhotoEvents() {
   // Dismisses the ambient screen when user interacts with the background photo.
-  if (LockScreen::HasInstance())
-    ShowHiddenUi();
+  if (IsLockScreenUi(ambient_ui_model_.ui_mode()))
+    HideLockScreenUi();
   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;
@@ -464,17 +482,6 @@ 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() {
@@ -483,8 +490,6 @@ 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,7 +20,6 @@
 #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"
@@ -75,12 +74,12 @@ class ASH_EXPORT AmbientController
   void AddAmbientViewDelegateObserver(AmbientViewDelegateObserver* observer);
   void RemoveAmbientViewDelegateObserver(AmbientViewDelegateObserver* observer);
 
-  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();
+  // Invoked to show/close ambient UI in |mode|.
+  void ShowUi(AmbientUiMode mode);
   void CloseUi();
 
+  void HideLockScreenUi();
+
   void ToggleInSessionUi();
 
   // Returns true if the |container_view_| is currently visible.
@@ -89,6 +88,8 @@ 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);
@@ -189,8 +190,6 @@ 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,9 +35,6 @@
 #include "ui/views/controls/label.h"
 
 namespace ash {
-namespace {
-constexpr float kFastForwardFactor = 1.0001;
-}  // namespace
 
 class TestAmbientURLLoaderImpl : public AmbientURLLoader {
  public:
@@ -144,7 +141,7 @@ void AmbientAshTestBase::SetAmbientModeEnabled(bool enabled) {
 
 void AmbientAshTestBase::ShowAmbientScreen() {
   // The widget will be destroyed in |AshTestBase::TearDown()|.
-  ambient_controller()->ShowUi();
+  ambient_controller()->ShowUi(AmbientUiMode::kInSessionUi);
   // The UI only shows when images are downloaded to avoid showing blank screen.
   FastForwardToNextImage();
   // Flush the message loop to finish all async calls.
@@ -152,11 +149,12 @@ void AmbientAshTestBase::ShowAmbientScreen() {
 }
 
 void AmbientAshTestBase::HideAmbientScreen() {
-  ambient_controller()->ShowHiddenUi();
+  ambient_controller()->HideLockScreenUi();
 }
 
 void AmbientAshTestBase::CloseAmbientScreen() {
-  ambient_controller()->CloseUi();
+  ambient_controller()->ambient_ui_model()->SetUiVisibility(
+      AmbientUiVisibility::kClosed);
 }
 
 void AmbientAshTestBase::LockScreen() {
@@ -244,51 +242,11 @@ MediaStringView* AmbientAshTestBase::GetMediaStringView() {
 
 void AmbientAshTestBase::FastForwardToInactivity() {
   task_environment()->FastForwardBy(
-      kFastForwardFactor * AmbientController::kAutoShowWaitTimeInterval);
+      2 * AmbientController::kAutoShowWaitTimeInterval);
 }
 
 void AmbientAshTestBase::FastForwardToNextImage() {
-  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();
+  task_environment()->FastForwardBy(1.2 * kPhotoRefreshInterval);
 }
 
 void AmbientAshTestBase::FastForwardToRefreshWeather() {

ash/ambient/test/ambient_ash_test_base.h

@@ -96,20 +96,9 @@ 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,7 +360,6 @@ 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,6 +44,13 @@ 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_);
@@ -61,19 +68,4 @@ 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,12 +51,18 @@ 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_;
 };
@@ -64,9 +70,6 @@ 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_