[ServiceWorker] Mojofy extendable message event.

[ServiceWorker] Mojofy extendable message event.

This CL converts extendable message event IPCs into mojo interface:
  ServiceWorkerMsg_ExtendableMessageEvent
  ServiceWorkerHostMsg_ExtendableMessageEventFinished

BUG=629701
TEST=content_unittests

Committed: https://crrev.com/b0aeab4ccafc6cb37e3f16a2dd4f214f9b56dda3
Cr-Commit-Position: refs/heads/master@{#437462}

[leonhsl(Using Gerrit), 2016-11-30 12:14:20]

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

[commit-bot: I haz the power, 2016-11-30 12:14:37]

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, 2016-11-30 12:41:23]

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

[commit-bot: I haz the power, 2016-11-30 12:41:23]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[leonhsl(Using Gerrit), 2016-12-01 06:16:29]

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

[leonhsl(Using Gerrit), 2016-12-01 06:16:43]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-12-01 06:16:45]

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, 2016-12-01 06:19:02]

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

[commit-bot: I haz the power, 2016-12-01 06:19:02]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[leonhsl(Using Gerrit), 2016-12-01 06:54:23]

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

[commit-bot: I haz the power, 2016-12-01 06:54:43]

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, 2016-12-01 07:34:01]

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

[commit-bot: I haz the power, 2016-12-01 07:34:02]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[leonhsl(Using Gerrit), 2016-12-01 07:56:27]

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

[commit-bot: I haz the power, 2016-12-01 07:56:42]

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), 2016-12-01 08:35:28]

Description was changed from

==========
[ServiceWorker] Mojofy extendable message event.

This CL converts extendable message event IPCs into mojo interface:
  ServiceWorkerMsg_ExtendableMessageEvent
  ServiceWorkerHostMsg_ExtendableMessageEventFinished

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

to

==========
[ServiceWorker] Mojofy extendable message event.

This CL converts extendable message event IPCs into mojo interface:
  ServiceWorkerMsg_ExtendableMessageEvent
  ServiceWorkerHostMsg_ExtendableMessageEventFinished

BUG=629701
TEST=content_unittests
==========

[leonhsl(Using Gerrit), 2016-12-01 08:43:40]

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

[leonhsl(Using Gerrit), 2016-12-01 08:43:41]

Hi, shimazu@, would you please help to take the first round review? Thanks.

[commit-bot: I haz the power, 2016-12-01 08:52:13]

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

[commit-bot: I haz the power, 2016-12-01 08:52:14]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[leonhsl(Using Gerrit), 2016-12-02 02:38:51]

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

[commit-bot: I haz the power, 2016-12-02 02:39:24]

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

[shimazu, 2016-12-02 04:00:33]

Thanks for working this! Overall looks good.

The failure of linux_chromium_asan_rel_ng seems to be caused by the IPC ordering
between ExtendableMessageEvent and BackgroundSyncEvent. I think we have to
figure out whether that is a problem of the test or this patch breaks the
necessary ordering.

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

