Wire up copy and move events to chromeos::FileChangeService.

This is part two of a series of CLs that will expose file change events
to observers of a new browser context keyed service.

In this CL, the new service is notified of copy and move events
originating from Chrome OS specific file system operations which are
created by the Chrome OS file system backend.

This involved creation of a new chromeos::FileSystemOperation class
serving as a thin extension to the default file system operation impl.
This thin extension allows changes to storage// code to be minimal.

Additional file change events will be wired up in a follow up CL
following the same pattern.

Design: http://shortn/_xrD1GHuIf1

Bug: 1136173
Change-Id: I9b906b309a1fd6b3d3f5b189c683ae6ffdd65f38
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2551636
Commit-Queue: David Black <dmblack@google.com>
Reviewed-by: Toni Baržić <tbarzic@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Sergei Datsenko <dats@chromium.org>
Cr-Commit-Position: refs/heads/master@{#835046}

chrome/browser/chrome_content_browser_client.cc

@@ -3721,6 +3721,7 @@ void ChromeContentBrowserClient::GetAdditionalFileSystemBackends(
       content::BrowserContext::GetMountPoints(browser_context);
   DCHECK(external_mount_points);
   auto backend = std::make_unique<chromeos::FileSystemBackend>(
+      Profile::FromBrowserContext(browser_context),
       std::make_unique<chromeos::file_system_provider::BackendDelegate>(),
       std::make_unique<chromeos::MTPFileSystemBackendDelegate>(
           storage_partition_path),

chrome/browser/chromeos/BUILD.gn

@@ -1405,13 +1405,16 @@ source_set("chromeos") {
     "fileapi/file_change_service.h",
     "fileapi/file_change_service_factory.cc",
     "fileapi/file_change_service_factory.h",
+    "fileapi/file_change_service_observer.h",
     "fileapi/file_system_backend.cc",
     "fileapi/file_system_backend.h",
     "fileapi/file_system_backend_delegate.h",
     "fileapi/mtp_file_system_backend_delegate.cc",
     "fileapi/mtp_file_system_backend_delegate.h",
     "fileapi/mtp_watcher_manager.cc",
     "fileapi/mtp_watcher_manager.h",
+    "fileapi/observable_file_system_operation_impl.cc",
+    "fileapi/observable_file_system_operation_impl.h",
     "fileapi/recent_arc_media_source.cc",
     "fileapi/recent_arc_media_source.h",
     "fileapi/recent_disk_source.cc",

chrome/browser/chromeos/fileapi/file_change_service.cc

@@ -8,47 +8,26 @@ namespace chromeos {
 
 FileChangeService::FileChangeService() = default;
 
-FileChangeService::~FileChangeService() {
-  DCHECK(!observer_list_.might_have_observers());
-}
+FileChangeService::~FileChangeService() = default;
 
-void FileChangeService::AddObserver(storage::FileChangeObserver* observer) {
+void FileChangeService::AddObserver(FileChangeServiceObserver* observer) {
   observer_list_.AddObserver(observer);
 }
 
-void FileChangeService::RemoveObserver(storage::FileChangeObserver* observer) {
+void FileChangeService::RemoveObserver(FileChangeServiceObserver* observer) {
   observer_list_.RemoveObserver(observer);
 }
 
-void FileChangeService::OnCreateFile(const storage::FileSystemURL& url) {
-  for (storage::FileChangeObserver& observer : observer_list_)
-    observer.OnCreateFile(url);
-}
-
-void FileChangeService::OnCreateFileFrom(const storage::FileSystemURL& url,
-                                         const storage::FileSystemURL& src) {
-  for (storage::FileChangeObserver& observer : observer_list_)
-    observer.OnCreateFileFrom(url, src);
-}
-
-void FileChangeService::OnRemoveFile(const storage::FileSystemURL& url) {
-  for (storage::FileChangeObserver& observer : observer_list_)
-    observer.OnRemoveFile(url);
-}
-
-void FileChangeService::OnModifyFile(const storage::FileSystemURL& url) {
-  for (storage::FileChangeObserver& observer : observer_list_)
-    observer.OnModifyFile(url);
-}
-
-void FileChangeService::OnCreateDirectory(const storage::FileSystemURL& url) {
-  for (storage::FileChangeObserver& observer : observer_list_)
-    observer.OnCreateDirectory(url);
+void FileChangeService::NotifyFileCopied(const storage::FileSystemURL& src,
+                                         const storage::FileSystemURL& dst) {
+  for (FileChangeServiceObserver& observer : observer_list_)
+    observer.OnFileCopied(src, dst);
 }
 
-void FileChangeService::OnRemoveDirectory(const storage::FileSystemURL& url) {
-  for (storage::FileChangeObserver& observer : observer_list_)
-    observer.OnRemoveDirectory(url);
+void FileChangeService::NotifyFileMoved(const storage::FileSystemURL& src,
+                                        const storage::FileSystemURL& dst) {
+  for (FileChangeServiceObserver& observer : observer_list_)
+    observer.OnFileMoved(src, dst);
 }
 
 }  // namespace chromeos

chrome/browser/chromeos/fileapi/file_change_service.h

@@ -6,37 +6,35 @@
 #define CHROME_BROWSER_CHROMEOS_FILEAPI_FILE_CHANGE_SERVICE_H_
 
 #include "base/observer_list.h"
+#include "chrome/browser/chromeos/fileapi/file_change_service_observer.h"
 #include "components/keyed_service/core/keyed_service.h"
-#include "storage/browser/file_system/file_observers.h"
 
 namespace chromeos {
 
 // A service which notifies observers of file change events from external file
 // systems. This serves as a bridge to allow for observation of file system
 // changes across all file system contexts within a browser context.
-class FileChangeService : public KeyedService,
-                          public storage::FileChangeObserver {
+class FileChangeService : public KeyedService {
  public:
   FileChangeService();
   FileChangeService(const FileChangeService& other) = delete;
   FileChangeService& operator=(const FileChangeService& other) = delete;
   ~FileChangeService() override;
 
-  // Adds/removes the specified `observer` for file change events.
-  void AddObserver(storage::FileChangeObserver* observer);
-  void RemoveObserver(storage::FileChangeObserver* observer);
+  // Adds/removes the specified `observer` for service events.
+  void AddObserver(FileChangeServiceObserver* observer);
+  void RemoveObserver(FileChangeServiceObserver* observer);
+
+  // Notifies the service that a file has been copied from `src` to `dst`.
+  void NotifyFileCopied(const storage::FileSystemURL& src,
+                        const storage::FileSystemURL& dst);
+
+  // Notifies the service that a file has been moved from `src` to `dst`.
+  void NotifyFileMoved(const storage::FileSystemURL& src,
+                       const storage::FileSystemURL& dst);
 
  private:
-  // storage::FileChangeObserver:
-  void OnCreateFile(const storage::FileSystemURL& url) override;
-  void OnCreateFileFrom(const storage::FileSystemURL& url,
-                        const storage::FileSystemURL& src) override;
-  void OnRemoveFile(const storage::FileSystemURL& url) override;
-  void OnModifyFile(const storage::FileSystemURL& url) override;
-  void OnCreateDirectory(const storage::FileSystemURL& url) override;
-  void OnRemoveDirectory(const storage::FileSystemURL& url) override;
-
-  base::ObserverList<storage::FileChangeObserver>::Unchecked observer_list_;
+  base::ObserverList<FileChangeServiceObserver> observer_list_;
 };
 
 }  // namespace chromeos

chrome/browser/chromeos/fileapi/file_change_service_observer.h

@@ -0,0 +1,30 @@
+// 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.
+
+#ifndef CHROME_BROWSER_CHROMEOS_FILEAPI_FILE_CHANGE_SERVICE_OBSERVER_H_
+#define CHROME_BROWSER_CHROMEOS_FILEAPI_FILE_CHANGE_SERVICE_OBSERVER_H_
+
+#include "base/observer_list_types.h"
+
+namespace storage {
+class FileSystemURL;
+}  // namespace storage
+
+namespace chromeos {
+
+// An interface for an observer which receives `FileChangeService` events.
+class FileChangeServiceObserver : public base::CheckedObserver {
+ public:
+  // Invoked when a file has been copied from `src` to `dst`.
+  virtual void OnFileCopied(const storage::FileSystemURL& src,
+                            const storage::FileSystemURL& dst) {}
+
+  // Invoked when a file has been moved from `src` to `dst`.
+  virtual void OnFileMoved(const storage::FileSystemURL& src,
+                           const storage::FileSystemURL& dst) {}
+};
+
+}  // namespace chromeos
+
+#endif  // CHROME_BROWSER_CHROMEOS_FILEAPI_FILE_CHANGE_SERVICE_OBSERVER_H_

chrome/browser/chromeos/fileapi/file_change_service_unittest.cc

@@ -4,14 +4,142 @@
 
 #include "chrome/browser/chromeos/fileapi/file_change_service.h"
 
+#include "base/files/scoped_temp_dir.h"
+#include "base/scoped_observation.h"
+#include "base/unguessable_token.h"
+#include "chrome/browser/chromeos/file_manager/app_id.h"
+#include "chrome/browser/chromeos/file_manager/fileapi_util.h"
 #include "chrome/browser/chromeos/fileapi/file_change_service_factory.h"
+#include "chrome/browser/chromeos/fileapi/file_change_service_observer.h"
 #include "chrome/test/base/browser_with_test_window_test.h"
 #include "chrome/test/base/testing_profile_manager.h"
+#include "storage/browser/file_system/external_mount_points.h"
+#include "storage/browser/file_system/file_system_operation_runner.h"
+#include "storage/browser/test/async_file_test_helper.h"
+#include "testing/gmock/include/gmock/gmock.h"
 
 namespace chromeos {
 
 namespace {
 
+// Helpers ---------------------------------------------------------------------
+
+// Returns the file system context associated with the specified `profile`.
+storage::FileSystemContext* GetFileSystemContext(Profile* profile) {
+  return file_manager::util::GetFileSystemContextForExtensionId(
+      profile, file_manager::kFileManagerAppId);
+}
+
+// Returns the file system operation runner associated with the specified
+// `profile`.
+storage::FileSystemOperationRunner* GetFileSystemOperationRunner(
+    Profile* profile) {
+  return GetFileSystemContext(profile)->operation_runner();
+}
+
+// MockFileChangeServiceObserver -----------------------------------------------
+
+class MockFileChangeServiceObserver : public FileChangeServiceObserver {
+ public:
+  // FileChangeServiceObserver:
+  MOCK_METHOD(void,
+              OnFileCopied,
+              (const storage::FileSystemURL& src,
+               const storage::FileSystemURL& dst),
+              (override));
+  MOCK_METHOD(void,
+              OnFileMoved,
+              (const storage::FileSystemURL& src,
+               const storage::FileSystemURL& dst),
+              (override));
+};
+
+// TempFileSystem --------------------------------------------------------------
+
+// A class which registers a temporary file system and provides convenient APIs
+// for interacting with that file system.
+class TempFileSystem {
+ public:
+  explicit TempFileSystem(Profile* profile)
+      : profile_(profile), name_(base::UnguessableToken::Create().ToString()) {}
+
+  TempFileSystem(const TempFileSystem&) = delete;
+  TempFileSystem& operator=(const TempFileSystem&) = delete;
+
+  ~TempFileSystem() {
+    storage::ExternalMountPoints::GetSystemInstance()->RevokeFileSystem(name_);
+  }
+
+  // Sets up and registers a temporary file system at `temp_dir_`.
+  void SetUp() {
+    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+
+    ASSERT_TRUE(
+        storage::ExternalMountPoints::GetSystemInstance()->RegisterFileSystem(
+            name_, storage::kFileSystemTypeNativeLocal,
+            storage::FileSystemMountOption(), temp_dir_.GetPath()));
+
+    GetFileSystemContext(profile_)
+        ->external_backend()
+        ->GrantFileAccessToExtension(file_manager::kFileManagerAppId,
+                                     base::FilePath(name_));
+  }
+
+  // Synchronously creates the file specified by `url`.
+  base::File::Error CreateFile(const storage::FileSystemURL& url) {
+    storage::FileSystemContext* context = GetFileSystemContext(profile_);
+    return storage::AsyncFileTestHelper::CreateFile(context, url);
+  }
+
+  // Returns a file system URL for the specified path relative to `temp_dir_`.
+  storage::FileSystemURL CreateFileSystemURL(const std::string& path) {
+    return GetFileSystemContext(profile_)->CreateCrackedFileSystemURL(
+        origin_, storage::kFileSystemTypeNativeLocal,
+        temp_dir_.GetPath().Append(base::FilePath::FromUTF8Unsafe(path)));
+  }
+
+  // Synchronously copies the file specified by `src` to `dst`.
+  base::File::Error CopyFile(const storage::FileSystemURL& src,
+                             const storage::FileSystemURL& dst) {
+    storage::FileSystemContext* context = GetFileSystemContext(profile_);
+    return storage::AsyncFileTestHelper::Copy(context, src, dst);
+  }
+
+  // Synchronously copies the file specified by `src` to `dst` locally.
+  base::File::Error CopyFileLocal(const storage::FileSystemURL& src,
+                                  const storage::FileSystemURL& dst) {
+    storage::FileSystemContext* context = GetFileSystemContext(profile_);
+    return storage::AsyncFileTestHelper::CopyFileLocal(context, src, dst);
+  }
+
+  // Synchronously moves the file specified by `src` to `dst`.
+  base::File::Error MoveFile(const storage::FileSystemURL& src,
+                             const storage::FileSystemURL& dst) {
+    storage::FileSystemContext* context = GetFileSystemContext(profile_);
+    return storage::AsyncFileTestHelper::Move(context, src, dst);
+  }
+
+  // Synchronously moves the file specified by `src` to `dst` locally.
+  base::File::Error MoveFileLocal(const storage::FileSystemURL& src,
+                                  const storage::FileSystemURL& dst) {
+    storage::FileSystemContext* context = GetFileSystemContext(profile_);
+    return storage::AsyncFileTestHelper::MoveFileLocal(context, src, dst);
+  }
+
+  // Synchronously removes the file specified by `url`.
+  base::File::Error RemoveFile(const storage::FileSystemURL& url,
+                               bool recursive = false) {
+    storage::FileSystemContext* context = GetFileSystemContext(profile_);
+    return storage::AsyncFileTestHelper::Remove(context, url, recursive);
+  }
+
+ private:
+  Profile* const profile_;
+  const url::Origin origin_;
+  const std::string name_;
+  base::ScopedTempDir temp_dir_;
+};
+
 // FileChangeServiceTest -------------------------------------------------------
 
 class FileChangeServiceTest : public BrowserWithTestWindowTest {
@@ -58,4 +186,66 @@ TEST_F(FileChangeServiceTest, CreatesServiceInstancesPerProfile) {
   ASSERT_NE(primary_profile_service, secondary_profile_service);
 }
 
+// Verifies `OnFileCopied` events are propagated to observers.
+TEST_F(FileChangeServiceTest, PropagatesOnFileCopiedEvents) {
+  auto* profile = GetProfile();
+  auto* service = FileChangeServiceFactory::GetInstance()->GetService(profile);
+  ASSERT_TRUE(service);
+
+  testing::NiceMock<MockFileChangeServiceObserver> mock_observer;
+  base::ScopedObservation<FileChangeService, FileChangeServiceObserver>
+      scoped_observation{&mock_observer};
+  scoped_observation.Observe(service);
+
+  TempFileSystem temp_file_system(profile);
+  temp_file_system.SetUp();
+
+  storage::FileSystemURL src = temp_file_system.CreateFileSystemURL("src");
+  storage::FileSystemURL dst = temp_file_system.CreateFileSystemURL("dst");
+
+  ASSERT_EQ(temp_file_system.CreateFile(src), base::File::FILE_OK);
+
+  EXPECT_CALL(mock_observer, OnFileCopied)
+      .WillRepeatedly([&](const storage::FileSystemURL& propagated_src,
+                          const storage::FileSystemURL& propagated_dst) {
+        EXPECT_EQ(src, propagated_src);
+        EXPECT_EQ(dst, propagated_dst);
+      });
+
+  ASSERT_EQ(temp_file_system.CopyFile(src, dst), base::File::FILE_OK);
+  ASSERT_EQ(temp_file_system.RemoveFile(dst), base::File::FILE_OK);
+  ASSERT_EQ(temp_file_system.CopyFileLocal(src, dst), base::File::FILE_OK);
+}
+
+// Verifies `OnFileMoved` events are propagated to observers.
+TEST_F(FileChangeServiceTest, PropagatesOnFileMovedEvents) {
+  auto* profile = GetProfile();
+  auto* service = FileChangeServiceFactory::GetInstance()->GetService(profile);
+  ASSERT_TRUE(service);
+
+  testing::NiceMock<MockFileChangeServiceObserver> mock_observer;
+  base::ScopedObservation<FileChangeService, FileChangeServiceObserver>
+      scoped_observation{&mock_observer};
+  scoped_observation.Observe(service);
+
+  TempFileSystem temp_file_system(profile);
+  temp_file_system.SetUp();
+
+  storage::FileSystemURL src = temp_file_system.CreateFileSystemURL("src");
+  storage::FileSystemURL dst = temp_file_system.CreateFileSystemURL("dst");
+
+  ASSERT_EQ(temp_file_system.CreateFile(src), base::File::FILE_OK);
+
+  EXPECT_CALL(mock_observer, OnFileMoved)
+      .WillRepeatedly([&](const storage::FileSystemURL& propagated_src,
+                          const storage::FileSystemURL& propagated_dst) {
+        EXPECT_EQ(src, propagated_src);
+        EXPECT_EQ(dst, propagated_dst);
+      });
+
+  ASSERT_EQ(temp_file_system.MoveFile(src, dst), base::File::FILE_OK);
+  std::swap(dst, src);
+  ASSERT_EQ(temp_file_system.MoveFileLocal(src, dst), base::File::FILE_OK);
+}
+
 }  // namespace chromeos