Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests

device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc

Index: device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc
diff --git a/device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc b/device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc
index d98c0ea8d2fcb7f5709672d03305dee3076abfe7..67928de6d1b2d42608168e997fd7058638904624 100644
--- a/device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc
+++ b/device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc
@@ -6,6 +6,7 @@
 
 #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"
@@ -19,8 +20,11 @@ BluetoothRemoteGattCharacteristicWin::BluetoothRemoteGattCharacteristicWin(
     : 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_);
@@ -41,7 +45,18 @@ BluetoothRemoteGattCharacteristicWin::BluetoothRemoteGattCharacteristicWin(
 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 {
@@ -105,8 +120,7 @@ BluetoothRemoteGattCharacteristicWin::GetPermissions() const {
 }
 
 bool BluetoothRemoteGattCharacteristicWin::IsNotifying() const {
-  NOTIMPLEMENTED();
-  return false;
+  return gatt_event_handle_ != nullptr;
 }
 
 std::vector<BluetoothGattDescriptor*>
@@ -140,8 +154,59 @@ bool BluetoothRemoteGattCharacteristicWin::UpdateValue(
 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(
@@ -216,6 +281,13 @@ void BluetoothRemoteGattCharacteristicWin::OnGetIncludedDescriptorsCallback(
   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);
@@ -236,8 +308,10 @@ void BluetoothRemoteGattCharacteristicWin::UpdateIncludedDescriptors(
     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)
@@ -255,8 +329,17 @@ void BluetoothRemoteGattCharacteristicWin::UpdateIncludedDescriptors(
                                                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(
@@ -324,18 +407,59 @@ void BluetoothRemoteGattCharacteristicWin::
 
 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.