Add two interfaces for ServiceWorkerProviderContext/ProviderHost

Add two interfaces for ServiceWorkerProviderContext/ProviderHost

This patch introduces two mojom types: ServiceWorkerProviderHost and
ServiceWorkerProvider. They are currently used for managing the lifetime of
ServiceWorkerProviderHost from the renderer, but these interfaces will have
methods managing registrations, registration associated by the document and
controller.
These interfaces are now associated with SWDispatcherHost which is already bound
with the channel. After crrev.com/2779763004, interfaces for worker's context
will be associated with EWInstanceClient. This will solve the ordering issue
between messages of Stopped and ProviderDestroyed.

BUG=629701, 676983, 668633
Review-Url: https://codereview.chromium.org/2653493009
Cr-Commit-Position: refs/heads/master@{#475809}
Committed: https://chromium.googlesource.com/chromium/src/+/4223216d2fa2a111c5e0636017bb6766682adda9

[shimazu, 2017-02-13 10:24:59]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 10:25:18]

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-02-13 11:17:15]

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

[commit-bot: I haz the power, 2017-02-13 11:17:16]

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

[shimazu, 2017-03-27 07:49:45]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 07:50:00]

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-27 08:49:49]

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

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

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_...)

[shimazu, 2017-03-27 09:36:37]

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

[shimazu, 2017-03-27 09:36:38]

Could you take a look?

// I need updating this patch since several tests are failing, though.

[falken, 2017-03-28 06:29:43]

Overall this seems to make sense but it'd be easier to understand with more high
level descriptions in the CL description and/or comments. Can you explain more
what this is trying to solve and the design decisions taken here? "This will
solve the ordering issues" is pretty vague.

https://codereview.chromium.org/2653493009/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/2653493009/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:62:
ResourceContext* /* resource_context */) override {
You can just omit the names when they don't add new info.

https://codereview.chromium.org/2653493009/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:76:
TRACE_EVENT0("ServiceWorker", "ServiceWorkerProviderHost::ProviderDestroyed");
Trace event is using the old name.

https://codereview.chromium.org/2653493009/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:165: // Mojo pile
is possible to be null when testing.
Can we instead fix the tests?

https://codereview.chromium.org/2653493009/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/2653493009/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:328:
mojom::ServiceWorkerProviderHostAssociatedRequest request,
Can ServiceWorkerProviderHost just take a ServiceWorkerProviderHostInfo as an
argument, and use ServiceWorkerProviderHostInfo as a member? This would help
documentation since all these variables are defined in SWProviderHostInfo.

https://codereview.chromium.org/2653493009/diff/120001/content/common/service...
File content/common/service_worker/service_worker.mojom (right):

https://codereview.chromium.org/2653493009/diff/120001/content/common/service...
content/common/service_worker/service_worker.mojom:13:
OnProviderCreated(ServiceWorkerProviderHostInfo provider_info);
Should we document what balances out OnProviderCreated, now that
OnProviderDestroyed is gone?

https://codereview.chromium.org/2653493009/diff/120001/content/common/service...
File content/common/service_worker/service_worker_provider_host_info.h (right):

https://codereview.chromium.org/2653493009/diff/120001/content/common/service...
content/common/service_worker/service_worker_provider_host_info.h:49:
mojom::ServiceWorkerProviderAssociatedPtrInfo client_ptr_info;
Can you add documentation? I know this is WIP but comments also would aid
review.

https://codereview.chromium.org/2653493009/diff/120001/content/common/service...
File content/common/service_worker/service_worker_provider_interfaces.mojom
(right):

https://codereview.chromium.org/2653493009/diff/120001/content/common/service...
content/common/service_worker/service_worker_provider_interfaces.mojom:7: //
Browser-side interface bound to ServiceWorkerProviderHost.
This might read better as "mojom::ServiceWorkerProviderHost is a browser-side
interface bound to content::ServiceWorkerProviderHost", since they are both
called SWProviderHost

Also may be easier to understand if it additionally says something like "The
renderer uses this interface to request the browser to do operations involving
service worker registrations."

https://codereview.chromium.org/2653493009/diff/120001/content/common/service...
content/common/service_worker/service_worker_provider_interfaces.mojom:8: //
ServiceWorkerProviderHost bouhd with this interface will be destroyed when
"A content::ServiceWorkerProviderHost bound with this interface..."

https://codereview.chromium.org/2653493009/diff/120001/content/common/service...
content/common/service_worker/service_worker_provider_interfaces.mojom:18: //
Renderer-side interface bound to ServiceWorkerProviderContext.
similar as above

[shimazu, 2017-04-24 08:38:24]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-24 08:38: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, 2017-04-24 11:38:49]

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

[commit-bot: I haz the power, 2017-04-24 11:38:50]

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_...)

[shimazu, 2017-05-02 04:48:40]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 04:48:53]

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-05-02 04:51:18]

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

[commit-bot: I haz the power, 2017-05-02 04:51:19]

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_...)

[shimazu, 2017-05-02 05:00:48]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 05:01:05]

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-05-02 06:46:51]

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

[commit-bot: I haz the power, 2017-05-02 06:46:52]

Dry run: This issue passed the CQ dry run.

[shimazu, 2017-05-08 02:24:48]

Description was changed from

==========
[WIP] Add two interfaces for ServiceWorkerProviderContext/ProviderHost

The associated interfaces will be bound with a channel owned by
EWInstanceClient on later patch. This will solve the ordering issues.

BUG=629701,676983,668633
==========

to

==========
Add two interfaces for ServiceWorkerProviderContext/ProviderHost

This patch introduces two mojom types: ServiceWorkerProviderHost and
ServiceWorkerProvider. They are currently used for managing the lifetime of
ServiceWorkerProviderHost from the renderer, but these interfaces will have
methods managing registrations, registration associated by the document and
controller.

BUG=629701,676983,668633
==========

[shimazu, 2017-05-08 02:29:52]

Description was changed from

==========
Add two interfaces for ServiceWorkerProviderContext/ProviderHost

This patch introduces two mojom types: ServiceWorkerProviderHost and
ServiceWorkerProvider. They are currently used for managing the lifetime of
ServiceWorkerProviderHost from the renderer, but these interfaces will have
methods managing registrations, registration associated by the document and
controller.

BUG=629701,676983,668633
==========

to

==========
Add two interfaces for ServiceWorkerProviderContext/ProviderHost

This patch introduces two mojom types: ServiceWorkerProviderHost and
ServiceWorkerProvider. They are currently used for managing the lifetime of
ServiceWorkerProviderHost from the renderer, but these interfaces will have
methods managing registrations, registration associated by the document and
controller.
These interfaces are now associated with SWDispatcherHost which is already bound
with the channel. After crrev.com/2779763004, interfaces for worker's context
will be associated with EWInstanceClient. This will solve the ordering issue
between messages of Stopped and ProviderDestroyed.

BUG=629701,676983,668633
==========

[shimazu, 2017-05-08 08:23:39]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-08 08:24:31]

Dry run: CQ is trying da patch.

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

[shimazu, 2017-05-08 08:34:29]

Thanks for your comments.
Updated this patch. PTAnL.

