Bluetooth: Add service data unit tests for chromeos

device/bluetooth/dbus/fake_bluetooth_device_client.cc

Index: device/bluetooth/dbus/fake_bluetooth_device_client.cc
diff --git a/device/bluetooth/dbus/fake_bluetooth_device_client.cc b/device/bluetooth/dbus/fake_bluetooth_device_client.cc
index 58f694f03cf6e3161e261e099f74dafe446a5215..635c000b3aadec657c914e612ce9c0900aa9eaec 100644
--- a/device/bluetooth/dbus/fake_bluetooth_device_client.cc
+++ b/device/bluetooth/dbus/fake_bluetooth_device_client.cc
@@ -1551,6 +1551,20 @@ void FakeBluetoothDeviceClient::UpdateDeviceRSSI(
   properties->rssi.ReplaceValue(rssi);
 }
 
+void FakeBluetoothDeviceClient::UpdateServiceData(
+    const dbus::ObjectPath& object_path,
+    const std::unordered_map<std::string, std::vector<uint8_t>>& service_data) {
+  PropertiesMap::const_iterator iter = properties_map_.find(object_path);
+  if (iter == properties_map_.end()) {
+    VLOG(2) << "Fake device does not exist: " << object_path.value();
+    return;
+  }
+  Properties* properties = iter->second.get();
+  DCHECK(properties);
+  properties->service_data.set_valid(true);
+  properties->service_data.ReplaceValue(service_data);

[ortuno, 2017/03/30 02:58:14]

+scheib fyi

*sigh* I wish BlueZ worked like this. Sadly BlueZ caches and combines all
values. Attempts to convince BlueZ folks to change this have failed, so we are
stuck with this until the new stack. For example:

1. Receive advertisement with service data only for UUID 0x180a
2. Receive advertisement with service data only for UUID 0x180b

After the two advertisements arrive properties->service_data will contain both
values :(

Because bluetooth_test is supposed to simulate the behavior of each platform
this function should match what BlueZ does and combine the new values with the
old values.

This will cause the test to fail D: D: which is expected because the behavior in
BlueZ is different. So we need to add #if defined(OS_CHROMEOS) to the test
explaining that BlueZ behaves differently. See [1] for docs explaining how the
behavior is different and [2] for other instances of this:

[1]
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_device.h?sq=p...
[2]
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_adapter_unitt...

[scheib, 2017/03/30 03:44:10]

Grr... Bluez.

I didn't go dig into Bluez code to see where it merges. I see that our code
using BlueZ just reads out all the values BlueZ provides when a 
BluetoothAdapterBlueZ::DevicePropertyChanged is called.

The fake here should mimic what BlueZ does -- and please include a comment
pointing to the BlueZ code to blame for it.

If our platforms behave differently, our unit tests should represent that and
have the regrettable #if defined sections (as in [2] above). If we can fix it,
we should file an issue and // TODO(issue-number) to fix it.

Most ideally we'd fix BlueZ by having a way to clear the cached values. Can our
BluetoothDeviceBlueZ::UpdateServiceData clear the properties->service_data?

[Miao, 2017/03/30 21:20:07]

As Ortuno mentioned, the current BlueZ caches all previously-found service data
and UUIDs, and the merging[1] happens when device found event is received by
bluetoothd. And I also agree that we should get it fixed as a CHROMIUM specific
fix, so please fix a bug and marked as p2 and marked the related issues within
the new bug.

Out of curious, is there any existing user case where you observe issues caused
by the caching?

[1]https://cs.corp.google.com/chromeos_public/src/third_party/bluez/src/shared/ad.c?q=src/shared/ad.c+file:%5Esrc/third_party/bluez/+package:%5Echromeos_public$&dr&l=557

[ortuno, 2017/03/30 22:18:17]

We discussed a solution with puthik a while ago. Left comments on the issue.
hmm we haven't seen any use cases/problems on the Web Bluetooth side because we
don't use this yet, but Arc++ uses it. The Android API[1] requires Arc++ to send
an event for each new packet with the contents of that packet only. Because we
are caching Service Data, Arc++ could be sending UUIDs, Service Data, etc.
values that are not part of the latest packet.

As mentioned before we haven't seen problems on the Web Bluetooth side because
we don't use it yet, but we do have problems with UUIDs, which have the same
caching policy. Eddystone beacons change their advertised services after being
configured to indicate that config is done. Because BlueZ caches UUIDs these
beacons appear as if they were still configurable even after config is done.

[1]
https://developer.android.com/reference/android/bluetooth/le/ScanCallback.htm...,
android.bluetooth.le.ScanResult)

[xiaoyinh(OOO Sep 11-29), 2017/03/31 16:26:53]

Changed it to merge data like what BlueZ does. 

[xiaoyinh(OOO Sep 11-29), 2017/03/31 16:26:53]

Done.

+}
+
 void FakeBluetoothDeviceClient::UpdateConnectionInfo(
     uint16_t connection_rssi,
     uint16_t transmit_power,
@@ -1775,7 +1789,8 @@ void FakeBluetoothDeviceClient::CreateTestDevice(
     const std::string alias,
     const std::string device_address,
     const std::vector<std::string>& service_uuids,
-    device::BluetoothTransport type) {
+    device::BluetoothTransport type,
+    const std::unordered_map<std::string, std::vector<uint8_t>>& service_data) {
   // Create a random device path.
   dbus::ObjectPath device_path;
   std::string id;
@@ -1818,6 +1833,11 @@ void FakeBluetoothDeviceClient::CreateTestDevice(
   }
   properties->type.set_valid(true);
 
+  if (!service_data.empty()) {
+    properties->service_data.ReplaceValue(service_data);
+    properties->service_data.set_valid(true);
+  }
+
   properties_map_.insert(std::make_pair(device_path, std::move(properties)));
   device_list_.push_back(device_path);
   for (auto& observer : observers_)