Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests

device/bluetooth/bluetooth_task_manager_win.cc

Index: device/bluetooth/bluetooth_task_manager_win.cc
diff --git a/device/bluetooth/bluetooth_task_manager_win.cc b/device/bluetooth/bluetooth_task_manager_win.cc
index bcb3823e094a32b7ac84faaa29288fb74af275d2..f15b4d6613f983be6724fcdff7b59d3d273b0d5b 100644
--- a/device/bluetooth/bluetooth_task_manager_win.cc
+++ b/device/bluetooth/bluetooth_task_manager_win.cc
@@ -122,6 +122,50 @@ void GetDeviceState(const BLUETOOTH_DEVICE_INFO& device_info,
   state->authenticated = !!device_info.fAuthenticated;
 }
 
+typedef std::pair<
+    BLUETOOTH_GATT_EVENT_HANDLE,
+    device::BluetoothTaskManagerWin::GattCharacteristicValueChangedCallback>
+    CharacteristicValueChangedRegistration;
+
+// The key of CharacteristicValueChangedRegistrationMap is a
+// GattCharacteristicValueChangedCallback pointer (cast to PVOID) to make it
+// unique for different callbacks.
+typedef std::unordered_map<PVOID, CharacteristicValueChangedRegistration>

[scheib, 2016/04/05 22:44:02]

Why does the pointer need to be cast to PVOID? 

[gogerald1, 2016/04/06 22:54:48]

It was passed into OS as context which is PVOID when registering event, and the
OS will pass it back as context identifier. Add additional comments.

+    CharacteristicValueChangedRegistrationMap;
+
+CharacteristicValueChangedRegistrationMap
+    characteristic_value_changed_registrations;

[scheib, 2016/04/05 22:44:02]

Move these to private member variables, unless there's a reason for them to be
globals. If there is, they should go in the anonymous namespace and use a g_
prefix. 

[gogerald1, 2016/04/06 22:54:48]

Done. These variables are used by BluetoothTaskManagerWin and OS, so I think
it's clear to keep them in anonymous namespace. Here is in anonymous namespace.

