Revert "ambient: construct photo_controller on enabled"

This reverts commit d82af7a9.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 842133 as the
culprit for failures in the build cycles as shown on:
https://analysis.chromium.org/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyRAsSDVdmU3VzcGVjdGVkQ0wiMWNocm9taXVtL2Q4MmFmN2E5ZWVmYzI1ZGRmMTI0ZmMxMjg0YTM3NmVkYzdmZGNmNWMM

Sample Failed Build: https://ci.chromium.org/b/8858364635543817904

Sample Failed Step: ash_unittests

Original change's description:
> ambient: construct photo_controller on enabled
> 
> AmbientPhotoController and AmbientPhotoCache are only necessary
> when a user is logged in and ambient mode is enabled. Construct them
> at that time, and destruct them if ambient mode is disabled.
> 
> Schedule fetching backup images when AmbientPhotoController is
> constructed, rather than in OnFirstSessionStarted.
> 
> BUG=b:176094707
> TEST=ash_unittests --gtest_filter="AmbientControllerTest.*"
> 
> Cq-Include-Trybots: luci.chrome.try:linux-chromeos-chrome
> Change-Id: Id87bb16e6d4cc14074dd9bdb352fb5795d0f8915
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2605744
> Commit-Queue: Jeffrey Young <cowmoo@chromium.org>
> Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
> Reviewed-by: Tao Wu <wutao@chromium.org>
> Reviewed-by: Jeroen Dhollander <jeroendh@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#842133}


No-Presubmit: true
No-Tree-Checks: true
No-Try: true
BUG=b:176094707
Cq-Include-Trybots: luci.chrome.try:linux-chromeos-chrome

Change-Id: Ieccf27239a7fb618deb72fe6ebe9519462df7add
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2623527
Cr-Commit-Position: refs/heads/master@{#842283}

ash/ambient/ambient_controller.cc

@@ -253,7 +253,6 @@ void AmbientController::OnAmbientUiVisibilityChanged(
         }
       } else {
         DCHECK(visibility == AmbientUiVisibility::kClosed);
-        GetAmbientBackendModel()->ResetImageFailures();
         inactivity_timer_.Stop();
         user_activity_observer_.Reset();
         power_status_observer_.Reset();
@@ -314,6 +313,11 @@ void AmbientController::OnLockStateChanged(bool locked) {
   }
 }
 
