Keep track of the list of currently mounted volumes in C++-side VolumeManager.

For the detail, refer the associated bug tracker. In short,
I want to do this for implementing decision procedure for whether
or not to mount zip archives based on originating profiles.

Besides, I believe the new code is more clean--the old code that
constructs the volume list every time FindVolumeById called is skewed.

BUG=343038

Review URL: https://codereview.chromium.org/160483004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@251013 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/chromeos/extensions/file_manager/file_browser_private_apitest.cc

@@ -128,10 +128,13 @@ class FileBrowserPrivateApiTest : public ExtensionApiTest {
         disk_mount_manager_mock_);
     disk_mount_manager_mock_->SetupDefaultReplies();
 
-    // OVERRIDE FindDiskBySourcePath mock function.
-    ON_CALL(*disk_mount_manager_mock_, FindDiskBySourcePath(_)).
-        WillByDefault(Invoke(
-            this, &FileBrowserPrivateApiTest::FindVolumeBySourcePath));
+    // OVERRIDE mock functions.
+    ON_CALL(*disk_mount_manager_mock_, FindDiskBySourcePath(_)).WillByDefault(
+        Invoke(this, &FileBrowserPrivateApiTest::FindVolumeBySourcePath));
+    EXPECT_CALL(*disk_mount_manager_mock_, disks())
+        .WillRepeatedly(ReturnRef(volumes_));
+    EXPECT_CALL(*disk_mount_manager_mock_, mount_points())
+        .WillRepeatedly(ReturnRef(mount_points_));
   }
 
   // ExtensionApiTest override
@@ -248,12 +251,6 @@ IN_PROC_BROWSER_TEST_F(FileBrowserPrivateApiTest, Mount) {
                       "archive_mount_path").AsUTF8Unsafe(),
                   chromeos::UNMOUNT_OPTIONS_NONE, _)).Times(1);
 
-  EXPECT_CALL(*disk_mount_manager_mock_, disks())
-      .WillRepeatedly(ReturnRef(volumes_));
-
-  EXPECT_CALL(*disk_mount_manager_mock_, mount_points())
-      .WillRepeatedly(ReturnRef(mount_points_));
-
   ASSERT_TRUE(RunComponentExtensionTest("file_browser/mount_test"))
       << message_;
 }

chrome/browser/chromeos/file_manager/volume_manager.cc

