components/arc: implement multi advertising

components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/arc/bluetooth/arc_bluetooth_bridge.h"
 
 #include <string>
 #include <utility>
 #include <vector>
 
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "base/strings/stringprintf.h"
 #include "components/arc/bluetooth/bluetooth_type_converters.h"
 #include "components/arc/common/bluetooth.mojom.h"
 #include "components/arc/test/fake_arc_bridge_service.h"
 #include "components/arc/test/fake_bluetooth_instance.h"
 #include "device/bluetooth/dbus/bluez_dbus_manager.h"
 #include "device/bluetooth/dbus/fake_bluetooth_adapter_client.h"
 #include "device/bluetooth/dbus/fake_bluetooth_device_client.h"
 #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"
 
 namespace arc {
 
 class ArcBluetoothBridgeTest : public testing::Test {
  protected:
   void AddTestDevice() {
     bluez::BluezDBusManager* dbus_manager = bluez::BluezDBusManager::Get();
     auto* fake_bluetooth_device_client =
@@ skipping 41 matching lines @@
         fake_bluetooth_instance_.get(), 2);
     arc_bluetooth_bridge_.reset(
         new ArcBluetoothBridge(fake_arc_bridge_service_.get()));
 
     device::BluetoothAdapterFactory::GetAdapter(base::Bind(
         &ArcBluetoothBridgeTest::OnAdapterInitialized, base::Unretained(this)));
     // We will quit the loop once we get the adapter.
     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_;
   scoped_refptr<device::BluetoothAdapter> adapter_;
   base::MessageLoop message_loop_;
   base::RunLoop get_adapter_run_loop_;
 };
 
 // When we add device to bluez::FakeBluetoothDeviceClient, ArcBluetoothBridge
 // should send new device data to Android. This test will then check
@@ skipping 109 matching lines @@
 
   EXPECT_EQ(device::BluetoothUUID(bluez::FakeBluetoothGattCharacteristicClient::
                                       kHeartRateControlPointUUID),
             db[4]->uuid);
   EXPECT_EQ(mojom::BluetoothGattDBAttributeType::BTGATT_DB_CHARACTERISTIC,
             db[4]->type);
   EXPECT_EQ(device::BluetoothGattCharacteristic::PROPERTY_WRITE,
             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