https://codereview.chromium.org/2534403002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:1090: void
ServiceWorkerVersion::OnSimpleEventResponse(
How about deduping the codes by calling OnSimpleEventFinished in
OnSimpleEventResponse?

and I think the names should be the same because there is no difference between
the two. I prefer OnSimpleEventFinished to OnSimpleEventResponse because it's
clearer.

https://codereview.chromium.org/2534403002/diff/60001/content/common/service_...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/2534403002/diff/60001/content/common/service_...
content/common/service_worker/service_worker_messages.h:55:
IPC_STRUCT_TRAITS_BEGIN(content::ExtendableMessageEventSource)
Could you remove this and write StructTraits in
service_worker_types_traits.cc/h?

https://codereview.chromium.org/2534403002/diff/60001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2534403002/diff/60001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.cc:680: if
(!callback)
DCHECK(callback) is better here because IIUC callback must not null here.

https://codereview.chromium.org/2534403002/diff/60001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.h (right):

https://codereview.chromium.org/2534403002/diff/60001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.h:230: void
DispatchExtendableMessageEvent(
Could you move this to l.222?
I think it's better to sort them by alphabetical order.

https://codereview.chromium.org/2534403002/diff/60001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.h:235: void
OnExtendableMessageEvent(
Could you remove this?

[commit-bot: I haz the power, 2016-12-02 04:24:44]

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

[commit-bot: I haz the power, 2016-12-02 04:24:45]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-12-02 08:36:44]

Thanks a lot for review! Although the trybots got all green now, I'll try to
figure out whether there has a real problem about the message ordering.

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

https://codereview.chromium.org/2534403002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:1090: void
ServiceWorkerVersion::OnSimpleEventResponse(
On 2016/12/02 04:00:33, shimazu wrote:
> How about deduping the codes by calling OnSimpleEventFinished in
> OnSimpleEventResponse?
> 
> and I think the names should be the same because there is no difference
between
> the two. I prefer OnSimpleEventFinished to OnSimpleEventResponse because it's
> clearer.

Acknowledged.

https://codereview.chromium.org/2534403002/diff/60001/content/common/service_...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/2534403002/diff/60001/content/common/service_...
content/common/service_worker/service_worker_messages.h:55:
IPC_STRUCT_TRAITS_BEGIN(content::ExtendableMessageEventSource)
On 2016/12/02 04:00:33, shimazu wrote:
> Could you remove this and write StructTraits in
> service_worker_types_traits.cc/h?

Yeah this work should be done sooner or later, but I see
|ServiceWorkerFetchRequest| is already  reusing the old IPC traits so I'd like
to focus on migrating IPC messages firstly, then create typemappings for them
all later. I suppose such way could avoid some additional work when we finally
decided where will these codes be decoupled out from //content :) Also, the
typemapping codes would be big and make the CL less-readable.

https://codereview.chromium.org/2534403002/diff/60001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2534403002/diff/60001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.cc:680: if
(!callback)
On 2016/12/02 04:00:33, shimazu wrote:
> DCHECK(callback) is better here because IIUC callback must not null here.

This is following didHandleSyncEvent(), |callback| would be null in case of no
instance corresponding with |request_id|.

https://codereview.chromium.org/2534403002/diff/60001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.h (right):

https://codereview.chromium.org/2534403002/diff/60001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.h:230: void
DispatchExtendableMessageEvent(
On 2016/12/02 04:00:33, shimazu wrote:
> Could you move this to l.222?
> I think it's better to sort them by alphabetical order.

Acknowledged.

https://codereview.chromium.org/2534403002/diff/60001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.h:235: void
OnExtendableMessageEvent(
On 2016/12/02 04:00:33, shimazu wrote:
> Could you remove this?

Acknowledged and thanks!

[leonhsl(Using Gerrit), 2016-12-02 09:21:53]

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

[commit-bot: I haz the power, 2016-12-02 09:22:17]

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, 2016-12-02 10:41:21]

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

[commit-bot: I haz the power, 2016-12-02 10:41:22]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-12-02 15:43:28]

Uploaded ps#4 to address comments, PTAnL, Thanks.

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

https://codereview.chromium.org/2534403002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:1090: void
ServiceWorkerVersion::OnSimpleEventResponse(
On 2016/12/02 08:36:44, leonhsl wrote:
> On 2016/12/02 04:00:33, shimazu wrote:
> > How about deduping the codes by calling OnSimpleEventFinished in
> > OnSimpleEventResponse?
> > 
> > and I think the names should be the same because there is no difference
> between
> > the two. I prefer OnSimpleEventFinished to OnSimpleEventResponse because
it's
> > clearer.
> 
> Acknowledged.

Put the duplicated codes into OnSimpleEventFinished and let
OnSimpleEventResponse call it. But for the naming change, if we renamed
OnSimpleEventResponse to OnSimpleEventFinished, at [1] base::Bind() can't deduce
which OnSimpleEventFinished should be bound.

[1]
https://cs.chromium.org/chromium/src/content/browser/service_worker/service_w...

https://codereview.chromium.org/2534403002/diff/60001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.h (right):

https://codereview.chromium.org/2534403002/diff/60001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.h:230: void
DispatchExtendableMessageEvent(
On 2016/12/02 08:36:44, leonhsl wrote:
> On 2016/12/02 04:00:33, shimazu wrote:
> > Could you move this to l.222?
> > I think it's better to sort them by alphabetical order.
> 
> Acknowledged.

Done.

https://codereview.chromium.org/2534403002/diff/60001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.h:235: void
OnExtendableMessageEvent(
On 2016/12/02 08:36:44, leonhsl wrote:
> On 2016/12/02 04:00:33, shimazu wrote:
> > Could you remove this?
> 
> Acknowledged and thanks!

Done.

[shimazu, 2016-12-05 03:40:20]

shimazu@chromium.org changed reviewers:
+ horo@chromium.org

[shimazu, 2016-12-05 03:40:21]

Thanks for your updates. 

+horo@-san: 
What do you think about the early return described in
service_worker_context_client.cc?

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

https://codereview.chromium.org/2534403002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:1090: void
ServiceWorkerVersion::OnSimpleEventResponse(
On 2016/12/02 15:43:27, leonhsl wrote:
> On 2016/12/02 08:36:44, leonhsl wrote:
> > On 2016/12/02 04:00:33, shimazu wrote:
> > > How about deduping the codes by calling OnSimpleEventFinished in
> > > OnSimpleEventResponse?
> > > 
> > > and I think the names should be the same because there is no difference
> > between
> > > the two. I prefer OnSimpleEventFinished to OnSimpleEventResponse because
> it's
> > > clearer.
> > 
> > Acknowledged.
> 
> Put the duplicated codes into OnSimpleEventFinished and let
> OnSimpleEventResponse call it. But for the naming change, if we renamed
> OnSimpleEventResponse to OnSimpleEventFinished, at [1] base::Bind() can't
deduce
> which OnSimpleEventFinished should be bound.
> 
> [1]
>
https://cs.chromium.org/chromium/src/content/browser/service_worker/service_w...

Thanks, I could understand why the names are different. How about adding a
comment about that (I put another comment to SWVersion) instead of renaming?

https://codereview.chromium.org/2534403002/diff/60001/content/browser/service...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/2534403002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_version.h:411: // through mojo,
|OnSimpleEventResponse| function could be removed.
Could you remove the vertical bars '|' from the method name?

https://codereview.chromium.org/2534403002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_version.h:626: void
OnSimpleEventResponse(int request_id,
How about adding a comment like
"Receiver function of responses of simple events dispatched through chromium
IPCs. This is internally the same with OnSimpleEventFinished and will be
replaced with OnSimpleEventFinished after all of simple events are dispatched
via mojo."?

https://codereview.chromium.org/2534403002/diff/60001/content/common/service_...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/2534403002/diff/60001/content/common/service_...
content/common/service_worker/service_worker_messages.h:55:
IPC_STRUCT_TRAITS_BEGIN(content::ExtendableMessageEventSource)
On 2016/12/02 08:36:44, leonhsl wrote:
> On 2016/12/02 04:00:33, shimazu wrote:
> > Could you remove this and write StructTraits in
> > service_worker_types_traits.cc/h?
> 
> Yeah this work should be done sooner or later, but I see
> |ServiceWorkerFetchRequest| is already  reusing the old IPC traits so I'd like
> to focus on migrating IPC messages firstly, then create typemappings for them
> all later. I suppose such way could avoid some additional work when we finally
> decided where will these codes be decoupled out from //content :) Also, the
> typemapping codes would be big and make the CL less-readable.

Sounds good! Let's move them on another CL after mojofication of event
dispatcher.

https://codereview.chromium.org/2534403002/diff/60001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2534403002/diff/60001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.cc:680: if
(!callback)
On 2016/12/02 08:36:44, leonhsl wrote:
> On 2016/12/02 04:00:33, shimazu wrote:
> > DCHECK(callback) is better here because IIUC callback must not null here.
> 
> This is following didHandleSyncEvent(), |callback| would be null in case of no
> instance corresponding with |request_id|.

Oops, I didn't realize that. 
Hmm.. I don't want to use early return here because in this case null-callback
situation is likely to be a bug. 

horo@-san: WDYT?

[horo, 2016-12-05 04:13:16]

https://codereview.chromium.org/2534403002/diff/60001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2534403002/diff/60001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.cc:680: if
(!callback)
On 2016/12/05 03:40:21, shimazu wrote:
> On 2016/12/02 08:36:44, leonhsl wrote:
> > On 2016/12/02 04:00:33, shimazu wrote:
> > > DCHECK(callback) is better here because IIUC callback must not null here.
> > 
> > This is following didHandleSyncEvent(), |callback| would be null in case of
no
> > instance corresponding with |request_id|.
> 
> Oops, I didn't realize that. 
> Hmm.. I don't want to use early return here because in this case null-callback
> situation is likely to be a bug. 
> 
> horo@-san: WDYT?

I prefer DCHECK(callback) too.

[leonhsl(Using Gerrit), 2016-12-06 03:22:04]

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

[commit-bot: I haz the power, 2016-12-06 03:22:19]

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, 2016-12-06 03:24:59]

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

[commit-bot: I haz the power, 2016-12-06 03:25:00]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[leonhsl(Using Gerrit), 2016-12-06 03:45:59]

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

[commit-bot: I haz the power, 2016-12-06 03:46:14]

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, 2016-12-06 05:00:36]

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

[commit-bot: I haz the power, 2016-12-06 05:00:37]

Dry run: 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), 2016-12-06 06:09:57]

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

[commit-bot: I haz the power, 2016-12-06 06:10:10]

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, 2016-12-06 07:32:29]

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

[commit-bot: I haz the power, 2016-12-06 07:32:30]

Dry run: 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), 2016-12-06 07:37:57]

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

[commit-bot: I haz the power, 2016-12-06 07:38:11]

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, 2016-12-06 08:50:13]

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

