[ServiceWorker] Convert ping-pong IPC into mojo interface ServiceWorkerEventDispatcher

[ServiceWorker] Convert ping-pong IPC into mojo interface ServiceWorkerEventDispatcher

This CL converts the following 2 IPCs
between ServiceWorkerVersion and ServiceWorkerContextClient
into the mojo interface ServiceWorkerEventDispatcher.
  ServiceWorkerMsg_Ping
  ServiceWorkerHostMsg_Pong

BUG=629701
Review-Url: https://codereview.chromium.org/2763453002
Cr-Commit-Position: refs/heads/master@{#458931}
Committed: https://chromium.googlesource.com/chromium/src/+/23f6a12fc807abff832a75e21d7d8df3f12e9949

[leonhsl(Using Gerrit), 2017-03-19 10:39:02]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-19 10:39:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-19 11:01:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-19 11:01:50]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[leonhsl(Using Gerrit), 2017-03-19 11:27:01]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-19 11:27:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[leonhsl(Using Gerrit), 2017-03-19 11:27:11]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-03-19 12:39:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-19 12:39:21]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[leonhsl(Using Gerrit), 2017-03-19 13:44:11]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-19 13:44:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-19 13:56:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-19 13:56:32]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2017-03-20 03:47:14]

leon.han@intel.com changed reviewers:
+ falken@chromium.org, shimazu@chromium.org

[leonhsl(Using Gerrit), 2017-03-20 03:47:15]

PTAL, Thanks! And sorry for the inline question I'm still not sure..

