Implement BluetoothGattNotifySession::Stop on Android, 2nd attempt

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 ec0310b5f65f3fb7fcb3b19e43c7419929d09ece..339329b33dba2733d292c5c256a75d462bde9038 100644
--- a/device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc
+++ b/device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc
@@ -1169,6 +1169,40 @@ TEST_F(BluetoothRemoteGattCharacteristicTest, StartNotifySession_AfterDeleted) {
 }
 #endif  // defined(OS_ANDROID)
 
+#if defined(OS_ANDROID)
+// Tests StartNotifySession completing before chrome objects are deleted.
+TEST_F(BluetoothRemoteGattCharacteristicTest,
+       StartNotifySession_BeforeDeleted) {
+  ASSERT_NO_FATAL_FAILURE(
+      FakeCharacteristicBoilerplate(/* properties: NOTIFY */ 0x10));
+  SimulateGattDescriptor(
+      characteristic1_,
+      BluetoothRemoteGattDescriptor::ClientCharacteristicConfigurationUuid()
+          .canonical_value());
+  ASSERT_EQ(1u, characteristic1_->GetDescriptors().size());
+
+  characteristic1_->StartNotifySession(
+      GetNotifyCallback(Call::EXPECTED),
+      GetGattErrorCallback(Call::NOT_EXPECTED));
+  EXPECT_EQ(1, gatt_notify_characteristic_attempts_);
+  EXPECT_EQ(0, callback_count_);
+
+  RememberCharacteristicForSubsequentAction(characteristic1_);

[ortuno, 2016/08/02 01:56:33]

nit: Move this after SimulateGattNotifySessionStarted.

[tommyt, 2016/08/05 12:22:29]

Done.

+  RememberCCCDescriptorForSubsequentAction(characteristic1_);
+
+  SimulateGattNotifySessionStarted(/* use remembered characteristic */ nullptr);

[ortuno, 2016/08/02 01:56:33]

nit: the characteristic is still alive so you don't need to pass in a nullptr.

[tommyt, 2016/08/05 12:22:28]

Done.

+  ASSERT_EQ(1u, notify_sessions_.size());
+
+  DeleteDevice(device_);  // TODO(576906) delete only the characteristic.
+

[ortuno, 2016/08/02 01:56:33]

We should also test the state of the notify session i.e. GetCharacteristicId(),
GetCharacteristic(), IsActive(), etc. Same after you stop the session.

[tommyt, 2016/08/05 12:22:29]

Done.

+  notify_sessions_[0]->Stop(
+      GetStopNotifyCallback(Call::EXPECTED, notify_sessions_[0]));
+  SimulateGattNotifySessionStopped(/* use remembered characteristic */ nullptr);
+  EXPECT_TRUE("Did not crash!");
+  ASSERT_EQ(0u, notify_sessions_.size());
+}
+#endif  // defined(OS_ANDROID)
+
 #if defined(OS_MACOSX) || defined(OS_WIN)
 // Tests StartNotifySession reentrant in start notify session success callback
 // and the reentrant start notify session success.
@@ -1287,6 +1321,234 @@ TEST_F(BluetoothRemoteGattCharacteristicTest,
 }
 #endif  // defined(OS_WIN)
 
+#if defined(OS_ANDROID)
+// Tests StopNotifySession success on a characteristic enabling Notify.

[ortuno, 2016/08/02 01:56:33]

nit: on a characteristic that enabled Notify.

[tommyt, 2016/08/05 12:22:29]

Done.

