Do lazy unmount only when the disk has been physically removed.

Also, wire up unmount failure event to file manager so error message
for unmount failure can e displayed.

TEST=manual
    (try unmounting USB while copying a file to the device ->
         unmount should fail and error message should be displayed;
     remove USB while copying a file to it in progress ->
         the device should be unmounted)
BUG=154403


Review URL: https://chromiumcodereview.appspot.com/11099004

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

7 files changed, 62 insertions, 31 deletions

diff --git a/chromeos/dbus/cros_disks_client.cc b/chromeos/dbus/cros_disks_client.cc
index 1688b8d..edafb18 100644
--- a/chromeos/dbus/cros_disks_client.cc
+++ b/chromeos/dbus/cros_disks_client.cc
@@ -28,9 +28,10 @@ const char* kDefaultMountOptions[] = {
 
 const char* kDefaultUnmountOptions[] = {
   "force",
-  "lazy",
 };
 
+const char kLazyUnmountOption[] = "lazy";
+
 const char kMountLabelOption[] = "mountlabel";
 
 // Checks if retrieved media type is in boundaries of DeviceMediaType.
@@ -150,15 +151,20 @@ class CrosDisksClientImpl : public CrosDisksClient {
 
   // CrosDisksClient override.
   virtual void Unmount(const std::string& device_path,
+                       UnmountOptions options,
                        const UnmountCallback& callback,
-                       const ErrorCallback& error_callback) OVERRIDE {
+                       const UnmountCallback& error_callback) OVERRIDE {
     dbus::MethodCall method_call(cros_disks::kCrosDisksInterface,
                                  cros_disks::kUnmount);
     dbus::MessageWriter writer(&method_call);
     writer.AppendString(device_path);
-    std::vector<std::string> unmount_options(kDefaultUnmountOptions,
-                                             kDefaultUnmountOptions +
-                                             arraysize(kDefaultUnmountOptions));
+
+    std::vector<std::string> unmount_options(
+        kDefaultUnmountOptions,
+        kDefaultUnmountOptions + arraysize(kDefaultUnmountOptions));
+    if (options == UNMOUNT_OPTIONS_LAZY)
+      unmount_options.push_back(kLazyUnmountOption);
+
     writer.AppendArrayOfStrings(unmount_options);
     proxy_->CallMethod(&method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
                        base::Bind(&CrosDisksClientImpl::OnUnmount,
@@ -276,10 +282,10 @@ class CrosDisksClientImpl : public CrosDisksClient {
   // Handles the result of Unount and calls |callback| or |error_callback|.
   void OnUnmount(const std::string& device_path,
                  const UnmountCallback& callback,
-                 const ErrorCallback& error_callback,
+                 const UnmountCallback& error_callback,
                  dbus::Response* response) {
     if (!response) {
-      error_callback.Run();
+      error_callback.Run(device_path);
       return;
     }
     callback.Run(device_path);
@@ -400,8 +406,9 @@ class CrosDisksClientStubImpl : public CrosDisksClient {
                      const MountCallback& callback,
                      const ErrorCallback& error_callback) OVERRIDE {}
   virtual void Unmount(const std::string& device_path,
+                       UnmountOptions options,
                        const UnmountCallback& callback,
-                       const ErrorCallback& error_callback) OVERRIDE {}
+                       const UnmountCallback& error_callback) OVERRIDE {}
   virtual void EnumerateAutoMountableDevices(
       const EnumerateAutoMountableDevicesCallback& callback,
       const ErrorCallback& error_callback) OVERRIDE {}
diff --git a/chromeos/dbus/cros_disks_client.h b/chromeos/dbus/cros_disks_client.h
index 3590ca6..62cc67b 100644
--- a/chromeos/dbus/cros_disks_client.h
+++ b/chromeos/dbus/cros_disks_client.h
@@ -18,6 +18,8 @@ class Bus;
 class Response;
 }
 
+// TODO(tbarzic): We should probably move these enums inside CrosDisksClient,
+// to be clearer where they come from.
 namespace chromeos {
 
 // Enum describing types of mount used by cros-disks.
@@ -64,6 +66,12 @@ enum MountEventType {
   FORMATTING_FINISHED,
 };
 
+// Additional unmount flags to be added to unmount request.
+enum UnmountOptions {
+  UNMOUNT_OPTIONS_NONE,
+  UNMOUNT_OPTIONS_LAZY,  // Do lazy unmount.
+};
+
 // A class to represent information about a disk sent from cros-disks.
 class DiskInfo {
  public:
@@ -219,8 +227,9 @@ class CHROMEOS_EXPORT CrosDisksClient {
   // Calls Unmount method.  |callback| is called after the method call succeeds,
   // otherwise, |error_callback| is called.
   virtual void Unmount(const std::string& device_path,
+                       UnmountOptions options,
                        const UnmountCallback& callback,
-                       const ErrorCallback& error_callback) = 0;
+                       const UnmountCallback& error_callback) = 0;
 
   // Calls EnumerateAutoMountableDevices method.  |callback| is called after the
   // method call succeeds, otherwise, |error_callback| is called.
diff --git a/chromeos/dbus/mock_cros_disks_client.h b/chromeos/dbus/mock_cros_disks_client.h
index c237d90..9b594001 100644
--- a/chromeos/dbus/mock_cros_disks_client.h
+++ b/chromeos/dbus/mock_cros_disks_client.h
@@ -23,9 +23,10 @@ class MockCrosDisksClient : public CrosDisksClient {
                            MountType,
                            const MountCallback&,
                            const ErrorCallback&));
-  MOCK_METHOD3(Unmount, void(const std::string&,
+  MOCK_METHOD4(Unmount, void(const std::string&,
+                             UnmountOptions,
                              const UnmountCallback&,
-                             const ErrorCallback&));
+                             const UnmountCallback&));
   MOCK_METHOD2(EnumerateAutoMountableDevices,
                void(const EnumerateAutoMountableDevicesCallback&,
                     const ErrorCallback&));
diff --git a/chromeos/disks/disk_mount_manager.cc b/chromeos/disks/disk_mount_manager.cc
index 65bcda5..6f450d2 100644
--- a/chromeos/disks/disk_mount_manager.cc
+++ b/chromeos/disks/disk_mount_manager.cc
@@ -81,12 +81,16 @@ class DiskMountManagerImpl : public DiskMountManager {
   }
 
   // DiskMountManager override.
-  virtual void UnmountPath(const std::string& mount_path) OVERRIDE {
+  virtual void UnmountPath(const std::string& mount_path,
+                           UnmountOptions options) OVERRIDE {
     UnmountChildMounts(mount_path);
-    cros_disks_client_->Unmount(mount_path,
+    cros_disks_client_->Unmount(mount_path, options,
                                 base::Bind(&DiskMountManagerImpl::OnUnmountPath,
-                                           weak_ptr_factory_.GetWeakPtr()),
-                                base::Bind(&base::DoNothing));
+                                           weak_ptr_factory_.GetWeakPtr(),
+                                           true),
+                                base::Bind(&DiskMountManagerImpl::OnUnmountPath,
+                                           weak_ptr_factory_.GetWeakPtr(),
+                                           false));
   }
 
   // DiskMountManager override.
@@ -135,7 +139,7 @@ class DiskMountManagerImpl : public DiskMountManager {
     }
     // Formatting process continues, after unmounting.
     formatting_pending_[disk->device_path()] = disk->file_path();
-    UnmountPath(disk->mount_path());
+    UnmountPath(disk->mount_path(), UNMOUNT_OPTIONS_NONE);
   }
 
   // DiskMountManager override.
@@ -176,13 +180,11 @@ class DiskMountManagerImpl : public DiskMountManager {
       for (size_t i = 0; i < devices_to_unmount.size(); ++i) {
         cros_disks_client_->Unmount(
             devices_to_unmount[i],
+            UNMOUNT_OPTIONS_NONE,
             base::Bind(&DiskMountManagerImpl::OnUnmountDeviceRecursive,
                        weak_ptr_factory_.GetWeakPtr(), cb_data, true),
             base::Bind(&DiskMountManagerImpl::OnUnmountDeviceRecursive,
-                       weak_ptr_factory_.GetWeakPtr(),
-                       cb_data,
-                       false,
-                       devices_to_unmount[i]));
+                       weak_ptr_factory_.GetWeakPtr(), cb_data, false));
       }
     } else {
       LOG(WARNING) << "Unmount recursive request failed for device "
@@ -242,7 +244,7 @@ class DiskMountManagerImpl : public DiskMountManager {
          ++it) {
       if (StartsWithASCII(it->second.source_path, mount_path,
                           true /*case sensitive*/)) {
-        UnmountPath(it->second.mount_path);
+        UnmountPath(it->second.mount_path, UNMOUNT_OPTIONS_NONE);
       }
     }
   }
@@ -253,7 +255,7 @@ class DiskMountManagerImpl : public DiskMountManager {
                                 const std::string& mount_path) {
     if (success) {
       // Do standard processing for Unmount event.
-      OnUnmountPath(mount_path);
+      OnUnmountPath(true, mount_path);
       LOG(INFO) << mount_path <<  " unmounted.";
     }
     // This is safe as long as all callbacks are called on the same thread as
@@ -309,34 +311,44 @@ class DiskMountManagerImpl : public DiskMountManager {
   }
 
   // Callback for UnmountPath.
-  void OnUnmountPath(const std::string& mount_path) {
+  void OnUnmountPath(bool success, const std::string& mount_path) {
     MountPointMap::iterator mount_points_it = mount_points_.find(mount_path);
     if (mount_points_it == mount_points_.end())
       return;
-    // TODO(tbarzic): Add separate, PathUnmounted event to Observer.
     NotifyMountCompleted(
-        UNMOUNTING, MOUNT_ERROR_NONE,
+        UNMOUNTING,
+        success ? MOUNT_ERROR_NONE : MOUNT_ERROR_INTERNAL,
         MountPointInfo(mount_points_it->second.source_path,
                        mount_points_it->second.mount_path,
                        mount_points_it->second.mount_type,
                        mount_points_it->second.mount_condition));
+
     std::string path(mount_points_it->second.source_path);
-    mount_points_.erase(mount_points_it);
+    if (success)
+      mount_points_.erase(mount_points_it);
+
     DiskMap::iterator iter = disks_.find(path);
     if (iter == disks_.end()) {
       // disk might have been removed by now.
       return;
     }
+
     Disk* disk = iter->second;
     DCHECK(disk);
-    disk->clear_mount_path();
+    if (success)
+      disk->clear_mount_path();
+
     // Check if there is a formatting scheduled.
     PathMap::iterator it = formatting_pending_.find(disk->device_path());
     if (it != formatting_pending_.end()) {
       // Copy the string before it gets erased.
       const std::string file_path = it->second;
       formatting_pending_.erase(it);
-      FormatUnmountedDevice(file_path);
+      if (success) {
+        FormatUnmountedDevice(file_path);
+      } else {
+        OnFormatDevice(file_path, false);
+      }
     }
   }
 
diff --git a/chromeos/disks/disk_mount_manager.h b/chromeos/disks/disk_mount_manager.h
index e6d46d8..0fe1a1a 100644
--- a/chromeos/disks/disk_mount_manager.h
+++ b/chromeos/disks/disk_mount_manager.h
@@ -236,7 +236,9 @@ class CHROMEOS_EXPORT DiskMountManager {
                          MountType type) = 0;
 
   // Unmounts a mounted disk.
-  virtual void UnmountPath(const std::string& mount_path) = 0;
+  // |UnmountOptions| enum defined in chromeos/dbus/cros_disks_client.h.
+  virtual void UnmountPath(const std::string& mount_path,
+                           UnmountOptions options) = 0;
 
   // Formats device given its file path.
   // Example: file_path: /dev/sdb1
diff --git a/chromeos/disks/mock_disk_mount_manager.cc b/chromeos/disks/mock_disk_mount_manager.cc
index 9af1651..9b6171e 100644
--- a/chromeos/disks/mock_disk_mount_manager.cc
+++ b/chromeos/disks/mock_disk_mount_manager.cc
@@ -170,7 +170,7 @@ void MockDiskMountManager::SetupDefaultReplies() {
       .Times(AnyNumber());
   EXPECT_CALL(*this, MountPath(_, _, _, _))
       .Times(AnyNumber());
-  EXPECT_CALL(*this, UnmountPath(_))
+  EXPECT_CALL(*this, UnmountPath(_, _))
       .Times(AnyNumber());
   EXPECT_CALL(*this, FormatUnmountedDevice(_))
       .Times(AnyNumber());
diff --git a/chromeos/disks/mock_disk_mount_manager.h b/chromeos/disks/mock_disk_mount_manager.h
index d78f6f3..e6b8e89 100644
--- a/chromeos/disks/mock_disk_mount_manager.h
+++ b/chromeos/disks/mock_disk_mount_manager.h
@@ -32,7 +32,7 @@ class MockDiskMountManager : public DiskMountManager {
   MOCK_METHOD0(RequestMountInfoRefresh, void(void));
   MOCK_METHOD4(MountPath, void(const std::string&, const std::string&,
                                const std::string&, MountType));
-  MOCK_METHOD1(UnmountPath, void(const std::string&));
+  MOCK_METHOD2(UnmountPath, void(const std::string&, UnmountOptions));
   MOCK_METHOD1(FormatUnmountedDevice, void(const std::string&));
   MOCK_METHOD1(FormatMountedDevice, void(const std::string&));
   MOCK_METHOD3(UnmountDeviceRecursive, void(const std::string&,