Chrome OS uWeave Characteristics Server

components/proximity_auth/ble/bluetooth_low_energy_advertisement_rotator.h

Index: components/proximity_auth/ble/bluetooth_low_energy_advertisement_rotator.h
diff --git a/components/proximity_auth/ble/bluetooth_low_energy_advertisement_rotator.h b/components/proximity_auth/ble/bluetooth_low_energy_advertisement_rotator.h
new file mode 100644
index 0000000000000000000000000000000000000000..332d3dc74aea547ecefcc959eddf4c0d174f46ec
--- /dev/null
+++ b/components/proximity_auth/ble/bluetooth_low_energy_advertisement_rotator.h
@@ -0,0 +1,97 @@
+// Copyright 2016 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 COMPONENTS_PROXIMITY_AUTH_BLE_BLUETOOTH_LOW_ENERGY_WEAVE_ADVERTISEMENT_ROTATOR_H_
+#define COMPONENTS_PROXIMITY_AUTH_BLE_BLUETOOTH_LOW_ENERGY_WEAVE_ADVERTISEMENT_ROTATOR_H_
+
+#include <stddef.h>
+#include <stdint.h>
+
+#include <map>
+#include <memory>
+#include <vector>
+
+#include "components/proximity_auth/ble/bluetooth_low_energy_eid_generator.h"
+#include "device/bluetooth/bluetooth_advertisement.h"
+
+namespace proximity_auth {
+namespace weave {
+namespace {
+
+const device::BluetoothAdvertisement::AdvertisementType kPeripheralAdType =

[rkc, 2016/07/27 00:56:33]

Why is this in an anonymous namespace in a header file?

[jingxuy, 2016/07/27 23:31:48]

Done.

+    device::BluetoothAdvertisement::AdvertisementType::
+        ADVERTISEMENT_TYPE_PERIPHERAL;
+
+}  // namespace
+
+class BluetoothLowEnergyAdvertisementRotator {

[rkc, 2016/07/27 00:56:34]

Class comment? What does this class do?

[Kyle Horimoto, 2016/07/27 17:58:48]

There should just be one EidGenerator used to generate all EIDs, but this
implementation stores a map of string to EidGenerator.

This class should just return the device that should be advertised to next
instead of attempting to generate an EID for that device.

[jingxuy, 2016/07/27 23:31:47]

If there is going to be different seed for different devices, why should there
just be one eid generator.

We had a discussion about the DeviceRotator when you left that meeting. And
Gustavo doesn't consider a queue itself is worthy of a separate class. He'd
agree to a separate class if it also rotate EID on top just device and suggested
naming the class AdvertisementRotator.

[jingxuy, 2016/07/27 23:31:48]

Done.

+ public:
+  class Factory {
+   public:
+    static std::unique_ptr<BluetoothLowEnergyAdvertisementRotator> NewInstance(
+        std::string service_uuid);
+
+    // Exposed for testing.
+    static void SetInstanceForTesting(Factory* factory);

[rkc, 2016/07/27 00:56:34]

Is there a better way to do this. We've moved away from the entire
"SetXXXForTesting" paradigm. Usually code should be designed in a way that we
can create stubs instead.

[Kyle Horimoto, 2016/07/27 06:34:53]

Rahul, I actually suggested this approach to Jing for some other classes.
However, I don't think that a Factory is needed at all for this class. It can
just be injected into the constructor and does not need to be created via a
Factory at all.

[jingxuy, 2016/07/27 23:31:48]

I would be open to other ways of doing this because I do find factories to be
awkward to use. Are we all agreeing that passing the rotator as constructor
parameter into the server class is the approach to solve this problem?

+
+   protected:
+    // Exposed for testing.
+    virtual std::unique_ptr<BluetoothLowEnergyAdvertisementRotator>
+    BuildInstance(std::string service_uuid);
+
+   private:
+    static Factory* factory_instance_;
+  };
+
+  typedef device::BluetoothAdvertisement::Data Advertisement;
+
+  ~BluetoothLowEnergyAdvertisementRotator();
+
+  // Return whether the rotator is empty.

[rkc, 2016/07/27 00:56:34]

Unnecessary comment.

[jingxuy, 2016/07/27 23:31:48]

Done.

+  bool IsRotatorEmpty();
+
+  // Get the next device in the rotator and it's corresponding advertisement.
+  // The rotator must not be empty.
+  std::pair<std::string, std::unique_ptr<Advertisement>> GetNextAdvertisement();
+
+  // Return a refreshed version of advertisement for the given device.
+  // Device must be already added to the rotator.
+  std::unique_ptr<Advertisement> GetAddedDeviceAdvertisement(
+      std::string bluetooth_address);
+
+  // Move to the next advertisement.
+  void RotateAdvertisement();
+
+  // Adds a device to the rotator.
+  // Device must not exist already in the rotator.
+  void AddDevice(const RemoteDevice& device);
+
+  // Remove the device from the rotator.
+  // Device must exist in the rotator.
+  void RemoveDevice(const RemoteDevice& device);
+
+  void CutAdvertisementLine(std::string bluetooth_address);
+
+  bool HasDeviceWithAddress(std::string bluetooth_address);
+
+ protected:
+  BluetoothLowEnergyAdvertisementRotator(std::string service_uuid);

[rkc, 2016/07/27 00:56:33]

Why is this protected? Why inherits this class?

[jingxuy, 2016/07/27 23:31:48]

This the part of the implementation for the factory thing. I'll move it when we
reach the consensus on how to solve the testing problem.

But I guess it can be private either way.

+
+ private:
+  int GetPosWithAddress(std::string bluetooth_address);
+
+  const std::string service_uuid_;
+
+  std::map<std::string, std::unique_ptr<BluetoothLowEnergyEidGenerator>>
+      map_to_eid_generator_;
+
+  std::vector<int> waiting_queue_;

[rkc, 2016/07/27 00:56:34]

What do these variables hold?

[jingxuy, 2016/07/27 23:31:48]

Done.

+  std::vector<RemoteDevice> devices_;
+};
+
+}  // namespace weave
+
+}  // namespace proximity_auth
+
+#endif  // COMPONENTS_PROXIMITY_AUTH_BLE_BLUETOOTH_LOW_ENERGY_WEAVE_ADVERTISEMENT_ROTATOR_H_