Update navigator.services API to use the new services.onconnect event [2/3].

content/browser/service_worker/service_worker_version.cc

Index: content/browser/service_worker/service_worker_version.cc
diff --git a/content/browser/service_worker/service_worker_version.cc b/content/browser/service_worker/service_worker_version.cc
index d5a17b5136ff019c187f8e4262a0e2b7f11c85aa..5db0ec52a9a63fc973b64f2e3aeb73ddd90a93c8 100644
--- a/content/browser/service_worker/service_worker_version.cc
+++ b/content/browser/service_worker/service_worker_version.cc
@@ -162,10 +162,11 @@ void RunErrorMessageCallback(
   callback.Run(status);
 }
 
-void RunErrorCrossOriginConnectCallback(
-    const ServiceWorkerVersion::CrossOriginConnectCallback& callback,
+void RunErrorServicePortConnectCallback(
+    const ServiceWorkerVersion::ServicePortConnectCallback& callback,
     ServiceWorkerStatusCode status) {
-  callback.Run(status, false /* accept_connection */);
+  callback.Run(status, false /* accept_connection */, base::string16(),
+               base::string16());
 }
 
 void RunErrorSendStashedPortsCallback(
@@ -897,14 +898,17 @@ void ServiceWorkerVersion::DispatchGeofencingEvent(
   }
 }
 
-void ServiceWorkerVersion::DispatchCrossOriginConnectEvent(
-    const CrossOriginConnectCallback& callback,
-    const NavigatorConnectClient& client) {
+void ServiceWorkerVersion::DispatchServicePortConnectEvent(
+    const ServicePortConnectCallback& callback,
+    const GURL& target_url,
+    const GURL& origin,
+    int port_id) {
   DCHECK_EQ(ACTIVATED, status()) << status();
 
   if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kEnableExperimentalWebPlatformFeatures)) {
-    callback.Run(SERVICE_WORKER_ERROR_ABORT, false);
+    callback.Run(SERVICE_WORKER_ERROR_ABORT, false, base::string16(),
+                 base::string16());
     return;
   }
 
@@ -912,20 +916,29 @@ void ServiceWorkerVersion::DispatchCrossOriginConnectEvent(
     // Schedule calling this method after starting the worker.
     StartWorker(
         base::Bind(&RunTaskAfterStartWorker, weak_factory_.GetWeakPtr(),
-                   base::Bind(&RunErrorCrossOriginConnectCallback, callback),
-                   base::Bind(&self::DispatchCrossOriginConnectEvent,
-                              weak_factory_.GetWeakPtr(), callback, client)));
+                   base::Bind(&RunErrorServicePortConnectCallback, callback),
+                   base::Bind(&self::DispatchServicePortConnectEvent,
+                              weak_factory_.GetWeakPtr(), callback, target_url,
+                              origin, port_id)));
     return;
   }
 
