Implement BluetoothRemoteGattServiceWin and related unit tests

device/bluetooth/bluetooth_low_energy_win.cc

Index: device/bluetooth/bluetooth_low_energy_win.cc
diff --git a/device/bluetooth/bluetooth_low_energy_win.cc b/device/bluetooth/bluetooth_low_energy_win.cc
index 29932b9301e258b97ff4241e14b1148fcff7fbc9..7eb811fdcfeb2ff504ba53e9333eeb29afd87e5c 100644
--- a/device/bluetooth/bluetooth_low_energy_win.cc
+++ b/device/bluetooth/bluetooth_low_energy_win.cc
@@ -650,10 +650,6 @@ BluetoothLowEnergyDeviceInfo::BluetoothLowEnergyDeviceInfo()
 BluetoothLowEnergyDeviceInfo::~BluetoothLowEnergyDeviceInfo() {
 }
 
-bool IsBluetoothLowEnergySupported() {
-  return base::win::GetVersion() >= base::win::VERSION_WIN8;
-}
-
 bool ExtractBluetoothAddressFromDeviceInstanceIdForTesting(
     const std::string& instance_id,
     BLUETOOTH_ADDRESS* btha,
@@ -682,6 +678,10 @@ void BluetoothLowEnergyWrapper::SetInstanceForTest(
 BluetoothLowEnergyWrapper::BluetoothLowEnergyWrapper() {}
 BluetoothLowEnergyWrapper::~BluetoothLowEnergyWrapper() {}
 
+bool BluetoothLowEnergyWrapper::IsBluetoothLowEnergySupported() {
+  return base::win::GetVersion() >= base::win::VERSION_WIN8;
+}
+
 bool BluetoothLowEnergyWrapper::EnumerateKnownBluetoothLowEnergyDevices(
     ScopedVector<BluetoothLowEnergyDeviceInfo>* devices,
     std::string* error) {
@@ -719,5 +719,87 @@ bool BluetoothLowEnergyWrapper::EnumerateKnownBluetoothLowEnergyServices(
   return CollectBluetoothLowEnergyDeviceServices(device_path, services, error);
 }
 
+HRESULT BluetoothLowEnergyWrapper::ReadIncludedServicesOfAService(
+    base::FilePath& service_path,
+    const PBTH_LE_GATT_SERVICE service,
+    scoped_ptr<BTH_LE_GATT_SERVICE>* out_included_services,
+    USHORT* out_counts) {
+  HRESULT hr = S_OK;
+  base::File file(service_path, base::File::FLAG_OPEN | base::File::FLAG_READ);
+  if (!file.IsValid()) {
+    hr = HRESULT_FROM_WIN32(ERROR_OPEN_FAILED);
+    return hr;
+  }
+
+  USHORT required_length = 0;
+  hr = BluetoothGATTGetIncludedServices(file.GetPlatformFile(), service, 0,
+                                        NULL, &required_length,
+                                        BLUETOOTH_GATT_FLAG_NONE);
+  if (hr != HRESULT_FROM_WIN32(ERROR_MORE_DATA)) {
+    return hr;
+  }
+
+  out_included_services->reset(new BTH_LE_GATT_SERVICE[required_length]);
+  USHORT actual_length = required_length;
+  hr = BluetoothGATTGetIncludedServices(
+      file.GetPlatformFile(), service, actual_length,
+      out_included_services->get(), &required_length, BLUETOOTH_GATT_FLAG_NONE);

[scheib, 2016/02/24 01:39:10]

actual_length and required_length are in wrong argument positions.

[gogerald1, 2016/02/24 16:09:06]

Here actual_length means actual buffer size offered to OS. required_length means
OS required_length. As above (line 735), required_length used to let OS indicate
required buffer size.

[scheib, 2016/02/25 06:22:43]

When others are confused by your code and suspect a logic error it is a good
indication that the code isn't clear.  When that happens, change the code, or
add comments, so that all future readers / maintainers will understand. After
this review no one will read the review comments, so explaining only in review
comments isn't sufficient.

In this case a 'actual' and 'required' are a bit ambiguous. Required in
BluetoothGATTGetIncludedServices(,,,NULL) makes sense as the OS is informing us
what size is required to be allocated. Using the same variable again becomes
confusing, as the same name used It's used to then allocate, and that makes
sense. It doesn't make The array is constructed using required, but in this code
review response you use actual to mean the allocated size of the array.
'required' 

I suggest 'allocated_length' and 'out_counts' 

First call to BluetoothGATTGetIncludedServices(,,0,NULL,allocated_length)

new [allocated_length]

then BluetoothGATTGetIncludedServices(,, allocated_length,
out_included_services->get(), out_counts)

+  if (SUCCEEDED(hr) && required_length != actual_length) {
+    LOG(ERROR) << "Retrieved included services is not equal to expected"
+               << " actual_length " << actual_length << " required_length "
+               << required_length;
+    hr = HRESULT_FROM_WIN32(ERROR_INVALID_USER_BUFFER);
+  }
+  *out_counts = actual_length;
+
+  if (FAILED(hr)) {
+    out_included_services->reset(nullptr);
+    *out_counts = 0;
+  }
+  return hr;
+}
+
+HRESULT BluetoothLowEnergyWrapper::ReadCharacteristicsOfAService(
+    base::FilePath& service_path,
+    const PBTH_LE_GATT_SERVICE service,
+    scoped_ptr<BTH_LE_GATT_CHARACTERISTIC>* out_included_characteristics,
+    USHORT* out_counts) {
+  HRESULT hr = S_OK;
+  base::File file(service_path, base::File::FLAG_OPEN | base::File::FLAG_READ);
+  if (!file.IsValid()) {
+    hr = HRESULT_FROM_WIN32(ERROR_OPEN_FAILED);
+    return hr;
+  }
+
+  USHORT required_length = 0;
+  hr = BluetoothGATTGetCharacteristics(file.GetPlatformFile(), service, 0, NULL,
+                                       &required_length,
+                                       BLUETOOTH_GATT_FLAG_NONE);
+  if (hr != HRESULT_FROM_WIN32(ERROR_MORE_DATA)) {
+    return hr;
+  }
+
+  out_included_characteristics->reset(
+      new BTH_LE_GATT_CHARACTERISTIC[required_length]);
+  USHORT actual_length = required_length;
+  hr = BluetoothGATTGetCharacteristics(
+      file.GetPlatformFile(), service, actual_length,

[scheib, 2016/02/24 01:39:09]

ditto

[gogerald1, 2016/02/24 16:09:06]

Acknowledged.

+      out_included_characteristics->get(), &required_length,
+      BLUETOOTH_GATT_FLAG_NONE);
+  if (SUCCEEDED(hr) && required_length != actual_length) {
+    LOG(ERROR) << "Retrieved charactersitics is not equal to expected"
+               << " actual_length " << actual_length << " required_length "
+               << required_length;
+    hr = HRESULT_FROM_WIN32(ERROR_INVALID_USER_BUFFER);
+  }
+  *out_counts = actual_length;
+
+  if (FAILED(hr)) {
+    out_included_characteristics->reset(nullptr);
+    *out_counts = 0;
+  }
+  return hr;
+}
+
 }  // namespace win
 }  // namespace device