Separate USB device permissions prompt logic into subclasses.

extensions/browser/api/device_permissions_prompt.cc

Index: extensions/browser/api/device_permissions_prompt.cc
diff --git a/extensions/browser/api/device_permissions_prompt.cc b/extensions/browser/api/device_permissions_prompt.cc
index 09751b36cc84fe14e1814a71352d745fe9cbfa04..c02aef2b53bee2201d287e812ab7d747287dd37e 100644
--- a/extensions/browser/api/device_permissions_prompt.cc
+++ b/extensions/browser/api/device_permissions_prompt.cc
@@ -5,9 +5,9 @@
 #include "extensions/browser/api/device_permissions_prompt.h"
 
 #include "base/barrier_closure.h"
+#include "base/scoped_observer.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
-#include "content/public/browser/browser_thread.h"
 #include "device/core/device_client.h"
 #include "device/usb/usb_device.h"
 #include "device/usb/usb_device_filter.h"
@@ -18,49 +18,162 @@
 #include "extensions/strings/grit/extensions_strings.h"
 #include "ui/base/l10n/l10n_util.h"
 
-using content::BrowserThread;
 using device::UsbDevice;
 using device::UsbDeviceFilter;
 using device::UsbService;
 
 namespace extensions {
 
-DevicePermissionsPrompt::Delegate::~Delegate() {
-}
+namespace {
+
+struct UsbDeviceInfo : public DevicePermissionsPrompt::Prompt::DeviceInfo {
+  UsbDeviceInfo(scoped_refptr<UsbDevice> device) : device(device) {
+    base::string16 manufacturer_string = device->manufacturer_string();
+    if (manufacturer_string.empty()) {
+      const char* vendor_name =
+          device::UsbIds::GetVendorName(device->vendor_id());
+      if (vendor_name) {
+        manufacturer_string = base::UTF8ToUTF16(vendor_name);
+      } else {
+        base::string16 vendor_id = base::ASCIIToUTF16(
+            base::StringPrintf("0x%04x", device->vendor_id()));
+        manufacturer_string =
+            l10n_util::GetStringFUTF16(IDS_DEVICE_UNKNOWN_VENDOR, vendor_id);
+      }
+    }
+
+    base::string16 product_string = device->product_string();
+    if (product_string.empty()) {
+      const char* product_name = device::UsbIds::GetProductName(
+          device->vendor_id(), device->product_id());
+      if (product_name) {
+        product_string = base::UTF8ToUTF16(product_name);
+      } else {
+        base::string16 product_id = base::ASCIIToUTF16(
+            base::StringPrintf("0x%04x", device->product_id()));
+        product_string =
+            l10n_util::GetStringFUTF16(IDS_DEVICE_UNKNOWN_PRODUCT, product_id);
+      }
+    }
+
+    name = l10n_util::GetStringFUTF16(IDS_DEVICE_PERMISSIONS_DEVICE_NAME,
+                                      product_string, manufacturer_string);
+    serial_number = device->serial_number();
+  }
+
+  ~UsbDeviceInfo() override {}
+
+  scoped_refptr<UsbDevice> device;

[Jeffrey Yasskin, 2015/04/23 20:40:07]

Does this need to own a reference? It looks like the device will be dropped in
OnDeviceRemoved(), which happens before the UsbDevice is destroyed.

[Reilly Grant (use Gerrit), 2015/04/23 23:02:47]

Added comments about why this needs to be a strong reference for now.

+};
+
+class UsbDevicePermissionsPrompt : public DevicePermissionsPrompt::Prompt,
+                                   public device::UsbService::Observer {
+ public:
+  UsbDevicePermissionsPrompt(
+      const Extension* extension,
+      content::BrowserContext* context,
+      bool multiple,
+      const std::vector<device::UsbDeviceFilter>& filters,
+      const DevicePermissionsPrompt::UsbDevicesCallback& callback)
+      : Prompt(extension, context, multiple),
+        filters_(filters),
+        callback_(callback),
+        service_observer_(this) {}
+
+ private:
+  ~UsbDevicePermissionsPrompt() override {}
+
+  // DevicePermissionsPrompt::Prompt implementation:
+  void SetObserver(
+      DevicePermissionsPrompt::Prompt::Observer* observer) override {
+    DevicePermissionsPrompt::Prompt::SetObserver(observer);
+
+    if (observer) {
+      UsbService* service = device::DeviceClient::Get()->GetUsbService();
+      if (service && !service_observer_.IsObserving(service)) {
+        service->GetDevices(
+            base::Bind(&UsbDevicePermissionsPrompt::OnDevicesEnumerated, this));
+        service_observer_.Add(service);
+      }
+    }
+  }
+
+  base::string16 GetHeading() const override {
+    return l10n_util::GetStringUTF16(
+        multiple() ? IDS_USB_DEVICE_PERMISSIONS_PROMPT_TITLE_MULTIPLE
+                   : IDS_USB_DEVICE_PERMISSIONS_PROMPT_TITLE_SINGLE);
+  }
+
+  void Dismissed() override {
+    DevicePermissionsManager* permissions_manager =
+        DevicePermissionsManager::Get(browser_context());
+    std::vector<scoped_refptr<UsbDevice>> devices;
+    for (const DeviceInfo* device : devices_) {
+      if (device->granted) {
+        const UsbDeviceInfo* usb_device =
+            static_cast<const UsbDeviceInfo*>(device);
+        devices.push_back(usb_device->device);
+        if (permissions_manager) {
+          permissions_manager->AllowUsbDevice(extension()->id(),
+                                              usb_device->device);
+        }
+      }
+    }
+    DCHECK(multiple() || devices.size() <= 1);
+    callback_.Run(devices);
+    callback_.Reset();
+  }
 
-DevicePermissionsPrompt::Prompt::DeviceInfo::DeviceInfo(
-    scoped_refptr<UsbDevice> device)
-    : device(device) {
-  base::string16 manufacturer_string = device->manufacturer_string();
-  if (manufacturer_string.empty()) {
-    const char* vendor_name =
-        device::UsbIds::GetVendorName(device->vendor_id());
-    if (vendor_name) {
-      manufacturer_string = base::UTF8ToUTF16(vendor_name);
-    } else {
-      base::string16 vendor_id =
-          base::ASCIIToUTF16(base::StringPrintf("0x%04x", device->vendor_id()));
-      manufacturer_string =
-          l10n_util::GetStringFUTF16(IDS_DEVICE_UNKNOWN_VENDOR, vendor_id);
+  // device::UsbService::Observer implementation:
+  void OnDeviceAdded(scoped_refptr<UsbDevice> device) override {
+    if (!(filters_.empty() || UsbDeviceFilter::MatchesAny(device, filters_))) {
+      return;
     }
+
+    device->CheckUsbAccess(base::Bind(
+        &UsbDevicePermissionsPrompt::AddCheckedUsbDevice, this, device));
   }
 
-  base::string16 product_string = device->product_string();
-  if (product_string.empty()) {
-    const char* product_name = device::UsbIds::GetProductName(
-        device->vendor_id(), device->product_id());
-    if (product_name) {
-      product_string = base::UTF8ToUTF16(product_name);
-    } else {
-      base::string16 product_id = base::ASCIIToUTF16(
-          base::StringPrintf("0x%04x", device->product_id()));
-      product_string =
-          l10n_util::GetStringFUTF16(IDS_DEVICE_UNKNOWN_PRODUCT, product_id);
+  void OnDeviceRemoved(scoped_refptr<UsbDevice> device) override {
+    for (auto it = devices_.begin(); it != devices_.end(); ++it) {
+      const UsbDeviceInfo* entry = static_cast<const UsbDeviceInfo*>(*it);
+      if (entry->device == device) {
+        devices_.erase(it);
+        if (observer()) {
+          observer()->OnDevicesChanged();
+        }
+        return;
+      }
     }
   }
 
-  name = l10n_util::GetStringFUTF16(IDS_DEVICE_PERMISSIONS_DEVICE_NAME,
-                                    product_string, manufacturer_string);
+  void OnDevicesEnumerated(
+      const std::vector<scoped_refptr<UsbDevice>>& devices) {
+    for (const auto& device : devices) {
+      if (filters_.empty() || UsbDeviceFilter::MatchesAny(device, filters_)) {
+        device->CheckUsbAccess(base::Bind(
+            &UsbDevicePermissionsPrompt::AddCheckedUsbDevice, this, device));
+      }
+    }
+  }
+
+  void AddCheckedUsbDevice(scoped_refptr<UsbDevice> device, bool allowed) {
+    if (allowed) {
+      devices_.push_back(new UsbDeviceInfo(device));
+      if (observer()) {
+        observer()->OnDevicesChanged();
+      }
+    }
+  }
+
+  std::vector<UsbDeviceFilter> filters_;
+  DevicePermissionsPrompt::UsbDevicesCallback callback_;
+  ScopedObserver<UsbService, UsbService::Observer> service_observer_;
+};
+
+}  // namespace
+
+DevicePermissionsPrompt::Prompt::DeviceInfo::DeviceInfo() {
 }
 
 DevicePermissionsPrompt::Prompt::DeviceInfo::~DeviceInfo() {
@@ -69,32 +182,14 @@ DevicePermissionsPrompt::Prompt::DeviceInfo::~DeviceInfo() {
 DevicePermissionsPrompt::Prompt::Observer::~Observer() {
 }
 
-DevicePermissionsPrompt::Prompt::Prompt(Delegate* delegate,
-                                        const Extension* extension,
-                                        content::BrowserContext* context)
-    : extension_(extension),
-      browser_context_(context),
-      delegate_(delegate),
-      usb_service_observer_(this) {
+DevicePermissionsPrompt::Prompt::Prompt(const Extension* extension,
+                                        content::BrowserContext* context,
+                                        bool multiple)
+    : extension_(extension), browser_context_(context), multiple_(multiple) {
 }
 
 void DevicePermissionsPrompt::Prompt::SetObserver(Observer* observer) {
   observer_ = observer;
-
-  if (observer_) {
-    UsbService* service = device::DeviceClient::Get()->GetUsbService();
-    if (service && !usb_service_observer_.IsObserving(service)) {
-      service->GetDevices(base::Bind(
-          &DevicePermissionsPrompt::Prompt::OnDevicesEnumerated, this));
-      usb_service_observer_.Add(service);
-    }
-  }
-}
-
-base::string16 DevicePermissionsPrompt::Prompt::GetHeading() const {
-  return l10n_util::GetStringUTF16(
-      multiple_ ? IDS_DEVICE_PERMISSIONS_PROMPT_TITLE_MULTIPLE
-                : IDS_DEVICE_PERMISSIONS_PROMPT_TITLE_SINGLE);
 }
 
 base::string16 DevicePermissionsPrompt::Prompt::GetPromptMessage() const {
@@ -107,90 +202,23 @@ base::string16 DevicePermissionsPrompt::Prompt::GetPromptMessage() const {
 base::string16 DevicePermissionsPrompt::Prompt::GetDeviceName(
     size_t index) const {
   DCHECK_LT(index, devices_.size());
-  return devices_[index].name;
+  return devices_[index]->name;
 }
 
 base::string16 DevicePermissionsPrompt::Prompt::GetDeviceSerialNumber(
     size_t index) const {
   DCHECK_LT(index, devices_.size());
-  return devices_[index].device->serial_number();
+  return devices_[index]->serial_number;
 }
 
 void DevicePermissionsPrompt::Prompt::GrantDevicePermission(size_t index) {
   DCHECK_LT(index, devices_.size());
-  devices_[index].granted = true;
-}
-
-void DevicePermissionsPrompt::Prompt::Dismissed() {
-  DevicePermissionsManager* permissions_manager =
-      DevicePermissionsManager::Get(browser_context_);
-  std::vector<scoped_refptr<UsbDevice>> devices;
-  for (const DeviceInfo& device : devices_) {
-    if (device.granted) {
-      devices.push_back(device.device);
-      if (permissions_manager) {
-        permissions_manager->AllowUsbDevice(extension_->id(), device.device);
-      }
-    }
-  }
-  delegate_->OnUsbDevicesChosen(devices);
-}
-
-void DevicePermissionsPrompt::Prompt::set_filters(
-    const std::vector<UsbDeviceFilter>& filters) {
-  filters_ = filters;
+  devices_[index]->granted = true;
 }
 
 DevicePermissionsPrompt::Prompt::~Prompt() {
 }
 
-void DevicePermissionsPrompt::Prompt::OnDeviceAdded(
-    scoped_refptr<UsbDevice> device) {
-  if (!(filters_.empty() || UsbDeviceFilter::MatchesAny(device, filters_))) {
-    return;
-  }
-
-  device->CheckUsbAccess(base::Bind(
-      &DevicePermissionsPrompt::Prompt::AddCheckedUsbDevice, this, device));
-}
-
-void DevicePermissionsPrompt::Prompt::OnDeviceRemoved(
-    scoped_refptr<UsbDevice> device) {
-  bool removed_entry = false;
-  for (std::vector<DeviceInfo>::iterator it = devices_.begin();
-       it != devices_.end(); ++it) {
-    if (it->device == device) {
-      devices_.erase(it);
-      removed_entry = true;
-      break;
-    }
-  }
-  if (observer_ && removed_entry) {
-    observer_->OnDevicesChanged();
-  }
-}
-
-void DevicePermissionsPrompt::Prompt::OnDevicesEnumerated(
-    const std::vector<scoped_refptr<UsbDevice>>& devices) {
-  for (const auto& device : devices) {
-    if (filters_.empty() || UsbDeviceFilter::MatchesAny(device, filters_)) {
-      device->CheckUsbAccess(base::Bind(
-          &DevicePermissionsPrompt::Prompt::AddCheckedUsbDevice, this, device));
-    }
-  }
-}
-
-void DevicePermissionsPrompt::Prompt::AddCheckedUsbDevice(
-    scoped_refptr<UsbDevice> device,
-    bool allowed) {
-  if (allowed) {
-    devices_.push_back(DeviceInfo(device));
-    if (observer_) {
-      observer_->OnDevicesChanged();
-    }
-  }
-}
-
 DevicePermissionsPrompt::DevicePermissionsPrompt(
     content::WebContents* web_contents)
     : web_contents_(web_contents) {
@@ -200,15 +228,13 @@ DevicePermissionsPrompt::~DevicePermissionsPrompt() {
 }
 
 void DevicePermissionsPrompt::AskForUsbDevices(
-    Delegate* delegate,
     const Extension* extension,
     content::BrowserContext* context,
     bool multiple,
-    const std::vector<UsbDeviceFilter>& filters) {
-  prompt_ = new Prompt(delegate, extension, context);
-  prompt_->set_multiple(multiple);
-  prompt_->set_filters(filters);
-
+    const std::vector<UsbDeviceFilter>& filters,
+    const UsbDevicesCallback& callback) {
+  prompt_ = new UsbDevicePermissionsPrompt(extension, context, multiple,
+                                           filters, callback);
   ShowDialog();
 }