Regularize ownerships and lifecycle for storage monitor platform classes.

This change regularizes the construction lifecycle of the StorageMonitor implementations so that they always exist for the construction of the Profile, but don't issue any notifications until after that construction is complete. This allows profile-based listeners to be sure they're synchronized with the monitor.

It also regularizes ownership of all storage-monitor-related objects. The base platform-specific object is owned by the ChromeBrowserMain object, and owns all subsidiary platform-specific objects needed to monitor and/or deal with volumes of various types (i.e. MTP).

R=vandebo@chromium.org
BUG=None


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=187332

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

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

13 files changed, 108 insertions, 58 deletions

diff --git a/chrome/browser/chrome_browser_main_linux.cc b/chrome/browser/chrome_browser_main_linux.cc
index 4f5c48a..c15d05d 100644
--- a/chrome/browser/chrome_browser_main_linux.cc
+++ b/chrome/browser/chrome_browser_main_linux.cc
@@ -4,11 +4,6 @@
 
 #include "chrome/browser/chrome_browser_main_linux.h"
 
-#include "base/message_loop_proxy.h"
-#include "chrome/browser/storage_monitor/media_transfer_protocol_device_observer_linux.h"
-#include "chrome/common/chrome_switches.h"
-#include "device/media_transfer_protocol/media_transfer_protocol_manager.h"
-
 #if !defined(OS_CHROMEOS)
 #include "chrome/browser/storage_monitor/storage_monitor_linux.h"
 #include "content/public/browser/browser_thread.h"
@@ -21,6 +16,7 @@
 #include "base/linux_util.h"
 #include "base/prefs/pref_service.h"
 #include "chrome/app/breakpad_linux.h"
+#include "chrome/common/chrome_switches.h"
 #include "chrome/common/env_vars.h"
 #include "chrome/common/pref_names.h"
 
@@ -108,13 +104,10 @@ bool IsCrashReportingEnabled(const PrefService* local_state) {
 
 ChromeBrowserMainPartsLinux::ChromeBrowserMainPartsLinux(
     const content::MainFunctionParams& parameters)
-    : ChromeBrowserMainPartsPosix(parameters),
-      initialized_media_transfer_protocol_manager_(false) {
+    : ChromeBrowserMainPartsPosix(parameters) {
 }
 
 ChromeBrowserMainPartsLinux::~ChromeBrowserMainPartsLinux() {
-  if (initialized_media_transfer_protocol_manager_)
-    device::MediaTransferProtocolManager::Shutdown();
 }
 
 void ChromeBrowserMainPartsLinux::PreProfileInit() {
@@ -134,30 +127,15 @@ void ChromeBrowserMainPartsLinux::PreProfileInit() {
 #if !defined(OS_CHROMEOS)
   const base::FilePath kDefaultMtabPath("/etc/mtab");
   storage_monitor_ = new chrome::StorageMonitorLinux(kDefaultMtabPath);
-  storage_monitor_->Init();
 #endif
 
-  if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kTestType)) {
-    scoped_refptr<base::MessageLoopProxy> loop_proxy;
-#if !defined(OS_CHROMEOS)
-    loop_proxy = content::BrowserThread::GetMessageLoopProxyForThread(
-        content::BrowserThread::FILE);
-#endif
-    device::MediaTransferProtocolManager::Initialize(loop_proxy);
-    initialized_media_transfer_protocol_manager_ = true;
-  }
-
   ChromeBrowserMainPartsPosix::PreProfileInit();
 }
 
 void ChromeBrowserMainPartsLinux::PostProfileInit() {
-  // TODO(gbillock): Make this owned by StorageMonitorLinux.
-  if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kTestType)) {
-    media_transfer_protocol_device_observer_.reset(
-        new chrome::MediaTransferProtocolDeviceObserverLinux());
-    media_transfer_protocol_device_observer_->SetNotifications(
-      chrome::StorageMonitor::GetInstance()->receiver());
-  }
+#if !defined(OS_CHROMEOS)
+  storage_monitor_->Init();
+#endif
 
   ChromeBrowserMainPartsPosix::PostProfileInit();
 }
