Manage ServiceWorkerDispatcherHost in ServiceWorkerContextCore

content/browser/service_worker/service_worker_context_core.h

Index: content/browser/service_worker/service_worker_context_core.h
diff --git a/content/browser/service_worker/service_worker_context_core.h b/content/browser/service_worker/service_worker_context_core.h
index 4832b8c4eb4c2680d855385a1b8c8bd542f44b71..fa0454035b223f8e0d45c041f9ebedbef67dcc9a 100644
--- a/content/browser/service_worker/service_worker_context_core.h
+++ b/content/browser/service_worker/service_worker_context_core.h
@@ -44,6 +44,7 @@ class EmbeddedWorkerRegistry;
 class ServiceWorkerContextObserver;
 class ServiceWorkerContextWrapper;
 class ServiceWorkerDatabaseTaskManager;
+class ServiceWorkerDispatcherHost;
 class ServiceWorkerJobCoordinator;
 class ServiceWorkerNavigationHandleCore;
 class ServiceWorkerProviderHost;
@@ -69,6 +70,7 @@ class CONTENT_EXPORT ServiceWorkerContextCore
                                              int64_t registration_id)>;
   using UnregistrationCallback =
       base::Callback<void(ServiceWorkerStatusCode status)>;
+  using DispatcherHostMap = IDMap<ServiceWorkerDispatcherHost*>;

[falken, 2017/01/26 05:15:43]

It feels misleading that the other *Map types here own their elements, and
DispatcherhostMap doesn't. I would just type out
IDMap<ServiceWorkerDispatcherHost*>. (I'm not really a fan of these Map aliases
in general, they don't really add clarify IMO.)

[shimazu, 2017/01/26 09:27:22]

Makes sense. Done.

   using ProviderMap = IDMap<std::unique_ptr<ServiceWorkerProviderHost>>;
   using ProcessToProviderMap = IDMap<std::unique_ptr<ProviderMap>>;
 
@@ -154,10 +156,18 @@ class CONTENT_EXPORT ServiceWorkerContextCore
     return job_coordinator_.get();
   }
 
+  // Maintains DispatcherHosts to exchange service worker related messages
+  // through them.

[falken, 2017/01/26 05:15:43]

It's a bit inconsistent that AddDispatcherHost doesn't take ownership of the
pointer, but AddProviderHost does. Maybe say "The DispatcherHosts are not owned
by this class."

[shimazu, 2017/01/26 09:27:22]

Done.

+  void AddDispatcherHost(int process_id,
+                         ServiceWorkerDispatcherHost* dispatcher_host);
+  void RemoveDispatcherHost(int process_id);
+
   // The context class owns the set of ProviderHosts.
-  ServiceWorkerProviderHost* GetProviderHost(int process_id, int provider_id);
   void AddProviderHost(
       std::unique_ptr<ServiceWorkerProviderHost> provider_host);
+  void AddProviderHost(int process_id,
+                       ServiceWorkerProviderHostInfo provider_host_info);

[falken, 2017/01/26 05:15:42]

nit: Consider removing the function overloading by having ProviderHost provide
the functions to create the host, and then all you need is
AddProviderHost(std::unique_ptr<>).

[shimazu, 2017/01/26 09:27:22]

Done.

+  ServiceWorkerProviderHost* GetProviderHost(int process_id, int provider_id);
   void RemoveProviderHost(int process_id, int provider_id);
   void RemoveAllProviderHostsForProcess(int process_id);
   std::unique_ptr<ProviderHostIterator> GetProviderHostIterator();
@@ -342,6 +352,7 @@ class CONTENT_EXPORT ServiceWorkerContextCore
   // because the Wrapper::Shutdown call that hops threads to destroy |this| uses
   // Bind() to hold a reference to |wrapper_| until |this| is fully destroyed.
   ServiceWorkerContextWrapper* wrapper_;
+  std::unique_ptr<DispatcherHostMap> dispatcher_hosts_;
   std::unique_ptr<ProcessToProviderMap> providers_;
   std::unique_ptr<ProviderByClientUUIDMap> provider_by_uuid_;
   std::unique_ptr<ServiceWorkerStorage> storage_;