-  int request_id = AddRequest(callback, &cross_origin_connect_callbacks_,
-                              REQUEST_CROSS_ORIGIN_CONNECT);
-  ServiceWorkerStatusCode status = embedded_worker_->SendMessage(
-      ServiceWorkerMsg_CrossOriginConnectEvent(request_id, client));
-  if (status != SERVICE_WORKER_OK) {
-    cross_origin_connect_callbacks_.Remove(request_id);
-    RunSoon(base::Bind(callback, status, false));
+  int request_id = AddRequest(callback, &service_port_connect_callbacks_,
+                              REQUEST_SERVICE_PORT_CONNECT);
+  if (!service_port_dispatcher_) {
+    embedded_worker_->GetServiceRegistry()->ConnectToRemoteService(

[iclelland, 2015/07/08 02:59:29]

This looks to me like it has the same problem as one of my earlier proposals for
DispatchSyncEvent -- that the ServiceWorkerVersion may outlive any given worker
thread -- I think that service_port_dispatcher_ here will be set once, and never
reset if the worker thread dies. The solution in the Sync case was to move the
mojo connection from the ServiceWorkerVersion to the
ServiceWorkerDispatcherHost.

(see michaeln's comment at
https://codereview.chromium.org/1171173002/diff/410001/content/browser/servic...
)

+        mojo::GetProxy(&service_port_dispatcher_));
+    service_port_dispatcher_.set_connection_error_handler(
+        base::Bind(&ServiceWorkerVersion::OnMojoConnectionError<
+                       ServicePortDispatcher, ServicePortConnectCallback>,
+                   weak_factory_.GetWeakPtr(), &service_port_dispatcher_,
+                   &service_port_connect_callbacks_,
+                   base::Bind(&RunErrorServicePortConnectCallback)));
   }
+  service_port_dispatcher_->Connect(

[michaeln, 2015/07/07 22:13:17]

What happens to service_port_dispatcher_ when the embedded instance 'stops' and
then is started in a new process?

[Marijn Kruisselbrink, 2015/07/07 23:58:42]

Hmm, good question. I seem to have assumed that that would automatically sever
the connection, thus causing service_port_dispatcher_ to call its connection
error handler, but I think you're right that either OnStoppedInternal or some
method like that should sever these connections on the browser side, or
ServicePortDispatcherImpl would have to destroy itself/sever the connection when
the embedded worker it "belongs" to is stopped.

[iclelland, 2015/07/08 02:59:29]

I took a closer look at this last week, since I had a draft message asking
essentially the same thing. As far as I can tell, when the embedded worker
stops, the connection will be shut down automatically, and the InterfacePtr will
test as false the next time that DispatchCrossOriginConnectEvent is called. (see
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/mojo/s...)
If there are outstanding callbacks, then the connection error handler will be
called first.
I'd like to test this behaviour, and probably loop someone like rockot@ in to
make sure it's okay, but I think it's correct.

+      mojo::String::From(target_url), mojo::String::From(origin), port_id,
+      base::Bind(&ServiceWorkerVersion::OnServicePortConnectEventFinished,
+                 weak_factory_.GetWeakPtr(), request_id));
 }
 
 void ServiceWorkerVersion::DispatchCrossOriginMessageEvent(
@@ -1193,8 +1206,6 @@ bool ServiceWorkerVersion::OnMessageReceived(const IPC::Message& message) {
                         OnPushEventFinished)
     IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_GeofencingEventFinished,
                         OnGeofencingEventFinished)
-    IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_CrossOriginConnectEventFinished,
-                        OnCrossOriginConnectEventFinished)
     IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_OpenWindow,
                         OnOpenWindow)
     IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_SetCachedMetadata,
@@ -1437,22 +1448,25 @@ void ServiceWorkerVersion::OnGeofencingEventFinished(int request_id) {
   RemoveCallbackAndStopIfRedundant(&geofencing_callbacks_, request_id);
 }
 
