Simplify DriveFs mounting by using chromeos::disks::MountPoint

Bug: None
Change-Id: Ib4e5288d590321f0911c9c60a583b97291716008
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2007794Reviewed-by: Sergei Datsenko <dats@chromium.org>
Commit-Queue: Anand Mistry <amistry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#733507}

chromeos/components/drivefs/drivefs_host_unittest.cc

@@ -15,6 +15,7 @@
 #include "base/strings/strcat.h"
 #include "base/strings/string_split.h"
 #include "base/test/bind_test_util.h"
+#include "base/test/gmock_callback_support.h"
 #include "base/test/simple_test_clock.h"
 #include "base/test/task_environment.h"
 #include "base/timer/mock_timer.h"
@@ -495,11 +496,16 @@ TEST_F(DriveFsHostTest, OnMountFailedFromDbus) {
 TEST_F(DriveFsHostTest, DestroyBeforeMojoConnection) {
   auto token = StartMount();
   DispatchMountSuccessEvent(token);
-  EXPECT_CALL(*disk_manager_, UnmountPath("/media/drivefsroot/salt-g-ID", _));
+
+  base::RunLoop run_loop;
+  EXPECT_CALL(*disk_manager_, UnmountPath("/media/drivefsroot/salt-g-ID", _))
+      .WillOnce(base::test::RunClosure(run_loop.QuitClosure()));
 
   host_.reset();
   EXPECT_FALSE(mojo_bootstrap::PendingConnectionManager::Get().OpenIpcChannel(
       token, {}));
+
+  run_loop.Run();
 }
 
 TEST_F(DriveFsHostTest, MountWhileAlreadyMounted) {

chromeos/components/drivefs/drivefs_session.cc

@@ -6,8 +6,11 @@
 
 #include <utility>
 
+#include "base/bind.h"
+#include "base/memory/weak_ptr.h"
 #include "base/strings/strcat.h"
 #include "chromeos/components/drivefs/drivefs_bootstrap.h"
+#include "chromeos/disks/mount_point.h"
 
 namespace drivefs {
 
@@ -19,74 +22,60 @@ constexpr char kMyFilesOption[] = "myfiles=";
 constexpr char kMountScheme[] = "drivefs://";
 constexpr base::TimeDelta kMountTimeout = base::TimeDelta::FromSeconds(20);
 
-class DiskMounterImpl : public DiskMounter,
-                        public chromeos::disks::DiskMountManager::Observer {
+class DiskMounterImpl : public DiskMounter {
  public:
   explicit DiskMounterImpl(
       chromeos::disks::DiskMountManager* disk_mount_manager)
       : disk_mount_manager_(disk_mount_manager) {}
 
-  ~DiskMounterImpl() override {
-    disk_mount_manager_->RemoveObserver(this);
-    if (!mount_path_.empty()) {
-      Unmount();
-    }
-  }
+  ~DiskMounterImpl() override = default;
 
   void Mount(const base::UnguessableToken& token,
              const base::FilePath& data_path,
              const base::FilePath& my_files_path,
              const std::string& desired_mount_dir_name,
              base::OnceCallback<void(base::FilePath)> callback) override {
-    DCHECK(mount_path_.empty());
+    DCHECK(!mount_point_);
     DCHECK(callback_.is_null());
     callback_ = std::move(callback);
 
-    disk_mount_manager_->AddObserver(this);
     source_path_ = base::StrCat({kMountScheme, token.ToString()});
     std::string datadir_option =
         base::StrCat({kDataDirOption, data_path.value()});
-    disk_mount_manager_->MountPath(
-        source_path_, "", desired_mount_dir_name,
+
+    chromeos::disks::MountPoint::Mount(
+        disk_mount_manager_, source_path_, "", desired_mount_dir_name,
         {datadir_option, base::StrCat({kMyFilesOption, my_files_path.value()})},
         chromeos::MOUNT_TYPE_NETWORK_STORAGE,
-        chromeos::MOUNT_ACCESS_MODE_READ_WRITE);
+        chromeos::MOUNT_ACCESS_MODE_READ_WRITE,
+        base::BindOnce(&DiskMounterImpl::OnMountDone,
+                       weak_factory_.GetWeakPtr()));
   }
 
  private:
-  void Unmount() {
-    disk_mount_manager_->RemoveObserver(this);
-    if (!mount_path_.empty()) {
-      disk_mount_manager_->UnmountPath(mount_path_.value(), {});
-      mount_path_.clear();
-    }
-  }
-
-  // DiskMountManager::Observer:
-  void OnMountEvent(chromeos::disks::DiskMountManager::MountEvent event,
-                    chromeos::MountError error_code,
-                    const chromeos::disks::DiskMountManager::MountPointInfo&
-                        mount_info) override {
-    if (mount_info.mount_type != chromeos::MOUNT_TYPE_NETWORK_STORAGE ||
-        mount_info.source_path != source_path_ ||
-        event != chromeos::disks::DiskMountManager::MOUNTING) {
+  // MountPoint::Mount() done callback.
+  void OnMountDone(chromeos::MountError error_code,
+                   std::unique_ptr<chromeos::disks::MountPoint> mount_point) {
+    DCHECK(callback_);
+
+    if (error_code != chromeos::MOUNT_ERROR_NONE) {
+      LOG(WARNING) << "DriveFs mount failed with error: " << error_code;
+      std::move(callback_).Run({});
       return;
     }
-    DCHECK(mount_path_.empty());
-    DCHECK(!callback_.is_null());
-    if (error_code == chromeos::MOUNT_ERROR_NONE) {
-      disk_mount_manager_->RemoveObserver(this);
-      DCHECK(!mount_info.mount_path.empty());
-      mount_path_ = base::FilePath(mount_info.mount_path);
-    }
-    std::move(callback_).Run(mount_path_);
+
+    DCHECK(mount_point);
+    mount_point_ = std::move(mount_point);
+    std::move(callback_).Run(mount_point_->mount_path());
   }
 
   chromeos::disks::DiskMountManager* const disk_mount_manager_;
   base::OnceCallback<void(base::FilePath)> callback_;
   // The path passed to cros-disks to mount.
   std::string source_path_;
-  base::FilePath mount_path_;
+  std::unique_ptr<chromeos::disks::MountPoint> mount_point_;
+
+  base::WeakPtrFactory<DiskMounterImpl> weak_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(DiskMounterImpl);
 };

chromeos/components/drivefs/drivefs_session_unittest.cc

@@ -9,6 +9,7 @@
 #include "base/run_loop.h"
 #include "base/strings/strcat.h"
 #include "base/test/bind_test_util.h"
+#include "base/test/gmock_callback_support.h"
 #include "base/test/simple_test_clock.h"
 #include "base/test/task_environment.h"
 #include "base/timer/mock_timer.h"
@@ -66,19 +67,25 @@ class DriveFsDiskMounterTest : public testing::Test {
   }
 
   MOCK_METHOD1(OnCompleted, void(base::FilePath));
+
+  base::test::TaskEnvironment task_environment_;
   chromeos::disks::MockDiskMountManager disk_manager_;
 };
 
 TEST_F(DriveFsDiskMounterTest, MountUnmount) {
   auto mounter = DiskMounter::Create(&disk_manager_);
   auto token = StartMount(mounter.get());
-  EXPECT_CALL(*this, OnCompleted(base::FilePath(kExpectedMountPath)));
+  base::RunLoop run_loop;
+  EXPECT_CALL(*this, OnCompleted(base::FilePath(kExpectedMountPath)))
+      .WillOnce(base::test::RunClosure(run_loop.QuitClosure()));
   DispatchMountEvent(chromeos::disks::DiskMountManager::MOUNTING,
                      chromeos::MOUNT_ERROR_NONE,
                      {base::StrCat({"drivefs://", token}),
                       kExpectedMountPath,
                       chromeos::MOUNT_TYPE_NETWORK_STORAGE,
                       {}});
+  run_loop.Run();
+
   EXPECT_CALL(disk_manager_, UnmountPath(kExpectedMountPath, _));
   mounter.reset();
 }
@@ -86,13 +93,17 @@ TEST_F(DriveFsDiskMounterTest, MountUnmount) {
 TEST_F(DriveFsDiskMounterTest, DestroyAfterMounted) {
   auto mounter = DiskMounter::Create(&disk_manager_);
   auto token = StartMount(mounter.get());
-  EXPECT_CALL(*this, OnCompleted(base::FilePath(kExpectedMountPath)));
+  base::RunLoop run_loop;
+  EXPECT_CALL(*this, OnCompleted(base::FilePath(kExpectedMountPath)))
+      .WillOnce(base::test::RunClosure(run_loop.QuitClosure()));
   DispatchMountEvent(chromeos::disks::DiskMountManager::MOUNTING,
                      chromeos::MOUNT_ERROR_NONE,
                      {base::StrCat({"drivefs://", token}),
                       kExpectedMountPath,
                       chromeos::MOUNT_TYPE_NETWORK_STORAGE,
                       {}});
+  run_loop.Run();
+
   EXPECT_CALL(disk_manager_, UnmountPath(kExpectedMountPath, _));
 }
 
@@ -102,39 +113,21 @@ TEST_F(DriveFsDiskMounterTest, DestroyBeforeMounted) {
   StartMount(mounter.get());
 }
 
-TEST_F(DriveFsDiskMounterTest, ObserveOtherEvents) {
-  EXPECT_CALL(*this, OnCompleted(_)).Times(0);
-  EXPECT_CALL(disk_manager_, UnmountPath(_, _)).Times(0);
-
-  auto mounter = DiskMounter::Create(&disk_manager_);
-  auto token = StartMount(mounter.get());
-
-  DispatchMountEvent(chromeos::disks::DiskMountManager::MOUNTING,
-                     chromeos::MOUNT_ERROR_DIRECTORY_CREATION_FAILED,
-                     {"some/other/mount/event",
-                      "/some/other/mount/point",
-                      chromeos::MOUNT_TYPE_DEVICE,
-                      {}});
-  DispatchMountEvent(chromeos::disks::DiskMountManager::UNMOUNTING,
-                     chromeos::MOUNT_ERROR_NONE,
-                     {base::StrCat({"drivefs://", token}),
-                      kExpectedMountPath,
-                      chromeos::MOUNT_TYPE_NETWORK_STORAGE,
-                      {}});
-}
-
 TEST_F(DriveFsDiskMounterTest, MountError) {
   EXPECT_CALL(disk_manager_, UnmountPath(_, _)).Times(0);
 
   auto mounter = DiskMounter::Create(&disk_manager_);
   auto token = StartMount(mounter.get());
-  EXPECT_CALL(*this, OnCompleted(base::FilePath()));
+  base::RunLoop run_loop;
+  EXPECT_CALL(*this, OnCompleted(base::FilePath()))
+      .WillOnce(base::test::RunClosure(run_loop.QuitClosure()));
   DispatchMountEvent(chromeos::disks::DiskMountManager::MOUNTING,
                      chromeos::MOUNT_ERROR_INVALID_MOUNT_OPTIONS,
                      {base::StrCat({"drivefs://", token}),
                       kExpectedMountPath,
                       chromeos::MOUNT_TYPE_NETWORK_STORAGE,
                       {}});
+  run_loop.Run();
 }
 
 // DiskMountManager sometimes sends mount events for all existing mount points.
@@ -142,7 +135,9 @@ TEST_F(DriveFsDiskMounterTest, MountError) {
 TEST_F(DriveFsDiskMounterTest, MultipleMountNotifications) {
   auto mounter = DiskMounter::Create(&disk_manager_);
   auto token = StartMount(mounter.get());
-  EXPECT_CALL(*this, OnCompleted(base::FilePath(kExpectedMountPath))).Times(1);
+  base::RunLoop run_loop;
+  EXPECT_CALL(*this, OnCompleted(base::FilePath(kExpectedMountPath)))
+      .WillOnce(base::test::RunClosure(run_loop.QuitClosure()));
   DispatchMountEvent(chromeos::disks::DiskMountManager::MOUNTING,
                      chromeos::MOUNT_ERROR_NONE,
                      {base::StrCat({"drivefs://", token}),
@@ -161,6 +156,7 @@ TEST_F(DriveFsDiskMounterTest, MultipleMountNotifications) {
                       kExpectedMountPath,
                       chromeos::MOUNT_TYPE_NETWORK_STORAGE,
                       {}});
+  run_loop.Run();
 }
 
 class MockDiskMounter : public DiskMounter {