[serial] Add observer interface for port add/remove events

This change adds an observer interface to the C++ and Mojo interfaces
for enumerating serial ports and wires it up to the serial port chooser
UI so that it updates in real time as devices are added and removed from
the system.

Bug: 918216
Change-Id: I9d0c3791a4dbb0093c1811d48b1a732bdf5c71fe
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2094624
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Ovidio de Jesús Ruiz-Henríquez <odejesush@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#751837}

chrome/browser/chooser_controller/mock_chooser_controller_view.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/chooser_controller/mock_chooser_controller_view.h"
+
+MockChooserControllerView::MockChooserControllerView() = default;
+
+MockChooserControllerView::~MockChooserControllerView() = default;

chrome/browser/chooser_controller/mock_chooser_controller_view.h

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_CHOOSER_CONTROLLER_MOCK_CHOOSER_CONTROLLER_VIEW_H_
+#define CHROME_BROWSER_CHOOSER_CONTROLLER_MOCK_CHOOSER_CONTROLLER_VIEW_H_
+
+#include "chrome/browser/chooser_controller/chooser_controller.h"
+
+#include "testing/gmock/include/gmock/gmock.h"
+
+class MockChooserControllerView : public ChooserController::View {
+ public:
+  MockChooserControllerView();
+  MockChooserControllerView(MockChooserControllerView&) = delete;
+  MockChooserControllerView& operator=(MockChooserControllerView&) = delete;
+  ~MockChooserControllerView() override;
+
+  // ChooserController::View
+  MOCK_METHOD0(OnOptionsInitialized, void());
+  MOCK_METHOD1(OnOptionAdded, void(size_t index));
+  MOCK_METHOD1(OnOptionRemoved, void(size_t index));
+  MOCK_METHOD1(OnOptionUpdated, void(size_t index));
+  MOCK_METHOD1(OnAdapterEnabledChanged, void(bool enabled));
+  MOCK_METHOD1(OnRefreshStateChanged, void(bool enabled));
+};
+
+#endif  // CHROME_BROWSER_CHOOSER_CONTROLLER_MOCK_CHOOSER_CONTROLLER_VIEW_H_

chrome/browser/serial/serial_chooser_context.cc

@@ -174,15 +174,54 @@ device::mojom::SerialPortManager* SerialChooserContext::GetPortManager() {
   return port_manager_.get();
 }
 
+void SerialChooserContext::AddPortObserver(PortObserver* observer) {
+  port_observer_list_.AddObserver(observer);
+}
+
+void SerialChooserContext::RemovePortObserver(PortObserver* observer) {
+  port_observer_list_.RemoveObserver(observer);
+}
+
 void SerialChooserContext::SetPortManagerForTesting(
     mojo::PendingRemote<device::mojom::SerialPortManager> manager) {
   SetUpPortManagerConnection(std::move(manager));
 }
 
+void SerialChooserContext::FlushPortManagerConnectionForTesting() {
+  port_manager_.FlushForTesting();
+}
+
 base::WeakPtr<SerialChooserContext> SerialChooserContext::AsWeakPtr() {
   return weak_factory_.GetWeakPtr();
 }
 