-void ServiceWorkerVersion::OnCrossOriginConnectEventFinished(
+void ServiceWorkerVersion::OnServicePortConnectEventFinished(
     int request_id,
-    bool accept_connection) {
+    ServicePortConnectResult result,
+    const mojo::String& name,
+    const mojo::String& data) {
   TRACE_EVENT1("ServiceWorker",
-               "ServiceWorkerVersion::OnCrossOriginConnectEventFinished",
+               "ServiceWorkerVersion::OnServicePortConnectEventFinished",
                "Request id", request_id);
-  CrossOriginConnectCallback* callback =
-      cross_origin_connect_callbacks_.Lookup(request_id);
+  ServicePortConnectCallback* callback =
+      service_port_connect_callbacks_.Lookup(request_id);
   if (!callback) {
     NOTREACHED() << "Got unexpected message: " << request_id;
     return;
   }
 
   scoped_refptr<ServiceWorkerVersion> protect(this);
-  callback->Run(SERVICE_WORKER_OK, accept_connection);
-  RemoveCallbackAndStopIfRedundant(&cross_origin_connect_callbacks_,
+  callback->Run(SERVICE_WORKER_OK, result == SERVICE_PORT_CONNECT_RESULT_ACCEPT,
+                name.To<base::string16>(), data.To<base::string16>());
+  RemoveCallbackAndStopIfRedundant(&service_port_connect_callbacks_,
                                    request_id);
 }
 
@@ -1950,16 +1964,12 @@ void ServiceWorkerVersion::StopWorkerIfIdle() {
 }
 
 bool ServiceWorkerVersion::HasInflightRequests() const {
-  return
-    !activate_callbacks_.IsEmpty() ||
-    !install_callbacks_.IsEmpty() ||
-    !fetch_callbacks_.IsEmpty() ||
-    !sync_callbacks_.IsEmpty() ||
-    !notification_click_callbacks_.IsEmpty() ||
-    !push_callbacks_.IsEmpty() ||
-    !geofencing_callbacks_.IsEmpty() ||
-    !cross_origin_connect_callbacks_.IsEmpty() ||
-    !streaming_url_request_jobs_.empty();
+  return !activate_callbacks_.IsEmpty() || !install_callbacks_.IsEmpty() ||
+         !fetch_callbacks_.IsEmpty() || !sync_callbacks_.IsEmpty() ||
+         !notification_click_callbacks_.IsEmpty() ||
+         !push_callbacks_.IsEmpty() || !geofencing_callbacks_.IsEmpty() ||
+         !service_port_connect_callbacks_.IsEmpty() ||
+         !streaming_url_request_jobs_.empty();
 }
 
 void ServiceWorkerVersion::RecordStartWorkerResult(
@@ -2047,10 +2057,11 @@ bool ServiceWorkerVersion::OnRequestTimeout(const RequestInfo& info) {
     case REQUEST_GEOFENCING:
       return RunIDMapCallback(&geofencing_callbacks_, info.id,
                               SERVICE_WORKER_ERROR_TIMEOUT);
-    case REQUEST_CROSS_ORIGIN_CONNECT:
-      return RunIDMapCallback(&cross_origin_connect_callbacks_, info.id,
+    case REQUEST_SERVICE_PORT_CONNECT:
+      return RunIDMapCallback(&service_port_connect_callbacks_, info.id,
                               SERVICE_WORKER_ERROR_TIMEOUT,
-                              false /* accept_connection */);
+                              false /* accept_connection */, base::string16(),
+                              base::string16());
   }
   NOTREACHED() << "Got unexpected request type: " << info.type;
   return false;
@@ -2165,8 +2176,9 @@ void ServiceWorkerVersion::OnStoppedInternal(
                     SERVICE_WORKER_ERROR_FAILED);
   RunIDMapCallbacks(&push_callbacks_, SERVICE_WORKER_ERROR_FAILED);
   RunIDMapCallbacks(&geofencing_callbacks_, SERVICE_WORKER_ERROR_FAILED);
-  RunIDMapCallbacks(&cross_origin_connect_callbacks_,
-                    SERVICE_WORKER_ERROR_FAILED, false);
+  RunIDMapCallbacks(&service_port_connect_callbacks_,
+                    SERVICE_WORKER_ERROR_FAILED, false, base::string16(),
+                    base::string16());
 
   streaming_url_request_jobs_.clear();
 
@@ -2176,4 +2188,18 @@ void ServiceWorkerVersion::OnStoppedInternal(
     StartWorkerInternal(false /* pause_after_download */);
 }
 
+template <typename Interface, typename Callback>
+void ServiceWorkerVersion::OnMojoConnectionError(
+    mojo::InterfacePtr<Interface>* interface,
+    IDMap<Callback, IDMapOwnPointer>* callbacks,
+    const base::Callback<void(const Callback&, ServiceWorkerStatusCode)>
+        callback) {
+  typename IDMap<Callback, IDMapOwnPointer>::iterator iter(callbacks);
+  while (!iter.IsAtEnd()) {
+    callback.Run(*iter.GetCurrentValue(), SERVICE_WORKER_ERROR_FAILED);
+    iter.Advance();
+  }
+  callbacks->Clear();
+}
+
 }  // namespace content