+TEST_F(BluetoothRemoteGattCharacteristicTest, StopNotifySession) {

[ortuno, 2016/08/02 01:56:32]

Also add a test to make sure notifications are stopped when the notify session
is destroyed.

[tommyt, 2016/08/05 12:22:29]

Done.

+  ASSERT_NO_FATAL_FAILURE(StartNotifyBoilerplate(
+      /* properties: NOTIFY */ 0x10,
+      /* expected_config_descriptor_value: NOTIFY */ 1));
+  notify_sessions_[0]->Stop(
+      GetStopNotifyCallback(Call::EXPECTED, notify_sessions_[0]));

[ortuno, 2016/08/02 01:56:32]

There are two things you need to make sure right after you call Stop on a notify
session:

1. The right values were written to the descriptor, and
2. The stop session is inactive.

For 1:

You can copy the expects from line 130.

For 2:
EXPECT_FALSE(notify_session_[0]->IsActive());
EXPECT_EQ(nullptr, notify_session_[0]->GetCharacteristic());
...

[tommyt, 2016/08/05 12:22:30]

Done.

+  SimulateGattNotifySessionStopped(characteristic1_);
+  base::RunLoop().RunUntilIdle();
+  ASSERT_EQ(0u, notify_sessions_.size());

[ortuno, 2016/08/02 01:56:33]

Also add EXPECTS for IsNotifying()

[tommyt, 2016/08/05 12:22:29]

Done.

+}
+#endif  // defined(OS_ANDROID)
+
+#if defined(OS_ANDROID)
+// Tests StopNotifySession success on a characteristic enabling Indicate.

[ortuno, 2016/08/02 01:56:32]

nit: on a characteristic that enabled Notify.

[tommyt, 2016/08/05 12:22:28]

Done.

+TEST_F(BluetoothRemoteGattCharacteristicTest, StopNotifySession_OnIndicate) {
+  ASSERT_NO_FATAL_FAILURE(StartNotifyBoilerplate(
+      /* properties: INDICATE */ 0x20,
+      /* expected_config_descriptor_value: INDICATE */ 2));
+  notify_sessions_[0]->Stop(
+      GetStopNotifyCallback(Call::EXPECTED, notify_sessions_[0]));
+  SimulateGattNotifySessionStopped(characteristic1_);

[ortuno, 2016/08/02 01:56:33]

Same here. Add the same EXPECTS as above.

[tommyt, 2016/08/05 12:22:29]

Done.

+  base::RunLoop().RunUntilIdle();
+  ASSERT_EQ(0u, notify_sessions_.size());
+}
+#endif  // defined(OS_ANDROID)
+
+#if defined(OS_ANDROID)
+// Tests StopNotifySession success on a characteristic enabling Notify &

[ortuno, 2016/08/02 01:56:33]

nit: on a characteristic that enabled Notify & Indicate.

[tommyt, 2016/08/05 12:22:29]

Done.