@@ -229,18 +229,42 @@ void VolumeManager::Initialize() {
 
   // Register 'Downloads' folder for the profile to the file system.
   if (!chromeos::ProfileHelper::IsSigninProfile(profile_)) {
-    bool success = RegisterDownloadsMountPoint(
-        profile_,
-        file_manager::util::GetDownloadsFolderForProfile(profile_));
+    const base::FilePath downloads =
+        file_manager::util::GetDownloadsFolderForProfile(profile_);
+    const bool success = RegisterDownloadsMountPoint(profile_, downloads);
     DCHECK(success);
+
+    DoMountEvent(chromeos::MOUNT_ERROR_NONE,
+                 CreateDownloadsVolumeInfo(downloads),
+                 false /* is_remounting */);
   }
 
   // Subscribe to DriveIntegrationService.
-  if (drive_integration_service_)
+  if (drive_integration_service_) {
     drive_integration_service_->AddObserver(this);
+    if (drive_integration_service_->IsMounted()) {
+      DoMountEvent(
+          chromeos::MOUNT_ERROR_NONE, CreateDriveVolumeInfo(profile_), false);
+    }
+  }
 
   // Subscribe to DiskMountManager.
   disk_mount_manager_->AddObserver(this);
+
+  const chromeos::disks::DiskMountManager::MountPointMap& mount_points =
+      disk_mount_manager_->mount_points();
+  for (chromeos::disks::DiskMountManager::MountPointMap::const_iterator it =
+           mount_points.begin();
+       it != mount_points.end();
+       ++it) {
+    DoMountEvent(
+        chromeos::MOUNT_ERROR_NONE,
+        CreateVolumeInfoFromMountPointInfo(
+            it->second,
+            disk_mount_manager_->FindDiskBySourcePath(it->second.source_path)),
+        false /* is_remounting */);
+  }
+
   disk_mount_manager_->RequestMountInfoRefresh();
 
   // Subscribe to Profile Preference change.
@@ -283,39 +307,12 @@ std::vector<VolumeInfo> VolumeManager::GetVolumeInfoList() const {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 
   std::vector<VolumeInfo> result;
-
-  // Adds "Drive" volume.
-  if (drive_integration_service_ && drive_integration_service_->IsMounted())
-    result.push_back(CreateDriveVolumeInfo(profile_));
-
-  // Adds "Downloads".
-  // Usually, the path of the directory is where we registered in Initialize(),
-  // but in tests, the mount point may be overridden. To take it into account,
-  // here we explicitly retrieves the path from the file API mount points.
-  base::FilePath downloads;
-  if (FindDownloadsMountPointPath(profile_, &downloads))
-    result.push_back(CreateDownloadsVolumeInfo(downloads));
-
-  // Adds disks (both removable disks and zip archives).
-  const chromeos::disks::DiskMountManager::MountPointMap& mount_points =
-      disk_mount_manager_->mount_points();
-  for (chromeos::disks::DiskMountManager::MountPointMap::const_iterator it =
-           mount_points.begin();
-       it != mount_points.end(); ++it) {
-    result.push_back(CreateVolumeInfoFromMountPointInfo(
-        it->second,
-        disk_mount_manager_->FindDiskBySourcePath(it->second.source_path)));
+  for (std::map<std::string, VolumeInfo>::const_iterator iter =
+           mounted_volumes_.begin();
+       iter != mounted_volumes_.end();
+       ++iter) {
+    result.push_back(iter->second);
   }
-
-  if (CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kEnablePrivetStorage)) {
-    for (local_discovery::PrivetVolumeLister::VolumeList::const_iterator i =
-             privet_volume_lister_->volume_list().begin();
-         i != privet_volume_lister_->volume_list().end(); i++) {
-      result.push_back(CreatePrivetVolumeInfo(*i));
-    }
-  }
-
   return result;
 }
 
@@ -324,21 +321,30 @@ bool VolumeManager::FindVolumeInfoById(const std::string& volume_id,
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   DCHECK(result);
 
-  std::vector<VolumeInfo> info_list = GetVolumeInfoList();
-  for (size_t i = 0; i < info_list.size(); ++i) {
-    if (info_list[i].volume_id == volume_id) {
-      *result = info_list[i];
-      return true;
-    }
-  }
-
-  return false;
+  std::map<std::string, VolumeInfo>::const_iterator iter =
+      mounted_volumes_.find(volume_id);
+  if (iter == mounted_volumes_.end())
+    return false;
+  *result = iter->second;
+  return true;
 }
 
 bool VolumeManager::RegisterDownloadsDirectoryForTesting(
     const base::FilePath& path) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
-  return RegisterDownloadsMountPoint(profile_, path);
+
+  base::FilePath old_path;
+  if (FindDownloadsMountPointPath(profile_, &old_path)) {
+    DoUnmountEvent(chromeos::MOUNT_ERROR_NONE,
+                   CreateDownloadsVolumeInfo(old_path));
+  }
+
+  bool success = RegisterDownloadsMountPoint(profile_, path);
+  DoMountEvent(
+      success ? chromeos::MOUNT_ERROR_NONE : chromeos::MOUNT_ERROR_INVALID_PATH,
+      CreateDownloadsVolumeInfo(path),
+      false /* is_remounting */);
+  return success;
 }
 
 void VolumeManager::OnFileSystemMounted() {
@@ -348,19 +354,15 @@ void VolumeManager::OnFileSystemMounted() {
   // We can pass chromeos::MOUNT_ERROR_NONE even when authentication is failed
   // or network is unreachable. These two errors will be handled later.
   VolumeInfo volume_info = CreateDriveVolumeInfo(profile_);
-  FOR_EACH_OBSERVER(VolumeManagerObserver, observers_,
-                    OnVolumeMounted(chromeos::MOUNT_ERROR_NONE,
-                                    volume_info,
-                                    false));  // Not remounting.
+  DoMountEvent(chromeos::MOUNT_ERROR_NONE, volume_info,
+               false /* is_remounting */);
 }
 
 void VolumeManager::OnFileSystemBeingUnmounted() {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 
   VolumeInfo volume_info = CreateDriveVolumeInfo(profile_);
-  FOR_EACH_OBSERVER(
-      VolumeManagerObserver, observers_,
-      OnVolumeUnmounted(chromeos::MOUNT_ERROR_NONE, volume_info));
+  DoUnmountEvent(chromeos::MOUNT_ERROR_NONE, volume_info);
 }
 
 void VolumeManager::OnDiskEvent(
@@ -477,14 +479,11 @@ void VolumeManager::OnMountEvent(
     case chromeos::disks::DiskMountManager::MOUNTING: {
       bool is_remounting =
           disk && mounted_disk_monitor_->DiskIsRemounting(*disk);
-      FOR_EACH_OBSERVER(
-          VolumeManagerObserver, observers_,
-          OnVolumeMounted(error_code, volume_info, is_remounting));
+      DoMountEvent(error_code, volume_info, is_remounting);
       return;
     }
     case chromeos::disks::DiskMountManager::UNMOUNTING:
-      FOR_EACH_OBSERVER(VolumeManagerObserver, observers_,
-                        OnVolumeUnmounted(error_code, volume_info));
+      DoUnmountEvent(error_code, volume_info);
       return;
   }
   NOTREACHED();
@@ -550,11 +549,33 @@ void VolumeManager::OnPrivetVolumesAvailable(
   for (local_discovery::PrivetVolumeLister::VolumeList::const_iterator i =
            volumes.begin(); i != volumes.end(); i++) {
     VolumeInfo volume_info = CreatePrivetVolumeInfo(*i);
-
-    FOR_EACH_OBSERVER(
-          VolumeManagerObserver, observers_,
-          OnVolumeMounted(chromeos::MOUNT_ERROR_NONE, volume_info, false));
+    DoMountEvent(chromeos::MOUNT_ERROR_NONE, volume_info, false);
   }
 }
 
+void VolumeManager::DoMountEvent(chromeos::MountError error_code,
+                                 const VolumeInfo& volume_info,
+                                 bool is_remounting) {
+  // TODO(kinaba): filter zip.
+
+  if (error_code == chromeos::MOUNT_ERROR_NONE || volume_info.mount_condition)
+    mounted_volumes_[volume_info.volume_id] = volume_info;
+
+  FOR_EACH_OBSERVER(VolumeManagerObserver,
+                    observers_,
+                    OnVolumeMounted(error_code, volume_info, is_remounting));
+}
+
+void VolumeManager::DoUnmountEvent(chromeos::MountError error_code,
+                                   const VolumeInfo& volume_info) {
+  if (mounted_volumes_.find(volume_info.volume_id) == mounted_volumes_.end())
+    return;
+  if (error_code == chromeos::MOUNT_ERROR_NONE)
+    mounted_volumes_.erase(volume_info.volume_id);
+
+  FOR_EACH_OBSERVER(VolumeManagerObserver,
+                    observers_,
+                    OnVolumeUnmounted(error_code, volume_info));
+}
+
 }  // namespace file_manager

chrome/browser/chromeos/file_manager/volume_manager.h

@@ -160,6 +160,11 @@ class VolumeManager : public BrowserContextKeyedService,
  private:
   void OnPrivetVolumesAvailable(
       const local_discovery::PrivetVolumeLister::VolumeList& volumes);
+  void DoMountEvent(chromeos::MountError error_code,
+                    const VolumeInfo& volume_info,
+                    bool is_remounting);
+  void DoUnmountEvent(chromeos::MountError error_code,
+                      const VolumeInfo& volume_info);
 
   Profile* profile_;
   drive::DriveIntegrationService* drive_integration_service_;
@@ -168,6 +173,9 @@ class VolumeManager : public BrowserContextKeyedService,
   PrefChangeRegistrar pref_change_registrar_;
   ObserverList<VolumeManagerObserver> observers_;
   scoped_ptr<local_discovery::PrivetVolumeLister> privet_volume_lister_;
+
+  std::map<std::string, VolumeInfo> mounted_volumes_;
+
   DISALLOW_COPY_AND_ASSIGN(VolumeManager);
 };
 

