Change filesystem passed to Format to be an enum

This is also done in preparation of recording the filesystem selected
when formatting into a UMA.

Bug: 632988
Change-Id: I91db06307638403a9907525e09558344e1b916c0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1707629
Commit-Queue: Austin Tankiang <austinct@chromium.org>
Reviewed-by: Noel Gordon <noel@chromium.org>
Reviewed-by: Anand Mistry <amistry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#678987}

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

@@ -48,6 +48,7 @@ using ::testing::ReturnRef;
 
 using chromeos::disks::Disk;
 using chromeos::disks::DiskMountManager;
+using chromeos::disks::FormatFileSystemType;
 
 namespace {
 
@@ -430,21 +431,21 @@ IN_PROC_BROWSER_TEST_F(FileManagerPrivateApiTest, FormatVolume) {
                   chromeos::CrosDisksClient::GetRemovableDiskMountPoint()
                       .AppendASCII("mount_path1")
                       .AsUTF8Unsafe(),
-                  "vfat", "NEWLABEL1"))
+                  FormatFileSystemType::kVfat, "NEWLABEL1"))
       .Times(1);
   EXPECT_CALL(*disk_mount_manager_mock_,
               FormatMountedDevice(
                   chromeos::CrosDisksClient::GetRemovableDiskMountPoint()
                       .AppendASCII("mount_path2")
                       .AsUTF8Unsafe(),
-                  "exfat", "NEWLABEL2"))
+                  FormatFileSystemType::kExfat, "NEWLABEL2"))
       .Times(1);
   EXPECT_CALL(*disk_mount_manager_mock_,
               FormatMountedDevice(
                   chromeos::CrosDisksClient::GetRemovableDiskMountPoint()
                       .AppendASCII("mount_path3")
                       .AsUTF8Unsafe(),
-                  "ntfs", "NEWLABEL3"))
+                  FormatFileSystemType::kNtfs, "NEWLABEL3"))
       .Times(1);
 
   ASSERT_TRUE(RunComponentExtensionTest("file_browser/format_test"));

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

@@ -339,6 +339,22 @@ std::vector<std::pair<base::FilePath, bool>> SearchByPattern(
   return prefix_matches;
 }
 
+chromeos::disks::FormatFileSystemType ApiFormatFileSystemToChromeEnum(
+    api::file_manager_private::FormatFileSystemType filesystem) {
+  switch (filesystem) {
+    case api::file_manager_private::FORMAT_FILE_SYSTEM_TYPE_NONE:
+      return chromeos::disks::FormatFileSystemType::kUnknown;
+    case api::file_manager_private::FORMAT_FILE_SYSTEM_TYPE_VFAT:
+      return chromeos::disks::FormatFileSystemType::kVfat;
+    case api::file_manager_private::FORMAT_FILE_SYSTEM_TYPE_EXFAT:
+      return chromeos::disks::FormatFileSystemType::kExfat;
+    case api::file_manager_private::FORMAT_FILE_SYSTEM_TYPE_NTFS:
+      return chromeos::disks::FormatFileSystemType::kNtfs;
+  }
+  NOTREACHED() << "Unknown format filesystem " << filesystem;
+  return chromeos::disks::FormatFileSystemType::kUnknown;
+}
+
 }  // namespace
 
 ExtensionFunction::ResponseAction
@@ -707,7 +723,7 @@ FileManagerPrivateFormatVolumeFunction::Run() {
 
   DiskMountManager::GetInstance()->FormatMountedDevice(
       volume->mount_path().AsUTF8Unsafe(),
-      api::file_manager_private::ToString(params->filesystem),
+      ApiFormatFileSystemToChromeEnum(params->filesystem),
       params->volume_label);
   return RespondNow(NoArguments());
 }

chrome/browser/chromeos/file_manager/fake_disk_mount_manager.cc

@@ -151,9 +151,10 @@ void FakeDiskMountManager::FailUnmountRequest(const std::string& mount_path,
   unmount_errors_[mount_path] = error_code;
 }
 
