Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests

device/bluetooth/bluetooth_remote_gatt_characteristic_win.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 "device/bluetooth/bluetooth_remote_gatt_characteristic_win.h"
 
 #include "base/bind.h"
 #include "device/bluetooth/bluetooth_adapter_win.h"
+#include "device/bluetooth/bluetooth_gatt_notify_session_win.h"
 #include "device/bluetooth/bluetooth_remote_gatt_descriptor_win.h"
 #include "device/bluetooth/bluetooth_remote_gatt_service_win.h"
 #include "device/bluetooth/bluetooth_task_manager_win.h"
 
 namespace device {
 
 BluetoothRemoteGattCharacteristicWin::BluetoothRemoteGattCharacteristicWin(
     BluetoothRemoteGattServiceWin* parent_service,
     BTH_LE_GATT_CHARACTERISTIC* characteristic_info,
     scoped_refptr<base::SequencedTaskRunner>& ui_task_runner)
     : parent_service_(parent_service),
       characteristic_info_(characteristic_info),
       ui_task_runner_(ui_task_runner),
+      included_descriptors_discovered_(false),
       characteristic_added_notified_(false),
       characteristic_value_read_or_write_in_progress_(false),
+      gatt_event_registeration_in_progress_(false),
+      gatt_event_handle_(nullptr),
       weak_ptr_factory_(this) {
   DCHECK(ui_task_runner_->RunsTasksOnCurrentThread());
   DCHECK(parent_service_);
   DCHECK(characteristic_info_);
 
   task_manager_ =
       parent_service_->GetWinAdapter()->GetWinBluetoothTaskManager();
   DCHECK(task_manager_);
   characteristic_uuid_ =
       BluetoothTaskManagerWin::BluetoothLowEnergyUuidToBluetoothUuid(
           characteristic_info_->CharacteristicUuid);
   characteristic_identifier_ =
       parent_service_->GetIdentifier() + "_" +
       std::to_string(characteristic_info_->AttributeHandle);
   Update();
 }
 
 BluetoothRemoteGattCharacteristicWin::~BluetoothRemoteGattCharacteristicWin() {
   DCHECK(ui_task_runner_->RunsTasksOnCurrentThread());
 
+  ClearIncludedDescriptors();
+
+  if (gatt_event_handle_ != nullptr) {
+    task_manager_->PostUnregisterGattCharacteristicValueChangedEvent(
+        gatt_event_handle_);
+    gatt_event_handle_ = nullptr;
+  }
   parent_service_->GetWinAdapter()->NotifyGattCharacteristicRemoved(this);
+
+  // Clear pending StartNotifySession callbacks.
+  for (const auto& callback : start_notify_session_callbacks_)
+    callback.second.Run(BluetoothGattService::GATT_ERROR_FAILED);
 }
 
 std::string BluetoothRemoteGattCharacteristicWin::GetIdentifier() const {
   return characteristic_identifier_;
 }
 
 BluetoothUUID BluetoothRemoteGattCharacteristicWin::GetUUID() const {
   return characteristic_uuid_;
 }
 
@@ skipping 43 matching lines @@
 
   if (characteristic_info_->IsReadable)
     permissions = permissions | PERMISSION_READ;
   if (characteristic_info_->IsWritable)
     permissions = permissions | PERMISSION_WRITE;
 
   return permissions;
 }
 
 bool BluetoothRemoteGattCharacteristicWin::IsNotifying() const {
-  NOTIMPLEMENTED();
-  return false;
+  return gatt_event_handle_ != nullptr;
 }
 
 std::vector<BluetoothGattDescriptor*>
 BluetoothRemoteGattCharacteristicWin::GetDescriptors() const {
   std::vector<BluetoothGattDescriptor*> descriptors;
   for (const auto& descriptor : included_descriptors_)
     descriptors.push_back(descriptor.second.get());
   return descriptors;
 }
 