https://codereview.chromium.org/2653493009/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/2653493009/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:62:
ResourceContext* /* resource_context */) override {
On 2017/03/28 06:29:43, falken (away until May 8) wrote:
> You can just omit the names when they don't add new info.

Done.

https://codereview.chromium.org/2653493009/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:76:
TRACE_EVENT0("ServiceWorker", "ServiceWorkerProviderHost::ProviderDestroyed");
On 2017/03/28 06:29:42, falken (away until May 8) wrote:
> Trace event is using the old name.

Done.

https://codereview.chromium.org/2653493009/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:165: // Mojo pile
is possible to be null when testing.
On 2017/03/28 06:29:43, falken wrote:
> Can we instead fix the tests?

Yes, I agree with avoiding testing code in production. 
Tried to fix this.

https://codereview.chromium.org/2653493009/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/2653493009/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:328:
mojom::ServiceWorkerProviderHostAssociatedRequest request,
On 2017/03/28 06:29:43, falken wrote:
> Can ServiceWorkerProviderHost just take a ServiceWorkerProviderHostInfo as an
> argument, and use ServiceWorkerProviderHostInfo as a member? This would help
> documentation since all these variables are defined in SWProviderHostInfo.

Done.

https://codereview.chromium.org/2653493009/diff/120001/content/common/service...
File content/common/service_worker/service_worker.mojom (right):

https://codereview.chromium.org/2653493009/diff/120001/content/common/service...
content/common/service_worker/service_worker.mojom:13:
OnProviderCreated(ServiceWorkerProviderHostInfo provider_info);
On 2017/03/28 06:29:43, falken wrote:
> Should we document what balances out OnProviderCreated, now that
> OnProviderDestroyed is gone?

Done.

https://codereview.chromium.org/2653493009/diff/120001/content/common/service...
File content/common/service_worker/service_worker_provider_host_info.h (right):

https://codereview.chromium.org/2653493009/diff/120001/content/common/service...
content/common/service_worker/service_worker_provider_host_info.h:49:
mojom::ServiceWorkerProviderAssociatedPtrInfo client_ptr_info;
On 2017/03/28 06:29:43, falken wrote:
> Can you add documentation? I know this is WIP but comments also would aid
> review.

Done.

https://codereview.chromium.org/2653493009/diff/120001/content/common/service...
File content/common/service_worker/service_worker_provider_interfaces.mojom
(right):

https://codereview.chromium.org/2653493009/diff/120001/content/common/service...
content/common/service_worker/service_worker_provider_interfaces.mojom:7: //
Browser-side interface bound to ServiceWorkerProviderHost.
On 2017/03/28 06:29:43, falken wrote:
> This might read better as "mojom::ServiceWorkerProviderHost is a browser-side
> interface bound to content::ServiceWorkerProviderHost", since they are both
> called SWProviderHost
> 
> Also may be easier to understand if it additionally says something like "The
> renderer uses this interface to request the browser to do operations involving
> service worker registrations."

Done.

https://codereview.chromium.org/2653493009/diff/120001/content/common/service...
content/common/service_worker/service_worker_provider_interfaces.mojom:8: //
ServiceWorkerProviderHost bouhd with this interface will be destroyed when
On 2017/03/28 06:29:43, falken wrote:
> "A content::ServiceWorkerProviderHost bound with this interface..."

Done.

https://codereview.chromium.org/2653493009/diff/120001/content/common/service...
content/common/service_worker/service_worker_provider_interfaces.mojom:18: //
Renderer-side interface bound to ServiceWorkerProviderContext.
On 2017/03/28 06:29:43, falken wrote:
> similar as above

Done.

[commit-bot: I haz the power, 2017-05-08 09:03:21]

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

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

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...)

[shimazu, 2017-05-09 01:14:15]

On 2017/05/08 08:34:29, shimazu wrote:
> Thanks for your comments.
> Updated this patch. PTAnL.
> 
>
https://codereview.chromium.org/2653493009/diff/120001/content/browser/servic...
> File content/browser/service_worker/service_worker_provider_host.cc (right):
> 
>
https://codereview.chromium.org/2653493009/diff/120001/content/browser/servic...
> content/browser/service_worker/service_worker_provider_host.cc:62:
> ResourceContext* /* resource_context */) override {
> On 2017/03/28 06:29:43, falken (away until May 8) wrote:
> > You can just omit the names when they don't add new info.
> 
> Done.
> 
>
https://codereview.chromium.org/2653493009/diff/120001/content/browser/servic...
> content/browser/service_worker/service_worker_provider_host.cc:76:
> TRACE_EVENT0("ServiceWorker", "ServiceWorkerProviderHost::ProviderDestroyed");
> On 2017/03/28 06:29:42, falken (away until May 8) wrote:
> > Trace event is using the old name.
> 
> Done.
> 
>
https://codereview.chromium.org/2653493009/diff/120001/content/browser/servic...
> content/browser/service_worker/service_worker_provider_host.cc:165: // Mojo
pile
> is possible to be null when testing.
> On 2017/03/28 06:29:43, falken wrote:
> > Can we instead fix the tests?
> 
> Yes, I agree with avoiding testing code in production. 
> Tried to fix this.
> 
>
https://codereview.chromium.org/2653493009/diff/120001/content/browser/servic...
> File content/browser/service_worker/service_worker_provider_host.h (right):
> 
>
https://codereview.chromium.org/2653493009/diff/120001/content/browser/servic...
> content/browser/service_worker/service_worker_provider_host.h:328:
> mojom::ServiceWorkerProviderHostAssociatedRequest request,
> On 2017/03/28 06:29:43, falken wrote:
> > Can ServiceWorkerProviderHost just take a ServiceWorkerProviderHostInfo as
an
> > argument, and use ServiceWorkerProviderHostInfo as a member? This would help
> > documentation since all these variables are defined in SWProviderHostInfo.
> 
> Done.
> 
>
https://codereview.chromium.org/2653493009/diff/120001/content/common/service...
> File content/common/service_worker/service_worker.mojom (right):
> 
>
https://codereview.chromium.org/2653493009/diff/120001/content/common/service...
> content/common/service_worker/service_worker.mojom:13:
> OnProviderCreated(ServiceWorkerProviderHostInfo provider_info);
> On 2017/03/28 06:29:43, falken wrote:
> > Should we document what balances out OnProviderCreated, now that
> > OnProviderDestroyed is gone?
> 
> Done.
> 
>
https://codereview.chromium.org/2653493009/diff/120001/content/common/service...
> File content/common/service_worker/service_worker_provider_host_info.h
(right):
> 
>
https://codereview.chromium.org/2653493009/diff/120001/content/common/service...
> content/common/service_worker/service_worker_provider_host_info.h:49:
> mojom::ServiceWorkerProviderAssociatedPtrInfo client_ptr_info;
> On 2017/03/28 06:29:43, falken wrote:
> > Can you add documentation? I know this is WIP but comments also would aid
> > review.
> 
> Done.
> 
>
https://codereview.chromium.org/2653493009/diff/120001/content/common/service...
> File content/common/service_worker/service_worker_provider_interfaces.mojom
> (right):
> 
>
https://codereview.chromium.org/2653493009/diff/120001/content/common/service...
> content/common/service_worker/service_worker_provider_interfaces.mojom:7: //
> Browser-side interface bound to ServiceWorkerProviderHost.
> On 2017/03/28 06:29:43, falken wrote:
> > This might read better as "mojom::ServiceWorkerProviderHost is a
browser-side
> > interface bound to content::ServiceWorkerProviderHost", since they are both
> > called SWProviderHost
> > 
> > Also may be easier to understand if it additionally says something like "The
> > renderer uses this interface to request the browser to do operations
involving
> > service worker registrations."
> 
> Done.
> 
>
https://codereview.chromium.org/2653493009/diff/120001/content/common/service...
> content/common/service_worker/service_worker_provider_interfaces.mojom:8: //
> ServiceWorkerProviderHost bouhd with this interface will be destroyed when
> On 2017/03/28 06:29:43, falken wrote:
> > "A content::ServiceWorkerProviderHost bound with this interface..."
> 
> Done.
> 
>
https://codereview.chromium.org/2653493009/diff/120001/content/common/service...
> content/common/service_worker/service_worker_provider_interfaces.mojom:18: //
> Renderer-side interface bound to ServiceWorkerProviderContext.
> On 2017/03/28 06:29:43, falken wrote:
> > similar as above
> 
> Done.

Oops, several tests related to PlzNavigate looks failing. I started
investigating it now. Please wait for a while, sorry!

[shimazu, 2017-05-09 02:26:42]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-09 02:29:09]

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-05-09 04:35:57]

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

