Stopping using DiskChanged event for mount/unmount event processing in...

Stopping using DiskChanged event for mount/unmount event processing in FileBrowserEventRouter and some bug fixes.

UnmountPath now expects mount_path to be passed to it.

Hiding device notification when disk added event is triggered instead when some
device is mounted (notification handleing needs some more work).

Fixed chromium-os:18339 and chromium-os:17777


BUG=chromium-os:18399, chromium-os:17531, chromium-os:17777
TEST=Made sure FileBrowser works as expected on Cr48.
Review URL: http://codereview.chromium.org/7466046

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

chrome/browser/chromeos/cros/mount_library.cc

@@ -73,9 +73,6 @@ MountLibrary::Disk::Disk(const std::string& device_path,
       is_read_only_(is_read_only),
       has_media_(has_media),
       on_boot_device_(on_boot_device) {
-  // Add trailing slash to mount path.
-  if (mount_path_.length() && mount_path_.at(mount_path_.length() -1) != '/')
-    mount_path_ = mount_path_.append("/");
 }
 
 MountLibrary::Disk::~Disk() {}
@@ -133,15 +130,17 @@ class MountLibraryImpl : public MountLibrary {
     MountSourcePath(source_path, type, options, &MountCompletedHandler, this);
   }
 
-  virtual void UnmountPath(const char* path) OVERRIDE {
+  virtual void UnmountPath(const char* mount_path) OVERRIDE {
     CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
     if (!CrosLibrary::Get()->EnsureLoaded()) {
-      OnUnmountPath(path,
+      OnUnmountPath(mount_path,
                     MOUNT_METHOD_ERROR_LOCAL,
                     kLibraryNotLoaded);
       return;
     }
-    UnmountMountPoint(path, &MountLibraryImpl::UnmountMountPointCallback, this);
+
+    UnmountMountPoint(mount_path, &MountLibraryImpl::UnmountMountPointCallback,
+                      this);
   }
 
   virtual void UnmountDeviceRecursive(const char* device_path,
@@ -158,16 +157,23 @@ class MountLibraryImpl : public MountLibrary {
       // Get list of all devices to unmount.
       int device_path_len = strlen(device_path);
       for (DiskMap::iterator it = disks_.begin(); it != disks_.end(); ++it) {
-        if (strncmp(device_path, it->second->device_path().c_str(),
+        if (!it->second->mount_path().empty() &&
+            strncmp(device_path, it->second->device_path().c_str(),
                     device_path_len) == 0) {
-          devices_to_unmount.push_back(it->second->device_path().c_str());
+          devices_to_unmount.push_back(it->second->mount_path().c_str());
         }
       }
 
       // We should detect at least original device.
       if (devices_to_unmount.size() == 0) {
+        if (disks_.find(device_path) == disks_.end()) {
           success = false;
           error_message = kDeviceNotFound;
+        } else {
+          // Nothing to unmount.
+          callback(user_data, true);
+          return;
+        }
       }
     }
 
@@ -223,17 +229,17 @@ class MountLibraryImpl : public MountLibrary {
 
   // Callback for UnmountRemovableDevice method.
   static void UnmountMountPointCallback(void* object,
-                                        const char* device_path,
+                                        const char* mount_path,
                                         MountMethodErrorType error,
                                         const char* error_message) {
     DCHECK(object);
     MountLibraryImpl* self = static_cast<MountLibraryImpl*>(object);
-    self->OnUnmountPath(device_path, error, error_message);
+    self->OnUnmountPath(mount_path, error, error_message);
   }
 
   // Callback for UnmountDeviceRecursive.
   static void UnmountDeviceRecursiveCallback(void* object,
-                                             const char* device_path,
+                                             const char* mount_path,
                                              MountMethodErrorType error,
                                              const char* error_message) {
     DCHECK(object);
@@ -241,13 +247,13 @@ class MountLibraryImpl : public MountLibrary {
         static_cast<UnmountDeviceRecursiveCallbackData*>(object);
 
     // Do standard processing for Unmount event.
-    cb_data->object->OnUnmountPath(device_path,
+    cb_data->object->OnUnmountPath(mount_path,
                                    error,
                                    error_message);
     if (error == MOUNT_METHOD_ERROR_LOCAL) {
       cb_data->success = false;
     } else if (error == MOUNT_METHOD_ERROR_NONE) {
-      LOG(WARNING) << device_path <<  " unmounted.";
+      LOG(INFO) << mount_path <<  " unmounted.";
    }
 
     // This is safe as long as all callbacks are called on the same thread as
@@ -302,14 +308,12 @@ class MountLibraryImpl : public MountLibrary {
                         const MountPointInfo& mount_info) {
     DCHECK(!mount_info.source_path.empty());
 
-    FireMountCompleted(MOUNTING,
-                       error_code,
-                       mount_info);
+    FireMountCompleted(MOUNTING, error_code, mount_info);
 
     if (error_code == MOUNT_ERROR_NONE &&
-        mount_points_.find(mount_info.source_path) == mount_points_.end()) {
+        mount_points_.find(mount_info.mount_path) == mount_points_.end()) {
       mount_points_.insert(MountPointMap::value_type(
-                               mount_info.source_path.c_str(),
+                               mount_info.mount_path.c_str(),
                                mount_info));
     }
 
@@ -330,13 +334,12 @@ class MountLibraryImpl : public MountLibrary {
     }
   }
 
-  void OnUnmountPath(const char* source_path,
+  void OnUnmountPath(const char* mount_path,
                      MountMethodErrorType error,
                      const char* error_message) {
-    DCHECK(source_path);
-
-    if (error == MOUNT_METHOD_ERROR_NONE && source_path) {
-      MountPointMap::iterator mount_points_it = mount_points_.find(source_path);
+    DCHECK(mount_path);
+    if (error == MOUNT_METHOD_ERROR_NONE && 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.
@@ -346,9 +349,9 @@ class MountLibraryImpl : public MountLibrary {
           MountPointInfo(mount_points_it->second.source_path.c_str(),
                          mount_points_it->second.mount_path.c_str(),
                          mount_points_it->second.mount_type));
+      std::string path(mount_points_it->second.source_path);
       mount_points_.erase(mount_points_it);
 
-      std::string path(source_path);
       DiskMap::iterator iter = disks_.find(path);
       if (iter == disks_.end()) {
         // disk might have been removed by now?
@@ -360,7 +363,7 @@ class MountLibraryImpl : public MountLibrary {
       FireDiskStatusUpdate(MOUNT_DISK_UNMOUNTED, disk);
     } else {
       LOG(WARNING) << "Unmount request failed for device "
-                   << source_path << ", with error: "
+                   << mount_path << ", with error: "
                    << (error_message ? error_message : "Unknown");
     }
   }
@@ -590,7 +593,7 @@ class MountLibraryStubImpl : public MountLibrary {
   virtual void RequestMountInfoRefresh() OVERRIDE {}
   virtual void MountPath(const char* source_path, MountType type,
                          const MountPathOptions& options) OVERRIDE {}
-  virtual void UnmountPath(const char* path) OVERRIDE {}
+  virtual void UnmountPath(const char* mount_path) OVERRIDE {}
   virtual void UnmountDeviceRecursive(const char* device_path,
       UnmountDeviceRecursiveCallbackType callback, void* user_data)
       OVERRIDE {}

chrome/browser/chromeos/cros/mount_library.h

@@ -101,7 +101,6 @@ class MountLibrary {
   };
   typedef std::map<std::string, Disk*> DiskMap;
 
-  // MountPointInfo: {mount_path, mount_type}.
   struct MountPointInfo {
     std::string source_path;
     std::string mount_path;
@@ -114,7 +113,7 @@ class MountLibrary {
     }
   };
 
-  // MountPointMap key is source_path.
+  // MountPointMap key is mount_path.
   typedef std::map<std::string, MountPointInfo> MountPointMap;
 
   typedef void(*UnmountDeviceRecursiveCallbackType)(void*, bool);
@@ -142,7 +141,7 @@ class MountLibrary {
   virtual void MountPath(const char* source_path,
                          MountType type,
                          const MountPathOptions& options) = 0;
-  // |path| may be source od mount path.
+  // |path| is device's mount path.
   virtual void UnmountPath(const char* path) = 0;
 
   // Unmounts device_poath and all of its known children.

chrome/browser/chromeos/extensions/file_browser_event_router.cc

@@ -6,6 +6,7 @@
 
 #include "base/json/json_writer.h"
 #include "base/stl_util.h"
+#include "base/utf_string_conversions.h"
 #include "base/values.h"
 #include "chrome/browser/chromeos/cros/cros_library.h"
 #include "chrome/browser/chromeos/login/user_manager.h"
@@ -158,8 +159,6 @@ void ExtensionFileBrowserEventRouter::DiskChanged(
     OnDiskAdded(disk);
   } else if (event == chromeos::MOUNT_DISK_REMOVED) {
     OnDiskRemoved(disk);
-  } else if (event == chromeos::MOUNT_DISK_CHANGED) {
-    OnDiskChanged(disk);
   }
 }
 
@@ -178,6 +177,38 @@ void ExtensionFileBrowserEventRouter::MountCompleted(
     chromeos::MountLibrary::MountEvent event_type,
     chromeos::MountError error_code,
     const chromeos::MountLibrary::MountPointInfo& mount_info) {
+  if (mount_info.mount_type == chromeos::MOUNT_TYPE_DEVICE &&
+      event_type == chromeos::MountLibrary::MOUNTING) {
+    chromeos::MountLibrary* mount_lib =
+        chromeos::CrosLibrary::Get()->GetMountLibrary();
+    chromeos::MountLibrary::Disk* disk =
+        mount_lib->disks().find(mount_info.source_path)->second;
+
+    if (!error_code) {
+      HideDeviceNotification(disk->system_path());
+    } else {
+      HideDeviceNotification(disk->system_path());
+      if (!disk->drive_label().empty()) {
+        ShowDeviceNotification(disk->system_path(),
+            IDR_PAGEINFO_INFO,
+            // TODO(tbarzic): Find more suitable message.
+            l10n_util::GetStringFUTF16(
+                IDS_FILE_BROWSER_ARCHIVE_MOUNT_FAILED,
+                ASCIIToUTF16(disk->drive_label()),
+                ASCIIToUTF16(MountErrorToString(error_code))));
+      } else {
+        ShowDeviceNotification(disk->system_path(),
+            IDR_PAGEINFO_INFO,
+            // TODO(tbarzic): Find more suitable message.
+            l10n_util::GetStringFUTF16(
+                IDS_FILE_BROWSER_ARCHIVE_MOUNT_FAILED,
+                l10n_util::GetStringUTF16(
+                    IDS_FILE_BROWSER_DEVICE_TYPE_UNDEFINED),
+                ASCIIToUTF16(MountErrorToString(error_code))));
+      }
+    }
+  }
+
   DispatchMountCompletedEvent(event_type, error_code, mount_info);
 }
 
@@ -224,7 +255,7 @@ void ExtensionFileBrowserEventRouter::DispatchFolderChangeEvent(
   }
 }
 
-void ExtensionFileBrowserEventRouter::DispatchMountEvent(
+void ExtensionFileBrowserEventRouter::DispatchDiskEvent(
     const chromeos::MountLibrary::Disk* disk, bool added) {
   if (!profile_) {
     NOTREACHED();
@@ -279,15 +310,18 @@ void ExtensionFileBrowserEventRouter::DispatchMountCompletedEvent(
     mount_info_value->SetString("sourceUrl", mount_info.source_path);
   }
 
+  FilePath relative_mount_path;
+  bool relative_mount_path_set = false;
   // If there were no error, add mountPath to the event.
   if (error_code == chromeos::MOUNT_ERROR_NONE) {
-    FilePath relative_mount_path;
     // Convert mount point path to relative path with the external file system
     // exposed within File API.
     if (FileManagerUtil::ConvertFileToRelativeFileSystemPath(profile_,
             FilePath(mount_info.mount_path),
             &relative_mount_path)) {
-      mount_info_value->SetString("mountPath", relative_mount_path.value());
+      mount_info_value->SetString("mountPath",
+                                  "/" + relative_mount_path.value());
+      relative_mount_path_set = true;
     }
   }
 
@@ -296,6 +330,12 @@ void ExtensionFileBrowserEventRouter::DispatchMountCompletedEvent(
   profile_->GetExtensionEventRouter()->DispatchEventToRenderers(
       extension_event_names::kOnFileBrowserMountCompleted, args_json, NULL,
       GURL());
+
+  if (relative_mount_path_set &&
+      mount_info.mount_type == chromeos::MOUNT_TYPE_DEVICE &&
+      event == chromeos::MountLibrary::MOUNTING) {
+    FileManagerUtil::ShowFullTabUrl(profile_, relative_mount_path);
+  }
 }
 
 void ExtensionFileBrowserEventRouter::OnDiskAdded(
@@ -305,12 +345,6 @@ void ExtensionFileBrowserEventRouter::OnDiskAdded(
     VLOG(1) << "Empty system path for " << disk->device_path();
     return;
   }
-  if (disk->is_parent()) {
-    if (!disk->has_media()) {
-      HideDeviceNotification(disk->system_path());
-      return;
-    }
-  }
 
   // If disk is not mounted yet, give it a try.
   if (disk->mount_path().empty()) {
@@ -321,41 +355,20 @@ void ExtensionFileBrowserEventRouter::OnDiskAdded(
                    chromeos::MOUNT_TYPE_DEVICE,
                    chromeos::MountPathOptions());  // Unused.
   }
+  DispatchDiskEvent(disk, true);
 }
 
 void ExtensionFileBrowserEventRouter::OnDiskRemoved(
     const chromeos::MountLibrary::Disk* disk) {
   VLOG(1) << "Disk removed: " << disk->device_path();
   HideDeviceNotification(disk->system_path());
-  MountPointMap::iterator iter = mounted_devices_.find(disk->device_path());
-  if (iter == mounted_devices_.end())
-    return;
 
+  if (!disk->mount_path().empty()) {
     chromeos::MountLibrary* lib =
        chromeos::CrosLibrary::Get()->GetMountLibrary();
-  // TODO(zelidrag): This for some reason does not work as advertized.
-  // we might need to clean up mount directory on FILE thread here as well.
-  lib->UnmountPath(disk->device_path().c_str());
-
-  DispatchMountEvent(disk, false);
-  mounted_devices_.erase(iter);
-}
-
-void ExtensionFileBrowserEventRouter::OnDiskChanged(
-    const chromeos::MountLibrary::Disk* disk) {
-  VLOG(1) << "Disk changed : " << disk->device_path();
-  if (!disk->mount_path().empty()) {
-    HideDeviceNotification(disk->system_path());
-    // Remember this mount point.
-    if (mounted_devices_.find(disk->device_path()) == mounted_devices_.end()) {
-      mounted_devices_.insert(
-          std::pair<std::string, std::string>(disk->device_path(),
-                                              disk->mount_path()));
-      DispatchMountEvent(disk, true);
-      HideDeviceNotification(disk->system_path());
-      FileManagerUtil::ShowFullTabUrl(profile_, FilePath(disk->mount_path()));
-    }
+    lib->UnmountPath(disk->mount_path().c_str());
   }
+  DispatchDiskEvent(disk, false);
 }
 
 void ExtensionFileBrowserEventRouter::OnDeviceAdded(
@@ -426,7 +439,6 @@ ExtensionFileBrowserEventRouter::NotificationMap::iterator
   return notifications_.end();
 }
 
-
 // ExtensionFileBrowserEventRouter::WatcherDelegate methods.
 ExtensionFileBrowserEventRouter::FileWatcherDelegate::FileWatcherDelegate(
     ExtensionFileBrowserEventRouter* router) : router_(router) {

chrome/browser/chromeos/extensions/file_browser_event_router.h

@@ -46,7 +46,6 @@ class ExtensionFileBrowserEventRouter
                            const chromeos::MountLibrary::Disk* disk) OVERRIDE;
   virtual void DeviceChanged(chromeos::MountLibraryEventType event,
                              const std::string& device_path) OVERRIDE;
-
   virtual void MountCompleted(chromeos::MountLibrary::MountEvent event_type,
       chromeos::MountError error_code,
       const chromeos::MountLibrary::MountPointInfo& mount_info) OVERRIDE;
@@ -56,7 +55,8 @@ class ExtensionFileBrowserEventRouter
       NotificationMap;
   typedef std::map<std::string, std::string> MountPointMap;
   typedef struct FileWatcherExtensions {
-    FileWatcherExtensions(const FilePath& path, const std::string& extension_id) {
+    FileWatcherExtensions(const FilePath& path,
+                          const std::string& extension_id) {
       file_watcher.reset(new base::files::FilePathWatcher());
       virtual_path = path;
       extensions.insert(extension_id);
@@ -87,7 +87,6 @@ class ExtensionFileBrowserEventRouter
   // USB mount event handlers.
   void OnDiskAdded(const chromeos::MountLibrary::Disk* disk);
   void OnDiskRemoved(const chromeos::MountLibrary::Disk* disk);
-  void OnDiskChanged(const chromeos::MountLibrary::Disk* disk);
   void OnDeviceAdded(const std::string& device_path);
   void OnDeviceRemoved(const std::string& device_path);
   void OnDeviceScanned(const std::string& device_path);
@@ -105,7 +104,7 @@ class ExtensionFileBrowserEventRouter
                                  const std::set<std::string>& extensions);
 
   // Sends filesystem changed extension message to all renderers.
-  void DispatchMountEvent(const chromeos::MountLibrary::Disk* disk, bool added);
+  void DispatchDiskEvent(const chromeos::MountLibrary::Disk* disk, bool added);
 
   void DispatchMountCompletedEvent(chromeos::MountLibrary::MountEvent event,
       chromeos::MountError error_code,

chrome/browser/resources/file_manager/js/file_manager.js

@@ -139,9 +139,6 @@ function FileManager(dialogDom, filesystem, rootEntries) {
   this.summarizeSelection_();
   this.updatePreview_();
 
-  chrome.fileBrowserPrivate.onDiskChanged.addListener(
-      this.onDiskChanged_.bind(this));
-
   this.refocus();
 
   // Pass all URLs to the metadata reader until we have a correct filter.
@@ -1685,9 +1682,18 @@ FileManager.prototype = {
           }
         }
       }
+
+      if (event.eventType == 'unmount' && event.status == 'success' &&
+          self.currentDirEntry_ &&
+          isParentPath(event.mountPath, self.currentDirEntry_.fullPath)) {
+        self.changeDirectory(getParentPath(event.mountPath));
+        return;
+      }
+
       // TODO(dgozman): rescan directory, only if it contains mounted points,
       // when mounts location will be decided.
-      this.rescanDirectory_();
+      if (event.status == 'success')
+        self.rescanDirectory_();
     });
   };
 
@@ -2440,23 +2446,6 @@ FileManager.prototype = {
       });
   };
 
-  /**
-   * Update the UI when a disk is mounted or unmounted.
-   *
-   * @param {string} path The path that has been mounted or unmounted.
-   */
-  FileManager.prototype.onDiskChanged_ = function(event) {
-    if (event.eventType == 'added') {
-      this.changeDirectory(event.volumeInfo.mountPath);
-    } else if (event.eventType == 'removed') {
-      if (this.currentDirEntry_ &&
-          isParentPath(event.volumeInfo.mountPath,
-                       this.currentDirEntry_.fullPath)) {
-        this.changeDirectory(getParentPath(event.volumeInfo.mountPath));
-      }
-    }
-  };
-
   /**
    * Rescan the current directory, refreshing the list.
    *

chrome/test/data/extensions/api_test/filebrowser_mount/test.html

@@ -2,7 +2,7 @@
 // These have to be sync'd with extension_file_browser_private_apitest.cc
 var expectedVolume1 = {
   devicePath: 'device_path1',
-  mountPath: 'mount_path1/',
+  mountPath: 'mount_path1',
   systemPath: 'system_path1',
   filePath: 'file_path1',
   deviceLabel: 'device_label1',
@@ -17,7 +17,7 @@ var expectedVolume1 = {
 
 var expectedVolume2 = {
   devicePath: 'device_path2',
-  mountPath: 'mount_path2/',
+  mountPath: 'mount_path2',
   systemPath: 'system_path2',
   filePath: 'file_path2',
   deviceLabel: 'device_label2',
@@ -32,7 +32,7 @@ var expectedVolume2 = {
 
 var expectedVolume3 = {
   devicePath: 'device_path3',
-  mountPath: 'mount_path3/',
+  mountPath: 'mount_path3',
   systemPath: 'system_path3',
   filePath: 'file_path3',
   deviceLabel: 'device_label3',