SystemMonitor: Pull device type into the device id

chrome/browser/intents/device_attached_intent_source.cc

 // Copyright (c) 2012 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/intents/device_attached_intent_source.h"
 
 #include <string>
 
 #include "base/bind.h"
 #include "base/file_path.h"
 #include "base/string16.h"
 #include "base/memory/ref_counted.h"
 #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"
 #include "content/public/browser/web_intents_dispatcher.h"
 #include "content/public/browser/web_contents_delegate.h"
 #include "webkit/fileapi/file_system_types.h"
 #include "webkit/fileapi/isolated_context.h"
 #include "webkit/glue/web_intent_data.h"
 #include "webkit/glue/web_intent_service_data.h"
 #include "webkit/fileapi/media/media_file_system_config.h"
@@ skipping 64 matching lines @@
 
 DeviceAttachedIntentSource::~DeviceAttachedIntentSource() {
   SystemMonitor* sys_monitor = SystemMonitor::Get();
   if (sys_monitor)
     sys_monitor->RemoveDevicesChangedObserver(this);
 }
 
 void DeviceAttachedIntentSource::OnMediaDeviceAttached(
     const std::string& id,
     const string16& name,
-    base::SystemMonitor::MediaDeviceType device_type,
     const FilePath::StringType& location) {
   if (!browser_->window()->IsActive())
     return;
 
   // TODO(kmadhusu): Dispatch intents on incognito window.
   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|.
   const FilePath device_path(location);
   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())->
       GetIntentServices(
           UTF8ToUTF16(kAction), UTF8ToUTF16(kIntentType),
           base::Bind(&DispatchIntentTaskHelper::MayDispatchIntentForService,
                      task.get()));
 }
 
 void DeviceAttachedIntentSource::DispatchIntentsForService(
@@ skipping 15 matching lines @@
 
   delegate_->WebIntentDispatch(NULL  /* no WebContents */,
                                content::WebIntentsDispatcher::Create(intent));
 }
 
 void DeviceAttachedIntentSource::OnMediaDeviceDetached(const std::string& id) {
   DeviceIdToInfoMap::iterator it = device_id_map_.find(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);
+  // 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
-      break;
-    case SystemMonitor::TYPE_PATH:
-      break;
-  }
   device_id_map_.erase(it);
 }