Storage Monitor: Change the StorageMonitorCros to live on a single thread.

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

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

5 files changed, 73 insertions, 55 deletions

diff --git a/chrome/browser/chromeos/chrome_browser_main_chromeos.cc b/chrome/browser/chromeos/chrome_browser_main_chromeos.cc
index 0c0fa09..d628d90 100644
--- a/chrome/browser/chromeos/chrome_browser_main_chromeos.cc
+++ b/chrome/browser/chromeos/chrome_browser_main_chromeos.cc
@@ -569,7 +569,7 @@ void ChromeBrowserMainPartsChromeos::PreProfileInit() {
                                   &tracker_);
 #endif
 
-  storage_monitor_ = new StorageMonitorCros();
+  storage_monitor_.reset(new StorageMonitorCros());
 
   // In Aura builds this will initialize ash::Shell.
   ChromeBrowserMainPartsLinux::PreProfileInit();
diff --git a/chrome/browser/chromeos/chrome_browser_main_chromeos.h b/chrome/browser/chromeos/chrome_browser_main_chromeos.h
index 3f18070..e094246 100644
--- a/chrome/browser/chromeos/chrome_browser_main_chromeos.h
+++ b/chrome/browser/chromeos/chrome_browser_main_chromeos.h
@@ -5,7 +5,6 @@
 #ifndef CHROME_BROWSER_CHROMEOS_CHROME_BROWSER_MAIN_CHROMEOS_H_
 #define CHROME_BROWSER_CHROMEOS_CHROME_BROWSER_MAIN_CHROMEOS_H_
 
-#include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "chrome/browser/chrome_browser_main_linux.h"
 #include "chrome/browser/chromeos/version_loader.h"
@@ -86,7 +85,7 @@ class ChromeBrowserMainPartsChromeos : public ChromeBrowserMainPartsLinux {
   scoped_ptr<content::PowerSaveBlocker> retail_mode_power_save_blocker_;
   scoped_ptr<UserActivityNotifier> user_activity_notifier_;
   scoped_ptr<VideoActivityNotifier> video_activity_notifier_;
-  scoped_refptr<StorageMonitorCros> storage_monitor_;
+  scoped_ptr<StorageMonitorCros> storage_monitor_;
   scoped_ptr<system::AutomaticRebootManager> automatic_reboot_manager_;
   scoped_ptr<IdleActionWarningObserver> idle_action_warning_observer_;
 
diff --git a/chrome/browser/storage_monitor/storage_monitor_chromeos.cc b/chrome/browser/storage_monitor/storage_monitor_chromeos.cc
index 9339aa6..bd8e091 100644
--- a/chrome/browser/storage_monitor/storage_monitor_chromeos.cc
+++ b/chrome/browser/storage_monitor/storage_monitor_chromeos.cc
@@ -102,12 +102,21 @@ bool GetDeviceInfo(const std::string& source_path,
   return true;
 }
 
+// Returns whether the mount point in |mount_info| is a media device or not.
+bool CheckMountedPathOnFileThread(
+    const disks::DiskMountManager::MountPointInfo& mount_info) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE));
+  return chrome::MediaStorageUtil::HasDcim(
+      base::FilePath(mount_info.mount_path));
+}
+
 }  // namespace
 
 using content::BrowserThread;
 using chrome::StorageInfo;
 
