ambient: Cache downloaded photo on disk

Previously we download one photo and display it and then discard the
photo. However when the access token expires, we cannot get photo link
info anymore. This patch saves previous downloaded photos to disk and
then cycle through those photos for a longer time span.

Bug: b/156919253
Test: new unittests
Change-Id: I6a0bd0b3f69e1e6c3d950c158960071dfce122c6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2207755
Commit-Queue: Tao Wu <wutao@chromium.org>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#784498}

ash/BUILD.gn

@@ -2604,6 +2604,7 @@ static_library("test_support") {
     "//ui/events:test_support",
     "//ui/events/devices",
     "//ui/events/devices:test_support",
+    "//ui/gfx:test_support",
     "//ui/gl",
     "//ui/gl:test_support",
     "//ui/message_center",

ash/ambient/ambient_constants.h

@@ -19,6 +19,9 @@ constexpr base::TimeDelta kAnimationDuration =
 constexpr base::TimeDelta kPhotoRefreshInterval =
     base::TimeDelta::FromSeconds(5);
 
+// Directory name of ambient mode.
+constexpr char kAmbientModeDirectoryName[] = "ambient-mode";
+
 }  // namespace ash
 
 #endif  // ASH_AMBIENT_AMBIENT_CONSTANTS_H_

ash/ambient/ambient_controller_unittest.cc

@@ -36,6 +36,9 @@ TEST_F(AmbientControllerTest, ShowAmbientScreenUponLock) {
   EXPECT_EQ(AmbientUiModel::Get()->ui_visibility(),
             AmbientUiVisibility::kShown);
   EXPECT_TRUE(ambient_controller()->IsShown());
+
+  // Clean up.
+  CloseAmbientScreen();
 }
 
 TEST_F(AmbientControllerTest, HideAmbientScreen) {
@@ -51,6 +54,9 @@ TEST_F(AmbientControllerTest, HideAmbientScreen) {
   EXPECT_EQ(AmbientUiModel::Get()->ui_visibility(),
             AmbientUiVisibility::kHidden);
   EXPECT_FALSE(container_view()->GetWidget()->IsVisible());
+
+  // Clean up.
+  CloseAmbientScreen();
 }
 
 TEST_F(AmbientControllerTest, CloseAmbientScreenUponUnlock) {
@@ -106,6 +112,9 @@ TEST_F(AmbientControllerTest, ShouldReturnCachedAccessToken) {
       }));
   EXPECT_FALSE(IsAccessTokenRequestPending());
   run_loop.Run();
