Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests

device/bluetooth/bluetooth_gatt_characteristic_unittest.cc

Index: device/bluetooth/bluetooth_gatt_characteristic_unittest.cc
diff --git a/device/bluetooth/bluetooth_gatt_characteristic_unittest.cc b/device/bluetooth/bluetooth_gatt_characteristic_unittest.cc
index 93a0c9ed93b17bdc5216be7b9cd0f35734bb2cb1..204faec5cef057c98deee15903470a089068836b 100644
--- a/device/bluetooth/bluetooth_gatt_characteristic_unittest.cc
+++ b/device/bluetooth/bluetooth_gatt_characteristic_unittest.cc
@@ -98,9 +98,9 @@ class BluetoothGattCharacteristicTest : public BluetoothTest {
         GetNotifyCallback(Call::EXPECTED),
         GetGattErrorCallback(Call::NOT_EXPECTED));
 
-    EXPECT_EQ(1, gatt_notify_characteristic_attempts_);
     EXPECT_EQ(0, callback_count_);
     SimulateGattNotifySessionStarted(characteristic1_);
+    EXPECT_EQ(1, gatt_notify_characteristic_attempts_);
     EXPECT_EQ(1, callback_count_);
     EXPECT_EQ(0, error_callback_count_);
     ASSERT_EQ(1u, notify_sessions_.size());
@@ -109,13 +109,16 @@ class BluetoothGattCharacteristicTest : public BluetoothTest {
               notify_sessions_[0]->GetCharacteristicIdentifier());
     EXPECT_TRUE(notify_sessions_[0]->IsActive());
 
+#ifndef OS_WIN
     // Verify the Client Characteristic Configuration descriptor was written to.
+    // Windows: No need to write CCCD explicitly to start notification.
     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
   }
 
   BluetoothDevice* device_ = nullptr;
@@ -273,13 +276,13 @@ TEST_F(BluetoothGattCharacteristicTest, ReadRemoteCharacteristic_Empty) {
   characteristic1_->ReadRemoteCharacteristic(
       GetReadValueCallback(Call::EXPECTED),
       GetGattErrorCallback(Call::NOT_EXPECTED));
-  EXPECT_EQ(1, gatt_read_characteristic_attempts_);
   std::vector<uint8_t> empty_vector;
   SimulateGattCharacteristicRead(characteristic1_, empty_vector);
 
   // Duplicate read reported from OS shouldn't cause a problem:
   SimulateGattCharacteristicRead(characteristic1_, empty_vector);
 
+  EXPECT_EQ(1, gatt_read_characteristic_attempts_);
   EXPECT_EQ(empty_vector, last_read_value_);
   EXPECT_EQ(empty_vector, characteristic1_->GetValue());
 }
@@ -295,9 +298,9 @@ TEST_F(BluetoothGattCharacteristicTest, WriteRemoteCharacteristic_Empty) {
   characteristic1_->WriteRemoteCharacteristic(
       empty_vector, GetCallback(Call::EXPECTED),
       GetGattErrorCallback(Call::NOT_EXPECTED));
-  EXPECT_EQ(1, gatt_write_characteristic_attempts_);
   SimulateGattCharacteristicWrite(characteristic1_);
 
+  EXPECT_EQ(1, gatt_write_characteristic_attempts_);
   EXPECT_EQ(empty_vector, last_write_value_);
 }
 #endif  // defined(OS_ANDROID) || defined(OS_WIN)
