bluetooth: Refactor how we update based on Advertising Data

device/bluetooth/bluetooth_device.h

Index: device/bluetooth/bluetooth_device.h
diff --git a/device/bluetooth/bluetooth_device.h b/device/bluetooth/bluetooth_device.h
index bc346e835ae91e6842a660fbda33dd751aa432b7..5c7bf095348349390bb5351d65cd6d82ce4d1e33 100644
--- a/device/bluetooth/bluetooth_device.h
+++ b/device/bluetooth/bluetooth_device.h
@@ -11,6 +11,7 @@
 #include <memory>
 #include <set>
 #include <string>
+#include <unordered_set>
 #include <vector>
 
 #include "base/callback.h"
@@ -117,6 +118,17 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothDevice {
   };
 
   typedef std::vector<BluetoothUUID> UUIDList;
+  typedef std::unordered_set<BluetoothUUID, BluetoothUUIDHash> UUIDSet;

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

This is where I'd really like to have a vector-backed set, which could be much
cheaper to create and use. However, Bluetooth probably isn't a big enough
fraction of browser time to make it worth adding that type just for us.

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

Is that so that we would always return the uuids in the order they were added?
Wouldn't std::set work for that?

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

No, I'm thinking of the implementation guts of an unordered_set: it needs an
allocated node for each entry, and for small sets like the ones in advertising
data, a linear search can be faster and more memory efficient than a hash
lookup. We do have
https://cs.chromium.org/chromium/src/base/containers/small_map.h, but this is
one of the cases where I wouldn't want to spend the extra code size.