+
+  // Clean up.
+  CloseAmbientScreen();
 }
 
 TEST_F(AmbientControllerTest, ShouldRefreshAccessTokenAfterFailure) {
@@ -122,6 +131,9 @@ TEST_F(AmbientControllerTest, ShouldRefreshAccessTokenAfterFailure) {
   // the returned token would expire again.
   task_environment()->FastForwardBy(kDefaultTokenExpirationDelay / 2);
   EXPECT_TRUE(IsAccessTokenRequestPending());
+
+  // Clean up.
+  CloseAmbientScreen();
 }
 
 TEST_F(AmbientControllerTest,
@@ -224,6 +236,9 @@ TEST_F(AmbientControllerTest, ShouldDismissContainerViewWhenKeyPressed) {
   GetEventGenerator()->PressKey(ui::VKEY_SPACE, /*flags=*/0);
 
   EXPECT_FALSE(container_view()->GetWidget()->IsVisible());
+
+  // Clean up.
+  CloseAmbientScreen();
 }
 
 }  // namespace ash

ash/ambient/ambient_photo_controller.cc

@@ -9,14 +9,30 @@
 
 #include "ash/ambient/ambient_constants.h"
 #include "ash/ambient/ambient_controller.h"
+#include "ash/public/cpp/ambient/ambient_client.h"
 #include "ash/public/cpp/image_downloader.h"
 #include "ash/shell.h"
+#include "base/base_paths.h"
 #include "base/bind.h"
 #include "base/callback.h"
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
+#include "base/guid.h"
+#include "base/hash/sha1.h"
 #include "base/optional.h"
+#include "base/path_service.h"
 #include "base/rand_util.h"
+#include "base/system/sys_info.h"
+#include "base/task/post_task.h"
+#include "base/task/task_traits.h"
+#include "base/task/thread_pool.h"
+#include "base/task_runner_util.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "net/traffic_annotation/network_traffic_annotation.h"
+#include "services/data_decoder/public/cpp/decode_image.h"
+#include "services/network/public/cpp/resource_request.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
+#include "services/network/public/cpp/simple_url_loader.h"
 #include "ui/gfx/image/image_skia.h"
 #include "url/gurl.h"
 
@@ -40,6 +56,12 @@ constexpr int kTopicsBatchSize = 2;
 // E.g. it will be max 36 seconds if we want to fetch 50 batches in 30 mins.
 constexpr base::TimeDelta kTopicFetchDelayMax = base::TimeDelta::FromSeconds(3);
 
+constexpr int kMaxImageSizeInBytes = 5 * 1024 * 1024;
+
+constexpr int kMaxReservedAvailableDiskSpaceByte = 200 * 1024 * 1024;
+
+constexpr char kPhotoFileExt[] = ".img";
+
 using DownloadCallback = base::OnceCallback<void(const gfx::ImageSkia&)>;
 
 void DownloadImageFromUrl(const std::string& url, DownloadCallback callback) {
@@ -49,16 +71,167 @@ void DownloadImageFromUrl(const std::string& url, DownloadCallback callback) {
                                    base::BindOnce(std::move(callback)));
 }
 
+// Get the root path for ambient mode.
+base::FilePath GetRootPath() {
+  base::FilePath home_dir;
+  CHECK(base::PathService::Get(base::DIR_HOME, &home_dir));
+  return home_dir.Append(FILE_PATH_LITERAL(kAmbientModeDirectoryName));
+}
+
+void DeletePathRecursively(const base::FilePath& path) {
+  base::DeletePathRecursively(path);
+}
+
+std::string ToPhotoFileName(const std::string& url) {
+  return base::SHA1HashString(url) + std::string(kPhotoFileExt);
+}
+
+void ToImageSkia(DownloadCallback callback, const SkBitmap& image) {
+  if (image.isNull()) {
+    std::move(callback).Run(gfx::ImageSkia());
+    return;
+  }
+
+  gfx::ImageSkia image_skia = gfx::ImageSkia::CreateFrom1xBitmap(image);
+  image_skia.MakeThreadSafe();
+
+  std::move(callback).Run(image_skia);
+}
+
+base::TaskTraits GetTaskTraits() {
+  return {base::MayBlock(), base::TaskPriority::USER_BLOCKING,
+          base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN};
+}
+
+// TODO: Move to ambient_util.
+void WriteFile(const base::FilePath& path, const std::string& data) {
+  if (!base::PathExists(GetRootPath()) &&
+      !base::CreateDirectory(GetRootPath())) {
+    LOG(ERROR) << "Cannot create ambient mode directory.";
+    return;
+  }
+
+  if (base::SysInfo::AmountOfFreeDiskSpace(GetRootPath()) <
+      kMaxReservedAvailableDiskSpaceByte) {
+    LOG(WARNING) << "Not enough disk space left.";
+    return;
+  }
+
+  if (!base::PathExists(path.DirName()) &&
+      !base::CreateDirectory(path.DirName())) {
+    LOG(ERROR) << "Cannot create a new session directory.";
+    return;
+  }
+
+  // Create a temp file.
+  base::FilePath temp_file;
+  if (!base::CreateTemporaryFileInDir(path.DirName(), &temp_file)) {
+    LOG(ERROR) << "Cannot create a temporary file.";
+    return;
+  }
+
+  // Write to the tmp file.
+  const int size = data.size();
+  int written_size = base::WriteFile(temp_file, data.data(), size);
+  if (written_size != size) {
+    LOG(ERROR) << "Cannot write the temporary file.";
+    base::DeleteFile(temp_file, /*recursive=*/false);
+    return;
+  }
+
+  // Replace the current file with the temp file.
+  if (!base::ReplaceFile(temp_file, path, /*error=*/nullptr))
+    LOG(ERROR) << "Cannot replace the temporary file.";
+}
+
 }  // namespace
 
