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..5bb1f705e60bb6aa3ffd4724e78d3bd09dddfadc 100644
--- a/device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc
+++ b/device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc
@@ -6,10 +6,24 @@
 
 #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 {
+
+// Function to be registered to OS to monitor Bluetooth LE GATT event.
+void OnGetGattEventWin(BTH_LE_GATT_EVENT_TYPE type,
+                       PVOID event_parameter,
+                       PVOID context) {
+  device::BluetoothRemoteGattCharacteristicWin* characteristic =
+      (device::BluetoothRemoteGattCharacteristicWin*)context;

[ortuno, 2016/03/07 18:48:45]

Space between parentheses and context.

[gogerald1, 2016/03/07 22:52:49]

Is this google code style? It looks making more sense without space and 'git cl
format' will remove space between them.

[ortuno, 2016/03/08 00:05:36]

Interesting. Well if git cl format says no space then no space it is.

[gogerald1, 2016/03/08 19:22:23]

Acknowledged.

+  characteristic->OnGetRemoteGattEvent(type, event_parameter);
+}
+
+}  // namespace
+
 namespace device {
 
 BluetoothRemoteGattCharacteristicWin::BluetoothRemoteGattCharacteristicWin(
@@ -21,6 +35,9 @@ BluetoothRemoteGattCharacteristicWin::BluetoothRemoteGattCharacteristicWin(
       ui_task_runner_(ui_task_runner),
       characteristic_added_notified_(false),
       characteristic_value_read_or_write_in_progress_(false),
+      number_of_active_notify_sessions_(0),
+      gatt_event_registeration_in_progress_(false),
+      gatt_event_handle_(NULL),
       weak_ptr_factory_(this) {
   DCHECK(ui_task_runner_->RunsTasksOnCurrentThread());
   DCHECK(parent_service_);
@@ -41,6 +58,11 @@ BluetoothRemoteGattCharacteristicWin::BluetoothRemoteGattCharacteristicWin(
 BluetoothRemoteGattCharacteristicWin::~BluetoothRemoteGattCharacteristicWin() {
   DCHECK(ui_task_runner_->RunsTasksOnCurrentThread());
 
+  if (gatt_event_handle_ != NULL) {
+    task_manager_->UnregisterGattCharacteristicValueChangedEvent(
+        gatt_event_handle_);
+    gatt_event_handle_ = NULL;
+  }
   parent_service_->GetWinAdapter()->NotifyGattCharacteristicRemoved(this);
 }
 
@@ -105,8 +127,7 @@ BluetoothRemoteGattCharacteristicWin::GetPermissions() const {
 }
 
 bool BluetoothRemoteGattCharacteristicWin::IsNotifying() const {
-  NOTIMPLEMENTED();
-  return false;
+  return gatt_event_handle_ != NULL;
 }
 
 std::vector<BluetoothGattDescriptor*>
@@ -140,8 +161,31 @@ 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()));
+    callback.Run(std::move(notify_session));

[ortuno, 2016/03/07 18:48:45]

StartNotifySession should be async. Post a task here.

[gogerald1, 2016/03/07 22:52:49]

Done.

+    number_of_active_notify_sessions_++;
+    return;
+  }
+
+  if (gatt_event_registeration_in_progress_) {
+    start_notify_session_callbacks_.push_back(
+        std::make_pair(callback, error_callback));
+    return;
+  }
+
+  start_notify_session_callbacks_.push_back(
+      std::make_pair(callback, error_callback));
+  task_manager_->PostRegisterGattCharacteristicValueChangedEvent(
+      parent_service_->GetServicePath(), characteristic_info_.get(),
+      base::Bind(
+          &BluetoothRemoteGattCharacteristicWin::GattEventRegistrationCallback,
+          weak_ptr_factory_.GetWeakPtr()),
+      &OnGetGattEventWin, (PVOID) this);

[ortuno, 2016/03/07 18:48:45]

Wouldn't you get a null pointer dereference if windows calls OnGetGattEventWin
after the characteristic is destroyed?

[gogerald1, 2016/03/07 22:52:49]

Event will be unregistered in ~BluetoothRemoteGattCharacteristicWin()

[ortuno, 2016/03/08 00:05:36]

Could the following happen?

PostTask(~BluetoothRemoteGattCharacteristicWin())
PostTask(OnGetGattEventWin())


This would mean that there is a OnGetGattEventWin waiting to be executed, is
~BluetoothRemoteGattCharacteristicWin going to cancel the pending
OnGetGattEventWin?

[gogerald1, 2016/03/08 19:22:23]

This could not happen since UnregisterGattCharacteristicValueChangedEvent is a
synchronous operation.

+  gatt_event_registeration_in_progress_ = true;

[ortuno, 2016/03/07 18:48:45]

Are you sure the registering for the event is enough? Don't you need to write to
the CCC descriptor as well?

[gogerald1, 2016/03/07 22:52:49]

No needed,

[ortuno, 2016/03/08 00:05:36]

This sort of statements are not useful when trying to understand your process.
Could you explain how you made sure that writing to the descriptor is not needed
and document your findings in the code?

[scheib, 2016/03/08 18:30:07]

If a question ever comes up from a reviewer it is best to explain the answer in
code or comments. The same question is likely to come up again for future
readers - and they will not read comments on the code review. 

[gogerald1, 2016/03/08 19:22:23]

My judgement is based on this API document
(https://msdn.microsoft.com/en-us/library/windows/hardware/hh450806(v=vs.85).aspx)
from Microsoft and experiment, what do you think is the best way to explain it
in code or comments?

[ortuno, 2016/03/09 16:53:59]

Nothing in the link you sent mentions anything about notifications/indications.
Could you please explain in detail how you know that you don't need to manually
write to the descriptor? Do you have an app that tells you when you write to the
descriptors? Or do you have a device like a heart rate monitor that starts
notifying after you wrote to the descriptors?

Sorry for being too pushy here but we've run into this problem before and I want
to make sure we get it right this time.

[gogerald1, 2016/03/09 23:59:12]

I definitely have told you that I have experimented these functionalities by
using Smartsetup extensions and Simulator app with Nexus 6. My Nexus 6 and
chrome can receive correct information. 

 }
 
 void BluetoothRemoteGattCharacteristicWin::ReadRemoteCharacteristic(
@@ -195,6 +239,27 @@ void BluetoothRemoteGattCharacteristicWin::WriteRemoteCharacteristic(
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
+void BluetoothRemoteGattCharacteristicWin::OnGetRemoteGattEvent(

[ortuno, 2016/03/07 18:48:45]

Does this get called after reading or writing to a characteristic. Android and
Chrome OS do it so I wouldn't be surprised if windows does it as well.

[gogerald1, 2016/03/07 22:52:49]

No the case on Windows. I don't see why it should work in that way (read doesn't
change the value, write indicates you know the value changed). Saw you create a
bug for it.

[ortuno, 2016/03/08 00:05:36]

Can you confirm that you registered for an event and then read the value and the
OnGetRemoteGattEvent didn't get called?
That's not really up to the implementation. The implementation has to match the
interface. If you don't agree with the interface you need to submit a change
changing the description of the callback.

https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b...

[scheib, 2016/03/08 18:30:07]

If the interface isn't documented precisely enough, please fix it as well.

[gogerald1, 2016/03/08 19:22:23]

I have confirmed it by experiment. I think it's clear enough. Please suggest
specific comments if you think could make it better.

[ortuno, 2016/03/09 16:53:59]

Just to be sure, you registered for the event and then tried reading the
characteristic and saw no event?

[gogerald1, 2016/03/09 23:59:12]

yes

+    BTH_LE_GATT_EVENT_TYPE type,
+    PVOID event_parameter) {
+  if (type == CharacteristicValueChangedEvent) {
+    BLUETOOTH_GATT_VALUE_CHANGED_EVENT* event =
+        (BLUETOOTH_GATT_VALUE_CHANGED_EVENT*)event_parameter;

[ortuno, 2016/03/07 18:48:45]

Space between parentheses and evet_parameter

[gogerald1, 2016/03/07 22:52:49]

Acknowledged.

+    PBTH_LE_GATT_CHARACTERISTIC_VALUE new_value_win =
+        event->CharacteristicValue;
+    scoped_ptr<uint8_t> new_value(new uint8_t[new_value_win->DataSize]);

[ortuno, 2016/03/07 18:48:45]

Why do you use a pointer to a uint8? I think scoped_ptr<std::vector<uint8_t>>
would be better.

scoped_ptr<std::vector<uint8_t>> new_value(new
std::vector<uint8_t>(new_value_win->DataSize));

That way you can do the following in OnGattCharacteristicValueChanged:

characteristic_value_.assign(new_value->first, new_value->last);

[gogerald1, 2016/03/07 22:52:49]

Done.

+    for (ULONG i = 0; i < new_value_win->DataSize; i++)
+      new_value.get()[i] = new_value_win->Data[i];
+
+    ui_task_runner_->PostTask(FROM_HERE,
+                              base::Bind(&BluetoothRemoteGattCharacteristicWin::
+                                             OnGattCharacteristicValueChanged,
+                                         weak_ptr_factory_.GetWeakPtr(),
+                                         base::Passed(std::move(new_value)),
+                                         new_value_win->DataSize));

[ortuno, 2016/03/07 18:48:45]

I think you should return here and add a NOTREACHED(); after.

[gogerald1, 2016/03/07 22:52:49]

Done.

+  }
+}
+
 void BluetoothRemoteGattCharacteristicWin::Update() {
   DCHECK(ui_task_runner_->RunsTasksOnCurrentThread());
 
@@ -209,6 +274,24 @@ uint16_t BluetoothRemoteGattCharacteristicWin::GetAttributeHandle() const {
   return characteristic_info_->AttributeHandle;
 }
 
+void BluetoothRemoteGattCharacteristicWin::StopNotifySession() {
+  DCHECK(ui_task_runner_->RunsTasksOnCurrentThread());
+
+  if (!IsNotifying())
+    return;
+
+  if (number_of_active_notify_sessions_ > 1) {
+    number_of_active_notify_sessions_--;
+    return;
+  }
+
+  task_manager_->UnregisterGattCharacteristicValueChangedEvent(
+      gatt_event_handle_);
+  gatt_event_handle_ = NULL;
+  number_of_active_notify_sessions_ = 0;
+  start_notify_session_callbacks_.clear();

[ortuno, 2016/03/07 18:48:45]

When would you have pending callbacks?

[gogerald1, 2016/03/07 22:52:49]

Done.

+}
+
 void BluetoothRemoteGattCharacteristicWin::OnGetIncludedDescriptorsCallback(
     scoped_ptr<BTH_LE_GATT_DESCRIPTOR> descriptors,
     uint16_t num,
@@ -333,9 +416,45 @@ BluetoothRemoteGattCharacteristicWin::HRESULTToGattErrorCode(HRESULT hr) {
     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<uint8_t> new_value,
+    ULONG size) {
+  DCHECK(ui_task_runner_->RunsTasksOnCurrentThread());
+
+  characteristic_value_.clear();
+  for (ULONG i = 0; i < size; i++)
+    characteristic_value_.push_back(new_value.get()[i]);

[ortuno, 2016/03/07 18:48:45]

As mentioned before I think you should pass in a vector to avoid manually
copying the new value.

[gogerald1, 2016/03/07 22:52:49]

Done.

+  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)) {
+    for (auto callback : callbacks) {

[ortuno, 2016/03/07 18:48:45]

You should use a reference of the callback not a copy:

for (auto callback& : callbacks)

[gogerald1, 2016/03/07 22:52:49]

Done.

+      scoped_ptr<BluetoothGattNotifySessionWin> notify_session(

[ortuno, 2016/03/07 18:48:45]

You don't need to create a scoped pointer first you can just do it inline:

callback.first.Run(make_scoped_ptr(new BluetoothGattNotifySessionWin(...)));

[gogerald1, 2016/03/07 22:52:49]

Done.

+          new BluetoothGattNotifySessionWin(weak_ptr_factory_.GetWeakPtr()));
+      callback.first.Run(std::move(notify_session));

[ortuno, 2016/03/07 18:48:45]

I think you should just post a task here. Otherwise don't change
gatt_event_registration_in_progress_ until you've run all the callbacks. A
callback could try to register a characteristic again and at this point both
IsNotifying() and gatt_event_registration_in_progress are false so we would try
to register for notifications again.

[gogerald1, 2016/03/07 22:52:49]

Done.

+      number_of_active_notify_sessions_++;
+    }
+    gatt_event_handle_ = event_handle;
+  } else {
+    for (auto callback : callbacks)

[ortuno, 2016/03/07 18:48:45]

Same here use a reference not a copy.

[gogerald1, 2016/03/07 22:52:49]

Done.

+      callback.second.Run(HRESULTToGattErrorCode(hr));
+  }
+}
+
 }  // namespace device.