@@ -170,7 +148,5 @@ void ChromeBrowserMainPartsLinux::PostMainMessageLoopRun() {
   storage_monitor_ = NULL;
 #endif
 
-  media_transfer_protocol_device_observer_.reset();
-
   ChromeBrowserMainPartsPosix::PostMainMessageLoopRun();
 }
diff --git a/chrome/browser/chrome_browser_main_linux.h b/chrome/browser/chrome_browser_main_linux.h
index 44e074f..e9f2c43 100644
--- a/chrome/browser/chrome_browser_main_linux.h
+++ b/chrome/browser/chrome_browser_main_linux.h
@@ -17,10 +17,6 @@ class StorageMonitorLinux;
 }
 #endif
 
-namespace chrome {
-class MediaTransferProtocolDeviceObserverLinux;
-}
-
 class ChromeBrowserMainPartsLinux : public ChromeBrowserMainPartsPosix {
  public:
   explicit ChromeBrowserMainPartsLinux(
@@ -36,9 +32,6 @@ class ChromeBrowserMainPartsLinux : public ChromeBrowserMainPartsPosix {
 #if !defined(OS_CHROMEOS)
   scoped_refptr<chrome::StorageMonitorLinux> storage_monitor_;
 #endif
-  scoped_ptr<chrome::MediaTransferProtocolDeviceObserverLinux>
-      media_transfer_protocol_device_observer_;
-  bool initialized_media_transfer_protocol_manager_;
 
   DISALLOW_COPY_AND_ASSIGN(ChromeBrowserMainPartsLinux);
 };
diff --git a/chrome/browser/chrome_browser_main_mac.h b/chrome/browser/chrome_browser_main_mac.h
index f822012b..d3cb484 100644
--- a/chrome/browser/chrome_browser_main_mac.h
+++ b/chrome/browser/chrome_browser_main_mac.h
@@ -9,7 +9,6 @@
 #include "base/memory/ref_counted.h"
 
 namespace chrome {
-class ImageCaptureDeviceManager;
 class StorageMonitorMac;
 }
 
@@ -23,6 +22,7 @@ class ChromeBrowserMainPartsMac : public ChromeBrowserMainPartsPosix {
   virtual void PreEarlyInitialization() OVERRIDE;
   virtual void PreMainMessageLoopStart() OVERRIDE;
   virtual void PreProfileInit() OVERRIDE;
+  virtual void PostProfileInit() OVERRIDE;
 
   // Perform platform-specific work that needs to be done after the main event
   // loop has ended. The embedder must be sure to call this.
@@ -31,8 +31,6 @@ class ChromeBrowserMainPartsMac : public ChromeBrowserMainPartsPosix {
  private:
   scoped_refptr<chrome::StorageMonitorMac> storage_monitor_;
 
-  scoped_ptr<chrome::ImageCaptureDeviceManager> image_capture_device_manager_;
-
   DISALLOW_COPY_AND_ASSIGN(ChromeBrowserMainPartsMac);
 };
 
diff --git a/chrome/browser/chrome_browser_main_mac.mm b/chrome/browser/chrome_browser_main_mac.mm
index 31613da..cd32ec4 100644
--- a/chrome/browser/chrome_browser_main_mac.mm
+++ b/chrome/browser/chrome_browser_main_mac.mm
@@ -22,7 +22,6 @@
 #include "chrome/browser/mac/keychain_reauthorize.h"
 #import "chrome/browser/mac/keystone_glue.h"
 #include "chrome/browser/metrics/metrics_service.h"
-#include "chrome/browser/storage_monitor/image_capture_device_manager.h"
 #include "chrome/browser/storage_monitor/storage_monitor_mac.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
@@ -283,16 +282,16 @@ void ChromeBrowserMainPartsMac::PreMainMessageLoopStart() {
 
 void ChromeBrowserMainPartsMac::PreProfileInit() {
   storage_monitor_ = new chrome::StorageMonitorMac();
-  // TODO(gbillock): Make the ImageCapture manager owned by StorageMonitorMac.
-  if (base::mac::IsOSLionOrLater()) {
-    image_capture_device_manager_.reset(new chrome::ImageCaptureDeviceManager);
-    image_capture_device_manager_->SetNotifications(
-        storage_monitor_->receiver());
-  }
 
   ChromeBrowserMainPartsPosix::PreProfileInit();
 }
 
+void ChromeBrowserMainPartsMac::PostProfileInit() {
+  storage_monitor_->Init();
+
+  ChromeBrowserMainPartsPosix::PostProfileInit();
+}
+
 void ChromeBrowserMainPartsMac::DidEndMainMessageLoop() {
   AppController* appController = [NSApp delegate];
   [appController didEndMainMessageLoop];
diff --git a/chrome/browser/chromeos/chrome_browser_main_chromeos.cc b/chrome/browser/chromeos/chrome_browser_main_chromeos.cc
index d2f0125..ec63844 100644
--- a/chrome/browser/chromeos/chrome_browser_main_chromeos.cc
+++ b/chrome/browser/chromeos/chrome_browser_main_chromeos.cc
@@ -545,6 +545,8 @@ void ChromeBrowserMainPartsChromeos::PreProfileInit() {
                                   &tracker_);
 #endif
 
+  storage_monitor_ = new StorageMonitorCros();
+
   // In Aura builds this will initialize ash::Shell.
   ChromeBrowserMainPartsLinux::PreProfileInit();
 }
@@ -620,7 +622,7 @@ void ChromeBrowserMainPartsChromeos::PostProfileInit() {
   }
   chromeos::accessibility::Initialize();
 
-  storage_monitor_ = new StorageMonitorCros();
+  storage_monitor_->Init();
 
   // Initialize the network portal detector for Chrome OS. The network
   // portal detector starts to listen for notifications from
diff --git a/chrome/browser/storage_monitor/media_transfer_protocol_device_observer_linux.cc b/chrome/browser/storage_monitor/media_transfer_protocol_device_observer_linux.cc
index beae982..6f841c9 100644
--- a/chrome/browser/storage_monitor/media_transfer_protocol_device_observer_linux.cc
+++ b/chrome/browser/storage_monitor/media_transfer_protocol_device_observer_linux.cc
@@ -127,6 +127,7 @@ MediaTransferProtocolDeviceObserverLinux()
 
   device::MediaTransferProtocolManager* mtp_manager =
       device::MediaTransferProtocolManager::GetInstance();
+  DCHECK(mtp_manager);
   mtp_manager->AddObserver(this);
   EnumerateStorages();
 }
@@ -137,8 +138,6 @@ MediaTransferProtocolDeviceObserverLinux(
     GetStorageInfoFunc get_storage_info_func)
     : get_storage_info_func_(get_storage_info_func),
       notifications_(NULL) {
-  // In unit tests, we don't have a media transfer protocol manager.
-  DCHECK(!device::MediaTransferProtocolManager::GetInstance());
   DCHECK(!g_mtp_device_observer);
   g_mtp_device_observer = this;
 }
diff --git a/chrome/browser/storage_monitor/storage_monitor.h b/chrome/browser/storage_monitor/storage_monitor.h
index 85b1f1b..35247fb 100644
--- a/chrome/browser/storage_monitor/storage_monitor.h
+++ b/chrome/browser/storage_monitor/storage_monitor.h
@@ -25,6 +25,11 @@ class TransientDeviceIds;
 
 // Base class for platform-specific instances watching for removable storage
 // attachments/detachments.
+// Lifecycle contracts: This class is created by ChromeBrowserMain
+// implementations before the profile is initialized, so listeners can be
+// created during profile construction. The platform-specific initialization,
+// which can lead to calling registered listeners with notifications of
+// attached volumes, will happen after profile construction.
 class StorageMonitor {
  public:
   // This interface is provided to generators of storage notifications.
diff --git a/chrome/browser/storage_monitor/storage_monitor_chromeos.cc b/chrome/browser/storage_monitor/storage_monitor_chromeos.cc
index 7b7998f..96fb4d1 100644
--- a/chrome/browser/storage_monitor/storage_monitor_chromeos.cc
+++ b/chrome/browser/storage_monitor/storage_monitor_chromeos.cc
@@ -6,6 +6,7 @@
 
 #include "chrome/browser/storage_monitor/storage_monitor_chromeos.h"
 
+#include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/logging.h"
 #include "base/stl_util.h"
@@ -13,8 +14,11 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/storage_monitor/media_storage_util.h"
+#include "chrome/browser/storage_monitor/media_transfer_protocol_device_observer_linux.h"
 #include "chrome/browser/storage_monitor/removable_device_constants.h"
+#include "chrome/common/chrome_switches.h"
 #include "content/public/browser/browser_thread.h"
+#include "device/media_transfer_protocol/media_transfer_protocol_manager.h"
 
 namespace chromeos {
 
@@ -104,18 +108,34 @@ using content::BrowserThread;
 using chrome::StorageInfo;
 
 StorageMonitorCros::StorageMonitorCros() {
-  DCHECK(disks::DiskMountManager::GetInstance());
-  disks::DiskMountManager::GetInstance()->AddObserver(this);
-  CheckExistingMountPointsOnUIThread();
 }
 
 StorageMonitorCros::~StorageMonitorCros() {
+  if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kTestType)) {
+    device::MediaTransferProtocolManager::Shutdown();
+  }
+
   disks::DiskMountManager* manager = disks::DiskMountManager::GetInstance();
   if (manager) {
     manager->RemoveObserver(this);
   }
 }
 
+void StorageMonitorCros::Init() {
+  DCHECK(disks::DiskMountManager::GetInstance());
+  disks::DiskMountManager::GetInstance()->AddObserver(this);
+  CheckExistingMountPointsOnUIThread();
+
+  if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kTestType)) {
+    scoped_refptr<base::MessageLoopProxy> loop_proxy;
+    device::MediaTransferProtocolManager::Initialize(loop_proxy);
+
+    media_transfer_protocol_device_observer_.reset(
+        new chrome::MediaTransferProtocolDeviceObserverLinux());
+    media_transfer_protocol_device_observer_->SetNotifications(receiver());
+  }
+}
+
 void StorageMonitorCros::CheckExistingMountPointsOnUIThread() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   const disks::DiskMountManager::MountPointMap& mount_point_map =
diff --git a/chrome/browser/storage_monitor/storage_monitor_chromeos.h b/chrome/browser/storage_monitor/storage_monitor_chromeos.h
index e59c125..ee4707e 100644
--- a/chrome/browser/storage_monitor/storage_monitor_chromeos.h
+++ b/chrome/browser/storage_monitor/storage_monitor_chromeos.h
@@ -21,6 +21,10 @@
 #include "chrome/browser/storage_monitor/storage_monitor.h"
 #include "chromeos/disks/disk_mount_manager.h"
 
+namespace chrome {
+class MediaTransferProtocolDeviceObserverLinux;
+}
+
 namespace chromeos {
 
 class StorageMonitorCros
@@ -31,6 +35,10 @@ class StorageMonitorCros
   // Should only be called by browser start up code. Use GetInstance() instead.
   StorageMonitorCros();
 
+  // Sets up disk listeners and issues notifications for any discovered
+  // mount points. Sets up MTP manager and listeners.
+  void Init();
+
   virtual void OnDiskEvent(disks::DiskMountManager::DiskEvent event,
                            const disks::DiskMountManager::Disk* disk) OVERRIDE;
   virtual void OnDeviceEvent(disks::DiskMountManager::DeviceEvent event,
@@ -86,6 +94,9 @@ class StorageMonitorCros
   // Only accessed on the UI thread.
   MountMap mount_map_;
 
+  scoped_ptr<chrome::MediaTransferProtocolDeviceObserverLinux>
+      media_transfer_protocol_device_observer_;
+
   DISALLOW_COPY_AND_ASSIGN(StorageMonitorCros);
 };
 
diff --git a/chrome/browser/storage_monitor/storage_monitor_linux.cc b/chrome/browser/storage_monitor/storage_monitor_linux.cc
index 1b54579..d2ab41b 100644
--- a/chrome/browser/storage_monitor/storage_monitor_linux.cc
+++ b/chrome/browser/storage_monitor/storage_monitor_linux.cc
@@ -13,6 +13,7 @@
 
 #include "base/basictypes.h"
 #include "base/bind.h"
+#include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/metrics/histogram.h"
 #include "base/stl_util.h"
@@ -20,8 +21,11 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/storage_monitor/media_storage_util.h"
+#include "chrome/browser/storage_monitor/media_transfer_protocol_device_observer_linux.h"
 #include "chrome/browser/storage_monitor/removable_device_constants.h"
 #include "chrome/browser/storage_monitor/udev_util_linux.h"
+#include "chrome/common/chrome_switches.h"
+#include "device/media_transfer_protocol/media_transfer_protocol_manager.h"
 
 namespace chrome {
 
@@ -270,6 +274,12 @@ StorageMonitorLinux::StorageMonitorLinux(const base::FilePath& path,
 
 StorageMonitorLinux::~StorageMonitorLinux() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+
+  if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kTestType)) {
+    BrowserThread::PostTask(
+        BrowserThread::UI, FROM_HERE,
+        base::Bind(&device::MediaTransferProtocolManager::Shutdown));
+  }
 }
 
 void StorageMonitorLinux::Init() {
@@ -282,6 +292,17 @@ void StorageMonitorLinux::Init() {
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       base::Bind(&StorageMonitorLinux::InitOnFileThread, this));
+
+  if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kTestType)) {
+    scoped_refptr<base::MessageLoopProxy> loop_proxy;
+    loop_proxy = content::BrowserThread::GetMessageLoopProxyForThread(
+        content::BrowserThread::FILE);
+    device::MediaTransferProtocolManager::Initialize(loop_proxy);
+
+    media_transfer_protocol_device_observer_.reset(
+        new MediaTransferProtocolDeviceObserverLinux());
+    media_transfer_protocol_device_observer_->SetNotifications(receiver());
+  }
 }
 
 bool StorageMonitorLinux::GetStorageInfoForPath(
diff --git a/chrome/browser/storage_monitor/storage_monitor_linux.h b/chrome/browser/storage_monitor/storage_monitor_linux.h
index 611dacb..725d3a37 100644
--- a/chrome/browser/storage_monitor/storage_monitor_linux.h
+++ b/chrome/browser/storage_monitor/storage_monitor_linux.h
@@ -43,6 +43,8 @@ typedef void (*GetDeviceInfoFunc)(const base::FilePath& device_path,
 
 namespace chrome {
 
+class MediaTransferProtocolDeviceObserverLinux;
+
 class StorageMonitorLinux
     : public StorageMonitor,
       public base::RefCountedThreadSafe<StorageMonitorLinux,
@@ -141,6 +143,9 @@ class StorageMonitorLinux
   // points.
   MountPriorityMap mount_priority_map_;
 
+  scoped_ptr<MediaTransferProtocolDeviceObserverLinux>
+      media_transfer_protocol_device_observer_;
+
   DISALLOW_COPY_AND_ASSIGN(StorageMonitorLinux);
 };
 
diff --git a/chrome/browser/storage_monitor/storage_monitor_mac.h b/chrome/browser/storage_monitor/storage_monitor_mac.h
index 89995a5..e52efe1 100644
--- a/chrome/browser/storage_monitor/storage_monitor_mac.h
+++ b/chrome/browser/storage_monitor/storage_monitor_mac.h
@@ -15,6 +15,8 @@
 
 namespace chrome {
 
+class ImageCaptureDeviceManager;
+
 // This class posts notifications to listeners when a new disk
 // is attached, removed, or changed.
 class StorageMonitorMac
@@ -30,6 +32,8 @@ class StorageMonitorMac
   // Should only be called by browser start up code.  Use GetInstance() instead.
   StorageMonitorMac();
 
+  void Init();
+
   void UpdateDisk(const DiskInfoMac& info, UpdateType update_type);
 
   virtual bool GetStorageInfoForPath(
@@ -65,6 +69,8 @@ class StorageMonitorMac
   // posted.
   std::map<std::string, DiskInfoMac> disk_info_map_;
 
+  scoped_ptr<chrome::ImageCaptureDeviceManager> image_capture_device_manager_;
+
   DISALLOW_COPY_AND_ASSIGN(StorageMonitorMac);
 };
 
diff --git a/chrome/browser/storage_monitor/storage_monitor_mac.mm b/chrome/browser/storage_monitor/storage_monitor_mac.mm
index 2582fa3..f52ad79 100644
--- a/chrome/browser/storage_monitor/storage_monitor_mac.mm
+++ b/chrome/browser/storage_monitor/storage_monitor_mac.mm
@@ -4,6 +4,8 @@
 
 #include "chrome/browser/storage_monitor/storage_monitor_mac.h"
 
+#include "base/mac/mac_util.h"
+#include "chrome/browser/storage_monitor/image_capture_device_manager.h"
 #include "chrome/browser/storage_monitor/media_storage_util.h"
 #include "content/public/browser/browser_thread.h"
 
@@ -13,6 +15,9 @@ namespace {
 
 const char kDiskImageModelName[] = "Disk Image";
 
+// TODO(gbillock): Make these take weak pointers and don't have
+// StorageMonitorMac be ref counted.
+
 void GetDiskInfoAndUpdateOnFileThread(
     const scoped_refptr<StorageMonitorMac>& monitor,
     base::mac::ScopedCFTypeRef<CFDictionaryRef> dict,
@@ -84,10 +89,17 @@ void EjectDisk(EjectDiskOptions* options) {
 }  // namespace
 
 StorageMonitorMac::StorageMonitorMac() {
-  session_.reset(DASessionCreate(NULL));
+}
 
-  DASessionScheduleWithRunLoop(
-      session_, CFRunLoopGetCurrent(), kCFRunLoopCommonModes);
+StorageMonitorMac::~StorageMonitorMac() {
+  if (session_.get()) {
+    DASessionUnscheduleFromRunLoop(
+        session_, CFRunLoopGetCurrent(), kCFRunLoopCommonModes);
+  }
+}
+
+void StorageMonitorMac::Init() {
+  session_.reset(DASessionCreate(NULL));
 
   // Register for callbacks for attached, changed, and removed devices.
   // This will send notifications for existing devices too.
@@ -107,11 +119,14 @@ StorageMonitorMac::StorageMonitorMac() {
       kDADiskDescriptionWatchVolumePath,
       DiskDescriptionChangedCallback,
       this);
-}
 
-StorageMonitorMac::~StorageMonitorMac() {
-  DASessionUnscheduleFromRunLoop(
+  DASessionScheduleWithRunLoop(
       session_, CFRunLoopGetCurrent(), kCFRunLoopCommonModes);
+
+  if (base::mac::IsOSLionOrLater()) {
+    image_capture_device_manager_.reset(new chrome::ImageCaptureDeviceManager);
+    image_capture_device_manager_->SetNotifications(receiver());
+  }
 }
 
 void StorageMonitorMac::UpdateDisk(const DiskInfoMac& info,