[ServiceWorker] Remove useless ServiceWorkerVersion::DispatchSimpleEvent().

[ServiceWorker] Remove useless ServiceWorkerVersion::DispatchSimpleEvent().

As all simple events have been converted into mojo interface
ServiceWorkerEventDispatcher, now there are no codes using
ServiceWorkerVersion::DispatchSimpleEvent(), so this CL removes it
completely.
No other code behavior change.

BUG=629701
TBR=tsepez@chromium.org
for adding comment into service_worker_event_dispatcher.mojom

Review-Url: https://codereview.chromium.org/2779093003
Cr-Commit-Position: refs/heads/master@{#460744}
Committed: https://chromium.googlesource.com/chromium/src/+/29783ad5a5cbc0569d4a5578c82feca23223a621

[leonhsl(Using Gerrit), 2017-03-29 12:02:58]

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

[commit-bot: I haz the power, 2017-03-29 12:03: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-29 12:40:04]

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

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

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

[leonhsl(Using Gerrit), 2017-03-29 12:53:28]

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

[commit-bot: I haz the power, 2017-03-29 12:53:42]

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-29 13:27:25]

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

[commit-bot: I haz the power, 2017-03-29 13:27:26]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2017-03-30 03:18:24]

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

[leonhsl(Using Gerrit), 2017-03-30 03:18:24]

A clean up CL, PTAL, Thanks.

[shimazu, 2017-03-30 03:32:21]

lgtm, Thanks!

[falken, 2017-03-30 03:51:39]

lgtm. It's a bit awkward that ServiceWorkerVersion still has a concept of
"simple event" (from CreateSimpleEventCallback and SimpleEventCallback) even
though there is no mention of dispatching simple events or what a simple event
is.

Ideally the mojom callbacks for (blink.mojom.ServiceWorkerEventStatus status,   
      mojo.common.mojom.Time dispatch_event_time);

in service_worker_event_dispatcher.mojom would have some documentation or
typename to make clear that this is the "simple event" being talked about in
ServiceWorkerVersion.

https://codereview.chromium.org/2779093003/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_version_unittest.cc (left):