[commit-bot: I haz the power, 2016-12-06 08:50:14]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-12-06 08:54:39]

Uploaded ps#5 and ps#6 to address comments, PTAnL, Thanks.

As for the failed test case BackgroundSyncBrowserTest.RegisterFromServiceWorker,
from the error log we can see that [2] is getting executed before [1] actually,
but our test case expects [1] firstly then [2], so caused failure.
After investigation I think this problem is not caused by this CL and we can
explain it reasonably. The root cause is that blink.mojom.BackgroundSyncService
and content.mojom.ServiceWorkerEventDispatcher bind/run on their own message
pipe independently, so have no any message order insurance between them, while
[1] is supposed to be triggered by the response of mojo function
BackgroundSyncService.Register() and [2] is triggered by mojo function
ServiceWorkerEventDispatcher.DispatchSyncEvent(), so, sometimes [2] could happen
before [1].
This means sometimes sync fired event could possibly be received while waiting
for response of sync.register. Maybe we could revise the test codes? WDYT?
Thanks.
[1]
https://cs.chromium.org/chromium/src/content/test/data/background_sync/servic...
[2]
https://cs.chromium.org/chromium/src/content/test/data/background_sync/servic...

https://codereview.chromium.org/2534403002/diff/60001/content/browser/service...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/2534403002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_version.h:411: // through mojo,
|OnSimpleEventResponse| function could be removed.
On 2016/12/05 03:40:21, shimazu wrote:
> Could you remove the vertical bars '|' from the method name?

