SystemMonitor: Pull device type into the device id

chrome/browser/intents/device_attached_intent_source.cc

Index: chrome/browser/intents/device_attached_intent_source.cc
diff --git a/chrome/browser/intents/device_attached_intent_source.cc b/chrome/browser/intents/device_attached_intent_source.cc
index 56fdbe358085dfbf2e515a962e90543c697e8ec7..9df5fbd2d2901a16ff481577a42def4250aca48e 100644
--- a/chrome/browser/intents/device_attached_intent_source.cc
+++ b/chrome/browser/intents/device_attached_intent_source.cc
@@ -13,6 +13,7 @@
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/intents/web_intents_registry.h"
 #include "chrome/browser/intents/web_intents_registry_factory.h"
+#include "chrome/browser/media_gallery/media_storage_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_window.h"
@@ -97,7 +98,6 @@ DeviceAttachedIntentSource::~DeviceAttachedIntentSource() {
 void DeviceAttachedIntentSource::OnMediaDeviceAttached(
     const std::string& id,
     const string16& name,
-    base::SystemMonitor::MediaDeviceType device_type,
     const FilePath::StringType& location) {
   if (!browser_->window()->IsActive())
     return;
@@ -106,9 +106,11 @@ void DeviceAttachedIntentSource::OnMediaDeviceAttached(
   if (browser_->profile()->IsOffTheRecord())
     return;
 
-  // Only handle FilePaths for now.
+  // Only handle mass storage for now.
   // TODO(kmadhusu): Handle all device types. http://crbug.com/140353.
-  if (device_type != SystemMonitor::TYPE_PATH)
+  chrome::MediaStorageUtil::Type type;

[kmadhusu, 2012/08/17 18:53:35]

nit: using chrome::MediaStorageUtil;

[vandebo (ex-Chrome), 2012/08/17 22:55:51]

We prefer to not use "using" unless it significantly improves code readability. 
I don't think it does in this case.

[kmadhusu, 2012/08/20 17:13:49]

Google C++ style guide
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...),
also encourages using declaration in cc file. In this file,
chrome::MediaStorageUtil is used at 6 places. Therefore, "using
chrome::MediaStorageUtil;" improves readability. 

[vandebo (ex-Chrome), 2012/08/20 18:35:14]

Done.

+  chrome::MediaStorageUtil::CrackDeviceId(id, &type, NULL);

[kmadhusu, 2012/08/17 18:53:35]

How about adding a DCHECK(type == MediaStorageUtil::USB_MASS_STORAGE); here

[groby-ooo-7-16, 2012/08/17 21:01:30]

If you DCHECK, you should _not_ handle that via if/return.

That would be probably not the best choice, unless there's no chance we ever get
an MTP device attached?

On 2012/08/17 18:53:35, kmadhusu wrote:

[vandebo (ex-Chrome), 2012/08/17 22:55:51]

Added DCHECK(MediaStorageUtil::IsRemovableDevice(id));

[vandebo (ex-Chrome), 2012/08/17 22:55:51]

We don't currently handle MTP, but will do so shortly, so this check is
temporary.

+  if (type == chrome::MediaStorageUtil::USB_MTP)
     return;
 
   // Sanity checks for |device_path|.
@@ -116,7 +118,7 @@ void DeviceAttachedIntentSource::OnMediaDeviceAttached(
   if (!device_path.IsAbsolute() || device_path.ReferencesParent())
     return;
 
-  SystemMonitor::MediaDeviceInfo device_info(id, name, device_type, location);
+  SystemMonitor::MediaDeviceInfo device_info(id, name, location);
   scoped_refptr<DispatchIntentTaskHelper> task = new DispatchIntentTaskHelper(
       AsWeakPtr(), device_info);
   WebIntentsRegistryFactory::GetForProfile(browser_->profile())->
@@ -152,19 +154,19 @@ void DeviceAttachedIntentSource::OnMediaDeviceDetached(const std::string& id) {
   if (it == device_id_map_.end())
     return;
 
-  // TODO(kmadhusu, vandebo): Clean up this code. http://crbug.com/140340.
-
+  // TODO(kmadhusu) This should be something like
+  // RevokeFileSystemByDevice(std::string)
   FilePath path(it->second.location);
   fileapi::IsolatedContext::GetInstance()->RevokeFileSystemByPath(path);
-  switch (it->second.type) {
-    case SystemMonitor::TYPE_MTP:
+
 #if defined(SUPPORT_MEDIA_FILESYSTEM)
-      MediaDeviceMapService::GetInstance()->RemoveMediaDevice(
-          it->second.location);
-#endif
-      break;
-    case SystemMonitor::TYPE_PATH:
-      break;
+  // TODO(kmadhusu, vandebo): Clean up this code. http://crbug.com/140340.
+  chrome::MediaStorageUtil::Type type;
+  chrome::MediaStorageUtil::CrackDeviceId(it->second.unique_id, &type, NULL);
+  if (type == chrome::MediaStorageUtil::USB_MTP) {
+    MediaDeviceMapService::GetInstance()->RemoveMediaDevice(
+        it->second.location);
   }
+#endif
   device_id_map_.erase(it);
 }