Show device attach web intent picker dialog only if we have registered services.

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 3a2e5d530cb6b06f0803e45b1d0798ffa4e40f52..c4e3d3dbc3202e599a63312b467f29a9c7dfb19c 100644
--- a/chrome/browser/intents/device_attached_intent_source.cc
+++ b/chrome/browser/intents/device_attached_intent_source.cc
@@ -6,18 +6,69 @@
 
 #include <string>
 
+#include "base/bind.h"
 #include "base/file_path.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/ui/browser.h"
 #include "chrome/browser/ui/browser_window.h"
+#include "content/public/browser/browser_thread.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"
 
 using base::SystemMonitor;
+using content::BrowserThread;
 using content::WebContentsDelegate;
+using webkit_glue::WebIntentServiceData;
+
+namespace {
+
+// Helper class to get the intent services.

[James Hawkins, 2012/08/03 17:44:59]

nit: Expand this documentation.

[kmadhusu, 2012/08/04 01:14:47]

Done.

+class DispatchIntentTaskHelper {
+ public:
+  DispatchIntentTaskHelper(DeviceAttachedIntentSource* source,
+                           Browser* browser,
+                           const string16& action,
+                           const string16& intent_type,
+                           SystemMonitor::MediaDeviceType device_type,
+                           const FilePath& device_path)
+      : source_(source),
+        browser_(browser),
+        action_(action),
+        intent_type_(intent_type),
+        device_type_(device_type),
+        device_path_(device_path) {
+  }
+
+  // Destruction happens on DidGetServicesForIntent function.
+  ~DispatchIntentTaskHelper() {

[James Hawkins, 2012/08/03 17:44:59]

nit: {}

[kmadhusu, 2012/08/04 01:14:47]

Done.

+  }
+
+  // Helper function to get intent services.
+  void GetServicesForIntent();
+
+ private:
+  void DidGetServicesForIntent(

[James Hawkins, 2012/08/03 17:44:59]

Need to document that this method deletes this object.

[kmadhusu, 2012/08/04 01:14:47]

Done.

+      const std::vector<webkit_glue::WebIntentServiceData>& services);
+
+  // Weak pointers.

[James Hawkins, 2012/08/03 17:44:59]

More importantly, document what they are used for.

[kmadhusu, 2012/08/04 01:14:47]

Done.

+  DeviceAttachedIntentSource* source_;
+  Browser* browser_;
+
+  const string16 action_;

[James Hawkins, 2012/08/03 17:44:59]

Document member variables.

[kmadhusu, 2012/08/04 01:14:47]

Done.

+  const string16 intent_type_;
+  SystemMonitor::MediaDeviceType device_type_;
+  const FilePath device_path_;
+
+  DISALLOW_COPY_AND_ASSIGN(DispatchIntentTaskHelper);
+};
+
+}  // namespace
 
 DeviceAttachedIntentSource::DeviceAttachedIntentSource(
     Browser* browser, WebContentsDelegate* delegate)
@@ -36,13 +87,13 @@ DeviceAttachedIntentSource::~DeviceAttachedIntentSource() {
 void DeviceAttachedIntentSource::OnMediaDeviceAttached(
     const std::string& id,
     const string16& name,
-    base::SystemMonitor::MediaDeviceType type,
+    base::SystemMonitor::MediaDeviceType device_type,
     const FilePath::StringType& location) {
   if (!browser_->window()->IsActive())
     return;
 
   // Only handle FilePaths for now.
-  if (type != SystemMonitor::TYPE_PATH)
+  if (device_type != SystemMonitor::TYPE_PATH)
     return;
 
   // Sanity checks for |device_path|.
@@ -50,21 +101,51 @@ void DeviceAttachedIntentSource::OnMediaDeviceAttached(
   if (!device_path.IsAbsolute() || device_path.ReferencesParent())
     return;
 
+  const string16 action = ASCIIToUTF16("chrome-extension://attach");

[James Hawkins, 2012/08/03 17:44:59]

Pull this out of this method into the unnamed namespace.

[kmadhusu, 2012/08/04 01:14:47]

Done.

+  const string16 intent_type = ASCIIToUTF16("chrome-extension://filesystem");
+  DispatchIntentTaskHelper* task = new DispatchIntentTaskHelper(
+      this, browser_, action, intent_type, device_type, device_path);
+  task->GetServicesForIntent();

[groby-ooo-7-16, 2012/08/03 17:37:20]

Please don't use a self-destructing class unless absolutely necessary. Is it
possible to simply have a callback on DeviceAttachedIntentSource instead? (Also,
this will save a PostTask with the associated - potential - delay)

[kmadhusu, 2012/08/04 01:14:47]

I need to store the device info in between calls. So I need to use a helper
class to store these information. I managed to remove all the post task calls.

+}
+
+void DeviceAttachedIntentSource::DispatchIntentsForService(
+    const string16& action,
+    const string16& intent_type,
+    base::SystemMonitor::MediaDeviceType device_type,
+    const FilePath& device_path) {
   std::string device_name;
 
   // Register device path as an isolated file system.
   // TODO(kinuko, kmadhusu): Use a different file system type for MTP.
-  const std::string filesystem_id =
-      fileapi::IsolatedContext::GetInstance()->RegisterFileSystemForPath(
-          fileapi::kFileSystemTypeNativeMedia, device_path, &device_name);
+  const std::string fs_id = fileapi::IsolatedContext::GetInstance()->
+      RegisterFileSystemForPath(fileapi::kFileSystemTypeNativeMedia,
+                                device_path, &device_name);
 
-  CHECK(!filesystem_id.empty());
-  webkit_glue::WebIntentData intent(
-      ASCIIToUTF16("chrome-extension://attach"),
-      ASCIIToUTF16("chrome-extension://filesystem"),
-      device_name,
-      filesystem_id);
+  CHECK(!fs_id.empty());

[James Hawkins, 2012/08/03 17:44:59]

Don't use CHECKs unless you're debugging a crash in the wild.  Use DCHECKs
instead.

[kmadhusu, 2012/08/04 01:14:47]

Done.

+  webkit_glue::WebIntentData intent(action, intent_type, device_name, fs_id);
 
   delegate_->WebIntentDispatch(NULL  /* no WebContents */,
                                content::WebIntentsDispatcher::Create(intent));
 }
+
+void DispatchIntentTaskHelper::GetServicesForIntent() {
+  WebIntentsRegistryFactory::GetForProfile(browser_->profile())->
+      GetIntentServices(
+          action_, intent_type_,
+          base::Bind(&DispatchIntentTaskHelper::DidGetServicesForIntent,
+                     base::Unretained(this)));
+}
+
+void DispatchIntentTaskHelper::DidGetServicesForIntent(
+    const std::vector<WebIntentServiceData>& services) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (!services.empty()) {
+    BrowserThread::PostTask(
+        BrowserThread::UI,
+        FROM_HERE,

[James Hawkins, 2012/08/03 17:44:59]

Optional nit: Condense parameters to save a line.

[kmadhusu, 2012/08/04 01:14:47]

Done.

+        base::Bind(&DeviceAttachedIntentSource::DispatchIntentsForService,
+                   base::Unretained(source_), action_, intent_type_,
+                   device_type_, device_path_));
+  }
+  delete this;

[James Hawkins, 2012/08/03 17:44:59]

What if this method is never called?  Will we leak this object?

[kmadhusu, 2012/08/04 01:14:47]

This code will not leak. Just to be on the safer side, modified
DispatchIntentTaskHelper to be refcounted. base::Bind will take care of
destructing the object.

+}