Implement BluetoothRemoteGattServiceWin and related unit tests

device/bluetooth/bluetooth_low_energy_win.cc

 // Copyright 2014 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_low_energy_win.h"
 
 #include "base/files/file.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/strings/sys_string_conversions.h"
@@ skipping 632 matching lines @@
 }
 
 BluetoothLowEnergyDeviceInfo::BluetoothLowEnergyDeviceInfo()
     : visible(false), authenticated(false), connected(false) {
   address.ullLong = BLUETOOTH_NULL_ADDRESS;
 }
 
 BluetoothLowEnergyDeviceInfo::~BluetoothLowEnergyDeviceInfo() {
 }
 
-bool IsBluetoothLowEnergySupported() {
-  return base::win::GetVersion() >= base::win::VERSION_WIN8;
-}
-
 bool ExtractBluetoothAddressFromDeviceInstanceIdForTesting(
     const std::string& instance_id,
     BLUETOOTH_ADDRESS* btha,
     std::string* error) {
   return ExtractBluetoothAddressFromDeviceInstanceId(instance_id, btha, error);
 }
 
 BluetoothLowEnergyWrapper* BluetoothLowEnergyWrapper::GetInstance() {
   if (g_instance_ == nullptr) {
     g_instance_ = new BluetoothLowEnergyWrapper();
   }
   return g_instance_;
 }
 
 void BluetoothLowEnergyWrapper::DeleteInstance() {
   delete g_instance_;
   g_instance_ = nullptr;
 }
 
 void BluetoothLowEnergyWrapper::SetInstanceForTest(
     BluetoothLowEnergyWrapper* instance) {
   delete g_instance_;
   g_instance_ = instance;
 }
 
 BluetoothLowEnergyWrapper::BluetoothLowEnergyWrapper() {}
 BluetoothLowEnergyWrapper::~BluetoothLowEnergyWrapper() {}
 
+bool BluetoothLowEnergyWrapper::IsBluetoothLowEnergySupported() {
+  return base::win::GetVersion() >= base::win::VERSION_WIN8;
+}
+
 bool BluetoothLowEnergyWrapper::EnumerateKnownBluetoothLowEnergyDevices(
     ScopedVector<BluetoothLowEnergyDeviceInfo>* devices,
     std::string* error) {
   if (!IsBluetoothLowEnergySupported()) {
     *error = kPlatformNotSupported;
     return false;
   }
 
   return EnumerateKnownBLEOrBLEGattServiceDevices(
       GUID_BLUETOOTHLE_DEVICE_INTERFACE, devices, error);
@@ skipping 17 matching lines @@
     ScopedVector<BluetoothLowEnergyServiceInfo>* services,
     std::string* error) {
   if (!IsBluetoothLowEnergySupported()) {
     *error = kPlatformNotSupported;
     return false;
   }
 
   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