https://codereview.chromium.org/2763453002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/2763453002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:1659:
event_dispatcher()->Ping(base::Bind(&ServiceWorkerVersion::OnPongFromWorker,
I can confirm that |event_dispatcher_| here should be always in bound state. But
I'm not sure what scenario could cause embedded_worker_->SendMessage() fail
before,, maybe when the renderer process exited unexpectedly?

[falken, 2017-03-21 06:54:05]

https://codereview.chromium.org/2763453002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/2763453002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:1659:
event_dispatcher()->Ping(base::Bind(&ServiceWorkerVersion::OnPongFromWorker,
On 2017/03/20 03:47:15, leonhsl wrote:
> I can confirm that |event_dispatcher_| here should be always in bound state.
But
> I'm not sure what scenario could cause embedded_worker_->SendMessage() fail
> before,, maybe when the renderer process exited unexpectedly?

Yes I wasn't entirely sure myself. Sender::Send() returns a bool so the ping
protocol code was written to handle it. I think this patch is OK for Mojo.

https://codereview.chromium.org/2763453002/diff/20001/content/common/service_...
File content/common/service_worker/service_worker_event_dispatcher.mojom
(right):

https://codereview.chromium.org/2763453002/diff/20001/content/common/service_...
content/common/service_worker/service_worker_event_dispatcher.mojom:76: Ping()
=> ();
Ping() doesn't really belong here, because it's not an event. shimazu@ do you
have plans about making new interfaces for all the messages, or perhaps changing
this interface name?

Also, should we have a comment like the messages.h file had?

[shimazu, 2017-03-21 07:59:28]

https://codereview.chromium.org/2763453002/diff/20001/content/common/service_...
File content/common/service_worker/service_worker_event_dispatcher.mojom
(right):

https://codereview.chromium.org/2763453002/diff/20001/content/common/service_...
content/common/service_worker/service_worker_event_dispatcher.mojom:76: Ping()
=> ();
On 2017/03/21 06:54:04, falken wrote:
> Ping() doesn't really belong here, because it's not an event. shimazu@ do you
> have plans about making new interfaces for all the messages, or perhaps
changing
> this interface name?

My understanding is that Ping() is a method for pinging to the SWEventDispatcher
itself. I feel it's okay as is even though Ping is not an event, but I also
agree that Ping() might be confusing because it's not an event.
How about changing the method name like CheckIfResponsible() to clarify that?
In that case, I think it's better to add a blank line before this method.

 
> Also, should we have a comment like the messages.h file had?

+1 to having a comment.

[falken, 2017-03-21 14:00:45]

https://codereview.chromium.org/2763453002/diff/20001/content/common/service_...
File content/common/service_worker/service_worker_event_dispatcher.mojom
(right):

https://codereview.chromium.org/2763453002/diff/20001/content/common/service_...
content/common/service_worker/service_worker_event_dispatcher.mojom:76: Ping()
=> ();
On 2017/03/21 07:59:27, shimazu wrote:
> On 2017/03/21 06:54:04, falken wrote:
> > Ping() doesn't really belong here, because it's not an event. shimazu@ do
you
> > have plans about making new interfaces for all the messages, or perhaps
> changing
> > this interface name?
> 
> My understanding is that Ping() is a method for pinging to the
SWEventDispatcher
> itself. I feel it's okay as is even though Ping is not an event, but I also
> agree that Ping() might be confusing because it's not an event.
> How about changing the method name like CheckIfResponsible() to clarify that?
> In that case, I think it's better to add a blank line before this method.
> 

Hm, I think of Ping() as pinging the service worker itself. I'm not sure
ServiceWorkerEventDispatcher is the right name if it's just the interface for
IPCs sent from the browser to the service worker. Are all the existing legacy
IPC messages going to be added to ServiceWorkerEventDispatcher? How many of them
are not about dispatching events? If there are many, maybe
ServiceWorkerEventDispatcher should be renamed something like
ServiceWorkerDispatcher.

[shimazu, 2017-03-22 01:32:29]

https://codereview.chromium.org/2763453002/diff/20001/content/common/service_...
File content/common/service_worker/service_worker_event_dispatcher.mojom
(right):

https://codereview.chromium.org/2763453002/diff/20001/content/common/service_...
content/common/service_worker/service_worker_event_dispatcher.mojom:76: Ping()
=> ();
On 2017/03/21 14:00:45, falken wrote:
> On 2017/03/21 07:59:27, shimazu wrote:
> > On 2017/03/21 06:54:04, falken wrote:
> > > Ping() doesn't really belong here, because it's not an event. shimazu@ do
> you
> > > have plans about making new interfaces for all the messages, or perhaps
> > changing
> > > this interface name?
> > 
> > My understanding is that Ping() is a method for pinging to the
> SWEventDispatcher
> > itself. I feel it's okay as is even though Ping is not an event, but I also
> > agree that Ping() might be confusing because it's not an event.
> > How about changing the method name like CheckIfResponsible() to clarify
that?
> > In that case, I think it's better to add a blank line before this method.
> > 
> 
> Hm, I think of Ping() as pinging the service worker itself. I'm not sure
> ServiceWorkerEventDispatcher is the right name if it's just the interface for
> IPCs sent from the browser to the service worker. Are all the existing legacy
> IPC messages going to be added to ServiceWorkerEventDispatcher? How many of
them
> are not about dispatching events? If there are many, maybe
> ServiceWorkerEventDispatcher should be renamed something like
> ServiceWorkerDispatcher.

I totally agree with that. ServiceWorkerEventDisptacher is the interface
representing the context of a service worker used from the browser process.

However, Ping() will be the only method except for Dispatch*Event
ServiceWorkerEventDispatcher will have. 
// Link to the design doc
https://docs.google.com/document/d/1xW-NXKmqKu7pQ0hTNz9rpdGXgNaItyonOySVigXdq...

ServiceWorkerEventDisptacher is possible to be considered as a counterpart of
ServiceWorkerGlobalScopeHost in the desgin doc, but I feel
ServiceWorkerEventDispatcher is clearer than "ServiceWorkerGlobalScope" or
"ServiceWorkerDispatcher" (but both have been already taken) because the
interface will have only Dispatch*Event except for Ping.

What do you think?

[falken, 2017-03-22 02:15:57]

https://codereview.chromium.org/2763453002/diff/20001/content/common/service_...
File content/common/service_worker/service_worker_event_dispatcher.mojom
(right):

https://codereview.chromium.org/2763453002/diff/20001/content/common/service_...
content/common/service_worker/service_worker_event_dispatcher.mojom:76: Ping()
=> ();
On 2017/03/22 01:32:29, shimazu wrote:
> On 2017/03/21 14:00:45, falken wrote:
> > On 2017/03/21 07:59:27, shimazu wrote:
> > > On 2017/03/21 06:54:04, falken wrote:
> > > > Ping() doesn't really belong here, because it's not an event. shimazu@
do
> > you
> > > > have plans about making new interfaces for all the messages, or perhaps
> > > changing
> > > > this interface name?
> > > 
> > > My understanding is that Ping() is a method for pinging to the
> > SWEventDispatcher
> > > itself. I feel it's okay as is even though Ping is not an event, but I
also
> > > agree that Ping() might be confusing because it's not an event.
> > > How about changing the method name like CheckIfResponsible() to clarify
> that?
> > > In that case, I think it's better to add a blank line before this method.
> > > 
> > 
> > Hm, I think of Ping() as pinging the service worker itself. I'm not sure
> > ServiceWorkerEventDispatcher is the right name if it's just the interface
for
> > IPCs sent from the browser to the service worker. Are all the existing
legacy
> > IPC messages going to be added to ServiceWorkerEventDispatcher? How many of
> them
> > are not about dispatching events? If there are many, maybe
> > ServiceWorkerEventDispatcher should be renamed something like
> > ServiceWorkerDispatcher.
> 
> I totally agree with that. ServiceWorkerEventDisptacher is the interface
> representing the context of a service worker used from the browser process.
> 
> However, Ping() will be the only method except for Dispatch*Event
> ServiceWorkerEventDispatcher will have. 
> // Link to the design doc
>
https://docs.google.com/document/d/1xW-NXKmqKu7pQ0hTNz9rpdGXgNaItyonOySVigXdq...
> 
> ServiceWorkerEventDisptacher is possible to be considered as a counterpart of
> ServiceWorkerGlobalScopeHost in the desgin doc, but I feel
> ServiceWorkerEventDispatcher is clearer than "ServiceWorkerGlobalScope" or
> "ServiceWorkerDispatcher" (but both have been already taken) because the
> interface will have only Dispatch*Event except for Ping.
> 
> What do you think?

OK, if it's the only exception I think we don't need to worry about it. I'd like
a comment above Ping() explaining what it is though, something like:

// Pings the service worker to check if it is responsive. If the callback is not
called within a certain period of time, the browser will terminate the worker.
Unlike the other functions in this interface, Ping() does not dispatch an event.

I slightly prefer Ping() over CheckIfResponsive() to align with the
ServiceWorkerVersion::Ping() naming.

[leonhsl(Using Gerrit), 2017-03-22 03:24:54]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 03:25:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 05:27:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 05:27:12]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't
started yet, builder probably lacks capacity)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT,
build hasn't started yet, builder probably lacks capacity)

[leonhsl(Using Gerrit), 2017-03-22 05:28:26]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 05:29:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[leonhsl(Using Gerrit), 2017-03-22 05:31:48]

Thank you very much for all the clear clarifications! Uploaded ps#2 based on
discussion result, PTAnL, Thanks.

[falken, 2017-03-22 05:46:30]

lgtm but let's wait for shimazu as the mojo expert

https://codereview.chromium.org/2763453002/diff/40001/content/common/service_...
File content/common/service_worker/service_worker_event_dispatcher.mojom
(right):

https://codereview.chromium.org/2763453002/diff/40001/content/common/service_...
content/common/service_worker/service_worker_event_dispatcher.mojom:81: Ping()
=> ();
Just to be sure, we are guaranteed Ping() is called on the service worker
thread, right?

[leonhsl(Using Gerrit), 2017-03-22 06:45:59]

Thanks for review!

https://codereview.chromium.org/2763453002/diff/40001/content/common/service_...
File content/common/service_worker/service_worker_event_dispatcher.mojom
(right):

https://codereview.chromium.org/2763453002/diff/40001/content/common/service_...
content/common/service_worker/service_worker_event_dispatcher.mojom:81: Ping()
=> ();
On 2017/03/22 05:46:30, falken wrote:
> Just to be sure, we are guaranteed Ping() is called on the service worker
> thread, right?

From my current knowledge, in browser side SWVersion calls this mojo function
Ping() on IO thread, and in renderer side SWContextClient executes its impl of
Ping() on its own service worker thread. Hopefully I'm understanding correctly
:)

[shimazu, 2017-03-22 06:46:26]

lgtm, too.

Thanks! :)