[commit-bot: I haz the power, 2017-05-09 04:35:59]

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_...)

[shimazu, 2017-05-16 09:26:08]

Patchset #13 (id:240001) has been deleted

[shimazu, 2017-05-16 09:37:15]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-16 09:37: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-05-16 10:32:32]

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

[commit-bot: I haz the power, 2017-05-16 10:32:33]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[shimazu, 2017-05-17 00:43:05]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 00:44:33]

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-05-17 02:52:27]

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

[commit-bot: I haz the power, 2017-05-17 02:52:29]

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_...)

[shimazu, 2017-05-17 05:55:20]

Sorry for very long delay. I've fixed this patch and also another patch for
browser-side creation. 
PTAnL.

The failing test doesn't seem related to my patch because it passes on my local,
but I'm now trying to rebase and run the tests again to check if they are
reproducible.

[shimazu, 2017-05-17 07:50:31]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 07:51:00]

Dry run: CQ is trying da patch.

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

[shimazu, 2017-05-17 07:53:11]

A couple of tests failed when BrowserSideNavigation is enabled. Fixed on ps#14.

[commit-bot: I haz the power, 2017-05-17 11:51:46]

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

[commit-bot: I haz the power, 2017-05-17 11:51:48]

Dry run: This issue passed the CQ dry run.

[falken, 2017-05-18 04:13:10]

This looks pretty good but I haven't been able to read it all.

Do you see any way to split this up into multiple CLs? For example, the
ServiceWorkerProviderHost refactoring to use ServiceWorkerProviderHostInfo looks
like it could be a different CL.

https://codereview.chromium.org/2653493009/diff/280001/content/browser/servic...
File content/browser/service_worker/service_worker_provider_host.h (left):

https://codereview.chromium.org/2653493009/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:397: const bool
is_parent_frame_secure_;
Can you put the refactoring to ServiceWorkerProviderHostInfo in another CL?

https://codereview.chromium.org/2653493009/diff/280001/content/browser/servic...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/2653493009/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:409:
mojom::ServiceWorkerProviderAssociatedPtr client_;
"client" is a really overloaded word in service worker land. This could use a
comment and a clearer name.

How about?

// The renderer-side Mojo endpoint for this provider.
mojom::ServiceWorkerProviderAssociatedPtr provider_;

https://codereview.chromium.org/2653493009/diff/280001/content/browser/servic...
File content/browser/service_worker/service_worker_test_utils.h (right):

https://codereview.chromium.org/2653493009/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_test_utils.h:49: 
please add a class level comment

