Cut reference from MountedDiskMonitor to DiskMountManager.

For number of reasons:

* The previous code implicitly relied on the fact that the Observer
events are notified to MountedDiskMonitor before VolumeManager
(otherwise VolumeManager::OnDiskEvent misses the hard-unplug event.)
It indeed holds, but it is not a part of explicit contract of base::ObserverList.
We should not rely on the detail.

* It simplifies MountedDiskMonitor (see the number of lines.)

* It simplifies the initialization process of VolumeManager/DiskMountManager,
i.e., removes duplicated RequestMountInfoRefresh calls.
This is desired for Bug 356583.

BUG=356583

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

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

chrome/browser/chromeos/file_manager/mounted_disk_monitor.cc

@@ -22,22 +22,16 @@ const base::TimeDelta kResumingTimeSpan = base::TimeDelta::FromSeconds(5);
 }  // namespace
 
 MountedDiskMonitor::MountedDiskMonitor(
-    chromeos::PowerManagerClient* power_manager_client,
-    chromeos::disks::DiskMountManager* disk_mount_manager)
+    chromeos::PowerManagerClient* power_manager_client)
     : power_manager_client_(power_manager_client),
-      disk_mount_manager_(disk_mount_manager),
       is_resuming_(false),
       resuming_time_span_(kResumingTimeSpan),
       weak_factory_(this) {
   DCHECK(power_manager_client_);
-  DCHECK(disk_mount_manager_);
   power_manager_client_->AddObserver(this);
-  disk_mount_manager_->AddObserver(this);
-  disk_mount_manager_->RequestMountInfoRefresh();
 }
 
 MountedDiskMonitor::~MountedDiskMonitor() {
-  disk_mount_manager_->RemoveObserver(this);
   power_manager_client_->RemoveObserver(this);
 }
 
