[Bluetooth] For dual mode devices, filter out appearance UNKNOWN in UI.

Several popular headphones (e.g., Bose QuietComforts, Sennheiser 550s,
Sony WH-1000XM3s, etc.), as well as other peripherals like printers,
send out BLE advertisements while on but not pairable. The current
Chrome OS behavior of displaying these advertisements is confusing to
users (because they try to pair them and can't) and noisy.

This CL filters these advertisements out (by way of not filtering out
dual mode devices whose type/appearance are not set), while still
surfacing the headphones in the UI once they become truly pairable.

Bug: 930229
Change-Id: I252c3e391aac3d546e455ab01e626c08af0c64b5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1656971
Commit-Queue: Ryan Hansberry <hansberry@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#670269}

device/bluetooth/chromeos/bluetooth_utils.cc

@@ -89,14 +89,30 @@ BluetoothAdapter::DeviceList FilterUnknownDevices(
           result.push_back(device);
         }
         break;
-      // For classic and dual mode devices, only filter out if the name is empty
-      // because the device could have an unknown or even known type and still
-      // also provide audio/HID functionality.
+      // For classic mode devices, only filter out if the name is empty because
+      // the device could have an unknown or even known type and still also
+      // provide audio/HID functionality.
       case BLUETOOTH_TRANSPORT_CLASSIC:
-      case BLUETOOTH_TRANSPORT_DUAL:
         if (device->GetName())
           result.push_back(device);
         break;
+      // For dual mode devices, a device::BluetoothDevice object without a name
+      // and type/appearance most likely signals that it is truly only a LE
+      // advertisement for a peripheral which is active, but not pairable. Many
+      // popular headphones behave in this exact way. Filter them out until they
+      // provide a type/appearance; this means they've become pairable. See
+      // https://crbug.com/1656971 for more.
+      case BLUETOOTH_TRANSPORT_DUAL:
+        if (device->GetName()) {
+          if (base::FeatureList::IsEnabled(
+                  chromeos::features::kBluetoothAggressiveAppearanceFilter) &&
+              device->GetDeviceType() == BluetoothDeviceType::UNKNOWN) {
+            continue;
+          }
+
+          result.push_back(device);
+        }
+        break;
     }
   }
   return result;

device/bluetooth/chromeos/bluetooth_utils_unittest.cc

@@ -4,19 +4,78 @@
 
 #include "device/bluetooth/chromeos/bluetooth_utils.h"
 
+#include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/macros.h"
+#include "base/run_loop.h"
 #include "base/test/scoped_feature_list.h"
+#include "base/test/scoped_task_environment.h"
 #include "chromeos/constants/chromeos_features.h"
+#include "device/bluetooth/bluetooth_adapter_factory.h"
+#include "device/bluetooth/bluetooth_device.h"
+#include "device/bluetooth/test/mock_bluetooth_adapter.h"
+#include "device/bluetooth/test/mock_bluetooth_device.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace device {
 
