Extract OnDiskEvent and OnFormatEvent logic part from EventRouter to VolumeManager.

This CL extracts non-notification-related logics from EventRouter and move it
to the VolumeManager.
Now, EventRouter::OnDiskAdded, OnDiskRemoved, OnFormatStarted and
OnFormatCompleted focus on event routing, and VolumeManager has responsibility
of other stuff.

BUG=279276
TEST=Ran unit_tests and tested manually.

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

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

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

@@ -390,16 +390,7 @@ void EventRouter::RemoveFileWatch(const base::FilePath& local_path,
 
 void EventRouter::OnDiskEvent(DiskMountManager::DiskEvent event,
                               const DiskMountManager::Disk* disk) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  // Disregard hidden devices.
-  if (disk->is_hidden())
-    return;
-  if (event == DiskMountManager::DISK_ADDED) {
-    OnDiskAdded(disk);
-  } else if (event == DiskMountManager::DISK_REMOVED) {
-    OnDiskRemoved(disk);
-  }
+  // Disk event is dispatched by VolumeManager now. Do nothing.
 }
 
 void EventRouter::OnDeviceEvent(DiskMountManager::DeviceEvent event,
@@ -461,11 +452,7 @@ void EventRouter::OnMountEvent(
 void EventRouter::OnFormatEvent(DiskMountManager::FormatEvent event,
                                 chromeos::FormatError error_code,
                                 const std::string& device_path) {
-  if (event == DiskMountManager::FORMAT_STARTED) {
-    OnFormatStarted(device_path, error_code == chromeos::FORMAT_ERROR_NONE);
-  } else if (event == DiskMountManager::FORMAT_COMPLETED) {
-    OnFormatCompleted(device_path, error_code == chromeos::FORMAT_ERROR_NONE);
-  }
+  // Format event is dispatched by VolumeManager now. Do nothing.
 }
 
 void EventRouter::NetworkManagerChanged() {
@@ -762,44 +749,21 @@ void EventRouter::ShowRemovableDeviceInFileManager(
       base::Bind(&util::OpenRemovableDrive, mount_path));
 }
 
-void EventRouter::OnDiskAdded(const DiskMountManager::Disk* disk) {
+void EventRouter::OnDiskAdded(
+    const DiskMountManager::Disk& disk, bool mounting) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  VLOG(1) << "Disk added: " << disk->device_path();
-  if (disk->device_path().empty()) {
-    VLOG(1) << "Empty system path for " << disk->device_path();
-    return;
-  }
-
-  // If disk is not mounted yet and it has media and there is no policy
-  // forbidding external storage, give it a try.
-  if (disk->mount_path().empty() && disk->has_media() &&
-      !profile_->GetPrefs()->GetBoolean(prefs::kExternalStorageDisabled)) {
-    // Initiate disk mount operation. MountPath auto-detects the filesystem
-    // format if the second argument is empty. The third argument (mount label)
-    // is not used in a disk mount operation.
-    DiskMountManager::GetInstance()->MountPath(
-        disk->device_path(), std::string(), std::string(),
-        chromeos::MOUNT_TYPE_DEVICE);
-  } else {
-    // Either the disk was mounted or it has no media. In both cases we don't
-    // want the Scanning notification to persist.
+  if (!mounting) {
+    // If the disk is not being mounted, we don't want the Scanning
+    // notification to persist.
     notifications_->HideNotification(DesktopNotifications::DEVICE,
-                                     disk->system_path_prefix());
+                                     disk.system_path_prefix());
   }
 }
 
-void EventRouter::OnDiskRemoved(const DiskMountManager::Disk* disk) {
+void EventRouter::OnDiskRemoved(const DiskMountManager::Disk& disk) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  VLOG(1) << "Disk removed: " << disk->device_path();
-
-  if (!disk->mount_path().empty()) {
-    DiskMountManager::GetInstance()->UnmountPath(
-        disk->mount_path(),
-        chromeos::UNMOUNT_OPTIONS_LAZY,
-        DiskMountManager::UnmountPathCallback());
-  }
+  // Do nothing.
 }
 
 void EventRouter::OnDeviceAdded(const std::string& device_path) {
@@ -857,11 +821,6 @@ void EventRouter::OnFormatCompleted(const std::string& device_path,
         DesktopNotifications::FORMAT_SUCCESS,
         device_path,
         base::TimeDelta::FromSeconds(4));
-    // MountPath auto-detects filesystem format if second argument is empty.
-    // The third argument (mount label) is not used in a disk mount operation.
-    DiskMountManager::GetInstance()->MountPath(device_path, std::string(),
-                                               std::string(),
-                                               chromeos::MOUNT_TYPE_DEVICE);
   } else {
     notifications_->HideNotification(DesktopNotifications::FORMAT_START,
                                      device_path);

chrome/browser/chromeos/extensions/file_manager/event_router.h

@@ -106,19 +106,24 @@ class EventRouter
   virtual void OnFileSystemBeingUnmounted() OVERRIDE;
 
   // VolumeManagerObserver overrides.
+  virtual void OnDiskAdded(
+      const chromeos::disks::DiskMountManager::Disk& disk,
+      bool mounting) OVERRIDE;
+  virtual void OnDiskRemoved(
+      const chromeos::disks::DiskMountManager::Disk& disk) OVERRIDE;
   virtual void OnDeviceAdded(const std::string& device_path) OVERRIDE;
   virtual void OnDeviceRemoved(const std::string& device_path) OVERRIDE;
+  virtual void OnFormatStarted(
+      const std::string& device_path, bool success) OVERRIDE;
+  virtual void OnFormatCompleted(
+      const std::string& device_path, bool success) OVERRIDE;
 
  private:
   typedef std::map<base::FilePath, FileWatcher*> WatcherMap;
 
   // USB mount event handlers.
-  void OnDiskAdded(const chromeos::disks::DiskMountManager::Disk* disk);
-  void OnDiskRemoved(const chromeos::disks::DiskMountManager::Disk* disk);
   void OnDiskMounted(const chromeos::disks::DiskMountManager::Disk* disk);
   void OnDiskUnmounted(const chromeos::disks::DiskMountManager::Disk* disk);
-  void OnFormatStarted(const std::string& device_path, bool success);
-  void OnFormatCompleted(const std::string& device_path, bool success);
 
   // Called on change to kExternalStorageDisabled pref.
   void OnExternalStorageDisabledChanged();

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

-// Copyright (c) 2013 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "chrome/browser/chromeos/extensions/file_manager/event_router.h"
-
-#include "base/prefs/pref_service.h"
-#include "chrome/browser/chromeos/extensions/file_manager/file_browser_private_api.h"
-#include "chrome/browser/chromeos/extensions/file_manager/file_browser_private_api_factory.h"
-#include "chrome/browser/profiles/profile.h"
-#include "chrome/browser/ui/browser.h"
-#include "chrome/common/pref_names.h"
-#include "chrome/test/base/in_process_browser_test.h"
-#include "chromeos/disks/disk_mount_manager.h"
-#include "chromeos/disks/mock_disk_mount_manager.h"
-
-using testing::_;
-using chromeos::disks::DiskMountManager;
-using chromeos::disks::MockDiskMountManager;
-
-namespace file_manager {
-
-class FileManagerEventRouterBrowserTest : public InProcessBrowserTest {
- public:
-  // ExtensionApiTest override
-  virtual void SetUpInProcessBrowserTestFixture() OVERRIDE {
-    InProcessBrowserTest::SetUpInProcessBrowserTestFixture();
-
-    disk_mount_manager_mock_ = new MockDiskMountManager;
-    chromeos::disks::DiskMountManager::InitializeForTesting(
-        disk_mount_manager_mock_);
-    disk_mount_manager_mock_->SetupDefaultReplies();
-  }
-
-  MockDiskMountManager* disk_mount_manager_mock_;
-};
-
-IN_PROC_BROWSER_TEST_F(FileManagerEventRouterBrowserTest,
-                       ExternalStoragePolicyTest) {
-  FileBrowserPrivateAPI* file_browser =
-      FileBrowserPrivateAPIFactory::GetForProfile(browser()->profile());
-  EventRouter* event_router =
-      file_browser->event_router();
-
-  DiskMountManager::DiskEvent event = DiskMountManager::DISK_ADDED;
-  // Prepare a fake disk. All that matters here is that the mount point is empty
-  // but the device path is not so that it will exercise the path we care about.
-  DiskMountManager::Disk disk(
-      "fake_path", "", "X", "X", "X", "X", "X", "X", "X", "X", "X", "X",
-      chromeos::DEVICE_TYPE_USB, 1, false, false, true, false, false);
-
-  // First we set the policy to prevent storage mounting and check that the
-  // callback is not called.
-  browser()->profile()->GetPrefs()->SetBoolean(prefs::kExternalStorageDisabled,
-                                               true);
-
-  EXPECT_CALL(*disk_mount_manager_mock_, MountPath(_, _, _, _)).Times(0);
-
-  event_router->OnDiskEvent(event, &disk);
-
-  // Next we repeat but with the policy not active this time.
-  browser()->profile()->GetPrefs()->SetBoolean(prefs::kExternalStorageDisabled,
-                                               false);
-
-  EXPECT_CALL(*disk_mount_manager_mock_,
-              MountPath("fake_path", "", "", chromeos::MOUNT_TYPE_DEVICE))
-      .Times(1);
-
-  event_router->OnDiskEvent(event, &disk);
-}
-
-}  // namespace file_manager

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

@@ -9,8 +9,11 @@
 #include "base/logging.h"
 #include "base/memory/singleton.h"
 #include "base/path_service.h"
+#include "base/prefs/pref_service.h"
 #include "chrome/browser/chromeos/extensions/file_manager/volume_manager_factory.h"
 #include "chrome/browser/chromeos/extensions/file_manager/volume_manager_observer.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/common/pref_names.h"
 #include "chromeos/dbus/cros_disks_client.h"
 #include "chromeos/disks/disk_mount_manager.h"
 #include "content/public/browser/browser_thread.h"
@@ -55,8 +58,10 @@ VolumeInfo CreateVolumeInfoFromMountPointInfo(
 }  // namespace
 
 VolumeManager::VolumeManager(
+    Profile* profile,
     chromeos::disks::DiskMountManager* disk_mount_manager)
-    : disk_mount_manager_(disk_mount_manager) {
+    : profile_(profile),
+      disk_mount_manager_(disk_mount_manager) {
   DCHECK(disk_mount_manager);
 }
 
@@ -119,7 +124,58 @@ std::vector<VolumeInfo> VolumeManager::GetVolumeInfoList() const {
 void VolumeManager::OnDiskEvent(
     chromeos::disks::DiskMountManager::DiskEvent event,
     const chromeos::disks::DiskMountManager::Disk* disk) {
-  // TODO(hidehiko): Move the implementation from EventRouter.
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  // Disregard hidden devices.
+  if (disk->is_hidden())
+    return;
+
+  switch (event) {
+    case chromeos::disks::DiskMountManager::DISK_ADDED: {
+      if (disk->device_path().empty()) {
+        DVLOG(1) << "Empty system path for " << disk->device_path();
+        return;
+      }
+
+      bool mounting = false;
+      if (disk->mount_path().empty() && disk->has_media() &&
+          !profile_->GetPrefs()->GetBoolean(prefs::kExternalStorageDisabled)) {
+        // If disk is not mounted yet and it has media and there is no policy
+        // forbidding external storage, give it a try.
+        // Initiate disk mount operation. MountPath auto-detects the filesystem
+        // format if the second argument is empty. The third argument (mount
+        // label) is not used in a disk mount operation.
+        disk_mount_manager_->MountPath(
+            disk->device_path(), std::string(), std::string(),
+            chromeos::MOUNT_TYPE_DEVICE);
+        mounting = true;
+      }
+
+      // Notify to observers.
+      FOR_EACH_OBSERVER(VolumeManagerObserver, observers_,
+                        OnDiskAdded(*disk, mounting));
+      return;
+    }
+
+    case chromeos::disks::DiskMountManager::DISK_REMOVED:
+      // If the disk is already mounted, unmount it.
+      if (!disk->mount_path().empty()) {
+        disk_mount_manager_->UnmountPath(
+            disk->mount_path(),
+            chromeos::UNMOUNT_OPTIONS_LAZY,
+            chromeos::disks::DiskMountManager::UnmountPathCallback());
+      }
+
+      // Notify to observers.
+      FOR_EACH_OBSERVER(VolumeManagerObserver, observers_,
+                        OnDiskRemoved(*disk));
+      return;
+
+    case chromeos::disks::DiskMountManager::DISK_CHANGED:
+      DVLOG(1) << "Ignore CHANGED event.";
+      return;
+  }
+  NOTREACHED();
 }
 
 void VolumeManager::OnDeviceEvent(
@@ -155,7 +211,36 @@ void VolumeManager::OnFormatEvent(
     chromeos::disks::DiskMountManager::FormatEvent event,
     chromeos::FormatError error_code,
     const std::string& device_path) {
-  // TODO(hidehiko): Move the implementation from EventRouter.
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  DVLOG(1) << "OnDeviceEvent: " << event << ", " << error_code
+           << ", " << device_path;
+
+  switch (event) {
+    case chromeos::disks::DiskMountManager::FORMAT_STARTED:
+      FOR_EACH_OBSERVER(
+          VolumeManagerObserver, observers_,
+          OnFormatStarted(device_path,
+                          error_code == chromeos::FORMAT_ERROR_NONE));
+      return;
+    case chromeos::disks::DiskMountManager::FORMAT_COMPLETED:
+      if (error_code == chromeos::FORMAT_ERROR_NONE) {
+        // If format is completed successfully, try to mount the device.
+        // MountPath auto-detects filesystem format if second argument is
+        // empty. The third argument (mount label) is not used in a disk mount
+        // operation.
+        disk_mount_manager_->MountPath(
+            device_path, std::string(), std::string(),
+            chromeos::MOUNT_TYPE_DEVICE);
+      }
+
+      FOR_EACH_OBSERVER(
+          VolumeManagerObserver, observers_,
+          OnFormatCompleted(device_path,
+                            error_code == chromeos::FORMAT_ERROR_NONE));
+
+      return;
+  }
+  NOTREACHED();
 }
 
 }  // namespace file_manager

chrome/browser/chromeos/extensions/file_manager/volume_manager.h

@@ -11,6 +11,8 @@
 #include "chromeos/disks/disk_mount_manager.h"
 #include "components/browser_context_keyed_service/browser_context_keyed_service.h"
 
+class Profile;
+
 namespace content {
 class BrowserContext;
 }  // namespace content
@@ -57,8 +59,8 @@ struct VolumeInfo {
 class VolumeManager : public BrowserContextKeyedService,
                       public chromeos::disks::DiskMountManager::Observer {
  public:
-  explicit VolumeManager(
-      chromeos::disks::DiskMountManager* disk_mount_manager);
+  VolumeManager(Profile* profile,
+                chromeos::disks::DiskMountManager* disk_mount_manager);
   virtual ~VolumeManager();
 
   // Returns the instance corresponding to the |context|.
@@ -98,8 +100,9 @@ class VolumeManager : public BrowserContextKeyedService,
       const std::string& device_path) OVERRIDE;
 
  private:
-  ObserverList<VolumeManagerObserver> observers_;
+  Profile* profile_;
   chromeos::disks::DiskMountManager* disk_mount_manager_;
+  ObserverList<VolumeManagerObserver> observers_;
   DISALLOW_COPY_AND_ASSIGN(VolumeManager);
 };
 

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

@@ -8,6 +8,7 @@
 #include "base/memory/singleton.h"
 #include "chrome/browser/chromeos/extensions/file_manager/volume_manager.h"
 #include "chrome/browser/profiles/incognito_helpers.h"
+#include "chrome/browser/profiles/profile.h"
 #include "chromeos/disks/disk_mount_manager.h"
 #include "components/browser_context_keyed_service/browser_context_dependency_manager.h"
 
@@ -39,7 +40,8 @@ bool VolumeManagerFactory::ServiceIsNULLWhileTesting() const {
 BrowserContextKeyedService* VolumeManagerFactory::BuildServiceInstanceFor(
     content::BrowserContext* profile) const {
   VolumeManager* instance =
-      new VolumeManager(chromeos::disks::DiskMountManager::GetInstance());
+      new VolumeManager(Profile::FromBrowserContext(profile),
+                        chromeos::disks::DiskMountManager::GetInstance());
   instance->Initialize();
   return instance;
 }

chrome/browser/chromeos/extensions/file_manager/volume_manager_observer.h

@@ -7,6 +7,8 @@
 
 #include <string>
 
+#include "chromeos/disks/disk_mount_manager.h"
+
 namespace file_manager {
 
 struct VolumeInfo;
@@ -16,11 +18,27 @@ class VolumeManagerObserver {
  public:
   virtual ~VolumeManagerObserver() {}
 
+  // Fired when a new disk is added.
+  virtual void OnDiskAdded(
+      const chromeos::disks::DiskMountManager::Disk& disk, bool mounting) = 0;
+
+  // Fired when a disk is removed.
+  virtual void OnDiskRemoved(
+      const chromeos::disks::DiskMountManager::Disk& disk) = 0;
+
   // Fired when a new device is added.
   virtual void OnDeviceAdded(const std::string& device_path) = 0;
 
-  // Fired when a new device is removed.
+  // Fired when a device is removed.
   virtual void OnDeviceRemoved(const std::string& device_path) = 0;
+
+  // Fired when formatting a device is started (or failed to start).
+  virtual void OnFormatStarted(
+      const std::string& device_path, bool success) = 0;
+
+  // Fired when formatting a device is completed (or terminated on error).
+  virtual void OnFormatCompleted(
+      const std::string& device_path, bool success) = 0;
 };
 
 }  // namespace file_manager

chrome/chrome_tests.gypi

@@ -1221,7 +1221,6 @@
         'browser/chromeos/drive/test_util.cc',
         'browser/chromeos/drive/test_util.h',
         'browser/chromeos/extensions/echo_private_apitest.cc',
-        'browser/chromeos/extensions/file_manager/event_router_browsertest.cc',
         'browser/chromeos/extensions/file_manager/file_browser_handler_api_test.cc',
         'browser/chromeos/extensions/file_manager/file_browser_private_apitest.cc',
         'browser/chromeos/extensions/info_private_apitest.cc',

chrome/chrome_tests_unit.gypi

@@ -657,6 +657,7 @@
         'browser/chromeos/drive/webkit_file_stream_reader_impl_unittest.cc',
         'browser/chromeos/drive/write_on_cache_file_unittest.cc',
         'browser/chromeos/extensions/default_app_order_unittest.cc',
+        'browser/chromeos/extensions/file_manager/volume_manager_unittest.cc',
         'browser/chromeos/file_manager/desktop_notifications_unittest.cc',
         'browser/chromeos/file_manager/file_tasks_unittest.cc',
         'browser/chromeos/file_manager/file_watcher_unittest.cc',