+// Indicate.
+TEST_F(BluetoothRemoteGattCharacteristicTest,
+       StopNotifySession_OnNotifyAndIndicate) {
+  ASSERT_NO_FATAL_FAILURE(StartNotifyBoilerplate(
+      /* properties: INDICATE */ 0x30,

[ortuno, 2016/08/02 01:56:33]

nit: properties: NOTIFY & INDICATE

[tommyt, 2016/08/05 12:22:29]

Done.

+      /* expected_config_descriptor_value: INDICATE */ 1));
+  notify_sessions_[0]->Stop(
+      GetStopNotifyCallback(Call::EXPECTED, notify_sessions_[0]));
+  SimulateGattNotifySessionStopped(characteristic1_);

[ortuno, 2016/08/02 01:56:32]

Same here. Please add same EXPECTS as above.

[tommyt, 2016/08/05 12:22:30]

Done.

+  base::RunLoop().RunUntilIdle();
+  ASSERT_EQ(0u, notify_sessions_.size());
+}
+#endif  // defined(OS_ANDROID)
+
+#if defined(OS_ANDROID)
+// Tests StopNotifySession error
+TEST_F(BluetoothRemoteGattCharacteristicTest, StopNotifySession_Error) {
+  ASSERT_NO_FATAL_FAILURE(StartNotifyBoilerplate(
+      /* properties: NOTIFY */ 0x10,
+      /* expected_config_descriptor_value: NOTIFY */ 1));
+  notify_sessions_[0]->Stop(
+      GetStopNotifyCallback(Call::EXPECTED, notify_sessions_[0]));

[ortuno, 2016/08/02 01:56:32]

Test the state of the notify session before and after the error as well.

[tommyt, 2016/08/05 12:22:29]

Done.

+  SimulateGattNotifySessionStopError(
+      characteristic1_, BluetoothRemoteGattService::GATT_ERROR_UNKNOWN);
+  base::RunLoop().RunUntilIdle();
+  ASSERT_EQ(0u, notify_sessions_.size());

[ortuno, 2016/08/02 01:56:33]

Also test IsNotifying()

[tommyt, 2016/08/05 12:22:29]

Done.

+}
+#endif  // defined(OS_ANDROID)
+
+#if defined(OS_ANDROID)
+// Tests multiple StopNotifySession calls for a single session.
+TEST_F(BluetoothRemoteGattCharacteristicTest, StopNotifySession_Multiple1) {
+  ASSERT_NO_FATAL_FAILURE(
+      FakeCharacteristicBoilerplate(/* properties: NOTIFY */ 0x10));
+  SimulateGattDescriptor(
+      characteristic1_,
+      BluetoothRemoteGattDescriptor::ClientCharacteristicConfigurationUuid()
+          .canonical_value());
+  ASSERT_EQ(1u, characteristic1_->GetDescriptors().size());
+
+  // Start notify session
+  characteristic1_->StartNotifySession(
+      GetNotifyCallback(Call::EXPECTED),
+      GetGattErrorCallback(Call::NOT_EXPECTED));
+  EXPECT_EQ(0, callback_count_);
+  SimulateGattNotifySessionStarted(characteristic1_);
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, gatt_notify_characteristic_attempts_);
+  EXPECT_EQ(1, callback_count_);
+  EXPECT_EQ(0, error_callback_count_);
+  ASSERT_EQ(1u, notify_sessions_.size());
+  ASSERT_TRUE(notify_sessions_[0]);
+  EXPECT_EQ(characteristic1_, notify_sessions_[0]->GetCharacteristic());
+  EXPECT_TRUE(notify_sessions_[0]->IsActive());
+
+  // Stop the notify session twice
+  notify_sessions_[0]->Stop(
+      GetStopNotifyCallback(Call::EXPECTED, notify_sessions_[0]));

[ortuno, 2016/08/02 01:56:33]

We need a way to test that callbacks are called in the right order. What do you
think about the following in bluetooth_test:

base::Closure NotifySessionCheckForPrecedingCalls( // Or similar name.
  base::Closure stop_callback,
  int num_of_preceding_calls) {
  
  EXPECT_EQ(num_of_preceding_calls, callback_count_);

  stop_callback.Run();
}

This test would look like this:

ResetEventCounts();
notify_session_[0]->Stop(
    base::Bind(BluetoothTest::NotifySessionCheckForPrecedingCalls,
               base::Unretained(this),
               GetStopNotifyCallback(Call::EXPECTED, notify_sessions_[0]),
               0 /* num_of_preceding_calls */));
    
notify_session_[0]->Stop(
    base::Bind(BluetoothTest::NotifySessionCheckForPrecedingCalls,
               base::Unretained(this),
               GetStopNotifyCallback(Call::EXPECTED, notify_sessions_[0]),
               1 /* num_of_preceding_calls */));

[tommyt, 2016/08/05 12:22:29]

Done.

+  notify_sessions_[0]->Stop(
+      GetStopNotifyCallback(Call::EXPECTED, notify_sessions_[0]));
+  SimulateGattNotifySessionStopped(characteristic1_);
+  base::RunLoop().RunUntilIdle();
+  ASSERT_EQ(0u, notify_sessions_.size());

[ortuno, 2016/08/02 01:56:34]

optional nit: maybe test IsNotifying(). Not as important since other tests test
it.

[tommyt, 2016/08/05 12:22:29]

Done.

