First step to refactor ServiceWorkerVersion to make event dispatching more modular.

content/browser/service_worker/service_worker_version.h

Index: content/browser/service_worker/service_worker_version.h
diff --git a/content/browser/service_worker/service_worker_version.h b/content/browser/service_worker/service_worker_version.h
index cbeb65597f94342489405655f80cda93e7520c58..bb54b0c8141331a075c5a42af93c85e664f7b0ae 100644
--- a/content/browser/service_worker/service_worker_version.h
+++ b/content/browser/service_worker/service_worker_version.h
@@ -13,6 +13,7 @@
 
 #include "base/basictypes.h"
 #include "base/callback.h"
+#include "base/containers/scoped_ptr_hash_map.h"
 #include "base/gtest_prod_util.h"
 #include "base/id_map.h"
 #include "base/memory/ref_counted.h"
@@ -24,7 +25,6 @@
 #include "content/browser/service_worker/service_worker_script_cache_map.h"
 #include "content/common/background_sync_service.mojom.h"
 #include "content/common/content_export.h"
-#include "content/common/service_port_service.mojom.h"
 #include "content/common/service_worker/service_worker_status_code.h"
 #include "content/common/service_worker/service_worker_types.h"
 #include "content/public/common/service_registry.h"
@@ -72,11 +72,6 @@ class CONTENT_EXPORT ServiceWorkerVersion
   typedef base::Callback<void(ServiceWorkerStatusCode,
                               ServiceWorkerFetchEventResult,
                               const ServiceWorkerResponse&)> FetchCallback;