https://codereview.chromium.org/2763453002/diff/40001/content/common/service_...
File content/common/service_worker/service_worker_event_dispatcher.mojom
(right):

https://codereview.chromium.org/2763453002/diff/40001/content/common/service_...
content/common/service_worker/service_worker_event_dispatcher.mojom:81: Ping()
=> ();
On 2017/03/22 05:46:30, falken wrote:
> Just to be sure, we are guaranteed Ping() is called on the service worker
> thread, right?

Yeah, ServiceWorkerEventDispather is bound after the worker thread has started.

[commit-bot: I haz the power, 2017-03-22 07:27:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 07:27:36]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[leonhsl(Using Gerrit), 2017-03-22 07:28:27]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 07:28:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 08:53:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 08:53:54]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2017-03-22 09:13:54]

leon.han@intel.com changed reviewers:
+ tsepez@chromium.org

[leonhsl(Using Gerrit), 2017-03-22 09:13:55]

Thanks all for kindly review!

Hi, Tom, would you PTAL for OWNER review of:
  service_worker_event_dispatcher.mojom
  service_worker_messages.h

[Tom Sepez, 2017-03-22 17:14:24]

lgtm

[leonhsl(Using Gerrit), 2017-03-22 23:25:21]

The CQ bit was checked by leon.han@intel.com