@@ -351,7 +354,6 @@ TEST_F(BluetoothGattCharacteristicTest, ReadRemoteCharacteristic) {
   characteristic1_->ReadRemoteCharacteristic(
       GetReadValueCallback(Call::EXPECTED),
       GetGattErrorCallback(Call::NOT_EXPECTED));
-  EXPECT_EQ(1, gatt_read_characteristic_attempts_);
 
   uint8_t values[] = {0, 1, 2, 3, 4, 0xf, 0xf0, 0xff};
   std::vector<uint8_t> test_vector(values, values + arraysize(values));
@@ -361,6 +363,7 @@ TEST_F(BluetoothGattCharacteristicTest, ReadRemoteCharacteristic) {
   std::vector<uint8_t> empty_vector;
   SimulateGattCharacteristicRead(characteristic1_, empty_vector);
 
+  EXPECT_EQ(1, gatt_read_characteristic_attempts_);
   EXPECT_EQ(test_vector, last_read_value_);
   EXPECT_EQ(test_vector, characteristic1_->GetValue());
 }
@@ -377,10 +380,10 @@ TEST_F(BluetoothGattCharacteristicTest, WriteRemoteCharacteristic) {
   characteristic1_->WriteRemoteCharacteristic(
       test_vector, GetCallback(Call::EXPECTED),
       GetGattErrorCallback(Call::NOT_EXPECTED));
-  EXPECT_EQ(1, gatt_write_characteristic_attempts_);
 
   SimulateGattCharacteristicWrite(characteristic1_);
 
+  EXPECT_EQ(1, gatt_write_characteristic_attempts_);
   EXPECT_EQ(test_vector, last_write_value_);
 }
 #endif  // defined(OS_ANDROID) || defined(OS_WIN)
@@ -394,11 +397,11 @@ TEST_F(BluetoothGattCharacteristicTest, ReadRemoteCharacteristic_Twice) {
   characteristic1_->ReadRemoteCharacteristic(
       GetReadValueCallback(Call::EXPECTED),
       GetGattErrorCallback(Call::NOT_EXPECTED));
-  EXPECT_EQ(1, gatt_read_characteristic_attempts_);
 
   uint8_t values[] = {0, 1, 2, 3, 4, 0xf, 0xf0, 0xff};
   std::vector<uint8_t> test_vector(values, values + arraysize(values));
   SimulateGattCharacteristicRead(characteristic1_, test_vector);
+  EXPECT_EQ(1, gatt_read_characteristic_attempts_);
   EXPECT_EQ(1, callback_count_);
   EXPECT_EQ(0, error_callback_count_);
   EXPECT_EQ(test_vector, last_read_value_);
@@ -409,9 +412,9 @@ TEST_F(BluetoothGattCharacteristicTest, ReadRemoteCharacteristic_Twice) {
   characteristic1_->ReadRemoteCharacteristic(
       GetReadValueCallback(Call::EXPECTED),
       GetGattErrorCallback(Call::NOT_EXPECTED));
-  EXPECT_EQ(1, gatt_read_characteristic_attempts_);
   std::vector<uint8_t> empty_vector;
   SimulateGattCharacteristicRead(characteristic1_, empty_vector);
+  EXPECT_EQ(1, gatt_read_characteristic_attempts_);
   EXPECT_EQ(1, callback_count_);
   EXPECT_EQ(0, error_callback_count_);
   EXPECT_EQ(empty_vector, last_read_value_);
@@ -430,9 +433,9 @@ TEST_F(BluetoothGattCharacteristicTest, WriteRemoteCharacteristic_Twice) {
   characteristic1_->WriteRemoteCharacteristic(
       test_vector, GetCallback(Call::EXPECTED),
       GetGattErrorCallback(Call::NOT_EXPECTED));
-  EXPECT_EQ(1, gatt_write_characteristic_attempts_);
 
   SimulateGattCharacteristicWrite(characteristic1_);
+  EXPECT_EQ(1, gatt_write_characteristic_attempts_);
   EXPECT_EQ(1, callback_count_);
   EXPECT_EQ(0, error_callback_count_);
   EXPECT_EQ(test_vector, last_write_value_);
@@ -443,8 +446,9 @@ TEST_F(BluetoothGattCharacteristicTest, WriteRemoteCharacteristic_Twice) {
   characteristic1_->WriteRemoteCharacteristic(
       empty_vector, GetCallback(Call::EXPECTED),
       GetGattErrorCallback(Call::NOT_EXPECTED));
-  EXPECT_EQ(1, gatt_write_characteristic_attempts_);
+
   SimulateGattCharacteristicWrite(characteristic1_);
+  EXPECT_EQ(1, gatt_write_characteristic_attempts_);
   EXPECT_EQ(1, callback_count_);
   EXPECT_EQ(0, error_callback_count_);
   EXPECT_EQ(empty_vector, last_write_value_);
@@ -464,7 +468,6 @@ TEST_F(BluetoothGattCharacteristicTest,
   characteristic2_->ReadRemoteCharacteristic(
       GetReadValueCallback(Call::EXPECTED),
       GetGattErrorCallback(Call::NOT_EXPECTED));
-  EXPECT_EQ(2, gatt_read_characteristic_attempts_);
   EXPECT_EQ(0, callback_count_);
   EXPECT_EQ(0, error_callback_count_);
 
@@ -478,6 +481,7 @@ TEST_F(BluetoothGattCharacteristicTest,
   SimulateGattCharacteristicRead(characteristic2_, test_vector2);
   EXPECT_EQ(test_vector2, last_read_value_);
 
+  EXPECT_EQ(2, gatt_read_characteristic_attempts_);
   EXPECT_EQ(2, callback_count_);
   EXPECT_EQ(0, error_callback_count_);
   EXPECT_EQ(test_vector1, characteristic1_->GetValue());
@@ -510,7 +514,6 @@ TEST_F(BluetoothGattCharacteristicTest,
   EXPECT_EQ(test_vector2, last_write_value_);
 #endif
 
-  EXPECT_EQ(2, gatt_write_characteristic_attempts_);
   EXPECT_EQ(0, callback_count_);
   EXPECT_EQ(0, error_callback_count_);
 
@@ -524,6 +527,7 @@ TEST_F(BluetoothGattCharacteristicTest,
   EXPECT_EQ(test_vector2, last_write_value_);
 #endif
 
+  EXPECT_EQ(2, gatt_write_characteristic_attempts_);
   EXPECT_EQ(2, callback_count_);
   EXPECT_EQ(0, error_callback_count_);
 
@@ -743,7 +747,7 @@ TEST_F(BluetoothGattCharacteristicTest, WriteRemoteCharacteristic_ReadPending) {
 }
 #endif  // defined(OS_ANDROID) || defined(OS_WIN)
 
-#if defined(OS_ANDROID)
+#if defined(OS_ANDROID) || defined(OS_WIN)
 // StartNotifySession fails if characteristic doesn't have Notify or Indicate
 // property.
 TEST_F(BluetoothGattCharacteristicTest, StartNotifySession_NoNotifyOrIndicate) {
@@ -758,12 +762,20 @@ TEST_F(BluetoothGattCharacteristicTest, StartNotifySession_NoNotifyOrIndicate) {
   EXPECT_EQ(0, error_callback_count_);
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(1, error_callback_count_);
+// TODO(crbug.com/584369) Android doesn't return the correct errors.
+#ifndef OS_ANDROID

[ortuno, 2016/03/15 02:55:31]

I think the style guide prefers #if defined(OS_ANDROID)

https://www.chromium.org/developers/coding-style#TOC-Platform-specific-code

[gogerald1, 2016/03/15 18:28:22]

Done. Do not see it explicitly, especially this looks only for Android, anyway,
change it to reviewer happy.

+  EXPECT_EQ(BluetoothGattService::GATT_ERROR_NOT_SUPPORTED,
+            last_gatt_error_code_);
+#endif

[ortuno, 2016/03/15 02:55:31]

#endif // defined(OS_ANDROID)

[gogerald1, 2016/03/15 18:28:22]

Done.

 }
-#endif  // defined(OS_ANDROID)
+#endif  // defined(OS_ANDROID) || defined(OS_WIN)
 
-#if defined(OS_ANDROID)
+#if defined(OS_ANDROID) || !defined(OS_WIN)
 // StartNotifySession fails if the characteristic is missing the Client
 // Characteristic Configuration descriptor.
+// Windows: TODO(crbug.com/594697):
+// BluetoothRemoteGattCharacteristicWin::StartNotifySession doesn't check the
+// existence of client characteristic configuration descriptor.
 TEST_F(BluetoothGattCharacteristicTest, StartNotifySession_NoConfigDescriptor) {
   ASSERT_NO_FATAL_FAILURE(StartNotifyBoilerplate(
       /* properties: NOTIFY */ 0x10,
@@ -777,12 +789,13 @@ TEST_F(BluetoothGattCharacteristicTest, StartNotifySession_NoConfigDescriptor) {
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(1, error_callback_count_);
 }
-#endif  // defined(OS_ANDROID)
+#endif  // defined(OS_ANDROID) || !defined(OS_WIN)
 
-#if defined(OS_ANDROID)
+#if defined(OS_ANDROID) || !defined(OS_WIN)
 // StartNotifySession fails synchronously when failing to set a characteristic
 // to enable notifications.
 // Android: This is mBluetoothGatt.setCharacteristicNotification failing.
+// Windows: Characteristic notification is set to OS asynchronously.
 TEST_F(BluetoothGattCharacteristicTest,
        StartNotifySession_FailToSetCharacteristicNotification) {
   ASSERT_NO_FATAL_FAILURE(StartNotifyBoilerplate(
@@ -798,10 +811,11 @@ TEST_F(BluetoothGattCharacteristicTest,
   EXPECT_EQ(0, gatt_notify_characteristic_attempts_);
   ASSERT_EQ(0u, notify_sessions_.size());
 }
-#endif  // defined(OS_ANDROID)
+#endif  // defined(OS_ANDROID) || !defined(OS_WIN)
 
-#if defined(OS_ANDROID)
+#if defined(OS_ANDROID) || !defined(OS_WIN)
 // Tests StartNotifySession descriptor write synchronous failure.
+// Windows: No need to write CCCD explicitly to start notification.
 TEST_F(BluetoothGattCharacteristicTest,
        StartNotifySession_WriteDescriptorSynchronousError) {
   ASSERT_NO_FATAL_FAILURE(StartNotifyBoilerplate(
@@ -817,27 +831,27 @@ TEST_F(BluetoothGattCharacteristicTest,
   EXPECT_EQ(1, gatt_notify_characteristic_attempts_);
   ASSERT_EQ(0u, notify_sessions_.size());
 }
-#endif  // defined(OS_ANDROID)
+#endif  // defined(OS_ANDROID) || !defined(OS_WIN)
 
-#if defined(OS_ANDROID)
+#if defined(OS_ANDROID) || defined(OS_WIN)
 // Tests StartNotifySession success on a characteristic enabling Notify.
 TEST_F(BluetoothGattCharacteristicTest, StartNotifySession) {
   ASSERT_NO_FATAL_FAILURE(StartNotifyBoilerplate(
       /* properties: NOTIFY */ 0x10,
       /* expected_config_descriptor_value: NOTIFY */ 1));
 }
-#endif  // defined(OS_ANDROID)
+#endif  // defined(OS_ANDROID) || defined(OS_WIN)
 
-#if defined(OS_ANDROID)
+#if defined(OS_ANDROID) || defined(OS_WIN)
 // Tests StartNotifySession success on a characteristic enabling Indicate.
 TEST_F(BluetoothGattCharacteristicTest, StartNotifySession_OnIndicate) {
   ASSERT_NO_FATAL_FAILURE(StartNotifyBoilerplate(
       /* properties: INDICATE */ 0x20,
       /* expected_config_descriptor_value: INDICATE */ 2));
 }
-#endif  // defined(OS_ANDROID)
+#endif  // defined(OS_ANDROID) || defined(OS_WIN)
 
-#if defined(OS_ANDROID)
+#if defined(OS_ANDROID) || defined(OS_WIN)
 // Tests StartNotifySession success on a characteristic enabling Notify &
 // Indicate.
 TEST_F(BluetoothGattCharacteristicTest,
@@ -846,9 +860,9 @@ TEST_F(BluetoothGattCharacteristicTest,
       /* properties: NOTIFY and INDICATE bits set */ 0x30,
       /* expected_config_descriptor_value: NOTIFY */ 1));
 }
-#endif  // defined(OS_ANDROID)
+#endif  // defined(OS_ANDROID) || defined(OS_WIN)
 
-#if defined(OS_ANDROID)
+#if defined(OS_ANDROID) || defined(OS_WIN)
 // Tests multiple StartNotifySession success.
 TEST_F(BluetoothGattCharacteristicTest, StartNotifySession_Multiple) {
   ASSERT_NO_FATAL_FAILURE(
@@ -865,9 +879,9 @@ TEST_F(BluetoothGattCharacteristicTest, StartNotifySession_Multiple) {
   characteristic1_->StartNotifySession(
       GetNotifyCallback(Call::EXPECTED),
       GetGattErrorCallback(Call::NOT_EXPECTED));
-  EXPECT_EQ(1, gatt_notify_characteristic_attempts_);
   EXPECT_EQ(0, callback_count_);
   SimulateGattNotifySessionStarted(characteristic1_);
+  EXPECT_EQ(1, gatt_notify_characteristic_attempts_);
   EXPECT_EQ(2, callback_count_);
   EXPECT_EQ(0, error_callback_count_);
   ASSERT_EQ(2u, notify_sessions_.size());
@@ -880,9 +894,9 @@ TEST_F(BluetoothGattCharacteristicTest, StartNotifySession_Multiple) {
   EXPECT_TRUE(notify_sessions_[0]->IsActive());
   EXPECT_TRUE(notify_sessions_[1]->IsActive());
 }
-#endif  // defined(OS_ANDROID)
+#endif  // defined(OS_ANDROID) || defined(OS_WIN)
 
-#if defined(OS_ANDROID)
+#if defined(OS_ANDROID) || defined(OS_WIN)
 // Tests Characteristic Value changes during a Notify Session.
 TEST_F(BluetoothGattCharacteristicTest, GattCharacteristicValueChanged) {
   ASSERT_NO_FATAL_FAILURE(StartNotifyBoilerplate(
@@ -903,11 +917,14 @@ TEST_F(BluetoothGattCharacteristicTest, GattCharacteristicValueChanged) {
   EXPECT_EQ(2, observer.gatt_characteristic_value_changed_count());
   EXPECT_EQ(test_vector2, characteristic1_->GetValue());
 }
-#endif  // defined(OS_ANDROID)
+#endif  // defined(OS_ANDROID) || defined(OS_WIN)
 
-#if defined(OS_ANDROID)
+#if defined(OS_ANDROID) || !defined(OS_WIN)
 // Tests Characteristic Value changing after a Notify Session and objects being
 // destroyed.
+// Windows: Characteristic changed notification is not available after deleting
+// the device because OS detects device deleting before corresponding objects
+// being destroyed.

[ortuno, 2016/03/15 02:55:31]

What happens when you enable this test on windows?

[gogerald1, 2016/03/15 18:28:22]

SimulateGattCharacteristicChanged has no target after the device has been
deleted from the OS.

[scheib, 2016/03/19 04:57:54]

ortuno mentioned in person wanting to understand this more.
RememberCharacteristicForSubsequentAction impl in BluetoothTestWin stores a
pointer, which is invalid and then can't be used by GetSimulatedCharacteristic.
That means that for Windows all tests using
RememberCharacteristicForSubsequentAction won't work.

We 3 had discussed in chat that the Windows impl expects the characteristic
object to always outlive the OS referencing that object. However, reading over
the code I see that there is a fair amount of complexity, and it's not trivial
to assert this even now, but certainly not in the future with refactoring being
done.

Please modify this code so that OnGetGattEventWin is called with cached data
from RememberCharacteristicForSubsequentAction.

To test that code works correctly it should be possible see 
 SimulateGattCharacteristicChanged(/* use remembered characteristic */ nullptr
...
succeed if a DeleteDevice(device_) was not called.

(that is, verify that RememberCharacteristicForSubsequentAction works.)

Then, after DeleteDevice is called, SimulateGattCharacteristicChanged should not
crash.

[gogerald1, 2016/03/21 14:22:22]

Done.

[ortuno, 2016/03/21 17:00:52]

Have you tried this locally? I just tried it and it's crashing.

[gogerald1, 2016/03/21 22:09:16]

Of course, and it passed trybots

[ortuno, 2016/03/22 15:02:24]

After looking into it a bit more I think we need to fix the test. The test is
incorrect because no observers are added so the code that would trigger a crash
is not being tested. If you add an observer then the test will be correct and
you'll observer a crash.

[gogerald1, 2016/03/29 15:12:00]

Done. Rebase to fixed test

 TEST_F(BluetoothGattCharacteristicTest,
        GattCharacteristicValueChanged_AfterDeleted) {
   ASSERT_NO_FATAL_FAILURE(StartNotifyBoilerplate(
@@ -922,7 +939,7 @@ TEST_F(BluetoothGattCharacteristicTest,
                                     empty_vector);
   EXPECT_TRUE("Did not crash!");
 }
-#endif  // defined(OS_ANDROID)
+#endif  // defined(OS_ANDROID) || !defined(OS_WIN)
 

[ortuno, 2016/03/15 02:55:31]

You still need to write a reentrancy test:

// Test
// ...
characteristic1_.StartNotifySession(GetReentrantStartNotifySessionCallback(
    Call::EXPECTED, characteristic1_));
EXPECT_EQ(0, callback_count_);
SimulateGattNotifySessionStarted(characteristic1_);
EXPECT_EQ(1, gatt_notify_characteristic_attempts_);
EXPECT_EQ(2, callback_count_);
EXPECT_EQ(0, error_callback_count_);
ASSERT_EQ(2u, notify_sessions_.size());
ASSERT_TRUE(notify_sessions_[1]);
EXPECT_EQ(characteristic1_->GetIdentifier(),
          notify_sessions_[0]->GetCharacteristicIdentifier());
EXPECT_TRUE(notify_sessions_[0]->IsActive());
EXPECT_EQ(characteristic1_->GetIdentifier(),
          notify_sessions_[1]->GetCharacteristicIdentifier());
EXPECT_TRUE(notify_sessions_[1]->IsActive());
// ...

// bluetooth_test.cc
BluetoothGattCharacteristic::NotifySessionCallback
BluetoothTestBase::GetReentrantStartNotifySessionCallback(
    Call expected,
    BluetoothGattCharacteristic* characteristic) {
  if (expected == Call::EXPECTED)
    ++expected_success_callback_calls_;
  return base::Bind(&BluetoothTestBase::ReentrantStartNotifySessionCallback,
                    weak_factory_.GetWeakPtr(), expected, characteristic);
}

void BluetoothTestBase::ReentrantStartNotifySessionCallback(
    Call expected,
    BluetoothGattCharacteristic* characteristic,
    scoped_ptr<BluetoothGattNotifySession> notify_session) {
  ++callback_count_;
  notify_sessions_.push_back(notify_session.release());

  if (expected == Call::EXPECTED)
    ++actual_success_callback_calls_;
  else
    unexpected_success_callback_ = true;

  characteristic->StartNotifySesssion(GetNotifyCallback(Call::EXPECTED));
}

[gogerald1, 2016/03/15 18:28:22]

Do it in the next CL

 #if defined(OS_ANDROID) || defined(OS_WIN)
 TEST_F(BluetoothGattCharacteristicTest, GetDescriptors_FindNone) {