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 d320f956062082ef61589bba6d8c84f303679d7b..4832fbaa355084255a7216dff13b64d5dd69a4b0 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());
@@ -273,13 +273,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_);

[ortuno, 2016/03/09 16:53:59]

This seems unrelated to notifications. Why are you changing it?

[gogerald1, 2016/03/09 23:59:12]

The previous place of this check assumes ReadRemoteCharacteristic is a
synchronous operation down to OS, however this is not the case on Windows. Made
this change after discussing with Vincent.

   EXPECT_EQ(empty_vector, last_read_value_);
   EXPECT_EQ(empty_vector, characteristic1_->GetValue());
 }
@@ -295,9 +295,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 +351,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 +360,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 +377,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 +394,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 +409,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 +430,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 +443,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 +465,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 +478,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 +511,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 +524,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 +744,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) {
@@ -756,12 +757,12 @@ TEST_F(BluetoothGattCharacteristicTest, StartNotifySession_NoNotifyOrIndicate) {
 
   // The expected error callback is asynchronous:
   EXPECT_EQ(0, error_callback_count_);
-  base::RunLoop().RunUntilIdle();
+  RunUntilIdle();
   EXPECT_EQ(1, error_callback_count_);

[ortuno, 2016/03/09 16:53:59]

I don't understand how this test is passing if you never check the
characteristics properties. We should check last_gatt_error_code_ to make sure
we are running the callback with the right error. This applies to the other
tests as well.

[gogerald1, 2016/03/09 23:59:12]

No idea how this unit test has been designed, may be a separate CL could be made
to fix the tests you indicate.

 }
-#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.
 TEST_F(BluetoothGattCharacteristicTest, StartNotifySession_NoConfigDescriptor) {
@@ -774,12 +775,12 @@ TEST_F(BluetoothGattCharacteristicTest, StartNotifySession_NoConfigDescriptor) {
 
   // The expected error callback is asynchronous:
   EXPECT_EQ(0, error_callback_count_);
-  base::RunLoop().RunUntilIdle();
+  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.
@@ -792,15 +793,15 @@ TEST_F(BluetoothGattCharacteristicTest,
 
   // The expected error callback is asynchronous:
   EXPECT_EQ(0, error_callback_count_);
-  base::RunLoop().RunUntilIdle();
+  RunUntilIdle();
   EXPECT_EQ(1, error_callback_count_);
 
   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.
 TEST_F(BluetoothGattCharacteristicTest,
        StartNotifySession_WriteDescriptorSynchronousError) {
@@ -811,33 +812,33 @@ TEST_F(BluetoothGattCharacteristicTest,
 
   // The expected error callback is asynchronous:
   EXPECT_EQ(0, error_callback_count_);
-  base::RunLoop().RunUntilIdle();
+  RunUntilIdle();
   EXPECT_EQ(1, error_callback_count_);
 
   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 +847,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 Characteristic Value changes during a Notify Session.
 TEST_F(BluetoothGattCharacteristicTest, GattCharacteristicValueChanged) {
   ASSERT_NO_FATAL_FAILURE(StartNotifyBoilerplate(
@@ -869,7 +870,7 @@ 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)
 // Tests Characteristic Value changing after a Notify Session and objects being

[ortuno, 2016/03/09 16:53:59]

Please add a comment as to why this doesn't apply to windows.

[gogerald1, 2016/03/09 23:59:12]

Done.

@@ -890,7 +891,7 @@ TEST_F(BluetoothGattCharacteristicTest,
 }
 #endif  // defined(OS_ANDROID)
 
-#if defined(OS_ANDROID)
+#if defined(OS_ANDROID) || defined(OS_WIN)
 // Tests multiple StartNotifySession success.
 TEST_F(BluetoothGattCharacteristicTest, StartNotifySession_Multiple) {
   ASSERT_NO_FATAL_FAILURE(
@@ -907,6 +908,9 @@ TEST_F(BluetoothGattCharacteristicTest, StartNotifySession_Multiple) {
   characteristic1_->StartNotifySession(
       GetNotifyCallback(Call::EXPECTED),
       GetGattErrorCallback(Call::NOT_EXPECTED));
+
+  EXPECT_EQ(0, callback_count_);
+  SimulateGattNotifySessionStarted(characteristic1_);
 #if defined(OS_ANDROID)
   // TODO(crbug.com/551634): Decide when implementing IsNotifying if Android
   // should trust the notification request always worked, or if we should always
@@ -915,8 +919,6 @@ TEST_F(BluetoothGattCharacteristicTest, StartNotifySession_Multiple) {
 #else
   EXPECT_EQ(1, gatt_notify_characteristic_attempts_);
 #endif
-  EXPECT_EQ(0, callback_count_);
-  SimulateGattNotifySessionStarted(characteristic1_);
   EXPECT_EQ(2, callback_count_);
   EXPECT_EQ(0, error_callback_count_);
   ASSERT_EQ(2u, notify_sessions_.size());
@@ -929,7 +931,150 @@ 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_WIN)

[ortuno, 2016/03/09 16:53:59]

The other tests have a comment about the test. We should be consistent and add
comments to these new tests.

[gogerald1, 2016/03/09 23:59:12]

Done.

+TEST_F(BluetoothGattCharacteristicTest, StopNotifySession_Twice) {
+  ASSERT_NO_FATAL_FAILURE(
+      FakeCharacteristicBoilerplate(/* properties: NOTIFY */ 0x10));
+  SimulateGattDescriptor(
+      characteristic1_,
+      /* Client Characteristic Configuration descriptor's standard UUID: */
+      "00002902-0000-1000-8000-00805F9B34FB");
+  ASSERT_EQ(1u, characteristic1_->GetDescriptors().size());
+
+  characteristic1_->StartNotifySession(
+      GetNotifyCallback(Call::EXPECTED),
+      GetGattErrorCallback(Call::NOT_EXPECTED));
+  characteristic1_->StartNotifySession(
+      GetNotifyCallback(Call::EXPECTED),
+      GetGattErrorCallback(Call::NOT_EXPECTED));
+
+  EXPECT_EQ(0, callback_count_);
+  SimulateGattNotifySessionStarted(characteristic1_);
+  EXPECT_EQ(2, callback_count_);
+  EXPECT_EQ(0, error_callback_count_);
+  ASSERT_EQ(2u, notify_sessions_.size());
+  ASSERT_TRUE(notify_sessions_[0]);
+  ASSERT_TRUE(notify_sessions_[1]);
+
+  ResetEventCounts();
+  notify_sessions_[0]->Stop(GetCallback(Call::EXPECTED));
+  RunUntilIdle();
+  EXPECT_EQ(1, callback_count_);

[ortuno, 2016/03/09 16:53:59]

I think you need a EXPECT_EQ(0, stop_notifications_attempt_) or similar.

[gogerald1, 2016/03/09 23:59:12]

I don't see how much benefit with it on Windows. I'd like to leave it for the
future work since I don't see symmetric start_notifications_attempt_ or similar.

+  EXPECT_FALSE(notify_sessions_[0]->IsActive());
+  EXPECT_TRUE(notify_sessions_[1]->IsActive());
+  EXPECT_TRUE(characteristic1_->IsNotifying());
+
+  // Stop notify session twice.
+  notify_sessions_[0]->Stop(GetCallback(Call::EXPECTED));
+  RunUntilIdle();
+  EXPECT_EQ(2, callback_count_);

[ortuno, 2016/03/09 16:53:59]

Same here.

[gogerald1, 2016/03/09 23:59:12]

ditto

+  EXPECT_FALSE(notify_sessions_[0]->IsActive());
+  EXPECT_TRUE(notify_sessions_[1]->IsActive());
+  EXPECT_TRUE(characteristic1_->IsNotifying());
+
+  // Stop last active notify session.
+  notify_sessions_[1]->Stop(GetCallback(Call::EXPECTED));
+  RunUntilIdle();
+  EXPECT_EQ(3, callback_count_);

[ortuno, 2016/03/09 16:54:00]

And here you would have EXPECT_EQ(1, stop_notifications_attempts_) or similar.

[gogerald1, 2016/03/09 23:59:12]

ditto

+  EXPECT_FALSE(notify_sessions_[1]->IsActive());
+  EXPECT_FALSE(characteristic1_->IsNotifying());
+
+  // Stop last active notify session twice.
+  notify_sessions_[1]->Stop(GetCallback(Call::EXPECTED));
+  RunUntilIdle();
+  EXPECT_EQ(4, callback_count_);

[ortuno, 2016/03/09 16:53:59]

Check stop_notifications_attempts here as well.

[gogerald1, 2016/03/09 23:59:12]

ditto

+  EXPECT_FALSE(notify_sessions_[1]->IsActive());
+  EXPECT_FALSE(characteristic1_->IsNotifying());
+}
+#endif  // defined(OS_WIN)
+
+#if defined(OS_WIN)
+TEST_F(BluetoothGattCharacteristicTest,
+       StopNotifySession_CharacteristicRemoved) {
+  ASSERT_NO_FATAL_FAILURE(
+      FakeCharacteristicBoilerplate(/* properties: NOTIFY */ 0x10));
+  SimulateGattDescriptor(
+      characteristic1_,
+      /* Client Characteristic Configuration descriptor's standard UUID: */
+      "00002902-0000-1000-8000-00805F9B34FB");
+  ASSERT_EQ(1u, characteristic1_->GetDescriptors().size());
+
+  characteristic1_->StartNotifySession(
+      GetNotifyCallback(Call::EXPECTED),
+      GetGattErrorCallback(Call::NOT_EXPECTED));
+  characteristic1_->StartNotifySession(
+      GetNotifyCallback(Call::EXPECTED),
+      GetGattErrorCallback(Call::NOT_EXPECTED));
+
+  EXPECT_EQ(0, callback_count_);
+  SimulateGattNotifySessionStarted(characteristic1_);
+  EXPECT_EQ(2, callback_count_);
+  EXPECT_EQ(0, error_callback_count_);
+  ASSERT_EQ(2u, notify_sessions_.size());
+  ASSERT_TRUE(notify_sessions_[0]);
+  ASSERT_TRUE(notify_sessions_[1]);
+
+  SimulateGattCharacteristicRemoved(service_, characteristic1_);
+  EXPECT_FALSE(notify_sessions_[0]->IsActive());
+  EXPECT_FALSE(notify_sessions_[1]->IsActive());
+  ResetEventCounts();
+  notify_sessions_[0]->Stop(GetCallback(Call::EXPECTED));
+  notify_sessions_[1]->Stop(GetCallback(Call::EXPECTED));
+  RunUntilIdle();
+  EXPECT_EQ(2, callback_count_);
+}
+#endif  // defined(OS_WIN)
+
+#if defined(OS_WIN)
+TEST_F(BluetoothGattCharacteristicTest,
+       StopNotifySession_PendingStartNotifySessionCallback) {
+  ASSERT_NO_FATAL_FAILURE(
+      FakeCharacteristicBoilerplate(/* properties: NOTIFY */ 0x10));
+  SimulateGattDescriptor(
+      characteristic1_,
+      /* Client Characteristic Configuration descriptor's standard UUID: */
+      "00002902-0000-1000-8000-00805F9B34FB");
+  ASSERT_EQ(1u, characteristic1_->GetDescriptors().size());
+
+  characteristic1_->StartNotifySession(
+      GetNotifyCallback(Call::EXPECTED),
+      GetGattErrorCallback(Call::NOT_EXPECTED));
+  characteristic1_->StartNotifySession(
+      GetNotifyCallback(Call::EXPECTED),
+      GetGattErrorCallback(Call::NOT_EXPECTED));
+
+  EXPECT_EQ(0, callback_count_);
+  SimulateGattNotifySessionStarted(characteristic1_);
+  EXPECT_EQ(2, callback_count_);
+  EXPECT_EQ(0, error_callback_count_);
+  ASSERT_EQ(2u, notify_sessions_.size());
+  ASSERT_TRUE(notify_sessions_[0]);
+  ASSERT_TRUE(notify_sessions_[1]);
+
+  // Create a start notify session request with pending callback.
+  characteristic1_->StartNotifySession(
+      GetNotifyCallback(Call::EXPECTED),
+      GetGattErrorCallback(Call::NOT_EXPECTED));
+  EXPECT_EQ(2, callback_count_);
+  EXPECT_EQ(0, error_callback_count_);
+  ASSERT_EQ(2u, notify_sessions_.size());
+  ASSERT_TRUE(notify_sessions_[0]);
+  ASSERT_TRUE(notify_sessions_[1]);
+
+  ResetEventCounts();
+  notify_sessions_[0]->Stop(GetCallback(Call::EXPECTED));
+  notify_sessions_[1]->Stop(GetCallback(Call::EXPECTED));
+  SimulateGattNotifySessionStarted(characteristic1_);
+  EXPECT_EQ(3, callback_count_);
+  ASSERT_EQ(3u, notify_sessions_.size());
+  EXPECT_FALSE(notify_sessions_[0]->IsActive());
+  EXPECT_FALSE(notify_sessions_[1]->IsActive());
+  EXPECT_TRUE(notify_sessions_[2]->IsActive());
+  EXPECT_TRUE(characteristic1_->IsNotifying());
+}
+#endif  // defined(OS_WIN)

[ortuno, 2016/03/09 16:54:00]

Nice tests. Another test I think we should do:

1. StartNotifySession whose callback stops notifications.
2. StartNotifySession.
3. Expect device to be notifying and the last session to still be active.

Another test to cover a previous bug:

1. StartNotifySession whose callback starts another notify session.
2. Expect only one start_notifications_attempt_ and both sessions to be active.

[gogerald1, 2016/03/09 23:59:12]

Do not understand the first suggested test. For the second one, why "Expect only
one start_notifications_attempt_"?. In addition, above unit tests has already
made sure StartNotifySession is an asynchronous call. So I think above three
tests is good enough for Windows for now. We could make a perfect cross platform
unit test cover in the future given that I am not dedicated working on test
fixture. Or I can set BluetoothGattNotifySessionWin::Stop to be "NOTIMPLEMENTED"
as other platforms to give more time to design tests.

[ortuno, 2016/03/14 01:37:37]

You no longer need this test since you are not implementing stop notify session
yet.
Sorry, I meant "gatt_notify_characteristic_attempts_".
gatt_notify_characteristic_attempts_ should track the number of calls to
BluetoothGattRegisterEvent. You had a bug earlier in which the following code
would result in BluetoothGattRegisterEvent being called twice:

characteristic1_.StartNotifySession([](scoped_ptr<BluetoothGattNotifySession>
session) {
  notify_sessions.push_back(std::move(session));
  characteristic1_->StartNotifySession(
      GetNotifyCallback(Call::EXPECTED),
      GetGattErrorCallback(Call::NOT_EXPECTED));
});
This has little to do with the test fixture. You are implementing a feature and
new features require good test coverage. The tests I proposed are increasing the
test coverage for your implementation.

[gogerald1, 2016/03/15 00:48:19]

Done. No longer need these tests in this CL

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