Bluetooth: Mac: implementation for start notification

device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc

Index: device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc
diff --git a/device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc b/device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc
index fda119d5c47371d4cf40e94abe53ba8e88e76a31..d9d65b27859cabc829b54ca5a0c60645a31510f4 100644
--- a/device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc
+++ b/device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc
@@ -86,8 +86,11 @@ class BluetoothRemoteGattCharacteristicTest : public BluetoothTest {
               .canonical_value());
       expected_descriptors_count++;
     }
+#if !defined(OS_MACOSX)

[ortuno, 2016/06/28 16:24:50]

Move this right before expected_descriptors_count = 0;

[jlebel, 2016/06/28 18:09:52]

Done.

+    // TODO(jlebel) Need to implement descriptors.

[ortuno, 2016/06/28 16:24:50]

I don't think we need the TODO as we don't need to mess with descriptors for
enabling notifications on macOS. Something like the following should suffice:

macOS: Not applicable. CoreBluetooth exposes setNotifyValue:forCharacteristic:
which handles all interactions with the CCC descriptor.

[jlebel, 2016/06/28 18:09:53]

Done.

     ASSERT_EQ(expected_descriptors_count,
               characteristic1_->GetDescriptors().size());
+#endif  // !defined(OS_MACOSX)
 
     if (error == StartNotifySetupError::SET_NOTIFY) {
       SimulateGattCharacteristicSetNotifyWillFailSynchronouslyOnce(
@@ -122,12 +125,15 @@ class BluetoothRemoteGattCharacteristicTest : public BluetoothTest {
     EXPECT_TRUE(notify_sessions_[0]->IsActive());
 
     // Verify the Client Characteristic Configuration descriptor was written to.
+#if !defined(OS_MACOSX)
+    // TODO(jlebel) Need to implement descriptors.

[ortuno, 2016/06/28 16:24:50]

No need for the TODO. Change to match the same comment as above.

[jlebel, 2016/06/28 18:09:53]

Done.

     EXPECT_EQ(1, gatt_write_descriptor_attempts_);
     EXPECT_EQ(2u, last_write_value_.size());
     uint8_t expected_byte0 = expected_config_descriptor_value & 0xFF;
     uint8_t expected_byte1 = (expected_config_descriptor_value >> 8) & 0xFF;
     EXPECT_EQ(expected_byte0, last_write_value_[0]);
     EXPECT_EQ(expected_byte1, last_write_value_[1]);
+#endif  // !defined(OS_MACOSX)
   }
 
   BluetoothDevice* device_ = nullptr;
@@ -343,8 +349,7 @@ TEST_F(BluetoothRemoteGattCharacteristicTest, WriteRemoteCharacteristic_Empty) {
 
 #if defined(OS_ANDROID) || defined(OS_WIN)
 // Tests ReadRemoteCharacteristic completing after Chrome objects are deleted.
-// This test is not relevant for macOS. This can never happen if CBPeripheral
-// delegate is set to nil.
+// macOS: Not applicable: CoreBluetooth doesn't support synchronous API.

[ortuno, 2016/06/28 16:24:50]

I think you meant to use the comment on the following test :)

[jlebel, 2016/06/28 18:09:53]

Done.

 TEST_F(BluetoothRemoteGattCharacteristicTest,
        ReadRemoteCharacteristic_AfterDeleted) {
   if (!PlatformSupportsLowEnergy()) {
@@ -370,8 +375,8 @@ TEST_F(BluetoothRemoteGattCharacteristicTest,
 
 #if defined(OS_ANDROID) || defined(OS_WIN)
 // Tests WriteRemoteCharacteristic completing after Chrome objects are deleted.
-// This test is not relevant for macOS. This can never happen if CBPeripheral
-// delegate is set to nil.
+// macOS: Not applicable: This can never happen if CBPeripheral delegate is set
+// to nil.
 TEST_F(BluetoothRemoteGattCharacteristicTest,
        WriteRemoteCharacteristic_AfterDeleted) {
   if (!PlatformSupportsLowEnergy()) {
@@ -895,7 +900,7 @@ TEST_F(BluetoothRemoteGattCharacteristicTest,
 }
 #endif  // defined(OS_ANDROID) || defined(OS_MACOSX) || defined(OS_WIN)
 
-#if defined(OS_ANDROID) || defined(OS_WIN)
+#if defined(OS_ANDROID) || defined(OS_MACOSX) || defined(OS_WIN)
 // StartNotifySession fails if characteristic doesn't have Notify or Indicate
 // property.
 TEST_F(BluetoothRemoteGattCharacteristicTest,
@@ -914,7 +919,7 @@ TEST_F(BluetoothRemoteGattCharacteristicTest,
   EXPECT_EQ(BluetoothRemoteGattService::GATT_ERROR_NOT_SUPPORTED,
             last_gatt_error_code_);
 }
-#endif  // defined(OS_ANDROID) || defined(OS_WIN)
+#endif  // defined(OS_ANDROID) || defined(OS_MACOSX) || defined(OS_WIN)
 
 #if defined(OS_ANDROID) || defined(OS_WIN)
 // StartNotifySession fails if the characteristic is missing the Client
@@ -962,6 +967,7 @@ TEST_F(BluetoothRemoteGattCharacteristicTest,
 // StartNotifySession fails synchronously when failing to set a characteristic
 // to enable notifications.
 // Android: This is mBluetoothGatt.setCharacteristicNotification failing.
+// macOS: Not applicable: CoreBluetooth doesn't support synchronous API.
 // Windows: Synchronous Test Not Applicable: OS calls are all made
 // asynchronously from BluetoothTaskManagerWin.
 TEST_F(BluetoothRemoteGattCharacteristicTest,
@@ -983,6 +989,7 @@ TEST_F(BluetoothRemoteGattCharacteristicTest,
 
 #if defined(OS_ANDROID)
 // Tests StartNotifySession descriptor write synchronous failure.
+// macOS: Not applicable: CoreBluetooth doesn't support synchronous API.

[ortuno, 2016/06/28 16:24:50]

I think this should be: macOS. Not applicable: No need to write to the
descriptor manually. setNotifyValue:forCharacteristic: takes care of it.

[jlebel, 2016/06/28 18:09:52]

Done.

 // Windows: Synchronous Test Not Applicable: OS calls are all made
 // asynchronously from BluetoothTaskManagerWin.
 TEST_F(BluetoothRemoteGattCharacteristicTest,
@@ -1002,25 +1009,25 @@ TEST_F(BluetoothRemoteGattCharacteristicTest,
 }
 #endif  // defined(OS_ANDROID)
 
-#if defined(OS_ANDROID) || defined(OS_WIN)
+#if defined(OS_ANDROID) || defined(OS_MACOSX) || defined(OS_WIN)
 // Tests StartNotifySession success on a characteristic enabling Notify.
 TEST_F(BluetoothRemoteGattCharacteristicTest, StartNotifySession) {
   ASSERT_NO_FATAL_FAILURE(StartNotifyBoilerplate(
       /* properties: NOTIFY */ 0x10,
       /* expected_config_descriptor_value: NOTIFY */ 1));
 }
-#endif  // defined(OS_ANDROID) || defined(OS_WIN)
+#endif  // defined(OS_ANDROID) || defined(OS_MACOSX) || defined(OS_WIN)
 
-#if defined(OS_ANDROID) || defined(OS_WIN)
+#if defined(OS_ANDROID) || defined(OS_MACOSX) || defined(OS_WIN)
 // Tests StartNotifySession success on a characteristic enabling Indicate.
 TEST_F(BluetoothRemoteGattCharacteristicTest, StartNotifySession_OnIndicate) {
   ASSERT_NO_FATAL_FAILURE(StartNotifyBoilerplate(
       /* properties: INDICATE */ 0x20,
       /* expected_config_descriptor_value: INDICATE */ 2));
 }
-#endif  // defined(OS_ANDROID) || defined(OS_WIN)
+#endif  // defined(OS_ANDROID) || defined(OS_MACOSX) || defined(OS_WIN)
 
-#if defined(OS_ANDROID) || defined(OS_WIN)
+#if defined(OS_ANDROID) || defined(OS_MACOSX) || defined(OS_WIN)
 // Tests StartNotifySession success on a characteristic enabling Notify &
 // Indicate.
 TEST_F(BluetoothRemoteGattCharacteristicTest,
@@ -1029,9 +1036,9 @@ TEST_F(BluetoothRemoteGattCharacteristicTest,
       /* properties: NOTIFY and INDICATE bits set */ 0x30,
       /* expected_config_descriptor_value: NOTIFY */ 1));
 }
-#endif  // defined(OS_ANDROID) || defined(OS_WIN)
+#endif  // defined(OS_ANDROID) || defined(OS_MACOSX) || defined(OS_WIN)
 
-#if defined(OS_ANDROID) || defined(OS_WIN)
+#if defined(OS_ANDROID) || defined(OS_MACOSX) || defined(OS_WIN)
 // Tests multiple StartNotifySession success.
 TEST_F(BluetoothRemoteGattCharacteristicTest, StartNotifySession_Multiple) {
   ASSERT_NO_FATAL_FAILURE(
@@ -1040,7 +1047,10 @@ TEST_F(BluetoothRemoteGattCharacteristicTest, StartNotifySession_Multiple) {
       characteristic1_,
       BluetoothRemoteGattDescriptor::ClientCharacteristicConfigurationUuid()
           .canonical_value());
+#if !defined(OS_MACOSX)
+  // TODO(jlebel) Need to implement descriptors.

[ortuno, 2016/06/28 16:24:50]

nit: open/point to an issue.

[jlebel, 2016/06/28 18:09:53]

Done.

   ASSERT_EQ(1u, characteristic1_->GetDescriptors().size());
+#endif  // !defined(OS_MACOSX)
 
   characteristic1_->StartNotifySession(
       GetNotifyCallback(Call::EXPECTED),
@@ -1063,9 +1073,9 @@ TEST_F(BluetoothRemoteGattCharacteristicTest, StartNotifySession_Multiple) {
   EXPECT_TRUE(notify_sessions_[0]->IsActive());
   EXPECT_TRUE(notify_sessions_[1]->IsActive());
 }
-#endif  // defined(OS_ANDROID) || defined(OS_WIN)
+#endif  // defined(OS_ANDROID) || defined(OS_MACOSX) || defined(OS_WIN)
 
-#if defined(OS_ANDROID)
+#if defined(OS_ANDROID) || defined(OS_MACOSX)
 // Tests multiple StartNotifySessions pending and then an error.
 TEST_F(BluetoothRemoteGattCharacteristicTest,
        StartNotifySessionError_Multiple) {
@@ -1075,7 +1085,10 @@ TEST_F(BluetoothRemoteGattCharacteristicTest,
       characteristic1_,
       BluetoothRemoteGattDescriptor::ClientCharacteristicConfigurationUuid()
           .canonical_value());
+#if !defined(OS_MACOSX)
+  // TODO(jlebel) Need to implement descriptors.

[ortuno, 2016/06/28 16:24:50]

nit: same here, point to an issue.

[jlebel, 2016/06/28 18:09:53]

Done.

   ASSERT_EQ(1u, characteristic1_->GetDescriptors().size());
+#endif  // !defined(OS_MACOSX)
 
   characteristic1_->StartNotifySession(GetNotifyCallback(Call::NOT_EXPECTED),
                                        GetGattErrorCallback(Call::EXPECTED));
@@ -1091,10 +1104,12 @@ TEST_F(BluetoothRemoteGattCharacteristicTest,
   EXPECT_EQ(BluetoothRemoteGattService::GATT_ERROR_FAILED,
             last_gatt_error_code_);
 }
-#endif  // defined(OS_ANDROID)
+#endif  // defined(OS_ANDROID) || defined(OS_MACOSX)
 
 #if defined(OS_ANDROID)
 // Tests StartNotifySession completing after chrome objects are deleted.
+// macOS: Not applicable: This can never happen if CBPeripheral delegate is set
+// to nil.
 TEST_F(BluetoothRemoteGattCharacteristicTest, StartNotifySession_AfterDeleted) {
   ASSERT_NO_FATAL_FAILURE(
       FakeCharacteristicBoilerplate(/* properties: NOTIFY */ 0x10));
@@ -1102,7 +1117,10 @@ TEST_F(BluetoothRemoteGattCharacteristicTest, StartNotifySession_AfterDeleted) {
       characteristic1_,
       BluetoothRemoteGattDescriptor::ClientCharacteristicConfigurationUuid()
           .canonical_value());
+#if !defined(OS_MACOSX)

[ortuno, 2016/06/28 16:24:50]

You don't need this one since this test doesn't apply to macOS.

[jlebel, 2016/06/28 18:09:52]

Done.

+  // TODO(jlebel) Need to implement descriptors.
   ASSERT_EQ(1u, characteristic1_->GetDescriptors().size());
+#endif  // !defined(OS_MACOSX)
 
   characteristic1_->StartNotifySession(GetNotifyCallback(Call::NOT_EXPECTED),
                                        GetGattErrorCallback(Call::EXPECTED));
@@ -1122,7 +1140,7 @@ TEST_F(BluetoothRemoteGattCharacteristicTest, StartNotifySession_AfterDeleted) {
 }
 #endif  // defined(OS_ANDROID)
 
-#if defined(OS_WIN)
+#if defined(OS_MACOSX) || defined(OS_WIN)
 // Tests StartNotifySession reentrant in start notify session success callback
 // and the reentrant start notify session success.
 TEST_F(BluetoothRemoteGattCharacteristicTest,
@@ -1132,7 +1150,10 @@ TEST_F(BluetoothRemoteGattCharacteristicTest,
   SimulateGattDescriptor(
       characteristic1_,
       BluetoothGattDescriptor::ClientCharacteristicConfigurationUuid().value());
+#if !defined(OS_MACOSX)
+  // TODO(jlebel) Need to implement descriptors.
   ASSERT_EQ(1u, characteristic1_->GetDescriptors().size());
+#endif  // !defined(OS_MACOSX)
 
   characteristic1_->StartNotifySession(
       GetReentrantStartNotifySessionSuccessCallback(Call::EXPECTED,
@@ -1146,7 +1167,7 @@ TEST_F(BluetoothRemoteGattCharacteristicTest,
 
   // Simulate reentrant StartNotifySession request from
   // BluetoothTestBase::ReentrantStartNotifySessionSuccessCallback.
-  SimulateGattNotifySessionStarted(characteristic1_);
+  base::RunLoop().RunUntilIdle();
   EXPECT_EQ(1, gatt_notify_characteristic_attempts_);
   EXPECT_EQ(2, callback_count_);
   EXPECT_EQ(0, error_callback_count_);
@@ -1158,7 +1179,7 @@ TEST_F(BluetoothRemoteGattCharacteristicTest,
     EXPECT_TRUE(notify_sessions_[i]->IsActive());
   }
 }
-#endif  // defined(OS_WIN)
+#endif  // defined(OS_MACOSX) || defined(OS_WIN)
 
 #if defined(OS_WIN)
 // Tests StartNotifySession reentrant in start notify session error callback
@@ -1170,7 +1191,10 @@ TEST_F(BluetoothRemoteGattCharacteristicTest,
   SimulateGattDescriptor(
       characteristic1_,
       BluetoothGattDescriptor::ClientCharacteristicConfigurationUuid().value());
+#if !defined(OS_MACOSX)
+  // TODO(jlebel) Need to implement descriptors.

[ortuno, 2016/06/28 16:24:50]

Remove since this is not enabled on macOS yet.

[jlebel, 2016/06/28 18:09:52]

Done.

   ASSERT_EQ(1u, characteristic1_->GetDescriptors().size());
+#endif  // !defined(OS_MACOSX)
 
   SimulateGattNotifySessionStartError(
       characteristic1_, BluetoothRemoteGattService::GATT_ERROR_UNKNOWN);
@@ -1209,7 +1233,10 @@ TEST_F(BluetoothRemoteGattCharacteristicTest,
   SimulateGattDescriptor(
       characteristic1_,
       BluetoothGattDescriptor::ClientCharacteristicConfigurationUuid().value());
+#if !defined(OS_MACOSX)

[ortuno, 2016/06/28 16:24:50]

Remove since this is not enabled on macOS yet.

[jlebel, 2016/06/28 18:09:53]

Done.

+  // TODO(jlebel) Need to implement descriptors.
   ASSERT_EQ(1u, characteristic1_->GetDescriptors().size());
+#endif  // !defined(OS_MACOSX)
 
   SimulateGattNotifySessionStartError(
       characteristic1_, BluetoothRemoteGattService::GATT_ERROR_UNKNOWN);
@@ -1233,7 +1260,7 @@ TEST_F(BluetoothRemoteGattCharacteristicTest,
 }
 #endif  // defined(OS_WIN)
 
-#if defined(OS_ANDROID) || defined(OS_WIN)
+#if defined(OS_ANDROID) || defined(OS_MACOSX) || defined(OS_WIN)
 // Tests Characteristic Value changes during a Notify Session.
 TEST_F(BluetoothRemoteGattCharacteristicTest, GattCharacteristicValueChanged) {
   ASSERT_NO_FATAL_FAILURE(StartNotifyBoilerplate(
@@ -1254,11 +1281,13 @@ TEST_F(BluetoothRemoteGattCharacteristicTest, GattCharacteristicValueChanged) {
   EXPECT_EQ(2, observer.gatt_characteristic_value_changed_count());
   EXPECT_EQ(test_vector2, characteristic1_->GetValue());
 }
-#endif  // defined(OS_ANDROID) || defined(OS_WIN)
+#endif  // defined(OS_ANDROID) || defined(OS_MACOSX) || defined(OS_WIN)
 
 #if defined(OS_ANDROID) || defined(OS_WIN)
 // Tests Characteristic Value changing after a Notify Session and objects being
 // destroyed.
+// macOS: Not applicable: This can never happen if CBPeripheral delegate is set
+// to nil.
 TEST_F(BluetoothRemoteGattCharacteristicTest,
        GattCharacteristicValueChanged_AfterDeleted) {
   ASSERT_NO_FATAL_FAILURE(StartNotifyBoilerplate(