bluetooth: bluez: Implement...

bluetooth: bluez: Implement BluetoothRemoteGattCharacteristicBluez::SubscribeToNotifications and UnsubscribeFromNotifications.

BUG=636275

Review-Url: https://codereview.chromium.org/2613473002
Cr-Commit-Position: refs/heads/master@{#441456}

device/bluetooth/bluetooth_remote_gatt_characteristic.cc

@@ -147,17 +147,13 @@ void BluetoothRemoteGattCharacteristic::ExecuteStartNotifySession(
 //
 // TODO(http://crbug.com/633191): Remove OS_MACOSX from this check.
 // TODO(http://crbug.com/636270): Remove OS_WIN from this check.
-// TODO(http://crbug.com/636275): Remove OS_CHROMEOS and OS_LINUX from this
-// check.
-#if defined(OS_CHROMEOS) || defined(OS_LINUX) || defined(OS_MACOSX) || \
-    defined(OS_WIN)
+#if defined(OS_MACOSX) || defined(OS_WIN)
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE,
       base::Bind(&BluetoothRemoteGattCharacteristic::OnStartNotifySessionError,
                  GetWeakPtr(), error_callback,
                  BluetoothRemoteGattService::GATT_ERROR_NOT_SUPPORTED));
-#else   // !(defined(OS_CHROMEOS) || defined(OS_LINUX) || defined(OS_MACOSX) ||
-  // defined(OS_WIN))
+#else   // !(defined(OS_MACOSX) || defined(OS_WIN))
   // Find the Client Characteristic Configuration descriptor.
   std::vector<BluetoothRemoteGattDescriptor*> ccc_descriptor =
       GetDescriptorsByUUID(BluetoothRemoteGattDescriptor::
@@ -187,8 +183,7 @@ void BluetoothRemoteGattCharacteristic::ExecuteStartNotifySession(
           GetWeakPtr(), callback),
       base::Bind(&BluetoothRemoteGattCharacteristic::OnStartNotifySessionError,
                  GetWeakPtr(), error_callback));
-#endif  // defined(OS_CHROMEOS) || defined(OS_LINUX) || defined(OS_MACOSX) ||
-        // defined(OS_WIN)
+#endif  // defined(OS_MACOSX) || defined(OS_WIN)
 }
 
 void BluetoothRemoteGattCharacteristic::CancelStartNotifySession(
@@ -285,17 +280,13 @@ void BluetoothRemoteGattCharacteristic::ExecuteStopNotifySession(
 //
 // TODO(http://crbug.com/633191): Remove OS_MACOSX from this check.
 // TODO(http://crbug.com/636270): Remove OS_WIN from this check.
-// TODO(http://crbug.com/636275): Remove OS_CHROMEOS and OS_LINUX from this
-// check.
-#if defined(OS_CHROMEOS) || defined(OS_LINUX) || defined(OS_MACOSX) || \
-    defined(OS_WIN)
+#if defined(OS_MACOSX) || defined(OS_WIN)
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE,
       base::Bind(&BluetoothRemoteGattCharacteristic::OnStopNotifySessionError,
                  GetWeakPtr(), session, callback,
                  BluetoothRemoteGattService::GATT_ERROR_NOT_SUPPORTED));
-#else   // !(defined(OS_CHROMEOS) || defined(OS_LINUX) || defined(OS_MACOSX) ||
-  // defined(OS_WIN))
+#else   // !(defined(OS_MACOSX) || defined(OS_WIN))
   // Find the Client Characteristic Configuration descriptor.
   std::vector<BluetoothRemoteGattDescriptor*> ccc_descriptor =
       GetDescriptorsByUUID(BluetoothRemoteGattDescriptor::
@@ -318,8 +309,7 @@ void BluetoothRemoteGattCharacteristic::ExecuteStopNotifySession(
                  GetWeakPtr(), session, callback),
       base::Bind(&BluetoothRemoteGattCharacteristic::OnStopNotifySessionError,
                  GetWeakPtr(), session, callback));
-#endif  // defined(OS_CHROMEOS) || defined(OS_LINUX) || defined(OS_MACOSX) ||
-        // defined(OS_WIN)
+#endif  // defined(OS_MACOSX) || defined(OS_WIN)
 }
 
 void BluetoothRemoteGattCharacteristic::CancelStopNotifySession(

device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc

@@ -1416,12 +1416,13 @@ TEST_F(BluetoothGattBlueZTest, NotifySessions) {
   EXPECT_EQ(0, error_callback_count_);
   EXPECT_EQ(1, observer.gatt_characteristic_value_changed_count());
   EXPECT_TRUE(update_sessions_.empty());
-  EXPECT_TRUE(characteristic->IsNotifying());
+  EXPECT_FALSE(characteristic->IsNotifying());
 
   // Run the main loop. The initial call should complete. The queued call should
   // succeed immediately.
   base::RunLoop().Run();
 
+  EXPECT_TRUE(characteristic->IsNotifying());
   EXPECT_EQ(3, success_callback_count_);
   EXPECT_EQ(0, error_callback_count_);
   EXPECT_EQ(1, observer.gatt_characteristic_value_changed_count());
@@ -1437,6 +1438,10 @@ TEST_F(BluetoothGattBlueZTest, NotifySessions) {
   EXPECT_TRUE(session->IsActive());
   session->Stop(base::Bind(&BluetoothGattBlueZTest::SuccessCallback,
                            base::Unretained(this)));
+
+  // Run message loop to stop the notify session.
+  base::RunLoop().Run();
+
   EXPECT_EQ(4, success_callback_count_);
   EXPECT_EQ(0, error_callback_count_);
   EXPECT_FALSE(session->IsActive());
@@ -1454,6 +1459,10 @@ TEST_F(BluetoothGattBlueZTest, NotifySessions) {
   // Clear the last session.
   update_sessions_.clear();
   EXPECT_TRUE(update_sessions_.empty());
+
+  // Run message loop in order to do proper cleanup of sessions.
+  base::RunLoop().RunUntilIdle();
+
   EXPECT_FALSE(characteristic->IsNotifying());
 
   success_callback_count_ = 0;
@@ -1469,7 +1478,7 @@ TEST_F(BluetoothGattBlueZTest, NotifySessions) {
   EXPECT_EQ(0, error_callback_count_);
   EXPECT_EQ(1, observer.gatt_characteristic_value_changed_count());
   EXPECT_TRUE(update_sessions_.empty());
-  EXPECT_TRUE(characteristic->IsNotifying());
+  EXPECT_FALSE(characteristic->IsNotifying());
 
   // Run the message loop. Notifications should begin.
   base::RunLoop().Run();
@@ -1491,6 +1500,10 @@ TEST_F(BluetoothGattBlueZTest, NotifySessions) {
                  base::Unretained(this)),
       base::Bind(&BluetoothGattBlueZTest::ServiceErrorCallback,
                  base::Unretained(this)));
+
+  // Run message loop to stop the notify session.
+  base::RunLoop().Run();
+
   EXPECT_EQ(2, success_callback_count_);
   EXPECT_EQ(0, error_callback_count_);
   EXPECT_EQ(2U, update_sessions_.size());
@@ -1564,7 +1577,7 @@ TEST_F(BluetoothGattBlueZTest, NotifySessionsMadeInactive) {
   EXPECT_EQ(0, success_callback_count_);
   EXPECT_EQ(0, error_callback_count_);
   EXPECT_EQ(1, observer.gatt_characteristic_value_changed_count());
-  EXPECT_TRUE(characteristic->IsNotifying());
+  EXPECT_FALSE(characteristic->IsNotifying());
   EXPECT_TRUE(update_sessions_.empty());
 
   // Run the main loop. The initial call should complete. The queued calls
@@ -1590,6 +1603,10 @@ TEST_F(BluetoothGattBlueZTest, NotifySessionsMadeInactive) {
                  base::Unretained(this)));
   EXPECT_EQ(5, success_callback_count_);
   EXPECT_EQ(0, error_callback_count_);
+
+  // Run message loop to stop the notify session.
+  base::RunLoop().RunUntilIdle();
+
   EXPECT_FALSE(characteristic->IsNotifying());
   EXPECT_EQ(4U, update_sessions_.size());
 
@@ -1607,10 +1624,13 @@ TEST_F(BluetoothGattBlueZTest, NotifySessionsMadeInactive) {
       base::Bind(&BluetoothGattBlueZTest::ServiceErrorCallback,
                  base::Unretained(this)));
 
+  // Run message loop to start the notify session.
+  base::RunLoop().RunUntilIdle();
+
   EXPECT_EQ(0, success_callback_count_);
   EXPECT_EQ(0, error_callback_count_);
   EXPECT_EQ(1, observer.gatt_characteristic_value_changed_count());
-  EXPECT_TRUE(characteristic->IsNotifying());
+  EXPECT_FALSE(characteristic->IsNotifying());
   EXPECT_TRUE(update_sessions_.empty());
 
   base::RunLoop().Run();

device/bluetooth/bluez/bluetooth_remote_gatt_characteristic_bluez.cc

@@ -43,8 +43,7 @@ BluetoothRemoteGattCharacteristicBlueZ::BluetoothRemoteGattCharacteristicBlueZ(
     BluetoothRemoteGattServiceBlueZ* service,
     const dbus::ObjectPath& object_path)
     : BluetoothGattCharacteristicBlueZ(object_path),
-      num_notify_sessions_(0),
-      notify_call_pending_(false),
+      has_notify_session_(false),
       service_(service),
       weak_ptr_factory_(this) {
   VLOG(1) << "Creating remote GATT characteristic with identifier: "
@@ -74,51 +73,6 @@ BluetoothRemoteGattCharacteristicBlueZ::
   for (DescriptorMap::iterator iter = descriptors_.begin();
        iter != descriptors_.end(); ++iter)
     delete iter->second;
-
-  // Report an error for all pending calls to StartNotifySession.
-  while (!pending_start_notify_calls_.empty()) {
-    PendingStartNotifyCall callbacks = pending_start_notify_calls_.front();
-    pending_start_notify_calls_.pop();
-    callbacks.second.Run(device::BluetoothRemoteGattService::GATT_ERROR_FAILED);
-  }
-}
-
-void BluetoothRemoteGattCharacteristicBlueZ::StopNotifySession(
-    device::BluetoothGattNotifySession* session,
-    const base::Closure& callback) {
-  VLOG(1) << __func__;
-
-  if (num_notify_sessions_ > 1) {
-    DCHECK(!notify_call_pending_);
-    --num_notify_sessions_;
-    callback.Run();
-    return;
-  }
-
-  // Notifications may have stopped outside our control. If the characteristic
-  // is no longer notifying, return success.
-  if (!IsNotifying()) {
-    num_notify_sessions_ = 0;
-    callback.Run();
-    return;
-  }
-
-  if (notify_call_pending_ || num_notify_sessions_ == 0) {
-    callback.Run();
-    return;
-  }
-
-  DCHECK(num_notify_sessions_ == 1);
-  notify_call_pending_ = true;
-  bluez::BluezDBusManager::Get()
-      ->GetBluetoothGattCharacteristicClient()
-      ->StopNotify(
-          object_path(),
-          base::Bind(
-              &BluetoothRemoteGattCharacteristicBlueZ::OnStopNotifySuccess,
-              weak_ptr_factory_.GetWeakPtr(), callback),
-          base::Bind(&BluetoothRemoteGattCharacteristicBlueZ::OnStopNotifyError,
-                     weak_ptr_factory_.GetWeakPtr(), callback));
 }
 
 device::BluetoothUUID BluetoothRemoteGattCharacteristicBlueZ::GetUUID() const {
@@ -198,7 +152,10 @@ bool BluetoothRemoteGattCharacteristicBlueZ::IsNotifying() const {
           ->GetProperties(object_path());
   DCHECK(properties);
 
-  return properties->notifying.value();
+  // It is not enough to only check notifying.value(). Bluez also
+  // needs a notify client/session in order to deliver the
+  // notifications.
+  return has_notify_session_ && properties->notifying.value();
 }
 
 std::vector<device::BluetoothRemoteGattDescriptor*>
@@ -220,58 +177,6 @@ BluetoothRemoteGattCharacteristicBlueZ::GetDescriptor(
   return iter->second;
 }
 
-void BluetoothRemoteGattCharacteristicBlueZ::StartNotifySession(
-    const NotifySessionCallback& callback,
-    const ErrorCallback& error_callback) {
-  VLOG(1) << __func__;
-
-  if (num_notify_sessions_ > 0) {
-    // The characteristic might have stopped notifying even though the session
-    // count is nonzero. This means that notifications stopped outside of our
-    // control and we should reset the count. If the characteristic is still
-    // notifying, then return success. Otherwise, reset the count and treat
-    // this call as if the count were 0.
-    if (IsNotifying()) {
-      // Check for overflows, though unlikely.
-      if (num_notify_sessions_ == std::numeric_limits<size_t>::max()) {
-        error_callback.Run(
-            device::BluetoothRemoteGattService::GATT_ERROR_FAILED);
-        return;
-      }
-
-      ++num_notify_sessions_;
-      DCHECK(service_);
-      DCHECK(service_->GetAdapter());
-      DCHECK(service_->GetDevice());
-      std::unique_ptr<device::BluetoothGattNotifySession> session(
-          new device::BluetoothGattNotifySession(
-              weak_ptr_factory_.GetWeakPtr()));
-      callback.Run(std::move(session));
-      return;
-    }
-
-    num_notify_sessions_ = 0;
-  }
-
-  // Queue the callbacks if there is a pending call to bluetoothd.
-  if (notify_call_pending_) {
-    pending_start_notify_calls_.push(std::make_pair(callback, error_callback));
-    return;
-  }
-
-  notify_call_pending_ = true;
-  bluez::BluezDBusManager::Get()
-      ->GetBluetoothGattCharacteristicClient()
-      ->StartNotify(
-          object_path(),
-          base::Bind(
-              &BluetoothRemoteGattCharacteristicBlueZ::OnStartNotifySuccess,
-              weak_ptr_factory_.GetWeakPtr(), callback),
-          base::Bind(
-              &BluetoothRemoteGattCharacteristicBlueZ::OnStartNotifyError,
-              weak_ptr_factory_.GetWeakPtr(), error_callback));
-}
-
 void BluetoothRemoteGattCharacteristicBlueZ::ReadRemoteCharacteristic(
     const ValueCallback& callback,
     const ErrorCallback& error_callback) {
@@ -305,16 +210,31 @@ void BluetoothRemoteGattCharacteristicBlueZ::SubscribeToNotifications(
     device::BluetoothRemoteGattDescriptor* ccc_descriptor,
     const base::Closure& callback,
     const ErrorCallback& error_callback) {
-  // TODO(http://crbug.com/636275): Implement this method
-  NOTIMPLEMENTED();
+  bluez::BluezDBusManager::Get()
+      ->GetBluetoothGattCharacteristicClient()
+      ->StartNotify(
+          object_path(),
+          base::Bind(
+              &BluetoothRemoteGattCharacteristicBlueZ::OnStartNotifySuccess,
+              weak_ptr_factory_.GetWeakPtr(), callback),
+          base::Bind(
+              &BluetoothRemoteGattCharacteristicBlueZ::OnStartNotifyError,
+              weak_ptr_factory_.GetWeakPtr(), error_callback));
 }
 
 void BluetoothRemoteGattCharacteristicBlueZ::UnsubscribeFromNotifications(
     device::BluetoothRemoteGattDescriptor* ccc_descriptor,
     const base::Closure& callback,
     const ErrorCallback& error_callback) {
-  // TODO(http://crbug.com/636275): Implement this method
-  NOTIMPLEMENTED();
+  bluez::BluezDBusManager::Get()
+      ->GetBluetoothGattCharacteristicClient()
+      ->StopNotify(
+          object_path(),
+          base::Bind(
+              &BluetoothRemoteGattCharacteristicBlueZ::OnStopNotifySuccess,
+              weak_ptr_factory_.GetWeakPtr(), callback),
+          base::Bind(&BluetoothRemoteGattCharacteristicBlueZ::OnStopNotifyError,
+                     weak_ptr_factory_.GetWeakPtr(), callback));
 }
 
 void BluetoothRemoteGattCharacteristicBlueZ::GattDescriptorAdded(
@@ -397,23 +317,11 @@ void BluetoothRemoteGattCharacteristicBlueZ::GattDescriptorPropertyChanged(
 }
 
 void BluetoothRemoteGattCharacteristicBlueZ::OnStartNotifySuccess(
-    const NotifySessionCallback& callback) {
+    const base::Closure& callback) {
   VLOG(1) << "Started notifications from characteristic: "
           << object_path().value();
-  DCHECK(num_notify_sessions_ == 0);
-  DCHECK(notify_call_pending_);
-
-  ++num_notify_sessions_;
-  notify_call_pending_ = false;
-
-  // Invoke the queued callbacks for this operation.
-  DCHECK(service_);
-  DCHECK(service_->GetDevice());
-  std::unique_ptr<device::BluetoothGattNotifySession> session(
-      new device::BluetoothGattNotifySession(weak_ptr_factory_.GetWeakPtr()));
-  callback.Run(std::move(session));
-
-  ProcessStartNotifyQueue();
+  has_notify_session_ = true;
+  callback.Run();
 }
 
 void BluetoothRemoteGattCharacteristicBlueZ::OnStartNotifyError(
@@ -423,27 +331,14 @@ void BluetoothRemoteGattCharacteristicBlueZ::OnStartNotifyError(
   VLOG(1) << "Failed to start notifications from characteristic: "
           << object_path().value() << ": " << error_name << ", "
           << error_message;
-  DCHECK(num_notify_sessions_ == 0);
-  DCHECK(notify_call_pending_);
-
-  notify_call_pending_ = false;
-
   error_callback.Run(
       BluetoothRemoteGattServiceBlueZ::DBusErrorToServiceError(error_name));
-
-  ProcessStartNotifyQueue();
 }
 
 void BluetoothRemoteGattCharacteristicBlueZ::OnStopNotifySuccess(
     const base::Closure& callback) {
-  DCHECK(notify_call_pending_);
-  DCHECK(num_notify_sessions_ == 1);
-
-  notify_call_pending_ = false;
-  --num_notify_sessions_;
+  has_notify_session_ = false;
   callback.Run();
-
-  ProcessStartNotifyQueue();
 }
 
 void BluetoothRemoteGattCharacteristicBlueZ::OnStopNotifyError(
@@ -458,14 +353,6 @@ void BluetoothRemoteGattCharacteristicBlueZ::OnStopNotifyError(
   OnStopNotifySuccess(callback);
 }
 
-void BluetoothRemoteGattCharacteristicBlueZ::ProcessStartNotifyQueue() {
-  while (!pending_start_notify_calls_.empty()) {
-    PendingStartNotifyCall callbacks = pending_start_notify_calls_.front();
-    pending_start_notify_calls_.pop();
-    StartNotifySession(callbacks.first, callbacks.second);
-  }
-}
-
 void BluetoothRemoteGattCharacteristicBlueZ::OnError(
     const ErrorCallback& error_callback,
     const std::string& error_name,

device/bluetooth/bluez/bluetooth_remote_gatt_characteristic_bluez.h

@@ -57,14 +57,6 @@ class BluetoothRemoteGattCharacteristicBlueZ
       const override;
   device::BluetoothRemoteGattDescriptor* GetDescriptor(
       const std::string& identifier) const override;
-  void StartNotifySession(const NotifySessionCallback& callback,
-                          const ErrorCallback& error_callback) override;
-  // Removes one value update session and invokes |callback| on completion. This
-  // decrements the session reference count by 1 and if the number reaches 0,
-  // makes a call to the subsystem to stop notifications from this
-  // characteristic.
-  void StopNotifySession(device::BluetoothGattNotifySession* session,
-                         const base::Closure& callback) override;
   void ReadRemoteCharacteristic(const ValueCallback& callback,
                                 const ErrorCallback& error_callback) override;
   void WriteRemoteCharacteristic(const std::vector<uint8_t>& value,
@@ -100,7 +92,7 @@ class BluetoothRemoteGattCharacteristicBlueZ
 
   // Called by dbus:: on successful completion of a request to start
   // notifications.
-  void OnStartNotifySuccess(const NotifySessionCallback& callback);
+  void OnStartNotifySuccess(const base::Closure& callback);
 
   // Called by dbus:: on unsuccessful completion of a request to start
   // notifications.
@@ -118,24 +110,14 @@ class BluetoothRemoteGattCharacteristicBlueZ
                          const std::string& error_name,
                          const std::string& error_message);
 
-  // Calls StartNotifySession for each queued request.
-  void ProcessStartNotifyQueue();
-
   // Called by dbus:: on unsuccessful completion of a request to read or write
   // the characteristic value.
   void OnError(const ErrorCallback& error_callback,
                const std::string& error_name,
                const std::string& error_message);
 
-  // The total number of currently active value update sessions.
-  size_t num_notify_sessions_;
-
-  // Calls to StartNotifySession that are pending. This can happen during the
-  // first remote call to start notifications.
-  std::queue<PendingStartNotifyCall> pending_start_notify_calls_;
-
-  // True, if a Start or Stop notify call to bluetoothd is currently pending.
-  bool notify_call_pending_;
+  // True, if there exists a Bluez notify session.
+  bool has_notify_session_;
 
   // TODO(rkc): Investigate and fix ownership of the descriptor objects in this
   // map. See crbug.com/604166.