components/arc: implement multi advertising

components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc

Index: components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc
diff --git a/components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc b/components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc
index 62f984996eadfb88a931bd74b978d8a4f2a6de92..0ac4cf20390b51446285e227a419b144a4b5a652 100644
--- a/components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc
+++ b/components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc
@@ -21,6 +21,7 @@
 #include "device/bluetooth/dbus/fake_bluetooth_gatt_characteristic_client.h"
 #include "device/bluetooth/dbus/fake_bluetooth_gatt_descriptor_client.h"
 #include "device/bluetooth/dbus/fake_bluetooth_gatt_service_client.h"
+#include "device/bluetooth/dbus/fake_bluetooth_le_advertising_manager_client.h"
 #include "mojo/public/cpp/bindings/array.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -82,6 +83,61 @@ class ArcBluetoothBridgeTest : public testing::Test {
     get_adapter_run_loop_.Run();
   }
 
+  // Helper methods for multi advertisement tests.
+  int32_t ReserveAdvertisementHandle() {
+    int32_t ret;

[Rahul Chaturvedi, 2016/09/29 22:49:18]

The typical pattern is to have ret as a field. That way, in case the callback
gets called after we exit this method, we don't accidentally overwrite to
arbitrary values on the stack.

[Eric Caruso, 2016/09/29 22:57:34]

I'd expect the callback would be done and called after the
base::RunLoop().RunUntilIdle() call before, but that's fine. I kept it inside
the method for encapsulation purposes.

[Rahul Chaturvedi, 2016/09/30 19:51:08]

Sure, but that expectation is only valid if the code is working as it should :)
The entire purpose of a test is to verify that. There have been many cases in
other code, where the callback did not get called back when expected, but did
get called back at some random time. If a stack address were given to it, it
would instead overwrite a random variable - which would lead to completely
impossible to predict results.

Hence, we should avoid giving addresses of local variables to callbacks :)

+    arc_bluetooth_bridge_->ReserveAdvertisementHandle(base::Bind(
+        &ArcBluetoothBridgeTest::ReserveAdvertisementHandleCallback, &ret));
+
+    base::RunLoop().RunUntilIdle();
+    return ret;
+  }
+
+  mojom::BluetoothGattStatus BroadcastAdvertisement(
+      int adv_handle,
+      std::unique_ptr<device::BluetoothAdvertisement::Data> data) {
+    mojom::BluetoothGattStatus ret;
+    arc_bluetooth_bridge_->BroadcastAdvertisement(
+        adv_handle, std::move(data),
+        base::Bind(&ArcBluetoothBridgeTest::StatusSetterCallback, &ret));
+
+    base::RunLoop().RunUntilIdle();
+    return ret;
+  }
+
+  mojom::BluetoothGattStatus ReleaseAdvertisementHandle(int adv_handle) {
+    mojom::BluetoothGattStatus ret;
+    arc_bluetooth_bridge_->ReleaseAdvertisementHandle(
+        adv_handle,
+        base::Bind(&ArcBluetoothBridgeTest::StatusSetterCallback, &ret));
+
+    base::RunLoop().RunUntilIdle();
+    return ret;

[Rahul Chaturvedi, 2016/09/29 22:49:18]

Set ret to a canary value add an assert against it. What if the callback never
gets called? We'll end up with a trash value.

+  }
+
+  static void ReserveAdvertisementHandleCallback(
+      int32_t* target_adv_handle,
+      mojom::BluetoothGattStatus status,
+      int32_t adv_handle) {
+    if (status == mojom::BluetoothGattStatus::GATT_FAILURE)
+      *target_adv_handle = -1;
+    else
+      *target_adv_handle = adv_handle;
+  }
+
+  static void StatusSetterCallback(mojom::BluetoothGattStatus* target_status,
+                                   mojom::BluetoothGattStatus status) {
+    *target_status = status;
+  }
+
+  bool CurrentlyAdvertising() {

[Rahul Chaturvedi, 2016/09/29 22:49:18]

nit: IsCurrentlyAdvertising()

+    bluez::FakeBluetoothLEAdvertisingManagerClient* adv_client =
+        static_cast<bluez::FakeBluetoothLEAdvertisingManagerClient*>(
+            bluez::BluezDBusManager::Get()
+                ->GetBluetoothLEAdvertisingManagerClient());
+    return adv_client->currently_registered();
+  }
+
   std::unique_ptr<FakeArcBridgeService> fake_arc_bridge_service_;
   std::unique_ptr<FakeBluetoothInstance> fake_bluetooth_instance_;
   std::unique_ptr<ArcBluetoothBridge> arc_bluetooth_bridge_;
@@ -211,4 +267,46 @@ TEST_F(ArcBluetoothBridgeTest, GetGattDB) {
             db[4]->properties);
 }
 
+// Invoke multi advertisement methods and make sure they are going down to the
+// D-Bus clients.
+TEST_F(ArcBluetoothBridgeTest, MultiAdvertisement) {

[Rahul Chaturvedi, 2016/09/29 22:49:18]

This is just SingleAdvertisement test :)

You need to modify the fake advertising manager client to be able to handle
multiple advertisements to add a meaningful MultiAdvertisement test.

[Eric Caruso, 2016/09/29 22:57:34]

Sure. I was using this as a simple test of whether or not calls to the API were
getting advertisements all the way down to the dbus managers.

+  int32_t handle = ReserveAdvertisementHandle();
+  EXPECT_NE(handle, -1);
+  EXPECT_FALSE(CurrentlyAdvertising());
+
+  auto adv_data = base::MakeUnique<device::BluetoothAdvertisement::Data>(
+      device::BluetoothAdvertisement::ADVERTISEMENT_TYPE_BROADCAST);
+  mojom::BluetoothGattStatus status =
+      BroadcastAdvertisement(handle, std::move(adv_data));
+  EXPECT_EQ(status, mojom::BluetoothGattStatus::GATT_SUCCESS);
+  EXPECT_TRUE(CurrentlyAdvertising());
+
+  status = ReleaseAdvertisementHandle(handle);
+  EXPECT_EQ(status, mojom::BluetoothGattStatus::GATT_SUCCESS);
+  EXPECT_FALSE(CurrentlyAdvertising());
+}
+
+// Our stack currently lets us register 5 advertisements simultaneously. We
+// want to make sure we can reserve several advertisement handles without using
+// all of those 5 slots.
+// This also tests that we support releasing reserved but unused handles.
+TEST_F(ArcBluetoothBridgeTest, ReserveAndReleaseMaximumAdvertisementHandles) {
+  const size_t kChromeMaxAdvertisements = 5;
+  std::vector<int32_t> reserved_handles;
+
+  for (size_t i = 0; i < kChromeMaxAdvertisements; i++) {
+    int32_t handle = ReserveAdvertisementHandle();
+    if (handle == -1)
+      break;
+    reserved_handles.push_back(handle);
+  }
+  EXPECT_GT(reserved_handles.size(), 1UL);
+  EXPECT_LT(reserved_handles.size(), kChromeMaxAdvertisements);
+
+  for (int32_t handle : reserved_handles) {
+    EXPECT_EQ(ReleaseAdvertisementHandle(handle),
+              mojom::BluetoothGattStatus::GATT_SUCCESS);
+  }
+}
+
 }  // namespace arc