refactor: partition HidDelegate observers by browser context (electro…

…n#40215)
felixrieseberg · Oct 25, 2023 · 732fe95 · 732fe95
1 parent 875848e
commit 732fe95
Show file tree

Hide file tree

Showing 4 changed files with 121 additions and 71 deletions.
diff --git a/shell/browser/hid/electron_hid_delegate.cc b/shell/browser/hid/electron_hid_delegate.cc
@@ -9,6 +9,7 @@
 
 #include "base/command_line.h"
 #include "base/containers/contains.h"
+#include "base/scoped_observation.h"
 #include "chrome/common/chrome_features.h"
 #include "content/public/browser/web_contents.h"
 #include "electron/buildflags/buildflags.h"
@@ -36,6 +37,70 @@ electron::HidChooserContext* GetChooserContext(
 
 namespace electron {
 
+// Manages the HidDelegate observers for a single browser context.
+class ElectronHidDelegate::ContextObservation
+    : public HidChooserContext::DeviceObserver {
+ public:
+  ContextObservation(ElectronHidDelegate* parent,
+                     content::BrowserContext* browser_context)
+      : parent_(parent), browser_context_(browser_context) {
+    auto* chooser_context = GetChooserContext(browser_context_);
+    device_observation_.Observe(chooser_context);
+  }
+
+  ContextObservation(ContextObservation&) = delete;
+  ContextObservation& operator=(ContextObservation&) = delete;
+  ~ContextObservation() override = default;
+
+  // HidChooserContext::DeviceObserver:
+  void OnDeviceAdded(const device::mojom::HidDeviceInfo& device_info) override {
+    for (auto& observer : observer_list_)
+      observer.OnDeviceAdded(device_info);
+  }
+
+  void OnDeviceRemoved(
+      const device::mojom::HidDeviceInfo& device_info) override {
+    for (auto& observer : observer_list_)
+      observer.OnDeviceRemoved(device_info);
+  }
+
+  void OnDeviceChanged(
+      const device::mojom::HidDeviceInfo& device_info) override {
+    for (auto& observer : observer_list_)
+      observer.OnDeviceChanged(device_info);
+  }
+
+  void OnHidManagerConnectionError() override {
+    for (auto& observer : observer_list_)
+      observer.OnHidManagerConnectionError();
+  }
+
+  void OnHidChooserContextShutdown() override {
+    parent_->observations_.erase(browser_context_);
+    // Return since `this` is now deleted.
+  }
+
+  void AddObserver(content::HidDelegate::Observer* observer) {
+    observer_list_.AddObserver(observer);
+  }
+
+  void RemoveObserver(content::HidDelegate::Observer* observer) {
+    observer_list_.RemoveObserver(observer);
+  }
+
+ private:
+  // Safe because `parent_` owns `this`.
+  const raw_ptr<ElectronHidDelegate> parent_;
+
+  // Safe because `this` is destroyed when the context is lost.
+  const raw_ptr<content::BrowserContext> browser_context_;
+
+  base::ScopedObservation<HidChooserContext, HidChooserContext::DeviceObserver>
+      device_observation_{this};
+
+  base::ObserverList<content::HidDelegate::Observer> observer_list_;
+};
+
 ElectronHidDelegate::ElectronHidDelegate() = default;
 
 ElectronHidDelegate::~ElectronHidDelegate() = default;
@@ -45,11 +110,11 @@ std::unique_ptr<content::HidChooser> ElectronHidDelegate::RunChooser(
     std::vector<blink::mojom::HidDeviceFilterPtr> filters,
     std::vector<blink::mojom::HidDeviceFilterPtr> exclusion_filters,
     content::HidChooser::Callback callback) {
-  DCHECK(render_frame_host);
-  auto* chooser_context =
-      GetChooserContext(render_frame_host->GetBrowserContext());
-  if (!device_observation_.IsObserving())
-    device_observation_.Observe(chooser_context);
+  auto* browser_context = render_frame_host->GetBrowserContext();
+
+  // Start observing HidChooserContext for permission and device events.
+  GetContextObserver(browser_context);
+  DCHECK(base::Contains(observations_, browser_context));
 
   HidChooserController* controller = ControllerForFrame(render_frame_host);
   if (controller) {
@@ -101,16 +166,14 @@ device::mojom::HidManager* ElectronHidDelegate::GetHidManager(
 
 void ElectronHidDelegate::AddObserver(content::BrowserContext* browser_context,
                                       Observer* observer) {
-  observer_list_.AddObserver(observer);
-  auto* chooser_context = GetChooserContext(browser_context);
-  if (!device_observation_.IsObserving())
-    device_observation_.Observe(chooser_context);
+  GetContextObserver(browser_context)->AddObserver(observer);
 }
 
 void ElectronHidDelegate::RemoveObserver(
     content::BrowserContext* browser_context,
     content::HidDelegate::Observer* observer) {
-  observer_list_.RemoveObserver(observer);
+  DCHECK(base::Contains(observations_, browser_context));
+  GetContextObserver(browser_context)->RemoveObserver(observer);
 }
 
 const device::mojom::HidDeviceInfo* ElectronHidDelegate::GetDeviceInfo(
@@ -140,33 +203,14 @@ bool ElectronHidDelegate::IsServiceWorkerAllowedForOrigin(
   return false;
 }
 
-void ElectronHidDelegate::OnDeviceAdded(
-    const device::mojom::HidDeviceInfo& device_info) {
-  for (auto& observer : observer_list_)
-    observer.OnDeviceAdded(device_info);
-}
-
-void ElectronHidDelegate::OnDeviceRemoved(
-    const device::mojom::HidDeviceInfo& device_info) {
-  for (auto& observer : observer_list_)
-    observer.OnDeviceRemoved(device_info);
-}
-
-void ElectronHidDelegate::OnDeviceChanged(
-    const device::mojom::HidDeviceInfo& device_info) {
-  for (auto& observer : observer_list_)
-    observer.OnDeviceChanged(device_info);
-}
-
-void ElectronHidDelegate::OnHidManagerConnectionError() {
-  device_observation_.Reset();
-
-  for (auto& observer : observer_list_)
-    observer.OnHidManagerConnectionError();
-}
-
-void ElectronHidDelegate::OnHidChooserContextShutdown() {
-  device_observation_.Reset();
+ElectronHidDelegate::ContextObservation*
+ElectronHidDelegate::GetContextObserver(
+    content::BrowserContext* browser_context) {
+  if (!base::Contains(observations_, browser_context)) {
+    observations_.emplace(browser_context, std::make_unique<ContextObservation>(
+                                               this, browser_context));
+  }
+  return observations_[browser_context].get();
 }
 
 HidChooserController* ElectronHidDelegate::ControllerForFrame(

diff --git a/shell/browser/hid/electron_hid_delegate.h b/shell/browser/hid/electron_hid_delegate.h
@@ -10,17 +10,24 @@
 #include <unordered_map>
 #include <vector>
 
-#include "base/observer_list.h"
-#include "base/scoped_observation.h"
+#include "base/containers/flat_map.h"
+#include "content/public/browser/hid_chooser.h"
 #include "content/public/browser/hid_delegate.h"
+#include "services/device/public/mojom/hid.mojom-forward.h"
 #include "shell/browser/hid/hid_chooser_context.h"
+#include "third_party/blink/public/mojom/hid/hid.mojom-forward.h"
+#include "url/origin.h"
+
+namespace content {
+class BrowserContext;
+class RenderFrameHost;
+}  // namespace content
 
 namespace electron {
 
 class HidChooserController;
 
-class ElectronHidDelegate : public content::HidDelegate,
-                            public HidChooserContext::DeviceObserver {
+class ElectronHidDelegate : public content::HidDelegate {
  public:
   ElectronHidDelegate();
   ElectronHidDelegate(ElectronHidDelegate&) = delete;
@@ -54,13 +61,6 @@ class ElectronHidDelegate : public content::HidDelegate,
   bool IsFidoAllowedForOrigin(content::BrowserContext* browser_context,
                               const url::Origin& origin) override;
   bool IsServiceWorkerAllowedForOrigin(const url::Origin& origin) override;
-
-  // 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;
   void IncrementConnectionCount(content::BrowserContext* browser_context,
                                 const url::Origin& origin) override {}
   void DecrementConnectionCount(content::BrowserContext* browser_context,
@@ -69,6 +69,14 @@ class ElectronHidDelegate : public content::HidDelegate,
   void DeleteControllerForFrame(content::RenderFrameHost* render_frame_host);
 
  private:
+  class ContextObservation;
+
+  ContextObservation* GetContextObserver(
+      content::BrowserContext* browser_context);
+
+  base::flat_map<content::BrowserContext*, std::unique_ptr<ContextObservation>>
+      observations_;
+
   HidChooserController* ControllerForFrame(
       content::RenderFrameHost* render_frame_host);
 
@@ -78,10 +86,6 @@ class ElectronHidDelegate : public content::HidDelegate,
       std::vector<blink::mojom::HidDeviceFilterPtr> exclusion_filters,
       content::HidChooser::Callback callback);
 
-  base::ScopedObservation<HidChooserContext, HidChooserContext::DeviceObserver>
-      device_observation_{this};
-  base::ObserverList<content::HidDelegate::Observer> observer_list_;
-
   std::unordered_map<content::RenderFrameHost*,
                      std::unique_ptr<HidChooserController>>
       controller_map_;
@@ -91,23 +95,4 @@ class ElectronHidDelegate : public content::HidDelegate,
 
 }  // namespace electron
 
-namespace base {
-
-template <>
-struct ScopedObservationTraits<electron::HidChooserContext,
-                               electron::HidChooserContext::DeviceObserver> {
-  static void AddObserver(
-      electron::HidChooserContext* source,
-      electron::HidChooserContext::DeviceObserver* observer) {
-    source->AddDeviceObserver(observer);
-  }
-  static void RemoveObserver(
-      electron::HidChooserContext* source,
-      electron::HidChooserContext::DeviceObserver* observer) {
-    source->RemoveDeviceObserver(observer);
-  }
-};
-
-}  // namespace base
-
 #endif  // ELECTRON_SHELL_BROWSER_HID_ELECTRON_HID_DELEGATE_H_
diff --git a/shell/browser/hid/hid_chooser_context.h b/shell/browser/hid/hid_chooser_context.h
@@ -14,6 +14,7 @@
 #include "base/memory/raw_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
+#include "base/scoped_observation_traits.h"
 #include "base/unguessable_token.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "content/public/browser/web_contents.h"
@@ -138,4 +139,23 @@ class HidChooserContext : public KeyedService,
 
 }  // namespace electron
 
+namespace base {
+
+template <>
+struct ScopedObservationTraits<electron::HidChooserContext,
+                               electron::HidChooserContext::DeviceObserver> {
+  static void AddObserver(
+      electron::HidChooserContext* source,
+      electron::HidChooserContext::DeviceObserver* observer) {
+    source->AddDeviceObserver(observer);
+  }
+  static void RemoveObserver(
+      electron::HidChooserContext* source,
+      electron::HidChooserContext::DeviceObserver* observer) {
+    source->RemoveDeviceObserver(observer);
+  }
+};
+
+}  // namespace base
+
 #endif  // ELECTRON_SHELL_BROWSER_HID_HID_CHOOSER_CONTEXT_H_
diff --git a/shell/browser/hid/hid_chooser_controller.h b/shell/browser/hid/hid_chooser_controller.h
@@ -10,6 +10,7 @@
 #include <vector>
 
 #include "base/memory/weak_ptr.h"
+#include "base/scoped_observation.h"
 #include "content/public/browser/global_routing_id.h"
 #include "content/public/browser/hid_chooser.h"
 #include "content/public/browser/web_contents.h"