@@ skipping 13 matching lines @@
 
 bool BluetoothRemoteGattCharacteristicWin::UpdateValue(
     const std::vector<uint8_t>& value) {
   NOTIMPLEMENTED();
   return false;
 }
 
 void BluetoothRemoteGattCharacteristicWin::StartNotifySession(
     const NotifySessionCallback& callback,
     const ErrorCallback& error_callback) {
-  NOTIMPLEMENTED();
-  error_callback.Run(BluetoothGattService::GATT_ERROR_NOT_SUPPORTED);
+  DCHECK(ui_task_runner_->RunsTasksOnCurrentThread());
+
+  if (IsNotifying()) {
+    scoped_ptr<BluetoothGattNotifySessionWin> notify_session(
+        new BluetoothGattNotifySessionWin(weak_ptr_factory_.GetWeakPtr()));
+    ui_task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(callback, base::Passed(std::move(notify_session))));
+    return;
+  }
+
+  if (!characteristic_info_->IsNotifiable &&
+      !characteristic_info_->IsIndicatable) {
+    ui_task_runner_->PostTask(
+        FROM_HERE, base::Bind(error_callback,
+                              BluetoothGattService::GATT_ERROR_NOT_SUPPORTED));
+    return;
+  }
+
+  start_notify_session_callbacks_.push_back(
+      std::make_pair(callback, error_callback));
+  if (gatt_event_registeration_in_progress_ ||
+      !included_descriptors_discovered_) {

[ortuno, 2016/03/30 22:34:23]

Maybe we don't need this logic. One should only access a characteristic if all
descriptors have been discovered. Web Bluetooth for example will only allow you
to access a characteristic once GattServicesDiscovered is true for a device,
which happens only after all services, characteristics and descriptors have been
discovered for the device. Similarly in Chrome Apps, the Service Added event is
only called once all characteristics and descriptors are discovered for the
service. Why do you think we need to check if the descriptors have been
discovered?

[gogerald1, 2016/03/31 17:44:13]

remove as we discussed on hangout

+    return;
+  }
+
+  StartNotifySession();
+}
+
+void BluetoothRemoteGattCharacteristicWin::StartNotifySession() {

[ortuno, 2016/03/30 22:34:23]

Move this function after GattEventRegistrationCallback to match the header.

[gogerald1, 2016/03/31 17:44:13]

Done.

+  gatt_event_registeration_in_progress_ = true;
+
+  if (ccc_descriptor_identifier_.empty()) {

[ortuno, 2016/03/30 22:34:23]

I don't think you need to save the descriptor. You can just use
GetDescriptorsByUUID and follow the same pattern as android:
https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b...

That code will also let you pass one of the new tests.

[gogerald1, 2016/03/31 17:44:13]

Done.

+    ui_task_runner_->PostTask(
+        FROM_HERE, base::Bind(&BluetoothRemoteGattCharacteristicWin::
+                                  GattEventRegistrationCallback,
+                              weak_ptr_factory_.GetWeakPtr(), nullptr,
+                              E_BLUETOOTH_ATT_REQUEST_NOT_SUPPORTED));
+    return;
+  }
+
+  BluetoothRemoteGattDescriptorWin* ccc_descriptor =
+      included_descriptors_.find(ccc_descriptor_identifier_)->second.get();
+  DCHECK(ccc_descriptor);
+  task_manager_->PostRegisterGattCharacteristicValueChangedEvent(
+      parent_service_->GetServicePath(), characteristic_info_.get(),
+      ccc_descriptor->GetWinDescriptorInfo(),
+      base::Bind(
+          &BluetoothRemoteGattCharacteristicWin::GattEventRegistrationCallback,
+          weak_ptr_factory_.GetWeakPtr()),
+      base::Bind(&BluetoothRemoteGattCharacteristicWin::
+                     OnGattCharacteristicValueChanged,
+                 weak_ptr_factory_.GetWeakPtr()));
 }
 
 void BluetoothRemoteGattCharacteristicWin::ReadRemoteCharacteristic(
     const ValueCallback& callback,
     const ErrorCallback& error_callback) {
   DCHECK(ui_task_runner_->RunsTasksOnCurrentThread());
 
   if (!characteristic_info_.get()->IsReadable) {
     error_callback.Run(BluetoothGattService::GATT_ERROR_NOT_PERMITTED);
     return;
@@ skipping 54 matching lines @@
   return characteristic_info_->AttributeHandle;
 }
 
 void BluetoothRemoteGattCharacteristicWin::OnGetIncludedDescriptorsCallback(
     scoped_ptr<BTH_LE_GATT_DESCRIPTOR> descriptors,
     uint16_t num,
     HRESULT hr) {
   DCHECK(ui_task_runner_->RunsTasksOnCurrentThread());
 
   UpdateIncludedDescriptors(descriptors.get(), num);
+
+  included_descriptors_discovered_ = true;
+  if (start_notify_session_callbacks_.size() &&
+      !gatt_event_registeration_in_progress_) {
+    StartNotifySession();
+  }
+
   if (!characteristic_added_notified_) {
     characteristic_added_notified_ = true;
     parent_service_->GetWinAdapter()->NotifyGattCharacteristicAdded(this);
   }
 }
 
 void BluetoothRemoteGattCharacteristicWin::UpdateIncludedDescriptors(
     PBTH_LE_GATT_DESCRIPTOR descriptors,
     uint16_t num) {
   if (num == 0) {
     included_descriptors_.clear();
     return;
   }
 
   // First, remove descriptors that no longer exist.
   std::vector<std::string> to_be_removed;
   for (const auto& d : included_descriptors_) {
     if (!DoesDescriptorExist(descriptors, num, d.second.get()))
       to_be_removed.push_back(d.second->GetIdentifier());
   }
-  for (auto id : to_be_removed)
+  for (auto id : to_be_removed) {
+    included_descriptors_[id].reset();

[ortuno, 2016/03/30 22:34:23]

Why reset and then delete?

[gogerald1, 2016/03/31 17:44:13]

In case of infinitely recurses, refer
https://codereview.chromium.org/1763983002/

     included_descriptors_.erase(id);
+  }
 
   // Return if no new descriptors have been added.
   if (included_descriptors_.size() == num)
     return;
 
   // Add new descriptors.
   for (uint16_t i = 0; i < num; i++) {
     if (!IsDescriptorDiscovered(descriptors[i].DescriptorUuid,
                                 descriptors[i].AttributeHandle)) {
       PBTH_LE_GATT_DESCRIPTOR win_descriptor_info =
           new BTH_LE_GATT_DESCRIPTOR();
       *win_descriptor_info = descriptors[i];
       BluetoothRemoteGattDescriptorWin* descriptor =
           new BluetoothRemoteGattDescriptorWin(this, win_descriptor_info,
                                                ui_task_runner_);
       included_descriptors_[descriptor->GetIdentifier()] =
           make_scoped_ptr(descriptor);
+      if (descriptor->GetUUID() ==
+          descriptor->ClientCharacteristicConfigurationUuid()) {
+        ccc_descriptor_identifier_ = descriptor->GetIdentifier();
+      }
     }
   }
+
+  if (included_descriptors_.find(ccc_descriptor_identifier_) ==
+      included_descriptors_.end()) {
+    ccc_descriptor_identifier_.clear();
+  }
 }
 
 bool BluetoothRemoteGattCharacteristicWin::IsDescriptorDiscovered(
     BTH_LE_UUID& uuid,
     uint16_t attribute_handle) {
   BluetoothUUID bt_uuid =
       BluetoothTaskManagerWin::BluetoothLowEnergyUuidToBluetoothUuid(uuid);
   for (const auto& d : included_descriptors_) {
     if (bt_uuid == d.second->GetUUID() &&
         attribute_handle == d.second->GetAttributeHandle()) {
@@ skipping 47 matching lines @@
   if (FAILED(hr)) {
     callbacks.second.Run(HRESULTToGattErrorCode(hr));
   } else {
     callbacks.first.Run();
   }
   characteristic_value_read_or_write_in_progress_ = false;
 }
 
 BluetoothGattService::GattErrorCode
 BluetoothRemoteGattCharacteristicWin::HRESULTToGattErrorCode(HRESULT hr) {
+  if (HRESULT_FROM_WIN32(ERROR_INVALID_USER_BUFFER) == hr)
+    return BluetoothGattService::GATT_ERROR_INVALID_LENGTH;
+
   switch (hr) {
     case E_BLUETOOTH_ATT_READ_NOT_PERMITTED:
     case E_BLUETOOTH_ATT_WRITE_NOT_PERMITTED:
       return BluetoothGattService::GATT_ERROR_NOT_PERMITTED;
     case E_BLUETOOTH_ATT_UNKNOWN_ERROR:
       return BluetoothGattService::GATT_ERROR_UNKNOWN;
-    case ERROR_INVALID_USER_BUFFER:
     case E_BLUETOOTH_ATT_INVALID_ATTRIBUTE_VALUE_LENGTH:
       return BluetoothGattService::GATT_ERROR_INVALID_LENGTH;
+    case E_BLUETOOTH_ATT_REQUEST_NOT_SUPPORTED:
+      return BluetoothGattService::GATT_ERROR_NOT_SUPPORTED;
     default:
       return BluetoothGattService::GATT_ERROR_FAILED;
   }
 }
 
+void BluetoothRemoteGattCharacteristicWin::OnGattCharacteristicValueChanged(
+    scoped_ptr<std::vector<uint8_t>> new_value) {
+  DCHECK(ui_task_runner_->RunsTasksOnCurrentThread());
+
+  characteristic_value_.assign(new_value->begin(), new_value->end());
+  parent_service_->GetWinAdapter()->NotifyGattCharacteristicValueChanged(
+      this, characteristic_value_);
+}
+
+void BluetoothRemoteGattCharacteristicWin::GattEventRegistrationCallback(
+    BLUETOOTH_GATT_EVENT_HANDLE event_handle,
+    HRESULT hr) {
+  DCHECK(ui_task_runner_->RunsTasksOnCurrentThread());
+
+  gatt_event_registeration_in_progress_ = false;
+  std::vector<std::pair<NotifySessionCallback, ErrorCallback>> callbacks;
+  callbacks.swap(start_notify_session_callbacks_);
+  if (SUCCEEDED(hr)) {
+    gatt_event_handle_ = event_handle;
+    for (const auto& callback : callbacks) {
+      callback.first.Run(make_scoped_ptr(
+          new BluetoothGattNotifySessionWin(weak_ptr_factory_.GetWeakPtr())));
+    }
+  } else {
+    for (const auto& callback : callbacks)
+      callback.second.Run(HRESULTToGattErrorCode(hr));
+  }
+}
+
+void BluetoothRemoteGattCharacteristicWin::ClearIncludedDescriptors() {
+  // Explicitly reset to null to ensure that calling GetDescriptor() on the
+  // removed descriptor in GattDescriptorRemoved() returns null.
+  for (auto& entry : included_descriptors_)
+    entry.second.reset();
+  included_descriptors_.clear();
+}
+
 }  // namespace device.