+void SerialChooserContext::OnPortAdded(device::mojom::SerialPortInfoPtr port) {
+  for (auto& observer : port_observer_list_)
+    observer.OnPortAdded(*port);
+}
+
+void SerialChooserContext::OnPortRemoved(
+    device::mojom::SerialPortInfoPtr port) {
+  for (auto& observer : port_observer_list_)
+    observer.OnPortRemoved(*port);
+
+  std::vector<std::pair<url::Origin, url::Origin>> revoked_url_pairs;
+  for (auto& map_entry : ephemeral_ports_) {
+    std::set<base::UnguessableToken>& ports = map_entry.second;
+    if (ports.erase(port->token) > 0)
+      revoked_url_pairs.push_back(map_entry.first);
+  }
+
+  port_info_.erase(port->token);
+
+  for (auto& observer : permission_observer_list_) {
+    observer.OnChooserObjectPermissionChanged(guard_content_settings_type_,
+                                              data_content_settings_type_);
+    for (const auto& url_pair : revoked_url_pairs)
+      observer.OnPermissionRevoked(url_pair.first, url_pair.second);
+  }
+}
+
 void SerialChooserContext::EnsurePortManagerConnection() {
   if (port_manager_)
     return;
@@ -199,9 +238,14 @@ void SerialChooserContext::SetUpPortManagerConnection(
   port_manager_.set_disconnect_handler(
       base::BindOnce(&SerialChooserContext::OnPortManagerConnectionError,
                      base::Unretained(this)));
+
+  port_manager_->SetClient(client_receiver_.BindNewPipeAndPassRemote());
 }
 
 void SerialChooserContext::OnPortManagerConnectionError() {
+  port_manager_.reset();
+  client_receiver_.reset();
+
   port_info_.clear();
 
   std::vector<std::pair<url::Origin, url::Origin>> revoked_origins;

chrome/browser/serial/serial_chooser_context.h

@@ -28,8 +28,16 @@ namespace base {
 class Value;
 }
 
-class SerialChooserContext : public permissions::ChooserContextBase {
+class SerialChooserContext : public permissions::ChooserContextBase,
+                             public device::mojom::SerialPortManagerClient {
  public:
+  class PortObserver : public base::CheckedObserver {
+   public:
+    virtual void OnPortAdded(const device::mojom::SerialPortInfo& port) = 0;
+    virtual void OnPortRemoved(const device::mojom::SerialPortInfo& port) = 0;
+    virtual void OnPortManagerConnectionError() = 0;
+  };
+
   explicit SerialChooserContext(Profile* profile);
   ~SerialChooserContext() override;
 
@@ -59,10 +67,18 @@ class SerialChooserContext : public permissions::ChooserContextBase {
 
   device::mojom::SerialPortManager* GetPortManager();
 
+  void AddPortObserver(PortObserver* observer);
+  void RemovePortObserver(PortObserver* observer);
+
   void SetPortManagerForTesting(
       mojo::PendingRemote<device::mojom::SerialPortManager> manager);
+  void FlushPortManagerConnectionForTesting();
   base::WeakPtr<SerialChooserContext> AsWeakPtr();
 
+  // SerialPortManagerClient implementation.
+  void OnPortAdded(device::mojom::SerialPortInfoPtr port) override;
+  void OnPortRemoved(device::mojom::SerialPortInfoPtr port) override;
+
  private:
   void EnsurePortManagerConnection();
   void SetUpPortManagerConnection(
@@ -85,6 +101,8 @@ class SerialChooserContext : public permissions::ChooserContextBase {
   std::map<base::UnguessableToken, base::Value> port_info_;
 
   mojo::Remote<device::mojom::SerialPortManager> port_manager_;
+  mojo::Receiver<device::mojom::SerialPortManagerClient> client_receiver_{this};
+  base::ObserverList<PortObserver> port_observer_list_;
 
   base::WeakPtrFactory<SerialChooserContext> weak_factory_{this};
 

chrome/browser/serial/serial_chooser_context_unittest.cc

@@ -5,35 +5,76 @@
 #include "chrome/browser/serial/serial_chooser_context.h"
 
 #include "base/run_loop.h"
+#include "base/scoped_observer.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/serial/serial_chooser_context_factory.h"
 #include "chrome/test/base/testing_profile.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/permissions/test/chooser_context_base_mock_permission_observer.h"
 #include "content/public/test/browser_task_environment.h"
+#include "services/device/public/cpp/test/fake_serial_port_manager.h"
 #include "services/device/public/mojom/serial.mojom.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace {
 
+class MockPortObserver : public SerialChooserContext::PortObserver {
+ public:
+  MockPortObserver() = default;
+  MockPortObserver(MockPortObserver&) = delete;
+  MockPortObserver& operator=(MockPortObserver&) = delete;
+  ~MockPortObserver() override = default;
+
+  MOCK_METHOD1(OnPortAdded, void(const device::mojom::SerialPortInfo&));
+  MOCK_METHOD1(OnPortRemoved, void(const device::mojom::SerialPortInfo&));
+  MOCK_METHOD0(OnPortManagerConnectionError, void());
+};
+
 class SerialChooserContextTest : public testing::Test {
  public:
-  SerialChooserContextTest() = default;
+  SerialChooserContextTest() {
+    mojo::PendingRemote<device::mojom::SerialPortManager> port_manager;
+    port_manager_.AddReceiver(port_manager.InitWithNewPipeAndPassReceiver());
+
+    context_ = SerialChooserContextFactory::GetForProfile(&profile_);
+    context_->SetPortManagerForTesting(std::move(port_manager));
+    scoped_permission_observer_.Add(context_);
+    scoped_port_observer_.Add(context_);
+
+    // Ensure |context_| is ready to receive SerialPortManagerClient messages.
+    context_->FlushPortManagerConnectionForTesting();
+  }
+
   ~SerialChooserContextTest() override = default;
 
-  Profile* profile() { return &profile_; }
-  permissions::MockPermissionObserver& observer() { return mock_observer_; }
+  // Disallow copy and assignment.
+  SerialChooserContextTest(SerialChooserContextTest&) = delete;
+  SerialChooserContextTest& operator=(SerialChooserContextTest&) = delete;
 
-  SerialChooserContext* GetContext(Profile* profile) {
-    auto* context = SerialChooserContextFactory::GetForProfile(profile);
-    context->AddObserver(&mock_observer_);
-    return context;
+  device::FakeSerialPortManager& port_manager() { return port_manager_; }
+  Profile* profile() { return &profile_; }
+  SerialChooserContext* context() { return context_; }
+  permissions::MockPermissionObserver& permission_observer() {
+    return permission_observer_;
   }
+  MockPortObserver& port_observer() { return port_observer_; }
 
  private:
   content::BrowserTaskEnvironment task_environment_;
+  device::FakeSerialPortManager port_manager_;
   TestingProfile profile_;
-  permissions::MockPermissionObserver mock_observer_;
+  SerialChooserContext* context_;
+  permissions::MockPermissionObserver permission_observer_;
+  ScopedObserver<permissions::ChooserContextBase,
+                 permissions::ChooserContextBase::PermissionObserver>
+      scoped_permission_observer_{&permission_observer_};
+  MockPortObserver port_observer_;
+  ScopedObserver<SerialChooserContext,
+                 SerialChooserContext::PortObserver,
+                 &SerialChooserContext::AddPortObserver,
+                 &SerialChooserContext::RemovePortObserver>
+      scoped_port_observer_{&port_observer_};
 };
 
 }  // namespace
@@ -44,22 +85,22 @@ TEST_F(SerialChooserContextTest, GrantAndRevokeEphemeralPermission) {
   auto port = device::mojom::SerialPortInfo::New();
   port->token = base::UnguessableToken::Create();
 
-  SerialChooserContext* context = GetContext(profile());
-  EXPECT_FALSE(context->HasPortPermission(origin, origin, *port));
+  EXPECT_FALSE(context()->HasPortPermission(origin, origin, *port));
 
-  EXPECT_CALL(observer(), OnChooserObjectPermissionChanged(
-                              ContentSettingsType::SERIAL_GUARD,
-                              ContentSettingsType::SERIAL_CHOOSER_DATA));
+  EXPECT_CALL(permission_observer(),
+              OnChooserObjectPermissionChanged(
+                  ContentSettingsType::SERIAL_GUARD,
+                  ContentSettingsType::SERIAL_CHOOSER_DATA));
 
-  context->GrantPortPermission(origin, origin, *port);
-  EXPECT_TRUE(context->HasPortPermission(origin, origin, *port));
+  context()->GrantPortPermission(origin, origin, *port);
+  EXPECT_TRUE(context()->HasPortPermission(origin, origin, *port));
 
   std::vector<std::unique_ptr<permissions::ChooserContextBase::Object>>
-      origin_objects = context->GetGrantedObjects(origin, origin);
+      origin_objects = context()->GetGrantedObjects(origin, origin);
   ASSERT_EQ(1u, origin_objects.size());
 
   std::vector<std::unique_ptr<permissions::ChooserContextBase::Object>>
-      objects = context->GetAllGrantedObjects();
+      objects = context()->GetAllGrantedObjects();
   ASSERT_EQ(1u, objects.size());
   EXPECT_EQ(origin.GetURL(), objects[0]->requesting_origin);
   EXPECT_EQ(origin.GetURL(), objects[0]->embedding_origin);
@@ -68,16 +109,52 @@ TEST_F(SerialChooserContextTest, GrantAndRevokeEphemeralPermission) {
             objects[0]->source);
   EXPECT_FALSE(objects[0]->incognito);
 
-  EXPECT_CALL(observer(), OnChooserObjectPermissionChanged(
-                              ContentSettingsType::SERIAL_GUARD,
-                              ContentSettingsType::SERIAL_CHOOSER_DATA));
-  EXPECT_CALL(observer(), OnPermissionRevoked(origin, origin));
+  EXPECT_CALL(permission_observer(),
+              OnChooserObjectPermissionChanged(
+                  ContentSettingsType::SERIAL_GUARD,
+                  ContentSettingsType::SERIAL_CHOOSER_DATA));
+  EXPECT_CALL(permission_observer(), OnPermissionRevoked(origin, origin));
 
-  context->RevokeObjectPermission(origin, origin, objects[0]->value);
-  EXPECT_FALSE(context->HasPortPermission(origin, origin, *port));
-  origin_objects = context->GetGrantedObjects(origin, origin);
+  context()->RevokeObjectPermission(origin, origin, objects[0]->value);
+  EXPECT_FALSE(context()->HasPortPermission(origin, origin, *port));
+  origin_objects = context()->GetGrantedObjects(origin, origin);
+  EXPECT_EQ(0u, origin_objects.size());
+  objects = context()->GetAllGrantedObjects();
+  EXPECT_EQ(0u, objects.size());
+}
+
+TEST_F(SerialChooserContextTest, EphemeralPermissionRevokedOnDisconnect) {
+  const auto origin = url::Origin::Create(GURL("https://google.com"));
+
+  auto port = device::mojom::SerialPortInfo::New();
+  port->token = base::UnguessableToken::Create();
+  port_manager().AddPort(port.Clone());
+
+  context()->GrantPortPermission(origin, origin, *port);
+  EXPECT_TRUE(context()->HasPortPermission(origin, origin, *port));
+
+  EXPECT_CALL(permission_observer(),
+              OnChooserObjectPermissionChanged(
+                  ContentSettingsType::SERIAL_GUARD,
+                  ContentSettingsType::SERIAL_CHOOSER_DATA));
+  EXPECT_CALL(permission_observer(), OnPermissionRevoked(origin, origin));
+
+  port_manager().RemovePort(port->token);
+  {
+    base::RunLoop run_loop;
+    EXPECT_CALL(port_observer(), OnPortRemoved(testing::_))
+        .WillOnce(
+            testing::Invoke([&](const device::mojom::SerialPortInfo& info) {
+              EXPECT_EQ(port->token, info.token);
+              EXPECT_TRUE(context()->HasPortPermission(origin, origin, info));
+              run_loop.Quit();
+            }));
+    run_loop.Run();
+  }
+  EXPECT_FALSE(context()->HasPortPermission(origin, origin, *port));
+  auto origin_objects = context()->GetGrantedObjects(origin, origin);
   EXPECT_EQ(0u, origin_objects.size());
-  objects = context->GetAllGrantedObjects();
+  auto objects = context()->GetAllGrantedObjects();
   EXPECT_EQ(0u, objects.size());
 }
 
@@ -87,21 +164,20 @@ TEST_F(SerialChooserContextTest, GuardPermission) {
   auto port = device::mojom::SerialPortInfo::New();
   port->token = base::UnguessableToken::Create();
 
-  SerialChooserContext* context = GetContext(profile());
-  context->GrantPortPermission(origin, origin, *port);
-  EXPECT_TRUE(context->HasPortPermission(origin, origin, *port));
+  context()->GrantPortPermission(origin, origin, *port);
+  EXPECT_TRUE(context()->HasPortPermission(origin, origin, *port));
 
   auto* map = HostContentSettingsMapFactory::GetForProfile(profile());
   map->SetContentSettingDefaultScope(origin.GetURL(), origin.GetURL(),
                                      ContentSettingsType::SERIAL_GUARD,
                                      std::string(), CONTENT_SETTING_BLOCK);
-  EXPECT_FALSE(context->HasPortPermission(origin, origin, *port));
+  EXPECT_FALSE(context()->HasPortPermission(origin, origin, *port));
 
   std::vector<std::unique_ptr<permissions::ChooserContextBase::Object>>
-      objects = context->GetGrantedObjects(origin, origin);
+      objects = context()->GetGrantedObjects(origin, origin);
   EXPECT_EQ(0u, objects.size());
 
   std::vector<std::unique_ptr<permissions::ChooserContextBase::Object>>
-      all_origin_objects = context->GetAllGrantedObjects();
+      all_origin_objects = context()->GetAllGrantedObjects();
   EXPECT_EQ(0u, all_origin_objects.size());
 }

chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc

@@ -7,32 +7,13 @@
 #include "base/bind.h"
 #include "base/macros.h"
 #include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/chooser_controller/mock_chooser_controller_view.h"
 #include "chrome/browser/ui/bluetooth/bluetooth_chooser_controller.h"
 #include "chrome/grit/generated_resources.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/l10n/l10n_util.h"
 
-namespace {
-
-class MockBluetoothChooserView : public ChooserController::View {
- public:
-  MockBluetoothChooserView() {}
-
-  // ChooserController::View:
-  MOCK_METHOD0(OnOptionsInitialized, void());
-  MOCK_METHOD1(OnOptionAdded, void(size_t index));
-  MOCK_METHOD1(OnOptionRemoved, void(size_t index));
-  MOCK_METHOD1(OnOptionUpdated, void(size_t index));
-  MOCK_METHOD1(OnAdapterEnabledChanged, void(bool enabled));
-  MOCK_METHOD1(OnRefreshStateChanged, void(bool enabled));
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(MockBluetoothChooserView);
-};
-
-}  // namespace
-
 class BluetoothChooserControllerTest : public testing::Test {
  public:
   BluetoothChooserControllerTest()
@@ -51,7 +32,7 @@ class BluetoothChooserControllerTest : public testing::Test {
   }
 
   BluetoothChooserController bluetooth_chooser_controller_;
-  MockBluetoothChooserView mock_bluetooth_chooser_view_;
+  MockChooserControllerView mock_bluetooth_chooser_view_;
   content::BluetoothChooser::Event last_event_;
   std::string last_device_id_;
 

chrome/browser/ui/bluetooth/bluetooth_scanning_prompt_controller_unittest.cc

@@ -7,32 +7,13 @@
 #include "base/bind.h"
 #include "base/macros.h"
 #include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/chooser_controller/mock_chooser_controller_view.h"
 #include "chrome/browser/ui/bluetooth/bluetooth_scanning_prompt_controller.h"
 #include "chrome/grit/generated_resources.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/l10n/l10n_util.h"
 
-namespace {
-
-class MockBluetoothScanningPromptView : public ChooserController::View {
- public:
-  MockBluetoothScanningPromptView() {}
-
-  // ChooserController::View:
-  MOCK_METHOD0(OnOptionsInitialized, void());
-  MOCK_METHOD1(OnOptionAdded, void(size_t index));
-  MOCK_METHOD1(OnOptionRemoved, void(size_t index));
-  MOCK_METHOD1(OnOptionUpdated, void(size_t index));
-  MOCK_METHOD1(OnAdapterEnabledChanged, void(bool enabled));
-  MOCK_METHOD1(OnRefreshStateChanged, void(bool enabled));
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(MockBluetoothScanningPromptView);
-};
-
-}  // namespace
-
 class BluetoothScanningPromptControllerTest : public testing::Test {
  public:
   BluetoothScanningPromptControllerTest()
@@ -52,7 +33,7 @@ class BluetoothScanningPromptControllerTest : public testing::Test {
   }
 
   BluetoothScanningPromptController bluetooth_scanning_prompt_controller_;
-  MockBluetoothScanningPromptView mock_bluetooth_scanning_prompt_view_;
+  MockChooserControllerView mock_bluetooth_scanning_prompt_view_;
   content::BluetoothScanningPrompt::Event last_event_;
 
  private:

chrome/browser/ui/serial/serial_chooser_controller.cc

@@ -11,7 +11,6 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/unguessable_token.h"
 #include "chrome/browser/profiles/profile.h"
-#include "chrome/browser/serial/serial_chooser_context.h"
 #include "chrome/browser/serial/serial_chooser_context_factory.h"
 #include "chrome/grit/generated_resources.h"
 #include "content/public/browser/web_contents.h"
@@ -39,6 +38,7 @@ SerialChooserController::SerialChooserController(
 
   chooser_context_->GetPortManager()->GetDevices(base::BindOnce(
       &SerialChooserController::OnGetDevices, weak_factory_.GetWeakPtr()));
+  observer_.Add(chooser_context_.get());
 }
 
 SerialChooserController::~SerialChooserController() {
@@ -110,6 +110,30 @@ void SerialChooserController::OpenHelpCenterUrl() const {
   NOTIMPLEMENTED();
 }
 
+void SerialChooserController::OnPortAdded(
+    const device::mojom::SerialPortInfo& port) {
+  ports_.push_back(port.Clone());
+  if (view())
+    view()->OnOptionAdded(ports_.size() - 1);
+}
+
+void SerialChooserController::OnPortRemoved(
+    const device::mojom::SerialPortInfo& port) {
+  const auto it = std::find_if(
+      ports_.begin(), ports_.end(),
+      [&port](const auto& ptr) { return ptr->token == port.token; });
+  if (it != ports_.end()) {
+    const size_t index = it - ports_.begin();
+    ports_.erase(it);
+    if (view())
+      view()->OnOptionRemoved(index);
+  }
+}
+
+void SerialChooserController::OnPortManagerConnectionError() {
+  observer_.RemoveAll();
+}
+
 void SerialChooserController::OnGetDevices(
     std::vector<device::mojom::SerialPortInfoPtr> ports) {
   for (auto& port : ports) {

chrome/browser/ui/serial/serial_chooser_controller.h

@@ -10,8 +10,10 @@
 
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "base/scoped_observer.h"
 #include "base/strings/string16.h"
 #include "chrome/browser/chooser_controller/chooser_controller.h"
+#include "chrome/browser/serial/serial_chooser_context.h"
 #include "content/public/browser/serial_chooser.h"
 #include "services/device/public/mojom/serial.mojom-forward.h"
 #include "third_party/blink/public/mojom/serial/serial.mojom.h"
@@ -21,10 +23,10 @@ namespace content {
 class RenderFrameHost;
 }  // namespace content
 
-class SerialChooserContext;
-
 // SerialChooserController provides data for the Serial API permission prompt.
-class SerialChooserController final : public ChooserController {
+class SerialChooserController final
+    : public ChooserController,
+      public SerialChooserContext::PortObserver {
  public:
   SerialChooserController(
       content::RenderFrameHost* render_frame_host,
@@ -43,6 +45,11 @@ class SerialChooserController final : public ChooserController {
   void Close() override;
   void OpenHelpCenterUrl() const override;
 
+  // SerialChooserContext::PortObserver:
+  void OnPortAdded(const device::mojom::SerialPortInfo& port) override;
+  void OnPortRemoved(const device::mojom::SerialPortInfo& port) override;
+  void OnPortManagerConnectionError() override;
+
  private:
   void OnGetDevices(std::vector<device::mojom::SerialPortInfoPtr> ports);
   bool FilterMatchesAny(const device::mojom::SerialPortInfo& port) const;
@@ -53,6 +60,12 @@ class SerialChooserController final : public ChooserController {
   url::Origin embedding_origin_;
 
   base::WeakPtr<SerialChooserContext> chooser_context_;
+  ScopedObserver<SerialChooserContext,
+                 SerialChooserContext::PortObserver,
+                 &SerialChooserContext::AddPortObserver,
+                 &SerialChooserContext::RemovePortObserver>
+      observer_{this};
+
   std::vector<device::mojom::SerialPortInfoPtr> ports_;
 
   base::WeakPtrFactory<SerialChooserController> weak_factory_{this};

chrome/browser/ui/serial/serial_chooser_controller_unittest.cc

@@ -4,7 +4,12 @@
 
 #include "chrome/browser/ui/serial/serial_chooser_controller.h"
 
+#include <memory>
+#include <utility>
+
+#include "base/strings/utf_string_conversions.h"
 #include "base/test/bind_test_util.h"
+#include "chrome/browser/chooser_controller/mock_chooser_controller_view.h"
 #include "chrome/browser/serial/serial_chooser_context.h"
 #include "chrome/browser/serial/serial_chooser_context_factory.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
@@ -16,6 +21,9 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/blink/public/mojom/serial/serial.mojom.h"
 
+using testing::_;
+using testing::Invoke;
+
 class SerialChooserControllerTest : public ChromeRenderViewHostTestHarness {
  public:
   void SetUp() override {
@@ -27,6 +35,8 @@ class SerialChooserControllerTest : public ChromeRenderViewHostTestHarness {
         ->SetPortManagerForTesting(std::move(port_manager));
   }
 
+  device::FakeSerialPortManager& port_manager() { return port_manager_; }
+
  private:
   device::FakeSerialPortManager port_manager_;
 };
@@ -54,3 +64,71 @@ TEST_F(SerialChooserControllerTest, GetPortsLateResponse) {
   // should be run.
   EXPECT_TRUE(callback_run);
 }
+
+TEST_F(SerialChooserControllerTest, PortsAddedAndRemoved) {
+  std::vector<blink::mojom::SerialPortFilterPtr> filters;
+  auto controller = std::make_unique<SerialChooserController>(
+      main_rfh(), std::move(filters), base::DoNothing());
+
+  MockChooserControllerView view;
+  controller->set_view(&view);
+
+  {
+    base::RunLoop run_loop;
+    EXPECT_CALL(view, OnOptionsInitialized).WillOnce(Invoke([&] {
+      run_loop.Quit();
+    }));
+    run_loop.Run();
+  }
+  EXPECT_EQ(0u, controller->NumOptions());
+
+  auto port = device::mojom::SerialPortInfo::New();
+  port->token = base::UnguessableToken::Create();
+  port->display_name = "Test Port 1";
+  port->path = base::FilePath(FILE_PATH_LITERAL("/dev/ttyS0"));
+  base::UnguessableToken port1_token = port->token;
+  port_manager().AddPort(std::move(port));
+  {
+    base::RunLoop run_loop;
+    EXPECT_CALL(view, OnOptionAdded(_)).WillOnce(Invoke([&](size_t index) {
+      EXPECT_EQ(0u, index);
+      run_loop.Quit();
+    }));
+    run_loop.Run();
+  }
+  EXPECT_EQ(1u, controller->NumOptions());
+  EXPECT_EQ(base::ASCIIToUTF16("Test Port 1 (ttyS0)"),
+            controller->GetOption(0));
+
+  port = device::mojom::SerialPortInfo::New();
+  port->token = base::UnguessableToken::Create();
+  port->display_name = "Test Port 2";
+  port->path = base::FilePath(FILE_PATH_LITERAL("/dev/ttyS1"));
+  port_manager().AddPort(std::move(port));
+  {
+    base::RunLoop run_loop;
+    EXPECT_CALL(view, OnOptionAdded(_)).WillOnce(Invoke([&](size_t index) {
+      EXPECT_EQ(1u, index);
+      run_loop.Quit();
+    }));
+    run_loop.Run();
+  }
+  EXPECT_EQ(2u, controller->NumOptions());
+  EXPECT_EQ(base::ASCIIToUTF16("Test Port 1 (ttyS0)"),
+            controller->GetOption(0));
+  EXPECT_EQ(base::ASCIIToUTF16("Test Port 2 (ttyS1)"),
+            controller->GetOption(1));
+
+  port_manager().RemovePort(port1_token);
+  {
+    base::RunLoop run_loop;
+    EXPECT_CALL(view, OnOptionRemoved(_)).WillOnce(Invoke([&](size_t index) {
+      EXPECT_EQ(0u, index);
+      run_loop.Quit();
+    }));
+    run_loop.Run();
+  }
+  EXPECT_EQ(1u, controller->NumOptions());
+  EXPECT_EQ(base::ASCIIToUTF16("Test Port 2 (ttyS1)"),
+            controller->GetOption(0));
+}

chrome/browser/usb/usb_chooser_controller_unittest.cc

@@ -10,6 +10,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/run_loop.h"
 #include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/chooser_controller/mock_chooser_controller_view.h"
 #include "chrome/browser/usb/usb_chooser_context.h"
 #include "chrome/browser/usb/usb_chooser_context_factory.h"
 #include "chrome/browser/usb/usb_chooser_controller.h"
@@ -25,25 +26,7 @@
 #include "url/gurl.h"
 
 namespace {
-
 const char kDefaultTestUrl[] = "https://www.google.com/";
-
-class MockUsbChooserView : public ChooserController::View {
- public:
-  MockUsbChooserView() {}
-
-  // ChooserController::View:
-  MOCK_METHOD1(OnOptionAdded, void(size_t index));
-  MOCK_METHOD1(OnOptionRemoved, void(size_t index));
-  void OnOptionsInitialized() override {}
-  void OnOptionUpdated(size_t index) override {}
-  void OnAdapterEnabledChanged(bool enabled) override {}
-  void OnRefreshStateChanged(bool enabled) override {}
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(MockUsbChooserView);
-};
-
 }  //  namespace
 
 class UsbChooserControllerTest : public ChromeRenderViewHostTestHarness {
@@ -66,9 +49,9 @@ class UsbChooserControllerTest : public ChromeRenderViewHostTestHarness {
     UsbChooserContextFactory::GetForProfile(profile())
         ->SetDeviceManagerForTesting(std::move(device_manager));
 
-    usb_chooser_controller_.reset(new UsbChooserController(
-        main_rfh(), std::move(device_filters), std::move(callback)));
-    mock_usb_chooser_view_.reset(new MockUsbChooserView());
+    usb_chooser_controller_ = std::make_unique<UsbChooserController>(
+        main_rfh(), std::move(device_filters), std::move(callback));
+    mock_usb_chooser_view_ = std::make_unique<MockChooserControllerView>();
     usb_chooser_controller_->set_view(mock_usb_chooser_view_.get());
     // Make sure the device::mojom::UsbDeviceManager::SetClient() call has
     // been received.
@@ -85,7 +68,7 @@ class UsbChooserControllerTest : public ChromeRenderViewHostTestHarness {
 
   device::FakeUsbDeviceManager device_manager_;
   std::unique_ptr<UsbChooserController> usb_chooser_controller_;
-  std::unique_ptr<MockUsbChooserView> mock_usb_chooser_view_;
+  std::unique_ptr<MockChooserControllerView> mock_usb_chooser_view_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(UsbChooserControllerTest);

chrome/test/BUILD.gn

@@ -4239,6 +4239,8 @@ test("unit_tests") {
       "../browser/apps/intent_helper/apps_navigation_throttle_unittest.cc",
       "../browser/apps/intent_helper/intent_picker_auto_display_service_unittest.cc",
       "../browser/apps/intent_helper/page_transition_util_unittest.cc",
+      "../browser/chooser_controller/mock_chooser_controller_view.cc",
+      "../browser/chooser_controller/mock_chooser_controller_view.h",
       "../browser/devtools/devtools_file_system_indexer_unittest.cc",
       "../browser/devtools/devtools_file_watcher_unittest.cc",
       "../browser/devtools/devtools_ui_bindings_unittest.cc",

extensions/browser/api/serial/serial_apitest.cc

@@ -297,6 +297,11 @@ class FakeSerialPortManager : public device::mojom::SerialPortManager {
 
  private:
   // device::mojom::SerialPortManager methods:
+  void SetClient(mojo::PendingRemote<device::mojom::SerialPortManagerClient>
+                     remote) override {
+    NOTIMPLEMENTED();
+  }
+
   void GetDevices(GetDevicesCallback callback) override {
     std::vector<device::mojom::SerialPortInfoPtr> ports;
     for (const auto& port : ports_)

services/device/public/cpp/test/fake_serial_port_manager.cc

@@ -97,6 +97,24 @@ void FakeSerialPortManager::AddReceiver(
 void FakeSerialPortManager::AddPort(mojom::SerialPortInfoPtr port) {
   base::UnguessableToken token = port->token;
   ports_[token] = std::move(port);
+
+  for (auto& client : clients_)
+    client->OnPortAdded(ports_[token]->Clone());
+}
+
+void FakeSerialPortManager::RemovePort(base::UnguessableToken token) {
+  auto it = ports_.find(token);
+  DCHECK(it != ports_.end());
+  mojom::SerialPortInfoPtr info = std::move(it->second);
+  ports_.erase(it);
+
+  for (auto& client : clients_)
+    client->OnPortRemoved(info.Clone());
+}
+
+void FakeSerialPortManager::SetClient(
+    mojo::PendingRemote<mojom::SerialPortManagerClient> client) {
+  clients_.Add(std::move(client));
 }
 
 void FakeSerialPortManager::GetDevices(GetDevicesCallback callback) {

services/device/public/cpp/test/fake_serial_port_manager.h

@@ -12,6 +12,7 @@
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
 #include "mojo/public/cpp/bindings/receiver_set.h"
+#include "mojo/public/cpp/bindings/remote_set.h"
 #include "services/device/public/mojom/serial.mojom.h"
 
 namespace device {
@@ -23,8 +24,11 @@ class FakeSerialPortManager : public mojom::SerialPortManager {
 
   void AddReceiver(mojo::PendingReceiver<mojom::SerialPortManager> receiver);
   void AddPort(mojom::SerialPortInfoPtr port);
+  void RemovePort(base::UnguessableToken token);
 
   // mojom::SerialPortManager
+  void SetClient(
+      mojo::PendingRemote<mojom::SerialPortManagerClient> client) override;
   void GetDevices(GetDevicesCallback callback) override;
   void GetPort(
       const base::UnguessableToken& token,
@@ -34,6 +38,7 @@ class FakeSerialPortManager : public mojom::SerialPortManager {
  private:
   std::map<base::UnguessableToken, mojom::SerialPortInfoPtr> ports_;
   mojo::ReceiverSet<mojom::SerialPortManager> receivers_;
+  mojo::RemoteSet<mojom::SerialPortManagerClient> clients_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeSerialPortManager);
 };

services/device/public/mojom/serial.mojom

@@ -92,6 +92,11 @@ struct SerialPortControlSignals {
 
 // Discovers and enumerates serial devices available to the host.
 interface SerialPortManager {
+  // Associates an interface the port manager can used to notify the client of
+  // events such as the addition or removal of serial ports from the host.
+  SetClient(pending_remote<SerialPortManagerClient> client);
+
+  // Returns the list of serial ports currently available on the host.
   GetDevices() => (array<SerialPortInfo> devices);
 
   // Creates a SerialPort instance attached to the port represented by |token|.
@@ -102,6 +107,15 @@ interface SerialPortManager {
           pending_remote<SerialPortConnectionWatcher>? watcher);
 };
 
+// Client interface for SerialPortManager.
+interface SerialPortManagerClient {
+  // This message indicates that a port has been added to the host.
+  OnPortAdded(SerialPortInfo port_info);
+
+  // This message indicates that a port has been removed from the host.
+  OnPortRemoved(SerialPortInfo port_info);
+};
+
 // Performs asynchronous I/O on serial devices.
 interface SerialPort {
   // Initiates an Open of the device then returns result.

services/device/serial/fake_serial_device_enumerator.cc

@@ -6,7 +6,6 @@
 
 #include <utility>
 
-#include "base/stl_util.h"
 
 namespace device {
 
@@ -14,23 +13,20 @@ FakeSerialEnumerator::FakeSerialEnumerator() = default;
 
 FakeSerialEnumerator::~FakeSerialEnumerator() = default;
 
-bool FakeSerialEnumerator::AddDevicePath(const base::FilePath& path) {
-  if (base::Contains(device_paths_, path))
-    return false;
-
-  device_paths_.push_back(path);
-  return true;
+void FakeSerialEnumerator::AddDevicePath(const base::FilePath& path) {
+  auto port = mojom::SerialPortInfo::New();
+  port->token = base::UnguessableToken::Create();
+  port->path = path;
+  paths_[path] = port->token;
+  AddPort(std::move(port));
 }
 
-std::vector<mojom::SerialPortInfoPtr> FakeSerialEnumerator::GetDevices() {
-  std::vector<device::mojom::SerialPortInfoPtr> devices;
-  for (const auto& path : device_paths_) {
-    auto device = device::mojom::SerialPortInfo::New();
-    device->token = GetTokenFromPath(path);
-    device->path = path;
-    devices.push_back(std::move(device));
-  }
-  return devices;
+void FakeSerialEnumerator::RemoveDevicePath(const base::FilePath& path) {
+  auto it = paths_.find(path);
+  DCHECK(it != paths_.end());
+  base::UnguessableToken token = it->second;
+  paths_.erase(it);
+  RemovePort(token);
 }
 
 }  // namespace device

services/device/serial/fake_serial_device_enumerator.h

@@ -5,7 +5,7 @@
 #ifndef SERVICES_DEVICE_SERIAL_FAKE_SERIAL_DEVICE_ENUMERATOR_H_
 #define SERVICES_DEVICE_SERIAL_FAKE_SERIAL_DEVICE_ENUMERATOR_H_
 
-#include <vector>
+#include <map>
 
 #include "base/files/file_path.h"
 #include "base/macros.h"
@@ -18,12 +18,12 @@ class FakeSerialEnumerator : public SerialDeviceEnumerator {
   FakeSerialEnumerator();
   ~FakeSerialEnumerator() override;
 
-  bool AddDevicePath(const base::FilePath& path);
-
-  std::vector<mojom::SerialPortInfoPtr> GetDevices() override;
+  void AddDevicePath(const base::FilePath& path);
+  void RemoveDevicePath(const base::FilePath& path);
 
  private:
-  std::vector<base::FilePath> device_paths_;
+  std::map<base::FilePath, base::UnguessableToken> paths_;
+
   DISALLOW_COPY_AND_ASSIGN(FakeSerialEnumerator);
 };
 

services/device/serial/serial_device_enumerator.cc

@@ -37,25 +37,58 @@ SerialDeviceEnumerator::SerialDeviceEnumerator() = default;
 
 SerialDeviceEnumerator::~SerialDeviceEnumerator() = default;
 
+std::vector<mojom::SerialPortInfoPtr> SerialDeviceEnumerator::GetDevices() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  std::vector<mojom::SerialPortInfoPtr> ports;
+  ports.reserve(ports_.size());
+  for (const auto& map_entry : ports_)
+    ports.push_back(map_entry.second->Clone());
+  return ports;
+}
+
+void SerialDeviceEnumerator::AddObserver(Observer* observer) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  observer_list_.AddObserver(observer);
+}
+
+void SerialDeviceEnumerator::RemoveObserver(Observer* observer) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  observer_list_.RemoveObserver(observer);
+}
+
 base::Optional<base::FilePath> SerialDeviceEnumerator::GetPathFromToken(
     const base::UnguessableToken& token) {
-  auto it = token_path_map_.find(token);
-  if (it == token_path_map_.end())
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  auto it = ports_.find(token);
+  if (it == ports_.end())
     return base::nullopt;
 
-  return it->second;
+  return it->second->path;
 }
 
-const base::UnguessableToken& SerialDeviceEnumerator::GetTokenFromPath(
-    const base::FilePath& path) {
-  for (const auto& pair : token_path_map_) {
-    if (pair.second == path)
-      return pair.first;
-  }
-  // A new serial path.
-  return token_path_map_
-      .insert(std::make_pair(base::UnguessableToken::Create(), path))
-      .first->first;
+void SerialDeviceEnumerator::AddPort(mojom::SerialPortInfoPtr port) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  base::UnguessableToken token = port->token;
+  auto result = ports_.insert(std::make_pair(token, std::move(port)));
+  DCHECK(result.second);  // |ports_| should not already contain |token|.
+
+  for (auto& observer : observer_list_)
+    observer.OnPortAdded(*result.first->second);
+}
+
+void SerialDeviceEnumerator::RemovePort(base::UnguessableToken token) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  auto it = ports_.find(token);
+  DCHECK(it != ports_.end());
+  mojom::SerialPortInfoPtr port = std::move(it->second);
+  ports_.erase(it);
+
+  for (auto& observer : observer_list_)
+    observer.OnPortRemoved(*port);
 }
 
 }  // namespace device

services/device/serial/serial_device_enumerator.h

@@ -9,7 +9,10 @@
 #include <memory>
 #include <vector>
 
+#include "base/observer_list.h"
 #include "base/optional.h"
+#include "base/sequence_checker.h"
+#include "base/unguessable_token.h"
 #include "services/device/public/mojom/serial.mojom.h"
 #include "services/device/serial/serial_io_handler.h"
 
@@ -22,7 +25,11 @@ namespace device {
 // Discovers and enumerates serial devices available to the host.
 class SerialDeviceEnumerator {
  public:
-  using TokenPathMap = std::map<base::UnguessableToken, base::FilePath>;
+  class Observer : public base::CheckedObserver {
+   public:
+    virtual void OnPortAdded(const mojom::SerialPortInfo& port) = 0;
+    virtual void OnPortRemoved(const mojom::SerialPortInfo& port) = 0;
+  };
 
   static std::unique_ptr<SerialDeviceEnumerator> Create(
       scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner);
@@ -30,16 +37,25 @@ class SerialDeviceEnumerator {
   SerialDeviceEnumerator();
   virtual ~SerialDeviceEnumerator();
 
-  virtual std::vector<mojom::SerialPortInfoPtr> GetDevices() = 0;
+  std::vector<mojom::SerialPortInfoPtr> GetDevices();
+  void AddObserver(Observer* observer);
+  void RemoveObserver(Observer* observer);
 
   virtual base::Optional<base::FilePath> GetPathFromToken(
       const base::UnguessableToken& token);
 
  protected:
-  const base::UnguessableToken& GetTokenFromPath(const base::FilePath& path);
+  // These helper methods take care of managing |ports_| and notifying
+  // observers. |port|s passed to AddPort() must be unique and the |token|
+  // passed to RemovePort() must have previously been added.
+  void AddPort(mojom::SerialPortInfoPtr port);
+  void RemovePort(base::UnguessableToken token);
+
+  SEQUENCE_CHECKER(sequence_checker_);
 
  private:
-  TokenPathMap token_path_map_;
+  std::map<base::UnguessableToken, mojom::SerialPortInfoPtr> ports_;
+  base::ObserverList<Observer> observer_list_;
 };
 
 }  // namespace device

services/device/serial/serial_device_enumerator_linux.cc

@@ -44,23 +44,6 @@ SerialDeviceEnumeratorLinux::~SerialDeviceEnumeratorLinux() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 }
 
-std::vector<mojom::SerialPortInfoPtr>
-SerialDeviceEnumeratorLinux::GetDevices() {
-  std::vector<mojom::SerialPortInfoPtr> ports;
-  ports.reserve(ports_.size());
-  for (const auto& map_entry : ports_)
-    ports.push_back(map_entry.second->Clone());
-  return ports;
-}
-
-base::Optional<base::FilePath> SerialDeviceEnumeratorLinux::GetPathFromToken(
-    const base::UnguessableToken& token) {
-  auto it = ports_.find(token);
-  if (it == ports_.end())
-    return base::nullopt;
-  return it->second->path;
-}
-
 void SerialDeviceEnumeratorLinux::OnDeviceAdded(ScopedUdevDevicePtr device) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   base::ScopedBlockingCall scoped_blocking_call(FROM_HERE,
@@ -111,9 +94,10 @@ void SerialDeviceEnumeratorLinux::OnDeviceRemoved(ScopedUdevDevicePtr device) {
   auto it = paths_.find(syspath);
   if (it == paths_.end())
     return;
+  base::UnguessableToken token = it->second;
 
-  ports_.erase(it->second);
   paths_.erase(it);
+  RemovePort(token);
 }
 
 void SerialDeviceEnumeratorLinux::CreatePort(ScopedUdevDevicePtr device,
@@ -146,8 +130,8 @@ void SerialDeviceEnumeratorLinux::CreatePort(ScopedUdevDevicePtr device,
   if (product_name)
     info->display_name.emplace(product_name);
 
-  ports_.insert(std::make_pair(token, std::move(info)));
   paths_.insert(std::make_pair(syspath, token));
+  AddPort(std::move(info));
 }
 
 }  // namespace device

services/device/serial/serial_device_enumerator_linux.h

@@ -10,10 +10,7 @@
 #include <string>
 
 #include "base/macros.h"
-#include "base/sequence_checker.h"
-#include "base/unguessable_token.h"
 #include "device/udev_linux/udev_watcher.h"
-#include "services/device/public/mojom/serial.mojom.h"
 #include "services/device/serial/serial_device_enumerator.h"
 
 namespace device {
@@ -25,11 +22,6 @@ class SerialDeviceEnumeratorLinux : public SerialDeviceEnumerator,
   SerialDeviceEnumeratorLinux();
   ~SerialDeviceEnumeratorLinux() override;
 
-  // SerialDeviceEnumerator
-  std::vector<mojom::SerialPortInfoPtr> GetDevices() override;
-  base::Optional<base::FilePath> GetPathFromToken(
-      const base::UnguessableToken& token) override;
-
   // UdevWatcher::Observer
   void OnDeviceAdded(ScopedUdevDevicePtr device) override;
   void OnDeviceChanged(ScopedUdevDevicePtr device) override;
@@ -39,11 +31,8 @@ class SerialDeviceEnumeratorLinux : public SerialDeviceEnumerator,
   void CreatePort(ScopedUdevDevicePtr device, const std::string& syspath);
 
   std::unique_ptr<UdevWatcher> watcher_;
-  std::map<base::UnguessableToken, mojom::SerialPortInfoPtr> ports_;
   std::map<std::string, base::UnguessableToken> paths_;
 
-  SEQUENCE_CHECKER(sequence_checker_);
-
   DISALLOW_COPY_AND_ASSIGN(SerialDeviceEnumeratorLinux);
 };
 

services/device/serial/serial_device_enumerator_mac.cc

@@ -137,23 +137,7 @@ SerialDeviceEnumeratorMac::SerialDeviceEnumeratorMac() {
   RemoveDevices();
 }
 
-SerialDeviceEnumeratorMac::~SerialDeviceEnumeratorMac() {}
-
-std::vector<mojom::SerialPortInfoPtr> SerialDeviceEnumeratorMac::GetDevices() {
-  std::vector<mojom::SerialPortInfoPtr> ports;
-  ports.reserve(ports_.size());
-  for (const auto& map_entry : ports_)
-    ports.push_back(map_entry.second->Clone());
-  return ports;
-}
-
-base::Optional<base::FilePath> SerialDeviceEnumeratorMac::GetPathFromToken(
-    const base::UnguessableToken& token) {
-  auto it = ports_.find(token);
-  if (it == ports_.end())
-    return base::nullopt;
-  return it->second->path;
-}
+SerialDeviceEnumeratorMac::~SerialDeviceEnumeratorMac() = default;
 
 // static
 void SerialDeviceEnumeratorMac::FirstMatchCallback(void* context,
@@ -211,8 +195,9 @@ void SerialDeviceEnumeratorMac::AddDevices() {
       mojom::SerialPortInfoPtr dialin_info = callout_info.Clone();
       dialin_info->path = base::FilePath(dialin_device);
       dialin_info->token = token;
-      ports_.insert(std::make_pair(token, std::move(dialin_info)));
+
       entries_[entry_id].first = token;
+      AddPort(std::move(dialin_info));
     }
 
     std::string callout_device;
@@ -221,8 +206,9 @@ void SerialDeviceEnumeratorMac::AddDevices() {
       auto token = base::UnguessableToken::Create();
       callout_info->path = base::FilePath(callout_device);
       callout_info->token = token;
-      ports_.insert(std::make_pair(token, std::move(callout_info)));
+
       entries_[entry_id].second = token;
+      AddPort(std::move(callout_info));
     }
   }
 }
@@ -241,11 +227,14 @@ void SerialDeviceEnumeratorMac::RemoveDevices() {
     if (it == entries_.end())
       continue;
 
-    if (it->second.first)
-      ports_.erase(it->second.first);
-    if (it->second.second)
-      ports_.erase(it->second.second);
+    std::pair<base::UnguessableToken, base::UnguessableToken> tokens =
+        it->second;
     entries_.erase(it);
+
+    if (tokens.first)
+      RemovePort(tokens.first);
+    if (tokens.second)
+      RemovePort(tokens.second);
   }
 }
 

services/device/serial/serial_device_enumerator_mac.h

@@ -9,6 +9,7 @@
 
 #include <map>
 #include <string>
+#include <utility>
 
 #include "base/mac/scoped_ionotificationportref.h"
 #include "base/mac/scoped_ioobject.h"
@@ -25,11 +26,6 @@ class SerialDeviceEnumeratorMac : public SerialDeviceEnumerator {
   SerialDeviceEnumeratorMac();
   ~SerialDeviceEnumeratorMac() override;
 
-  // SerialDeviceEnumerator
-  std::vector<mojom::SerialPortInfoPtr> GetDevices() override;
-  base::Optional<base::FilePath> GetPathFromToken(
-      const base::UnguessableToken& token) override;
-
  private:
   static void FirstMatchCallback(void* context, io_iterator_t iterator);
   static void TerminatedCallback(void* context, io_iterator_t iterator);
@@ -37,7 +33,6 @@ class SerialDeviceEnumeratorMac : public SerialDeviceEnumerator {
   void AddDevices();
   void RemoveDevices();
 
-  std::map<base::UnguessableToken, mojom::SerialPortInfoPtr> ports_;
   // Each IORegistry entry potentially creates two serial ports for the dialin
   // and callout device nodes.
   std::map<uint64_t, std::pair<base::UnguessableToken, base::UnguessableToken>>
@@ -47,8 +42,6 @@ class SerialDeviceEnumeratorMac : public SerialDeviceEnumerator {
   base::mac::ScopedIOObject<io_iterator_t> devices_added_iterator_;
   base::mac::ScopedIOObject<io_iterator_t> devices_removed_iterator_;
 
-  SEQUENCE_CHECKER(sequence_checker_);
-
   DISALLOW_COPY_AND_ASSIGN(SerialDeviceEnumeratorMac);
 };
 

services/device/serial/serial_device_enumerator_win.cc

@@ -178,22 +178,6 @@ base::Optional<base::FilePath> SerialDeviceEnumeratorWin::GetPath(
   return FixUpPortName(com_port);
 }
 
-std::vector<mojom::SerialPortInfoPtr> SerialDeviceEnumeratorWin::GetDevices() {
-  std::vector<mojom::SerialPortInfoPtr> ports;
-  ports.reserve(ports_.size());
-  for (const auto& map_entry : ports_)
-    ports.push_back(map_entry.second->Clone());
-  return ports;
-}
-
-base::Optional<base::FilePath> SerialDeviceEnumeratorWin::GetPathFromToken(
-    const base::UnguessableToken& token) {
-  auto it = ports_.find(token);
-  if (it == ports_.end())
-    return base::nullopt;
-  return it->second->path;
-}
-
 void SerialDeviceEnumeratorWin::OnPathAdded(const base::string16& device_path) {
   ScopedDevInfo dev_info(SetupDiCreateDeviceInfoList(nullptr, nullptr));
   if (!dev_info.is_valid())
@@ -245,8 +229,10 @@ void SerialDeviceEnumeratorWin::OnPathRemoved(
   if (it == paths_.end())
     return;
 
-  ports_.erase(it->second);
+  base::UnguessableToken token = it->second;
+
   paths_.erase(it);
+  RemovePort(token);
 }
 
 void SerialDeviceEnumeratorWin::DoInitialEnumeration() {
@@ -305,8 +291,8 @@ void SerialDeviceEnumeratorWin::EnumeratePort(HDEVINFO dev_info,
     }
   }
 
-  ports_[token] = std::move(info);
   paths_.insert(std::make_pair(*path, token));
+  AddPort(std::move(info));
 }
 
 }  // namespace device

services/device/serial/serial_device_enumerator_win.h

@@ -11,6 +11,9 @@
 #include "device/base/device_monitor_win.h"
 #include "services/device/serial/serial_device_enumerator.h"
 
+typedef void* HDEVINFO;
+typedef struct _SP_DEVINFO_DATA SP_DEVINFO_DATA;
+
 namespace device {
 
 // Discovers and enumerates serial devices available to the host.
@@ -26,11 +29,6 @@ class SerialDeviceEnumeratorWin : public SerialDeviceEnumerator {
   static base::Optional<base::FilePath> GetPath(
       const std::string& friendly_name);
 
-  // SerialDeviceEnumerator
-  std::vector<mojom::SerialPortInfoPtr> GetDevices() override;
-  base::Optional<base::FilePath> GetPathFromToken(
-      const base::UnguessableToken& token) override;
-
   void OnPathAdded(const base::string16& device_path);
   void OnPathRemoved(const base::string16& device_path);
 
@@ -40,7 +38,6 @@ class SerialDeviceEnumeratorWin : public SerialDeviceEnumerator {
   void DoInitialEnumeration();
   void EnumeratePort(HDEVINFO dev_info, SP_DEVINFO_DATA* dev_info_data);
 
-  std::map<base::UnguessableToken, mojom::SerialPortInfoPtr> ports_;
   std::map<base::FilePath, base::UnguessableToken> paths_;
 
   std::unique_ptr<UiThreadHelper, base::OnTaskRunnerDeleter> helper_;

services/device/serial/serial_port_manager_impl.cc

@@ -32,11 +32,19 @@ void SerialPortManagerImpl::SetSerialEnumeratorForTesting(
     std::unique_ptr<SerialDeviceEnumerator> fake_enumerator) {
   DCHECK(fake_enumerator);
   enumerator_ = std::move(fake_enumerator);
+  observed_enumerator_.Add(enumerator_.get());
+}
+
+void SerialPortManagerImpl::SetClient(
+    mojo::PendingRemote<mojom::SerialPortManagerClient> client) {
+  clients_.Add(std::move(client));
 }
 
 void SerialPortManagerImpl::GetDevices(GetDevicesCallback callback) {
-  if (!enumerator_)
+  if (!enumerator_) {
     enumerator_ = SerialDeviceEnumerator::Create(ui_task_runner_);
+    observed_enumerator_.Add(enumerator_.get());
+  }
   std::move(callback).Run(enumerator_->GetDevices());
 }
 
@@ -44,8 +52,10 @@ void SerialPortManagerImpl::GetPort(
     const base::UnguessableToken& token,
     mojo::PendingReceiver<mojom::SerialPort> receiver,
     mojo::PendingRemote<mojom::SerialPortConnectionWatcher> watcher) {
-  if (!enumerator_)
+  if (!enumerator_) {
     enumerator_ = SerialDeviceEnumerator::Create(ui_task_runner_);
+    observed_enumerator_.Add(enumerator_.get());
+  }
   base::Optional<base::FilePath> path = enumerator_->GetPathFromToken(token);
   if (path) {
     io_task_runner_->PostTask(
@@ -55,4 +65,14 @@ void SerialPortManagerImpl::GetPort(
   }
 }
 
+void SerialPortManagerImpl::OnPortAdded(const mojom::SerialPortInfo& port) {
+  for (auto& client : clients_)
+    client->OnPortAdded(port.Clone());
+}
+
+void SerialPortManagerImpl::OnPortRemoved(const mojom::SerialPortInfo& port) {
+  for (auto& client : clients_)
+    client->OnPortRemoved(port.Clone());
+}
+
 }  // namespace device

services/device/serial/serial_port_manager_impl.h

@@ -9,10 +9,13 @@
 
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
+#include "base/scoped_observer.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
 #include "mojo/public/cpp/bindings/receiver_set.h"
+#include "mojo/public/cpp/bindings/remote_set.h"
 #include "services/device/public/mojom/serial.mojom.h"
+#include "services/device/serial/serial_device_enumerator.h"
 
 namespace base {
 class SingleThreadTaskRunner;
@@ -21,12 +24,11 @@ class UnguessableToken;
 
 namespace device {
 
-class SerialDeviceEnumerator;
-
 // TODO(leonhsl): Merge this class with SerialDeviceEnumerator if/once
 // SerialDeviceEnumerator is exposed only via the Device Service.
 // crbug.com/748505
-class SerialPortManagerImpl : public mojom::SerialPortManager {
+class SerialPortManagerImpl : public mojom::SerialPortManager,
+                              public SerialDeviceEnumerator::Observer {
  public:
   SerialPortManagerImpl(
       scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,
@@ -39,18 +41,27 @@ class SerialPortManagerImpl : public mojom::SerialPortManager {
 
  private:
   // mojom::SerialPortManager methods:
+  void SetClient(
+      mojo::PendingRemote<mojom::SerialPortManagerClient> client) override;
   void GetDevices(GetDevicesCallback callback) override;
   void GetPort(
       const base::UnguessableToken& token,
       mojo::PendingReceiver<mojom::SerialPort> receiver,
       mojo::PendingRemote<mojom::SerialPortConnectionWatcher> watcher) override;
 
+  // SerialDeviceEnumerator::Observer methods:
+  void OnPortAdded(const mojom::SerialPortInfo& port) override;
+  void OnPortRemoved(const mojom::SerialPortInfo& port) override;
+
   std::unique_ptr<SerialDeviceEnumerator> enumerator_;
+  ScopedObserver<SerialDeviceEnumerator, SerialDeviceEnumerator::Observer>
+      observed_enumerator_{this};
 
   scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_;
   scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner_;
 
   mojo::ReceiverSet<SerialPortManager> receivers_;
+  mojo::RemoteSet<mojom::SerialPortManagerClient> clients_;
 
   DISALLOW_COPY_AND_ASSIGN(SerialPortManagerImpl);
 };

services/device/serial/serial_port_manager_impl_unittest.cc

@@ -13,7 +13,6 @@
 #include "base/macros.h"
 #include "base/task/post_task.h"
 #include "base/test/bind_test_util.h"
-#include "base/test/task_environment.h"
 #include "base/threading/thread.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
@@ -22,8 +21,12 @@
 #include "services/device/device_service_test_base.h"
 #include "services/device/public/mojom/serial.mojom.h"
 #include "services/device/serial/fake_serial_device_enumerator.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using testing::_;
+using testing::Invoke;
+
 namespace device {
 
 namespace {
@@ -31,37 +34,54 @@ namespace {
 const base::FilePath kFakeDevicePath1(FILE_PATH_LITERAL("/dev/fakeserialmojo"));
 const base::FilePath kFakeDevicePath2(FILE_PATH_LITERAL("\\\\COM800\\"));
 
-void CreateAndBindOnBlockableRunner(
-    mojo::PendingReceiver<mojom::SerialPortManager> receiver,
-    scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,
-    scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner) {
-  auto manager = std::make_unique<SerialPortManagerImpl>(
-      std::move(io_task_runner), std::move(ui_task_runner));
-  auto fake_enumerator = std::make_unique<FakeSerialEnumerator>();
-  fake_enumerator->AddDevicePath(kFakeDevicePath1);
-  fake_enumerator->AddDevicePath(kFakeDevicePath2);
-  manager->SetSerialEnumeratorForTesting(std::move(fake_enumerator));
-  mojo::MakeSelfOwnedReceiver(std::move(manager), std::move(receiver));
-}
+class MockSerialPortManagerClient : public mojom::SerialPortManagerClient {
+ public:
+  MockSerialPortManagerClient() = default;
+  MockSerialPortManagerClient(const MockSerialPortManagerClient&) = delete;
+  MockSerialPortManagerClient& operator=(const MockSerialPortManagerClient&) =
+      delete;
+  ~MockSerialPortManagerClient() override = default;
+
+  mojo::PendingRemote<mojom::SerialPortManagerClient>
+  BindNewPipeAndPassRemote() {
+    return receiver_.BindNewPipeAndPassRemote();
+  }
+
+  // mojom::SerialPortManagerClient
+  MOCK_METHOD1(OnPortAdded, void(mojom::SerialPortInfoPtr));
+  MOCK_METHOD1(OnPortRemoved, void(mojom::SerialPortInfoPtr));
+
+ private:
+  mojo::Receiver<mojom::SerialPortManagerClient> receiver_{this};
+};
 
 }  // namespace
 
 class SerialPortManagerImplTest : public DeviceServiceTestBase {
  public:
-  SerialPortManagerImplTest() = default;
+  SerialPortManagerImplTest() {
+    auto enumerator = std::make_unique<FakeSerialEnumerator>();
+    enumerator_ = enumerator.get();
+    enumerator_->AddDevicePath(kFakeDevicePath1);
+    enumerator_->AddDevicePath(kFakeDevicePath2);
+
+    manager_ = std::make_unique<SerialPortManagerImpl>(
+        io_task_runner_, base::ThreadTaskRunnerHandle::Get());
+    manager_->SetSerialEnumeratorForTesting(std::move(enumerator));
+  }
+
   ~SerialPortManagerImplTest() override = default;
 
  protected:
-  void SetUp() override { DeviceServiceTestBase::SetUp(); }
-
-  void BindSerialPortManager(
-      mojo::PendingReceiver<mojom::SerialPortManager> receiver) {
-    file_task_runner_->PostTask(
-        FROM_HERE,
-        base::BindOnce(&CreateAndBindOnBlockableRunner, std::move(receiver),
-                       io_task_runner_, base::ThreadTaskRunnerHandle::Get()));
+  FakeSerialEnumerator* enumerator_;
+
+  void Bind(mojo::PendingReceiver<mojom::SerialPortManager> receiver) {
+    manager_->Bind(std::move(receiver));
   }
 
+ private:
+  std::unique_ptr<SerialPortManagerImpl> manager_;
+
   DISALLOW_COPY_AND_ASSIGN(SerialPortManagerImplTest);
 };
 
@@ -92,7 +112,7 @@ TEST_F(SerialPortManagerImplTest, SimpleConnectTest) {
 
 TEST_F(SerialPortManagerImplTest, GetDevices) {
   mojo::Remote<mojom::SerialPortManager> port_manager;
-  BindSerialPortManager(port_manager.BindNewPipeAndPassReceiver());
+  Bind(port_manager.BindNewPipeAndPassReceiver());
   const std::set<base::FilePath> expected_paths = {kFakeDevicePath1,
                                                    kFakeDevicePath2};
 
@@ -109,9 +129,58 @@ TEST_F(SerialPortManagerImplTest, GetDevices) {
   loop.Run();
 }
 
+TEST_F(SerialPortManagerImplTest, PortRemovedAndAdded) {
+  mojo::Remote<mojom::SerialPortManager> port_manager;
+  Bind(port_manager.BindNewPipeAndPassReceiver());
+
+  MockSerialPortManagerClient client;
+  port_manager->SetClient(client.BindNewPipeAndPassRemote());
+
+  base::UnguessableToken port1_token;
+  {
+    base::RunLoop run_loop;
+    port_manager->GetDevices(base::BindLambdaForTesting(
+        [&](std::vector<mojom::SerialPortInfoPtr> results) {
+          for (const auto& port : results) {
+            if (port->path == kFakeDevicePath1) {
+              port1_token = port->token;
+              break;
+            }
+          }
+          run_loop.Quit();
+        }));
+    run_loop.Run();
+  }
+  ASSERT_FALSE(port1_token.is_empty());
+
+  enumerator_->RemoveDevicePath(kFakeDevicePath1);
+  {
+    base::RunLoop run_loop;
+    EXPECT_CALL(client, OnPortRemoved(_))
+        .WillOnce(Invoke([&](mojom::SerialPortInfoPtr port) {
+          EXPECT_EQ(port1_token, port->token);
+          EXPECT_EQ(kFakeDevicePath1, port->path);
+          run_loop.Quit();
+        }));
+    run_loop.Run();
+  }
+
+  enumerator_->AddDevicePath(kFakeDevicePath1);
+  {
+    base::RunLoop run_loop;
+    EXPECT_CALL(client, OnPortAdded(_))
+        .WillOnce(Invoke([&](mojom::SerialPortInfoPtr port) {
+          EXPECT_NE(port1_token, port->token);
+          EXPECT_EQ(kFakeDevicePath1, port->path);
+          run_loop.Quit();
+        }));
+    run_loop.Run();
+  }
+}
+
 TEST_F(SerialPortManagerImplTest, GetPort) {
   mojo::Remote<mojom::SerialPortManager> port_manager;
-  BindSerialPortManager(port_manager.BindNewPipeAndPassReceiver());
+  Bind(port_manager.BindNewPipeAndPassReceiver());
 
   base::RunLoop loop;
   port_manager->GetDevices(base::BindLambdaForTesting(