hid: Allow late enumeration of HID collections

On Windows, each top-level HID collection is enumerated as a
separate device node. The HID backend attempts to detect
sibling device nodes that represent top-level collections and
merge them back into a single logical device.

The current strategy relies on all sibling nodes being
available when the notification for the first node is received.
However, this is not always the case which can lead to one or
more top-level collections being lost.

This CL enables the HID backend to notify clients with a
DeviceChanged event when an additional top-level collection is
found.

Bug: 1186331
Change-Id: I69df14109a555801e0d5767d73310290a68e466b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2773820
Commit-Queue: Matt Reynolds <mattreynolds@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#868296}

chrome/browser/hid/chrome_hid_delegate.cc

@@ -114,6 +114,12 @@ void ChromeHidDelegate::OnDeviceRemoved(
     observer.OnDeviceRemoved(device_info);
 }
 
+void ChromeHidDelegate::OnDeviceChanged(
+    const device::mojom::HidDeviceInfo& device_info) {
+  for (auto& observer : observer_list_)
+    observer.OnDeviceChanged(device_info);
+}
+
 void ChromeHidDelegate::OnHidManagerConnectionError() {
   device_observer_.RemoveAll();
   permission_observer_.RemoveAll();

chrome/browser/hid/chrome_hid_delegate.h

@@ -47,6 +47,7 @@ class ChromeHidDelegate
   // HidChooserContext::DeviceObserver:
   void OnDeviceAdded(const device::mojom::HidDeviceInfo&) override;
   void OnDeviceRemoved(const device::mojom::HidDeviceInfo&) override;
+  void OnDeviceChanged(const device::mojom::HidDeviceInfo&) override;
   void OnHidManagerConnectionError() override;
   void OnHidChooserContextShutdown() override;
 

chrome/browser/hid/hid_chooser_context.cc

@@ -53,6 +53,9 @@ void HidChooserContext::DeviceObserver::OnDeviceAdded(
 void HidChooserContext::DeviceObserver::OnDeviceRemoved(
     const device::mojom::HidDeviceInfo& device) {}
 
+void HidChooserContext::DeviceObserver::OnDeviceChanged(
+    const device::mojom::HidDeviceInfo& device) {}
+
 void HidChooserContext::DeviceObserver::OnHidManagerConnectionError() {}
 
 HidChooserContext::HidChooserContext(Profile* profile)
@@ -276,7 +279,18 @@ void HidChooserContext::SetHidManagerForTesting(
       &HidChooserContext::OnHidManagerConnectionError, base::Unretained(this)));
 
   hid_manager_->GetDevicesAndSetClient(
-      client_receiver_.BindNewEndpointAndPassRemote(), std::move(callback));
+      client_receiver_.BindNewEndpointAndPassRemote(),
+      base::BindOnce(&HidChooserContext::OnHidManagerInitializedForTesting,
+                     weak_factory_.GetWeakPtr(), std::move(callback)));
+}
+
+void HidChooserContext::OnHidManagerInitializedForTesting(
+    device::mojom::HidManager::GetDevicesCallback callback,
+    std::vector<device::mojom::HidDeviceInfoPtr> devices) {
+  DCHECK(devices.empty());
+  DCHECK(pending_get_devices_requests_.empty());
+  is_initialized_ = true;
+  std::move(callback).Run({});
 }
 
 base::WeakPtr<HidChooserContext> HidChooserContext::AsWeakPtr() {
@@ -329,6 +343,18 @@ void HidChooserContext::DeviceRemoved(device::mojom::HidDeviceInfoPtr device) {
   }
 }
 