+}
+#endif  // defined(OS_ANDROID)
+
+#if defined(OS_ANDROID)
+// Tests multiple StartNotifySession calls and multiple StopNotifySession calls.
+TEST_F(BluetoothRemoteGattCharacteristicTest, StopNotifySession_Multiple2) {
+  ASSERT_NO_FATAL_FAILURE(
+      FakeCharacteristicBoilerplate(/* properties: NOTIFY */ 0x10));
+  SimulateGattDescriptor(

[ortuno, 2016/08/02 01:56:32]

Surround this with # !defined(OS_MACOSX) with a todo to remove it once
http://crbug.com/624017 is fixed.

[tommyt, 2016/08/05 12:22:29]

Done.

+      characteristic1_,
+      BluetoothRemoteGattDescriptor::ClientCharacteristicConfigurationUuid()
+          .canonical_value());
+  ASSERT_EQ(1u, characteristic1_->GetDescriptors().size());
+
+  // Start notify sessions
+  characteristic1_->StartNotifySession(

[ortuno, 2016/08/02 01:56:32]

Same here. I think we need a way to test that callbacks are getting called in
the right order.

[tommyt, 2016/08/05 12:22:29]

Done.

[ortuno, 2016/08/09 21:57:55]

I think you forgot to change this test :)

[tommyt, 2016/08/10 12:07:17]

Yes, or I did change it but I only added these tests to the Stop calls below. I
have fixed this properly in the latest patchset.

+      GetNotifyCallback(Call::EXPECTED),
+      GetGattErrorCallback(Call::NOT_EXPECTED));
+  characteristic1_->StartNotifySession(
+      GetNotifyCallback(Call::EXPECTED),
+      GetGattErrorCallback(Call::NOT_EXPECTED));
+  EXPECT_EQ(0, callback_count_);
+  SimulateGattNotifySessionStarted(characteristic1_);
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, gatt_notify_characteristic_attempts_);
+  EXPECT_EQ(2, callback_count_);
+  EXPECT_EQ(0, error_callback_count_);
+  ASSERT_EQ(2u, notify_sessions_.size());

[ortuno, 2016/08/02 01:56:33]

Also test is IsNotifying()

[tommyt, 2016/08/05 12:22:29]

Done.

+  ASSERT_TRUE(notify_sessions_[0]);
+  ASSERT_TRUE(notify_sessions_[1]);
+  EXPECT_EQ(characteristic1_, notify_sessions_[0]->GetCharacteristic());
+  EXPECT_EQ(characteristic1_, notify_sessions_[1]->GetCharacteristic());
+  EXPECT_TRUE(notify_sessions_[0]->IsActive());
+  EXPECT_TRUE(notify_sessions_[1]->IsActive());
+
+  // Stop the notify sessions
+  notify_sessions_[1]->Stop(
+      GetStopNotifyCallback(Call::EXPECTED, notify_sessions_[1]));
+  base::RunLoop().RunUntilIdle();
+  ASSERT_EQ(1u, notify_sessions_.size());

[ortuno, 2016/08/02 01:56:33]

Also test IsNotifying() and that the other session is still active.

[tommyt, 2016/08/05 12:22:29]

Done.

+
+  notify_sessions_[0]->Stop(
+      GetStopNotifyCallback(Call::EXPECTED, notify_sessions_[0]));
+  SimulateGattNotifySessionStopped(characteristic1_);
+  base::RunLoop().RunUntilIdle();
+  ASSERT_EQ(0u, notify_sessions_.size());
+}
+#endif  // defined(OS_ANDROID)
+
+#if defined(OS_ANDROID)
+// Tests starting a new notify session before the previous stop request resolves

[ortuno, 2016/08/02 01:56:32]

nit: Period at the end of sentence.

[tommyt, 2016/08/05 12:22:28]

Done.