https://codereview.chromium.org/2653493009/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_test_utils.h:50: class
ServiceWorkerRemoteProviderEndpoint {
seems this should be a struct?

https://codereview.chromium.org/2653493009/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_test_utils.h:58:
mojom::ServiceWorkerProviderAssociatedRequest client_request;
Can you comment what these two members do?

https://codereview.chromium.org/2653493009/diff/280001/content/child/service_...
File content/child/service_worker/service_worker_provider_context.h (right):

https://codereview.chromium.org/2653493009/diff/280001/content/child/service_...
content/child/service_worker/service_worker_provider_context.h:51: public:
Can you document what |request| is?

https://codereview.chromium.org/2653493009/diff/280001/content/common/service...
File content/common/service_worker/service_worker.mojom (right):

https://codereview.chromium.org/2653493009/diff/280001/content/common/service...
content/common/service_worker/service_worker.mojom:9: // Per-process
browser-side interface bound to ServiceWorkerDispatcherHost.
content::ServiceWorkerDispatcherHost

https://codereview.chromium.org/2653493009/diff/280001/content/common/service...
content/common/service_worker/service_worker.mojom:10: // Each InterfacePtrs on
the same render process will be bound to the same
All InterfacePtrs on the same renderer process are bound to the same
mojom::ServiceWorkerDispatcherHost (or content::ServiceWorkerDispatcherHost??)

https://codereview.chromium.org/2653493009/diff/280001/content/common/service...
content/common/service_worker/service_worker.mojom:11: //
ServiceWorkerDispatcherHost.
Add sentence like: The renderer uses this interface to tell the browser when
potential service worker clients are created and when service workers are
starting up.

https://codereview.chromium.org/2653493009/diff/280001/content/common/service...
content/common/service_worker/service_worker.mojom:14: // have mojo endpoints to
connect the provider host and the provider on the
has Mojo endpoints

https://codereview.chromium.org/2653493009/diff/280001/content/common/service...
content/common/service_worker/service_worker.mojom:16: // managed with the
interface.
"managed with the interface" -> "tied to this mojom::ServiceWorkerDispatcherHost
interface?"

https://codereview.chromium.org/2653493009/diff/280001/content/common/service...
File content/common/service_worker/service_worker_provider.mojom (right):

https://codereview.chromium.org/2653493009/diff/280001/content/common/service...
content/common/service_worker/service_worker_provider.mojom:11: // This contains
the params for the constructor of ServiceWorkerProviderHost.
content::ServiceWorkerProviderHost

https://codereview.chromium.org/2653493009/diff/280001/content/common/service...
content/common/service_worker/service_worker_provider.mojom:20: associated
ServiceWorkerProvider client_ptr_info;
Does it make more sense to just name these |provider_host| and |provider|? I
probably just don't understand Mojo enough but why the nouns "request" and
"client pointer info"?

https://codereview.chromium.org/2653493009/diff/280001/content/common/service...
File content/common/service_worker/service_worker_provider_host_info.h (right):

https://codereview.chromium.org/2653493009/diff/280001/content/common/service...
content/common/service_worker/service_worker_provider_host_info.h:55: // be
valid when ServiceWorkerProviderHostInfo is passed to any mojo methods.
Mojo

https://codereview.chromium.org/2653493009/diff/280001/content/common/service...
content/common/service_worker/service_worker_provider_host_info.h:61: // should
be valid when ServiceWorkerProviderHostInfo is passed to any mojo
Mojo

[shimazu, 2017-05-19 08:31:07]

Thanks for your comments.
Uploaded ps#18. PTAnL.

https://codereview.chromium.org/2653493009/diff/280001/content/browser/servic...
File content/browser/service_worker/service_worker_provider_host.h (left):

https://codereview.chromium.org/2653493009/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:397: const bool
is_parent_frame_secure_;
On 2017/05/18 04:13:09, falken wrote:
> Can you put the refactoring to ServiceWorkerProviderHostInfo in another CL?

Done.

https://codereview.chromium.org/2653493009/diff/280001/content/browser/servic...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/2653493009/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:409:
mojom::ServiceWorkerProviderAssociatedPtr client_;
On 2017/05/18 04:13:09, falken wrote:
> "client" is a really overloaded word in service worker land. This could use a
> comment and a clearer name.
> 
> How about?
> 
> // The renderer-side Mojo endpoint for this provider.
> mojom::ServiceWorkerProviderAssociatedPtr provider_;

Sounds good! Done.

https://codereview.chromium.org/2653493009/diff/280001/content/browser/servic...
File content/browser/service_worker/service_worker_test_utils.h (right):

https://codereview.chromium.org/2653493009/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_test_utils.h:49: 
On 2017/05/18 04:13:09, falken wrote:
> please add a class level comment

Done.

https://codereview.chromium.org/2653493009/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_test_utils.h:50: class
ServiceWorkerRemoteProviderEndpoint {
On 2017/05/18 04:13:09, falken wrote:
> seems this should be a struct?

Done.

https://codereview.chromium.org/2653493009/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_test_utils.h:58:
mojom::ServiceWorkerProviderAssociatedRequest client_request;
On 2017/05/18 04:13:09, falken wrote:
> Can you comment what these two members do?

Done.

https://codereview.chromium.org/2653493009/diff/280001/content/child/service_...
File content/child/service_worker/service_worker_provider_context.h (right):

https://codereview.chromium.org/2653493009/diff/280001/content/child/service_...
content/child/service_worker/service_worker_provider_context.h:51: public:
On 2017/05/18 04:13:09, falken wrote:
> Can you document what |request| is?

Done.

https://codereview.chromium.org/2653493009/diff/280001/content/common/service...
File content/common/service_worker/service_worker.mojom (right):

https://codereview.chromium.org/2653493009/diff/280001/content/common/service...
content/common/service_worker/service_worker.mojom:9: // Per-process
browser-side interface bound to ServiceWorkerDispatcherHost.
On 2017/05/18 04:13:09, falken wrote:
> content::ServiceWorkerDispatcherHost

Done.

https://codereview.chromium.org/2653493009/diff/280001/content/common/service...
content/common/service_worker/service_worker.mojom:10: // Each InterfacePtrs on
the same render process will be bound to the same
On 2017/05/18 04:13:10, falken wrote:
> All InterfacePtrs on the same renderer process are bound to the same
> mojom::ServiceWorkerDispatcherHost (or content::ServiceWorkerDispatcherHost??)

Done.

https://codereview.chromium.org/2653493009/diff/280001/content/common/service...
content/common/service_worker/service_worker.mojom:11: //
ServiceWorkerDispatcherHost.
On 2017/05/18 04:13:09, falken wrote:
> Add sentence like: The renderer uses this interface to tell the browser when
> potential service worker clients are created and when service workers are
> starting up.

Done.

https://codereview.chromium.org/2653493009/diff/280001/content/common/service...
content/common/service_worker/service_worker.mojom:14: // have mojo endpoints to
connect the provider host and the provider on the
On 2017/05/18 04:13:10, falken wrote:
> has Mojo endpoints

Done.

https://codereview.chromium.org/2653493009/diff/280001/content/common/service...
content/common/service_worker/service_worker.mojom:16: // managed with the
interface.
On 2017/05/18 04:13:10, falken wrote:
> "managed with the interface" -> "tied to this
mojom::ServiceWorkerDispatcherHost
> interface?"

Done.

https://codereview.chromium.org/2653493009/diff/280001/content/common/service...
File content/common/service_worker/service_worker_provider.mojom (right):

https://codereview.chromium.org/2653493009/diff/280001/content/common/service...
content/common/service_worker/service_worker_provider.mojom:11: // This contains
the params for the constructor of ServiceWorkerProviderHost.
On 2017/05/18 04:13:10, falken wrote:
> content::ServiceWorkerProviderHost

Done.

https://codereview.chromium.org/2653493009/diff/280001/content/common/service...
content/common/service_worker/service_worker_provider.mojom:20: associated
ServiceWorkerProvider client_ptr_info;
On 2017/05/18 04:13:10, falken wrote:
> Does it make more sense to just name these |provider_host| and |provider|? I
> probably just don't understand Mojo enough but why the nouns "request" and
> "client pointer info"?

I thought _request/_ptr_info are informative because there are several types
related to an interface. For example, these four types are used for
ServiceWorkerProviderHost interface:

- ServiceWorkerProviderHost
- AssociatedRequest<ServiceWorkerProviderHost>
- AssociatedBinding<ServiceWorkerProviderHost>
- AssociatedInterfacePtrInfo<ServiceWorkerProviderHost>
- AssociatedInterfacePtr<ServiceWorkerProviderHost>

Request and InterfacePtrInfo are the information to carry the endpoints over
mojo connection, and Binding and InterfacePtr are receptacles of these
endpoints.

What do you think we have _request/_ptr_info for Request and PtrInfo, and no
suffix for Binding and InterfacePtr?

https://codereview.chromium.org/2653493009/diff/280001/content/common/service...
File content/common/service_worker/service_worker_provider_host_info.h (right):

https://codereview.chromium.org/2653493009/diff/280001/content/common/service...
content/common/service_worker/service_worker_provider_host_info.h:55: // be
valid when ServiceWorkerProviderHostInfo is passed to any mojo methods.
On 2017/05/18 04:13:10, falken wrote:
> Mojo

Done.

https://codereview.chromium.org/2653493009/diff/280001/content/common/service...
content/common/service_worker/service_worker_provider_host_info.h:61: // should
be valid when ServiceWorkerProviderHostInfo is passed to any mojo
On 2017/05/18 04:13:10, falken wrote:
> Mojo

Done.

[shimazu, 2017-05-20 23:56:48]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-20 23:56:56]

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-05-21 01:31:40]

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

[commit-bot: I haz the power, 2017-05-21 01:31:41]

Dry run: This issue passed the CQ dry run.

[falken, 2017-05-22 08:28:13]

OK I think this lgtm. Thanks for the massive updates to the unit tests.

I'm wondering if we should wait until the branch to land this. WDYT?

https://codereview.chromium.org/2653493009/diff/280001/content/common/service...
File content/common/service_worker/service_worker_provider.mojom (right):