+void HidChooserContext::DeviceChanged(device::mojom::HidDeviceInfoPtr device) {
+  DCHECK(device);
+  DCHECK(base::Contains(devices_, device->guid));
+
+  // Update the device list.
+  devices_[device->guid] = device->Clone();
+
+  // Notify all observers.
+  for (auto& observer : device_observer_list_)
+    observer.OnDeviceChanged(*device);
+}
+
 void HidChooserContext::EnsureHidManagerConnection() {
   if (hid_manager_)
     return;

chrome/browser/hid/hid_chooser_context.h

@@ -39,6 +39,7 @@ class HidChooserContext : public permissions::ChooserContextBase,
    public:
     virtual void OnDeviceAdded(const device::mojom::HidDeviceInfo&);
     virtual void OnDeviceRemoved(const device::mojom::HidDeviceInfo&);
+    virtual void OnDeviceChanged(const device::mojom::HidDeviceInfo&);
     virtual void OnHidManagerConnectionError();
 
     // Called when the HidChooserContext is shutting down. Observers must remove
@@ -103,11 +104,15 @@ class HidChooserContext : public permissions::ChooserContextBase,
   // device::mojom::HidManagerClient implementation:
   void DeviceAdded(device::mojom::HidDeviceInfoPtr device_info) override;
   void DeviceRemoved(device::mojom::HidDeviceInfoPtr device_info) override;
+  void DeviceChanged(device::mojom::HidDeviceInfoPtr device_info) override;
 
   void EnsureHidManagerConnection();
   void SetUpHidManagerConnection(
       mojo::PendingRemote<device::mojom::HidManager> manager);
   void InitDeviceList(std::vector<device::mojom::HidDeviceInfoPtr> devices);
+  void OnHidManagerInitializedForTesting(
+      device::mojom::HidManager::GetDevicesCallback callback,
+      std::vector<device::mojom::HidDeviceInfoPtr> devices);
   void OnHidManagerConnectionError();
 
   const bool is_incognito_;

chrome/browser/hid/hid_chooser_context_unittest.cc

@@ -87,10 +87,18 @@ class HidChooserContextTest : public testing::Test {
         device::mojom::HidBusType::kHIDBusTypeUSB);
   }
 
+  void ConnectDevice(const device::mojom::HidDeviceInfo& device) {
+    hid_manager_.AddDevice(device.Clone());
+  }
+
   void DisconnectDevice(const device::mojom::HidDeviceInfo& device) {
     hid_manager_.RemoveDevice(device.guid);
   }
 
+  void UpdateDevice(const device::mojom::HidDeviceInfo& device) {
+    hid_manager_.ChangeDevice(device.Clone());
+  }
+
   void SimulateHidManagerConnectionError() {
     hid_manager_.SimulateConnectionError();
   }
@@ -399,3 +407,80 @@ TEST_F(HidChooserContextTest, ConnectionErrorWithPersistentPermission) {
 
   EXPECT_TRUE(context->HasDevicePermission(origin(), *device));
 }
+
+namespace {
+
+device::mojom::HidDeviceInfoPtr CreateDeviceWithOneCollection(
+    const std::string& guid) {
+  auto device_info = device::mojom::HidDeviceInfo::New();
+  device_info->guid = guid;
+  auto collection = device::mojom::HidCollectionInfo::New();
+  collection->usage = device::mojom::HidUsageAndPage::New(1, 1);
+  collection->input_reports.push_back(
+      device::mojom::HidReportDescription::New());
+  device_info->collections.push_back(std::move(collection));
+  return device_info;
+}
+
+device::mojom::HidDeviceInfoPtr CreateDeviceWithTwoCollections(
+    const std::string& guid) {
+  auto device_info = CreateDeviceWithOneCollection(guid);
+  auto collection = device::mojom::HidCollectionInfo::New();
+  collection->usage = device::mojom::HidUsageAndPage::New(2, 2);
+  collection->output_reports.push_back(
+      device::mojom::HidReportDescription::New());
+  device_info->collections.push_back(std::move(collection));
+  return device_info;
+}
+
+}  // namespace
+
+TEST_F(HidChooserContextTest, AddChangeRemoveDevice) {
+  const char kTestGuid[] = "guid";
+
+  HidChooserContext* context = GetContext();
+
+  EXPECT_FALSE(context->GetDeviceInfo(kTestGuid));
+
+  // Connect a partially-initialized device.
+  base::RunLoop device_added_loop;
+  EXPECT_CALL(device_observer(), OnDeviceAdded).WillOnce([&](const auto& d) {
+    EXPECT_EQ(d.guid, kTestGuid);
+    EXPECT_EQ(d.collections.size(), 1u);
+    device_added_loop.Quit();
+  });
+  auto partial_device = CreateDeviceWithOneCollection(kTestGuid);
+  ConnectDevice(*partial_device);
+  device_added_loop.Run();
+
+  auto* device_info = context->GetDeviceInfo(kTestGuid);
+  ASSERT_TRUE(device_info);
+  EXPECT_EQ(device_info->collections.size(), 1u);
+
+  // Update the device to add another collection.
+  base::RunLoop device_changed_loop;
+  EXPECT_CALL(device_observer(), OnDeviceChanged).WillOnce([&](const auto& d) {
+    EXPECT_EQ(d.guid, kTestGuid);
+    EXPECT_EQ(d.collections.size(), 2u);
+    device_changed_loop.Quit();
+  });
+  auto complete_device = CreateDeviceWithTwoCollections(kTestGuid);
+  UpdateDevice(*complete_device);
+  device_changed_loop.Run();
+
+  device_info = context->GetDeviceInfo(kTestGuid);
+  ASSERT_TRUE(device_info);
+  EXPECT_EQ(device_info->collections.size(), 2u);
+
+  // Disconnect the device.
+  base::RunLoop device_removed_loop;
+  EXPECT_CALL(device_observer(), OnDeviceRemoved).WillOnce([&](const auto& d) {
+    EXPECT_EQ(d.guid, kTestGuid);
+    EXPECT_EQ(d.collections.size(), 2u);
+    device_removed_loop.Quit();
+  });
+  DisconnectDevice(*complete_device);
+  device_removed_loop.Run();
+
+  ASSERT_FALSE(context->GetDeviceInfo(kTestGuid));
+}

