Add basic error states to RemoteDevice::Connect response

A basic true/false does not encapsulate the full error reasons. Some
clients may want to behave different on a ConnectionPending error vs a
real connection failure.

Bug: None
Test: None
Change-Id: Ic38eaad6dd0d27046e9c9d8d6fe45847756046e1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2431408
Commit-Queue: Matt Swartwout <mwswartwout@google.com>
Auto-Submit: Matt Swartwout <mwswartwout@google.com>
Reviewed-by: Luke Halliwell (slow) <halliwell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#811433}

chromecast/device/bluetooth/le/gatt_client_manager_impl_test.cc

@@ -220,8 +220,8 @@ class GattClientManagerTest : public ::testing::Test {
   void Connect(const bluetooth_v2_shlib::Addr& addr) {
     EXPECT_CALL(*gatt_client_, Connect(addr)).WillOnce(Return(true));
     scoped_refptr<RemoteDevice> device = GetDevice(addr);
-    EXPECT_CALL(cb_, Run(true));
+    EXPECT_CALL(connect_cb_, Run(RemoteDevice::ConnectStatus::kSuccess));
-    device->Connect(cb_.Get());
+    device->Connect(connect_cb_.Get());
     bluetooth_v2_shlib::Gatt::Client::Delegate* delegate =
         gatt_client_->delegate();
     EXPECT_CALL(*gatt_client_, GetServices(addr)).WillOnce(Return(true));
@@ -231,6 +231,7 @@ class GattClientManagerTest : public ::testing::Test {
   }
 
   base::MockCallback<RemoteDevice::StatusCallback> cb_;
+  base::MockCallback<RemoteDevice::ConnectCallback> connect_cb_;
   base::test::SingleThreadTaskEnvironment task_environment_{
       base::test::TaskEnvironment::TimeSource::MOCK_TIME};
   std::unique_ptr<GattClientManagerImpl> gatt_client_manager_;
@@ -274,14 +275,14 @@ TEST_F(GattClientManagerTest, RemoteDeviceConnect) {
   EXPECT_CALL(*gatt_client_, Connect(kTestAddr1)).WillOnce(Return(false));
   EXPECT_CALL(*gatt_client_, ClearPendingConnect(kTestAddr1))
       .WillOnce(Return(true));
-  EXPECT_CALL(cb_, Run(false));
+  EXPECT_CALL(connect_cb_, Run(RemoteDevice::ConnectStatus::kFailure));
-  device->Connect(cb_.Get());
+  device->Connect(connect_cb_.Get());
   EXPECT_FALSE(device->IsConnected());
 
   // Second connect request succeeds.
   EXPECT_CALL(*gatt_client_, Connect(kTestAddr1)).WillOnce(Return(true));
-  EXPECT_CALL(cb_, Run(true));
+  EXPECT_CALL(connect_cb_, Run(RemoteDevice::ConnectStatus::kSuccess));
-  device->Connect(cb_.Get());
+  device->Connect(connect_cb_.Get());
   EXPECT_CALL(*gatt_client_, GetServices(kTestAddr1)).WillOnce(Return(true));
   delegate->OnConnectChanged(kTestAddr1, true /* status */,
                              true /* connected */);
@@ -411,10 +412,10 @@ TEST_F(GattClientManagerTest, RemoteDeviceConnectConcurrent) {
   scoped_refptr<RemoteDevice> device4 = GetDevice(kTestAddr4);
   scoped_refptr<RemoteDevice> device5 = GetDevice(kTestAddr5);
 
-  base::MockCallback<RemoteDevice::StatusCallback> cb1;
+  base::MockCallback<RemoteDevice::ConnectCallback> cb1;
-  base::MockCallback<RemoteDevice::StatusCallback> cb2;
+  base::MockCallback<RemoteDevice::ConnectCallback> cb2;
-  base::MockCallback<RemoteDevice::StatusCallback> cb3;
+  base::MockCallback<RemoteDevice::ConnectCallback> cb3;
-  base::MockCallback<RemoteDevice::StatusCallback> cb4;
+  base::MockCallback<RemoteDevice::ConnectCallback> cb4;
   base::MockCallback<RemoteDevice::StatusCallback> cb5;
 
   // Device5 is already connected at the beginning.
@@ -435,15 +436,15 @@ TEST_F(GattClientManagerTest, RemoteDeviceConnectConcurrent) {
 
   // Queued Connect requests will not be called until we receive OnGetServices
   // of the current Connect request if it is successful.
-  EXPECT_CALL(cb1, Run(true));
+  EXPECT_CALL(cb1, Run(RemoteDevice::ConnectStatus::kSuccess));
   EXPECT_CALL(*gatt_client_, Connect(kTestAddr2)).WillOnce(Return(false));
-  EXPECT_CALL(cb2, Run(false));
+  EXPECT_CALL(cb2, Run(RemoteDevice::ConnectStatus::kFailure));
   // If the Connect request fails in the initial request (not in the callback),
   // the next queued request will be executed immediately.
   EXPECT_CALL(*gatt_client_, Connect(kTestAddr3)).WillOnce(Return(true));
   delegate->OnGetServices(kTestAddr1, {});
 
-  EXPECT_CALL(cb3, Run(false));
+  EXPECT_CALL(cb3, Run(RemoteDevice::ConnectStatus::kFailure));
   EXPECT_CALL(*gatt_client_, Connect(kTestAddr4)).WillOnce(Return(true));
   delegate->OnConnectChanged(kTestAddr3, true /* status */,
                              false /* connected */);
@@ -452,7 +453,7 @@ TEST_F(GattClientManagerTest, RemoteDeviceConnectConcurrent) {
   delegate->OnConnectChanged(kTestAddr4, true /* status */,
                              true /* connected */);
 
-  EXPECT_CALL(cb4, Run(true));
+  EXPECT_CALL(cb4, Run(RemoteDevice::ConnectStatus::kSuccess));
   EXPECT_CALL(*gatt_client_, Disconnect(kTestAddr5)).WillOnce(Return(true));
   delegate->OnGetServices(kTestAddr4, {});
 
@@ -479,13 +480,13 @@ TEST_F(GattClientManagerTest, ConnectTimeout) {
 
   // Issue a Connect request
   EXPECT_CALL(*gatt_client_, Connect(kTestAddr1)).WillOnce(Return(true));
-  device->Connect(cb_.Get());
+  device->Connect(connect_cb_.Get());
 
   // Let Connect request timeout
   // We should expect to receive Connect failure message
   EXPECT_CALL(*gatt_client_, ClearPendingConnect(kTestAddr1))
       .WillOnce(Return(true));
-  EXPECT_CALL(cb_, Run(false));
+  EXPECT_CALL(connect_cb_, Run(RemoteDevice::ConnectStatus::kFailure));
   task_environment_.FastForwardBy(GattClientManagerImpl::kConnectTimeout);
   EXPECT_FALSE(device->IsConnected());
 }
@@ -498,7 +499,7 @@ TEST_F(GattClientManagerTest, GetServicesTimeout) {
 
   // Issue a Connect request and let Connect succeed
   EXPECT_CALL(*gatt_client_, Connect(kTestAddr1)).WillOnce(Return(true));
-  device->Connect(cb_.Get());
+  device->Connect(connect_cb_.Get());
   EXPECT_CALL(*gatt_client_, GetServices(kTestAddr1)).WillOnce(Return(true));
   delegate->OnConnectChanged(kTestAddr1, true /* status */,
                              true /* connected */);
@@ -511,7 +512,7 @@ TEST_F(GattClientManagerTest, GetServicesTimeout) {
   // Make sure we issued a disconnect.
   testing::Mock::VerifyAndClearExpectations(gatt_client_.get());
 
-  EXPECT_CALL(cb_, Run(false));
+  EXPECT_CALL(connect_cb_, Run(RemoteDevice::ConnectStatus::kFailure));
   delegate->OnConnectChanged(kTestAddr1, true /* status */,
                              false /* connected */);
 
@@ -643,7 +644,7 @@ TEST_F(GattClientManagerTest, Connectability) {
 
   // Start a connection.
   EXPECT_CALL(*gatt_client_, Connect(kTestAddr1)).WillOnce(Return(true));
-  device->Connect(cb_.Get());
+  device->Connect(connect_cb_.Get());
 
   // Disable GATT client connectability while connection is pending.
   EXPECT_TRUE(gatt_client_manager_->SetGattClientConnectable(false));
@@ -654,15 +655,15 @@ TEST_F(GattClientManagerTest, Connectability) {
   delegate->OnConnectChanged(kTestAddr1, true /* status */,
                              true /* connected */);
 
-  EXPECT_CALL(cb_, Run(false));
+  EXPECT_CALL(connect_cb_, Run(RemoteDevice::ConnectStatus::kFailure));
   delegate->OnConnectChanged(kTestAddr1, true /* status */,
                              false /* connected */);
   ASSERT_FALSE(device->IsConnected());
 
   // Connect should fail when GATT client connectability is already disabled.
   EXPECT_CALL(*gatt_client_, Connect(_)).Times(0);
-  EXPECT_CALL(cb_, Run(false));
+  EXPECT_CALL(connect_cb_, Run(RemoteDevice::ConnectStatus::kFailure));
-  device->Connect(cb_.Get());
+  device->Connect(connect_cb_.Get());
   ASSERT_FALSE(device->IsConnected());
 
   // Re-enable connectability.
@@ -1199,11 +1200,11 @@ TEST_F(GattClientManagerTest, ConnectMultiple) {
 TEST_F(GattClientManagerTest, GetServicesFailOnConnect) {
   scoped_refptr<RemoteDevice> device = GetDevice(kTestAddr1);
   EXPECT_CALL(*gatt_client_, Connect(kTestAddr1)).WillOnce(Return(true));
-  device->Connect(cb_.Get());
+  device->Connect(connect_cb_.Get());
   bluetooth_v2_shlib::Gatt::Client::Delegate* delegate =
       gatt_client_->delegate();
 
-  EXPECT_CALL(cb_, Run(false));
+  EXPECT_CALL(connect_cb_, Run(RemoteDevice::ConnectStatus::kFailure));
   EXPECT_CALL(*gatt_client_, GetServices(kTestAddr1)).WillOnce(Return(false));
   delegate->OnConnectChanged(kTestAddr1, true /* status */,
                              true /* connected */);
@@ -1221,8 +1222,8 @@ TEST_F(GattClientManagerTest, GetServicesSuccessAfterConnectCallback) {
       [](GattClientManagerTest* gcmt,
          const std::vector<bluetooth_v2_shlib::Gatt::Service>*
              expected_services,
-         bool* cb_called, bool success) {
+         bool* cb_called, RemoteDevice::ConnectStatus status) {
-        EXPECT_TRUE(success);
+        EXPECT_EQ(RemoteDevice::ConnectStatus::kSuccess, status);
         *cb_called = true;
 
         auto device = gcmt->GetDevice(kTestAddr1);

chromecast/device/bluetooth/le/mock_remote_device.h

@@ -18,8 +18,8 @@ class MockRemoteDevice : public RemoteDevice {
  public:
   explicit MockRemoteDevice(const bluetooth_v2_shlib::Addr& addr);
 
-  MOCK_METHOD0(Connect, bool());
+  MOCK_METHOD0(Connect, ConnectStatus());
-  void Connect(StatusCallback cb) override { std::move(cb).Run(Connect()); }
+  void Connect(ConnectCallback cb) override { std::move(cb).Run(Connect()); }
 
   MOCK_METHOD0(Disconnect, bool());
   void Disconnect(StatusCallback cb) override {

chromecast/device/bluetooth/le/remote_device.h

@@ -24,12 +24,21 @@ class RemoteDevice : public base::RefCountedThreadSafe<RemoteDevice> {
     kDefaultMtu = 20,
   };
 
+  enum class ConnectStatus {
+    kUndefined,
+    kGattClientManagerDestroyed,
+    kConnectPending,
+    kFailure,
+    kSuccess,
+  };
+
   using StatusCallback = base::OnceCallback<void(bool)>;
+  using ConnectCallback = base::OnceCallback<void(ConnectStatus)>;
 
   // Initiate a connection to this device. Callback will return |true| if
   // connected successfully, otherwise false. Only one pending call is allowed
   // at a time.
-  virtual void Connect(StatusCallback cb) = 0;
+  virtual void Connect(ConnectCallback cb) = 0;
 
   // Disconnect from this device. Callback will return |true| if disconnected
   // successfully, otherwise false. Only one pending call is allowed at a time.

chromecast/device/bluetooth/le/remote_device_impl.cc

@@ -69,22 +69,21 @@ RemoteDeviceImpl::RemoteDeviceImpl(
 
 RemoteDeviceImpl::~RemoteDeviceImpl() = default;
 
-void RemoteDeviceImpl::Connect(StatusCallback cb) {
+void RemoteDeviceImpl::Connect(ConnectCallback cb) {
   MAKE_SURE_IO_THREAD(Connect, BindToCurrentSequence(std::move(cb)));
   LOG(INFO) << "Connect(" << util::AddrLastByteString(addr_) << ")";
 
   if (!gatt_client_manager_) {
     LOG(ERROR) << __func__ << " failed: Destroyed";
-    EXEC_CB_AND_RET(cb, false);
+    EXEC_CB_AND_RET(cb, ConnectStatus::kGattClientManagerDestroyed);
   }
 
-  if (connect_pending_) {
+  if (connect_cb_) {
     LOG(ERROR) << __func__ << " failed: Connection pending";
-    EXEC_CB_AND_RET(cb, false);
+    EXEC_CB_AND_RET(cb, ConnectStatus::kConnectPending);
   }
 
   gatt_client_manager_->NotifyConnect(addr_);
-  connect_pending_ = true;
   connect_cb_ = std::move(cb);
   gatt_client_manager_->EnqueueConnectRequest(addr_, true);
 }
@@ -519,11 +518,9 @@ void RemoteDeviceImpl::OnReadRemoteRssiComplete(bool status, int rssi) {
 
 void RemoteDeviceImpl::ConnectComplete(bool success) {
   DCHECK(io_task_runner_->BelongsToCurrentThread());
-  if (connect_pending_) {
+  if (connect_cb_) {
-    connect_pending_ = false;
+    std::move(connect_cb_)
-    if (connect_cb_) {
+        .Run(success ? ConnectStatus::kSuccess : ConnectStatus::kFailure);
-      std::move(connect_cb_).Run(success);
-    }
   }
 }
 

chromecast/device/bluetooth/le/remote_device_impl.h

@@ -38,7 +38,7 @@ class RemoteDeviceImpl : public RemoteDevice {
       base::TimeDelta::FromSeconds(30);
 
   // RemoteDevice implementation
-  void Connect(StatusCallback cb) override;
+  void Connect(ConnectCallback cb) override;
   void Disconnect(StatusCallback cb) override;
   void CreateBond(StatusCallback cb) override;
   void RemoveBond(StatusCallback cb) override;
@@ -153,8 +153,7 @@ class RemoteDeviceImpl : public RemoteDevice {
 
   bool services_discovered_ = false;
 
-  bool connect_pending_ = false;
+  ConnectCallback connect_cb_;
-  StatusCallback connect_cb_;
 
   bool disconnect_pending_ = false;
   StatusCallback disconnect_cb_;

device/bluetooth/cast/bluetooth_device_cast.cc

@@ -361,11 +361,15 @@ void BluetoothDeviceCast::DisconnectGatt() {
   // The device is intentionally not disconnected.
 }
 
-void BluetoothDeviceCast::OnConnect(bool success) {
+void BluetoothDeviceCast::OnConnect(
+    chromecast::bluetooth::RemoteDevice::ConnectStatus status) {
+  bool success =
+      (status == chromecast::bluetooth::RemoteDevice::ConnectStatus::kSuccess);
   DVLOG(2) << __func__ << " success:" << success;
   pending_connect_ = false;
-  if (!success)
+  if (!success) {
     DidFailToConnectGatt(ERROR_FAILED);
+  }
 }
 
 }  // namespace device

device/bluetooth/cast/bluetooth_device_cast.h

@@ -121,7 +121,7 @@ class BluetoothDeviceCast : public BluetoothDevice {
   void DisconnectGatt() override;
 
   // Called back from connect requests generated from CreateGattConnectionImpl.
-  void OnConnect(bool success);
+  void OnConnect(chromecast::bluetooth::RemoteDevice::ConnectStatus status);
 
   // Called in response to GetServices
   void OnGetServices(