ambient: move file writing to AmbientPhotoCache

Mock out all Ambient mode disk access in unit tests.

BUG=b:165481552,b:169813071
TEST=ash_unittests --gtest_filter="AmbientPhotoControllerTest.*"
TEST=ash_unittests --gtest_filter="AmbientPhotoCacheTest.*"

Cq-Include-Trybots: luci.chrome.try:linux-chromeos-chrome
Change-Id: Ic063e6b7686a74c67fbe7a23e146a9e1640abfd9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2558590
Commit-Queue: Jeffrey Young <cowmoo@chromium.org>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Reviewed-by: Tao Wu <wutao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#838748}

ash/BUILD.gn

@@ -1969,6 +1969,7 @@ test("ash_unittests") {
     "accessibility/touch_exploration_controller_unittest.cc",
     "accessibility/touch_exploration_manager_unittest.cc",
     "ambient/ambient_controller_unittest.cc",
+    "ambient/ambient_photo_cache_unittest.cc",
     "ambient/ambient_photo_controller_unittest.cc",
     "ambient/autotest_ambient_api_unittest.cc",
     "ambient/model/ambient_backend_model_unittest.cc",

ash/ambient/ambient_controller.cc

@@ -509,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";
 

ash/ambient/ambient_photo_cache.h

@@ -8,6 +8,7 @@
 #include <memory>
 #include <string>
 
+#include "ash/ash_export.h"
 #include "base/callback_forward.h"
 #include "base/files/file_path.h"
 
@@ -17,31 +18,77 @@ class ImageSkia;
 
 namespace ash {
 
+// Holds the return value for |AmbientPhotoCache::ReadFiles| to use with
+// |task_runner_->PostTaskAndReplyWithResult|.
+// Represented on disk by a file for each of |image|, |details|, and
+// |related_image|.
+struct ASH_EXPORT PhotoCacheEntry {
+  PhotoCacheEntry();
+
+  PhotoCacheEntry(std::unique_ptr<std::string> image,
+                  std::unique_ptr<std::string> details,
+                  std::unique_ptr<std::string> related_image);
+
+  PhotoCacheEntry(const PhotoCacheEntry&) = delete;
+  PhotoCacheEntry& operator=(const PhotoCacheEntry&) = delete;
+  PhotoCacheEntry(PhotoCacheEntry&&);
+
+  ~PhotoCacheEntry();
+
+  void reset();
+
+  std::unique_ptr<std::string> image;
+  std::unique_ptr<std::string> details;
+  std::unique_ptr<std::string> related_image;
+};
+
 // Interface for downloading and decoding photos for Ambient mode. Mocked for
-// testing to isolate from network.
-class AmbientPhotoCache {
+// testing to isolate from network and file system.
+// Each cache entry is written to disk as three files in the |root_path|, with
+// filenames prefixed by |cache_index|.
+class ASH_EXPORT AmbientPhotoCache {
  public:
   AmbientPhotoCache() = default;
   AmbientPhotoCache(const AmbientPhotoCache&) = delete;
   AmbientPhotoCache& operator=(const AmbientPhotoCache&) = delete;
   virtual ~AmbientPhotoCache() = default;
 
-  static std::unique_ptr<AmbientPhotoCache> Create();
+  static std::unique_ptr<AmbientPhotoCache> Create(base::FilePath root_path);
 
   virtual void DownloadPhoto(
       const std::string& url,
       base::OnceCallback<void(std::unique_ptr<std::string>)> callback) = 0;
 
-  // Saves the photo to |file_path| and calls |callback| when complete. If an
-  // error occurs, will call |callback| with an empty path.
-  virtual void DownloadPhotoToFile(
-      const std::string& url,
-      base::OnceCallback<void(base::FilePath)> callback,
-      const base::FilePath& file_path) = 0;
+  // Saves the photo at |url| to |cache_index| and calls |callback| with a
+  // boolean that indicates success. Setting |is_related| will change the
+  // filename to indicate that this is a paired photo.
+  virtual void DownloadPhotoToFile(const std::string& url,
+                                   int cache_index,
+                                   bool is_related,
+                                   base::OnceCallback<void(bool)> callback) = 0;
 
   virtual void DecodePhoto(
       std::unique_ptr<std::string> data,
       base::OnceCallback<void(const gfx::ImageSkia&)> callback) = 0;
+
+  // Write files to disk at |cache_index| and call |callback| when complete.
+  // |image| and |related_image| are encoded jpg images that must be decoded
+  // with |DecodePhoto| to display. |details| is human readable text.
+  virtual void WriteFiles(int cache_index,
+                          const std::string* const image,
+                          const std::string* const details,
+                          const std::string* const related_image,
+                          base::OnceClosure callback) = 0;
+
+  // Read the files at |cache_index| and call |callback| with a struct
+  // containing the contents of each file. If a particular file fails to be
+  // read, it may be represented as nullptr or empty string.
+  virtual void ReadFiles(
+      int cache_index,
+      base::OnceCallback<void(PhotoCacheEntry)> callback) = 0;
+
+  // Erase all stored files from disk.
+  virtual void Clear() = 0;
 };
 
 }  // namespace ash

ash/ambient/ambient_photo_cache_unittest.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ash/ambient/ambient_photo_cache.h"
+
+#include "ash/ambient/ambient_constants.h"
+#include "base/callback_forward.h"
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
+#include "base/path_service.h"
+#include "base/run_loop.h"
+#include "base/test/task_environment.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace ash {
+
+namespace {
+
+base::FilePath GetTestPath() {
+  base::FilePath path;
+  EXPECT_TRUE(base::PathService::Get(base::DIR_TEMP, &path));
+  path = path.Append(FILE_PATH_LITERAL(kAmbientModeDirectoryName));
+  return path;
+}
+
+}  // namespace
+
+class AmbientPhotoCacheTest : public testing::Test {
+ public:
+  void SetUp() override {
+    auto test_path = GetTestPath();
+    base::DeletePathRecursively(test_path);
+    photo_cache_ = AmbientPhotoCache::Create(test_path);
+  }
+
+  void TearDown() override { base::DeletePathRecursively(GetTestPath()); }
+
+  AmbientPhotoCache* photo_cache() { return photo_cache_.get(); }
+
+ protected:
+  base::test::TaskEnvironment task_environment_;
+
+ private:
+  std::unique_ptr<AmbientPhotoCache> photo_cache_;
+};
+
+TEST_F(AmbientPhotoCacheTest, ReadsBackWrittenFiles) {
+  int cache_index = 0;
+  std::string image("image");
+  std::string details("details");
+  std::string related_image("related image");
+
+  {
+    base::RunLoop loop;
+    photo_cache()->WriteFiles(cache_index, &image, &details, &related_image,
+                              loop.QuitClosure());
+    loop.Run();
+  }
+
+  {
+    base::RunLoop loop;
+    // Read the files back using photo cache.
+    photo_cache()->ReadFiles(
+        cache_index,
+        base::BindOnce(
+            [](base::OnceClosure done, PhotoCacheEntry cache_read) {
+              EXPECT_EQ(*cache_read.image, "image");
+              EXPECT_EQ(*cache_read.details, "details");
+              EXPECT_EQ(*cache_read.related_image, "related image");
+              std::move(done).Run();
+            },
+            loop.QuitClosure()));
+    loop.Run();
+  }
+}
+
+TEST_F(AmbientPhotoCacheTest, WritesFileToDisk) {
+  base::FilePath test_path = GetTestPath();
+
+  int cache_index = 5;
+  std::string image("image 5");
+  std::string details("details 5");
+  std::string related_image("related image 5");
+
+  // Make sure files are not on disk.
+  EXPECT_FALSE(base::PathExists(test_path.Append(FILE_PATH_LITERAL("5.img"))));
+  EXPECT_FALSE(base::PathExists(test_path.Append(FILE_PATH_LITERAL("5.txt"))));
+  EXPECT_FALSE(
+      base::PathExists(test_path.Append(FILE_PATH_LITERAL("5_r.img"))));
+
+  // Write the data to the cache.
+  {
+    base::RunLoop loop;
+    photo_cache()->WriteFiles(cache_index, &image, &details, &related_image,
+                              loop.QuitClosure());
+    loop.Run();
+  }
+
+  // Verify that expected files are written to disk.
+  std::string actual_image;
+  EXPECT_TRUE(base::ReadFileToString(
+      test_path.Append(FILE_PATH_LITERAL("5.img")), &actual_image));
+  EXPECT_EQ(actual_image, image);
+
+  std::string actual_details;
+  EXPECT_TRUE(base::ReadFileToString(
+      test_path.Append(FILE_PATH_LITERAL("5.txt")), &actual_details));
+  EXPECT_EQ(actual_details, details);
+
+  std::string actual_related_image;
+  EXPECT_TRUE(base::ReadFileToString(
+      test_path.Append(FILE_PATH_LITERAL("5_r.img")), &actual_related_image));
+  EXPECT_EQ(actual_related_image, related_image);
+}
+
+TEST_F(AmbientPhotoCacheTest, SetsDataToEmptyStringWhenFilesMissing) {
+  base::FilePath test_path = GetTestPath();
+  EXPECT_FALSE(base::DirectoryExists(test_path));
+  {
+    base::RunLoop loop;
+    photo_cache()->ReadFiles(
+        /*cache_index=*/1,
+        base::BindOnce(
+            [](base::OnceClosure done, PhotoCacheEntry cache_read) {
+              EXPECT_TRUE(cache_read.image->empty());
+              EXPECT_TRUE(cache_read.details->empty());
+              EXPECT_TRUE(cache_read.related_image->empty());
+              std::move(done).Run();
+            },
+            loop.QuitClosure()));
+  }
+}
+
+}  // namespace ash

ash/ambient/ambient_photo_controller.h

@@ -78,6 +78,8 @@ class ASH_EXPORT AmbientPhotoController : public AmbientBackendModelObserver {
   // Clear cache when Settings changes.
   void ClearCache();
 
+  void InitCache();
+
  private:
   friend class AmbientAshTestBase;
 
@@ -89,13 +91,10 @@ class ASH_EXPORT AmbientPhotoController : public AmbientBackendModelObserver {
 
   void ScheduleRefreshImage();
 
-  // Create the backup cache directory and start downloading images.
-  void PrepareFetchBackupImages();
-
   // Download backup cache images.
   void FetchBackupImages();
 
-  void OnBackupImageFetched(base::FilePath file_path);
+  void OnBackupImageFetched(bool success);
 
   void GetScreenUpdateInfo();
 
@@ -114,13 +113,17 @@ class ASH_EXPORT AmbientPhotoController : public AmbientBackendModelObserver {
   // Try to read photo raw data from cache.
   void TryReadPhotoRawData();
 
-  void OnPhotoRawDataAvailable(bool from_downloading,
-                               bool is_related_image,
-                               base::RepeatingClosure on_done,
-                               std::unique_ptr<std::string> details,
-                               std::unique_ptr<std::string> data);
+  void OnPhotoRawDataDownloaded(bool is_related_image,
+                                base::RepeatingClosure on_done,
+                                std::unique_ptr<std::string> details,
+                                std::unique_ptr<std::string> data);
+
+  void OnAllPhotoRawDataDownloaded();
 
-  void OnAllPhotoRawDataAvailable(bool from_downloading);
+  void OnAllPhotoRawDataAvailable(bool from_downloading,
+                                  PhotoCacheEntry cache_entry);
+
+  void OnPhotoRawDataSaved(bool from_downloading, PhotoCacheEntry cache_entry);
 
   void DecodePhotoRawData(bool from_downloading,
                           bool is_related_image,
@@ -152,6 +155,15 @@ class ASH_EXPORT AmbientPhotoController : public AmbientBackendModelObserver {
     return photo_cache_.get();
   }
 
+  void set_backup_photo_cache_for_testing(
+      std::unique_ptr<AmbientPhotoCache> photo_cache) {
+    backup_photo_cache_ = std::move(photo_cache);
+  }
+
+  AmbientPhotoCache* get_backup_photo_cache_for_testing() {
+    return backup_photo_cache_.get();
+  }
+
   void FetchTopicsForTesting();
 
   void FetchImageForTesting();
@@ -206,13 +218,12 @@ class ASH_EXPORT AmbientPhotoController : public AmbientBackendModelObserver {
       ambient_backend_model_observer_{this};
 
   std::unique_ptr<AmbientPhotoCache> photo_cache_;
+  std::unique_ptr<AmbientPhotoCache> backup_photo_cache_;
 
   scoped_refptr<base::SequencedTaskRunner> task_runner_;
 
   // Temporary data store when fetching images and details.
-  std::unique_ptr<std::string> image_data_;
-  std::unique_ptr<std::string> related_image_data_;
-  std::unique_ptr<std::string> image_details_;
+  PhotoCacheEntry cache_entry_;
   gfx::ImageSkia image_;
   gfx::ImageSkia related_image_;
 

ash/ambient/test/ambient_ash_test_base.cc

@@ -4,12 +4,14 @@
 
 #include "ash/ambient/test/ambient_ash_test_base.h"
 
+#include <map>
 #include <memory>
 #include <utility>
 #include <vector>
 
 #include "ash/ambient/ambient_access_token_controller.h"
 #include "ash/ambient/ambient_constants.h"
+#include "ash/ambient/ambient_photo_cache.h"
 #include "ash/ambient/ambient_photo_controller.h"
 #include "ash/ambient/test/ambient_ash_test_helper.h"
 #include "ash/ambient/ui/ambient_background_image_view.h"
@@ -24,9 +26,7 @@
 #include "ash/session/session_controller_impl.h"
 #include "ash/shell.h"
 #include "base/callback.h"
-#include "base/files/file_util.h"
 #include "base/memory/ptr_util.h"
-#include "base/notreached.h"
 #include "base/run_loop.h"
 #include "base/sequenced_task_runner.h"
 #include "base/threading/scoped_blocking_call.h"
@@ -35,7 +35,6 @@
 #include "chromeos/dbus/power/fake_power_manager_client.h"
 #include "chromeos/dbus/power/power_manager_client.h"
 #include "chromeos/dbus/power_manager/idle.pb.h"
-#include "ui/display/screen.h"
 #include "ui/gfx/image/image_skia.h"
 #include "ui/gfx/image/image_unittest_util.h"
 #include "ui/views/controls/label.h"
@@ -66,26 +65,30 @@ class TestAmbientPhotoCacheImpl : public AmbientPhotoCache {
         FROM_HERE, base::BindOnce(std::move(callback), std::move(data)),
         base::TimeDelta::FromMilliseconds(1));
   }
+
   void DownloadPhotoToFile(const std::string& url,
-                           base::OnceCallback<void(base::FilePath)> callback,
-                           const base::FilePath& file_path) override {
+                           int cache_index,
+                           bool is_related,
+                           base::OnceCallback<void(bool)> callback) override {
     if (!download_data_) {
       base::SequencedTaskRunnerHandle::Get()->PostTask(
-          FROM_HERE, base::BindOnce(std::move(callback), base::FilePath()));
+          FROM_HERE, base::BindOnce(std::move(callback), /*success=*/false));
       return;
     }
 
-    if (!WriteFile(file_path, *download_data_)) {
-      LOG(WARNING) << "error writing file to file_path: " << file_path;
-
-      base::SequencedTaskRunnerHandle::Get()->PostTask(
-          FROM_HERE, base::BindOnce(std::move(callback), base::FilePath()));
-      return;
-    }
+    files_.insert(std::pair<int, PhotoCacheEntry>(
+        cache_index,
+        PhotoCacheEntry(
+            is_related ? nullptr
+                       : std::make_unique<std::string>(*download_data_),
+            /*details=*/nullptr,
+            is_related ? std::make_unique<std::string>(*download_data_)
+                       : nullptr)));
 
     base::SequencedTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::BindOnce(std::move(callback), file_path));
+        FROM_HERE, base::BindOnce(std::move(callback), /*success=*/true));
   }
+
   void DecodePhoto(
       std::unique_ptr<std::string> data,
       base::OnceCallback<void(const gfx::ImageSkia&)> callback) override {
@@ -101,6 +104,47 @@ class TestAmbientPhotoCacheImpl : public AmbientPhotoCache {
         FROM_HERE, base::BindOnce(std::move(callback), image));
   }
 
+  void WriteFiles(int cache_index,
+                  const std::string* const image,
+                  const std::string* const details,
+                  const std::string* const related_image,
+                  base::OnceClosure callback) override {
+    files_.insert(std::pair<int, PhotoCacheEntry>(
+        cache_index,
+        PhotoCacheEntry(
+            image ? std::make_unique<std::string>(*image) : nullptr,
+            details ? std::make_unique<std::string>(*details) : nullptr,
+            related_image ? std::make_unique<std::string>(*related_image)
+                          : nullptr)));
+    std::move(callback).Run();
+  }
+
+  void ReadFiles(int cache_index,
+                 base::OnceCallback<void(PhotoCacheEntry)> callback) override {
+    auto it = files_.find(cache_index);
+    if (it == files_.end()) {
+      std::move(callback).Run(PhotoCacheEntry());
+      return;
+    }
+
+    std::move(callback).Run(PhotoCacheEntry(
+        it->second.image ? std::make_unique<std::string>(*(it->second.image))
+                         : nullptr,
+        it->second.details
+            ? std::make_unique<std::string>(*(it->second.details))
+            : nullptr,
+        it->second.related_image
+            ? std::make_unique<std::string>(*(it->second.related_image))
+            : nullptr));
+  }
+  void Clear() override {
+    download_count_ = 0;
+    download_data_.reset();
+    decoded_size_ = gfx::Size(10, 20);
+    decoded_image_.reset();
+    files_.clear();
+  }
+
   void SetDownloadData(std::unique_ptr<std::string> download_data) {
     download_data_ = std::move(download_data);
   }
@@ -112,6 +156,8 @@ class TestAmbientPhotoCacheImpl : public AmbientPhotoCache {
 
   void SetDecodedPhoto(const gfx::ImageSkia& image) { decoded_image_ = image; }
 
+  const std::map<int, PhotoCacheEntry>& get_files() { return files_; }
+
  private:
   int download_count_ = 0;
 
@@ -122,6 +168,8 @@ class TestAmbientPhotoCacheImpl : public AmbientPhotoCache {
   gfx::Size decoded_size_{10, 20};
   // If set, will replay this image.
   base::Optional<gfx::ImageSkia> decoded_image_;
+
+  std::map<int, PhotoCacheEntry> files_;
 };
 
 AmbientAshTestBase::AmbientAshTestBase()
@@ -139,6 +187,8 @@ void AmbientAshTestBase::SetUp() {
       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);
@@ -380,6 +430,21 @@ base::TimeDelta AmbientAshTestBase::GetRefreshTokenDelay() {
   return token_controller()->GetTimeUntilReleaseForTesting();
 }
 
+const std::map<int, PhotoCacheEntry>& AmbientAshTestBase::GetCachedFiles() {
+  auto* photo_cache = static_cast<TestAmbientPhotoCacheImpl*>(
+      photo_controller()->get_photo_cache_for_testing());
+
+  return photo_cache->get_files();
+}
+
+const std::map<int, PhotoCacheEntry>&
+AmbientAshTestBase::GetBackupCachedFiles() {
+  auto* photo_cache = static_cast<TestAmbientPhotoCacheImpl*>(
+      photo_controller()->get_backup_photo_cache_for_testing());
+
+  return photo_cache->get_files();
+}
+
 AmbientController* AmbientAshTestBase::ambient_controller() {
   return Shell::Get()->ambient_controller();
 }
@@ -388,6 +453,10 @@ AmbientPhotoController* AmbientAshTestBase::photo_controller() {
   return ambient_controller()->ambient_photo_controller();
 }
 
+AmbientPhotoCache* AmbientAshTestBase::photo_cache() {
+  return photo_controller()->get_photo_cache_for_testing();
+}
+
 std::vector<AmbientContainerView*> AmbientAshTestBase::GetContainerViews() {
   std::vector<AmbientContainerView*> result;
   for (auto* ctrl : RootWindowController::root_window_controllers()) {
@@ -449,6 +518,20 @@ void AmbientAshTestBase::ClearDownloadPhotoData() {
   photo_cache->SetDownloadData(nullptr);
 }
 
+void AmbientAshTestBase::SetBackupDownloadPhotoData(std::string data) {
+  auto* backup_cache = static_cast<TestAmbientPhotoCacheImpl*>(
+      photo_controller()->get_backup_photo_cache_for_testing());
+
+  backup_cache->SetDownloadData(std::make_unique<std::string>(std::move(data)));
+}
+
+void AmbientAshTestBase::ClearBackupDownloadPhotoData() {
+  auto* backup_cache = static_cast<TestAmbientPhotoCacheImpl*>(
+      photo_controller()->get_backup_photo_cache_for_testing());
+
+  backup_cache->SetDownloadData(nullptr);
+}
+
 void AmbientAshTestBase::SetDecodePhotoImage(const gfx::ImageSkia& image) {
   auto* photo_cache = static_cast<TestAmbientPhotoCacheImpl*>(
       photo_controller()->get_photo_cache_for_testing());

ash/ambient/test/ambient_ash_test_base.h

@@ -137,10 +137,15 @@ class AmbientAshTestBase : public AshTestBase {
   // Returns the media string view for the default display.
   MediaStringView* GetMediaStringView();
 
+  const std::map<int, PhotoCacheEntry>& GetCachedFiles();
+  const std::map<int, PhotoCacheEntry>& GetBackupCachedFiles();
+
   AmbientController* ambient_controller();
 
   AmbientPhotoController* photo_controller();
 
+  AmbientPhotoCache* photo_cache();
+
   // Returns the top-level views which contains all the ambient components.
   std::vector<AmbientContainerView*> GetContainerViews();
   // Returns the top level ambient container view for the primary root window.
@@ -160,6 +165,10 @@ class AmbientAshTestBase : public AshTestBase {
 
   void ClearDownloadPhotoData();
 
+  void SetBackupDownloadPhotoData(std::string data);
+
+  void ClearBackupDownloadPhotoData();
+
   void SetDecodePhotoImage(const gfx::ImageSkia& image);
 
  private: