Add bluetoothPrivate.connect

extensions/browser/api/bluetooth/bluetooth_private_api.cc

Index: extensions/browser/api/bluetooth/bluetooth_private_api.cc
diff --git a/extensions/browser/api/bluetooth/bluetooth_private_api.cc b/extensions/browser/api/bluetooth/bluetooth_private_api.cc
index 43fca8ca2ccc2897b7873eee8c415ed8204a1a6a..633f5faca27f89852b463ecd12971c050c5babe3 100644
--- a/extensions/browser/api/bluetooth/bluetooth_private_api.cc
+++ b/extensions/browser/api/bluetooth/bluetooth_private_api.cc
@@ -20,9 +20,20 @@ namespace SetDiscoveryFilter = bt_private::SetDiscoveryFilter;
 
 namespace extensions {
 
-static base::LazyInstance<BrowserContextKeyedAPIFactory<BluetoothPrivateAPI> >
+static base::LazyInstance<BrowserContextKeyedAPIFactory<BluetoothPrivateAPI>>
     g_factory = LAZY_INSTANCE_INITIALIZER;
 
+namespace {
+
+std::string GetExtensionId(const EventListenerInfo& details) {

[Devlin, 2015/10/29 18:26:44]

nit: Isn't the point that this can return more than just an extension id?  Maybe
GetListenerId?

[stevenjb, 2015/10/29 19:10:12]

Done.

+  std::string id = details.extension_id;

[Devlin, 2015/10/29 18:26:45]

nit:
return !details.extension_id.empty() ? details.extension_id :
details.listener_url.host();

[stevenjb, 2015/10/29 19:10:12]

Done.

+  if (!id.empty())
+    return id;
+  return details.listener_url.host();
+}
+
+}  // namespace
+
 // static
 BrowserContextKeyedAPIFactory<BluetoothPrivateAPI>*
 BluetoothPrivateAPI::GetFactoryInstance() {
@@ -47,8 +58,9 @@ void BluetoothPrivateAPI::OnListenerAdded(const EventListenerInfo& details) {
   if (!details.browser_context)
     return;
 
-  BluetoothAPI::Get(browser_context_)->event_router()->AddPairingDelegate(
-      details.extension_id);
+  BluetoothAPI::Get(browser_context_)
+      ->event_router()
+      ->AddPairingDelegate(GetExtensionId(details));
 }
 
 void BluetoothPrivateAPI::OnListenerRemoved(const EventListenerInfo& details) {
@@ -57,8 +69,9 @@ void BluetoothPrivateAPI::OnListenerRemoved(const EventListenerInfo& details) {
   if (!details.browser_context)
     return;
 
-  BluetoothAPI::Get(browser_context_)->event_router()->RemovePairingDelegate(
-      details.extension_id);
+  BluetoothAPI::Get(browser_context_)
+      ->event_router()
+      ->RemovePairingDelegate(GetExtensionId(details));
 }
 
 namespace api {
@@ -68,27 +81,17 @@ namespace {
 const char kNameProperty[] = "name";
 const char kPoweredProperty[] = "powered";
 const char kDiscoverableProperty[] = "discoverable";
-
 const char kSetAdapterPropertyError[] = "Error setting adapter properties: $1";
-
-const char kDeviceNotFoundError[] =
-    "Given address is not a valid Bluetooth device.";
-
-const char kDeviceNotConnectedError[] = "Device is not connected";
-
-const char kPairingNotEnabled[] =
-    "Pairing must be enabled to set a pairing response.";
-
+const char kDeviceNotFoundError[] = "Invalid Bluetooth device";
+const char kDeviceConnectedError[] = "Device already connected";
+const char kDeviceNotConnectedError[] = "Device not connected";
+const char kPairingNotEnabled[] = "Pairing not enabled";
 const char kInvalidPairingResponseOptions[] =
     "Invalid pairing response options";
-
-const char kAdapterNotPresent[] =
-    "Could not find a Bluetooth adapter.";
-
+const char kAdapterNotPresent[] = "Failed to find a Bluetooth adapter.";
 const char kDisconnectError[] = "Failed to disconnect device";
-
 const char kSetDiscoveryFilterFailed[] = "Failed to set discovery filter";
-
+const char kConnectFailed[] = "Connect failed";
 const char kPairingFailed[] = "Pairing failed";
 
 // Returns true if the pairing response options passed into the
@@ -97,8 +100,8 @@ bool ValidatePairingResponseOptions(
     const device::BluetoothDevice* device,
     const bt_private::SetPairingResponseOptions& options) {
   bool response = options.response != bt_private::PAIRING_RESPONSE_NONE;
-  bool pincode = options.pincode.get() != NULL;
-  bool passkey = options.passkey.get() != NULL;
+  bool pincode = options.pincode.get() != nullptr;
+  bool passkey = options.passkey.get() != nullptr;
 
   if (!response && !pincode && !passkey)
     return false;
@@ -125,6 +128,8 @@ bool ValidatePairingResponseOptions(
 
 }  // namespace
 
+////////////////////////////////////////////////////////////////////////////////
+
 BluetoothPrivateSetAdapterStateFunction::
     BluetoothPrivateSetAdapterStateFunction() {}
 
@@ -152,23 +157,20 @@ bool BluetoothPrivateSetAdapterStateFunction::DoWork(
 
   if (name && adapter->GetName() != *name) {
     pending_properties_.insert(kNameProperty);
-    adapter->SetName(*name,
-                     CreatePropertySetCallback(kNameProperty),
+    adapter->SetName(*name, CreatePropertySetCallback(kNameProperty),
                      CreatePropertyErrorCallback(kNameProperty));
   }
 
   if (powered && adapter->IsPowered() != *powered) {
     pending_properties_.insert(kPoweredProperty);
-    adapter->SetPowered(*powered,
-                        CreatePropertySetCallback(kPoweredProperty),
+    adapter->SetPowered(*powered, CreatePropertySetCallback(kPoweredProperty),
                         CreatePropertyErrorCallback(kPoweredProperty));
   }
 
   if (discoverable && adapter->IsDiscoverable() != *discoverable) {
     pending_properties_.insert(kDiscoverableProperty);
     adapter->SetDiscoverable(
-        *discoverable,
-        CreatePropertySetCallback(kDiscoverableProperty),
+        *discoverable, CreatePropertySetCallback(kDiscoverableProperty),
         CreatePropertyErrorCallback(kDiscoverableProperty));
   }
 
@@ -181,8 +183,7 @@ base::Closure
 BluetoothPrivateSetAdapterStateFunction::CreatePropertySetCallback(
     const std::string& property_name) {
   return base::Bind(
-      &BluetoothPrivateSetAdapterStateFunction::OnAdapterPropertySet,
-      this,
+      &BluetoothPrivateSetAdapterStateFunction::OnAdapterPropertySet, this,
       property_name);
 }
 
@@ -190,8 +191,7 @@ base::Closure
 BluetoothPrivateSetAdapterStateFunction::CreatePropertyErrorCallback(
     const std::string& property_name) {
   return base::Bind(
-      &BluetoothPrivateSetAdapterStateFunction::OnAdapterPropertyError,
-      this,
+      &BluetoothPrivateSetAdapterStateFunction::OnAdapterPropertyError, this,
       property_name);
 }
 
@@ -225,18 +225,19 @@ void BluetoothPrivateSetAdapterStateFunction::SendError() {
   DCHECK(!failed_properties_.empty());
 
   std::vector<std::string> failed_vector;
-  std::copy(failed_properties_.begin(),
-            failed_properties_.end(),
+  std::copy(failed_properties_.begin(), failed_properties_.end(),
             std::back_inserter(failed_vector));
 
   std::vector<std::string> replacements(1);
   replacements[0] = base::JoinString(failed_vector, ", ");
   std::string error = base::ReplaceStringPlaceholders(kSetAdapterPropertyError,
-                                                      replacements, NULL);
+                                                      replacements, nullptr);
   SetError(error);
   SendResponse(false);
 }
 
+////////////////////////////////////////////////////////////////////////////////
+
 BluetoothPrivateSetPairingResponseFunction::
     BluetoothPrivateSetPairingResponseFunction() {}
 
@@ -296,12 +297,13 @@ bool BluetoothPrivateSetPairingResponseFunction::DoWork(
   return true;
 }
 
+////////////////////////////////////////////////////////////////////////////////
+
 BluetoothPrivateDisconnectAllFunction::BluetoothPrivateDisconnectAllFunction() {
 }
 
 BluetoothPrivateDisconnectAllFunction::
-    ~BluetoothPrivateDisconnectAllFunction() {
-}
+    ~BluetoothPrivateDisconnectAllFunction() {}
 
 bool BluetoothPrivateDisconnectAllFunction::DoWork(
     scoped_refptr<device::BluetoothAdapter> adapter) {
@@ -350,14 +352,7 @@ void BluetoothPrivateDisconnectAllFunction::OnErrorCallback(
   SendResponse(false);
 }
 
-void BluetoothPrivateSetDiscoveryFilterFunction::OnSuccessCallback() {
-  SendResponse(true);
-}
-
-void BluetoothPrivateSetDiscoveryFilterFunction::OnErrorCallback() {
-  SetError(kSetDiscoveryFilterFailed);
-  SendResponse(false);
-}
+////////////////////////////////////////////////////////////////////////////////

[Devlin, 2015/10/29 18:26:44]

I personally think that dividing lines like this between each extension function
is a little much, but I don't really care either way (so probably just do
whatever a) is done in the rest of this API or b) you feel passionately about)

[stevenjb, 2015/10/29 19:10:12]

(b) :) I find it *really* helpful to be able to quickly group the functions and
their associated observers.

 
 bool BluetoothPrivateSetDiscoveryFilterFunction::DoWork(
     scoped_refptr<device::BluetoothAdapter> adapter) {
@@ -420,24 +415,108 @@ bool BluetoothPrivateSetDiscoveryFilterFunction::DoWork(
   return true;
 }
 
-BluetoothPrivatePairFunction::BluetoothPrivatePairFunction() {}
+void BluetoothPrivateSetDiscoveryFilterFunction::OnSuccessCallback() {
+  SendResponse(true);
+}
 
-BluetoothPrivatePairFunction::~BluetoothPrivatePairFunction() {}
+void BluetoothPrivateSetDiscoveryFilterFunction::OnErrorCallback() {
+  SetError(kSetDiscoveryFilterFailed);
+  SendResponse(false);
+}
 
-void BluetoothPrivatePairFunction::OnSuccessCallback() {
+////////////////////////////////////////////////////////////////////////////////
+
+BluetoothPrivateConnectFunction::BluetoothPrivateConnectFunction() {}
+
+BluetoothPrivateConnectFunction::~BluetoothPrivateConnectFunction() {}
+
+bool BluetoothPrivateConnectFunction::DoWork(
+    scoped_refptr<device::BluetoothAdapter> adapter) {
+  scoped_ptr<bt_private::Connect::Params> params(
+      bt_private::Connect::Params::Create(*args_));
+  EXTENSION_FUNCTION_VALIDATE(params.get());
+
+  device::BluetoothDevice* device = adapter->GetDevice(params->device_address);
+  if (!device) {
+    SetError(kDeviceNotFoundError);
+    SendResponse(false);
+    return true;

[Devlin, 2015/10/29 18:26:44]

Gah.  Talk about hard to read.

Two big problems:
1. DoWork() is an abstraction on an (outdated) abstraction.  New extension
functions should use Run() instead of RunAsync(), and it makes it *much* easier
to understand what's going on.  Not to mention cleaning up the
SetError/SendResponse/etc calls.
2. This is a random bool that's never checked and doesn't make sense.

Unfortunately, both of those probably rely on refactoring
BluetoothExtensionFunction, which could be a lot of work. :/

I think the best answer for now would be to add a new BluetoothExtensionFunction
type that doesn't use RunSync/Async, rename the old one globally to indicate
it's bad, and write the new function in the new style.  That way, at least,
we're not adding *more* functions that are hard to read and need to one day be
converted.  But if you think it's too much work in this CL, I understand, and
won't push for it.

[stevenjb, 2015/10/29 19:10:12]

1. This is the last function that needs adding, at least for Settings. I would
much prefer to keep it consistent with the other function in this API and
schedule it for re-factoring.

2. I tried changing DoWork to return void which would make much more sense, but
EXTENSION_FUNCTION_VALIDATE returns a book. See (1).

[Devlin, 2015/10/29 19:32:08]

Re 1, okay, as long as this really is the last one. ;)
Re 2, shoot.  Ah well.  It'll be fixed once we fix 1.

+  }
+
+  if (device->IsConnected()) {
+    SetError(kDeviceConnectedError);
+    SendResponse(false);
+    return true;
+  }
+
+  device::BluetoothDevice::PairingDelegate* pairing_delegate =
+      BluetoothAPI::Get(browser_context())
+          ->event_router()
+          ->GetPairingDelegate(GetExtensionId());
+  if (!pairing_delegate) {
+    LOG(ERROR) << "No pairing delegate for: " << GetExtensionId();
+    SetError(kPairingNotEnabled);
+    SendResponse(false);
+    return true;
+  }
+  device->Connect(
+      pairing_delegate,
+      base::Bind(&BluetoothPrivateConnectFunction::OnSuccessCallback, this),
+      base::Bind(&BluetoothPrivateConnectFunction::OnErrorCallback, this));
+  return true;
+}
+
+void BluetoothPrivateConnectFunction::OnSuccessCallback() {
+  results_ = bt_private::Connect::Results::Create(
+      bt_private::CONNECT_RESULT_TYPE_SUCCESS);
   SendResponse(true);
 }
 
-void BluetoothPrivatePairFunction::OnErrorCallback(
+void BluetoothPrivateConnectFunction::OnErrorCallback(
     device::BluetoothDevice::ConnectErrorCode error) {
-  SetError(kPairingFailed);
+  bt_private::ConnectResultType result = bt_private::CONNECT_RESULT_TYPE_NONE;
+  switch (error) {
+    case device::BluetoothDevice::ERROR_UNKNOWN:
+      result = bt_private::CONNECT_RESULT_TYPE_UNKNOWNERROR;
+      break;
+    case device::BluetoothDevice::ERROR_INPROGRESS:
+      result = bt_private::CONNECT_RESULT_TYPE_INPROGRESS;
+      break;
+    case device::BluetoothDevice::ERROR_FAILED:
+      result = bt_private::CONNECT_RESULT_TYPE_FAILED;
+      break;
+    case device::BluetoothDevice::ERROR_AUTH_FAILED:
+      result = bt_private::CONNECT_RESULT_TYPE_AUTHFAILED;
+      break;
+    case device::BluetoothDevice::ERROR_AUTH_CANCELED:
+      result = bt_private::CONNECT_RESULT_TYPE_AUTHCANCELED;
+      break;
+    case device::BluetoothDevice::ERROR_AUTH_REJECTED:
+      result = bt_private::CONNECT_RESULT_TYPE_AUTHREJECTED;
+      break;
+    case device::BluetoothDevice::ERROR_AUTH_TIMEOUT:
+      result = bt_private::CONNECT_RESULT_TYPE_AUTHTIMEOUT;
+      break;
+    case device::BluetoothDevice::ERROR_UNSUPPORTED_DEVICE:
+      result = bt_private::CONNECT_RESULT_TYPE_UNSUPPORTEDDEVICE;
+      break;
+  }
+  results_ = bt_private::Connect::Results::Create(result);
+  SetError(kConnectFailed);
   SendResponse(false);
 }
 
+////////////////////////////////////////////////////////////////////////////////
+
+BluetoothPrivatePairFunction::BluetoothPrivatePairFunction() {}
+
+BluetoothPrivatePairFunction::~BluetoothPrivatePairFunction() {}
+
 bool BluetoothPrivatePairFunction::DoWork(
     scoped_refptr<device::BluetoothAdapter> adapter) {
   scoped_ptr<bt_private::Pair::Params> params(
       bt_private::Pair::Params::Create(*args_));
+  EXTENSION_FUNCTION_VALIDATE(params.get());
 
   device::BluetoothDevice* device = adapter->GetDevice(params->device_address);
   if (!device) {
@@ -461,6 +540,18 @@ bool BluetoothPrivatePairFunction::DoWork(
   return true;
 }
 
+void BluetoothPrivatePairFunction::OnSuccessCallback() {
+  SendResponse(true);
+}
+
+void BluetoothPrivatePairFunction::OnErrorCallback(
+    device::BluetoothDevice::ConnectErrorCode error) {
+  SetError(kPairingFailed);
+  SendResponse(false);
+}
+
+////////////////////////////////////////////////////////////////////////////////
+
 }  // namespace api
 
 }  // namespace extensions