bluetooth: Refactor how we update based on Advertising Data

device/bluetooth/bluetooth_device.h

 // Copyright (c) 2012 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.
 
 #ifndef DEVICE_BLUETOOTH_BLUETOOTH_DEVICE_H_
 #define DEVICE_BLUETOOTH_BLUETOOTH_DEVICE_H_
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <memory>
 #include <set>
 #include <string>
+#include <unordered_set>
 #include <vector>
 
 #include "base/callback.h"
 #include "base/containers/scoped_ptr_hash_map.h"
 #include "base/gtest_prod_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/optional.h"
 #include "base/strings/string16.h"
 #include "base/time/time.h"
 #include "device/bluetooth/bluetooth_common.h"
@@ skipping 86 matching lines @@
     ERROR_OFFSET_INVALID,
     ERROR_READ_NOT_PERMITTED,
     ERROR_REQUEST_NOT_SUPPORTED,
     ERROR_UNKNOWN,
     ERROR_UNSUPPORTED_DEVICE,
     ERROR_WRITE_NOT_PERMITTED,
     NUM_CONNECT_ERROR_CODES  // Keep as last enum.
   };
 
   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 {
    public:
     virtual ~PairingDelegate() {}
 
     // This method will be called when the Bluetooth daemon requires a
     // PIN Code for authentication of the device |device|, the delegate should
     // obtain the code from the user and call SetPinCode() on the device to
     // provide it, or RejectPairing() or CancelPairing() to reject or cancel
@@ skipping 165 matching lines @@
   //    UUIDs.
   //  * For connected Low Energy Devices for which services have not been
   //    discovered returns an empty list.
   //  * For connected Low Energy Devices for which services have been discovered
   //    returns the UUIDs of the device's services.
   //  * For dual mode devices this may be collected from both.
   //
   // 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.
   virtual base::Optional<int8_t> GetInquiryRSSI() const = 0;
 
   // The transmitted power level. This field is avaliable only for LE devices
   // that include this field in AD. It is avaliable and valid only during
   // discovery.
   virtual base::Optional<int8_t> GetInquiryTxPower() const = 0;
 
@@ skipping 154 matching lines @@
   virtual bool IsGattServicesDiscoveryComplete() const;
 
   // Returns the list of discovered GATT services.
   virtual std::vector<BluetoothRemoteGattService*> GetGattServices() const;
 
   // Returns the GATT service with device-specific identifier |identifier|.
   // Returns NULL, if no such service exists.
   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.
   static std::string CanonicalizeAddress(const std::string& address);
 
   // 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_; }
 
  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);
   FRIEND_TEST_ALL_PREFIXES(
       BluetoothTest,
       BluetoothGattConnection_DisconnectGatt_SimulateDisconnect);
   FRIEND_TEST_ALL_PREFIXES(BluetoothTest,
                            BluetoothGattConnection_ErrorAfterConnection);
   FRIEND_TEST_ALL_PREFIXES(BluetoothTest,
                            BluetoothGattConnection_DisconnectGatt_Cleanup);
   FRIEND_TEST_ALL_PREFIXES(BluetoothTest, GetName_NullName);
   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
   // changes.
   virtual void CreateGattConnectionImpl() = 0;
 
   // Disconnects GATT connection on platforms that maintain a specific GATT
   // connection.
   virtual void DisconnectGatt() = 0;
 
   // Calls any pending callbacks for CreateGattConnection based on result of
   // subclasses actions initiated in CreateGattConnectionImpl or related
   // disconnection events. These may be called at any time, even multiple times,
   // to ensure a change in platform state is correctly tracked.
   //
   // Under normal behavior it is expected that after CreateGattConnectionImpl
   // an platform will call DidConnectGatt or DidFailToConnectGatt, but not
   // DidDisconnectGatt.
   void DidConnectGatt();
   void DidFailToConnectGatt(ConnectErrorCode);
   void DidDisconnectGatt();
 
   // Tracks BluetoothGattConnection instances that act as a reference count
   // keeping the GATT connection open. Instances call Add/RemoveGattConnection
   // at creation & deletion.
   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();
 
   // Raw pointer to adapter owning this device object. Subclasses use platform
   // specific pointers via adapter_.
   BluetoothAdapter* adapter_;
 
   // Callbacks for pending success and error result of CreateGattConnection.
   std::vector<GattConnectionCallback> create_gatt_connection_success_callbacks_;
   std::vector<ConnectErrorCallback> create_gatt_connection_error_callbacks_;
 
   // 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_;
 
  private:
   // Returns a localized string containing the device's bluetooth address and
   // a device type for display when |name_| is empty.
   base::string16 GetAddressWithLocalizedDeviceTypeName() const;
 };
 
 }  // namespace device
 
 #endif  // DEVICE_BLUETOOTH_BLUETOOTH_DEVICE_H_