@@ -80,14 +74,13 @@ void MountedDiskMonitor::MarkAsHardUnpluggedReported(
 void MountedDiskMonitor::OnMountEvent(
     chromeos::disks::DiskMountManager::MountEvent event,
     chromeos::MountError error_code,
-    const chromeos::disks::DiskMountManager::MountPointInfo& mount_info) {
+    const chromeos::disks::DiskMountManager::MountPointInfo& mount_info,
+    const DiskMountManager::Disk* disk) {
   if (mount_info.mount_type != chromeos::MOUNT_TYPE_DEVICE)
     return;
 
   switch (event) {
     case DiskMountManager::MOUNTING: {
-      const DiskMountManager::Disk* disk =
-          disk_mount_manager_->FindDiskBySourcePath(mount_info.source_path);
       if (!disk || error_code != chromeos::MOUNT_ERROR_NONE)
         return;
       mounted_disks_[mount_info.source_path] = disk->fs_uuid();
@@ -133,12 +126,6 @@ void MountedDiskMonitor::OnDeviceEvent(
   }
 }
 
-void MountedDiskMonitor::OnFormatEvent(
-    chromeos::disks::DiskMountManager::FormatEvent event,
-    chromeos::FormatError error_code,
-    const std::string& device_path) {
-}
-
 void MountedDiskMonitor::Reset() {
   unmounted_while_resuming_.clear();
   is_resuming_ = false;

chrome/browser/chromeos/file_manager/mounted_disk_monitor.h

@@ -21,35 +21,28 @@ namespace file_manager {
 // couple of seconds. This is to give DiskManager time to process device
 // removed/added events (events for the devices that were present before suspend
 // should not trigger any new notifications or file manager windows).
-class MountedDiskMonitor
-    : public chromeos::PowerManagerClient::Observer,
-      public chromeos::disks::DiskMountManager::Observer {
+class MountedDiskMonitor : public chromeos::PowerManagerClient::Observer {
  public:
-  MountedDiskMonitor(
-      chromeos::PowerManagerClient* power_manager_client,
-      chromeos::disks::DiskMountManager* disk_mount_manager);
+  explicit MountedDiskMonitor(
+      chromeos::PowerManagerClient* power_manager_client);
   virtual ~MountedDiskMonitor();
 
   // PowerManagerClient::Observer overrides:
   virtual void SuspendImminent() OVERRIDE;
   virtual void SuspendDone(const base::TimeDelta& sleep_duration) OVERRIDE;
 
-  // DiskMountManager::Observer overrides.
-  virtual void OnDiskEvent(
+  // Receives forwarded notifications originates from DiskMountManager.
+  void OnDiskEvent(
       chromeos::disks::DiskMountManager::DiskEvent event,
-      const chromeos::disks::DiskMountManager::Disk* disk) OVERRIDE;
-  virtual void OnDeviceEvent(
+      const chromeos::disks::DiskMountManager::Disk* disk);
+  void OnDeviceEvent(
       chromeos::disks::DiskMountManager::DeviceEvent event,
-      const std::string& device_path) OVERRIDE;
-  virtual void OnMountEvent(
+      const std::string& device_path);
+  void OnMountEvent(
       chromeos::disks::DiskMountManager::MountEvent event,
       chromeos::MountError error_code,
-      const chromeos::disks::DiskMountManager::MountPointInfo& mount_info)
-      OVERRIDE;
-  virtual void OnFormatEvent(
-      chromeos::disks::DiskMountManager::FormatEvent event,
-      chromeos::FormatError error_code,
-      const std::string& device_path) OVERRIDE;
+      const chromeos::disks::DiskMountManager::MountPointInfo& mount_info,
+      const chromeos::disks::DiskMountManager::Disk* disk);
 
   // Checks if the disk is being remounted. The disk is remounting if it has
   // been unmounted during the resuming time span.
@@ -77,7 +70,6 @@ class MountedDiskMonitor
   void Reset();
 
   chromeos::PowerManagerClient* power_manager_client_;
-  chromeos::disks::DiskMountManager* disk_mount_manager_;
 
   bool is_resuming_;
   DiskMap mounted_disks_;

chrome/browser/chromeos/file_manager/mounted_disk_monitor_unittest.cc

@@ -7,8 +7,6 @@
 #include "base/basictypes.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
-#include "base/stl_util.h"
-#include "chrome/browser/chromeos/file_manager/fake_disk_mount_manager.h"
 #include "chromeos/dbus/fake_power_manager_client.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -29,25 +27,16 @@ scoped_ptr<chromeos::disks::DiskMountManager::Disk> CreateDisk(
 
 class MountedDiskMonitorTest : public testing::Test {
  protected:
-  MountedDiskMonitorTest() {
-  }
-
-  virtual ~MountedDiskMonitorTest() {
-  }
-
   virtual void SetUp() OVERRIDE {
     power_manager_client_.reset(new chromeos::FakePowerManagerClient);
-    disk_mount_manager_.reset(new FakeDiskMountManager);
     mounted_disk_monitor_.reset(new MountedDiskMonitor(
-        power_manager_client_.get(),
-        disk_mount_manager_.get()));
+        power_manager_client_.get()));
     mounted_disk_monitor_->set_resuming_time_span_for_testing(
         base::TimeDelta::FromSeconds(0));
   }
 
   base::MessageLoop message_loop_;
   scoped_ptr<chromeos::FakePowerManagerClient> power_manager_client_;
-  scoped_ptr<FakeDiskMountManager> disk_mount_manager_;
   scoped_ptr<MountedDiskMonitor> mounted_disk_monitor_;
 };
 
@@ -57,36 +46,35 @@ TEST_F(MountedDiskMonitorTest, WithoutSuspend) {
   scoped_ptr<chromeos::disks::DiskMountManager::Disk> disk(
       CreateDisk("removable_device1", "uuid1"));
 
-  chromeos::disks::DiskMountManager::Disk* disk_ptr = disk.get();
-
   const chromeos::disks::DiskMountManager::MountPointInfo kMountPoint(
       "removable_device1", "/tmp/removable_device1",
       chromeos::MOUNT_TYPE_DEVICE, chromeos::disks::MOUNT_CONDITION_NONE);
 
-  ASSERT_TRUE(disk_mount_manager_->AddDiskForTest(disk.release()));
-
   // First, the disk is not remounting.
-  EXPECT_FALSE(mounted_disk_monitor_->DiskIsRemounting(*disk_ptr));
+  EXPECT_FALSE(mounted_disk_monitor_->DiskIsRemounting(*disk));
 
   // Simple mounting and unmounting doesn't affect remounting state.
   mounted_disk_monitor_->OnMountEvent(
       chromeos::disks::DiskMountManager::MOUNTING,
       chromeos::MOUNT_ERROR_NONE,
-      kMountPoint);
-  EXPECT_FALSE(mounted_disk_monitor_->DiskIsRemounting(*disk_ptr));
+      kMountPoint,
+      disk.get());
+  EXPECT_FALSE(mounted_disk_monitor_->DiskIsRemounting(*disk));
 
   mounted_disk_monitor_->OnMountEvent(
       chromeos::disks::DiskMountManager::UNMOUNTING,
       chromeos::MOUNT_ERROR_NONE,
-      kMountPoint);
-  EXPECT_FALSE(mounted_disk_monitor_->DiskIsRemounting(*disk_ptr));
+      kMountPoint,
+      disk.get());
+  EXPECT_FALSE(mounted_disk_monitor_->DiskIsRemounting(*disk));
 
   // Mounting again also should not affect remounting state.
   mounted_disk_monitor_->OnMountEvent(
       chromeos::disks::DiskMountManager::MOUNTING,
       chromeos::MOUNT_ERROR_NONE,
-      kMountPoint);
-  EXPECT_FALSE(mounted_disk_monitor_->DiskIsRemounting(*disk_ptr));
+      kMountPoint,
+      disk.get());
+  EXPECT_FALSE(mounted_disk_monitor_->DiskIsRemounting(*disk));
 }
 
 // Makes sure that the unmounting after system resuming triggers the
@@ -97,9 +85,6 @@ TEST_F(MountedDiskMonitorTest, SuspendAndResume) {
   scoped_ptr<chromeos::disks::DiskMountManager::Disk> disk2(
       CreateDisk("removable_device2", "uuid2"));
 
-  chromeos::disks::DiskMountManager::Disk* disk1_ptr = disk1.get();
-  chromeos::disks::DiskMountManager::Disk* disk2_ptr = disk2.get();
-
   const chromeos::disks::DiskMountManager::MountPointInfo kMountPoint1(
       "removable_device1", "/tmp/removable_device1",
       chromeos::MOUNT_TYPE_DEVICE, chromeos::disks::MOUNT_CONDITION_NONE);
@@ -107,15 +92,13 @@ TEST_F(MountedDiskMonitorTest, SuspendAndResume) {
       "removable_device2", "/tmp/removable_device2",
       chromeos::MOUNT_TYPE_DEVICE, chromeos::disks::MOUNT_CONDITION_NONE);
 
-  ASSERT_TRUE(disk_mount_manager_->AddDiskForTest(disk1.release()));
-  ASSERT_TRUE(disk_mount_manager_->AddDiskForTest(disk2.release()));
-
   // Mount |disk1|.
   mounted_disk_monitor_->OnMountEvent(
       chromeos::disks::DiskMountManager::MOUNTING,
       chromeos::MOUNT_ERROR_NONE,
-      kMountPoint1);
-  EXPECT_FALSE(mounted_disk_monitor_->DiskIsRemounting(*disk1_ptr));
+      kMountPoint1,
+      disk1.get());
+  EXPECT_FALSE(mounted_disk_monitor_->DiskIsRemounting(*disk1));
 
   // Pseudo system suspend and resume.
   mounted_disk_monitor_->SuspendImminent();
@@ -126,27 +109,30 @@ TEST_F(MountedDiskMonitorTest, SuspendAndResume) {
   mounted_disk_monitor_->OnMountEvent(
       chromeos::disks::DiskMountManager::UNMOUNTING,
       chromeos::MOUNT_ERROR_NONE,
-      kMountPoint1);
-  EXPECT_TRUE(mounted_disk_monitor_->DiskIsRemounting(*disk1_ptr));
+      kMountPoint1,
+      disk1.get());
+  EXPECT_TRUE(mounted_disk_monitor_->DiskIsRemounting(*disk1));
 
   mounted_disk_monitor_->OnMountEvent(
       chromeos::disks::DiskMountManager::MOUNTING,
       chromeos::MOUNT_ERROR_NONE,
-      kMountPoint1);
-  EXPECT_TRUE(mounted_disk_monitor_->DiskIsRemounting(*disk1_ptr));
+      kMountPoint1,
+      disk1.get());
+  EXPECT_TRUE(mounted_disk_monitor_->DiskIsRemounting(*disk1));
 
   // New disk should not be "remounting."
-  EXPECT_FALSE(mounted_disk_monitor_->DiskIsRemounting(*disk2_ptr));
+  EXPECT_FALSE(mounted_disk_monitor_->DiskIsRemounting(*disk2));
   mounted_disk_monitor_->OnMountEvent(
       chromeos::disks::DiskMountManager::MOUNTING,
       chromeos::MOUNT_ERROR_NONE,
-      kMountPoint2);
-  EXPECT_FALSE(mounted_disk_monitor_->DiskIsRemounting(*disk2_ptr));
+      kMountPoint2,
+      disk2.get());
+  EXPECT_FALSE(mounted_disk_monitor_->DiskIsRemounting(*disk2));
 
   // After certain period, remounting state should be cleared.
   base::RunLoop().RunUntilIdle();  // Emulate time passage.
