bluetooth: Refactor how we update based on Advertising Data

device/bluetooth/bluetooth_device.cc

Index: device/bluetooth/bluetooth_device.cc
diff --git a/device/bluetooth/bluetooth_device.cc b/device/bluetooth/bluetooth_device.cc
index 6f61faec6904e0c89360b80cf2895f79bbc6368a..b448d011dc65cf2b0d74646e5ff8d136d5d286a2 100644
--- a/device/bluetooth/bluetooth_device.cc
+++ b/device/bluetooth/bluetooth_device.cc
@@ -4,6 +4,7 @@
 
 #include "device/bluetooth/bluetooth_device.h"
 
+#include <iterator>
 #include <memory>
 #include <string>
 
@@ -21,10 +22,52 @@
 
 namespace device {
 
+BluetoothDevice::DeviceUUIDs::DeviceUUIDs() {}
+
+BluetoothDevice::DeviceUUIDs::~DeviceUUIDs() {}
+
+void BluetoothDevice::DeviceUUIDs::ReplaceAdvertisedUUIDs(
+    std::vector<std::string> new_advertised_uuids) {
+  advertised_uuids_.clear();
+  for (auto& it : new_advertised_uuids) {
+    advertised_uuids_.insert(BluetoothUUID(std::move(it)));
+  }
+  UpdateDeviceUUIDs();
+}
+
+void BluetoothDevice::DeviceUUIDs::ClearAdvertisedUUIDs() {
+  advertised_uuids_.clear();
+  UpdateDeviceUUIDs();
+}
+
+void BluetoothDevice::DeviceUUIDs::ReplaceServiceUUIDs(
+    const GattServiceMap& gatt_services) {
+  service_uuids_.clear();
+  for (const auto& gatt_service_pair : gatt_services) {
+    service_uuids_.insert(gatt_service_pair.second->GetUUID());
+  }
+  UpdateDeviceUUIDs();
+}
+
+void BluetoothDevice::DeviceUUIDs::ClearServiceUUIDs() {
+  service_uuids_.clear();
+  UpdateDeviceUUIDs();
+}
+
+BluetoothDevice::UUIDSet BluetoothDevice::DeviceUUIDs::GetUUIDs() const {

[Jeffrey Yasskin, 2016/08/18 19:56:30]

Since you're returning an object that keeps living after the call, consider
returning a const BluetoothDevice::UUIDSet&.

[ortuno, 2016/08/19 20:50:33]

Done.

+  return device_uuids_;
+}
+
+void BluetoothDevice::DeviceUUIDs::UpdateDeviceUUIDs() {
+  device_uuids_.clear();
+  std::set_union(advertised_uuids_.begin(), advertised_uuids_.end(),
+                 service_uuids_.begin(), service_uuids_.end(),
+                 std::inserter(device_uuids_, device_uuids_.begin()));
+}
+
 BluetoothDevice::BluetoothDevice(BluetoothAdapter* adapter)
     : adapter_(adapter),
       gatt_services_discovery_complete_(false),
-      services_data_(new base::DictionaryValue()),
       last_update_time_(base::Time()) {}
 
 BluetoothDevice::~BluetoothDevice() {
@@ -243,20 +286,29 @@ bool BluetoothDevice::IsTrustable() const {
   return false;
 }
 
-BluetoothDevice::UUIDList BluetoothDevice::GetUUIDs() const {
-  if (!IsGattConnected()) {
-    DCHECK(service_uuids_.empty());
-    return advertised_uuids_;
-  }
+BluetoothDevice::UUIDSet BluetoothDevice::GetUUIDs() const {

[Jeffrey Yasskin, 2016/08/19 15:09:53]

If device_uuids_.GetUUIDs() starts returning a reference, this should too.

[ortuno, 2016/08/19 20:50:33]

Both BluetoothClassicDeviceMac and BluetoothDeviceBluez don't hold the set of
uuids themselves but rather just construct them when GetUUIDs is called. We
could refactor BluetoothDeviceBlueZ to hold to the uuids and update them when
they changed but there is no indication that the UUIDs changed for classic mac
devices :( So we either keep returning a copy or we remove the const qualifier.
Which one do you think it's better?

[Jeffrey Yasskin, 2016/08/19 22:08:09]

Returning a copy sounds fine, if that's what Mac and Bluez will need to do
anyway.

+  return device_uuids_.GetUUIDs();
+}
 
-  if (IsGattServicesDiscoveryComplete()) {
-    DCHECK(advertised_uuids_.empty());
-    return service_uuids_;
+const BluetoothDevice::ServiceDataMap* BluetoothDevice::GetServiceData() const {
+  return &service_data_;

[Jeffrey Yasskin, 2016/08/19 15:09:53]

The interface says this'll be null when not scanning, but it's actually empty.
That seems fine, but then you can return a reference.

[ortuno, 2016/08/19 20:50:33]

The interface says this will be empty :) GetServiceDataForUUID will be nullptr.

[Jeffrey Yasskin, 2016/08/19 22:08:09]

From
https://codereview.chromium.org/2244693002/diff/290001/device/bluetooth/bluet...

// Returns the last advertised Service Data. Returns nullptr if the adapter
// is not discovering.
const ServiceDataMap* GetServiceData() const;

+}
+
+BluetoothDevice::UUIDSet BluetoothDevice::GetServiceDataUUIDs() const {
+  UUIDSet service_data_uuids;
+  for (const auto& uuid_service_data_pair : service_data_) {
+    service_data_uuids.insert(uuid_service_data_pair.first);
   }
+  return service_data_uuids;
+}
 
-  DCHECK(service_uuids_.empty());
-  DCHECK(advertised_uuids_.empty());
-  return BluetoothDevice::UUIDList();
+const std::vector<uint8_t>* BluetoothDevice::GetServiceDataForUUID(
+    const BluetoothUUID& uuid) const {
+  auto it = service_data_.find(uuid);
+  if (it != service_data_.end()) {
+    return &it->second;
+  }
+  return nullptr;
 }
 
 void BluetoothDevice::CreateGattConnection(
@@ -329,27 +381,25 @@ std::string BluetoothDevice::CanonicalizeAddress(const std::string& address) {
 
 std::string BluetoothDevice::GetIdentifier() const { return GetAddress(); }
 
-base::BinaryValue* BluetoothDevice::GetServiceData(
-    BluetoothUUID serviceUUID) const {
-  base::BinaryValue* value;
-  if (!services_data_->GetBinary(serviceUUID.value(), &value))
-    return NULL;
-  return value;
-}
-
-BluetoothDevice::UUIDList BluetoothDevice::GetServiceDataUUIDs() const {
-  std::vector<device::BluetoothUUID> uuids;
-  base::DictionaryValue::Iterator iter(*services_data_);
-  while (!iter.IsAtEnd()) {
-    BluetoothUUID uuid(iter.key());
-    uuids.push_back(uuid);
-    iter.Advance();
+void BluetoothDevice::UpdateAdvertisementData(
+    std::vector<std::string> advertised_uuids,
+    std::unordered_map<std::string, std::vector<uint8_t>> service_data) {
+  UpdateTimestamp();
+  device_uuids_.ReplaceAdvertisedUUIDs(std::move(advertised_uuids));
+
+  service_data_.clear();
+  for (auto& it : service_data) {
+    service_data_.insert({BluetoothUUID(it.first), std::move(it.second)});

[Jeffrey Yasskin, 2016/08/19 15:09:53]

Maybe this should just take an unordered_map of the right type, and move-assign
it?

[ortuno, 2016/08/19 20:50:33]

Hmm I wanted to have UpdateAdvertisementData take only strings so that each
function that parsed the data didn't have to create the BluetoothUUIDs
themselves. But I agree that moving the map of BluetoothUUIDs would be better,
so I changed the function to take BluetoothUUIDs rather than strings. That said
I'm sad that I have to convert all the strings to BluetoothUUIDs in each
platform :(

[Jeffrey Yasskin, 2016/08/19 22:08:09]

I think I'm relatively happy about that: it does force you to duplicate a bit of
code, but it establishes type safety earlier. One purpose of the platform layer
is to convert from untyped OS interfaces to typed Chrome interfaces, so it seems
like a reasonable place to do this conversion.

[ortuno, 2016/08/23 00:38:04]

That makes sense.

   }
-  return uuids;
+}
+
+void BluetoothDevice::ClearAdvertisementData() {
+  device_uuids_.ClearAdvertisedUUIDs();
+  service_data_.clear();
+  GetAdapter()->NotifyDeviceChanged(this);
 }
 
 void BluetoothDevice::DidConnectGatt() {
-  advertised_uuids_.clear();
   for (const auto& callback : create_gatt_connection_success_callbacks_) {
     callback.Run(
         base::WrapUnique(new BluetoothGattConnection(adapter_, GetAddress())));
@@ -396,22 +446,6 @@ void BluetoothDevice::RemoveGattConnection(
     DisconnectGatt();
 }
 
-void BluetoothDevice::UpdateServiceUUIDs() {
-  std::unordered_set<BluetoothUUID, BluetoothUUIDHash> uuid_set;
-  for (const auto& gatt_service_pair : gatt_services_) {
-    uuid_set.insert(gatt_service_pair.second->GetUUID());
-  }
-  service_uuids_ = UUIDList(uuid_set.begin(), uuid_set.end());
-}
-
-void BluetoothDevice::ClearServiceData() { services_data_->Clear(); }
-
-void BluetoothDevice::SetServiceData(BluetoothUUID serviceUUID,
-                                     const char* buffer, size_t size) {
-  services_data_->Set(serviceUUID.value(),
-                      base::BinaryValue::CreateWithCopiedBuffer(buffer, size));
-}
-
 void BluetoothDevice::SetAsExpiredForTesting() {
   last_update_time_ =
       base::Time::NowFromSystemTime() -