+TEST_F(BluetoothRemoteGattCharacteristicTest, StopNotifySession_StopStart) {
+  ASSERT_NO_FATAL_FAILURE(
+      FakeCharacteristicBoilerplate(/* properties: NOTIFY */ 0x10));
+  SimulateGattDescriptor(
+      characteristic1_,
+      BluetoothRemoteGattDescriptor::ClientCharacteristicConfigurationUuid()
+          .canonical_value());
+  ASSERT_EQ(1u, characteristic1_->GetDescriptors().size());
+
+  // Start notify session

[ortuno, 2016/08/02 01:56:32]

Same here. Please add some code to make sure the callbacks are called in the
right order.

[tommyt, 2016/08/05 12:22:29]

Done.

+  characteristic1_->StartNotifySession(
+      GetNotifyCallback(Call::EXPECTED),
+      GetGattErrorCallback(Call::NOT_EXPECTED));
+  SimulateGattNotifySessionStarted(characteristic1_);
+  base::RunLoop().RunUntilIdle();
+  ASSERT_EQ(1u, notify_sessions_.size());
+  ASSERT_TRUE(notify_sessions_[0]);
+  EXPECT_TRUE(notify_sessions_[0]->IsActive());
+
+  // Stop the notify session
+  notify_sessions_[0]->Stop(
+      GetStopNotifyCallback(Call::EXPECTED, notify_sessions_[0]));
+
+  // Start another notify session
+  characteristic1_->StartNotifySession(
+      GetNotifyCallback(Call::EXPECTED),
+      GetGattErrorCallback(Call::NOT_EXPECTED));
+
+  SimulateGattNotifySessionStopped(characteristic1_);
+  base::RunLoop().RunUntilIdle();
+  ASSERT_EQ(0u, notify_sessions_.size());
+
+  SimulateGattNotifySessionStarted(characteristic1_);
+  base::RunLoop().RunUntilIdle();
+  ASSERT_EQ(1u, notify_sessions_.size());
+  ASSERT_TRUE(notify_sessions_[0]);
+  EXPECT_TRUE(notify_sessions_[0]->IsActive());
+}

[ortuno, 2016/08/02 01:56:32]

Also test IsNotifying()

[tommyt, 2016/08/05 12:22:30]

Done.

+#endif  // defined(OS_ANDROID)
+
+#if defined(OS_ANDROID)
+// Tests starting a new notify session before the previous stop request resolves

[ortuno, 2016/08/02 01:56:33]

nit: Period at the end of sentence.

before previous stop request*s* resolve.

[tommyt, 2016/08/05 12:22:29]

Done.

+TEST_F(BluetoothRemoteGattCharacteristicTest, StopNotifySession_StopStopStart) {

[ortuno, 2016/08/02 01:56:32]

Another similar test but for StartNotifySession to make sure that a success
start doesn't cause other starts to jump ahead.

TEST_F(..., StartNotifySession_StartStopStart) {
  // Make sure the order of calls is correct.
  // Setup
  
  characteristic1_->StartNotifySession(...)

  // Start succeeds.

  characteristic1_->StartNotifySession(
    base::Bind(BluetoothTest::NotifySessionCheckForPrecedingCalls,
               base::Unretained(this),
               1 /* num_of_preceding_calls */),
    GetStartNotifyErrorCallback(...));

  notify_sessions_[0]->Stop(
      base::Bind(BluetoothTest::NotifySessionCheckForPrecedingCalls,
                 base::Unretained(this),
                 2 /* num_of_preceding_calls */));
          
  characteristic1_->StartNotifySession(
      base::Bind(BluetoothTest::NotifySessionCheckForPrecedingCalls,
                 base::Unretained(this),
                 3 /* num_of_preceding_calls */),
      GetStartNotifyErrorCallback(...));
   
  // Simulate necessary events and run loop.

  EXPECT_FALSE(notify_sessions_[0]->IsActive());
  EXPECT_TRUE(notify_sessions_[1]->IsActive());
  EXPECT_TRUE(notify_sessions_[2]->IsActive());

  EXPECT_TRUE(characteristic1_->IsNotifying());
  // EXPECT value of CCC descriptor is 1.
  // EXPECT only one write to the CCC descriptor.

}

[tommyt, 2016/08/05 12:22:29]

This test case would have to be added later, after we do the change where
StartNotifySession returns a session object immediately. At the moment, it is
not possible to queue up a start, stop, start, and then simulate all the events
(since the stop depends on the initial start having finished).

[ortuno, 2016/08/05 14:47:00]

There are three StartNotifications calls. The stop is called on the session that
the first StartNotifications() call produced.

[tommyt, 2016/08/08 13:55:08]

Right, this approach is possible. The latest patchset adds this unittest.

+  ASSERT_NO_FATAL_FAILURE(
+      FakeCharacteristicBoilerplate(/* properties: NOTIFY */ 0x10));
+  SimulateGattDescriptor(
+      characteristic1_,
+      BluetoothRemoteGattDescriptor::ClientCharacteristicConfigurationUuid()
+          .canonical_value());
+  ASSERT_EQ(1u, characteristic1_->GetDescriptors().size());
+
+  // Start notify session
+  characteristic1_->StartNotifySession(
+      GetNotifyCallback(Call::EXPECTED),
+      GetGattErrorCallback(Call::NOT_EXPECTED));
+  SimulateGattNotifySessionStarted(characteristic1_);
+  base::RunLoop().RunUntilIdle();
+  ASSERT_EQ(1u, notify_sessions_.size());
+  ASSERT_TRUE(notify_sessions_[0]);
+  EXPECT_TRUE(notify_sessions_[0]->IsActive());
+
+  // Stop the notify session twice
+  notify_sessions_[0]->Stop(
+      GetStopNotifyCallback(Call::EXPECTED, notify_sessions_[0]));
+  notify_sessions_[0]->Stop(
+      GetStopNotifyCallback(Call::EXPECTED, notify_sessions_[0]));
+
+  // Start another notify session
+  characteristic1_->StartNotifySession(
+      GetNotifyCallback(Call::EXPECTED),
+      GetGattErrorCallback(Call::NOT_EXPECTED));
+
+  SimulateGattNotifySessionStopped(characteristic1_);
+  base::RunLoop().RunUntilIdle();
+  ASSERT_EQ(0u, notify_sessions_.size());
+
+  SimulateGattNotifySessionStarted(characteristic1_);
+  base::RunLoop().RunUntilIdle();
+  ASSERT_EQ(1u, notify_sessions_.size());
+  ASSERT_TRUE(notify_sessions_[0]);
+  EXPECT_TRUE(notify_sessions_[0]->IsActive());
+}