Done.

https://codereview.chromium.org/2534403002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_version.h:626: void
OnSimpleEventResponse(int request_id,
On 2016/12/05 03:40:21, shimazu wrote:
> How about adding a comment like
> "Receiver function of responses of simple events dispatched through chromium
> IPCs. This is internally the same with OnSimpleEventFinished and will be
> replaced with OnSimpleEventFinished after all of simple events are dispatched
> via mojo."?

Done and thanks!

https://codereview.chromium.org/2534403002/diff/60001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2534403002/diff/60001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.cc:680: if
(!callback)
On 2016/12/05 04:13:16, horo wrote:
> On 2016/12/05 03:40:21, shimazu wrote:
> > On 2016/12/02 08:36:44, leonhsl wrote:
> > > On 2016/12/02 04:00:33, shimazu wrote:
> > > > DCHECK(callback) is better here because IIUC callback must not null
here.
> > > 
> > > This is following didHandleSyncEvent(), |callback| would be null in case
of
> no
> > > instance corresponding with |request_id|.
> > 
> > Oops, I didn't realize that. 
> > Hmm.. I don't want to use early return here because in this case
null-callback
> > situation is likely to be a bug. 
> > 
> > horo@-san: WDYT?
> 
> I prefer DCHECK(callback) too.

Done.

[shimazu, 2016-12-07 09:59:03]

non-owner lgtm.

horo-san: could you take a look?


On 2016/12/06 08:54:39, leonhsl wrote:
> Uploaded ps#5 and ps#6 to address comments, PTAnL, Thanks.
> 
> As for the failed test case
BackgroundSyncBrowserTest.RegisterFromServiceWorker,
> from the error log we can see that [2] is getting executed before [1]
actually,
> but our test case expects [1] firstly then [2], so caused failure.
> After investigation I think this problem is not caused by this CL and we can
> explain it reasonably. The root cause is that
blink.mojom.BackgroundSyncService
> and content.mojom.ServiceWorkerEventDispatcher bind/run on their own message
> pipe independently, so have no any message order insurance between them, while
> [1] is supposed to be triggered by the response of mojo function
> BackgroundSyncService.Register() and [2] is triggered by mojo function
> ServiceWorkerEventDispatcher.DispatchSyncEvent(), so, sometimes [2] could
happen
> before [1].
> This means sometimes sync fired event could possibly be received while waiting
> for response of sync.register. Maybe we could revise the test codes? WDYT?
> Thanks.
> [1]
>
https://cs.chromium.org/chromium/src/content/test/data/background_sync/servic...
> [2]
>
https://cs.chromium.org/chromium/src/content/test/data/background_sync/servic...
> 

Thanks for your investigation!
Your scenario sounds correct. There is no guarantee of the ordering between
DispatchSyncEvent and RegisterCallback(the response of Register), and
additionally DispatchSyncEvent is called at [1]. That's before calling the
callback at [2]. 

[1]
https://cs.chromium.org/chromium/src/content/browser/background_sync/backgrou...
[2]
https://cs.chromium.org/chromium/src/content/browser/background_sync/backgrou...


I created another issue: https://crbug.com/671980 

>
https://codereview.chromium.org/2534403002/diff/60001/content/browser/service...
> File content/browser/service_worker/service_worker_version.h (right):
> 
>
https://codereview.chromium.org/2534403002/diff/60001/content/browser/service...
> content/browser/service_worker/service_worker_version.h:411: // through mojo,
> |OnSimpleEventResponse| function could be removed.
> On 2016/12/05 03:40:21, shimazu wrote:
> > Could you remove the vertical bars '|' from the method name?
> 
> Done.
> 
>
https://codereview.chromium.org/2534403002/diff/60001/content/browser/service...
> content/browser/service_worker/service_worker_version.h:626: void
> OnSimpleEventResponse(int request_id,
> On 2016/12/05 03:40:21, shimazu wrote:
> > How about adding a comment like
> > "Receiver function of responses of simple events dispatched through chromium
> > IPCs. This is internally the same with OnSimpleEventFinished and will be
> > replaced with OnSimpleEventFinished after all of simple events are
dispatched
> > via mojo."?
> 
> Done and thanks!
> 
>
https://codereview.chromium.org/2534403002/diff/60001/content/renderer/servic...
> File content/renderer/service_worker/service_worker_context_client.cc (right):
> 
>
https://codereview.chromium.org/2534403002/diff/60001/content/renderer/servic...
> content/renderer/service_worker/service_worker_context_client.cc:680: if
> (!callback)
> On 2016/12/05 04:13:16, horo wrote:
> > On 2016/12/05 03:40:21, shimazu wrote:
> > > On 2016/12/02 08:36:44, leonhsl wrote:
> > > > On 2016/12/02 04:00:33, shimazu wrote:
> > > > > DCHECK(callback) is better here because IIUC callback must not null
> here.
> > > > 
> > > > This is following didHandleSyncEvent(), |callback| would be null in case
> of
> > no
> > > > instance corresponding with |request_id|.
> > > 
> > > Oops, I didn't realize that. 
> > > Hmm.. I don't want to use early return here because in this case
> null-callback
> > > situation is likely to be a bug. 
> > > 
> > > horo@-san: WDYT?
> > 
> > I prefer DCHECK(callback) too.
> 
> Done.

[horo, 2016-12-08 02:16:53]

lgtm

[leonhsl(Using Gerrit), 2016-12-08 07:43:38]

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

[leonhsl(Using Gerrit), 2016-12-08 07:43:39]

Hi, Tom, would you PTAL for security review? Thanks.

[Tom Sepez, 2016-12-08 17:39:30]

On 2016/12/08 07:43:39, leonhsl wrote:
> Hi, Tom, would you PTAL for security review? Thanks.

messages/mojom LGTM

[leonhsl(Using Gerrit), 2016-12-08 23:54:07]

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

[commit-bot: I haz the power, 2016-12-08 23:54:37]

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, 2016-12-09 00:02:30]

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

[commit-bot: I haz the power, 2016-12-09 00:02:32]

Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[leonhsl(Using Gerrit), 2016-12-09 00:13:19]

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

[commit-bot: I haz the power, 2016-12-09 00:13:49]

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, 2016-12-09 00:20:01]

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