-void FakeDiskMountManager::FormatMountedDevice(const std::string& mount_path,
-                                               const std::string& filesystem,
-                                               const std::string& label) {}
+void FakeDiskMountManager::FormatMountedDevice(
+    const std::string& mount_path,
+    chromeos::disks::FormatFileSystemType filesystem,
+    const std::string& label) {}
 
 void FakeDiskMountManager::RenameMountedDevice(const std::string& mount_path,
                                                const std::string& volume_name) {

chrome/browser/chromeos/file_manager/fake_disk_mount_manager.h

@@ -99,7 +99,7 @@ class FakeDiskMountManager : public chromeos::disks::DiskMountManager {
   void RemountAllRemovableDrives(
       chromeos::MountAccessMode access_mode) override;
   void FormatMountedDevice(const std::string& mount_path,
-                           const std::string& filesystem,
+                           chromeos::disks::FormatFileSystemType filesystem,
                            const std::string& label) override;
   void RenameMountedDevice(const std::string& mount_path,
                            const std::string& volume_name) override;

chromeos/disks/disk_mount_manager.cc

@@ -49,6 +49,21 @@ void OnAllUnmountDeviceRecursively(
   std::move(cb_data->callback).Run(cb_data->error_code);
 }
 
+std::string FormatFileSystemTypeToString(FormatFileSystemType filesystem) {
+  switch (filesystem) {
+    case FormatFileSystemType::kUnknown:
+      return "";
+    case FormatFileSystemType::kVfat:
+      return "vfat";
+    case FormatFileSystemType::kExfat:
+      return "exfat";
+    case FormatFileSystemType::kNtfs:
+      return "ntfs";
+  }
+  NOTREACHED() << "Unknown filesystem type " << static_cast<int>(filesystem);
+  return "";
+}
+
 // The DiskMountManager implementation.
 class DiskMountManagerImpl : public DiskMountManager,
                              public CrosDisksClient::Observer {
@@ -135,8 +150,14 @@ class DiskMountManagerImpl : public DiskMountManager,
 
   // DiskMountManager override.
   void FormatMountedDevice(const std::string& mount_path,
-                           const std::string& filesystem,
+                           FormatFileSystemType filesystem,
                            const std::string& label) override {
+    if (filesystem == FormatFileSystemType::kUnknown) {
+      LOG(ERROR) << "Unknown filesystem passed to FormatMountedDevice";
+      OnFormatCompleted(FORMAT_ERROR_UNSUPPORTED_FILESYSTEM, mount_path);
+      return;
+    }
+
     MountPointMap::const_iterator mount_point = mount_points_.find(mount_path);
     if (mount_point == mount_points_.end()) {
       LOG(ERROR) << "Mount point with path \"" << mount_path << "\" not found.";
@@ -516,7 +537,7 @@ class DiskMountManagerImpl : public DiskMountManager,
   }
 
   void OnUnmountPathForFormat(const std::string& device_path,
-                              const std::string& filesystem,
+                              FormatFileSystemType filesystem,
                               const std::string& label,
                               MountError error_code) {
     if (error_code == MOUNT_ERROR_NONE &&
@@ -529,15 +550,16 @@ class DiskMountManagerImpl : public DiskMountManager,
 
   // Starts device formatting.
   void FormatUnmountedDevice(const std::string& device_path,
-                             const std::string& filesystem,
+                             FormatFileSystemType filesystem,
                              const std::string& label) {
     DiskMap::const_iterator disk = disks_.find(device_path);
     DCHECK(disk != disks_.end() && disk->second->mount_path().empty());
 
-    pending_format_changes_[device_path] = {filesystem, label};
+    const std::string filesystem_str = FormatFileSystemTypeToString(filesystem);
+    pending_format_changes_[device_path] = {filesystem_str, label};
 
     cros_disks_client_->Format(
-        device_path, filesystem, label,
+        device_path, filesystem_str, label,
         base::BindOnce(&DiskMountManagerImpl::OnFormatStarted,
                        weak_ptr_factory_.GetWeakPtr(), device_path));
   }

chromeos/disks/disk_mount_manager.h

@@ -27,6 +27,14 @@ enum MountCondition {
   MOUNT_CONDITION_UNSUPPORTED_FILESYSTEM,
 };
 
+// Possible filesystem types that can be passed to FormatMountedDevice.
+enum class FormatFileSystemType {
+  kUnknown = 0,
+  kVfat = 1,
+  kExfat = 2,
+  kNtfs = 3,
+};
+
 // This class handles the interaction with cros-disks.
 // Other classes can add themselves as observers.
 class COMPONENT_EXPORT(CHROMEOS_DISKS) DiskMountManager {
@@ -178,10 +186,10 @@ class COMPONENT_EXPORT(CHROMEOS_DISKS) DiskMountManager {
   // Formats device mounted at |mount_path| with the given filesystem and label.
   // Also unmounts the device before formatting.
   // Example: mount_path: /media/VOLUME_LABEL
-  //          filesystem: ntfs
+  //          filesystem: FormatFileSystemType::kNtfs
   //          label: MYUSB
   virtual void FormatMountedDevice(const std::string& mount_path,
-                                   const std::string& filesystem,
+                                   FormatFileSystemType filesystem,
                                    const std::string& label) = 0;
 
   // Renames Device given its mount path.

chromeos/disks/disk_mount_manager_unittest.cc

@@ -37,8 +37,11 @@ const char kReadOnlyDeviceMountPath[] = "/device/read_only_mount_path";
 const char kReadOnlyDeviceSourcePath[] = "/device/read_only_source_path";
 const char kFileSystemType1[] = "ntfs";
 const char kFileSystemType2[] = "exfat";
-const char kFormatFileSystemType1[] = "vfat";
-const char kFormatFileSystemType2[] = "exfat";
+const FormatFileSystemType kFormatFileSystemType1 = FormatFileSystemType::kVfat;
+const FormatFileSystemType kFormatFileSystemType2 =
+    FormatFileSystemType::kExfat;
+const char kFormatFileSystemType1Str[] = "vfat";
+const char kFormatFileSystemType2Str[] = "exfat";
 const char kFormatLabel1[] = "UNTITLED";
 const char kFormatLabel2[] = "TESTUSB";
 
@@ -694,7 +697,7 @@ TEST_F(DiskMountManagerTest, Format_FormatFailsToStart) {
   EXPECT_EQ(1, fake_cros_disks_client_->format_call_count());
   EXPECT_EQ(kDevice1SourcePath,
             fake_cros_disks_client_->last_format_device_path());
-  EXPECT_EQ(kFormatFileSystemType1,
+  EXPECT_EQ(kFormatFileSystemType1Str,
             fake_cros_disks_client_->last_format_filesystem());
   EXPECT_EQ(kFormatLabel1, fake_cros_disks_client_->last_format_label());
 
@@ -752,7 +755,7 @@ TEST_F(DiskMountManagerTest, Format_ConcurrentFormatCalls) {
   EXPECT_EQ(1, fake_cros_disks_client_->format_call_count());
   EXPECT_EQ(kDevice1SourcePath,
             fake_cros_disks_client_->last_format_device_path());
-  EXPECT_EQ(kFormatFileSystemType1,
+  EXPECT_EQ(kFormatFileSystemType1Str,
             fake_cros_disks_client_->last_format_filesystem());
   EXPECT_EQ(kFormatLabel1, fake_cros_disks_client_->last_format_label());
 
@@ -791,7 +794,7 @@ TEST_F(DiskMountManagerTest, Format_FormatFails) {
   EXPECT_EQ(1, fake_cros_disks_client_->format_call_count());
   EXPECT_EQ(kDevice1SourcePath,
             fake_cros_disks_client_->last_format_device_path());
-  EXPECT_EQ(kFormatFileSystemType1,
+  EXPECT_EQ(kFormatFileSystemType1Str,
             fake_cros_disks_client_->last_format_filesystem());
   EXPECT_EQ(kFormatLabel1, fake_cros_disks_client_->last_format_label());
 
@@ -843,7 +846,7 @@ TEST_F(DiskMountManagerTest, Format_FormatSuccess) {
   EXPECT_EQ(1, fake_cros_disks_client_->format_call_count());
   EXPECT_EQ(kDevice1SourcePath,
             fake_cros_disks_client_->last_format_device_path());
-  EXPECT_EQ(kFormatFileSystemType1,
+  EXPECT_EQ(kFormatFileSystemType1Str,
             fake_cros_disks_client_->last_format_filesystem());
   EXPECT_EQ(kFormatLabel1, fake_cros_disks_client_->last_format_label());
 
@@ -867,7 +870,7 @@ TEST_F(DiskMountManagerTest, Format_FormatSuccess) {
             observer_->GetFormatEvent(2));
 
   // Disk should have new values for file system type and device label name
-  EXPECT_EQ(kFormatFileSystemType1,
+  EXPECT_EQ(kFormatFileSystemType1Str,
             disks.find(kDevice1SourcePath)->second->file_system_type());
   EXPECT_EQ(kFormatLabel1,
             disks.find(kDevice1SourcePath)->second->device_label());
@@ -894,7 +897,7 @@ TEST_F(DiskMountManagerTest, Format_ConsecutiveFormatCalls) {
   EXPECT_EQ(1, fake_cros_disks_client_->format_call_count());
   EXPECT_EQ(kDevice1SourcePath,
             fake_cros_disks_client_->last_format_device_path());
-  EXPECT_EQ(kFormatFileSystemType1,
+  EXPECT_EQ(kFormatFileSystemType1Str,
             fake_cros_disks_client_->last_format_filesystem());
   EXPECT_EQ(kFormatLabel1, fake_cros_disks_client_->last_format_label());
 
@@ -927,7 +930,7 @@ TEST_F(DiskMountManagerTest, Format_ConsecutiveFormatCalls) {
   EXPECT_EQ(2, fake_cros_disks_client_->format_call_count());
   EXPECT_EQ(kDevice1SourcePath,
             fake_cros_disks_client_->last_format_device_path());
-  EXPECT_EQ(kFormatFileSystemType2,
+  EXPECT_EQ(kFormatFileSystemType2Str,
             fake_cros_disks_client_->last_format_filesystem());
   EXPECT_EQ(kFormatLabel2, fake_cros_disks_client_->last_format_label());
 

chromeos/disks/mock_disk_mount_manager.h

@@ -50,7 +50,7 @@ class MockDiskMountManager : public DiskMountManager {
   MOCK_METHOD1(RemountAllRemovableDrives, void(MountAccessMode));
   MOCK_METHOD3(FormatMountedDevice,
                void(const std::string&,
-                    const std::string&,
+                    FormatFileSystemType,
                     const std::string&));
   MOCK_METHOD2(RenameMountedDevice,
                void(const std::string&, const std::string&));