[commit-bot: I haz the power, 2017-03-22 23:26:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 23:34:52]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490225121140150,
"parent_rev": "12e779b85deea4e5752fc02631aa9c3718aeb63e", "commit_rev":
"23f6a12fc807abff832a75e21d7d8df3f12e9949"}

[commit-bot: I haz the power, 2017-03-22 23:35:40]

Description was changed from

==========
[ServiceWorker] Convert ping-pong IPC into mojo interface
ServiceWorkerEventDispatcher

This CL converts the following 2 IPCs
between ServiceWorkerVersion and ServiceWorkerContextClient
into the mojo interface ServiceWorkerEventDispatcher.
  ServiceWorkerMsg_Ping
  ServiceWorkerHostMsg_Pong

BUG=629701
==========

to

==========
[ServiceWorker] Convert ping-pong IPC into mojo interface
ServiceWorkerEventDispatcher

This CL converts the following 2 IPCs
between ServiceWorkerVersion and ServiceWorkerContextClient
into the mojo interface ServiceWorkerEventDispatcher.
  ServiceWorkerMsg_Ping
  ServiceWorkerHostMsg_Pong

BUG=629701

Review-Url: https://codereview.chromium.org/2763453002
Cr-Commit-Position: refs/heads/master@{#458931}
Committed:
https://chromium.googlesource.com/chromium/src/+/23f6a12fc807abff832a75e21d7d...
==========

[commit-bot: I haz the power, 2017-03-22 23:35:42]

Committed patchset #2 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/23f6a12fc807abff832a75e21d7d...