-  typedef base::Callback<void(ServiceWorkerStatusCode,
-                              bool /* accept_connction */,
-                              const base::string16& /* name */,
-                              const base::string16& /* data */)>
-      ServicePortConnectCallback;
 
   enum RunningStatus {
     STOPPED = EmbeddedWorkerInstance::STOPPED,
@@ -177,6 +172,35 @@ class CONTENT_EXPORT ServiceWorkerVersion
   // Starts an update now.
   void StartUpdate();
 
+  // Starts the worker if it isn't already running, and calls |task| when the
+  // worker is running, or |error_callback| if starting the worker failed.
+  void RunAfterStartWorker(const StatusCallback& error_callback,
+                           const base::Closure& task);
+
+  // Call this while the worker is running before dispatching an event to the
+  // worker. This informs ServiceWorkerVersion about the event in progress.
+  // Returns a request id, which should later be passed to FinishRequest when
+  // the event finished.
+  // The |error_callback| is called if either ServiceWorkerVersion decides the
+  // event is taking too long, or if for some reason the worker stops or is
+  // killed before the request finishes.
+  int StartRequest(const StatusCallback& error_callback);
+
+  // Informs ServiceWorkerVersion that an event has finished being dispatched.
+  // Returns false if no pending requests with the provided id exist, for
+  // example if the request has already timed out.
+  bool FinishRequest(int request_id);
+
+  // Connects to a specific mojo service exposed by the (running) service
+  // worker. If a connection to a service for the same Interface already exists
+  // this will return that existing connection. The |request_id| must be a value
+  // previously returned by StartRequest. If the connection to the service
+  // fails or closes before the request finished, the error callback associated
+  // with |request_id| is called.
+  // Only call GetMojoServiceForRequest once for a specific |request_id|.
+  template <typename Interface>
+  base::WeakPtr<Interface> GetMojoServiceForRequest(int request_id);
+
   // Sends a message event to the associated embedded worker.
   void DispatchMessageEvent(
       const base::string16& message,
@@ -253,16 +277,6 @@ class CONTENT_EXPORT ServiceWorkerVersion
       const std::string& region_id,
       const blink::WebCircularGeofencingRegion& region);
 
-  // Sends a ServicePort connect event to the associated embedded worker and
-  // asynchronously calls |callback| with the response from the worker.
-  //
-  // This must be called when the status() is ACTIVATED.
-  void DispatchServicePortConnectEvent(
-      const ServicePortConnectCallback& callback,
-      const GURL& target_url,
-      const GURL& origin,
-      int port_id);
-
   // Sends a cross origin message event to the associated embedded worker and
   // asynchronously calls |callback| when the message was sent (or failed to
   // sent).
@@ -360,6 +374,7 @@ class CONTENT_EXPORT ServiceWorkerVersion
   FRIEND_TEST_ALL_PREFIXES(ServiceWorkerVersionTest,
                            StaleUpdate_DoNotDeferTimer);
   FRIEND_TEST_ALL_PREFIXES(ServiceWorkerWaitForeverInFetchTest, RequestTimeout);
+  FRIEND_TEST_ALL_PREFIXES(ServiceWorkerVersionTest, RequestTimeout);
   FRIEND_TEST_ALL_PREFIXES(ServiceWorkerFailToStartTest, Timeout);
   FRIEND_TEST_ALL_PREFIXES(ServiceWorkerVersionBrowserTest,
                            TimeoutStartingWorker);
@@ -385,7 +400,7 @@ class CONTENT_EXPORT ServiceWorkerVersion
     REQUEST_NOTIFICATION_CLICK,
     REQUEST_PUSH,
     REQUEST_GEOFENCING,
-    REQUEST_SERVICE_PORT_CONNECT,
+    REQUEST_CUSTOM,

[nhiroki, 2016/01/05 09:33:24]

I think it'd be better that each event has a unique type for UMA as is instead
of a general one. As you noted in your docs, how about passing the request type
into StartRequest()?

[Marijn Kruisselbrink, 2016/01/05 23:23:16]

Currently my approach was to add necessary functionality as I'm migrating
existing events. So given that fetch is currently the only event for which UMA
stats are collected by ServiceWorkerVersion I didn't yet add support for that. I
do still plan to add a ServiceWorkerMetrics::EventType parameter to
StartRequest, although I'm not quite sure yet how to deal with the potential
explosion of parameters to this method (especially where many of the parameters
should at least conceptually be optional. For background sync StartRequest will
need custom timeout behavior for example. But I don't really want to end up
with:
StartRequest(callback)
StartRequestWithEventType(callback, event_type)
StartRequestWithCustomTimeout(callback, timeout)
StartRequestWithEventTypeAndCustomTimeout(callback, event_type, timeout)
and who knows what other attributes eventually might apply to a request...
(or just having one overloaded method, or even one method with default
parameters, both of which aren't recommended by the styleguide).

So I figured I'd ignore this problem for now and deal with it when I actually
have to.

[nhiroki, 2016/01/06 10:13:57]

Thank you for the explanation.

On 2016/01/05 23:23:16, Marijn Kruisselbrink wrote:
You mean "ServiceWorker.EventHandledRatioType"? It's collected only for fetch
event, but there is other UMA stat collected for all event types
("ServiceWorker.RequestTimeouts.Count").
I think we may need to make |event_type| a required parameter to collect the
timeout stats. |timeout| would be optional.

[Marijn Kruisselbrink, 2016/01/07 00:59:08]

Ah yes, not sure how I missed the "Used for UMA" comment on RequestType. In that
case yes, this is something I need to deal with here already. In the latest
version I changed the code to no longer use RequestType for UMA stats, but
instead use ServiceWorkerMetrics::EventType (and I added all the existing values
from RequestType in the same order/with the same values to ensure histograms
keep being valid). That makes the existing code in SWVersion temporarily a bit
more complicated, as now all the event dispatching code has to specify both a
RequestType and an EventType, but in the future when all event dispatching is
done using StartRequest we should be able to completely get rid of RequestType
(and make various other simplifications as well).
As suggested, EventType now is also a required parameter to StartRequest, so
histograms should remain working the same. RecordEventHandledRatio still only
works (and is only called) for fetch events; I imagine if we'd want similar
histograms for other event types we could add some kind of event handled status
to FinishRequest, which would then collect the correct metrics.

     NUM_REQUEST_TYPES
   };
 
@@ -405,6 +420,44 @@ class CONTENT_EXPORT ServiceWorkerVersion
 
     CallbackType callback;
     base::TimeTicks start_time;
+    // Name of the mojo service this request is associated with. Used to call
+    // the callback when a connection closes with outstanding requests.
+    // Compared as pointer, so should only contain static strings. Typically
+    // this would be Interface::Name_ for some mojo interface.
+    const char* mojo_service = nullptr;
+  };
+
+  // Base class to enable storing a list of mojo interface pointers for
+  // arbitrary interfaces. The destructor is also responsible for calling the
+  // error callbacks for any outstanding requests using this service.
+  class CONTENT_EXPORT BaseMojoService {
+   public:
+    BaseMojoService(ServiceWorkerVersion* worker, const char* service_name);
+    virtual ~BaseMojoService();
+
+   private:
+    ServiceWorkerVersion* worker_;
+    const char* service_name_;
+  };
+
+  // Wrapper around a mojo::InterfacePtr, which passes out WeakPtr's to the
+  // interface.
+  template <typename Interface>
+  class MojoService : public BaseMojoService {
+   public:
+    MojoService(ServiceWorkerVersion* worker,
+                mojo::InterfacePtr<Interface> interface)
+        : BaseMojoService(worker, Interface::Name_),
+          interface_(std::move(interface)),
+          weak_ptr_factory_(interface_.get()) {}
+
+    base::WeakPtr<Interface> GetWeakPtr() {
+      return weak_ptr_factory_.GetWeakPtr();
+    }
+
+   private:
+    mojo::InterfacePtr<Interface> interface_;
+    base::WeakPtrFactory<Interface> weak_ptr_factory_;
   };
 
   typedef ServiceWorkerVersion self;
@@ -475,10 +528,6 @@ class CONTENT_EXPORT ServiceWorkerVersion
   void OnPushEventFinished(int request_id,
                            blink::WebServiceWorkerEventResult result);
   void OnGeofencingEventFinished(int request_id);
-  void OnServicePortConnectEventFinished(int request_id,
-                                         ServicePortConnectResult result,
-                                         const mojo::String& name,
-                                         const mojo::String& data);
   void OnOpenWindow(int request_id, GURL url);
   void OnOpenWindowFinished(int request_id,
                             ServiceWorkerStatusCode status,
@@ -589,9 +638,11 @@ class CONTENT_EXPORT ServiceWorkerVersion
   // Called when a connection to a mojo event Dispatcher drops or fails.
   // Calls callbacks for any outstanding requests to the dispatcher as well
   // as cleans up the dispatcher.
-  void OnServicePortDispatcherConnectionError();
   void OnBackgroundSyncDispatcherConnectionError();
 
+  // Called when the remote side of a connection to a mojo service is lost.
+  void OnMojoConnectionError(const char* service_name);
+
   // Called at the beginning of each Dispatch*Event function: records
   // the time elapsed since idle (generally the time since the previous
   // event ended).
@@ -619,12 +670,18 @@ class CONTENT_EXPORT ServiceWorkerVersion
       notification_click_requests_;
   IDMap<PendingRequest<StatusCallback>, IDMapOwnPointer> push_requests_;
   IDMap<PendingRequest<StatusCallback>, IDMapOwnPointer> geofencing_requests_;
-  IDMap<PendingRequest<ServicePortConnectCallback>, IDMapOwnPointer>
-      service_port_connect_requests_;
+  IDMap<PendingRequest<StatusCallback>, IDMapOwnPointer> custom_requests_;
 
-  ServicePortDispatcherPtr service_port_dispatcher_;
   BackgroundSyncServiceClientPtr background_sync_dispatcher_;
 
+  // Stores all open connections to mojo services. Maps the service name to
+  // the actual interface pointer. When a connection is closed it is removed
+  // from this map.
+  // mojo_services_[Interface::Name_] is assumed to always contain a
+  // MojoService<Interface> instance.
+  base::ScopedPtrHashMap<const char*, scoped_ptr<BaseMojoService>>
+      mojo_services_;
+
   std::set<const ServiceWorkerURLRequestJob*> streaming_url_request_jobs_;
 
   std::map<std::string, ServiceWorkerProviderHost*> controllee_map_;
@@ -679,6 +736,31 @@ class CONTENT_EXPORT ServiceWorkerVersion
   DISALLOW_COPY_AND_ASSIGN(ServiceWorkerVersion);
 };
 
+template <typename Interface>
+base::WeakPtr<Interface> ServiceWorkerVersion::GetMojoServiceForRequest(
+    int request_id) {
+  DCHECK_EQ(RUNNING, running_status());
+  PendingRequest<StatusCallback>* request = custom_requests_.Lookup(request_id);
+  DCHECK(request) << "Invalid request id";
+  DCHECK(!request->mojo_service)
+      << "Request is already associated with a mojo service";
+
+  MojoService<Interface>* service = static_cast<MojoService<Interface>*>(
+      mojo_services_.get(Interface::Name_));
+  if (!service) {
+    mojo::InterfacePtr<Interface> interface;
+    embedded_worker_->GetServiceRegistry()->ConnectToRemoteService(
+        mojo::GetProxy(&interface));
+    interface.set_connection_error_handler(
+        base::Bind(&ServiceWorkerVersion::OnMojoConnectionError,
+                   weak_factory_.GetWeakPtr(), Interface::Name_));
+    service = new MojoService<Interface>(this, std::move(interface));
+    mojo_services_.add(Interface::Name_, make_scoped_ptr(service));
+  }
+  request->mojo_service = Interface::Name_;
+  return service->GetWeakPtr();
+}
+
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_SERVICE_WORKER_SERVICE_WORKER_VERSION_H_