[CrOS PhoneHub] Add BleScanner support for Nearby scan results

This CL:
(1) Updates the BleScanner::Observer interface so that scan results also
    include the associated ConnectionMedium.
(2) Changes BleConnectionManagerImpl's observer callback so that it
    ignores non-BLE scan results (which preserves its existing
    functionality).
(3) Updates BleScannerImpl to handle Nearby scan results.

Bug: 1106937
Change-Id: Idd35b4e09ec70e55bfd353841b73ecc7aba2c0a3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2401804
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: James Vecore <vecore@google.com>
Cr-Commit-Position: refs/heads/master@{#805534}

chromeos/services/secure_channel/ble_connection_manager_impl.cc

@@ -337,7 +337,12 @@ void BleConnectionManagerImpl::OnFailureToGenerateAdvertisement(
 void BleConnectionManagerImpl::OnReceivedAdvertisement(
     multidevice::RemoteDeviceRef remote_device,
     device::BluetoothDevice* bluetooth_device,
+    ConnectionMedium connection_medium,
     ConnectionRole connection_role) {
+  // Only process advertisements received as part of the BLE connection flow.
+  if (connection_medium != ConnectionMedium::kBluetoothLowEnergy)
+    return;
+
   remote_device_id_to_timestamps_map_[remote_device.GetDeviceId()]
       ->RecordAdvertisementReceived();
 

chromeos/services/secure_channel/ble_connection_manager_impl.h

@@ -134,6 +134,7 @@ class BleConnectionManagerImpl : public BleConnectionManager,
   // BleScanner::Observer:
   void OnReceivedAdvertisement(multidevice::RemoteDeviceRef remote_device,
                                device::BluetoothDevice* bluetooth_device,
+                               ConnectionMedium connection_medium,
                                ConnectionRole connection_role) override;
 
   // SecureChannel::Observer:

chromeos/services/secure_channel/ble_connection_manager_impl_unittest.cc

@@ -563,7 +563,8 @@ class SecureChannelBleConnectionManagerImplTest : public testing::Test {
         mock_bluetooth_device.get());
 
     fake_ble_scanner()->NotifyReceivedAdvertisementFromDevice(
-        remote_device, mock_bluetooth_device.get(), connection_role);
+        remote_device, mock_bluetooth_device.get(),
+        ConnectionMedium::kBluetoothLowEnergy, connection_role);
 
     // As a result of the connection, all ongoing connection attmepts should
     // have been canceled, since a connection is in progress.

chromeos/services/secure_channel/ble_scanner.cc

@@ -62,10 +62,11 @@ DeviceIdPairSet BleScanner::GetAllDeviceIdPairs() {
 void BleScanner::NotifyReceivedAdvertisementFromDevice(
     const multidevice::RemoteDeviceRef& remote_device,
     device::BluetoothDevice* bluetooth_device,
+    ConnectionMedium connection_medium,
     ConnectionRole connection_role) {
   for (auto& observer : observer_list_) {
     observer.OnReceivedAdvertisement(remote_device, bluetooth_device,
-                                     connection_role);
+                                     connection_medium, connection_role);
   }
 }
 

chromeos/services/secure_channel/ble_scanner.h

@@ -16,6 +16,7 @@
 #include "chromeos/services/secure_channel/connection_attempt_details.h"
 #include "chromeos/services/secure_channel/connection_role.h"
 #include "chromeos/services/secure_channel/device_id_pair.h"
+#include "chromeos/services/secure_channel/public/cpp/shared/connection_medium.h"
 
 namespace device {
 class BluetoothDevice;
@@ -36,6 +37,7 @@ class BleScanner {
     virtual void OnReceivedAdvertisement(
         multidevice::RemoteDeviceRef remote_device,
         device::BluetoothDevice* bluetooth_device,
+        ConnectionMedium connection_medium,
         ConnectionRole connection_role) = 0;
   };
 
@@ -70,6 +72,7 @@ class BleScanner {
   void NotifyReceivedAdvertisementFromDevice(
       const multidevice::RemoteDeviceRef& remote_device,
       device::BluetoothDevice* bluetooth_device,
+      ConnectionMedium connection_medium,
       ConnectionRole connection_role);
 
  private:

chromeos/services/secure_channel/ble_scanner_impl.cc

@@ -219,27 +219,41 @@ void BleScannerImpl::HandlePotentialScanResult(
     const std::string& service_data,
     const BleServiceDataHelper::DeviceWithBackgroundBool& potential_result,
     device::BluetoothDevice* bluetooth_device) {
-  // Background advertisements correspond to the listener role; foreground
-  // advertisements correspond to the initiator role.
-  ConnectionRole connection_role = potential_result.second
-                                       ? ConnectionRole::kListenerRole
-                                       : ConnectionRole::kInitiatorRole;
+  std::vector<std::pair<ConnectionMedium, ConnectionRole>> results;
 
   // Check to see if a corresponding scan request exists. At this point, it is
   // possible that a scan result was received for the correct DeviceIdPair but
-  // incorrect ConnectionRole.
-  bool does_corresponding_scan_request_exist = false;
+  // incorrect ConnectionMedium and/or ConnectionRole.
   for (const auto& scan_request : scan_requests()) {
     if (scan_request.remote_device_id() !=
         potential_result.first.GetDeviceId()) {
       continue;
     }
 
-    // TODO(khorimoto): Also handle Nearby cases; currently, it is assumed that
-    // only BLE scan results are handled.
-    if (scan_request.connection_role() == connection_role) {
-      does_corresponding_scan_request_exist = true;
-      break;
+    switch (scan_request.connection_medium()) {
+      // BLE scans for background advertisements in the listener role and for
+      // foreground advertisements in the initiator role.
+      case ConnectionMedium::kBluetoothLowEnergy:
+        if (potential_result.second &&
+            scan_request.connection_role() == ConnectionRole::kListenerRole) {
+          results.emplace_back(ConnectionMedium::kBluetoothLowEnergy,
+                               ConnectionRole::kListenerRole);
+        } else if (!potential_result.second &&
+                   scan_request.connection_role() ==
+                       ConnectionRole::kInitiatorRole) {
+          results.emplace_back(ConnectionMedium::kBluetoothLowEnergy,
+                               ConnectionRole::kInitiatorRole);
+        }
+        break;
+
+      // Nearby Connections scans for background advertisements in the initiator
+      // role and does not have support for the listener role.
+      case ConnectionMedium::kNearbyConnections:
+        DCHECK_EQ(ConnectionRole::kInitiatorRole,
+                  scan_request.connection_role());
+        results.emplace_back(ConnectionMedium::kNearbyConnections,
+                             ConnectionRole::kInitiatorRole);
+        break;
     }
   }
 
@@ -249,7 +263,7 @@ void BleScannerImpl::HandlePotentialScanResult(
   for (const auto& character : service_data)
     ss << static_cast<uint32_t>(character);
 
-  if (!does_corresponding_scan_request_exist) {
+  if (results.empty()) {
     PA_LOG(WARNING) << "BleScannerImpl::HandleDeviceUpdated(): Received scan "
                     << "result from device with ID \""
                     << potential_result.first.GetTruncatedDeviceIdForLogs()
@@ -267,8 +281,10 @@ void BleScannerImpl::HandlePotentialScanResult(
                << ", Background advertisement: "
                << (potential_result.second ? "true" : "false");
 
-  NotifyReceivedAdvertisementFromDevice(potential_result.first,
-                                        bluetooth_device, connection_role);
+  for (const std::pair<ConnectionMedium, ConnectionRole>& result : results) {
+    NotifyReceivedAdvertisementFromDevice(
+        potential_result.first, bluetooth_device, result.first, result.second);
+  }
 }
 
 void BleScannerImpl::SetServiceDataProviderForTesting(

chromeos/services/secure_channel/ble_scanner_impl_unittest.cc

@@ -56,7 +56,13 @@ class FakeBluetoothDevice : public device::MockBluetoothDevice {
   DISALLOW_COPY_AND_ASSIGN(FakeBluetoothDevice);
 };
 
-// const size_t kMinNumBytesInServiceData = 2;
+std::vector<std::pair<ConnectionMedium, ConnectionRole>>
+CreateSingleBleScanResult(bool is_background_advertisement) {
+  return std::vector<std::pair<ConnectionMedium, ConnectionRole>>{
+      {ConnectionMedium::kBluetoothLowEnergy,
+       is_background_advertisement ? ConnectionRole::kListenerRole
+                                   : ConnectionRole::kInitiatorRole}};
+}
 
 }  // namespace
 
@@ -154,7 +160,7 @@ class SecureChannelBleScannerImplTest : public testing::Test {
 
   void ProcessScanResultAndVerifyNoDeviceIdentified(
       const std::string& service_data) {
-    const FakeBleScannerObserver::ScannedResultList& results =
+    const std::vector<FakeBleScannerObserver::Result>& results =
         fake_delegate_->handled_scan_results();
 
     size_t num_results_before_call = results.size();
@@ -162,11 +168,22 @@ class SecureChannelBleScannerImplTest : public testing::Test {
     EXPECT_EQ(num_results_before_call, results.size());
   }
 
+  // |expected_scan_results| contains the data expected to be provided to
+  // scan observers; if null, we default to a single BLE scan result.
   void ProcessScanResultAndVerifyDevice(
       const std::string& service_data,
       multidevice::RemoteDeviceRef expected_remote_device,
-      bool is_background_advertisement) {
-    const FakeBleScannerObserver::ScannedResultList& results =
+      bool is_background_advertisement,
+      const base::Optional<
+          std::vector<std::pair<ConnectionMedium, ConnectionRole>>>&
+          expected_scan_results = base::nullopt) {
+    std::vector<std::pair<ConnectionMedium, ConnectionRole>>
+        new_expected_results =
+            expected_scan_results.has_value()
+                ? *expected_scan_results
+                : CreateSingleBleScanResult(is_background_advertisement);
+
+    const std::vector<FakeBleScannerObserver::Result>& results =
         fake_delegate_->handled_scan_results();
 
     fake_ble_service_data_helper_->SetIdentifiedDevice(
@@ -175,13 +192,17 @@ class SecureChannelBleScannerImplTest : public testing::Test {
     size_t num_results_before_call = results.size();
     std::unique_ptr<FakeBluetoothDevice> fake_bluetooth_device =
         SimulateScanResult(service_data);
-    EXPECT_EQ(num_results_before_call + 1u, results.size());
-
-    EXPECT_EQ(expected_remote_device, std::get<0>(results.back()));
-    EXPECT_EQ(fake_bluetooth_device.get(), std::get<1>(results.back()));
-    EXPECT_EQ(is_background_advertisement ? ConnectionRole::kListenerRole
-                                          : ConnectionRole::kInitiatorRole,
-              std::get<2>(results.back()));
+    EXPECT_EQ(num_results_before_call + new_expected_results.size(),
+              results.size());
+
+    for (size_t i = 0; i < new_expected_results.size(); ++i) {
+      const auto& result =
+          results[results.size() - new_expected_results.size() + i];
+      EXPECT_EQ(expected_remote_device, result.remote_device);
+      EXPECT_EQ(fake_bluetooth_device.get(), result.bluetooth_device);
+      EXPECT_EQ(new_expected_results[i].first, result.connection_medium);
+      EXPECT_EQ(new_expected_results[i].second, result.connection_role);
+    }
   }
 
   void InvokeStartDiscoveryCallback(bool success, size_t command_index) {
@@ -320,6 +341,37 @@ TEST_F(SecureChannelBleScannerImplTest, IdentifyDevice_Background) {
   EXPECT_FALSE(discovery_session_is_active());
 }
 
+TEST_F(SecureChannelBleScannerImplTest, IdentifyDevice_BleAndNearby) {
+  ConnectionAttemptDetails ble_filter(
+      DeviceIdPair(test_devices()[0].GetDeviceId(),
+                   test_devices()[1].GetDeviceId()),
+      ConnectionMedium::kBluetoothLowEnergy, ConnectionRole::kListenerRole);
+  ConnectionAttemptDetails nearby_filter(
+      DeviceIdPair(test_devices()[0].GetDeviceId(),
+                   test_devices()[1].GetDeviceId()),
+      ConnectionMedium::kNearbyConnections, ConnectionRole::kInitiatorRole);
+
+  AddScanRequest(ble_filter);
+  InvokeStartDiscoveryCallback(true /* success */, 0u /* command_index */);
+  EXPECT_TRUE(discovery_session_is_active());
+
+  AddScanRequest(nearby_filter);
+  EXPECT_TRUE(discovery_session_is_active());
+
+  std::vector<std::pair<ConnectionMedium, ConnectionRole>> expected_results{
+      {ConnectionMedium::kBluetoothLowEnergy, ConnectionRole::kListenerRole},
+      {ConnectionMedium::kNearbyConnections, ConnectionRole::kInitiatorRole}};
+
+  ProcessScanResultAndVerifyDevice("device0ServiceData", test_devices()[0],
+                                   true /* is_background_advertisement */,
+                                   expected_results);
+
+  RemoveScanRequest(ble_filter);
+  RemoveScanRequest(nearby_filter);
+  InvokeStopDiscoveryCallback(true /* success */, 1u /* command_index */);
+  EXPECT_FALSE(discovery_session_is_active());
+}
+
 TEST_F(SecureChannelBleScannerImplTest, IdentifyDevice_Foreground) {
   ConnectionAttemptDetails filter(DeviceIdPair(test_devices()[0].GetDeviceId(),
                                                test_devices()[1].GetDeviceId()),

chromeos/services/secure_channel/fake_ble_scanner.cc

@@ -27,6 +27,18 @@ void FakeBleScanner::HandleScanRequestChange() {
   ++num_scan_request_changes_handled_;
 }
 
+FakeBleScannerObserver::Result::Result(
+    multidevice::RemoteDeviceRef remote_device,
+    device::BluetoothDevice* bluetooth_device,
+    ConnectionMedium connection_medium,
+    ConnectionRole connection_role)
+    : remote_device(remote_device),
+      bluetooth_device(bluetooth_device),
+      connection_medium(connection_medium),
+      connection_role(connection_role) {}
+
+FakeBleScannerObserver::Result::~Result() = default;
+
 FakeBleScannerObserver::FakeBleScannerObserver() = default;
 
 FakeBleScannerObserver::~FakeBleScannerObserver() = default;
@@ -34,9 +46,10 @@ FakeBleScannerObserver::~FakeBleScannerObserver() = default;
 void FakeBleScannerObserver::OnReceivedAdvertisement(
     multidevice::RemoteDeviceRef remote_device,
     device::BluetoothDevice* bluetooth_device,
+    ConnectionMedium connection_medium,
     ConnectionRole connection_role) {
-  handled_scan_results_.push_back(
-      std::make_tuple(remote_device, bluetooth_device, connection_role));
+  handled_scan_results_.emplace_back(remote_device, bluetooth_device,
+                                     connection_medium, connection_role);
 }
 
 }  // namespace secure_channel

chromeos/services/secure_channel/fake_ble_scanner.h

@@ -5,7 +5,6 @@
 #ifndef CHROMEOS_SERVICES_SECURE_CHANNEL_FAKE_BLE_SCANNER_H_
 #define CHROMEOS_SERVICES_SECURE_CHANNEL_FAKE_BLE_SCANNER_H_
 
-#include <tuple>
 #include <vector>
 
 #include "base/macros.h"
@@ -44,23 +43,33 @@ class FakeBleScanner : public BleScanner {
 // Test BleScanner::Observer implementation.
 class FakeBleScannerObserver : public BleScanner::Observer {
  public:
+  struct Result {
+    Result(multidevice::RemoteDeviceRef remote_device,
+           device::BluetoothDevice* bluetooth_device,
+           ConnectionMedium connection_medium,
+           ConnectionRole connection_role);
+    ~Result();
+
+    multidevice::RemoteDeviceRef remote_device;
+    device::BluetoothDevice* bluetooth_device;
+    ConnectionMedium connection_medium;
+    ConnectionRole connection_role;
+  };
+
   FakeBleScannerObserver();
   ~FakeBleScannerObserver() override;
 
-  using ScannedResultList = std::vector<std::tuple<multidevice::RemoteDeviceRef,
-                                                   device::BluetoothDevice*,
-                                                   ConnectionRole>>;
-
-  const ScannedResultList& handled_scan_results() const {
+  const std::vector<Result>& handled_scan_results() const {
     return handled_scan_results_;
   }
 
  private:
   void OnReceivedAdvertisement(multidevice::RemoteDeviceRef remote_device,
                                device::BluetoothDevice* bluetooth_device,
+                               ConnectionMedium connection_medium,
                                ConnectionRole connection_role) override;
 
-  ScannedResultList handled_scan_results_;
+  std::vector<Result> handled_scan_results_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeBleScannerObserver);
 };