+void AmbientController::OnFirstSessionStarted() {
+  if (IsAmbientModeEnabled())
+    ambient_photo_controller_.ScheduleFetchBackupImages();
+}
+
 void AmbientController::OnActiveUserPrefServiceChanged(
     PrefService* pref_service) {
   if (!AmbientClient::Get()->IsAmbientModeAllowed() ||
@@ -445,6 +449,7 @@ void AmbientController::CloseUi() {
   DVLOG(1) << __func__;
 
   ambient_ui_model_.SetUiVisibility(AmbientUiVisibility::kClosed);
+  GetAmbientBackendModel()->ResetImageFailures();
 }
 
 void AmbientController::ToggleInSessionUi() {
@@ -504,6 +509,10 @@ void AmbientController::CloseAllWidgets(bool immediately) {
 }
 
 void AmbientController::OnEnabledPrefChanged() {
+  // TODO(b/176094707) conditionally create/destroy photo_controller and cache
+  // if Ambient is enabled
+  ambient_photo_controller_.InitCache();
+
   if (IsAmbientModeEnabled()) {
     DVLOG(1) << "Ambient mode enabled";
 
@@ -530,11 +539,10 @@ void AmbientController::OnEnabledPrefChanged() {
     OnLockScreenBackgroundTimeoutPrefChanged();
     OnPhotoRefreshIntervalPrefChanged();
 
-    ambient_photo_controller_ = std::make_unique<AmbientPhotoController>();
-
     ambient_ui_model_observer_.Observe(&ambient_ui_model_);
 
-    ambient_backend_model_observer_.Observe(GetAmbientBackendModel());
+    ambient_backend_model_observer_.Observe(
+        ambient_photo_controller_.ambient_backend_model());
 
     auto* power_manager_client = chromeos::PowerManagerClient::Get();
     DCHECK(power_manager_client);
@@ -556,8 +564,6 @@ void AmbientController::OnEnabledPrefChanged() {
         pref_change_registrar_->Remove(pref_name);
     }
 
-    ambient_photo_controller_.reset();
-
     ambient_ui_model_observer_.Reset();
     ambient_backend_model_observer_.Reset();
     power_manager_client_observer_.Reset();
@@ -624,8 +630,7 @@ void AmbientController::DismissUI() {
 }
 
 AmbientBackendModel* AmbientController::GetAmbientBackendModel() {
-  DCHECK(ambient_photo_controller_);
-  return ambient_photo_controller_->ambient_backend_model();
+  return ambient_photo_controller_.ambient_backend_model();
 }
 
 void AmbientController::OnImagesReady() {
@@ -680,13 +685,11 @@ void AmbientController::CreateAndShowWidgets() {
 }
 
 void AmbientController::StartRefreshingImages() {
-  DCHECK(ambient_photo_controller_);
-  ambient_photo_controller_->StartScreenUpdate();
+  ambient_photo_controller_.StartScreenUpdate();
 }
 
 void AmbientController::StopRefreshingImages() {
-  DCHECK(ambient_photo_controller_);
-  ambient_photo_controller_->StopScreenUpdate();
+  ambient_photo_controller_.StopScreenUpdate();
 }
 
 void AmbientController::set_backend_controller_for_testing(

ash/ambient/ambient_controller.h

@@ -65,6 +65,7 @@ class ASH_EXPORT AmbientController
 
   // SessionObserver:
   void OnLockStateChanged(bool locked) override;
+  void OnFirstSessionStarted() override;
   void OnActiveUserPrefServiceChanged(PrefService* pref_service) override;
 
   // PowerStatus::Observer:
@@ -118,7 +119,7 @@ class ASH_EXPORT AmbientController
   }
 
   AmbientPhotoController* ambient_photo_controller() {
-    return ambient_photo_controller_.get();
+    return &ambient_photo_controller_;
   }
 
   AmbientUiModel* ambient_ui_model() { return &ambient_ui_model_; }
@@ -175,7 +176,7 @@ class ASH_EXPORT AmbientController
 
   AmbientAccessTokenController access_token_controller_;
   std::unique_ptr<AmbientBackendController> ambient_backend_controller_;
-  std::unique_ptr<AmbientPhotoController> ambient_photo_controller_;
+  AmbientPhotoController ambient_photo_controller_;
 
   // Monitors the device inactivity and controls the auto-show of ambient.
   base::OneShotTimer inactivity_timer_;

ash/ambient/ambient_photo_controller.cc

@@ -104,14 +104,9 @@ base::FilePath GetCacheRootPath() {
 AmbientPhotoController::AmbientPhotoController()
     : fetch_topic_retry_backoff_(&kFetchTopicRetryBackoffPolicy),
       resume_fetch_image_backoff_(&kResumeFetchImageBackoffPolicy),
-      photo_cache_(AmbientPhotoCache::Create(GetCacheRootPath().Append(
-          FILE_PATH_LITERAL(kAmbientModeCacheDirectoryName)))),
-      backup_photo_cache_(AmbientPhotoCache::Create(GetCacheRootPath().Append(
-          FILE_PATH_LITERAL(kAmbientModeBackupCacheDirectoryName)))),
       task_runner_(
           base::ThreadPool::CreateSequencedTaskRunner(GetTaskTraits())) {
   ambient_backend_model_observer_.Add(&ambient_backend_model_);
-  ScheduleFetchBackupImages();
 }
 
 AmbientPhotoController::~AmbientPhotoController() = default;
@@ -144,6 +139,18 @@ void AmbientPhotoController::StopScreenUpdate() {
   weak_factory_.InvalidateWeakPtrs();
 }
 
+void AmbientPhotoController::ScheduleFetchBackupImages() {
+  if (backup_photo_refresh_timer_.IsRunning())
+    return;
+
+  backup_photo_refresh_timer_.Start(
+      FROM_HERE,
+      std::max(kBackupPhotoRefreshDelay,
+               resume_fetch_image_backoff_.GetTimeUntilRelease()),
+      base::BindOnce(&AmbientPhotoController::FetchBackupImages,
+                     weak_factory_.GetWeakPtr()));
+}
+
 void AmbientPhotoController::OnTopicsChanged() {
   if (ambient_backend_model_.topics().size() < kMaxNumberOfCachedImages)
     ScheduleFetchTopics(/*backoff=*/false);
@@ -180,6 +187,17 @@ void AmbientPhotoController::ClearCache() {
   backup_photo_cache_->Clear();
 }
 
+void AmbientPhotoController::InitCache() {
+  if (!photo_cache_) {
+    photo_cache_ = AmbientPhotoCache::Create(GetCacheRootPath().Append(
+        FILE_PATH_LITERAL(kAmbientModeCacheDirectoryName)));
+  }
+  if (!backup_photo_cache_) {
+    backup_photo_cache_ = AmbientPhotoCache::Create(GetCacheRootPath().Append(
+        FILE_PATH_LITERAL(kAmbientModeBackupCacheDirectoryName)));
+  }
+}
+
 void AmbientPhotoController::ScheduleFetchTopics(bool backoff) {
   // If retry, using the backoff delay, otherwise the default delay.
   const base::TimeDelta delay =
@@ -199,19 +217,6 @@ void AmbientPhotoController::ScheduleRefreshImage() {
                      weak_factory_.GetWeakPtr()));
 }
 
-void AmbientPhotoController::ScheduleFetchBackupImages() {
-  DVLOG(3) << __func__;
-  if (backup_photo_refresh_timer_.IsRunning())
-    return;
-
-  backup_photo_refresh_timer_.Start(
-      FROM_HERE,
-      std::max(kBackupPhotoRefreshDelay,
-               resume_fetch_image_backoff_.GetTimeUntilRelease()),
-      base::BindOnce(&AmbientPhotoController::FetchBackupImages,
-                     weak_factory_.GetWeakPtr()));
-}
-
 void AmbientPhotoController::FetchBackupImages() {
   const auto& backup_photo_urls = GetBackupPhotoUrls();
   backup_retries_to_read_from_cache_ = backup_photo_urls.size();

ash/ambient/ambient_photo_controller.h

@@ -58,6 +58,8 @@ class ASH_EXPORT AmbientPhotoController : public AmbientBackendModelObserver {
   void StartScreenUpdate();
   void StopScreenUpdate();
 
+  void ScheduleFetchBackupImages();
+
   AmbientBackendModel* ambient_backend_model() {
     return &ambient_backend_model_;
   }
@@ -66,7 +68,7 @@ class ASH_EXPORT AmbientPhotoController : public AmbientBackendModelObserver {
     return photo_refresh_timer_;
   }
 
-  base::OneShotTimer& backup_photo_refresh_timer_for_testing() {
+  const base::OneShotTimer& backup_photo_refresh_timer_for_testing() const {
     return backup_photo_refresh_timer_;
   }
 
@@ -76,9 +78,10 @@ class ASH_EXPORT AmbientPhotoController : public AmbientBackendModelObserver {
   // Clear cache when Settings changes.
   void ClearCache();
 
+  void InitCache();
+
  private:
   friend class AmbientAshTestBase;
-  friend class AmbientPhotoControllerTest;
 
   void FetchTopics();
 
@@ -88,8 +91,6 @@ class ASH_EXPORT AmbientPhotoController : public AmbientBackendModelObserver {
 
   void ScheduleRefreshImage();
 
-  void ScheduleFetchBackupImages();
-
   // Download backup cache images.
   void FetchBackupImages();
 

ash/ambient/ambient_photo_controller_unittest.cc

@@ -80,10 +80,6 @@ class AmbientPhotoControllerTest : public AmbientAshTestBase {
                               loop.QuitClosure());
     loop.Run();
   }
-
-  void ScheduleFetchBackupImages() {
-    photo_controller()->ScheduleFetchBackupImages();
-  }
 };
 
 // Test that topics are downloaded when starting screen update.
@@ -286,7 +282,7 @@ TEST_F(AmbientPhotoControllerTest, ShouldDownloadBackupImagesWhenScheduled) {
   std::string expected_data = "backup data";
   SetBackupDownloadPhotoData(expected_data);
 
-  ScheduleFetchBackupImages();
+  photo_controller()->ScheduleFetchBackupImages();
 
   EXPECT_TRUE(
       photo_controller()->backup_photo_refresh_timer_for_testing().IsRunning());
@@ -312,7 +308,7 @@ TEST_F(AmbientPhotoControllerTest, ShouldDownloadBackupImagesWhenScheduled) {
 }
 
 TEST_F(AmbientPhotoControllerTest, ShouldResetTimerWhenBackupImagesFail) {
-  ScheduleFetchBackupImages();
+  photo_controller()->ScheduleFetchBackupImages();
 
   EXPECT_TRUE(
       photo_controller()->backup_photo_refresh_timer_for_testing().IsRunning());
@@ -330,7 +326,7 @@ TEST_F(AmbientPhotoControllerTest, ShouldResetTimerWhenBackupImagesFail) {
 
 TEST_F(AmbientPhotoControllerTest,
        ShouldStartDownloadBackupImagesOnAmbientModeStart) {
-  ScheduleFetchBackupImages();
+  photo_controller()->ScheduleFetchBackupImages();
 
   EXPECT_TRUE(
       photo_controller()->backup_photo_refresh_timer_for_testing().IsRunning());

ash/ambient/test/ambient_ash_test_base.cc

@@ -185,6 +185,10 @@ void AmbientAshTestBase::SetUp() {
   ambient_controller()->set_backend_controller_for_testing(nullptr);
   ambient_controller()->set_backend_controller_for_testing(
       std::make_unique<FakeAmbientBackendControllerImpl>());
+  photo_controller()->set_photo_cache_for_testing(
+      std::make_unique<TestAmbientPhotoCacheImpl>());
+  photo_controller()->set_backup_photo_cache_for_testing(
+      std::make_unique<TestAmbientPhotoCacheImpl>());
   token_controller()->SetTokenUsageBufferForTesting(
       base::TimeDelta::FromSeconds(30));
   SetAmbientModeEnabled(true);
@@ -198,14 +202,6 @@ void AmbientAshTestBase::TearDown() {
 void AmbientAshTestBase::SetAmbientModeEnabled(bool enabled) {
   Shell::Get()->session_controller()->GetActivePrefService()->SetBoolean(
       ambient::prefs::kAmbientModeEnabled, enabled);
-
-  if (enabled) {
-    photo_controller()->set_photo_cache_for_testing(
-        std::make_unique<TestAmbientPhotoCacheImpl>());
-    photo_controller()->set_backup_photo_cache_for_testing(
-        std::make_unique<TestAmbientPhotoCacheImpl>());
-    photo_controller()->backup_photo_refresh_timer_for_testing().Stop();
-  }
 }
 
 void AmbientAshTestBase::ShowAmbientScreen() {