arc: bluetooth: Always acquire a NotifySession

components/arc/bluetooth/arc_bluetooth_bridge.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/arc/bluetooth/arc_bluetooth_bridge.h"
 
 #include <bluetooth/bluetooth.h>
 #include <fcntl.h>
 #include <stddef.h>
 #include <sys/socket.h>
@@ skipping 1081 matching lines @@
     mojom::BluetoothGattIDPtr char_id,
     const RegisterForGattNotificationCallback& callback) {
   BluetoothRemoteGattCharacteristic* characteristic = FindGattCharacteristic(
       std::move(remote_addr), std::move(service_id), std::move(char_id));
 
   if (!characteristic) {
     LOG(WARNING) << __func__ << " Characteristic is not existed.";
     return;
   }
 
-  if (characteristic->IsNotifying()) {
-    callback.Run(mojom::BluetoothGattStatus::GATT_SUCCESS);
-    return;
-  }
-
   characteristic->StartNotifySession(

[puthik_chromium, 2016/07/28 22:40:31]

The notifying session is always acquire here.

       base::Bind(&ArcBluetoothBridge::OnGattNotifyStartDone,
                  weak_factory_.GetWeakPtr(), callback,
                  characteristic->GetIdentifier()),
       base::Bind(&OnGattOperationError, callback));
 }
 
 void ArcBluetoothBridge::DeregisterForGattNotification(
     mojom::BluetoothAddressPtr remote_addr,
     mojom::BluetoothGattServiceIDPtr service_id,
     mojom::BluetoothGattIDPtr char_id,
     const DeregisterForGattNotificationCallback& callback) {
   DCHECK(CalledOnValidThread());
 
   BluetoothRemoteGattCharacteristic* characteristic = FindGattCharacteristic(
       std::move(remote_addr), std::move(service_id), std::move(char_id));
 
   if (!characteristic) {
     LOG(WARNING) << __func__ << " Characteristic is not existed.";
     return;
   }
 
-  if (!characteristic->IsNotifying()) {
-    callback.Run(mojom::BluetoothGattStatus::GATT_SUCCESS);
-    return;
-  }
-
   std::string char_id_str = characteristic->GetIdentifier();
+
+  if (!characteristic->IsNotifying()) {

[ortuno, 2016/07/28 22:56:00]

IsNotifying() will always be true as long as there is a client holding a
NotifySession. If arc has a notify session then this will always be true. The
callback for Stop will always be called so you can just call stop on your notify
session.

[puthik_chromium, 2016/07/28 23:11:12]

Since this is always true. Changed to DCHECK.

+    callback.Run(mojom::BluetoothGattStatus::GATT_SUCCESS);
+    LOG(WARNING) << __func__ << ("Notification is stopped");

[ortuno, 2016/07/28 22:21:06]

I don't think this solves the problem. You are still not acquiring a
NotifySession if any other clients e.g. a website, have a NotifySession. Once
other clients drops their sessions notifications will stop.

[puthik_chromium, 2016/07/28 22:40:31]

This is the Deregister function. I think delete the if
(characteristic->IsNotifying()) in Register function above should solve the
problem.

[ortuno, 2016/07/28 22:56:00]

Ah my bad. You are right.

+    if (notification_session_.find(char_id_str) != notification_session_.end())
+      notification_session_.erase(char_id_str);
+    return;
+  }
+
   std::unique_ptr<BluetoothGattNotifySession> notify =
       std::move(notification_session_[char_id_str]);
   notification_session_.erase(char_id_str);
   notify->Stop(base::Bind(&OnGattOperationDone, callback));
 }
 
 void ArcBluetoothBridge::ReadRemoteRssi(
     mojom::BluetoothAddressPtr remote_addr,
     const ReadRemoteRssiCallback& callback) {
   BluetoothDevice* device =
@@ skipping 507 matching lines @@
   LOG(WARNING) << "Bluetooth instance is too old (version " << version
                << ") need version " << version_need;
   return false;
 }
 
 bool ArcBluetoothBridge::CalledOnValidThread() {
   return thread_checker_.CalledOnValidThread();
 }
 
 }  // namespace arc