[ServiceWorker] Mojofy ActivateEvent of Service Worker

content/browser/service_worker/service_worker_registration.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/service_worker/service_worker_registration.h"
 
 #include <vector>
 
 #include "base/threading/thread_task_runner_handle.h"
 #include "content/browser/service_worker/embedded_worker_status.h"
@@ skipping 409 matching lines @@
     return;
   }
 
   DCHECK_EQ(ServiceWorkerVersion::ACTIVATING, activating_version->status());
   DCHECK_EQ(EmbeddedWorkerStatus::RUNNING, activating_version->running_status())
       << "Worker stopped too soon after it was started.";
   int request_id = activating_version->StartRequest(
       ServiceWorkerMetrics::EventType::ACTIVATE,
       base::Bind(&ServiceWorkerRegistration::OnActivateEventFinished, this,
                  activating_version));
-  activating_version
-      ->DispatchSimpleEvent<ServiceWorkerHostMsg_ActivateEventFinished>(
-          request_id, ServiceWorkerMsg_ActivateEvent(request_id));
+  activating_version->event_dispatcher()->DispatchActivateEvent(
+      base::Bind(&ServiceWorkerVersion::OnSimpleEventFinished,
+                 activating_version, request_id));

[shimazu, 2017/02/07 04:43:48]

Could you use base::Unretained(activating_version.get()) instead of passing
scoped_refptr<SWVersion> to avoid reference loop (version -> dispatcher ->
callback -> version)

[xiaofengzhang, 2017/02/07 06:15:36]

Sure. But I am a little confused that other places in this file always use
activating_version directly, such as line 426 and 342.., and in my previous
patch(https://codereview.chromium.org/2569993002/), Peter comments that I should
pass it directly for it is the scoped_refptr.

[shimazu, 2017/02/07 07:19:31]

Oops, I didn't realize that. I think base::Unretained(activating_version.get())
is better as I mentioned. (that might be confusing a little, though.)
peter@: WDYT?

[Peter Beverloo, 2017/02/08 15:13:16]

That's great context - thanks shimazu!

It would be very helpful if the Dispatch*Event methods could be annotated with a
comment that says it's safe to use callbacks pointing to an (active) version w/
base::Unretained(), and why.

Generally it's a bit scary seeing base::Unretained() in the code.

[shimazu, 2017/02/10 05:42:27]

Yeah, I also feel it's scary. 
How about having a helper method to create a callback to call
OnSimpleEventFinished with base::Unretained instead of using base::Unretained
explicitly?

==
// Creates a callback passed to event_dispatcher().
StatusCallback
ServiceWorkerVersion::CreateSimpleEventCallback(int request_id) {
  // base::Unretained(this) should be used here otherwise
  // version->event_dispatcher()->DispatchSomeEvent(callback) would have a
  // reference loop.
  return base::Bind(&ServiceWorkerVersion::OnSimpleEventFinished,
                    base::Unretained(this), request_id);
}
==

 }
 
 void ServiceWorkerRegistration::OnActivateEventFinished(
     scoped_refptr<ServiceWorkerVersion> activating_version,
     ServiceWorkerStatusCode status) {
   // Activate is prone to failing due to shutdown, because it's triggered when
   // tabs close.
   bool is_shutdown =
       !context_ || context_->wrapper()->process_manager()->IsShutdown();
   ServiceWorkerMetrics::RecordActivateEventStatus(status, is_shutdown);
@@ skipping 71 matching lines @@
   if (!context_) {
     callback.Run(SERVICE_WORKER_ERROR_ABORT);
     return;
   }
   context_->storage()->NotifyDoneInstallingRegistration(
       this, version.get(), status);
   callback.Run(status);
 }
 
 }  // namespace content