-StorageMonitorCros::StorageMonitorCros() {
+StorageMonitorCros::StorageMonitorCros()
+    : ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {
 }
 
 StorageMonitorCros::~StorageMonitorCros() {
@@ -124,7 +133,7 @@ StorageMonitorCros::~StorageMonitorCros() {
 void StorageMonitorCros::Init() {
   DCHECK(disks::DiskMountManager::GetInstance());
   disks::DiskMountManager::GetInstance()->AddObserver(this);
-  CheckExistingMountPointsOnUIThread();
+  CheckExistingMountPoints();
 
   if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kTestType)) {
     scoped_refptr<base::MessageLoopProxy> loop_proxy;
@@ -135,16 +144,17 @@ void StorageMonitorCros::Init() {
   }
 }
 
-void StorageMonitorCros::CheckExistingMountPointsOnUIThread() {
+void StorageMonitorCros::CheckExistingMountPoints() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   const disks::DiskMountManager::MountPointMap& mount_point_map =
       disks::DiskMountManager::GetInstance()->mount_points();
   for (disks::DiskMountManager::MountPointMap::const_iterator it =
            mount_point_map.begin(); it != mount_point_map.end(); ++it) {
-    BrowserThread::PostTask(
+    BrowserThread::PostTaskAndReplyWithResult(
         BrowserThread::FILE, FROM_HERE,
-        base::Bind(&StorageMonitorCros::CheckMountedPathOnFileThread, this,
-                   it->second));
+        base::Bind(&CheckMountedPathOnFileThread, it->second),
+        base::Bind(&StorageMonitorCros::AddMountedPath,
+                   weak_ptr_factory_.GetWeakPtr(), it->second));
   }
 }
 
@@ -180,10 +190,11 @@ void StorageMonitorCros::OnMountEvent(
         return;
       }
 
-      BrowserThread::PostTask(
+      BrowserThread::PostTaskAndReplyWithResult(
           BrowserThread::FILE, FROM_HERE,
-          base::Bind(&StorageMonitorCros::CheckMountedPathOnFileThread, this,
-                     mount_info));
+          base::Bind(&CheckMountedPathOnFileThread, mount_info),
+          base::Bind(&StorageMonitorCros::AddMountedPath,
+                     weak_ptr_factory_.GetWeakPtr(), mount_info));
       break;
     }
     case disks::DiskMountManager::UNMOUNTING: {
@@ -268,20 +279,7 @@ void StorageMonitorCros::EjectDevice(
                        base::Bind(NotifyUnmountResult, callback));
 }
 
-void StorageMonitorCros::CheckMountedPathOnFileThread(
-    const disks::DiskMountManager::MountPointInfo& mount_info) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-
-  bool has_dcim =
-      chrome::MediaStorageUtil::HasDcim(base::FilePath(mount_info.mount_path));
-
-  BrowserThread::PostTask(
-      BrowserThread::UI, FROM_HERE,
-      base::Bind(&StorageMonitorCros::AddMountedPathOnUIThread, this,
-                 mount_info, has_dcim));
-}
-
-void StorageMonitorCros::AddMountedPathOnUIThread(
+void StorageMonitorCros::AddMountedPath(
     const disks::DiskMountManager::MountPointInfo& mount_info, bool has_dcim) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
diff --git a/chrome/browser/storage_monitor/storage_monitor_chromeos.h b/chrome/browser/storage_monitor/storage_monitor_chromeos.h
index c4ac711..68daa39 100644
--- a/chrome/browser/storage_monitor/storage_monitor_chromeos.h
+++ b/chrome/browser/storage_monitor/storage_monitor_chromeos.h
@@ -4,6 +4,7 @@
 
 // chromeos::StorageMonitorCros listens for mount point changes and notifies
 // listeners about the addition and deletion of media devices.
+// This class lives on the UI thread.
 
 #ifndef CHROME_BROWSER_STORAGE_MONITOR_STORAGE_MONITOR_CHROMEOS_H_
 #define CHROME_BROWSER_STORAGE_MONITOR_STORAGE_MONITOR_CHROMEOS_H_
@@ -17,7 +18,8 @@
 
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
-#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "chrome/browser/storage_monitor/storage_monitor.h"
 #include "chromeos/disks/disk_mount_manager.h"
 
@@ -27,18 +29,20 @@ class MediaTransferProtocolDeviceObserverLinux;
 
 namespace chromeos {
 
-class StorageMonitorCros
-    : public chrome::StorageMonitor,
-      public base::RefCountedThreadSafe<StorageMonitorCros>,
-      public disks::DiskMountManager::Observer {
+class StorageMonitorCros : public chrome::StorageMonitor,
+                           public disks::DiskMountManager::Observer {
  public:
-  // Should only be called by browser start up code. Use GetInstance() instead.
+  // Should only be called by browser start up code.
+  // Use StorageMonitor::GetInstance() instead.
   StorageMonitorCros();
+  virtual ~StorageMonitorCros();
 
   // Sets up disk listeners and issues notifications for any discovered
   // mount points. Sets up MTP manager and listeners.
   void Init();
 
+ protected:
+  // disks::DiskMountManager::Observer implementation.
   virtual void OnDiskEvent(disks::DiskMountManager::DiskEvent event,
                            const disks::DiskMountManager::Disk* disk) OVERRIDE;
   virtual void OnDeviceEvent(disks::DiskMountManager::DeviceEvent event,
@@ -51,48 +55,36 @@ class StorageMonitorCros
                              FormatError error_code,
                              const std::string& device_path) OVERRIDE;
 
-  // Finds the device that contains |path| and populates |device_info|.
-  // Returns false if unable to find the device.
+  // StorageMonitor implementation.
   virtual bool GetStorageInfoForPath(
       const base::FilePath& path,
       chrome::StorageInfo* device_info) const OVERRIDE;
-
   virtual void EjectDevice(
       const std::string& device_id,
       base::Callback<void(EjectStatus)> callback) OVERRIDE;
 
  private:
-  friend class base::RefCountedThreadSafe<StorageMonitorCros>;
-
   // Mapping of mount path to removable mass storage info.
   typedef std::map<std::string, chrome::StorageInfo> MountMap;
 
-  // Private to avoid code deleting the object.
-  virtual ~StorageMonitorCros();
-
-  // Checks existing mount points map for media devices. For each mount point,
-  // call CheckMountedPathOnFileThread() below.
-  void CheckExistingMountPointsOnUIThread();
-
-  // Checks if the mount point in |mount_info| is a media device. If it is,
-  // then continue with AddMountedPathOnUIThread() below.
-  void CheckMountedPathOnFileThread(
-      const disks::DiskMountManager::MountPointInfo& mount_info);
+  // Helper method that checks existing mount points to see if they are media
+  // devices. Eventually calls AddMountedPath for all mount points.
+  void CheckExistingMountPoints();
 
   // Adds the mount point in |mount_info| to |mount_map_| and send a media
   // device attach notification. |has_dcim| is true if the attached device has
   // a DCIM folder.
-  void AddMountedPathOnUIThread(
-      const disks::DiskMountManager::MountPointInfo& mount_info,
-      bool has_dcim);
+  void AddMountedPath(const disks::DiskMountManager::MountPointInfo& mount_info,
+                      bool has_dcim);
 
   // Mapping of relevant mount points and their corresponding mount devices.
-  // Only accessed on the UI thread.
   MountMap mount_map_;
 
   scoped_ptr<chrome::MediaTransferProtocolDeviceObserverLinux>
       media_transfer_protocol_device_observer_;
 
+  base::WeakPtrFactory<StorageMonitorCros> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(StorageMonitorCros);
 };
 
diff --git a/chrome/browser/storage_monitor/storage_monitor_chromeos_unittest.cc b/chrome/browser/storage_monitor/storage_monitor_chromeos_unittest.cc
index 63e7e4f..6cd53cb 100644
--- a/chrome/browser/storage_monitor/storage_monitor_chromeos_unittest.cc
+++ b/chrome/browser/storage_monitor/storage_monitor_chromeos_unittest.cc
@@ -57,6 +57,35 @@ std::string GetDCIMDeviceId(const std::string& unique_id) {
       chrome::kFSUniqueIdPrefix + unique_id);
 }
 
+// A test version of StorageMonitorCros that exposes protected methods to tests.
+class TestStorageMonitorCros : public StorageMonitorCros {
+ public:
+  TestStorageMonitorCros() {}
+
+  virtual ~TestStorageMonitorCros() {}
+
+  virtual void OnMountEvent(
+      disks::DiskMountManager::MountEvent event,
+      MountError error_code,
+      const disks::DiskMountManager::MountPointInfo& mount_info) OVERRIDE {
+    StorageMonitorCros::OnMountEvent(event, error_code, mount_info);
+  }
+
+  virtual bool GetStorageInfoForPath(
+      const base::FilePath& path,
+      chrome::StorageInfo* device_info) const OVERRIDE {
+    return StorageMonitorCros::GetStorageInfoForPath(path, device_info);
+  }
+  virtual void EjectDevice(
+      const std::string& device_id,
+      base::Callback<void(EjectStatus)> callback) OVERRIDE {
+    StorageMonitorCros::EjectDevice(device_id, callback);
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestStorageMonitorCros);
+};
+
 // Wrapper class to test StorageMonitorCros.
 class StorageMonitorCrosTest : public testing::Test {
  public:
@@ -100,7 +129,7 @@ class StorageMonitorCrosTest : public testing::Test {
 
   MessageLoop ui_loop_;
 
-  scoped_refptr<StorageMonitorCros> monitor_;
+  scoped_ptr<TestStorageMonitorCros> monitor_;
 
   // Owned by DiskMountManager.
   disks::MockDiskMountManager* disk_mount_manager_mock_;
@@ -141,13 +170,13 @@ void StorageMonitorCrosTest::SetUp() {
   mock_storage_observer_.reset(new chrome::MockRemovableStorageObserver);
 
   // Initialize the test subject.
-  monitor_ = new StorageMonitorCros();
+  monitor_.reset(new TestStorageMonitorCros());
   monitor_->AddObserver(mock_storage_observer_.get());
 }
 
 void StorageMonitorCrosTest::TearDown() {
   monitor_->RemoveObserver(mock_storage_observer_.get());
-  monitor_ = NULL;
+  monitor_.reset();
 
   disk_mount_manager_mock_ = NULL;
   DiskMountManager::Shutdown();