-  EXPECT_FALSE(mounted_disk_monitor_->DiskIsRemounting(*disk1_ptr));
-  EXPECT_FALSE(mounted_disk_monitor_->DiskIsRemounting(*disk2_ptr));
+  EXPECT_FALSE(mounted_disk_monitor_->DiskIsRemounting(*disk1));
+  EXPECT_FALSE(mounted_disk_monitor_->DiskIsRemounting(*disk2));
 }
 
 }  // namespace file_manager

chrome/browser/chromeos/file_manager/volume_manager.cc

@@ -254,8 +254,7 @@ VolumeManager::VolumeManager(
     : profile_(profile),
       drive_integration_service_(drive_integration_service),
       disk_mount_manager_(disk_mount_manager),
-      mounted_disk_monitor_(
-          new MountedDiskMonitor(power_manager_client, disk_mount_manager)),
+      mounted_disk_monitor_(new MountedDiskMonitor(power_manager_client)),
       file_system_provider_service_(file_system_provider_service),
       snapshot_manager_(new SnapshotManager(profile_)),
       weak_ptr_factory_(this) {
@@ -505,6 +504,8 @@ void VolumeManager::OnDiskEvent(
     const chromeos::disks::DiskMountManager::Disk* disk) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
+  mounted_disk_monitor_->OnDiskEvent(event, disk);
+
   // Disregard hidden devices.
   if (disk->is_hidden())
     return;
@@ -565,8 +566,10 @@ void VolumeManager::OnDeviceEvent(
     chromeos::disks::DiskMountManager::DeviceEvent event,
     const std::string& device_path) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  DVLOG(1) << "OnDeviceEvent: " << event << ", " << device_path;
 
+  mounted_disk_monitor_->OnDeviceEvent(event, device_path);
+
+  DVLOG(1) << "OnDeviceEvent: " << event << ", " << device_path;
   switch (event) {
     case chromeos::disks::DiskMountManager::DEVICE_ADDED:
       FOR_EACH_OBSERVER(VolumeManagerObserver, observers_,
@@ -591,6 +594,10 @@ void VolumeManager::OnMountEvent(
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK_NE(chromeos::MOUNT_TYPE_INVALID, mount_info.mount_type);
 
+  const chromeos::disks::DiskMountManager::Disk* disk =
+      disk_mount_manager_->FindDiskBySourcePath(mount_info.source_path);
+  mounted_disk_monitor_->OnMountEvent(event, error_code, mount_info, disk);
+
   if (mount_info.mount_type == chromeos::MOUNT_TYPE_ARCHIVE) {
     // If the file is not mounted now, tell it to drive file system so that
     // it can handle file caching correctly.
@@ -611,8 +618,6 @@ void VolumeManager::OnMountEvent(
   }
 
   // Notify a mounting/unmounting event to observers.
-  const chromeos::disks::DiskMountManager::Disk* disk =
-      disk_mount_manager_->FindDiskBySourcePath(mount_info.source_path);
   VolumeInfo volume_info =
       CreateVolumeInfoFromMountPointInfo(mount_info, disk);
   switch (event) {

chrome/browser/chromeos/file_manager/volume_manager_unittest.cc

@@ -385,11 +385,15 @@ TEST_F(VolumeManagerTest, OnDiskEvent_Removed) {
   volume_manager()->OnDiskEvent(
       chromeos::disks::DiskMountManager::DISK_REMOVED, &kMountedDisk);
 
-  ASSERT_EQ(1U, observer.events().size());
+  ASSERT_EQ(2U, observer.events().size());
   const LoggingObserver::Event& event = observer.events()[0];
   EXPECT_EQ(LoggingObserver::Event::DISK_REMOVED, event.type);
   EXPECT_EQ("device1", event.device_path);
 
+  // Since the Disk has non-empty mount_path, it's regarded as hard unplugging.
+  EXPECT_EQ(LoggingObserver::Event::HARD_UNPLUGGED,
+            observer.events()[1].type);
+
   ASSERT_EQ(1U, disk_mount_manager_->unmount_requests().size());
   const FakeDiskMountManager::UnmountRequest& unmount_request =
       disk_mount_manager_->unmount_requests()[0];
@@ -524,31 +528,38 @@ TEST_F(VolumeManagerTest, OnMountEvent_Remounting) {
   disk_mount_manager_->MountPath(
       "device1", "", "", chromeos::MOUNT_TYPE_DEVICE);
 
+  const chromeos::disks::DiskMountManager::MountPointInfo kMountPoint(
+      "device1",
+      "mount1",
+      chromeos::MOUNT_TYPE_DEVICE,
+      chromeos::disks::MOUNT_CONDITION_NONE);
+
+  volume_manager()->OnMountEvent(
+      chromeos::disks::DiskMountManager::MOUNTING,
+      chromeos::MOUNT_ERROR_NONE,
+      kMountPoint);
+
+  LoggingObserver observer;
+
   // Emulate system suspend and then resume.
   {
     power_manager_client_->SendSuspendImminent();
     power_manager_client_->SendSuspendDone();
 
     // After resume, the device is unmounted and then mounted.
-    disk_mount_manager_->UnmountPath(
-        "device1", chromeos::UNMOUNT_OPTIONS_NONE,
-        chromeos::disks::DiskMountManager::UnmountPathCallback());
-    disk_mount_manager_->MountPath(
-        "device1", "", "", chromeos::MOUNT_TYPE_DEVICE);
-  }
-
-  LoggingObserver observer;
-  volume_manager()->AddObserver(&observer);
+    volume_manager()->OnMountEvent(
+        chromeos::disks::DiskMountManager::UNMOUNTING,
+        chromeos::MOUNT_ERROR_NONE,
+        kMountPoint);
 
-  const chromeos::disks::DiskMountManager::MountPointInfo kMountPoint(
-      "device1",
-      "mount1",
-      chromeos::MOUNT_TYPE_DEVICE,
-      chromeos::disks::MOUNT_CONDITION_NONE);
+    // Observe what happened for the mount event.
+    volume_manager()->AddObserver(&observer);
 
-  volume_manager()->OnMountEvent(chromeos::disks::DiskMountManager::MOUNTING,
-                                chromeos::MOUNT_ERROR_NONE,
-                                kMountPoint);
+    volume_manager()->OnMountEvent(
+        chromeos::disks::DiskMountManager::MOUNTING,
+        chromeos::MOUNT_ERROR_NONE,
+        kMountPoint);
+  }
 
   ASSERT_EQ(1U, observer.events().size());
   const LoggingObserver::Event& event = observer.events()[0];