[ServiceWorker] Mojofy PushEvent of Service Worker.

content/browser/push_messaging/push_messaging_router.cc

 // Copyright 2014 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/push_messaging/push_messaging_router.h"
 
 #include <string>
 
 #include "base/bind.h"
 #include "base/metrics/histogram_macros.h"
@@ skipping 105 matching lines @@
     const scoped_refptr<ServiceWorkerRegistration>& service_worker_registration,
     const PushEventPayload& payload,
     const DeliverMessageCallback& deliver_message_callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   int request_id = service_worker->StartRequestWithCustomTimeout(
       ServiceWorkerMetrics::EventType::PUSH,
       base::Bind(&PushMessagingRouter::DeliverMessageEnd,
                  deliver_message_callback, service_worker_registration),
       base::TimeDelta::FromSeconds(kPushMessageTimeoutSeconds),
       ServiceWorkerVersion::KILL_ON_TIMEOUT);
-  service_worker->DispatchSimpleEvent<ServiceWorkerHostMsg_PushEventFinished>(
-      request_id, ServiceWorkerMsg_PushEvent(request_id, payload));
+
+  // |event_dispatcher| is owned by |service_worker|, once |service_worker|
+  // got destroyed, the bound function will never be called, so it is safe to
+  // use

[shimazu, 2016/12/14 04:42:34]

nit: please fix indent

[xiaofengzhang, 2016/12/15 01:30:49]

Acknowledged.

+  // base::Unretained() here.
+  service_worker->event_dispatcher()->DispatchPushEvent(
+      payload, base::Bind(&ServiceWorkerVersion::OnSimpleEventFinished,
+                          base::Unretained(service_worker.get()), request_id));

[Peter Beverloo, 2016/12/13 13:31:32]

Instead of using base::Unretained(), just pass |service_worker| directly. It's a
scoped_refptr so that should work just fine, and that allows you to drop the
comment.

Ideally this entire class would stop using const& for the scoped_refptr<>
arguments. That would allow us to use std::move() instead of making copies. But
that's a clean-up for another time :).

[leonhsl(Using Gerrit), 2016/12/14 03:29:18]

I have one question which I'm not so sure: This way the bound callback would
hold an additional reference on |service_worker|, then only after this callback
got fired or destroyed, |service_worker| would got chance to be destroyed, this
would be a little different with previous scenario of |service_worker|
lifecycle?

[xiaofengzhang, 2016/12/15 01:30:48]

I re-submit and pass |service_worker| directly, but please also see leon's
question.
For "stop using const& for the scoped_refptr", yes, I will do it in another next
cl. Thanks :)

[xiaofengzhang, 2016/12/16 01:34:48]

Any comments on Leon's question:
"I have one question which I'm not so sure: This way the bound callback would
hold an additional reference on |service_worker|, then only after this callback
got fired or destroyed, |service_worker| would got chance to be destroyed, this
would be a little different with previous scenario of |service_worker|
lifecycle?"

 }
 
 // static
 void PushMessagingRouter::DeliverMessageEnd(
     const DeliverMessageCallback& deliver_message_callback,
     const scoped_refptr<ServiceWorkerRegistration>& service_worker_registration,
     ServiceWorkerStatusCode service_worker_status) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   UMA_HISTOGRAM_ENUMERATION("PushMessaging.DeliveryStatus.ServiceWorkerEvent",
                             service_worker_status,
@@ skipping 31 matching lines @@
     case SERVICE_WORKER_ERROR_MAX_VALUE:
       NOTREACHED() << "Got unexpected error code: " << service_worker_status
                    << " " << ServiceWorkerStatusToString(service_worker_status);
       delivery_status = PUSH_DELIVERY_STATUS_SERVICE_WORKER_ERROR;
       break;
   }
   RunDeliverCallback(deliver_message_callback, delivery_status);
 }
 
 }  // namespace content