Observe cros-disks FormatCompleted signal instead of FormattingFinished.

The FormattingFinished signal has been long deprecated (back in 2011)
and replaced by the FormatCompleted signal, which allows cros-disks to
properly notify Chrome the completion of a format operation with an
appropriate error code. This CL modifies CrosDisksClient and
DiskMountManager to use the FormatCompleted signal.

BUG=369877
TEST=Tested the following:
1. Run chromeos_unittests.
2. Insert a USB drive and format the mounted partition via Files.app.
   Verify that Files.app notifies the successful completion of the
   format operation.
3. Insert a write-protected SD card and format the mounted partition via
   Files.app. Verify that Files.app reports an error.

R=stevenjb@chromium.org, tbarzic@chromium.org

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

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

2 files changed, 48 insertions, 56 deletions

diff --git a/chromeos/disks/disk_mount_manager.cc b/chromeos/disks/disk_mount_manager.cc
index a781107..75ab642 100644
--- a/chromeos/disks/disk_mount_manager.cc
+++ b/chromeos/disks/disk_mount_manager.cc
@@ -31,11 +31,15 @@ class DiskMountManagerImpl : public DiskMountManager {
     DCHECK(dbus_thread_manager);
     cros_disks_client_ = dbus_thread_manager->GetCrosDisksClient();
     DCHECK(cros_disks_client_);
-    cros_disks_client_->SetUpConnections(
+    cros_disks_client_->SetMountEventHandler(
         base::Bind(&DiskMountManagerImpl::OnMountEvent,
-                   weak_ptr_factory_.GetWeakPtr()),
+                   weak_ptr_factory_.GetWeakPtr()));
+    cros_disks_client_->SetMountCompletedHandler(
         base::Bind(&DiskMountManagerImpl::OnMountCompleted,
                    weak_ptr_factory_.GetWeakPtr()));
+    cros_disks_client_->SetFormatCompletedHandler(
+        base::Bind(&DiskMountManagerImpl::OnFormatCompleted,
+                   weak_ptr_factory_.GetWeakPtr()));
   }
 
   virtual ~DiskMountManagerImpl() {
@@ -280,7 +284,7 @@ class DiskMountManagerImpl : public DiskMountManager {
     if (success) {
       // Do standard processing for Unmount event.
       OnUnmountPath(UnmountPathCallback(), true, mount_path);
-      LOG(INFO) << mount_path <<  " unmounted.";
+      VLOG(1) << mount_path <<  " unmounted.";
     }
     // This is safe as long as all callbacks are called on the same thread as
     // UnmountDeviceRecursively.
@@ -407,6 +411,33 @@ class DiskMountManagerImpl : public DiskMountManager {
     NotifyFormatStatusUpdate(FORMAT_STARTED, error_code, device_path);
   }
 
+  // Callback to handle FormatCompleted signal and Format method call failure.
+  void OnFormatCompleted(FormatError error_code,
+                        const std::string& device_path) {
+    std::string actual_device_path;
+    // Depending on cros-disks implementation the event may return either file
+    // path or device path. We want to use device path.
+    // TODO(benchan): This shouldn't be necessary. Find out the issue and ensure
+    // that cros-disks simply returns the path format that Chrome expects, then
+    // we should remove the following code.
+    for (DiskMountManager::DiskMap::const_iterator it = disks_.begin();
+         it != disks_.end(); ++it) {
+      if (it->second->file_path() == device_path ||
+          it->second->device_path() == device_path) {
+        actual_device_path = it->second->device_path();
+        break;
+      }
+    }
+
+    if (actual_device_path.empty()) {
+      LOG(ERROR) << "Error while handling disks metadata. Cannot find "
+                 << "device that is being formatted.";
+      return;
+    }
+
+    NotifyFormatStatusUpdate(FORMAT_COMPLETED, error_code, actual_device_path);
+  }
+
   // Callbcak for GetDeviceProperties.
   void OnGetDeviceProperties(const DiskInfo& disk_info) {
     // TODO(zelidrag): Find a better way to filter these out before we
@@ -513,20 +544,6 @@ class DiskMountManagerImpl : public DiskMountManager {
         NotifyDeviceStatusUpdate(DEVICE_SCANNED, device_path);
         break;
       }
-      case CROS_DISKS_FORMATTING_FINISHED: {
-        std::string path;
-        FormatError error_code;
-        ParseFormatFinishedPath(device_path, &path, &error_code);
-
-        if (!path.empty()) {
-          NotifyFormatStatusUpdate(FORMAT_COMPLETED, error_code, path);
-          break;
-        }
-
-        LOG(ERROR) << "Error while handling disks metadata. Cannot find "
-                   << "device that is being formatted.";
-        break;
-      }
       default: {
         LOG(ERROR) << "Unknown event: " << event;
       }
@@ -560,31 +577,6 @@ class DiskMountManagerImpl : public DiskMountManager {
                       OnFormatEvent(event, error_code, device_path));
   }
 
-  // Converts file path to device path.
-  void ParseFormatFinishedPath(const std::string& received_path,
-                               std::string* device_path,
-                               FormatError* error_code) {
-    // TODO(tbarzic): Refactor error handling code like here.
-    // Appending "!" is not the best way to indicate error.  This kind of trick
-    // also makes it difficult to simplify the code paths.
-    bool success = !StartsWithASCII(received_path, "!", true);
-    *error_code = success ? FORMAT_ERROR_NONE : FORMAT_ERROR_UNKNOWN;
-
-    std::string path = received_path.substr(success ? 0 : 1);
-
-    // Depending on cros disks implementation the event may return either file
-    // path or device path. We want to use device path.
-    for (DiskMountManager::DiskMap::iterator it = disks_.begin();
-         it != disks_.end(); ++it) {
-      // Skip the leading '!' on the failure case.
-      if (it->second->file_path() == path ||
-          it->second->device_path() == path) {
-        *device_path = it->second->device_path();
-        return;
-      }
-    }
-  }
-
   // Finds system path prefix from |system_path|.
   const std::string& FindSystemPathPrefix(const std::string& system_path) {
     if (system_path.empty())
diff --git a/chromeos/disks/disk_mount_manager_unittest.cc b/chromeos/disks/disk_mount_manager_unittest.cc
index 36e67ad..3786dcd3 100644
--- a/chromeos/disks/disk_mount_manager_unittest.cc
+++ b/chromeos/disks/disk_mount_manager_unittest.cc
@@ -386,7 +386,7 @@ TEST_F(DiskMountManagerTest, Format_FormatFails) {
   // The observer should get notified that the device was unmounted and that
   // formatting has started.
   // After the formatting starts, the test will simulate failing
-  // FORMATTING_FINISHED signal, so the observer should also be notified the
+  // FORMAT_COMPLETED signal, so the observer should also be notified the
   // formatting has failed (FORMAT_COMPLETED event).
   {
     InSequence s;
@@ -429,14 +429,14 @@ TEST_F(DiskMountManagerTest, Format_FormatFails) {
   // The device should be unmounted by now.
   EXPECT_FALSE(HasMountPoint("/device/mount_path"));
 
-  // Send failing FORMATTING_FINISHED signal.
+  // Send failing FORMAT_COMPLETED signal.
   // The failure is marked by ! in fromt of the path (but this should change
   // soon).
-  fake_cros_disks_client_->SendMountEvent(
-      chromeos::CROS_DISKS_FORMATTING_FINISHED, "!/device/source_path");
+  fake_cros_disks_client_->SendFormatCompletedEvent(
+      chromeos::FORMAT_ERROR_UNKNOWN, "/device/source_path");
 }
 
-// Tests the same case as Format_FormatFails, but the FORMATTING_FINISHED event
+// Tests the same case as Format_FormatFails, but the FORMAT_COMPLETED event
 // is sent with file_path of the formatted device (instead of its device path).
 TEST_F(DiskMountManagerTest, Format_FormatFailsAndReturnFilePath) {
   // Set up expectations for observer mock.
@@ -481,9 +481,9 @@ TEST_F(DiskMountManagerTest, Format_FormatFailsAndReturnFilePath) {
   // The device should be unmounted by now.
   EXPECT_FALSE(HasMountPoint("/device/mount_path"));
 
-  // Send failing FORMATTING_FINISHED signal with the device's file path.
-  fake_cros_disks_client_->SendMountEvent(
-      chromeos::CROS_DISKS_FORMATTING_FINISHED, "!/device/file_path");
+  // Send failing FORMAT_COMPLETED signal with the device's file path.
+  fake_cros_disks_client_->SendFormatCompletedEvent(
+      chromeos::FORMAT_ERROR_UNKNOWN, "/device/file_path");
 }
 
 // Tests the case when formatting completes successfully.
@@ -536,8 +536,8 @@ TEST_F(DiskMountManagerTest, Format_FormatSuccess) {
   EXPECT_FALSE(HasMountPoint("/device/mount_path"));
 
   // Simulate cros_disks reporting success.
-  fake_cros_disks_client_->SendMountEvent(
-      chromeos::CROS_DISKS_FORMATTING_FINISHED, "/device/source_path");
+  fake_cros_disks_client_->SendFormatCompletedEvent(
+      chromeos::FORMAT_ERROR_NONE, "/device/source_path");
 }
 
 // Tests that it's possible to format the device twice in a row (this may not be
@@ -597,8 +597,8 @@ TEST_F(DiskMountManagerTest, Format_ConsecutiveFormatCalls) {
   EXPECT_FALSE(HasMountPoint("/device/mount_path"));
 
   // Simulate cros_disks reporting success.
-  fake_cros_disks_client_->SendMountEvent(
-      chromeos::CROS_DISKS_FORMATTING_FINISHED, "/device/source_path");
+  fake_cros_disks_client_->SendFormatCompletedEvent(
+      chromeos::FORMAT_ERROR_NONE, "/device/source_path");
 
   // Simulate the device remounting.
   fake_cros_disks_client_->SendMountCompletedEvent(
@@ -627,8 +627,8 @@ TEST_F(DiskMountManagerTest, Format_ConsecutiveFormatCalls) {
             fake_cros_disks_client_->last_format_device_filesystem());
 
   // Simulate cros_disks reporting success.
-  fake_cros_disks_client_->SendMountEvent(
-      chromeos::CROS_DISKS_FORMATTING_FINISHED, "/device/source_path");
+  fake_cros_disks_client_->SendFormatCompletedEvent(
+      chromeos::FORMAT_ERROR_NONE, "/device/source_path");
 }
 
 }  // namespace