It's possible that using Blink's HashMap (which doesn't use separate nodes)
would beat the linear search. But anyway, it's not worth adding something to
Chrome for this purpose.

+  typedef std::unordered_map<BluetoothUUID,
+                             std::vector<uint8_t>,
+                             BluetoothUUIDHash>
+      ServiceDataMap;
+
+  // Mapping from the platform-specific GATT service identifiers to
+  // BluetoothRemoteGattService objects.
+  typedef base::ScopedPtrHashMap<std::string,

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

Use unordered_map instead:
https://cs.chromium.org/chromium/src/base/containers/scoped_ptr_hash_map.h?l=21

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

I had to fight the urge to convert this to an unordered_map, but I'm just moving
this from below because DeviceUUIDs::ReplaceServiceUUIDs takes a GattServiceMap.
I opened an issue to convert all of these to std::unordered_map:
http://crbug.com/639089

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

Oh, ok.

+                                 std::unique_ptr<BluetoothRemoteGattService>>
+      GattServiceMap;
 
   // Interface for negotiating pairing of bluetooth devices.
   class PairingDelegate {
@@ -302,7 +314,20 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothDevice {
   // Note: On ChromeOS and Linux, Bluez persists all services meaning if
   // a device stops advertising a service this function will still return
   // its UUID.
-  virtual UUIDList GetUUIDs() const;
+  virtual UUIDSet GetUUIDs() const;
+
+  // Returns the last advertised Service Data. Returns nullptr if the adapter
+  // is not discoverying.

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

sp: discoverying->discovering 

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

Done.

+  const ServiceDataMap* GetServiceData() const;
+
+  // Returns the UUIDs of services for which the device advertises Service Data.
+  // Returns an empty set if the adapter is not discoverying.

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

I think I'd rather return something consistent between GetServiceData() and
GetServiceDataUUIDs() if the adapter isn't discovering. Empty seems fine for
both.

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

An empty array means that the service just has no data but it's still in the
service data field e.g. [0x3 /* length */, 0x16 /* service data */, 0xAA, 0xFE
/* service uuid */]. Whereas a nullptr means the service uuid is not in the
service data field or we are not advertising.

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

I think that's for GetServiceDataForUUID()? I was talking about
GetServiceData(), which doesn't identify a particular service.

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

Ah right. Changed GetServiceData to return an empty map.

+  UUIDSet GetServiceDataUUIDs() const;
+
+  // Returns a pointer to the Service Data for Service with |uuid|. Returns
+  // nullptr if |uuid| has no Service Data.
+  const std::vector<uint8_t>* GetServiceDataForUUID(
+      const BluetoothUUID& uuid) const;
 
   // The received signal strength, in dBm. This field is avaliable and valid
   // only during discovery.
@@ -477,12 +502,6 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothDevice {
   virtual BluetoothRemoteGattService* GetGattService(
       const std::string& identifier) const;
 
-  // Returns service data of a service given its UUID.
-  virtual base::BinaryValue* GetServiceData(BluetoothUUID serviceUUID) const;
-
-  // Returns the list UUIDs of services that have service data.
-  virtual UUIDList GetServiceDataUUIDs() const;
-
   // Returns the |address| in the canonical format: XX:XX:XX:XX:XX:XX, where
   // each 'X' is a hex digit.  If the input |address| is invalid, returns an
   // empty string.
@@ -491,8 +510,14 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothDevice {
   // Return the timestamp for when this device was last seen.
   base::Time GetLastUpdateTime() const { return last_update_time_; }
 
-  // Update the last time this device was seen.
-  void UpdateTimestamp();
+  // Called by BluetoothAdapter when a new Advertisement is seen for this
+  // device. This replaces previously seen Advertisement Data.
+  void UpdateAdvertisementData(
+      std::vector<std::string> advertised_uuids,
+      std::unordered_map<std::string, std::vector<uint8_t>> service_data);
+
+  // Called by BluetoothAdapter when it stops discoverying.
+  void ClearAdvertisementData();
 
   // Return associated BluetoothAdapter.
   BluetoothAdapter* GetAdapter() { return adapter_; }
@@ -500,7 +525,6 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothDevice {
  protected:
   // BluetoothGattConnection is a friend to call Add/RemoveGattConnection.
   friend BluetoothGattConnection;
-  FRIEND_TEST_ALL_PREFIXES(BluetoothTest, GetUUIDs);
   FRIEND_TEST_ALL_PREFIXES(
       BluetoothTest,
       BluetoothGattConnection_DisconnectGatt_SimulateConnect);
@@ -515,8 +539,40 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothDevice {
   FRIEND_TEST_ALL_PREFIXES(BluetoothTest, RemoveOutdatedDevices);
   FRIEND_TEST_ALL_PREFIXES(BluetoothTest, RemoveOutdatedDeviceGattConnect);
 
+  // Helper class to easily update the sets of UUIDs and them in sycn with the

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

sp: sycn

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

Done. Heh, I was also missing "keep".

+  // set of all the device's UUIDs
+  class DeviceUUIDs {
+   public:
+    DeviceUUIDs();
+    ~DeviceUUIDs();
+
+    // Advertised Service UUIDs functions
+    void ReplaceAdvertisedUUIDs(std::vector<std::string> new_advertised_uuids);
+
+    void ClearAdvertisedUUIDs();
+
+    // Service UUIDs functions
+    void ReplaceServiceUUIDs(
+        const BluetoothDevice::GattServiceMap& gatt_services);
+
+    void ClearServiceUUIDs();
+
+    // Returns the union of Advertised UUIDs and Service UUIDs.
+    UUIDSet GetUUIDs() const;
+
+   private:
+    void UpdateDeviceUUIDs();
+
+    BluetoothDevice::UUIDSet advertised_uuids_;
+    BluetoothDevice::UUIDSet service_uuids_;
+    BluetoothDevice::UUIDSet device_uuids_;
+  };
+
   BluetoothDevice(BluetoothAdapter* adapter);
 
+  // Update the last time this device was seen.
+  void UpdateTimestamp();
+
   // Implements platform specific operations to initiate a GATT connection.
   // Subclasses must also call DidConnectGatt, DidFailToConnectGatt, or
   // DidDisconnectGatt immediately or asynchronously as the connection state
@@ -545,15 +601,6 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothDevice {
   void AddGattConnection(BluetoothGattConnection*);
   void RemoveGattConnection(BluetoothGattConnection*);
 
-  void UpdateServiceUUIDs();
-
-  // Clears the list of service data.
-  void ClearServiceData();
-
-  // Set the data of a given service designated by its UUID.
-  void SetServiceData(BluetoothUUID serviceUUID, const char* buffer,
-                      size_t size);
-
   // Update last_update_time_ so that the device appears as expired.
   void SetAsExpiredForTesting();
 
@@ -568,24 +615,14 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothDevice {
   // BluetoothGattConnection objects keeping the GATT connection alive.
   std::set<BluetoothGattConnection*> gatt_connections_;
 
-  // Mapping from the platform-specific GATT service identifiers to
-  // BluetoothRemoteGattService objects.
-  typedef base::ScopedPtrHashMap<std::string,
-                                 std::unique_ptr<BluetoothRemoteGattService>>
-      GattServiceMap;
   GattServiceMap gatt_services_;
   bool gatt_services_discovery_complete_;
 
-  // Last advertised service UUIDs. Should be empty if the device is connected.
-  UUIDList advertised_uuids_;
-  // UUIDs of the device's services. Should be empty if the device is not
-  // connected or it's services have not been discovered yet.
-  UUIDList service_uuids_;
+  // Class that holds the union of Advertised UUIDs and Service UUIDs.
+  DeviceUUIDs device_uuids_;
 
-  // Mapping from service UUID represented as a std::string of a bluetooth
-  // service to
-  // the specific data. The data is stored as BinaryValue.
-  std::unique_ptr<base::DictionaryValue> services_data_;
+  // Map of BluetoothUUIDs to their advertised Service Data.
+  ServiceDataMap service_data_;
 
   // Timestamp for when an advertisement was last seen.
   base::Time last_update_time_;