https://codereview.chromium.org/2653493009/diff/280001/content/common/service...
content/common/service_worker/service_worker_provider.mojom:20: associated
ServiceWorkerProvider client_ptr_info;
On 2017/05/19 08:31:07, shimazu wrote:
> On 2017/05/18 04:13:10, falken wrote:
> > Does it make more sense to just name these |provider_host| and |provider|? I
> > probably just don't understand Mojo enough but why the nouns "request" and
> > "client pointer info"?
> 
> I thought _request/_ptr_info are informative because there are several types
> related to an interface. For example, these four types are used for
> ServiceWorkerProviderHost interface:
> 
> - ServiceWorkerProviderHost
> - AssociatedRequest<ServiceWorkerProviderHost>
> - AssociatedBinding<ServiceWorkerProviderHost>
> - AssociatedInterfacePtrInfo<ServiceWorkerProviderHost>
> - AssociatedInterfacePtr<ServiceWorkerProviderHost>
> 
> Request and InterfacePtrInfo are the information to carry the endpoints over
> mojo connection, and Binding and InterfacePtr are receptacles of these
> endpoints.
> 
> What do you think we have _request/_ptr_info for Request and PtrInfo, and no
> suffix for Binding and InterfacePtr?
> 

OK if this is consistent with a common Mojo naming scheme, then I'm OK with it.

https://codereview.chromium.org/2653493009/diff/360001/content/browser/servic...
File content/browser/service_worker/service_worker_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/2653493009/diff/360001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:561:
const int kProviderId = (IsBrowserSideNavigationEnabled() ? -2 : 1001);
Can PlzNavigate be -1001 also to make it look symmetric?

// PlzNavigate uses negative ids.
const int kProviderId = (IsBrowserSideNavigationEnabled() ? -1001 : 1001);

https://codereview.chromium.org/2653493009/diff/360001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:564:
// Setup ServiceWorkerProviderHostInfo
nit: period at end of sentence

https://codereview.chromium.org/2653493009/diff/360001/content/browser/servic...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/2653493009/diff/360001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:90: // matching
host will be found.
"pre-created host" means ServiceWorkerUtils::IsBrowserAssignedProviderId is
true? Maybe the comment should make that clear.

https://codereview.chromium.org/2653493009/diff/360001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:92:
ServiceWorkerUtils::IsBrowserAssignedProviderId(provider_id));
Why a CHECK here? From the style guide: "Use CHECK() if the consequence of a
failed assertion would be a security vulnerability, where crashing the browser
is preferable. "
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md

If you do CHECK, you don't need the return in line 93.

This doesn't look like a security vulnerability though, so I would probably opt
for a DCHECK.

https://codereview.chromium.org/2653493009/diff/360001/content/browser/servic...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/2653493009/diff/360001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:409: // The
renderer-side Mojo endpoint for this provider.
Can you docu binding_ also?

// |provider_| is the renderer-side Mojo endpoint for provider.
mojom::ServiceWorkerProviderAssociatedPtr provider_;
// |binding_| is the Mojo binding that keeps the connection to |provider_|
alive.
mojo::AssociatedBinding<mojom::ServiceWorkerProviderHost> binding_;

Is that right?

https://codereview.chromium.org/2653493009/diff/360001/content/browser/servic...
File content/browser/service_worker/service_worker_provider_host_unittest.cc
(right):

https://codereview.chromium.org/2653493009/diff/360001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host_unittest.cc:82:
static int next_browser_provided_id = -2;
could this be a class member like |next_provider_id_|  instead?

https://codereview.chromium.org/2653493009/diff/360001/content/browser/servic...
File content/browser/service_worker/service_worker_test_utils.cc (right):

https://codereview.chromium.org/2653493009/diff/360001/content/browser/servic...
content/browser/service_worker/service_worker_test_utils.cc:33:
info.host_request = mojo::MakeIsolatedRequest(&output_endpoint->host_ptr);
these four lines seem to be repeated throughout this patch,. is it possible to
factor them out to a function?

https://codereview.chromium.org/2653493009/diff/360001/content/browser/servic...
File content/browser/service_worker/service_worker_test_utils.h (right):

https://codereview.chromium.org/2653493009/diff/360001/content/browser/servic...
content/browser/service_worker/service_worker_test_utils.h:50: // Container for
keeping the Mojo endpoints which should be on the renderer.
The comment should probably mention "provider" somehow.

"A container for keeping the Mojo connection to the service worker provider on
the renderer alive."
?

https://codereview.chromium.org/2653493009/diff/360001/content/browser/servic...
content/browser/service_worker/service_worker_test_utils.h:51: // When
destroying the pointer or request, its error handler will be called.
its means "the |client_request|'s" ?

https://codereview.chromium.org/2653493009/diff/360001/content/browser/servic...
content/browser/service_worker/service_worker_test_utils.h:62: //
ServiceWorkerProviderHost.
content::ServiceWorkerProviderHost?

https://codereview.chromium.org/2653493009/diff/360001/content/child/service_...
File content/child/service_worker/service_worker_provider_context.h (right):

https://codereview.chromium.org/2653493009/diff/360001/content/child/service_...
content/child/service_worker/service_worker_provider_context.h:102:
scoped_refptr<ThreadSafeSender> thread_safe_sender_;
// Mojo binding for the |request| passed to the constructor. This keeps the
connection to the ServiceWorkerProviderHost in the browser process alive.

?

[shimazu, 2017-05-23 06:25:09]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 06:25: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, 2017-05-23 06:28:01]

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

[commit-bot: I haz the power, 2017-05-23 06:28:03]

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...)

[shimazu, 2017-05-23 06:29:32]

shimazu@chromium.org changed reviewers:
+ kinuko@chromium.org, tsepez@chromium.org

[shimazu, 2017-05-23 06:29:34]

kinuko@: PTAL at BUILD.gn and roughly overall. 
tsepez@: PTAL at mojo related stuff (.mojom and struct_traits.(cc|h)).

https://codereview.chromium.org/2653493009/diff/360001/content/browser/servic...
File content/browser/service_worker/service_worker_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/2653493009/diff/360001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:561:
const int kProviderId = (IsBrowserSideNavigationEnabled() ? -2 : 1001);
On 2017/05/22 08:28:12, falken wrote:
> Can PlzNavigate be -1001 also to make it look symmetric?
> 
> // PlzNavigate uses negative ids.
> const int kProviderId = (IsBrowserSideNavigationEnabled() ? -1001 : 1001);

Actually it should be -2 because kProviderId is not used to create the provider
(provider is created on PreCreateNavigationHost) and OnProviderCreated is used
to verify the id owned by SWProviderHostInfo is matching with the receiver's id.

https://codereview.chromium.org/2653493009/diff/360001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:564:
// Setup ServiceWorkerProviderHostInfo
On 2017/05/22 08:28:12, falken wrote:
> nit: period at end of sentence

Done.

https://codereview.chromium.org/2653493009/diff/360001/content/browser/servic...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/2653493009/diff/360001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:90: // matching
host will be found.
On 2017/05/22 08:28:12, falken wrote:
> "pre-created host" means ServiceWorkerUtils::IsBrowserAssignedProviderId is
> true? Maybe the comment should make that clear.

Done.

https://codereview.chromium.org/2653493009/diff/360001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:92:
ServiceWorkerUtils::IsBrowserAssignedProviderId(provider_id));
On 2017/05/22 08:28:12, falken wrote:
> Why a CHECK here? From the style guide: "Use CHECK() if the consequence of a
> failed assertion would be a security vulnerability, where crashing the browser
> is preferable. "
> https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md
> 
> If you do CHECK, you don't need the return in line 93.
> 
> This doesn't look like a security vulnerability though, so I would probably
opt
> for a DCHECK.

