[ServiceWorker] Mojofy InstallEvent of Service Worker

content/browser/service_worker/service_worker_register_job.cc

Index: content/browser/service_worker/service_worker_register_job.cc
diff --git a/content/browser/service_worker/service_worker_register_job.cc b/content/browser/service_worker/service_worker_register_job.cc
index a3c5b69c6803fd001f8002068c1f6eeabc6f19ab..6482f4d18f84b7efc56d03f0c88612cde812d6cb 100644
--- a/content/browser/service_worker/service_worker_register_job.cc
+++ b/content/browser/service_worker/service_worker_register_job.cc
@@ -17,10 +17,12 @@
 #include "content/browser/service_worker/service_worker_registration.h"
 #include "content/browser/service_worker/service_worker_storage.h"
 #include "content/browser/service_worker/service_worker_write_to_cache_job.h"
+#include "content/common/service_worker/service_worker_event_dispatcher.mojom.h"
 #include "content/common/service_worker/service_worker_messages.h"
 #include "content/common/service_worker/service_worker_types.h"
 #include "content/common/service_worker/service_worker_utils.h"
 #include "content/public/browser/browser_thread.h"
+#include "mojo/public/cpp/bindings/strong_associated_binding.h"
 #include "net/base/net_errors.h"
 
 namespace content {
@@ -77,6 +79,37 @@ ServiceWorkerRegisterJob::~ServiceWorkerRegisterJob() {
       << "Jobs should only be interrupted during shutdown.";
 }
 
+class ServiceWorkerRegisterJob::InstallEventMethodsReceiver

[shimazu, 2017/04/24 05:00:13]

This is not necessary to be an inner class of ServcieWorkerRegisterJob. I think
you can implement this in unnamed namespace with passing a raw pointer to the
SWVersion in the constructor.

[xiaofengzhang, 2017/04/25 06:01:29]

Done.

+    : public mojom::ServiceWorkerInstallEventMethods {
+ public:
+  static mojom::ServiceWorkerInstallEventMethodsAssociatedPtrInfo CreateAndBind(
+      const base::WeakPtr<ServiceWorkerRegisterJob>& job) {
+    mojom::ServiceWorkerInstallEventMethodsAssociatedPtrInfo ptr_info;
+    auto request = mojo::MakeRequest(&ptr_info);
+    mojo::MakeStrongAssociatedBinding(

[shimazu, 2017/04/24 05:00:13]

I feel managing its lifecycle in browser process is better to understand the
behavior, so I prefer normal AssociatedBinding for this receiver.
WDYT?

[leonhsl(Using Gerrit), 2017/04/24 05:34:26]

In some cases the callback(OnInstallFinished()) of install event may never be
called because of unexpected disconnection of EventDispatcher mojo interface.
For example, after dispatched Install event, the browser side timer out
happened, ServiceWorkerVersion::OnTimeoutTimer() will close EventDispatcher mojo
connection, or sometimes renderer process crashed unexpectedly. 
With StrongAssociatedBinding, the InstallEventMethodsReceiver instance still can
be destroyed correctly even in above cases.

[shimazu, 2017/04/24 06:22:18]

Thanks for explanation! It makes sense.

If we have the instance as an unique_ptr in the callback of EventDispatcher, it
will be also discarded correctly. 
It's like the following in short:
event_dispatcher->DispatchEvent(base::Bind(&Callback,
base::MakeUnique<Receiver>()));


I prefer having a receiver instance in the callback because this pattern ensures
the lifecycle matches to the event_dispatcher comparing to waiting for a message
of "pipe has been closed" asynchronously. 

[leonhsl(Using Gerrit), 2017/04/24 06:37:38]

Oh I see.. I think you're right, even if EventDispatcher mojo interface
disconnected unexpectedly, although the install callback can not be called, but
mojo will destroy it, so that the base::Passed() unique_ptr could also be
destroyed as well. Thanks!

[xiaofengzhang, 2017/04/25 06:01:29]

Done.

+        base::WrapUnique(new InstallEventMethodsReceiver(job)),

[leonhsl(Using Gerrit), 2017/04/23 03:33:10]

Use base::MakeUnique instead.

[xiaofengzhang, 2017/04/24 05:02:40]

Done.

+        std::move(request));
+    return ptr_info;
+  }
+
+  InstallEventMethodsReceiver(
+      const base::WeakPtr<ServiceWorkerRegisterJob>& job)
+      : job_(job) {}
+  ~InstallEventMethodsReceiver() override {}
+
+  // mojom::ServiceWorkerInstallEventMethod implementation.
+  void RegisterForeignFetchScopes(
+      const std::vector<GURL>& sub_scopes,
+      const std::vector<url::Origin>& origins) override {
+    if (!job_)
+      return;
+    job_->new_version()->RegisterForeignFetchScopes(sub_scopes, origins);
+  }
+
+ private:
+  base::WeakPtr<ServiceWorkerRegisterJob> job_;
+};
+
 void ServiceWorkerRegisterJob::AddCallback(
     const RegistrationCallback& callback,
     ServiceWorkerProviderHost* provider_host) {
@@ -431,34 +464,20 @@ void ServiceWorkerRegisterJob::DispatchInstallEvent() {
       ServiceWorkerMetrics::EventType::INSTALL,
       base::Bind(&ServiceWorkerRegisterJob::OnInstallFailed,
                  weak_factory_.GetWeakPtr()));
-  new_version()
-      ->RegisterRequestCallback<ServiceWorkerHostMsg_InstallEventFinished>(
-          request_id, base::Bind(&ServiceWorkerRegisterJob::OnInstallFinished,
-                                 weak_factory_.GetWeakPtr()));
-  new_version()->DispatchEvent({request_id},
-                               ServiceWorkerMsg_InstallEvent(request_id));
+
+  new_version()->event_dispatcher()->DispatchInstallEvent(
+      InstallEventMethodsReceiver::CreateAndBind(weak_factory_.GetWeakPtr()),
+      base::Bind(&ServiceWorkerRegisterJob::OnInstallFinished,

[shimazu, 2017/04/24 05:00:13]

To clarify InstallEventMethodsReceiver is valid only between
DispatchInstallEvent and OnInstallFinished, how about passing the receiver
instance to OnInstallFinished and remove it in OnInstallFinished?

ex)

  ...
  auto install_event_methods =
base::MakeUnique<InstallEventMethodsReceiver>(...);
  new_version()->event_dispatcher()->DispatchInstallEvent(
    install_event_methods.CreateAndBindInterface(),
    base::Bind(&ServiceWorkerRegisterJob::OnInstallFinished,
               weak_factory_.GetWeakPtr(), request_id, 
               base::Passed(&install_event_methods)));
}

void ServiceWorkerRegisterJob::OnInstallFinished(
    int request_id,
    std::unique_ptr<InstallEventMethodsReceiver> instal_event_methods,
    ...) { 
  install_event_methods.reset();
  ...
}

[xiaofengzhang, 2017/04/25 06:01:28]

Done.

+                 weak_factory_.GetWeakPtr(), request_id));
 }
 
 void ServiceWorkerRegisterJob::OnInstallFinished(
     int request_id,
-    blink::WebServiceWorkerEventResult result,
+    ServiceWorkerStatusCode status,
     bool has_fetch_handler,
     base::Time dispatch_event_time) {
-  new_version()->FinishRequest(
-      request_id, result == blink::kWebServiceWorkerEventResultCompleted,
-      dispatch_event_time);
-
-  ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_FAILED;
-  switch (result) {
-    case blink::kWebServiceWorkerEventResultCompleted:
-      status = SERVICE_WORKER_OK;
-      break;
-    case blink::kWebServiceWorkerEventResultRejected:
-      status = SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED;
-      break;
-    default:
-      NOTREACHED();
-  }
+  new_version()->FinishRequest(request_id, status == SERVICE_WORKER_OK,
+                               dispatch_event_time);
 
   if (status != SERVICE_WORKER_OK) {
     OnInstallFailed(status);