[webhid] Show one chooser item per physical device

This CL modifies the return value from navigator.hid.requestDevice
to return an array of HIDDevice objects instead of a single
HIDDevice. If the device chooser is canceled without making a
selection, the promise is resolved with an empty array instead of
rejecting with a DOM exception.

The HID device chooser has also been modified such that each item
in the chooser represents a single physical device. If multiple
logical HID interfaces are exposed by the same physical device,
granting permission to access the single physical device grants
permission to access any logical HID interfaces hosted by the device
that satisfy the chooser filters.

BUG=1036439

Change-Id: Ib67b71bcee32e261d3c9425cfc8bd7173f4ebb8d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1966496Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: Matt Reynolds <mattreynolds@chromium.org>
Cr-Commit-Position: refs/heads/master@{#729890}

chrome/browser/hid/chrome_hid_delegate.cc

@@ -28,7 +28,7 @@ std::unique_ptr<content::HidChooser> ChromeHidDelegate::RunChooser(
   Browser* browser = chrome::FindBrowserWithWebContents(
       content::WebContents::FromRenderFrameHost(frame));
   if (!browser) {
-    std::move(callback).Run(nullptr);
+    std::move(callback).Run(std::vector<device::mojom::HidDeviceInfoPtr>());
     return nullptr;
   }
 

chrome/browser/ui/hid/hid_chooser_controller.cc

@@ -47,7 +47,7 @@ HidChooserController::HidChooserController(
 
 HidChooserController::~HidChooserController() {
   if (callback_)
-    std::move(callback_).Run(nullptr);
+    std::move(callback_).Run(std::vector<device::mojom::HidDeviceInfoPtr>());
 }
 
 bool HidChooserController::ShouldShowHelpButton() const {
@@ -68,7 +68,10 @@ size_t HidChooserController::NumOptions() const {
 
 base::string16 HidChooserController::GetOption(size_t index) const {
   DCHECK_LT(index, devices_.size());
-  const device::mojom::HidDeviceInfo& device = *devices_[index];
+  auto it = devices_.begin();
+  std::advance(it, index);
+  DCHECK_GT(it->second.size(), 0u);
+  auto& device = *it->second[0];
   if (device.product_name.empty()) {
     return l10n_util::GetStringFUTF16(
         IDS_HID_CHOOSER_ITEM_WITHOUT_NAME,
@@ -87,8 +90,17 @@ bool HidChooserController::IsPaired(size_t index) const {
   if (!chooser_context_)
     return false;
 
-  return chooser_context_->HasDevicePermission(
-      requesting_origin_, embedding_origin_, *devices_[index]);
+  auto it = devices_.begin();
+  std::advance(it, index);
+  DCHECK_GT(it->second.size(), 0u);
+  for (auto& device : it->second) {
+    if (!chooser_context_->HasDevicePermission(requesting_origin_,
+                                               embedding_origin_, *device)) {
+      return false;
+    }
+  }
+
+  return true;
 }
 
 void HidChooserController::Select(const std::vector<size_t>& indices) {
@@ -98,13 +110,18 @@ void HidChooserController::Select(const std::vector<size_t>& indices) {
   DCHECK_LT(index, devices_.size());
 
   if (!chooser_context_) {
-    std::move(callback_).Run(nullptr);
+    std::move(callback_).Run(std::vector<device::mojom::HidDeviceInfoPtr>());
     return;
   }
 
-  chooser_context_->GrantDevicePermission(requesting_origin_, embedding_origin_,
-                                          *devices_[index]);
-  std::move(callback_).Run(std::move(devices_[index]));
+  auto it = devices_.begin();
+  std::advance(it, index);
+  DCHECK_GT(it->second.size(), 0u);
+  for (auto& device : it->second) {
+    chooser_context_->GrantDevicePermission(requesting_origin_,
+                                            embedding_origin_, *device);
+  }
+  std::move(callback_).Run(std::move(it->second));
 }
 
 void HidChooserController::Cancel() {
@@ -128,8 +145,18 @@ void HidChooserController::OnGotDevices(
     if (ShouldExcludeDevice(*device))
       continue;
 
-    if (FilterMatchesAny(*device))
-      devices_.push_back(std::move(device));
+    if (FilterMatchesAny(*device)) {
+      // A single physical device may expose multiple HID interfaces, each
+      // represented by a HidDeviceInfo object. When a device exposes multiple
+      // HID interfaces, the HidDeviceInfo objects will share a common
+      // |physical_device_id|. Group these devices so that a single chooser item
+      // is shown for each physical device. If a device's physical device ID is
+      // empty, use its GUID instead.
+      const std::string& key = device->physical_device_id.empty()
+                                   ? device->guid
+                                   : device->physical_device_id;
+      devices_[key].push_back(std::move(device));
+    }
   }
 
   if (view())

chrome/browser/ui/hid/hid_chooser_controller.h

@@ -5,6 +5,7 @@
 #ifndef CHROME_BROWSER_UI_HID_HID_CHOOSER_CONTROLLER_H_
 #define CHROME_BROWSER_UI_HID_HID_CHOOSER_CONTROLLER_H_
 
+#include <map>
 #include <string>
 #include <vector>
 
@@ -63,7 +64,12 @@ class HidChooserController : public ChooserController {
   // The lifetime of the chooser context is tied to the browser context used to
   // create it, and may be destroyed while the chooser is still active.
   base::WeakPtr<HidChooserContext> chooser_context_;
-  std::vector<device::mojom::HidDeviceInfoPtr> devices_;
+
+  // Information about connected devices and their HID interfaces. A single
+  // physical device may expose multiple HID interfaces. Keys are physical
+  // device IDs, values are collections of HidDeviceInfo objects representing
+  // the HID interfaces hosted by the physical device.
+  std::map<std::string, std::vector<device::mojom::HidDeviceInfoPtr>> devices_;
 
   base::WeakPtrFactory<HidChooserController> weak_factory_{this};
 

chrome/browser/ui/hid/hid_chooser_controller_unittest.cc

@@ -337,3 +337,49 @@ TEST_F(HidChooserControllerTest, DeviceIdAndUsageFilterUnion) {
 
   EXPECT_EQ(3u, hid_chooser_controller->NumOptions());
 }
+
+TEST_F(HidChooserControllerTest, OneItemForSamePhysicalDevice) {
+  auto hid_chooser_controller = CreateHidChooserControllerWithoutFilters();
+
+  base::RunLoop run_loop;
+  fake_hid_chooser_view_.set_options_initialized_quit_closure(
+      run_loop.QuitClosure());
+
+  // These two devices have the same physical device ID and should be coalesced
+  // into a single chooser item.
+  CreateAndAddFakeHidDevice(kTestPhysicalDeviceIds[0], 1, 1, "a", "001",
+                            device::mojom::kPageGenericDesktop,
+                            device::mojom::kGenericDesktopGamePad);
+  CreateAndAddFakeHidDevice(kTestPhysicalDeviceIds[0], 1, 1, "a", "001",
+                            device::mojom::kPageSimulation, 5);
+
+  // This device has the same info as the first device except for the physical
+  // device ID. It should have a separate chooser item.
+  CreateAndAddFakeHidDevice(kTestPhysicalDeviceIds[1], 1, 1, "a", "001",
+                            device::mojom::kPageGenericDesktop,
+                            device::mojom::kGenericDesktopGamePad);
+
+  run_loop.Run();
+
+  EXPECT_EQ(2u, hid_chooser_controller->NumOptions());
+}
+
+TEST_F(HidChooserControllerTest, NoMergeWithEmptyPhysicalDeviceId) {
+  auto hid_chooser_controller = CreateHidChooserControllerWithoutFilters();
+
+  base::RunLoop run_loop;
+  fake_hid_chooser_view_.set_options_initialized_quit_closure(
+      run_loop.QuitClosure());
+
+  // These two devices have an empty string for the physical device ID and
+  // should not be coalesced.
+  CreateAndAddFakeHidDevice("", 1, 1, "a", "001",
+                            device::mojom::kPageGenericDesktop,
+                            device::mojom::kGenericDesktopGamePad);
+  CreateAndAddFakeHidDevice("", 1, 1, "a", "001",
+                            device::mojom::kPageSimulation, 5);
+
+  run_loop.Run();
+
+  EXPECT_EQ(2u, hid_chooser_controller->NumOptions());
+}

content/browser/hid/hid_browsertest.cc

@@ -94,13 +94,17 @@ IN_PROC_BROWSER_TEST_F(HidTest, RequestDevice) {
 
   auto device = device::mojom::HidDeviceInfo::New();
   device->guid = "test-guid";
+  std::vector<device::mojom::HidDeviceInfoPtr> devices;
+  devices.push_back(std::move(device));
   EXPECT_CALL(delegate(), RunChooserInternal)
-      .WillOnce(Return(ByMove(std::move(device))));
+      .WillOnce(Return(ByMove(std::move(devices))));
 
   EXPECT_EQ(true, EvalJs(shell(),
                          R"((async () => {
-               let device = await navigator.hid.requestDevice({filters:[]});
-               return device instanceof HIDDevice;
+               let devices = await navigator.hid.requestDevice({filters:[]});
+               return devices instanceof Array
+                      && devices.length == 1
+                      && devices[0] instanceof HIDDevice;
              })())"));
 }
 
@@ -111,15 +115,11 @@ IN_PROC_BROWSER_TEST_F(HidTest, DisallowRequestDevice) {
       .WillOnce(Return(false));
   EXPECT_CALL(delegate(), RunChooserInternal).Times(Exactly(0));
 
-  EXPECT_EQ(false, EvalJs(shell(),
-                          R"((async () => {
-                            try {
-                              await navigator.hid.requestDevice({filters:[]});
-                              return true;
-                            } catch (e) {
-                              return false;
-                            }
-                          })())"));
+  EXPECT_EQ(0, EvalJs(shell(),
+                      R"((async () => {
+               let devices = await navigator.hid.requestDevice({filters:[]});
+               return devices.length;
+             })())"));
 }
 
 }  // namespace content

content/browser/hid/hid_service.cc

@@ -70,7 +70,7 @@ void HidService::RequestDevice(
     RequestDeviceCallback callback) {
   HidDelegate* delegate = GetContentClient()->browser()->GetHidDelegate();
   if (!delegate->CanRequestDevicePermission(web_contents(), origin())) {
-    std::move(callback).Run(nullptr);
+    std::move(callback).Run(std::vector<device::mojom::HidDeviceInfoPtr>());
     return;
   }
 
@@ -126,14 +126,10 @@ void HidService::FinishGetDevices(
   std::move(callback).Run(std::move(result));
 }
 
-void HidService::FinishRequestDevice(RequestDeviceCallback callback,
-                                     device::mojom::HidDeviceInfoPtr device) {
-  if (!device) {
-    std::move(callback).Run(nullptr);
-    return;
-  }
-
-  std::move(callback).Run(std::move(device));
+void HidService::FinishRequestDevice(
+    RequestDeviceCallback callback,
+    std::vector<device::mojom::HidDeviceInfoPtr> devices) {
+  std::move(callback).Run(std::move(devices));
 }
 
 void HidService::FinishConnect(

content/browser/hid/hid_service.h

@@ -48,8 +48,9 @@ class HidService : public content::FrameServiceBase<blink::mojom::HidService>,
 
   void FinishGetDevices(GetDevicesCallback callback,
                         std::vector<device::mojom::HidDeviceInfoPtr> devices);
-  void FinishRequestDevice(RequestDeviceCallback callback,
-                           device::mojom::HidDeviceInfoPtr device);
+  void FinishRequestDevice(
+      RequestDeviceCallback callback,
+      std::vector<device::mojom::HidDeviceInfoPtr> devices);
   void FinishConnect(
       ConnectCallback callback,
       mojo::PendingRemote<device::mojom::HidConnection> connection);

content/browser/hid/hid_service_unittest.cc

@@ -143,24 +143,27 @@ TEST_F(HidServiceTest, RequestDevice) {
 
   auto device_info = device::mojom::HidDeviceInfo::New();
   device_info->guid = kTestGuid;
-  hid_manager()->AddDevice(device_info.Clone());
+  std::vector<device::mojom::HidDeviceInfoPtr> device_infos;
+  device_infos.push_back(device_info.Clone());
+  hid_manager()->AddDevice(std::move(device_info));
 
   EXPECT_CALL(hid_delegate(), CanRequestDevicePermission)
       .WillOnce(testing::Return(true));
   EXPECT_CALL(hid_delegate(), RunChooserInternal)
-      .WillOnce(testing::Return(testing::ByMove(std::move(device_info))));
+      .WillOnce(testing::Return(testing::ByMove(std::move(device_infos))));
 
   base::RunLoop run_loop;
-  device::mojom::HidDeviceInfoPtr chosen_device;
+  std::vector<device::mojom::HidDeviceInfoPtr> chosen_devices;
   service->RequestDevice(
       std::vector<blink::mojom::HidDeviceFilterPtr>(),
       base::BindLambdaForTesting(
-          [&run_loop, &chosen_device](device::mojom::HidDeviceInfoPtr d) {
-            chosen_device = std::move(d);
+          [&run_loop,
+           &chosen_devices](std::vector<device::mojom::HidDeviceInfoPtr> d) {
+            chosen_devices = std::move(d);
             run_loop.Quit();
           }));
   run_loop.Run();
-  EXPECT_TRUE(chosen_device);
+  EXPECT_EQ(1u, chosen_devices.size());
 }
 
 TEST_F(HidServiceTest, OpenAndCloseHidConnection) {

content/browser/hid/hid_test_utils.h

@@ -29,7 +29,8 @@ class MockHidDelegate : public HidDelegate {
       std::vector<blink::mojom::HidDeviceFilterPtr> filters,
       HidChooser::Callback callback) override;
 
-  MOCK_METHOD0(RunChooserInternal, device::mojom::HidDeviceInfoPtr());
+  MOCK_METHOD0(RunChooserInternal,
+               std::vector<device::mojom::HidDeviceInfoPtr>());
   MOCK_METHOD2(CanRequestDevicePermission,
                bool(content::WebContents* web_contents,
                     const url::Origin& requesting_origin));

content/public/browser/hid_chooser.h

@@ -16,9 +16,10 @@ namespace content {
 // object should cancel the prompt.
 class CONTENT_EXPORT HidChooser {
  public:
-  // Callback type used to report the user action. Passed |nullptr| if no device
-  // was selected.
-  using Callback = base::OnceCallback<void(device::mojom::HidDeviceInfoPtr)>;
+  // Callback type used to report the user action. An empty vector is passed if
+  // no device was selected.
+  using Callback =
+      base::OnceCallback<void(std::vector<device::mojom::HidDeviceInfoPtr>)>;
 
   HidChooser() = default;
   virtual ~HidChooser() = default;

third_party/blink/public/mojom/hid/hid.mojom

@@ -76,10 +76,11 @@ interface HidService {
   // Requests access to a device. A chooser dialog is displayed with a list of
   // connected devices. If |filters| is non-empty, only devices that match one
   // or more of the filters will be included in the chooser list. If |filters|
-  // is empty, all connected devices are included. |device| contains the chosen
-  // device, or nullptr if the chooser dialog was canceled.
+  // is empty, all connected devices are included. |devices| contains logical
+  // HID interfaces exposed by the device. If the chooser dialog is canceled,
+  // |devices| is an empty array.
   RequestDevice(array<HidDeviceFilter> filters)
-      => (device.mojom.HidDeviceInfo? device);
+      => (array<device.mojom.HidDeviceInfo> devices);
 
   // Opens a connection to the device with GUID matching |device_guid|. |client|
   // will be notified when an input report is received from the device.

third_party/blink/renderer/modules/hid/hid.cc

@@ -26,7 +26,6 @@ namespace {
 const char kContextGone[] = "Script context has shut down.";
 const char kFeaturePolicyBlocked[] =
     "Access to the feature \"hid\" is disallowed by feature policy.";
-const char kNoDeviceSelected[] = "No device selected.";
 
 void RejectWithTypeError(const String& message,
                          ScriptPromiseResolver* resolver) {
@@ -211,17 +210,15 @@ void HID::FinishGetDevices(
 
 void HID::FinishRequestDevice(
     ScriptPromiseResolver* resolver,
-    device::mojom::blink::HidDeviceInfoPtr device_info) {
+    Vector<device::mojom::blink::HidDeviceInfoPtr> device_infos) {
   DCHECK(request_device_promises_.Contains(resolver));
   request_device_promises_.erase(resolver);
 
-  if (device_info) {
-    resolver->Resolve(GetOrCreateDevice(std::move(device_info)));
-  } else {
-    resolver->Reject(MakeGarbageCollected<DOMException>(
-        DOMExceptionCode::kNotFoundError, kNoDeviceSelected));
-  }
-  request_device_promises_.erase(resolver);
+  HeapVector<Member<HIDDevice>> devices;
+  for (auto& device_info : device_infos)
+    devices.push_back(GetOrCreateDevice(std::move(device_info)));
+
+  resolver->Resolve(devices);
 }
 
 void HID::EnsureServiceConnection() {
@@ -250,10 +247,8 @@ void HID::OnServiceConnectionError() {
 
   HeapHashSet<Member<ScriptPromiseResolver>> request_device_promises;
   request_device_promises_.swap(request_device_promises);
-  for (ScriptPromiseResolver* resolver : request_device_promises) {
-    resolver->Reject(MakeGarbageCollected<DOMException>(
-        DOMExceptionCode::kNotFoundError, kNoDeviceSelected));
-  }
+  for (ScriptPromiseResolver* resolver : request_device_promises)
+    resolver->Resolve(HeapVector<Member<HIDDevice>>());
 }
 
 void HID::Trace(blink::Visitor* visitor) {

third_party/blink/renderer/modules/hid/hid.h

@@ -67,7 +67,7 @@ class HID : public EventTargetWithInlineData, public ContextLifecycleObserver {
   void FinishGetDevices(ScriptPromiseResolver*,
                         Vector<device::mojom::blink::HidDeviceInfoPtr>);
   void FinishRequestDevice(ScriptPromiseResolver*,
-                           device::mojom::blink::HidDeviceInfoPtr);
+                           Vector<device::mojom::blink::HidDeviceInfoPtr>);
 
   mojo::Remote<mojom::blink::HidService> service_;
   HeapHashSet<Member<ScriptPromiseResolver>> get_devices_promises_;

third_party/blink/web_tests/hid/hidDevice_deviceInfo.html

@@ -93,8 +93,11 @@ hid_test(async (t, fake) => {
   fake.setSelectedDevice(guid);
 
   await trustedClick();
-  const d = await navigator.hid.requestDevice({filters: []});
+  const devices = await navigator.hid.requestDevice({filters: []});
+  assert_true(devices instanceof Array, 'devices instanceof Array');
+  assert_equals(devices.length, 1, 'devices.length');
 
+  const d = devices[0];
   assert_true(d instanceof HIDDevice, 'device instanceof HIDDevice');
   assert_false(d.opened, 'device.opened');
   assert_equals(d.vendorId, kTestVendorId, 'device.vendorId');
@@ -168,8 +171,11 @@ hid_test(async (t, fake) => {
   fake.setSelectedDevice(guid);
 
   await trustedClick();
-  const d = await navigator.hid.requestDevice({filters: []});
+  const devices = await navigator.hid.requestDevice({filters: []});
+  assert_true(devices instanceof Array, 'devices instanceof Array');
+  assert_equals(devices.length, 1, 'devices.length');
 
+  const d = devices[0];
   assert_true(d instanceof HIDDevice, 'device instanceof HIDDevice');
   assert_equals(d.collections.length, 1, 'device.collections.length');
 

third_party/blink/web_tests/hid/hidDevice_openAndClose.html

@@ -18,7 +18,11 @@ hid_test(async (t, fake) => {
   fake.setSelectedDevice(guid);
 
   await trustedClick();
-  const device = await navigator.hid.requestDevice({filters: []});
+  const devices = await navigator.hid.requestDevice({filters: []});
+  assert_true(devices instanceof Array);
+  assert_equals(devices.length, 1);
+
+  const device = devices[0];
   assert_true(device instanceof HIDDevice);
   assert_false(device.opened);
 
@@ -31,7 +35,12 @@ hid_test(async (t, fake) => {
   fake.setSelectedDevice(guid);
 
   await trustedClick();
-  const device = await navigator.hid.requestDevice({filters: []});
+  const devices = await navigator.hid.requestDevice({filters: []});
+  assert_true(devices instanceof Array);
+  assert_equals(devices.length, 1);
+
+  const device = devices[0];
+  assert_true(device instanceof HIDDevice);
   assert_false(device.opened);
 
   await device.open();
@@ -46,7 +55,12 @@ hid_test(async (t, fake) => {
   fake.setSelectedDevice(guid);
 
   await trustedClick();
-  const device = await navigator.hid.requestDevice({filters: []});
+  const devices = await navigator.hid.requestDevice({filters: []});
+  assert_true(devices instanceof Array);
+  assert_equals(devices.length, 1);
+
+  const device = devices[0];
+  assert_true(device instanceof HIDDevice);
   assert_false(device.opened);
 
   await device.open();
@@ -58,7 +72,12 @@ hid_test(async (t, fake) => {
   fake.setSelectedDevice(guid);
 
   await trustedClick();
-  const device = await navigator.hid.requestDevice({filters: []});
+  const devices = await navigator.hid.requestDevice({filters: []});
+  assert_true(devices instanceof Array);
+  assert_equals(devices.length, 1);
+
+  const device = devices[0];
+  assert_true(device instanceof HIDDevice);
   assert_false(device.opened);
 
   const firstRequest = device.open();

third_party/blink/web_tests/hid/hidDevice_reports.html

@@ -19,11 +19,16 @@ const kReportBytes = new Uint8Array([0x23, 0x45, 0x67, 0x89, 0xab, 0xcd, 0xef]);
 // the newly created device, and returns the HidConnection fake.
 async function addAndOpenDevice(fake) {
   const deviceInfo = fake.makeDevice(kTestVendorId, kTestProductId);
-  const guid = fake.addDevice(deviceInfo);
-  fake.setSelectedDevice(guid);
+  const guid = deviceInfo.guid;
+  const key = fake.addDevice(deviceInfo);
+  fake.setSelectedDevice(key);
 
   await trustedClick();
-  const device = await navigator.hid.requestDevice({filters: []});
+  const devices = await navigator.hid.requestDevice({filters: []});
+  assert_true(devices instanceof Array);
+  assert_equals(devices.length, 1);
+
+  const device = devices[0];
   assert_true(device instanceof HIDDevice);
 
   await device.open();

third_party/blink/web_tests/hid/hid_requestDevice.html

@@ -25,27 +25,32 @@ promise_test(async (t) => {
   interceptor.start();
 
   await trustedClick();
+  let devices = null;
   try {
-    await promise_rejects(
-        t, 'NotFoundError', navigator.hid.requestDevice({filters: []}));
+    devices = await navigator.hid.requestDevice({filters: []});
   } finally {
     interceptor.stop();
   }
-}, 'requestDevice() rejects if Mojo service connection fails');
+  assert_true(devices instanceof Array);
+  assert_equals(devices.length, 0);
+}, 'requestDevice() returns an empty array if Mojo service connection fails');
 
 hid_test(async (t, fake) => {
   await trustedClick();
-  return promise_rejects(
-      t, 'NotFoundError', navigator.hid.requestDevice({filters: []}));
-}, 'requestDevice() rejects if no device has been selected');
+  let devices = await navigator.hid.requestDevice({filters: []});
+  assert_true(devices instanceof Array);
+  assert_equals(devices.length, 0);
+}, 'requestDevice() returns an empty array if no device has been selected');
 
 hid_test(async (t, fake) => {
   let guid = fake.addDevice(fake.makeDevice(kTestVendorId, kTestProductId));
   fake.setSelectedDevice(guid);
 
   await trustedClick();
-  let device = await navigator.hid.requestDevice({filters: []});
-  assert_true(device instanceof HIDDevice);
+  let devices = await navigator.hid.requestDevice({filters: []});
+  assert_true(devices instanceof Array);
+  assert_equals(devices.length, 1);
+  assert_true(devices[0] instanceof HIDDevice);
 }, 'requestDevice() returns the selected device');
 
 hid_test(async (t, fake) => {
@@ -53,12 +58,56 @@ hid_test(async (t, fake) => {
   fake.setSelectedDevice(guid);
 
   await trustedClick();
-  let firstDevice = await navigator.hid.requestDevice({filters: []});
-  assert_true(firstDevice instanceof HIDDevice);
-  let secondDevice = await navigator.hid.requestDevice({filters: []});
-  assert_true(secondDevice instanceof HIDDevice);
-  assert_true(firstDevice === secondDevice);
+  let firstDevices = await navigator.hid.requestDevice({filters: []});
+  assert_true(firstDevices instanceof Array);
+  assert_equals(firstDevices.length, 1);
+  assert_true(firstDevices[0] instanceof HIDDevice);
+  let secondDevices = await navigator.hid.requestDevice({filters: []});
+  assert_false(firstDevices === secondDevices);
+  assert_true(secondDevices instanceof Array);
+  assert_equals(secondDevices.length, 1);
+  assert_true(secondDevices[0] instanceof HIDDevice);
+  assert_true(firstDevices[0] === secondDevices[0]);
 }, 'requestDevice() returns the same device object every time');
 
+hid_test(async (t, fake) => {
+  // Construct two HidDeviceInfo objects with the same physical device ID to
+  // simulate a device with two HID interfaces.
+  let device0 = fake.makeDevice(kTestVendorId, kTestProductId);
+  let device1 = fake.makeDevice(kTestVendorId, kTestProductId);
+  device1.physicalDeviceId = device0.physicalDeviceId;
+  let key0 = fake.addDevice(device0);
+  let key1 = fake.addDevice(device1);
+  assert_equals(key0, key1);
+  fake.setSelectedDevice(key0);
+
+  await trustedClick();
+  let devices = await navigator.hid.requestDevice({filters: []});
+  assert_true(devices instanceof Array);
+  assert_equals(2, devices.length);
+  assert_true(devices[0] instanceof HIDDevice);
+  assert_true(devices[1] instanceof HIDDevice);
+  assert_false(devices[0] === devices[1]);
+}, 'requestDevice() returns 2 HIDDevices for a device with 2 HID interfaces');
+
+hid_test(async (t, fake) => {
+  // Construct two HidDeviceInfo objects with empty physical device IDs.
+  let device0 = fake.makeDevice(kTestVendorId, kTestProductId);
+  let device1 = fake.makeDevice(kTestVendorId, kTestProductId);
+  device0.physicalDeviceId = '';
+  device1.physicalDeviceId = '';
+  let key0 = fake.addDevice(device0);
+  let key1 = fake.addDevice(device1);
+  assert_not_equals('', key0);
+  assert_not_equals(key1, key0);
+  fake.setSelectedDevice(key0);
+
+  await trustedClick();
+  let devices = await navigator.hid.requestDevice({filters: []});
+  assert_true(devices instanceof Array);
+  assert_equals(1, devices.length);
+  assert_true(devices[0] instanceof HIDDevice);
+}, 'requestDevice() does not merge devices with empty physical device IDs');
+
 </script>
 </body>

third_party/blink/web_tests/hid/resources/hid-test-utils.js

@@ -152,7 +152,7 @@ class FakeHidService {
   reset() {
     this.devices_ = new Map();
     this.fakeConnections_ = new Map();
-    this.selectedDevice_ = null;
+    this.selectedDevices_ = [];
   }
 
   // Creates and returns a HidDeviceInfo with the specified device IDs.
@@ -171,24 +171,32 @@ class FakeHidService {
     return info;
   }
 
-  // Simulates a connected device. Returns the device GUID. A device connection
-  // event is not generated.
+  // Simulates a connected device. Returns the key used to store the device in
+  // the map. The key is either the physical device ID, or the device GUID if it
+  // has no physical device ID. A device connection event is not generated.
   addDevice(deviceInfo) {
-    this.devices_.set(deviceInfo.guid, deviceInfo);
-    return deviceInfo.guid;
+    let key = deviceInfo.physicalDeviceId;
+    if (key.length === 0)
+      key = deviceInfo.guid;
+    let devices = this.devices_.get(key);
+    if (devices === undefined)
+      devices = [];
+    devices.push(deviceInfo);
+    this.devices_.set(key, devices);
+    return key;
   }
 
   // Simulates disconnecting a connected device. A device disconnection event is
   // not generated.
-  removeDevice(guid) {
-    this.devices_.delete(guid);
+  removeDevice(key) {
+    this.devices_.delete(key);
   }
 
-  // Sets the GUID of the device that will be returned as the selected item the
-  // next time requestDevice is called. The device with this GUID must have been
+  // Sets the key of the device that will be returned as the selected item the
+  // next time requestDevice is called. The device with this key must have been
   // previously added with addDevice.
-  setSelectedDevice(guid) {
-    this.selectedDevice_ = this.devices_.get(guid);
+  setSelectedDevice(key) {
+    this.selectedDevices_ = this.devices_.get(key);
   }
 
   // Returns the fake HidConnection object for this device, if there is one. A
@@ -205,13 +213,17 @@ class FakeHidService {
   // devices that the client has already been granted permission to access, but
   // for the fake implementation all simulated devices are returned.
   async getDevices() {
-    return { devices: Array.from(this.devices_.values()) };
+    let devices = [];
+    this.devices_.forEach((value) => {
+      devices = devices.concat(value);
+    });
+    return { devices: devices };
   }
 
-  // Simulates a device chooser prompt, returning |selectedDevice_| as the
+  // Simulates a device chooser prompt, returning |selectedDevices_| as the
   // simulated selection. |filters| is ignored.
   async requestDevice(filters) {
-    return { device: this.selectedDevice_ };
+    return { devices: this.selectedDevices_ };
   }
 
   // Returns a fake connection to the device with the specified GUID. If