+namespace {
+
+constexpr char kTestBluetoothDisplayName[] = "test_device_name";
+constexpr char kTestBluetoothDeviceAddress[] = "01:02:03:04:05:06";
+constexpr char kHIDServiceUUID[] = "1812";
+constexpr char kSecurityKeyServiceUUID[] = "FFFD";
+constexpr char kUnexpectedServiceUUID[] = "1234";
+const size_t kMaxDevicesForFilter = 5;
+
+}  // namespace
+
 class BluetoothUtilsTest : public testing::Test {
  protected:
   BluetoothUtilsTest() = default;
 
+  void SetUp() override {
+    BluetoothAdapterFactory::SetAdapterForTesting(adapter_);
+  }
+
+  MockBluetoothDevice* AddMockBluetoothDeviceToAdapter(
+      BluetoothTransport transport) {
+    auto mock_bluetooth_device =
+        std::make_unique<testing::NiceMock<MockBluetoothDevice>>(
+            adapter_.get(), 0 /* bluetooth_class */, kTestBluetoothDisplayName,
+            kTestBluetoothDeviceAddress, false /* paired */,
+            false /* connected */);
+
+    ON_CALL(*mock_bluetooth_device, GetType)
+        .WillByDefault(testing::Return(transport));
+
+    auto* mock_bluetooth_device_ptr = mock_bluetooth_device.get();
+    adapter_->AddMockDevice(std::move(mock_bluetooth_device));
+    return mock_bluetooth_device_ptr;
+  }
+
+  MockBluetoothAdapter* adapter() { return adapter_.get(); }
+
+  MockBluetoothDevice* GetMockBluetoothDevice(size_t index) {
+    return static_cast<MockBluetoothDevice*>(
+        adapter()->GetMockDevices()[index]);
+  }
+
+  void VerifyFilterBluetoothDeviceList(BluetoothFilterType filter_type,
+                                       size_t num_expected_remaining_devices) {
+    BluetoothAdapter::DeviceList filtered_device_list =
+        FilterBluetoothDeviceList(adapter_->GetMockDevices(), filter_type,
+                                  kMaxDevicesForFilter);
+    EXPECT_EQ(num_expected_remaining_devices, filtered_device_list.size());
+  }
+
+  void EnableAggressiveAppearanceFilter() {
+    feature_list_.InitAndEnableFeature(
+        chromeos::features::kBluetoothAggressiveAppearanceFilter);
+  }
+
   void SetLongTermKeys(const std::string& keys) {
     feature_list_.InitAndEnableFeatureWithParameters(
         chromeos::features::kBlueZLongTermKeyBlocklist,
@@ -24,12 +83,135 @@ class BluetoothUtilsTest : public testing::Test {
   }
 
  private:
+  base::test::ScopedTaskEnvironment scoped_task_environment_;
+  scoped_refptr<MockBluetoothAdapter> adapter_ =
+      base::MakeRefCounted<testing::NiceMock<MockBluetoothAdapter>>();
   base::test::ScopedFeatureList feature_list_;
 
   DISALLOW_COPY_AND_ASSIGN(BluetoothUtilsTest);
 };
 
-TEST_F(BluetoothUtilsTest, TestListIncludesBadLtks) {
+TEST_F(BluetoothUtilsTest,
+       TestFilterBluetoothDeviceList_FilterAll_NoDevicesFiltered) {
+  // If BluetoothFilterType::KNOWN were passed, this device would otherwise be
+  // filtered out, but we expect it to not be.
+  AddMockBluetoothDeviceToAdapter(BLUETOOTH_TRANSPORT_INVALID);
+
+  VerifyFilterBluetoothDeviceList(BluetoothFilterType::ALL,
+                                  1u /* num_expected_remaining_devices */);
+}
+
+TEST_F(BluetoothUtilsTest,
+       TestFilterBluetoothDeviceList_FilterAll_MaxDevicesExceeded) {
+  for (size_t i = 0; i < kMaxDevicesForFilter * 2; ++i)
+    AddMockBluetoothDeviceToAdapter(BLUETOOTH_TRANSPORT_INVALID);
+
+  VerifyFilterBluetoothDeviceList(
+      BluetoothFilterType::ALL,
+      kMaxDevicesForFilter /* num_expected_remaining_devices */);
+}
+
+TEST_F(BluetoothUtilsTest,
+       TestFilterBluetoothDeviceList_FilterKnown_AlwaysKeepPairedDevices) {
+  auto* mock_bluetooth_device =
+      AddMockBluetoothDeviceToAdapter(BLUETOOTH_TRANSPORT_INVALID);
+  EXPECT_CALL(*mock_bluetooth_device, IsPaired)
+      .WillRepeatedly(testing::Return(true));
+
+  VerifyFilterBluetoothDeviceList(BluetoothFilterType::KNOWN,
+                                  1u /* num_expected_remaining_devices */);
+}
+
+TEST_F(BluetoothUtilsTest,
+       TestFilterBluetoothDeviceList_FilterKnown_RemoveInvalidDevices) {
+  AddMockBluetoothDeviceToAdapter(BLUETOOTH_TRANSPORT_INVALID);
+
+  VerifyFilterBluetoothDeviceList(BluetoothFilterType::KNOWN,
+                                  0u /* num_expected_remaining_devices */);
+}
+
+TEST_F(BluetoothUtilsTest,
+       TestFilterBluetoothDeviceList_FilterKnown_KeepClassicDevicesWithNames) {
+  AddMockBluetoothDeviceToAdapter(BLUETOOTH_TRANSPORT_CLASSIC);
+
+  VerifyFilterBluetoothDeviceList(BluetoothFilterType::KNOWN,
+                                  1u /* num_expected_remaining_devices */);
+}
+
+TEST_F(
+    BluetoothUtilsTest,
+    TestFilterBluetoothDeviceList_FilterKnown_RemoveClassicDevicesWithoutNames) {
+  auto* mock_bluetooth_device =
+      AddMockBluetoothDeviceToAdapter(BLUETOOTH_TRANSPORT_CLASSIC);
+  EXPECT_CALL(*mock_bluetooth_device, GetName)
+      .WillOnce(testing::Return(base::nullopt));
+
+  VerifyFilterBluetoothDeviceList(BluetoothFilterType::KNOWN,
+                                  0u /* num_expected_remaining_devices */);
+}
+
+TEST_F(
+    BluetoothUtilsTest,
+    TestFilterBluetoothDeviceList_FilterKnown_RemoveBleDevicesWithoutExpectedUuids) {
+  auto* mock_bluetooth_device =
+      AddMockBluetoothDeviceToAdapter(BLUETOOTH_TRANSPORT_LE);
+  mock_bluetooth_device->AddUUID(device::BluetoothUUID(kUnexpectedServiceUUID));
+
+  VerifyFilterBluetoothDeviceList(BluetoothFilterType::KNOWN,
+                                  0u /* num_expected_remaining_devices */);
+}
+
+TEST_F(
+    BluetoothUtilsTest,
+    TestFilterBluetoothDeviceList_FilterKnown_KeepBleDevicesWithExpectedUuids) {
+  auto* mock_bluetooth_device_1 =
+      AddMockBluetoothDeviceToAdapter(BLUETOOTH_TRANSPORT_LE);
+  mock_bluetooth_device_1->AddUUID(device::BluetoothUUID(kHIDServiceUUID));
+
+  auto* mock_bluetooth_device_2 =
+      AddMockBluetoothDeviceToAdapter(BLUETOOTH_TRANSPORT_LE);
+  mock_bluetooth_device_2->AddUUID(
+      device::BluetoothUUID(kSecurityKeyServiceUUID));
+
+  VerifyFilterBluetoothDeviceList(BluetoothFilterType::KNOWN,
+                                  2u /* num_expected_remaining_devices */);
+}
+
+TEST_F(
+    BluetoothUtilsTest,
+    TestFilterBluetoothDeviceList_FilterKnown_KeepDualDevicesWithNamesAndAppearances) {
+  EnableAggressiveAppearanceFilter();
+
+  auto* mock_bluetooth_device =
+      AddMockBluetoothDeviceToAdapter(BLUETOOTH_TRANSPORT_DUAL);
+  EXPECT_CALL(*mock_bluetooth_device, GetDeviceType)
+      .WillOnce(testing::Return(BluetoothDeviceType::AUDIO));
+
+  VerifyFilterBluetoothDeviceList(BluetoothFilterType::KNOWN,
+                                  1u /* num_expected_remaining_devices */);
+}
+
+TEST_F(
+    BluetoothUtilsTest,
+    TestFilterBluetoothDeviceList_FilterKnown_DualDevicesWithoutAppearances_KeepWithFilterFlagDisabled) {
+  AddMockBluetoothDeviceToAdapter(BLUETOOTH_TRANSPORT_DUAL);
+
+  VerifyFilterBluetoothDeviceList(BluetoothFilterType::KNOWN,
+                                  1u /* num_expected_remaining_devices */);
+}
+
+TEST_F(
+    BluetoothUtilsTest,
+    TestFilterBluetoothDeviceList_FilterKnown_DualDevicesWithoutAppearances_RemoveWithFilterFlagEnabled) {
+  EnableAggressiveAppearanceFilter();
+
+  AddMockBluetoothDeviceToAdapter(BLUETOOTH_TRANSPORT_DUAL);
+
+  VerifyFilterBluetoothDeviceList(BluetoothFilterType::KNOWN,
+                                  0u /* num_expected_remaining_devices */);
+}
+
+TEST_F(BluetoothUtilsTest, TestGetBlockedLongTermKeys_ListIncludesBadLtks) {
   // One nibble too long, one nibble too short, and one nibble just right.
   std::string hex_key_1 = "000000000000000000000000000012345";
   std::string hex_key_2 = "0000000000000000000000000000123";
@@ -45,7 +227,7 @@ TEST_F(BluetoothUtilsTest, TestListIncludesBadLtks) {
   EXPECT_EQ(expected_array, device::GetBlockedLongTermKeys());
 }
 
-TEST_F(BluetoothUtilsTest, TestListIncludesNonHexInput) {
+TEST_F(BluetoothUtilsTest, TestGetBlockedLongTermKeys_ListIncludesNonHexInput) {
   std::string hex_key_1 = "bad00input00but00correct00length";
   std::string hex_key_2 = "00000000000000000000000000001234";
   SetLongTermKeys(hex_key_1 + ',' + hex_key_2);
@@ -67,7 +249,7 @@ TEST_F(BluetoothUtilsTest, TestEmptyList) {
   EXPECT_EQ(expected_array, device::GetBlockedLongTermKeys());
 }
 
-TEST_F(BluetoothUtilsTest, TestOneElementList) {
+TEST_F(BluetoothUtilsTest, TestGetBlockedLongTermKeys_OneElementList) {
   std::string hex_key_1 = "012300004567000089ab0000cdef0000";
   std::vector<uint8_t> expected_key_1 = {0x01, 0x23, 0x00, 0x00, 0x45, 0x67,
                                          0x00, 0x00, 0x89, 0xab, 0x00, 0x00,
@@ -81,7 +263,7 @@ TEST_F(BluetoothUtilsTest, TestOneElementList) {
   EXPECT_EQ(expected_array, device::GetBlockedLongTermKeys());
 }
 
-TEST_F(BluetoothUtilsTest, TestMultipleElementList) {
+TEST_F(BluetoothUtilsTest, TestGetBlockedLongTermKeys_MultipleElementList) {
   std::string hex_key_1 = "012300004567000089ab0000cdef0000";
   std::vector<uint8_t> expected_key_1 = {0x01, 0x23, 0x00, 0x00, 0x45, 0x67,
                                          0x00, 0x00, 0x89, 0xab, 0x00, 0x00,