https://codereview.chromium.org/2779093003/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_version_unittest.cc:1560:
TEST_F(ServiceWorkerVersionTest, DispatchSimpleEvent_Completed) {
I assume this test coverage is elsewhere for the Mojo-based events?

[leonhsl(Using Gerrit), 2017-03-30 07:13:10]

Yeah I agree we'd better make some connections between events in .mojom file and
the 'simple event' notion in ServiceWorkerVersion. I checked around and seems no
good way to put something like typename into .mojom file, so can we just add a
comment for interface ServiceWorkerEventDispatcher like:
'
Those events expecting such response (blink.mojom.ServiceWorkerEventStatus
status, mojo.common.mojom.Time dispatch_event_time) are dispatched as 'simple
event'. See ServiceWorkerVersion for implementation details.
'

https://codereview.chromium.org/2779093003/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_version_unittest.cc (left):

https://codereview.chromium.org/2779093003/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_version_unittest.cc:1560:
TEST_F(ServiceWorkerVersionTest, DispatchSimpleEvent_Completed) {
On 2017/03/30 03:51:39, falken wrote:
> I assume this test coverage is elsewhere for the Mojo-based events?

This unit test is for testing SWVersion::DispatchSimpleEvent<>, the goal is to
verify that the mechanism composed by {template DispatchSimpleEvent<>, template
RegisterSimpleRequest<>, function OnSimpleEventResponse()} can work correctly,
now we're removing this mechanism, so I think we can remove all its unit tests
together.

As for the equivalent test for Mojo-based events, I think
service_worker_browsertest.cc is covering some cases, activate and fetch event
etc. I think they're already verifying that the Mojo-based event dispatch
mechanism can work correctly.

[falken, 2017-03-30 07:50:15]

On 2017/03/30 07:13:10, leonhsl wrote:
> Yeah I agree we'd better make some connections between events in .mojom file
and
> the 'simple event' notion in ServiceWorkerVersion. I checked around and seems
no
> good way to put something like typename into .mojom file, so can we just add a
> comment for interface ServiceWorkerEventDispatcher like:
> '
> Those events expecting such response (blink.mojom.ServiceWorkerEventStatus
> status, mojo.common.mojom.Time dispatch_event_time) are dispatched as 'simple
> event'. See ServiceWorkerVersion for implementation details.
> '

SGTM. We should additionally a comment in SWVersion in the other direction.

> 
>
https://codereview.chromium.org/2779093003/diff/1/content/browser/service_wor...
> File content/browser/service_worker/service_worker_version_unittest.cc (left):
> 
>
https://codereview.chromium.org/2779093003/diff/1/content/browser/service_wor...
> content/browser/service_worker/service_worker_version_unittest.cc:1560:
> TEST_F(ServiceWorkerVersionTest, DispatchSimpleEvent_Completed) {
> On 2017/03/30 03:51:39, falken wrote:
> > I assume this test coverage is elsewhere for the Mojo-based events?
> 
> This unit test is for testing SWVersion::DispatchSimpleEvent<>, the goal is to
> verify that the mechanism composed by {template DispatchSimpleEvent<>,
template
> RegisterSimpleRequest<>, function OnSimpleEventResponse()} can work correctly,
> now we're removing this mechanism, so I think we can remove all its unit tests
> together.
> 
> As for the equivalent test for Mojo-based events, I think
> service_worker_browsertest.cc is covering some cases, activate and fetch event
> etc. I think they're already verifying that the Mojo-based event dispatch
> mechanism can work correctly.

Sure, OK.

[leonhsl(Using Gerrit), 2017-03-30 08:17: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-30 08:17:22]

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-30 08:17:59]

Uploaded ps#2, PTAnL, Thanks.

[falken, 2017-03-30 08:21:29]

thanks, stil lgtm with a comment tweak request

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

https://codereview.chromium.org/2779093003/diff/20001/content/common/service_...
content/common/service_worker/service_worker_event_dispatcher.mojom:59: // as
'simple event'. See ServiceWorkerVersion for implementation details.
I would rather phrase this as "... are considered 'simple events'.
ServiceWorkerVersion::CreateSimpleEventCallback can be used to create the
callback for these".

Otherwise it sounds like there is a material difference between how to dispatch
a simple event and non-simple event.

[commit-bot: I haz the power, 2017-03-30 09:43:21]

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

[commit-bot: I haz the power, 2017-03-30 09:43:22]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2017-03-30 09:45:37]

Description was changed from

==========
[ServiceWorker] Remove useless ServiceWorkerVersion::DispatchSimpleEvent().

As all simple events have been converted into mojo interface
ServiceWorkerEventDispatcher, now there are no codes using
ServiceWorkerVersion::DispatchSimpleEvent(), so this CL removes it
completely.
No other code behavior change.

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

to

==========
[ServiceWorker] Remove useless ServiceWorkerVersion::DispatchSimpleEvent().

As all simple events have been converted into mojo interface
ServiceWorkerEventDispatcher, now there are no codes using
ServiceWorkerVersion::DispatchSimpleEvent(), so this CL removes it
completely.
No other code behavior change.

BUG=629701
TBR=tsepez@chromium.org
for adding comment into service_worker_event_dispatcher.mojom
==========

[leonhsl(Using Gerrit), 2017-03-30 09:46:34]

Thanks! Let me TBR Tom for adding comment into
service_worker_event_dispatcher.mojom, send to CQ now.

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

https://codereview.chromium.org/2779093003/diff/20001/content/common/service_...
content/common/service_worker/service_worker_event_dispatcher.mojom:59: // as
'simple event'. See ServiceWorkerVersion for implementation details.
On 2017/03/30 08:21:29, falken wrote:
> I would rather phrase this as "... are considered 'simple events'.
> ServiceWorkerVersion::CreateSimpleEventCallback can be used to create the
> callback for these".
> 
> Otherwise it sounds like there is a material difference between how to
dispatch
> a simple event and non-simple event.

Done.

[leonhsl(Using Gerrit), 2017-03-30 09:46:47]

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

[leonhsl(Using Gerrit), 2017-03-30 09:46:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
shimazu@chromium.org, falken@chromium.org
Link to the patchset: https://codereview.chromium.org/2779093003/#ps40001
(title: "Refine comment")

[commit-bot: I haz the power, 2017-03-30 09:47:00]

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-30 09:47:08]

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

[commit-bot: I haz the power, 2017-03-30 11:08:48]

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

[commit-bot: I haz the power, 2017-03-30 11:08:49]

Try jobs failed on following builders:
  linux_chromium_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-30 12:24:49]

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

[commit-bot: I haz the power, 2017-03-30 12:25:07]

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-30 13:42:43]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490876689832950,
"parent_rev": "39d403eb61f5598b7308a36decfbfe8aaee3e97d", "commit_rev":
"29783ad5a5cbc0569d4a5578c82feca23223a621"}

[commit-bot: I haz the power, 2017-03-30 13:43:31]

Description was changed from

==========
[ServiceWorker] Remove useless ServiceWorkerVersion::DispatchSimpleEvent().

As all simple events have been converted into mojo interface
ServiceWorkerEventDispatcher, now there are no codes using
ServiceWorkerVersion::DispatchSimpleEvent(), so this CL removes it
completely.
No other code behavior change.

BUG=629701
TBR=tsepez@chromium.org
for adding comment into service_worker_event_dispatcher.mojom
==========

to

==========
[ServiceWorker] Remove useless ServiceWorkerVersion::DispatchSimpleEvent().

As all simple events have been converted into mojo interface
ServiceWorkerEventDispatcher, now there are no codes using
ServiceWorkerVersion::DispatchSimpleEvent(), so this CL removes it
completely.
No other code behavior change.

BUG=629701
TBR=tsepez@chromium.org
for adding comment into service_worker_event_dispatcher.mojom

Review-Url: https://codereview.chromium.org/2779093003
Cr-Commit-Position: refs/heads/master@{#460744}
Committed:
https://chromium.googlesource.com/chromium/src/+/29783ad5a5cbc0569d4a5578c82f...
==========

[commit-bot: I haz the power, 2017-03-30 13:43:32]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/29783ad5a5cbc0569d4a5578c82f...