Address TODO in DedicatedWorkerServiceImpl

This CL refactors DedicatedWorkerServiceImpl to keep a pointer to the
DedicatedWorkerHost instead of the DedicatedWorkerInfo.

Bug: chromium:1145158
Change-Id: I081c77c04fa4741dd244ca1939cac6916a7f15c0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2517579
Commit-Queue: Sigurd Schneider <sigurds@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Patrick Monette <pmonette@chromium.org>
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Cr-Commit-Position: refs/heads/master@{#829661}

content/browser/worker_host/dedicated_worker_host.cc

@@ -67,8 +67,7 @@ DedicatedWorkerHost::DedicatedWorkerHost(
 
   scoped_process_host_observation_.Observe(worker_process_host_);
 
-  service_->NotifyWorkerCreated(token_, worker_process_host_->GetID(),
-                                ancestor_render_frame_host_id_, this);
+  service_->NotifyWorkerCreated(this);
 }
 
 DedicatedWorkerHost::~DedicatedWorkerHost() {
@@ -251,6 +250,7 @@ void DedicatedWorkerHost::DidStartScriptLoad(
 
   // TODO(https://crbug.com/986188): Check if the main script's final response
   // URL is committable.
+  final_response_url_ = final_response_url;
   service_->NotifyWorkerFinalResponseURLDetermined(token_, final_response_url);
 
   // TODO(cammie): Change this approach when we support shared workers

content/browser/worker_host/dedicated_worker_host.h

@@ -66,6 +66,12 @@ class DedicatedWorkerHost final : public RenderProcessHostObserver {
   const blink::DedicatedWorkerToken& GetToken() const { return token_; }
   RenderProcessHost* GetProcessHost() const { return worker_process_host_; }
   const url::Origin& GetWorkerOrigin() const { return worker_origin_; }
+  const GlobalFrameRoutingId& GetAncestorRenderFrameHostId() const {
+    return ancestor_render_frame_host_id_;
+  }
+  const base::Optional<GURL>& GetFinalResponseURL() const {
+    return final_response_url_;
+  }
 
   void CreateContentSecurityNotifier(
       mojo::PendingReceiver<blink::mojom::ContentSecurityNotifier> receiver);
@@ -221,6 +227,8 @@ class DedicatedWorkerHost final : public RenderProcessHostObserver {
   // context_url.
   mojo::Remote<network::mojom::CrossOriginEmbedderPolicyReporter>
       coep_reporter_;  // Never null.
+  // Will be set once the worker script started loading.
+  base::Optional<GURL> final_response_url_;
 
   base::WeakPtrFactory<DedicatedWorkerHost> weak_factory_{this};
 

content/browser/worker_host/dedicated_worker_service_impl.cc

@@ -9,23 +9,6 @@
 
 namespace content {
 
-DedicatedWorkerServiceImpl::DedicatedWorkerInfo::DedicatedWorkerInfo(
-    int worker_process_id,
-    GlobalFrameRoutingId ancestor_render_frame_host_id,
-    DedicatedWorkerHost* host)
-    : worker_process_id(worker_process_id),
-      ancestor_render_frame_host_id(ancestor_render_frame_host_id),
-      dedicated_worker_host(host) {}
-
-DedicatedWorkerServiceImpl::DedicatedWorkerInfo::DedicatedWorkerInfo(
-    const DedicatedWorkerInfo& info) = default;
-DedicatedWorkerServiceImpl::DedicatedWorkerInfo&
-DedicatedWorkerServiceImpl::DedicatedWorkerInfo::operator=(
-    const DedicatedWorkerInfo& info) = default;
-
-DedicatedWorkerServiceImpl::DedicatedWorkerInfo::~DedicatedWorkerInfo() =
-    default;
-
 DedicatedWorkerServiceImpl::DedicatedWorkerServiceImpl() = default;
 
 DedicatedWorkerServiceImpl::~DedicatedWorkerServiceImpl() = default;
@@ -39,43 +22,37 @@ void DedicatedWorkerServiceImpl::RemoveObserver(Observer* observer) {
 }
 
 void DedicatedWorkerServiceImpl::EnumerateDedicatedWorkers(Observer* observer) {
-  for (const auto& kv : dedicated_worker_infos_) {
+  for (const auto& kv : dedicated_worker_hosts_) {
     const blink::DedicatedWorkerToken& dedicated_worker_token = kv.first;
-    const DedicatedWorkerInfo& dedicated_worker_info = kv.second;
-
-    observer->OnWorkerCreated(
-        dedicated_worker_token, dedicated_worker_info.worker_process_id,
-        dedicated_worker_info.ancestor_render_frame_host_id);
-    if (dedicated_worker_info.final_response_url) {
-      observer->OnFinalResponseURLDetermined(
-          dedicated_worker_token, *dedicated_worker_info.final_response_url);
+    DedicatedWorkerHost* host = kv.second;
+
+    observer->OnWorkerCreated(dedicated_worker_token,
+                              host->GetProcessHost()->GetID(),
+                              host->GetAncestorRenderFrameHostId());
+    auto& maybe_url = host->GetFinalResponseURL();
+    if (maybe_url) {
+      observer->OnFinalResponseURLDetermined(dedicated_worker_token,
+                                             *maybe_url);
     }
   }
 }
 
 void DedicatedWorkerServiceImpl::NotifyWorkerCreated(
-    const blink::DedicatedWorkerToken& worker_token,
-    int worker_process_id,
-    GlobalFrameRoutingId ancestor_render_frame_host_id,
     DedicatedWorkerHost* host) {
   bool inserted =
-      dedicated_worker_infos_
-          .emplace(worker_token,
-                   DedicatedWorkerInfo(worker_process_id,
-                                       ancestor_render_frame_host_id, host))
-          .second;
+      dedicated_worker_hosts_.emplace(host->GetToken(), host).second;
   DCHECK(inserted);
 
   for (Observer& observer : observers_) {
-    observer.OnWorkerCreated(worker_token, worker_process_id,
-                             ancestor_render_frame_host_id);
+    observer.OnWorkerCreated(host->GetToken(), host->GetProcessHost()->GetID(),
+                             host->GetAncestorRenderFrameHostId());
   }
 }
 
 void DedicatedWorkerServiceImpl::NotifyBeforeWorkerDestroyed(
     const blink::DedicatedWorkerToken& dedicated_worker_token,
     GlobalFrameRoutingId ancestor_render_frame_host_id) {
-  size_t removed = dedicated_worker_infos_.erase(dedicated_worker_token);
+  size_t removed = dedicated_worker_hosts_.erase(dedicated_worker_token);
   DCHECK_EQ(1u, removed);
 
   for (Observer& observer : observers_) {
@@ -87,10 +64,8 @@ void DedicatedWorkerServiceImpl::NotifyBeforeWorkerDestroyed(
 void DedicatedWorkerServiceImpl::NotifyWorkerFinalResponseURLDetermined(
     const blink::DedicatedWorkerToken& dedicated_worker_token,
     const GURL& url) {
-  auto it = dedicated_worker_infos_.find(dedicated_worker_token);
-  DCHECK(it != dedicated_worker_infos_.end());
-
-  it->second.final_response_url = url;
+  auto it = dedicated_worker_hosts_.find(dedicated_worker_token);
+  DCHECK(it != dedicated_worker_hosts_.end());
 
   for (Observer& observer : observers_)
     observer.OnFinalResponseURLDetermined(dedicated_worker_token, url);
@@ -98,16 +73,17 @@ void DedicatedWorkerServiceImpl::NotifyWorkerFinalResponseURLDetermined(
 
 bool DedicatedWorkerServiceImpl::HasToken(
     const blink::DedicatedWorkerToken& worker_token) const {
-  return dedicated_worker_infos_.count(worker_token);
+  return dedicated_worker_hosts_.count(worker_token);
 }
 
 DedicatedWorkerHost*
 DedicatedWorkerServiceImpl::GetDedicatedWorkerHostFromToken(
     const blink::DedicatedWorkerToken& dedicated_worker_token) const {
-  auto it = dedicated_worker_infos_.find(dedicated_worker_token);
-  if (it == dedicated_worker_infos_.end())
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  auto it = dedicated_worker_hosts_.find(dedicated_worker_token);
+  if (it == dedicated_worker_hosts_.end())
     return nullptr;
-  return it->second.dedicated_worker_host;
+  return it->second;
 }
 
 }  // namespace content

content/browser/worker_host/dedicated_worker_service_impl.h

@@ -27,10 +27,7 @@ class CONTENT_EXPORT DedicatedWorkerServiceImpl
   void EnumerateDedicatedWorkers(Observer* observer) override;
 
   // Notifies all observers about a new worker.
-  void NotifyWorkerCreated(const blink::DedicatedWorkerToken& worker_token,
-                           int worker_process_id,
-                           GlobalFrameRoutingId ancestor_render_frame_host_id,
-                           DedicatedWorkerHost* host);
+  void NotifyWorkerCreated(DedicatedWorkerHost* host);
 
   // Notifies all observers about a worker being destroyed.
   void NotifyBeforeWorkerDestroyed(
@@ -54,26 +51,8 @@ class CONTENT_EXPORT DedicatedWorkerServiceImpl
 
  private:
   base::ObserverList<Observer> observers_;
-
-  // TODO(chromium:1145158): Remove this struct.
-  struct DedicatedWorkerInfo {
-    DedicatedWorkerInfo(int worker_process_id,
-                        GlobalFrameRoutingId ancestor_render_frame_host_id,
-                        DedicatedWorkerHost* dedicated_worker_host);
-    ~DedicatedWorkerInfo();
-
-    DedicatedWorkerInfo(const DedicatedWorkerInfo& info);
-    DedicatedWorkerInfo& operator=(const DedicatedWorkerInfo& info);
-
-    int worker_process_id;
-    GlobalFrameRoutingId ancestor_render_frame_host_id;
-    base::Optional<GURL> final_response_url;
-    // This rawptr is valid while the corresponding DedicatedWorkerInfo is kept
-    // by DedicatedWorkerServiceImpl.
-    DedicatedWorkerHost* dedicated_worker_host;
-  };
-  base::flat_map<blink::DedicatedWorkerToken, DedicatedWorkerInfo>
-      dedicated_worker_infos_;
+  base::flat_map<blink::DedicatedWorkerToken, DedicatedWorkerHost*>
+      dedicated_worker_hosts_;
 };
 
 }  // namespace content