PM: Small readability improvements in worker code.

Change-Id: I6fbf2bdfcba2445997dd55e102dac491b2f981d0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2517622
Commit-Queue: Sigurður Ásgeirsson <siggi@chromium.org>
Reviewed-by: Patrick Monette <pmonette@chromium.org>
Cr-Commit-Position: refs/heads/master@{#823670}

components/performance_manager/service_worker_context_adapter.cc

@@ -19,10 +19,12 @@ class ServiceWorkerContextAdapter::RunningServiceWorker
     : content::RenderProcessHostObserver {
  public:
   RunningServiceWorker(int64_t version_id,
-                       content::RenderProcessHost* worker_process_host,
                        ServiceWorkerContextAdapter* adapter);
   ~RunningServiceWorker() override;
 
+  void Subscribe(content::RenderProcessHost* worker_process_host);
+  void Unsubscribe();
+
   void RenderProcessExited(
       content::RenderProcessHost* host,
       const content::ChildProcessTerminationInfo& info) override;
@@ -30,31 +32,43 @@ class ServiceWorkerContextAdapter::RunningServiceWorker
 
  private:
   // The version ID of the service worker.
-  int version_id_;
+  int const version_id_;
 
   // The adapter that owns |this|. Notified when RenderProcessExited() is
   // called.
   ServiceWorkerContextAdapter* const adapter_;
-
-  ScopedObserver<content::RenderProcessHost, content::RenderProcessHostObserver>
-      scoped_render_process_host_observer_{this};
+  // The render process host this instance is observing.
+  content::RenderProcessHost* observing_ = nullptr;
 };
 
 ServiceWorkerContextAdapter::RunningServiceWorker::RunningServiceWorker(
     int64_t version_id,
-    content::RenderProcessHost* worker_process_host,
     ServiceWorkerContextAdapter* adapter)
-    : version_id_(version_id), adapter_(adapter) {
-  scoped_render_process_host_observer_.Add(worker_process_host);
+    : version_id_(version_id), adapter_(adapter) {}
+
+ServiceWorkerContextAdapter::RunningServiceWorker::~RunningServiceWorker() {
+  DCHECK_EQ(observing_, nullptr);
+}
+
+void ServiceWorkerContextAdapter::RunningServiceWorker::Subscribe(
+    content::RenderProcessHost* worker_process_host) {
+  DCHECK_EQ(observing_, nullptr);
+  worker_process_host->AddObserver(this);
+  observing_ = worker_process_host;
 }
 
-ServiceWorkerContextAdapter::RunningServiceWorker::~RunningServiceWorker() =
-    default;
+void ServiceWorkerContextAdapter::RunningServiceWorker::Unsubscribe() {
+  DCHECK_NE(observing_, nullptr);
+  observing_->RemoveObserver(this);
+  observing_ = nullptr;
+}
 
 void ServiceWorkerContextAdapter::RunningServiceWorker::RenderProcessExited(
     content::RenderProcessHost* host,
     const content::ChildProcessTerminationInfo& info) {
   adapter_->OnRenderProcessExited(version_id_);
+
+  /* This object is deleted inside the above, don't touch "this". */
 }
 
 void ServiceWorkerContextAdapter::RunningServiceWorker::
@@ -69,7 +83,12 @@ ServiceWorkerContextAdapter::ServiceWorkerContextAdapter(
   scoped_underlying_context_observer_.Add(underlying_context);
 }
 
-ServiceWorkerContextAdapter::~ServiceWorkerContextAdapter() = default;
+ServiceWorkerContextAdapter::~ServiceWorkerContextAdapter() {
+  // Clean up any outstanding running service worker process subscriptions.
+  for (const auto& item : running_service_workers_)
+    item.second->Unsubscribe();
+  running_service_workers_.clear();
+}
 
 void ServiceWorkerContextAdapter::AddObserver(
     content::ServiceWorkerContextObserver* observer) {
@@ -226,7 +245,7 @@ void ServiceWorkerContextAdapter::OnVersionRedundant(int64_t version_id,
 void ServiceWorkerContextAdapter::OnVersionStartedRunning(
     int64_t version_id,
     const content::ServiceWorkerRunningInfo& running_info) {
-  auto* worker_process_host =
+  content::RenderProcessHost* worker_process_host =
       content::RenderProcessHost::FromID(running_info.render_process_id);
 
   // It's possible that the renderer is already gone since the notification
@@ -240,19 +259,13 @@ void ServiceWorkerContextAdapter::OnVersionStartedRunning(
     return;
   }
 
-  bool inserted =
-      running_service_workers_
-          .emplace(version_id, std::make_unique<RunningServiceWorker>(
-                                   version_id, worker_process_host, this))
-          .second;
-  DCHECK(inserted);
-
+  AddRunningServiceWorker(version_id, worker_process_host);
   for (auto& observer : observer_list_)
     observer.OnVersionStartedRunning(version_id, running_info);
 }
 
 void ServiceWorkerContextAdapter::OnVersionStoppedRunning(int64_t version_id) {
-  size_t removed = running_service_workers_.erase(version_id);
+  bool removed = MaybeRemoveRunningServiceWorker(version_id);
   if (!removed) {
 #if DCHECK_IS_ON()
     // If this service worker could not be found, then it must be because its
@@ -352,8 +365,8 @@ void ServiceWorkerContextAdapter::OnDestruct(ServiceWorkerContext* context) {
 }
 
 void ServiceWorkerContextAdapter::OnRenderProcessExited(int64_t version_id) {
-  size_t removed = running_service_workers_.erase(version_id);
-  DCHECK_EQ(removed, 1u);
+  bool removed = MaybeRemoveRunningServiceWorker(version_id);
+  DCHECK(removed);
 
   for (auto& observer : observer_list_)
     observer.OnVersionStoppedRunning(version_id);
@@ -366,4 +379,30 @@ void ServiceWorkerContextAdapter::OnRenderProcessExited(int64_t version_id) {
 #endif  // DCHECK_IS_ON()
 }
 
+void ServiceWorkerContextAdapter::AddRunningServiceWorker(
+    int64_t version_id,
+    content::RenderProcessHost* worker_process_host) {
+  std::unique_ptr<ServiceWorkerContextAdapter::RunningServiceWorker>
+      running_service_worker =
+          std::make_unique<RunningServiceWorker>(version_id, this);
+
+  running_service_worker->Subscribe(worker_process_host);
+  bool inserted = running_service_workers_
+                      .emplace(version_id, std::move(running_service_worker))
+                      .second;
+  DCHECK(inserted);
+}
+
+bool ServiceWorkerContextAdapter::MaybeRemoveRunningServiceWorker(
+    int64_t version_id) {
+  auto it = running_service_workers_.find(version_id);
+  if (it == running_service_workers_.end())
+    return false;
+
+  it->second->Unsubscribe();
+  running_service_workers_.erase(it);
+
+  return true;
+}
+
 }  // namespace performance_manager

components/performance_manager/service_worker_context_adapter.h

@@ -123,6 +123,15 @@ class ServiceWorkerContextAdapter
   // has exited.
   void OnRenderProcessExited(int64_t version_id);
 
+  // Adds a registration to |worker_process_host| that will result in
+  // |OnRenderProcessExited| with |version_id| when it exits.
+  void AddRunningServiceWorker(int64_t version_id,
+                               content::RenderProcessHost* worker_process_host);
+
+  // Removes a registration made by |AddRunningServiceWorker| if one exists,
+  // returns true if a registration existed, false otherwise.
+  bool MaybeRemoveRunningServiceWorker(int64_t version_id);
+
   ScopedObserver<content::ServiceWorkerContext,
                  content::ServiceWorkerContextObserver>
       scoped_underlying_context_observer_{this};

components/performance_manager/worker_watcher.cc

@@ -28,8 +28,8 @@ void RecordWorkerClientFound(bool found) {
 
 // Helper function to add |client_frame_node| as a client of |worker_node| on
 // the PM sequence.
-void ConnectClientOnGraph(WorkerNodeImpl* worker_node,
-                          FrameNodeImpl* client_frame_node) {
+void ConnectClientFrameOnGraph(WorkerNodeImpl* worker_node,
+                               FrameNodeImpl* client_frame_node) {
   PerformanceManagerImpl::CallOnGraphImpl(
       FROM_HERE,
       base::BindOnce(&WorkerNodeImpl::AddClientFrame,
@@ -38,8 +38,8 @@ void ConnectClientOnGraph(WorkerNodeImpl* worker_node,
 
 // Helper function to remove |client_frame_node| as a client of |worker_node|
 // on the PM sequence.
-void DisconnectClientOnGraph(WorkerNodeImpl* worker_node,
-                             FrameNodeImpl* client_frame_node) {
+void DisconnectClientFrameOnGraph(WorkerNodeImpl* worker_node,
+                                  FrameNodeImpl* client_frame_node) {
   PerformanceManagerImpl::CallOnGraphImpl(
       FROM_HERE,
       base::BindOnce(&WorkerNodeImpl::RemoveClientFrame,
@@ -48,8 +48,8 @@ void DisconnectClientOnGraph(WorkerNodeImpl* worker_node,
 
 // Helper function to add |client_worker_node| as a client of |worker_node| on
 // the PM sequence.
-void ConnectClientOnGraph(WorkerNodeImpl* worker_node,
-                          WorkerNodeImpl* client_worker_node) {
+void ConnectClientWorkerOnGraph(WorkerNodeImpl* worker_node,
+                                WorkerNodeImpl* client_worker_node) {
   PerformanceManagerImpl::CallOnGraphImpl(
       FROM_HERE,
       base::BindOnce(&WorkerNodeImpl::AddClientWorker,
@@ -58,8 +58,8 @@ void ConnectClientOnGraph(WorkerNodeImpl* worker_node,
 
 // Helper function to remove |client_worker_node| as a client of |worker_node|
 // on the PM sequence.
-void DisconnectClientOnGraph(WorkerNodeImpl* worker_node,
-                             WorkerNodeImpl* client_worker_node) {
+void DisconnectClientWorkerOnGraph(WorkerNodeImpl* worker_node,
+                                   WorkerNodeImpl* client_worker_node) {
   PerformanceManagerImpl::CallOnGraphImpl(
       FROM_HERE,
       base::BindOnce(&WorkerNodeImpl::RemoveClientWorker,
@@ -505,7 +505,7 @@ void WorkerWatcher::ConnectFrameClient(
 
   RecordWorkerClientFound(true);
 
-  ConnectClientOnGraph(worker_node, frame_node);
+  ConnectClientFrameOnGraph(worker_node, frame_node);
 
   // Keep track of the workers that this frame is a client to.
   if (AddChildWorker(client_render_frame_host_id, worker_node)) {
@@ -550,7 +550,7 @@ void WorkerWatcher::DisconnectFrameClient(
     return;
   }
 
-  DisconnectClientOnGraph(worker_node, frame_node);
+  DisconnectClientFrameOnGraph(worker_node, frame_node);
 
   // Remove |worker_node| from the set of workers that this frame is a client
   // of.
@@ -563,8 +563,8 @@ void WorkerWatcher::ConnectDedicatedWorkerClient(
     blink::DedicatedWorkerToken client_dedicated_worker_token) {
   DCHECK(worker_node);
 
-  ConnectClientOnGraph(worker_node,
-                       GetDedicatedWorkerNode(client_dedicated_worker_token));
+  ConnectClientWorkerOnGraph(
+      worker_node, GetDedicatedWorkerNode(client_dedicated_worker_token));
 
   // Remember that |worker_node| is a child worker of this dedicated worker.
   bool inserted = dedicated_worker_child_workers_[client_dedicated_worker_token]
@@ -590,7 +590,7 @@ void WorkerWatcher::DisconnectDedicatedWorkerClient(
   if (child_workers.empty())
     dedicated_worker_child_workers_.erase(it);
 
-  DisconnectClientOnGraph(
+  DisconnectClientWorkerOnGraph(
       worker_node, GetDedicatedWorkerNode(client_dedicated_worker_token));
 }
 
@@ -599,8 +599,8 @@ void WorkerWatcher::ConnectSharedWorkerClient(
     blink::SharedWorkerToken client_shared_worker_token) {
   DCHECK(worker_node);
 
-  ConnectClientOnGraph(worker_node,
-                       GetSharedWorkerNode(client_shared_worker_token));
+  ConnectClientWorkerOnGraph(worker_node,
+                             GetSharedWorkerNode(client_shared_worker_token));
 
   // Remember that |worker_node| is a child worker of this shared worker.
   bool inserted = shared_worker_child_workers_[client_shared_worker_token]
@@ -625,8 +625,8 @@ void WorkerWatcher::DisconnectSharedWorkerClient(
   if (child_workers.empty())
     shared_worker_child_workers_.erase(it);
 
-  DisconnectClientOnGraph(worker_node,
-                          GetSharedWorkerNode(client_shared_worker_token));
+  DisconnectClientWorkerOnGraph(
+      worker_node, GetSharedWorkerNode(client_shared_worker_token));
 }
 
 void WorkerWatcher::ConnectAllServiceWorkerClients(