+base::Lock characteristic_value_changed_registrations_lock;
+
+// 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) {
+  if (type != CharacteristicValueChangedEvent) {

[scheib, 2016/04/05 22:44:02]

Add DCHECK to assert what threads this is or is not run on. From below I read
that it may be a different thread then we make the windows API calls from.

[gogerald1, 2016/04/06 22:54:48]

Post callback to ui_task_runner_ below and according to my test OS invokes this
function in BluetoothApis.dll thread (comment below). I did not 
DCHECK(!ui_task_runner_) since test code simulates characteristic value change
in ui_task_runner_. 

+    // Right now, only characteristic value changed event is supported.
+    NOTREACHED();
+    return;
+  }
+
+  BLUETOOTH_GATT_VALUE_CHANGED_EVENT* event =
+      (BLUETOOTH_GATT_VALUE_CHANGED_EVENT*)event_parameter;
+  PBTH_LE_GATT_CHARACTERISTIC_VALUE new_value_win = event->CharacteristicValue;
+  scoped_ptr<std::vector<uint8_t>> new_value(
+      new std::vector<uint8_t>(new_value_win->DataSize));
+  for (ULONG i = 0; i < new_value_win->DataSize; i++)
+    (*new_value)[i] = new_value_win->Data[i];
+
+  // Lock shared data structure since this callback is invoked in
+  // BluetoothApis.dll thread.
+  base::AutoLock auto_lock(characteristic_value_changed_registrations_lock);
+  CharacteristicValueChangedRegistrationMap::const_iterator it =
+      characteristic_value_changed_registrations.find(context);
+  if (it == characteristic_value_changed_registrations.end())
+    return;
+
+  it->second.second.Run(std::move(new_value));
+}
+
 }  // namespace
 
 namespace device {
@@ -844,6 +888,65 @@ void BluetoothTaskManagerWin::WriteGattCharacteristicValue(
   ui_task_runner_->PostTask(FROM_HERE, base::Bind(callback, hr));
 }
 
+void BluetoothTaskManagerWin::RegisterGattCharacteristicValueChangedEvent(
+    base::FilePath service_path,
+    BTH_LE_GATT_CHARACTERISTIC characteristic,
+    BTH_LE_GATT_DESCRIPTOR ccc_descriptor,
+    const GattEventRegistrationCallback& callback,
+    const GattCharacteristicValueChangedCallback& registered_callback) {
+  BLUETOOTH_GATT_EVENT_HANDLE win_event_handle = NULL;

[scheib, 2016/04/05 22:44:02]

DCHECK(bluetooth_task_runner_->RunsTasksOnCurrentThread());

[gogerald1, 2016/04/06 22:54:48]

Done.

+
+  BLUETOOTH_GATT_VALUE_CHANGED_EVENT_REGISTRATION win_event_parameter;
+  memcpy(&(win_event_parameter.Characteristics[0]), &characteristic,
+         sizeof(BTH_LE_GATT_CHARACTERISTIC));
+  win_event_parameter.NumCharacteristics = 1;
+  PVOID user_event_handle = (PVOID)&registered_callback;
+  HRESULT hr =
+      win::BluetoothLowEnergyWrapper::GetInstance()->RegisterGattEvents(
+          service_path, CharacteristicValueChangedEvent, &win_event_parameter,
+          &OnGetGattEventWin, user_event_handle, &win_event_handle);
+
+  // Sets the Client Characteristic Configuration descriptor.
+  if (SUCCEEDED(hr)) {
+    BTH_LE_GATT_DESCRIPTOR_VALUE new_cccd_value;
+    RtlZeroMemory(&new_cccd_value, sizeof(new_cccd_value));
+    new_cccd_value.DescriptorType = ClientCharacteristicConfiguration;
+    if (characteristic.IsNotifiable) {
+      new_cccd_value.ClientCharacteristicConfiguration
+          .IsSubscribeToNotification = TRUE;
+    } else {
+      new_cccd_value.ClientCharacteristicConfiguration.IsSubscribeToIndication =
+          TRUE;
+    }
+
+    hr = win::BluetoothLowEnergyWrapper::GetInstance()->WriteDescriptorValue(
+        service_path, (PBTH_LE_GATT_DESCRIPTOR)(&ccc_descriptor),
+        &new_cccd_value);
+  }
+
+  if (SUCCEEDED(hr)) {
+    characteristic_value_changed_registrations[user_event_handle] =

[scheib, 2016/04/05 22:44:02]

This also needs to be guarded?

[gogerald1, 2016/04/06 22:54:48]

may have no problem since remove and add operation are running in the same
thread (bluetooth_task_runner_), and add and read operation should have no
conflict. Add operation should always add a different callback. But add the lock
here to secure.

[scheib, 2016/04/06 23:56:39]

The map was searched and added to from different threads, so there was a race.
The lock here is required. 

[gogerald1, 2016/04/08 17:03:30]

Acknowledged.

+        std::make_pair(win_event_handle, registered_callback);
+  }
+
+  ui_task_runner_->PostTask(FROM_HERE,
+                            base::Bind(callback, user_event_handle, hr));
+}
+
+void BluetoothTaskManagerWin::UnregisterGattCharacteristicValueChangedEvent(
+    PVOID event_handle) {
+  DCHECK(bluetooth_task_runner_->RunsTasksOnCurrentThread());
+
+  base::AutoLock auto_lock(characteristic_value_changed_registrations_lock);
+  CharacteristicValueChangedRegistrationMap::const_iterator it =
+      characteristic_value_changed_registrations.find(event_handle);
+  if (it != characteristic_value_changed_registrations.end()) {
+    win::BluetoothLowEnergyWrapper::GetInstance()->UnregisterGattEvent(
+        it->second.first);
+    characteristic_value_changed_registrations.erase(event_handle);
+  }
+}
+
 void BluetoothTaskManagerWin::PostGetGattIncludedCharacteristics(
     const base::FilePath& service_path,
     const BluetoothUUID& uuid,
@@ -876,8 +979,6 @@ void BluetoothTaskManagerWin::PostReadGattCharacteristicValue(
       FROM_HERE,
       base::Bind(&BluetoothTaskManagerWin::ReadGattCharacteristicValue, this,
                  service_path, *characteristic, callback));
-  FOR_EACH_OBSERVER(BluetoothTaskManagerWin::Observer, observers_,
-                    OnAttemptReadGattCharacteristic());
 }
 
 void BluetoothTaskManagerWin::PostWriteGattCharacteristicValue(
@@ -890,8 +991,30 @@ void BluetoothTaskManagerWin::PostWriteGattCharacteristicValue(
       FROM_HERE,
       base::Bind(&BluetoothTaskManagerWin::WriteGattCharacteristicValue, this,
                  service_path, *characteristic, new_value, callback));
-  FOR_EACH_OBSERVER(BluetoothTaskManagerWin::Observer, observers_,
-                    OnAttemptWriteGattCharacteristic());
+}
+
+void BluetoothTaskManagerWin::PostRegisterGattCharacteristicValueChangedEvent(
+    const base::FilePath& service_path,
+    const PBTH_LE_GATT_CHARACTERISTIC characteristic,
+    const PBTH_LE_GATT_DESCRIPTOR ccc_descriptor,
+    const GattEventRegistrationCallback& callback,
+    const GattCharacteristicValueChangedCallback& registered_callback) {
+  DCHECK(ui_task_runner_->RunsTasksOnCurrentThread());
+  bluetooth_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(
+          &BluetoothTaskManagerWin::RegisterGattCharacteristicValueChangedEvent,
+          this, service_path, *characteristic, *ccc_descriptor, callback,
+          registered_callback));
+}
+
+void BluetoothTaskManagerWin::PostUnregisterGattCharacteristicValueChangedEvent(
+    PVOID event_handle) {
+  DCHECK(ui_task_runner_->RunsTasksOnCurrentThread());
+  bluetooth_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&BluetoothTaskManagerWin::
+                                UnregisterGattCharacteristicValueChangedEvent,
+                            this, event_handle));
 }
 
 }  // namespace device