-AmbientPhotoController::AmbientPhotoController() {
-  ambient_backedn_model_observer_.Add(&ambient_backend_model_);
+class AmbientURLLoaderImpl : public AmbientURLLoader {
+ public:
+  AmbientURLLoaderImpl() = default;
+  ~AmbientURLLoaderImpl() override = default;
+
+  // AmbientURLLoader:
+  void Download(
+      const std::string& url,
+      network::SimpleURLLoader::BodyAsStringCallback callback) override {
+    auto resource_request = std::make_unique<network::ResourceRequest>();
+    resource_request->url = GURL(url);
+    resource_request->method = "GET";
+    resource_request->credentials_mode = network::mojom::CredentialsMode::kOmit;
+
+    auto simple_loader = network::SimpleURLLoader::Create(
+        std::move(resource_request), NO_TRAFFIC_ANNOTATION_YET);
+    auto* loader_ptr = simple_loader.get();
+    auto loader_factory = AmbientClient::Get()->GetURLLoaderFactory();
+    loader_ptr->DownloadToString(
+        loader_factory.get(),
+        base::BindOnce(&AmbientURLLoaderImpl::OnUrlDownloaded,
+                       weak_factory_.GetWeakPtr(), std::move(callback),
+                       std::move(simple_loader), loader_factory),
+        kMaxImageSizeInBytes);
+  }
+
+ private:
+  // Called when the download completes.
+  void OnUrlDownloaded(
+      network::SimpleURLLoader::BodyAsStringCallback callback,
+      std::unique_ptr<network::SimpleURLLoader> simple_loader,
+      scoped_refptr<network::SharedURLLoaderFactory> loader_factory,
+      std::unique_ptr<std::string> response_body) {
+    if (simple_loader->NetError() == net::OK && response_body) {
+      std::move(callback).Run(std::move(response_body));
+      return;
+    }
+
+    int response_code = -1;
+    if (simple_loader->ResponseInfo() &&
+        simple_loader->ResponseInfo()->headers) {
+      response_code = simple_loader->ResponseInfo()->headers->response_code();
+    }
+
+    LOG(ERROR) << "Downloading Backdrop proto failed with error code: "
+               << response_code << " with network error"
+               << simple_loader->NetError();
+    std::move(callback).Run(std::make_unique<std::string>());
+  }
+
+  base::WeakPtrFactory<AmbientURLLoaderImpl> weak_factory_{this};
+};
+
+class AmbientImageDecoderImpl : public AmbientImageDecoder {
+ public:
+  AmbientImageDecoderImpl() = default;
+  ~AmbientImageDecoderImpl() override = default;
+
+  // AmbientImageDecoder:
+  void Decode(
+      const std::vector<uint8_t>& encoded_bytes,
+      base::OnceCallback<void(const gfx::ImageSkia&)> callback) override {
+    data_decoder::DecodeImageIsolated(
+        std::move(encoded_bytes), data_decoder::mojom::ImageCodec::DEFAULT,
+        /*shrink_to_fit=*/true, data_decoder::kDefaultMaxSizeInBytes,
+        /*desired_image_frame_size=*/gfx::Size(),
+        base::BindOnce(&ToImageSkia, std::move(callback)));
+  }
+};
+
+AmbientPhotoController::AmbientPhotoController()
+    : url_loader_(std::make_unique<AmbientURLLoaderImpl>()),
+      image_decoder_(std::make_unique<AmbientImageDecoderImpl>()),
+      task_runner_(
+          base::ThreadPool::CreateSequencedTaskRunner(GetTaskTraits())) {
+  ambient_backend_model_observer_.Add(&ambient_backend_model_);
 }
 
 AmbientPhotoController::~AmbientPhotoController() = default;
 
 void AmbientPhotoController::StartScreenUpdate() {
-  FetchTopics();
+  root_path_ = GetRootPath().Append(FILE_PATH_LITERAL(base::GenerateGUID()));
+  task_runner_->PostTaskAndReply(
+      FROM_HERE, base::BindOnce(&DeletePathRecursively, GetRootPath()),
+      base::BindOnce(&AmbientPhotoController::FetchTopics,
+                     weak_factory_.GetWeakPtr()));
 }
 
 void AmbientPhotoController::StopScreenUpdate() {
@@ -67,6 +240,9 @@ void AmbientPhotoController::StopScreenUpdate() {
   topics_batch_fetched_ = 0;
   ambient_backend_model_.Clear();
   weak_factory_.InvalidateWeakPtrs();
+
+  task_runner_->PostTask(FROM_HERE,
+                         base::BindOnce(&DeletePathRecursively, root_path_));
 }
 
 void AmbientPhotoController::OnTopicsChanged() {
@@ -108,7 +284,7 @@ void AmbientPhotoController::ScheduleRefreshImage() {
   // is true.
   photo_refresh_timer_.Start(
       FROM_HERE, refresh_interval,
-      base::BindOnce(&AmbientPhotoController::GetNextImage,
+      base::BindOnce(&AmbientPhotoController::TryReadPhotoRawData,
                      weak_factory_.GetWeakPtr()));
 }
 
@@ -125,14 +301,6 @@ const AmbientModeTopic& AmbientPhotoController::GetNextTopic() {
   return topics[topic_index_++];
 }
 
-void AmbientPhotoController::GetNextImage() {
-  const AmbientModeTopic& topic = GetNextTopic();
-  const std::string& image_url = topic.portrait_image_url.value_or(topic.url);
-  DownloadImageFromUrl(
-      image_url, base::BindOnce(&AmbientPhotoController::OnPhotoDownloaded,
-                                weak_factory_.GetWeakPtr()));
-}
-
 void AmbientPhotoController::OnScreenUpdateInfoFetched(
     const ash::ScreenUpdate& screen_update) {
   // It is possible that |screen_update| is an empty instance if fatal errors
@@ -148,6 +316,95 @@ void AmbientPhotoController::OnScreenUpdateInfoFetched(
   StartDownloadingWeatherConditionIcon(screen_update);
 }
 
+void AmbientPhotoController::TryReadPhotoRawData() {
+  const AmbientModeTopic& topic = GetNextTopic();
+  const std::string& image_url = topic.portrait_image_url.value_or(topic.url);
+
+  base::FilePath path = root_path_.Append(ToPhotoFileName(image_url));
+  task_runner_->PostTaskAndReplyWithResult(
+      FROM_HERE,
+      base::BindOnce(
+          [](const base::FilePath& path) {
+            auto data = std::make_unique<std::string>();
+            if (!base::ReadFileToString(path, data.get()))
+              data = nullptr;
+            return data;
+          },
+          path),
+      base::BindOnce(&AmbientPhotoController::OnPhotoRawDataRead,
+                     weak_factory_.GetWeakPtr(), image_url));
+}
+
+void AmbientPhotoController::OnPhotoRawDataRead(
+    const std::string& image_url,
+    std::unique_ptr<std::string> data) {
+  if (!data || data->empty()) {
+    url_loader_->Download(
+        image_url,
+        base::BindOnce(&AmbientPhotoController::OnPhotoRawDataAvailable,
+                       weak_factory_.GetWeakPtr(), image_url,
+                       /*need_to_save=*/true));
+  } else {
+    OnPhotoRawDataAvailable(image_url, /*need_to_save=*/false, std::move(data));
+  }
+}
+
+void AmbientPhotoController::OnPhotoRawDataAvailable(
+    const std::string& image_url,
+    bool need_to_save,
+    std::unique_ptr<std::string> response_body) {
+  if (!response_body) {
+    LOG(ERROR) << "Failed to download image";
+
+    // Continue to get next photo on error.
+    // TODO(b/148485116): Add exponential backoff retry logic.
+    const base::TimeDelta kDelay = base::TimeDelta::FromMilliseconds(100);
+    base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
+        FROM_HERE,
+        base::BindOnce(&AmbientPhotoController::ScheduleRefreshImage,
+                       weak_factory_.GetWeakPtr()),
+        kDelay);
+    return;
+  }
+
+  const base::FilePath path = root_path_.Append(ToPhotoFileName(image_url));
+  task_runner_->PostTaskAndReply(
+      FROM_HERE,
+      base::BindOnce(
+          [](const base::FilePath& path, bool need_to_save,
+             const std::string& data) {
+            if (need_to_save)
+              WriteFile(path, data);
+          },
+          path, need_to_save, *response_body),
+      base::BindOnce(&AmbientPhotoController::DecodePhotoRawData,
+                     weak_factory_.GetWeakPtr(), std::move(response_body)));
+}
+
+void AmbientPhotoController::DecodePhotoRawData(
+    std::unique_ptr<std::string> data) {
+  std::vector<uint8_t> image_bytes(data->begin(), data->end());
+  image_decoder_->Decode(image_bytes,
+                         base::BindOnce(&AmbientPhotoController::OnPhotoDecoded,
+                                        weak_factory_.GetWeakPtr()));
+}
+
+void AmbientPhotoController::OnPhotoDecoded(const gfx::ImageSkia& image) {
+  base::TimeDelta kDelay;
+  if (image.isNull()) {
+    LOG(WARNING) << "Image is null";
+    kDelay = base::TimeDelta::FromMilliseconds(100);
+  } else {
+    ambient_backend_model_.AddNextImage(image);
+  }
+
+  base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
+      FROM_HERE,
+      base::BindOnce(&AmbientPhotoController::ScheduleRefreshImage,
+                     weak_factory_.GetWeakPtr()),
+      kDelay);
+}
+
 void AmbientPhotoController::StartDownloadingWeatherConditionIcon(
     const ash::ScreenUpdate& screen_update) {
   if (!screen_update.weather_info.has_value()) {
@@ -175,13 +432,6 @@ void AmbientPhotoController::StartDownloadingWeatherConditionIcon(
                      screen_update.weather_info->temp_f));
 }
 
-void AmbientPhotoController::OnPhotoDownloaded(const gfx::ImageSkia& image) {
-  if (!image.isNull())
-    ambient_backend_model_.AddNextImage(image);
-
-  ScheduleRefreshImage();
-}
-
 void AmbientPhotoController::OnWeatherConditionIconDownloaded(
     base::Optional<float> temp_f,
     const gfx::ImageSkia& icon) {

ash/ambient/ambient_photo_controller.h

@@ -5,6 +5,9 @@
 #ifndef ASH_AMBIENT_AMBIENT_PHOTO_CONTROLLER_H_
 #define ASH_AMBIENT_AMBIENT_PHOTO_CONTROLLER_H_
 
+#include <memory>
+#include <string>
+#include <utility>
 #include <vector>
 
 #include "ash/ambient/model/ambient_backend_model.h"
@@ -13,10 +16,12 @@
 #include "ash/public/cpp/ambient/ambient_backend_controller.h"
 #include "base/callback_forward.h"
 #include "base/macros.h"
+#include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/optional.h"
 #include "base/scoped_observer.h"
 #include "base/timer/timer.h"
+#include "services/network/public/cpp/simple_url_loader.h"
 
 namespace gfx {
 class ImageSkia;
@@ -24,6 +29,36 @@ class ImageSkia;
 
 namespace ash {
 
+// A wrapper class of SimpleURLLoader to download the photo raw data. In the
+// test, this will be used to provide fake data.
+class ASH_EXPORT AmbientURLLoader {
+ public:
+  AmbientURLLoader() = default;
+  AmbientURLLoader(const AmbientURLLoader&) = delete;
+  AmbientURLLoader& operator=(const AmbientURLLoader&) = delete;
+  virtual ~AmbientURLLoader() = default;
+
+  // Download data from the given |url|.
+  virtual void Download(
+      const std::string& url,
+      network::SimpleURLLoader::BodyAsStringCallback callback) = 0;
+};
+
+// A wrapper class of |data_decoder| to decode the photo raw data. In the test,
+// this will be used to provide fake data.
+class ASH_EXPORT AmbientImageDecoder {
+ public:
+  AmbientImageDecoder() = default;
+  AmbientImageDecoder(const AmbientImageDecoder&) = delete;
+  AmbientImageDecoder& operator=(const AmbientImageDecoder&) = delete;
+  virtual ~AmbientImageDecoder() = default;
+
+  // Decode |encoded_bytes| to ImageSkia.
+  virtual void Decode(
+      const std::vector<uint8_t>& encoded_bytes,
+      base::OnceCallback<void(const gfx::ImageSkia&)> callback) = 0;
+};
+
 // Class to handle photos in ambient mode.
 class ASH_EXPORT AmbientPhotoController : public AmbientBackendModelObserver {
  public:
@@ -75,20 +110,42 @@ class ASH_EXPORT AmbientPhotoController : public AmbientBackendModelObserver {
   // Return a topic to download the image.
   const AmbientModeTopic& GetNextTopic();
 
-  void GetNextImage();
-
   void OnScreenUpdateInfoFetched(const ash::ScreenUpdate& screen_update);
 
+  // Try to read photo raw data from disk.
+  void TryReadPhotoRawData();
+
+  // If photo raw data is read successfully, call OnPhotoRawDataAvailable() to
+  // decode data. Otherwise, download the raw data and save to disk.
+  void OnPhotoRawDataRead(const std::string& image_url,
+                          std::unique_ptr<std::string> data);
+
+  void OnPhotoRawDataAvailable(const std::string& image_url,
+                               bool need_to_save,
+                               std::unique_ptr<std::string> response_body);
+
+  void DecodePhotoRawData(std::unique_ptr<std::string> data);
+
+  void OnPhotoDecoded(const gfx::ImageSkia& image);
+
   void StartDownloadingWeatherConditionIcon(
       const ash::ScreenUpdate& screen_update);
 
-  void OnPhotoDownloaded(const gfx::ImageSkia& image);
-
   // Invoked upon completion of the weather icon download, |icon| can be a null
   // image if the download attempt from the url failed.
   void OnWeatherConditionIconDownloaded(base::Optional<float> temp_f,
                                         const gfx::ImageSkia& icon);
 
+  void set_url_loader_for_testing(
+      std::unique_ptr<AmbientURLLoader> url_loader) {
+    url_loader_ = std::move(url_loader);
+  }
+
+  void set_image_decoder_for_testing(
+      std::unique_ptr<AmbientImageDecoder> image_decoder) {
+    image_decoder_ = std::move(image_decoder);
+  }
+
   AmbientBackendModel ambient_backend_model_;
 
   // The timer to refresh photos.
@@ -101,7 +158,15 @@ class ASH_EXPORT AmbientPhotoController : public AmbientBackendModelObserver {
   int topics_batch_fetched_ = 0;
 
   ScopedObserver<AmbientBackendModel, AmbientBackendModelObserver>
-      ambient_backedn_model_observer_{this};
+      ambient_backend_model_observer_{this};
+
+  base::FilePath root_path_;
+
+  std::unique_ptr<AmbientURLLoader> url_loader_;
+
+  std::unique_ptr<AmbientImageDecoder> image_decoder_;
+
+  scoped_refptr<base::SequencedTaskRunner> task_runner_;
 
   base::WeakPtrFactory<AmbientPhotoController> weak_factory_{this};
 

ash/ambient/ambient_photo_controller_unittest.cc

@@ -13,9 +13,15 @@
 #include "ash/public/cpp/ambient/ambient_backend_controller.h"
 #include "ash/shell.h"
 #include "base/barrier_closure.h"
+#include "base/base_paths.h"
 #include "base/bind.h"
 #include "base/callback.h"
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
+#include "base/hash/sha1.h"
+#include "base/path_service.h"
 #include "base/run_loop.h"
+#include "base/system/sys_info.h"
 #include "base/test/bind_test_util.h"
 #include "base/test/task_environment.h"
 #include "base/timer/timer.h"
@@ -52,7 +58,7 @@ TEST_F(AmbientPhotoControllerTest, ShouldStartToDownloadImages) {
 
   // Start to refresh images.
   photo_controller()->StartScreenUpdate();
-  base::RunLoop().RunUntilIdle();
+  task_environment()->FastForwardBy(1.2 * kPhotoRefreshInterval);
   image = photo_controller()->ambient_backend_model()->GetNextImage();
   EXPECT_FALSE(image.isNull());
 
@@ -70,7 +76,7 @@ TEST_F(AmbientPhotoControllerTest, ShouldUpdatePhotoPeriodically) {
 
   // Start to refresh images.
   photo_controller()->StartScreenUpdate();
-  base::RunLoop().RunUntilIdle();
+  task_environment()->FastForwardBy(1.2 * kPhotoRefreshInterval);
   image1 = photo_controller()->ambient_backend_model()->GetNextImage();
   EXPECT_FALSE(image1.isNull());
   EXPECT_TRUE(image2.isNull());
@@ -88,6 +94,36 @@ TEST_F(AmbientPhotoControllerTest, ShouldUpdatePhotoPeriodically) {
   EXPECT_FALSE(image3.isNull());
   EXPECT_FALSE(image1.BackedBySameObjectAs(image3));
   EXPECT_FALSE(image2.BackedBySameObjectAs(image3));
+
+  // Stop to refresh images.
+  photo_controller()->StopScreenUpdate();
+}
+
+// Test that image is saved and deleted when starting/stopping screen update.
+TEST_F(AmbientPhotoControllerTest, ShouldSaveAndDeleteImagesOnDisk) {
+  base::FilePath home_dir;
+  base::PathService::Get(base::DIR_HOME, &home_dir);
+
+  // Start to refresh images.
+  photo_controller()->StartScreenUpdate();
+  base::FilePath root_path =
+      home_dir.Append(FILE_PATH_LITERAL(kAmbientModeDirectoryName));
+  EXPECT_TRUE(!base::PathExists(root_path) ||
+              base::IsDirectoryEmpty(root_path));
+
+  task_environment()->FastForwardBy(1.2 * kPhotoRefreshInterval);
+  auto image = photo_controller()->ambient_backend_model()->GetNextImage();
+  EXPECT_FALSE(image.isNull());
+  EXPECT_TRUE(base::PathExists(root_path));
+  EXPECT_FALSE(base::IsDirectoryEmpty(root_path));
+
+  // Stop to refresh images.
+  photo_controller()->StopScreenUpdate();
+  task_environment()->FastForwardBy(1.2 * kPhotoRefreshInterval);
+  image = photo_controller()->ambient_backend_model()->GetNextImage();
+  EXPECT_TRUE(image.isNull());
+  EXPECT_TRUE(base::PathExists(root_path));
+  EXPECT_TRUE(base::IsDirectoryEmpty(root_path));
 }
 
 }  // namespace ash

ash/ambient/test/ambient_ash_test_base.cc

@@ -5,18 +5,58 @@
 #include "ash/ambient/test/ambient_ash_test_base.h"
 
 #include <memory>
+#include <utility>
+#include <vector>
 
 #include "ash/ambient/ambient_photo_controller.h"
 #include "ash/ambient/fake_ambient_backend_controller_impl.h"
 #include "ash/ambient/ui/ambient_container_view.h"
 #include "ash/ambient/ui/photo_view.h"
 #include "ash/shell.h"
+#include "base/callback.h"
+#include "base/memory/ptr_util.h"
 #include "base/run_loop.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 #include "chromeos/constants/chromeos_features.h"
 #include "ui/gfx/image/image_skia.h"
+#include "ui/gfx/image/image_unittest_util.h"
 
 namespace ash {
 
+class TestAmbientURLLoaderImpl : public AmbientURLLoader {
+ public:
+  TestAmbientURLLoaderImpl() = default;
+  ~TestAmbientURLLoaderImpl() override = default;
+
+  // AmbientURLLoader:
+  void Download(
+      const std::string& url,
+      network::SimpleURLLoader::BodyAsStringCallback callback) override {
+    auto data = std::make_unique<std::string>();
+    *data = "test";
+    // Pretend to respond asynchronously.
+    base::SequencedTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::BindOnce(std::move(callback), std::move(data)));
+  }
+};
+
+class TestAmbientImageDecoderImpl : public AmbientImageDecoder {
+ public:
+  TestAmbientImageDecoderImpl() = default;
+  ~TestAmbientImageDecoderImpl() override = default;
+
+  // AmbientImageDecoder:
+  void Decode(
+      const std::vector<uint8_t>& encoded_bytes,
+      base::OnceCallback<void(const gfx::ImageSkia&)> callback) override {
+    // Pretend to respond asynchronously.
+    base::SequencedTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::BindOnce(std::move(callback), gfx::test::CreateImageSkia(
+                                                /*width=*/10, /*height=*/10)));
+  }
+};
+
 AmbientAshTestBase::AmbientAshTestBase()
     : AshTestBase(base::test::TaskEnvironment::TimeSource::MOCK_TIME) {}
 
@@ -32,10 +72,13 @@ void AmbientAshTestBase::SetUp() {
 
   // Need to reset first and then assign the TestPhotoClient because can only
   // have one instance of AmbientBackendController.
-  Shell::Get()->ambient_controller()->set_backend_controller_for_testing(
-      nullptr);
-  Shell::Get()->ambient_controller()->set_backend_controller_for_testing(
+  ambient_controller()->set_backend_controller_for_testing(nullptr);
+  ambient_controller()->set_backend_controller_for_testing(
       std::make_unique<FakeAmbientBackendControllerImpl>());
+  photo_controller()->set_url_loader_for_testing(
+      std::make_unique<TestAmbientURLLoaderImpl>());
+  photo_controller()->set_image_decoder_for_testing(
+      std::make_unique<TestAmbientImageDecoderImpl>());
 }
 
 void AmbientAshTestBase::TearDown() {
@@ -57,6 +100,11 @@ void AmbientAshTestBase::HideAmbientScreen() {
   ambient_controller()->HideContainerView();
 }
 
+void AmbientAshTestBase::CloseAmbientScreen() {
+  ambient_controller()->ambient_ui_model()->SetUiVisibility(
+      AmbientUiVisibility::kClosed);
+}
+
 void AmbientAshTestBase::LockScreen() {
   GetSessionControllerClient()->LockScreen();
 }

ash/ambient/test/ambient_ash_test_base.h

@@ -42,6 +42,10 @@ class AmbientAshTestBase : public AshTestBase {
   // called.
   void HideAmbientScreen();
 
+  // Closes ambient screen. Can only be called after |ShowAmbientScreen| has
+  // been called.
+  void CloseAmbientScreen();
+
   // Simulates user locks/unlocks screen which will result in ambient widget
   // shown/closed.
   void LockScreen();

ash/ambient/ui/ambient_container_view_unittest.cc

@@ -27,6 +27,9 @@ TEST_F(AmbientContainerViewTest, WindowFullscreenSize) {
   gfx::Rect container_window_bounds =
       widget->GetNativeWindow()->GetBoundsInScreen();
   EXPECT_EQ(root_window_bounds, container_window_bounds);
+
+  // Clean up.
+  CloseAmbientScreen();
 }
 
 }  // namespace ash