[ortuno, 2016/08/02 01:56:33]

Same here we need to make sure that the callbacks are run in the right order and
the right values were written to the descriptor.

[tommyt, 2016/08/05 12:22:29]

Done.

+#endif  // defined(OS_ANDROID)
+
 #if defined(OS_ANDROID) || defined(OS_MACOSX) || defined(OS_WIN)

[ortuno, 2016/08/02 01:56:33]

Also tests for reentrancy i.e. you need to make sure calling StartNotifySession
and Stop() inside a callback are correctly handled. We already have a bunch for
StartNotifySession, but we are missing tests for Stop calls inside
StartNotifySession's callback and StartNotifySession called inside Stop's
callback. The good thing is that now you can use Bind lambdas so the tests
shouldn't be as hard as the already existing tests. I believe there are 3
combinations:

StartSuccess => Stop
Stop => StartSuccess
Stop => StartError

TEST_F(StartNotifySession_Reentrant_Success_Stop) {
  // Setup

  characteristic1_->StartNotifySession(
      base::Bind([](NotifySessionCallback callback,
                    std::unique_ptr<BluetoothGattNotifySession> session) {
                      BluetoothGattNotifySession* s = session.get(); // Or pass
in a reference to the sessions array.
                      callback.Run(std::move(session));
                      s->GetCharacteristic()->Stop(...);
                 }, GetStartNotifySessionCallback(...)),
      GetStartNotifySessionErrorCallback(...));

  // Run loop.
  EXPECT_FALSE(characteristic1_->IsNotifying());
  EXPECT_FALSE(notify_sessions_->IsActvice());
  // EXPECT two calls to write to descriptors.
  // EXPECT final value of descriptor to be 0.
}

If lambdas don't work you might have to add a function for each case in
bluetooth_test.

[tommyt, 2016/08/05 12:22:29]

Done.

 // Tests Characteristic Value changes during a Notify Session.
 TEST_F(BluetoothRemoteGattCharacteristicTest, GattCharacteristicValueChanged) {