chrome/browser/hid/mock_hid_device_observer.h

@@ -18,6 +18,7 @@ class MockHidDeviceObserver : public HidChooserContext::DeviceObserver {
 
   MOCK_METHOD1(OnDeviceAdded, void(const device::mojom::HidDeviceInfo&));
   MOCK_METHOD1(OnDeviceRemoved, void(const device::mojom::HidDeviceInfo&));
+  MOCK_METHOD1(OnDeviceChanged, void(const device::mojom::HidDeviceInfo&));
   MOCK_METHOD0(OnHidManagerConnectionError, void());
   MOCK_METHOD0(OnHidChooserContextShutdown, void());
 };

chrome/browser/ui/hid/hid_chooser_controller.cc

@@ -8,6 +8,9 @@
 
 #include "base/bind.h"
 #include "base/command_line.h"
+#include "base/containers/contains.h"
+#include "base/ranges/algorithm.h"
+#include "base/stl_util.h"
 #include "chrome/browser/hid/hid_chooser_context.h"
 #include "chrome/browser/hid/hid_chooser_context_factory.h"
 #include "chrome/browser/hid/web_hid_histograms.h"
@@ -187,6 +190,25 @@ void HidChooserController::OnDeviceRemoved(
     view()->OnOptionRemoved(index);
 }
 
+void HidChooserController::OnDeviceChanged(
+    const device::mojom::HidDeviceInfo& device) {
+  bool has_chooser_item =
+      base::Contains(items_, PhysicalDeviceIdFromDeviceInfo(device));
+  if (!DisplayDevice(device)) {
+    if (has_chooser_item)
+      OnDeviceRemoved(device);
+    return;
+  }
+
+  if (!has_chooser_item) {
+    OnDeviceAdded(device);
+    return;
+  }
+
+  // Update the item to replace the old device info with |device|.
+  UpdateDeviceInfo(device);
+}
+
 void HidChooserController::OnHidManagerConnectionError() {
   observer_.RemoveAll();
 }
@@ -318,3 +340,17 @@ bool HidChooserController::RemoveDeviceInfo(
   base::Erase(items_, id);
   return true;
 }
+
+void HidChooserController::UpdateDeviceInfo(
+    const device::mojom::HidDeviceInfo& device) {
+  auto id = PhysicalDeviceIdFromDeviceInfo(device);
+  auto physical_device_it = device_map_.find(id);
+  DCHECK(physical_device_it != device_map_.end());
+  auto& device_infos = physical_device_it->second;
+  auto device_it = base::ranges::find_if(
+      device_infos, [&device](const device::mojom::HidDeviceInfoPtr& d) {
+        return d->guid == device.guid;
+      });
+  DCHECK(device_it != device_infos.end());
+  *device_it = device.Clone();
+}

chrome/browser/ui/hid/hid_chooser_controller.h

@@ -59,6 +59,8 @@ class HidChooserController : public ChooserController,
   void OnDeviceAdded(const device::mojom::HidDeviceInfo& device_info) override;
   void OnDeviceRemoved(
       const device::mojom::HidDeviceInfo& device_info) override;
+  void OnDeviceChanged(
+      const device::mojom::HidDeviceInfo& device_info) override;
   void OnHidManagerConnectionError() override;
   void OnHidChooserContextShutdown() override;
 
@@ -78,6 +80,10 @@ class HidChooserController : public ChooserController,
   // is not in the chooser item. Returns true if an item was removed.
   bool RemoveDeviceInfo(const device::mojom::HidDeviceInfo& device_info);
 
+  // Update the information for the device described by |device_info| in the
+  // |device_map_|.
+  void UpdateDeviceInfo(const device::mojom::HidDeviceInfo& device_info);
+
   std::vector<blink::mojom::HidDeviceFilterPtr> filters_;
   content::HidChooser::Callback callback_;
   const url::Origin origin_;