[commit-bot: I haz the power, 2016-12-09 00:20:03]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[leonhsl(Using Gerrit), 2016-12-09 00:50:18]

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

[leonhsl(Using Gerrit), 2016-12-09 00:50:19]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
shimazu@chromium.org, horo@chromium.org
Link to the patchset: https://codereview.chromium.org/2534403002/#ps140001
(title: "Rebase only")

[commit-bot: I haz the power, 2016-12-09 00:50:44]

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, 2016-12-09 04:17:19]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1481244618715510,
"parent_rev": "3d9abc8885c1c4f574ec08f22f144e458d012903", "commit_rev":
"a309cba76aebd453dc153c04b2deed67dfba5f74"}

[commit-bot: I haz the power, 2016-12-09 04:18:08]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-12-09 04:22:59]

Description was changed from

==========
[ServiceWorker] Mojofy extendable message event.

This CL converts extendable message event IPCs into mojo interface:
  ServiceWorkerMsg_ExtendableMessageEvent
  ServiceWorkerHostMsg_ExtendableMessageEventFinished

BUG=629701
TEST=content_unittests
==========

to

==========
[ServiceWorker] Mojofy extendable message event.

This CL converts extendable message event IPCs into mojo interface:
  ServiceWorkerMsg_ExtendableMessageEvent
  ServiceWorkerHostMsg_ExtendableMessageEventFinished

BUG=629701
TEST=content_unittests

Committed: https://crrev.com/b0aeab4ccafc6cb37e3f16a2dd4f214f9b56dda3
Cr-Commit-Position: refs/heads/master@{#437462}
==========

[commit-bot: I haz the power, 2016-12-09 04:23:01]

Patchset 7 (id:??) landed as
https://crrev.com/b0aeab4ccafc6cb37e3f16a2dd4f214f9b56dda3
Cr-Commit-Position: refs/heads/master@{#437462}