[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..621a35434aa0dae59b793b56789c66a33ca9a856 100644
--- a/content/browser/service_worker/service_worker_register_job.cc
+++ b/content/browser/service_worker/service_worker_register_job.cc
@@ -17,16 +17,47 @@
 #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/associated_binding.h"
 #include "net/base/net_errors.h"
 
 namespace content {
 
 namespace {
 
+class InstallEventMethodsReceiver
+    : public mojom::ServiceWorkerInstallEventMethods {
+ public:
+  InstallEventMethodsReceiver(ServiceWorkerVersion* version)
+      : version_(version), install_methods_binding_(this) {}
+  ~InstallEventMethodsReceiver() override {}
+
+  void BindInterface(
+      mojom::ServiceWorkerInstallEventMethodsAssociatedPtrInfo* ptr_info) {
+    install_methods_binding_.Bind(ptr_info);
+  }
+
+  // mojom::ServiceWorkerInstallEventMethod implementation.

[shimazu, 2017/04/27 02:36:44]

nit: We usually use a style of "Implements SomeInterface.".
Could you change this to "Implements mojom::ServiceWorkerInstallEventMethods."?

[xiaofengzhang, 2017/04/27 05:15:32]

Sure. I refered to service_worker_register_job.h line 146 & 69.

+  void RegisterForeignFetchScopes(
+      const std::vector<GURL>& sub_scopes,
+      const std::vector<url::Origin>& origins) override {
+    DCHECK(version_);
+    version_->RegisterForeignFetchScopes(sub_scopes, origins);
+  }
+
+ private:
+  ServiceWorkerVersion* version_;
+
+  mojo::AssociatedBinding<mojom::ServiceWorkerInstallEventMethods>
+      install_methods_binding_;
+
+  DISALLOW_COPY_AND_ASSIGN(InstallEventMethodsReceiver);
+};
+
 void RunSoon(const base::Closure& closure) {
   base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, closure);
 }
@@ -431,34 +462,27 @@ 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));
+
+  std::unique_ptr<InstallEventMethodsReceiver> install_event_methods =
+      base::MakeUnique<InstallEventMethodsReceiver>(new_version());
+  mojom::ServiceWorkerInstallEventMethodsAssociatedPtrInfo ptr_info;

[shimazu, 2017/04/27 02:36:44]

Why can this solve the test failures?
I'm also not sure what's the cause of failures on Bind.

I'll also investigate it, but if you have any idea, please let me know:)

[xiaofengzhang, 2017/04/27 05:15:32]

It's seems the test case passed.
We just guessed that if take "install_event_methods->CreateAndBindInterface()"
as the parameter of "new_version()->event_dispatcher()->DispatchInstallEvent()",
the execution order of it and "base::Passed(&install_event_methods)" maybe not
certain?

[xiaofengzhang, 2017/04/27 05:39:27]

And do you think it's better to define a static function in
InstallEventMethodsReceiver as below:

static std::unique_ptr<InstallEventMethodsReceiver>
CreateAndBindInterface(ServiceWorkerVersion* version,
            mojom::ServiceWorkerInstallEventMethodsAssociatedPtrInfo* ptr_info)
{
    auto receiver_ptr = base::MakeUnique<InstallEventMethodsReceiver>(version);
    receiver_ptr->install_methods_binding_.Bind(ptr_info);
    return receiver_ptr;
}

and use it here:

  std::unique_ptr<InstallEventMethodsReceiver> install_methods_receiver =
InstallEventMethodsReceiver::CreateAndBindInterface(new_version(), &ptr_info);

[shimazu, 2017/04/27 06:16:42]

I got it. That seems the problem.
I think it's okay as is. 

+  install_event_methods->BindInterface(&ptr_info);
+  new_version()->event_dispatcher()->DispatchInstallEvent(
+      std::move(ptr_info),
+      base::Bind(&ServiceWorkerRegisterJob::OnInstallFinished,
+                 weak_factory_.GetWeakPtr(), request_id,
+                 base::Passed(&install_event_methods)));
 }
 
 void ServiceWorkerRegisterJob::OnInstallFinished(
     int request_id,
-    blink::WebServiceWorkerEventResult result,
+    std::unique_ptr<InstallEventMethodsReceiver> install_event_methods,
+    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();
-  }
+  install_event_methods.reset();
+  new_version()->FinishRequest(request_id, status == SERVICE_WORKER_OK,
+                               dispatch_event_time);
 
   if (status != SERVICE_WORKER_OK) {
     OnInstallFailed(status);