Storage Monitor: Remove...

Storage Monitor: Remove MediaTransferProtocolDeviceObserverLinux::GetInstance() and access it only from StorageMonitor.

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

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

chrome/browser/storage_monitor/media_storage_util.cc

@@ -17,10 +17,6 @@
 #include "content/public/browser/browser_thread.h"
 #include "ui/base/text/bytes_formatting.h"
 
-#if defined(OS_LINUX)  // Implies OS_CHROMEOS
-#include "chrome/browser/storage_monitor/media_transfer_protocol_device_observer_linux.h"
-#endif
-
 using content::BrowserThread;
 
 namespace chrome {
@@ -301,20 +297,9 @@ bool MediaStorageUtil::GetDeviceInfoFromPath(const base::FilePath& path,
   if (!path.IsAbsolute())
     return false;
 
-  bool found_device = false;
   StorageInfo info;
   StorageMonitor* monitor = StorageMonitor::GetInstance();
-  found_device = monitor->GetStorageInfoForPath(path, &info);
-
-// TODO(gbillock): Move this upstream into the RemovableStorageNotifications
-// implementation to handle in its GetDeviceInfoForPath call.
-#if defined(OS_LINUX)
-  if (!found_device) {
-    MediaTransferProtocolDeviceObserverLinux* mtp_manager =
-        MediaTransferProtocolDeviceObserverLinux::GetInstance();
-    found_device = mtp_manager->GetStorageInfoForPath(path, &info);
-  }
-#endif
+  bool found_device = monitor->GetStorageInfoForPath(path, &info);
 
   if (found_device && IsRemovableDevice(info.device_id)) {
     base::FilePath sub_folder_path;
@@ -346,7 +331,7 @@ bool MediaStorageUtil::GetDeviceInfoFromPath(const base::FilePath& path,
 #endif
 
   // Handle non-removable devices. Note: this is just overwriting
-  // good values from RemovableStorageNotifications.
+  // good values from StorageMonitor.
   // TODO(gbillock): Make sure return values from that class are definitive,
   // and don't do this here.
   info.device_id = MakeDeviceId(FIXED_MASS_STORAGE, path.AsUTF8Unsafe());

chrome/browser/storage_monitor/media_transfer_protocol_device_observer_linux.cc

@@ -4,6 +4,8 @@
 
 #include "chrome/browser/storage_monitor/media_transfer_protocol_device_observer_linux.h"
 
+#include <vector>
+
 #include "base/files/file_path.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
@@ -120,8 +122,9 @@ void GetStorageInfo(const std::string& storage_name,
 }  // namespace
 
 MediaTransferProtocolDeviceObserverLinux::
-MediaTransferProtocolDeviceObserverLinux()
-    : get_storage_info_func_(&GetStorageInfo) {
+MediaTransferProtocolDeviceObserverLinux(StorageMonitor::Receiver* receiver)
+    : get_storage_info_func_(&GetStorageInfo),
+      notifications_(receiver) {
   DCHECK(!g_mtp_device_observer);
   g_mtp_device_observer = this;
 
@@ -135,9 +138,10 @@ MediaTransferProtocolDeviceObserverLinux()
 // This constructor is only used by unit tests.
 MediaTransferProtocolDeviceObserverLinux::
 MediaTransferProtocolDeviceObserverLinux(
+    StorageMonitor::Receiver* receiver,
     GetStorageInfoFunc get_storage_info_func)
     : get_storage_info_func_(get_storage_info_func),
-      notifications_(NULL) {
+      notifications_(receiver) {
   DCHECK(!g_mtp_device_observer);
   g_mtp_device_observer = this;
 }
@@ -153,13 +157,6 @@ MediaTransferProtocolDeviceObserverLinux::
     mtp_manager->RemoveObserver(this);
 }
 
-// static
-MediaTransferProtocolDeviceObserverLinux*
-MediaTransferProtocolDeviceObserverLinux::GetInstance() {
-  DCHECK(g_mtp_device_observer != NULL);
-  return g_mtp_device_observer;
-}
-
 bool MediaTransferProtocolDeviceObserverLinux::GetStorageInfoForPath(
     const base::FilePath& path,
     StorageInfo* storage_info) const {
@@ -184,11 +181,6 @@ bool MediaTransferProtocolDeviceObserverLinux::GetStorageInfoForPath(
   return true;
 }
 
-void MediaTransferProtocolDeviceObserverLinux::SetNotifications(
-    StorageMonitor::Receiver* notifications) {
-  notifications_ = notifications;
-}
-
 // device::MediaTransferProtocolManager::Observer override.
 void MediaTransferProtocolDeviceObserverLinux::StorageChanged(
     bool is_attached,

chrome/browser/storage_monitor/media_transfer_protocol_device_observer_linux.h

@@ -30,24 +30,19 @@ typedef void (*GetStorageInfoFunc)(const std::string& storage_name,
 class MediaTransferProtocolDeviceObserverLinux
     : public device::MediaTransferProtocolManager::Observer {
  public:
-  // Should only be called by browser start up code. Use GetInstance() instead.
-  MediaTransferProtocolDeviceObserverLinux();
+  explicit MediaTransferProtocolDeviceObserverLinux(
+      StorageMonitor::Receiver* receiver);
   virtual ~MediaTransferProtocolDeviceObserverLinux();
 
-  static MediaTransferProtocolDeviceObserverLinux* GetInstance();
-
   // Finds the storage that contains |path| and populates |storage_info|.
   // Returns false if unable to find the storage.
   bool GetStorageInfoForPath(const base::FilePath& path,
                              StorageInfo* storage_info) const;
 
-  // Set the volume notifications object to be used when new
-  // MTP devices are found.
-  void SetNotifications(StorageMonitor::Receiver* notifications);
-
  protected:
   // Only used in unit tests.
-  explicit MediaTransferProtocolDeviceObserverLinux(
+  MediaTransferProtocolDeviceObserverLinux(
+      StorageMonitor::Receiver* receiver,
       GetStorageInfoFunc get_storage_info_func);
 
   // device::MediaTransferProtocolManager::Observer implementation.
@@ -71,9 +66,7 @@ class MediaTransferProtocolDeviceObserverLinux
 
   // The notifications object to use to signal newly attached devices.
   // Guaranteed to outlive this class.
-  // TODO(gbillock): Edit this when this class is owned by a
-  // StorageMonitor subclass.
-  StorageMonitor::Receiver* notifications_;
+  StorageMonitor::Receiver* const notifications_;
 
   DISALLOW_COPY_AND_ASSIGN(MediaTransferProtocolDeviceObserverLinux);
 };

chrome/browser/storage_monitor/media_transfer_protocol_device_observer_linux_unittest.cc

@@ -10,6 +10,7 @@
 
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop.h"
+#include "base/run_loop.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/storage_monitor/media_storage_util.h"
 #include "chrome/browser/storage_monitor/mock_removable_storage_observer.h"
@@ -54,17 +55,40 @@ void GetStorageInfo(const std::string& storage_name,
     *location = kStorageLocation;
 }
 
+class TestMediaTransferProtocolDeviceObserverLinux
+    : public MediaTransferProtocolDeviceObserverLinux {
+ public:
+  TestMediaTransferProtocolDeviceObserverLinux(
+      StorageMonitor::Receiver* receiver)
+      : MediaTransferProtocolDeviceObserverLinux(receiver, &GetStorageInfo) {
+  }
+
+  // Notifies MediaTransferProtocolDeviceObserverLinux about the attachment of
+  // mtp storage device given the |storage_name|.
+  void MtpStorageAttached(const std::string& storage_name) {
+    StorageChanged(true, storage_name);
+    base::RunLoop().RunUntilIdle();
+  }
+
+  // Notifies MediaTransferProtocolDeviceObserverLinux about the detachment of
+  // mtp storage device given the |storage_name|.
+  void MtpStorageDetached(const std::string& storage_name) {
+    StorageChanged(false, storage_name);
+    base::RunLoop().RunUntilIdle();
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestMediaTransferProtocolDeviceObserverLinux);
+};
+
 }  // namespace
 
 // A class to test the functionality of MediaTransferProtocolDeviceObserverLinux
 // member functions.
-class MediaTransferProtocolDeviceObserverLinuxTest
-    : public testing::Test,
-      public MediaTransferProtocolDeviceObserverLinux {
+class MediaTransferProtocolDeviceObserverLinuxTest : public testing::Test {
  public:
   MediaTransferProtocolDeviceObserverLinuxTest()
-      : MediaTransferProtocolDeviceObserverLinux(&GetStorageInfo),
-        message_loop_(MessageLoop::TYPE_IO),
+      : message_loop_(MessageLoop::TYPE_IO),
         file_thread_(content::BrowserThread::FILE, &message_loop_) {}
 
   virtual ~MediaTransferProtocolDeviceObserverLinuxTest() {}
@@ -72,12 +96,14 @@ class MediaTransferProtocolDeviceObserverLinuxTest
  protected:
   virtual void SetUp() OVERRIDE {
     mock_storage_observer_.reset(new MockRemovableStorageObserver);
+    mtp_device_observer_.reset(
+        new TestMediaTransferProtocolDeviceObserverLinux(monitor_.receiver()));
     monitor_.AddObserver(mock_storage_observer_.get());
-    SetNotifications(monitor_.receiver());
   }
 
   virtual void TearDown() OVERRIDE {
     monitor_.RemoveObserver(mock_storage_observer_.get());
+    mtp_device_observer_.reset();
   }
 
   // Returns the device changed observer object.
@@ -85,20 +111,8 @@ class MediaTransferProtocolDeviceObserverLinuxTest
     return *mock_storage_observer_;
   }
 
-  // Notifies MediaTransferProtocolDeviceObserverLinux about the attachment of
-  // mtp storage device given the |storage_name|.
-  void MtpStorageAttached(const std::string& storage_name) {
-    MediaTransferProtocolDeviceObserverLinux::StorageChanged(true,
-                                                             storage_name);
-    message_loop_.RunUntilIdle();
-  }
-
-  // Notifies MediaTransferProtocolDeviceObserverLinux about the detachment of
-  // mtp storage device given the |storage_name|.
-  void MtpStorageDetached(const std::string& storage_name) {
-    MediaTransferProtocolDeviceObserverLinux::StorageChanged(false,
-                                                             storage_name);
-    message_loop_.RunUntilIdle();
+  TestMediaTransferProtocolDeviceObserverLinux* mtp_device_observer() {
+    return mtp_device_observer_.get();
   }
 
  private:
@@ -106,6 +120,7 @@ class MediaTransferProtocolDeviceObserverLinuxTest
   content::TestBrowserThread file_thread_;
 
   chrome::test::TestStorageMonitor monitor_;
+  scoped_ptr<TestMediaTransferProtocolDeviceObserverLinux> mtp_device_observer_;
   scoped_ptr<MockRemovableStorageObserver> mock_storage_observer_;
 
   DISALLOW_COPY_AND_ASSIGN(MediaTransferProtocolDeviceObserverLinuxTest);
@@ -116,7 +131,7 @@ TEST_F(MediaTransferProtocolDeviceObserverLinuxTest, BasicAttachDetach) {
   std::string device_id = GetMtpDeviceId(kStorageUniqueId);
 
   // Attach a mtp storage.
-  MtpStorageAttached(kStorageWithValidInfo);
+  mtp_device_observer()->MtpStorageAttached(kStorageWithValidInfo);
 
   EXPECT_EQ(1, observer().attach_calls());
   EXPECT_EQ(0, observer().detach_calls());
@@ -125,7 +140,7 @@ TEST_F(MediaTransferProtocolDeviceObserverLinuxTest, BasicAttachDetach) {
   EXPECT_EQ(kStorageLocation, observer().last_attached().location);
 
   // Detach the attached storage.
-  MtpStorageDetached(kStorageWithValidInfo);
+  mtp_device_observer()->MtpStorageDetached(kStorageWithValidInfo);
 
   EXPECT_EQ(1, observer().attach_calls());
   EXPECT_EQ(1, observer().detach_calls());
@@ -137,10 +152,10 @@ TEST_F(MediaTransferProtocolDeviceObserverLinuxTest, BasicAttachDetach) {
 // notifications.
 TEST_F(MediaTransferProtocolDeviceObserverLinuxTest, StorageWithInvalidInfo) {
   // Attach the mtp storage with invalid storage info.
-  MtpStorageAttached(kStorageWithInvalidInfo);
+  mtp_device_observer()->MtpStorageAttached(kStorageWithInvalidInfo);
 
   // Detach the attached storage.
-  MtpStorageDetached(kStorageWithInvalidInfo);
+  mtp_device_observer()->MtpStorageDetached(kStorageWithInvalidInfo);
 
   EXPECT_EQ(0, observer().attach_calls());
   EXPECT_EQ(0, observer().detach_calls());

chrome/browser/storage_monitor/storage_monitor_chromeos.cc

@@ -131,8 +131,7 @@ void StorageMonitorCros::Init() {
     device::MediaTransferProtocolManager::Initialize(loop_proxy);
 
     media_transfer_protocol_device_observer_.reset(
-        new chrome::MediaTransferProtocolDeviceObserverLinux());
-    media_transfer_protocol_device_observer_->SetNotifications(receiver());
+        new chrome::MediaTransferProtocolDeviceObserverLinux(receiver()));
   }
 }
 
@@ -207,6 +206,14 @@ void StorageMonitorCros::OnFormatEvent(
 bool StorageMonitorCros::GetStorageInfoForPath(
     const base::FilePath& path,
     StorageInfo* device_info) const {
+  // TODO(thestig) |media_transfer_protocol_device_observer_| should always be
+  // valid.
+  if (media_transfer_protocol_device_observer_ &&
+      media_transfer_protocol_device_observer_->GetStorageInfoForPath(
+          path, device_info)) {
+    return true;
+  }
+
   if (!path.IsAbsolute())
     return false;
 

chrome/browser/storage_monitor/storage_monitor_linux.cc

@@ -258,8 +258,7 @@ void StorageMonitorLinux::Init() {
     device::MediaTransferProtocolManager::Initialize(loop_proxy);
 
     media_transfer_protocol_device_observer_.reset(
-        new MediaTransferProtocolDeviceObserverLinux());
-    media_transfer_protocol_device_observer_->SetNotifications(receiver());
+        new MediaTransferProtocolDeviceObserverLinux(receiver()));
   }
 }
 
@@ -268,6 +267,14 @@ bool StorageMonitorLinux::GetStorageInfoForPath(
     StorageInfo* device_info) const {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
+  // TODO(thestig) |media_transfer_protocol_device_observer_| should always be
+  // valid.
+  if (media_transfer_protocol_device_observer_ &&
+      media_transfer_protocol_device_observer_->GetStorageInfoForPath(
+          path, device_info)) {
+    return true;
+  }
+
   if (!path.IsAbsolute())
     return false;