Thanks, it's just inherited from the previous implementation. Changed to DCHECK.

https://codereview.chromium.org/2653493009/diff/360001/content/browser/servic...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/2653493009/diff/360001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:409: // The
renderer-side Mojo endpoint for this provider.
On 2017/05/22 08:28:12, falken wrote:
> Can you docu binding_ also?
> 
> // |provider_| is the renderer-side Mojo endpoint for provider.
> mojom::ServiceWorkerProviderAssociatedPtr provider_;
> // |binding_| is the Mojo binding that keeps the connection to |provider_|
> alive.
> mojo::AssociatedBinding<mojom::ServiceWorkerProviderHost> binding_;
> 
> Is that right?

Done.

https://codereview.chromium.org/2653493009/diff/360001/content/browser/servic...
File content/browser/service_worker/service_worker_provider_host_unittest.cc
(right):

https://codereview.chromium.org/2653493009/diff/360001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host_unittest.cc:82:
static int next_browser_provided_id = -2;
On 2017/05/22 08:28:12, falken wrote:
> could this be a class member like |next_provider_id_|  instead?

Done.

https://codereview.chromium.org/2653493009/diff/360001/content/browser/servic...
File content/browser/service_worker/service_worker_test_utils.cc (right):

https://codereview.chromium.org/2653493009/diff/360001/content/browser/servic...
content/browser/service_worker/service_worker_test_utils.cc:33:
info.host_request = mojo::MakeIsolatedRequest(&output_endpoint->host_ptr);
On 2017/05/22 08:28:12, falken wrote:
> these four lines seem to be repeated throughout this patch,. is it possible to
> factor them out to a function?

Done.

https://codereview.chromium.org/2653493009/diff/360001/content/browser/servic...
File content/browser/service_worker/service_worker_test_utils.h (right):

https://codereview.chromium.org/2653493009/diff/360001/content/browser/servic...
content/browser/service_worker/service_worker_test_utils.h:50: // Container for
keeping the Mojo endpoints which should be on the renderer.
On 2017/05/22 08:28:12, falken wrote:
> The comment should probably mention "provider" somehow.
> 
> "A container for keeping the Mojo connection to the service worker provider on
> the renderer alive."
> ?

Done.

https://codereview.chromium.org/2653493009/diff/360001/content/browser/servic...
content/browser/service_worker/service_worker_test_utils.h:51: // When
destroying the pointer or request, its error handler will be called.
On 2017/05/22 08:28:12, falken wrote:
> its means "the |client_request|'s" ?

Removed this line because the comment seems duplicated with the comments on top
of |host_ptr| and |client_request|.

https://codereview.chromium.org/2653493009/diff/360001/content/browser/servic...
content/browser/service_worker/service_worker_test_utils.h:62: //
ServiceWorkerProviderHost.
On 2017/05/22 08:28:12, falken wrote:
> content::ServiceWorkerProviderHost?

Done.

https://codereview.chromium.org/2653493009/diff/360001/content/child/service_...
File content/child/service_worker/service_worker_provider_context.h (right):

https://codereview.chromium.org/2653493009/diff/360001/content/child/service_...
content/child/service_worker/service_worker_provider_context.h:102:
scoped_refptr<ThreadSafeSender> thread_safe_sender_;
On 2017/05/22 08:28:12, falken wrote:
> // Mojo binding for the |request| passed to the constructor. This keeps the
> connection to the ServiceWorkerProviderHost in the browser process alive.
> 
> ?

Done.

[shimazu, 2017-05-23 06:32:20]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 06:32:35]

Dry run: CQ is trying da patch.

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

[shimazu, 2017-05-23 06:34:10]

> I'm wondering if we should wait until the branch to land this. WDYT?

Yeah, I agree. Let's land this after the branch cut.

[falken, 2017-05-23 08:31:49]

https://codereview.chromium.org/2653493009/diff/360001/content/browser/servic...
File content/browser/service_worker/service_worker_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/2653493009/diff/360001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:561:
const int kProviderId = (IsBrowserSideNavigationEnabled() ? -2 : 1001);
On 2017/05/23 06:29:33, shimazu wrote:
> On 2017/05/22 08:28:12, falken wrote:
> > Can PlzNavigate be -1001 also to make it look symmetric?
> > 
> > // PlzNavigate uses negative ids.
> > const int kProviderId = (IsBrowserSideNavigationEnabled() ? -1001 : 1001);
> 
> Actually it should be -2 because kProviderId is not used to create the
provider
> (provider is created on PreCreateNavigationHost) and OnProviderCreated is used
> to verify the id owned by SWProviderHostInfo is matching with the receiver's
id.

Can you add a comment like:

// For PlzNavigate, kProviderId must be -2 in order to match the provider id
assigned in ServiceWorkerProviderHost::PreCreateNavigationHost.

https://codereview.chromium.org/2653493009/diff/360001/content/browser/servic...
File content/browser/service_worker/service_worker_test_utils.h (right):

https://codereview.chromium.org/2653493009/diff/360001/content/browser/servic...
content/browser/service_worker/service_worker_test_utils.h:51: // When
destroying the pointer or request, its error handler will be called.
On 2017/05/23 06:29:34, shimazu wrote:
> On 2017/05/22 08:28:12, falken wrote:
> > its means "the |client_request|'s" ?
> 
> Removed this line because the comment seems duplicated with the comments on
top
> of |host_ptr| and |client_request|.

Acknowledged.

[commit-bot: I haz the power, 2017-05-23 08:48:24]

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

[commit-bot: I haz the power, 2017-05-23 08:48:25]

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

[kinuko, 2017-05-23 12:57:21]

lgtm, I have some documentation-comments on mojom file though. (falken@ might
have different opinion?)

https://codereview.chromium.org/2653493009/diff/400001/content/browser/servic...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/2653493009/diff/400001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:414: // provider
will be destroyed.
nit: It feels the term 'provider' is used ambiguously for both provider and
provider_host?  s/provider/provider host/ ?

https://codereview.chromium.org/2653493009/diff/400001/content/common/service...
File content/common/service_worker/service_worker_provider_interfaces.mojom
(right):

https://codereview.chromium.org/2653493009/diff/400001/content/common/service...
content/common/service_worker/service_worker_provider_interfaces.mojom:8: //
interface bound to content::ServiceWorkerProviderHost. The renderer uses this
Hmm, I can see what this comment tries to clarify but to me it feels a bit weird
to explicitly note that some interface is bound to some implementation... this
is a definition for an interface that can be implemented by any impl class,
while content::ServiceWorkerProviderHost is one of the implementation class,
right?

https://codereview.chromium.org/2653493009/diff/400001/content/common/service...
content/common/service_worker/service_worker_provider_interfaces.mojom:12: //
the connection is disconnected.
Again this seems to be implementation detail which is unrelated to this
interface definition.

[Tom Sepez, 2017-05-23 16:40:21]

lgtm

[falken, 2017-05-29 08:47:29]