chrome/browser/chromeos/file_manager/volume_manager_unittest.cc

@@ -159,31 +159,24 @@ class VolumeManagerTest : public testing::Test {
   scoped_ptr<VolumeManager> volume_manager_;
 };
 
-TEST_F(VolumeManagerTest, OnFileSystemMounted) {
+TEST_F(VolumeManagerTest, OnDriveFileSystemMountAndUnmount) {
   LoggingObserver observer;
   volume_manager_->AddObserver(&observer);
 
   volume_manager_->OnFileSystemMounted();
 
   ASSERT_EQ(1U, observer.events().size());
-  const LoggingObserver::Event& event = observer.events()[0];
+  LoggingObserver::Event event = observer.events()[0];
   EXPECT_EQ(LoggingObserver::Event::VOLUME_MOUNTED, event.type);
   EXPECT_EQ(drive::util::GetDriveMountPointPath(profile_.get()).AsUTF8Unsafe(),
             event.device_path);
   EXPECT_EQ(chromeos::MOUNT_ERROR_NONE, event.mount_error);
   EXPECT_FALSE(event.is_remounting);
 
-  volume_manager_->RemoveObserver(&observer);
-}
-
-TEST_F(VolumeManagerTest, OnFileSystemBeingUnmounted) {
-  LoggingObserver observer;
-  volume_manager_->AddObserver(&observer);
-
   volume_manager_->OnFileSystemBeingUnmounted();
 
-  ASSERT_EQ(1U, observer.events().size());
-  const LoggingObserver::Event& event = observer.events()[0];
+  ASSERT_EQ(2U, observer.events().size());
+  event = observer.events()[1];
   EXPECT_EQ(LoggingObserver::Event::VOLUME_UNMOUNTED, event.type);
   EXPECT_EQ(drive::util::GetDriveMountPointPath(profile_.get()).AsUTF8Unsafe(),
             event.device_path);
@@ -192,6 +185,16 @@ TEST_F(VolumeManagerTest, OnFileSystemBeingUnmounted) {
   volume_manager_->RemoveObserver(&observer);
 }
 
+TEST_F(VolumeManagerTest, OnDriveFileSystemUnmountWithoutMount) {
+  LoggingObserver observer;
+  volume_manager_->AddObserver(&observer);
+  volume_manager_->OnFileSystemBeingUnmounted();
+
+  // Unmount event for non-mounted volume is not reported.
+  ASSERT_EQ(0U, observer.events().size());
+  volume_manager_->RemoveObserver(&observer);
+}
+
 TEST_F(VolumeManagerTest, OnDiskEvent_Hidden) {
   LoggingObserver observer;
   volume_manager_->AddObserver(&observer);
@@ -435,7 +438,7 @@ TEST_F(VolumeManagerTest, OnDeviceEvent_Scanned) {
   volume_manager_->RemoveObserver(&observer);
 }
 
-TEST_F(VolumeManagerTest, OnMountEvent_Mounting) {
+TEST_F(VolumeManagerTest, OnMountEvent_MountingAndUnmounting) {
   LoggingObserver observer;
   volume_manager_->AddObserver(&observer);
 
@@ -450,12 +453,22 @@ TEST_F(VolumeManagerTest, OnMountEvent_Mounting) {
                                 kMountPoint);
 
   ASSERT_EQ(1U, observer.events().size());
-  const LoggingObserver::Event& event = observer.events()[0];
+  LoggingObserver::Event event = observer.events()[0];
   EXPECT_EQ(LoggingObserver::Event::VOLUME_MOUNTED, event.type);
   EXPECT_EQ("device1", event.device_path);
   EXPECT_EQ(chromeos::MOUNT_ERROR_NONE, event.mount_error);
   EXPECT_FALSE(event.is_remounting);
 
+  volume_manager_->OnMountEvent(chromeos::disks::DiskMountManager::UNMOUNTING,
+                                chromeos::MOUNT_ERROR_NONE,
+                                kMountPoint);
+
+  ASSERT_EQ(2U, observer.events().size());
+  event = observer.events()[1];
+  EXPECT_EQ(LoggingObserver::Event::VOLUME_UNMOUNTED, event.type);
+  EXPECT_EQ("device1", event.device_path);
+  EXPECT_EQ(chromeos::MOUNT_ERROR_NONE, event.mount_error);
+
   volume_manager_->RemoveObserver(&observer);
 }
 
@@ -510,7 +523,7 @@ TEST_F(VolumeManagerTest, OnMountEvent_Remounting) {
   volume_manager_->RemoveObserver(&observer);
 }
 
-TEST_F(VolumeManagerTest, OnMountEvent_Unmounting) {
+TEST_F(VolumeManagerTest, OnMountEvent_UnmountingWithoutMounting) {
   LoggingObserver observer;
   volume_manager_->AddObserver(&observer);
 
@@ -524,11 +537,8 @@ TEST_F(VolumeManagerTest, OnMountEvent_Unmounting) {
                                 chromeos::MOUNT_ERROR_NONE,
                                 kMountPoint);
 
-  ASSERT_EQ(1U, observer.events().size());
-  const LoggingObserver::Event& event = observer.events()[0];
-  EXPECT_EQ(LoggingObserver::Event::VOLUME_UNMOUNTED, event.type);
-  EXPECT_EQ("device1", event.device_path);
-  EXPECT_EQ(chromeos::MOUNT_ERROR_NONE, event.mount_error);
+  // Unmount event for a disk not mounted in this manager is not reported.
+  ASSERT_EQ(0U, observer.events().size());
 
   volume_manager_->RemoveObserver(&observer);
 }

chrome/test/data/extensions/api_test/file_browser/mount_test/test.js

@@ -70,11 +70,11 @@ var expectedArchiveVolume = {
 
 // List of expected mount points.
 // NOTE: this has to be synced with values in file_browser_private_apitest.cc
-//       and values sorted by mountPath.
+//       and values sorted by volumeId.
 var expectedVolumeList = [
-  expectedDriveVolume,
-  expectedDownloadsVolume,
   expectedArchiveVolume,
+  expectedDownloadsVolume,
+  expectedDriveVolume,
   expectedVolume1,
   expectedVolume2,
   expectedVolume3,
@@ -126,7 +126,7 @@ chrome.test.runTests([
   function getVolumeMetadataList() {
     chrome.fileBrowserPrivate.getVolumeMetadataList(
         chrome.test.callbackPass(function(result) {
-          chrome.test.assertEq(result.length, expectedVolumeList.length,
+          chrome.test.assertEq(expectedVolumeList.length, result.length,
               'getMountPoints returned wrong number of mount points.');
           for (var i = 0; i < expectedVolumeList.length; i++) {
             chrome.test.assertTrue(