https://codereview.chromium.org/2653493009/diff/400001/content/browser/servic...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/2653493009/diff/400001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:58: // resource
loads made directly by the service worker.
Can we add the comments about lifetime here. Who creates/destroys this and when?

https://codereview.chromium.org/2653493009/diff/400001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:414: // provider
will be destroyed.
On 2017/05/23 12:57:20, kinuko wrote:
> nit: It feels the term 'provider' is used ambiguously for both provider and
> provider_host?  s/provider/provider host/ ?

Yea or even better let's just write out "this content::
ServiceWorkerProviderHost"

https://codereview.chromium.org/2653493009/diff/400001/content/common/service...
File content/common/service_worker/service_worker_provider_interfaces.mojom
(right):

https://codereview.chromium.org/2653493009/diff/400001/content/common/service...
content/common/service_worker/service_worker_provider_interfaces.mojom:8: //
interface bound to content::ServiceWorkerProviderHost. The renderer uses this
On 2017/05/23 12:57:20, kinuko wrote:
> Hmm, I can see what this comment tries to clarify but to me it feels a bit
weird
> to explicitly note that some interface is bound to some implementation... this
> is a definition for an interface that can be implemented by any impl class,
> while content::ServiceWorkerProviderHost is one of the implementation class,
> right?

Fair enough, yea. Let's remove the mentions of the implementation. Does it still
make sense to call this a "browser-side interface"? Seems like the common case
even though I guess the impl could put the interface on the renderer.

https://codereview.chromium.org/2653493009/diff/400001/content/common/service...
content/common/service_worker/service_worker_provider_interfaces.mojom:12: //
the connection is disconnected.
On 2017/05/23 12:57:20, kinuko wrote:
> Again this seems to be implementation detail which is unrelated to this
> interface definition.

Sure let's move this documentation to content::ServiceWorkerProviderHost. Class
documentation should talk about the class's lifetime.

https://codereview.chromium.org/2653493009/diff/400001/content/common/service...
content/common/service_worker/service_worker_provider_interfaces.mojom:22: //
content::ServiceWorkerProviderContext.
By the same logic I guess we should just call this a renderer-side interface.
"The browser process uses this interface to do XXX"?

[shimazu, 2017-05-30 02:20:08]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-30 02:20:20]

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-05-30 02:22:31]

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

[commit-bot: I haz the power, 2017-05-30 02:22:33]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[shimazu, 2017-05-30 03:09:33]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-30 03:10:02]

Dry run: CQ is trying da patch.

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

[shimazu, 2017-05-30 03:50:07]

Thanks, updated the comments (ps#23).
falken: Could you take another look just in case?

https://codereview.chromium.org/2653493009/diff/400001/content/browser/servic...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/2653493009/diff/400001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:58: // resource
loads made directly by the service worker.
On 2017/05/29 08:47:28, falken wrote:
> Can we add the comments about lifetime here. Who creates/destroys this and
when?

Done.

https://codereview.chromium.org/2653493009/diff/400001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:414: // provider
will be destroyed.
On 2017/05/29 08:47:28, falken wrote:
> On 2017/05/23 12:57:20, kinuko wrote:
> > nit: It feels the term 'provider' is used ambiguously for both provider and
> > provider_host?  s/provider/provider host/ ?
> 
> Yea or even better let's just write out "this content::
> ServiceWorkerProviderHost"

Done.

https://codereview.chromium.org/2653493009/diff/400001/content/common/service...
File content/common/service_worker/service_worker_provider_interfaces.mojom
(right):

https://codereview.chromium.org/2653493009/diff/400001/content/common/service...
content/common/service_worker/service_worker_provider_interfaces.mojom:8: //
interface bound to content::ServiceWorkerProviderHost. The renderer uses this
On 2017/05/29 08:47:28, falken wrote:
> On 2017/05/23 12:57:20, kinuko wrote:
> > Hmm, I can see what this comment tries to clarify but to me it feels a bit
> weird
> > to explicitly note that some interface is bound to some implementation...
this
> > is a definition for an interface that can be implemented by any impl class,
> > while content::ServiceWorkerProviderHost is one of the implementation class,
> > right?
> 
> Fair enough, yea. Let's remove the mentions of the implementation. Does it
still
> make sense to call this a "browser-side interface"? Seems like the common case
> even though I guess the impl could put the interface on the renderer.

Thanks for your comments. It sounds good to separate the implementation detail
out from mojom. Done.

https://codereview.chromium.org/2653493009/diff/400001/content/common/service...
content/common/service_worker/service_worker_provider_interfaces.mojom:12: //
the connection is disconnected.
On 2017/05/29 08:47:28, falken wrote:
> On 2017/05/23 12:57:20, kinuko wrote:
> > Again this seems to be implementation detail which is unrelated to this
> > interface definition.
> 
> Sure let's move this documentation to content::ServiceWorkerProviderHost.
Class
> documentation should talk about the class's lifetime.

Done.

https://codereview.chromium.org/2653493009/diff/400001/content/common/service...
content/common/service_worker/service_worker_provider_interfaces.mojom:22: //
content::ServiceWorkerProviderContext.
On 2017/05/29 08:47:28, falken wrote:
> By the same logic I guess we should just call this a renderer-side interface.
> "The browser process uses this interface to do XXX"?

Done.

[commit-bot: I haz the power, 2017-05-30 04:02:47]

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

[commit-bot: I haz the power, 2017-05-30 04:02:49]

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

[falken, 2017-05-30 04:12:32]

LGTM % nits

https://codereview.chromium.org/2653493009/diff/440001/content/browser/servic...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/2653493009/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:60: // This
instance is created when navigation is started and
s/This instance/A ServiceWorkerProviderHost instance/

I don't think "navigation is started" is correct: SWPH is also created when you
start a SW.

https://codereview.chromium.org/2653493009/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:61: //
ServiceWorkerNetworkProvider is create on the renderer process. Mojo's
s/create/created

https://codereview.chromium.org/2653493009/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:64: // If
PlzNavigate is turned on, this instance is pre-created on the browser
s/this instance/an instance

https://codereview.chromium.org/2653493009/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:66: // navigation
is possible to be initiated on the browser side. In that case,
s/is possible to be/is/

https://codereview.chromium.org/2653493009/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:68: //
ServiceWorkerNetworkProvider is created on the renderer.
Thanks for the comment!

https://codereview.chromium.org/2653493009/diff/460001/content/browser/servic...
File content/browser/service_worker/service_worker_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/2653493009/diff/460001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:558:
const int kProviderId = (IsBrowserSideNavigationEnabled() ? -2 : 1001);
Can you comment that in the PlzNavigate case it must be -2 to match the
pre-created provider host, and for the non-PlzNavigate case 1001 is just a dummy
value.

https://codereview.chromium.org/2653493009/diff/460001/content/common/service...
File content/common/service_worker/service_worker.mojom (right):

https://codereview.chromium.org/2653493009/diff/460001/content/common/service...
content/common/service_worker/service_worker.mojom:14: // service workers are
starting up.
I guess this violates our new policy of not mentioning impl specifically in the
interface comments.

But the comment already existed before this CL no need to block on it. We can
work this later.

https://codereview.chromium.org/2653493009/diff/460001/content/common/service...
File content/common/service_worker/service_worker_provider_interfaces.mojom
(right):

https://codereview.chromium.org/2653493009/diff/460001/content/common/service...
content/common/service_worker/service_worker_provider_interfaces.mojom:19: //
The browser process uses this interface to carry messages to the page's or
s/carry/send
s/the page's/pages

[shimazu, 2017-05-30 07:24:35]

https://codereview.chromium.org/2653493009/diff/460001/content/browser/servic...
File content/browser/service_worker/service_worker_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/2653493009/diff/460001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:558:
const int kProviderId = (IsBrowserSideNavigationEnabled() ? -2 : 1001);
On 2017/05/30 04:12:32, falken wrote:
> Can you comment that in the PlzNavigate case it must be -2 to match the
> pre-created provider host, and for the non-PlzNavigate case 1001 is just a
dummy
> value.

Done.

https://codereview.chromium.org/2653493009/diff/460001/content/common/service...
File content/common/service_worker/service_worker.mojom (right):

https://codereview.chromium.org/2653493009/diff/460001/content/common/service...
content/common/service_worker/service_worker.mojom:14: // service workers are
starting up.
On 2017/05/30 04:12:32, falken wrote:
> I guess this violates our new policy of not mentioning impl specifically in
the
> interface comments.
> 
> But the comment already existed before this CL no need to block on it. We can
> work this later.

Acknowledged.

https://codereview.chromium.org/2653493009/diff/460001/content/common/service...
File content/common/service_worker/service_worker_provider_interfaces.mojom
(right):

https://codereview.chromium.org/2653493009/diff/460001/content/common/service...
content/common/service_worker/service_worker_provider_interfaces.mojom:19: //
The browser process uses this interface to carry messages to the page's or
On 2017/05/30 04:12:32, falken wrote:
> s/carry/send
> s/the page's/pages

Done.

[shimazu, 2017-05-30 07:24:50]

The CQ bit was checked by shimazu@chromium.org

[shimazu, 2017-05-30 07:24:51]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
kinuko@chromium.org, falken@chromium.org
Link to the patchset: https://codereview.chromium.org/2653493009/#ps480001
(title: "Updated comments and fixed a mistake of rebasing")

[commit-bot: I haz the power, 2017-05-30 07:25:08]

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-05-30 09:36:14]

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

[commit-bot: I haz the power, 2017-05-30 09:36:16]

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_...)

[shimazu, 2017-05-31 02:52:57]

The CQ bit was checked by shimazu@chromium.org

[shimazu, 2017-05-31 02:52:58]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
tsepez@chromium.org, kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/2653493009/#ps500001
(title: "Skip unittest for CrossSiteTransfer when PlzNavigate")

[commit-bot: I haz the power, 2017-05-31 02:53:22]

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-05-31 05:16:14]

CQ is committing da patch.

Bot data: {"patchset_id": 500001, "attempt_start_ts": 1496199177712690,
"parent_rev": "d9b1515a258e10ce170b40eff942232a49d24faf", "commit_rev":
"4223216d2fa2a111c5e0636017bb6766682adda9"}

[commit-bot: I haz the power, 2017-05-31 05:16:25]

Description was changed from

==========
Add two interfaces for ServiceWorkerProviderContext/ProviderHost

This patch introduces two mojom types: ServiceWorkerProviderHost and
ServiceWorkerProvider. They are currently used for managing the lifetime of
ServiceWorkerProviderHost from the renderer, but these interfaces will have
methods managing registrations, registration associated by the document and
controller.
These interfaces are now associated with SWDispatcherHost which is already bound
with the channel. After crrev.com/2779763004, interfaces for worker's context
will be associated with EWInstanceClient. This will solve the ordering issue
between messages of Stopped and ProviderDestroyed.

BUG=629701,676983,668633
==========

to

==========
Add two interfaces for ServiceWorkerProviderContext/ProviderHost

This patch introduces two mojom types: ServiceWorkerProviderHost and
ServiceWorkerProvider. They are currently used for managing the lifetime of
ServiceWorkerProviderHost from the renderer, but these interfaces will have
methods managing registrations, registration associated by the document and
controller.
These interfaces are now associated with SWDispatcherHost which is already bound
with the channel. After crrev.com/2779763004, interfaces for worker's context
will be associated with EWInstanceClient. This will solve the ordering issue
between messages of Stopped and ProviderDestroyed.

BUG=629701,676983,668633

Review-Url: https://codereview.chromium.org/2653493009
Cr-Commit-Position: refs/heads/master@{#475809}
Committed:
https://chromium.googlesource.com/chromium/src/+/4223216d2fa2a111c5e0636017bb...
==========

[commit-bot: I haz the power, 2017-05-31 05:16:27]

Committed patchset #25 (id:500001) as
https://chromium.googlesource.com/chromium/src/+/4223216d2fa2a111c5e0636017bb...

[falken, 2017-06-01 07:56:41]

On 2017/05/31 05:16:27, commit-bot: I haz the power wrote:
> Committed patchset #25 (id:500001) as
>
https://chromium.googlesource.com/chromium/src/+/4223216d2fa2a111c5e0636017bb...

I think you missed the first half of my nits :)

[shimazu, 2017-06-01 08:18:54]

Oops... I missed them. 
I created another CL to fix them:
https://chromium-review.googlesource.com/c/520445/

https://codereview.chromium.org/2653493009/diff/440001/content/browser/servic...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/2653493009/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:60: // This
instance is created when navigation is started and
On 2017/05/30 04:12:31, falken wrote:
> s/This instance/A ServiceWorkerProviderHost instance/
> 
> I don't think "navigation is started" is correct: SWPH is also created when
you
> start a SW.

Addressed this at https://chromium-review.googlesource.com/520445

https://codereview.chromium.org/2653493009/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:61: //
ServiceWorkerNetworkProvider is create on the renderer process. Mojo's
On 2017/05/30 04:12:32, falken wrote:
> s/create/created

Addressed this at https://chromium-review.googlesource.com/520445

https://codereview.chromium.org/2653493009/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:64: // If
PlzNavigate is turned on, this instance is pre-created on the browser
On 2017/05/30 04:12:32, falken wrote:
> s/this instance/an instance

Addressed this at https://chromium-review.googlesource.com/520445

https://codereview.chromium.org/2653493009/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:66: // navigation
is possible to be initiated on the browser side. In that case,
On 2017/05/30 04:12:32, falken wrote:
> s/is possible to be/is/

Addressed this at https://chromium-review.googlesource.com/520445

https://codereview.chromium.org/2653493009/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:68: //
ServiceWorkerNetworkProvider is created on the renderer.
On 2017/05/30 04:12:31, falken wrote:
> Thanks for the comment!

Your welcome!:)

https://codereview.chromium.org/2653493009/diff/460001/content/common/service...
File content/common/service_worker/service_worker.mojom (right):

https://codereview.chromium.org/2653493009/diff/460001/content/common/service...
content/common/service_worker/service_worker.mojom:14: // service workers are
starting up.
On 2017/05/30 04:12:32, falken wrote:
> I guess this violates our new policy of not mentioning impl specifically in
the
> interface comments.
> 
> But the comment already existed before this CL no need to block on it. We can
> work this later.

Addressed this at https://chromium-review.googlesource.com/520445