Manage ServiceWorkerDispatcherHost in ServiceWorkerContextCore

Manage ServiceWorkerDispatcherHost in ServiceWorkerContextCore

ServiceWorkerProviders for the service workers currently have the mesasge
ordering issue between ProviderDestroyed and WorkerStopped. Associating the
message pipes correctly will fix that. I'm planning to bind the
ServiceWorkerProvider interfaces with the channel's pipe for pages and with
the EmbeddedWorkerInstanceClient's pipe for service workers.

This is the first patch to introduce the associated provider hosts.
This patch
 - adds ServiceWorkerProviderHostInfo to send the params to SWDispatcherHost
 - manages SWDispatcherHost in SWContextCore
 - creates SWProviderHost by SWProviderHostInfo
 - removes three legacy IPC messages
   - ServiceWorkerHostMsg_ProviderCreated
   - ServiceWorkerHostMsg_ProviderDestroyed
   - ServiceWorkerHostMsg_SetVersionId

BUG=668633, 676983, 629701
Review-Url: https://codereview.chromium.org/2638313002
Cr-Commit-Position: refs/heads/master@{#451745}
Committed: https://chromium.googlesource.com/chromium/src/+/3b85dfb581ef764c8a02dbd803f54d0bbab1f318

[shimazu, 2017-01-19 08:33:23]

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

[commit-bot: I haz the power, 2017-01-19 08:33:48]

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-01-19 08:41:24]

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

[commit-bot: I haz the power, 2017-01-19 08:41:25]

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

[shimazu, 2017-01-23 05:53:38]

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

[commit-bot: I haz the power, 2017-01-23 05:53:47]

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-01-23 07:46:45]

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

[commit-bot: I haz the power, 2017-01-23 07:46:46]

Dry run: This issue passed the CQ dry run.

[shimazu, 2017-01-25 04:29:52]

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

[commit-bot: I haz the power, 2017-01-25 04:30:30]

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-01-25 04:43:14]

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

[commit-bot: I haz the power, 2017-01-25 04:43:16]

Dry run: 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...)

[shimazu, 2017-01-25 05:49:37]

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

[commit-bot: I haz the power, 2017-01-25 05:49:50]

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

[shimazu, 2017-01-25 06:06:53]

Description was changed from

==========
[WIP] Managing ServiceWorkerDispatcherHosts in ServiceWorkerContextCore

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

to

==========
Construct ServiceWorkerProviderHost in ServiceWorkerContextCore

This patch 
 - introduces ServiceWorkerProviderHostInfo to send the params to
SWDispatcherHost
 - manages SWDispatcherHost in SWContextCore
 - creates SWProviderHost in SWContextCore
 - removes three legacy IPC messages
   - ServiceWorkerHostMsg_ProviderCreated
   - ServiceWorkerHostMsg_ProviderDestroyed
   - ServiceWorkerHostMsg_SetVersionId

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

[shimazu, 2017-01-25 06:07:09]

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

[commit-bot: I haz the power, 2017-01-25 06:07:22]

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

[shimazu, 2017-01-25 07:21:43]

Description was changed from

==========
Construct ServiceWorkerProviderHost in ServiceWorkerContextCore

This patch 
 - introduces ServiceWorkerProviderHostInfo to send the params to
SWDispatcherHost
 - manages SWDispatcherHost in SWContextCore
 - creates SWProviderHost in SWContextCore
 - removes three legacy IPC messages
   - ServiceWorkerHostMsg_ProviderCreated
   - ServiceWorkerHostMsg_ProviderDestroyed
   - ServiceWorkerHostMsg_SetVersionId

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

to

==========
Construct ServiceWorkerProviderHost in ServiceWorkerContextCore

ServiceWorkerProviders for the service workers currently have the mesasge
ordering issue between ProviderDestroyed and WorkerStopped. Associating the
message pipes correctly will fix that. I'm planning to bind the
ServiceWorkerProvider interfaces to send the IPC with the channel for pages and
with the EmbeddedWorkerInstanceClient's pipe for service workers.

This is the first patch to introduce the associated provider hosts.
This patch
 - adds ServiceWorkerProviderHostInfo to send the params to SWDispatcherHost
 - manages SWDispatcherHost in SWContextCore
 - creates SWProviderHost in SWContextCore
 - removes three legacy IPC messages
   - ServiceWorkerHostMsg_ProviderCreated
   - ServiceWorkerHostMsg_ProviderDestroyed
   - ServiceWorkerHostMsg_SetVersionId

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

[shimazu, 2017-01-25 07:22:51]

Description was changed from

==========
Construct ServiceWorkerProviderHost in ServiceWorkerContextCore

ServiceWorkerProviders for the service workers currently have the mesasge
ordering issue between ProviderDestroyed and WorkerStopped. Associating the
message pipes correctly will fix that. I'm planning to bind the
ServiceWorkerProvider interfaces to send the IPC with the channel for pages and
with the EmbeddedWorkerInstanceClient's pipe for service workers.

This is the first patch to introduce the associated provider hosts.
This patch
 - adds ServiceWorkerProviderHostInfo to send the params to SWDispatcherHost
 - manages SWDispatcherHost in SWContextCore
 - creates SWProviderHost in SWContextCore
 - removes three legacy IPC messages
   - ServiceWorkerHostMsg_ProviderCreated
   - ServiceWorkerHostMsg_ProviderDestroyed
   - ServiceWorkerHostMsg_SetVersionId

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

to

==========
Construct ServiceWorkerProviderHost in ServiceWorkerContextCore

ServiceWorkerProviders for the service workers currently have the mesasge
ordering issue between ProviderDestroyed and WorkerStopped. Associating the
message pipes correctly will fix that. I'm planning to bind the
ServiceWorkerProvider interfaces with the channel's pipe for pages and with
the EmbeddedWorkerInstanceClient's pipe for service workers.

This is the first patch to introduce the associated provider hosts.
This patch
 - adds ServiceWorkerProviderHostInfo to send the params to SWDispatcherHost
 - manages SWDispatcherHost in SWContextCore
 - creates SWProviderHost in SWContextCore
 - removes three legacy IPC messages
   - ServiceWorkerHostMsg_ProviderCreated
   - ServiceWorkerHostMsg_ProviderDestroyed
   - ServiceWorkerHostMsg_SetVersionId

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

[shimazu, 2017-01-25 07:31:28]

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

[shimazu, 2017-01-25 07:31:29]

external/wpt/html/browsers/origin/cross-origin-objects/cross-origin-objects-exceptions.html
still looks failing, but could you give me a first round review?

[horo, 2017-01-26 02:40:23]

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

https://codereview.chromium.org/2638313002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:84: static
std::unique_ptr<ServiceWorkerProviderHost> CreateForTesting(
How about adding "bool is_parent_frame_secure" argument?
You can remove FrameSecurityLevel::UNINITIALIZED and make
|parent_frame_security_level_| const, and remove set_parent_frame_secure().

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

https://codereview.chromium.org/2638313002/diff/140001/content/common/service...
content/common/service_worker/service_worker.mojom:14: // |provider_id| is
unique within its child process. When this provider is
These comments should be in service_worker_provider.mojom before
ServiceWorkerProviderHostInfo.

[falken, 2017-01-26 05:15:41]

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

[falken, 2017-01-26 05:15:43]

Some comments.

https://codereview.chromium.org/2638313002/diff/140001/content/browser/servic...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/2638313002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_context_core.cc:281:
dispatcher_host->message_port_message_filter());
Why does ServiceWorkerContextCore need dispatcher_hosts_ if
EmbeddedWorkerRegistry also stores them? I think all we pass to
AddChildProcessSender is ServiceWorkerDispatcherHosts. I'd rather not store it
in two maps.

https://codereview.chromium.org/2638313002/diff/140001/content/browser/servic...
File content/browser/service_worker/service_worker_context_core.h (right):

https://codereview.chromium.org/2638313002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_context_core.h:73: using
DispatcherHostMap = IDMap<ServiceWorkerDispatcherHost*>;
It feels misleading that the other *Map types here own their elements, and
DispatcherhostMap doesn't. I would just type out
IDMap<ServiceWorkerDispatcherHost*>. (I'm not really a fan of these Map aliases
in general, they don't really add clarify IMO.)

https://codereview.chromium.org/2638313002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_context_core.h:160: // through
them.
It's a bit inconsistent that AddDispatcherHost doesn't take ownership of the
pointer, but AddProviderHost does. Maybe say "The DispatcherHosts are not owned
by this class."

https://codereview.chromium.org/2638313002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_context_core.h:169:
ServiceWorkerProviderHostInfo provider_host_info);
nit: Consider removing the function overloading by having ProviderHost provide
the functions to create the host, and then all you need is
AddProviderHost(std::unique_ptr<>).

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

https://codereview.chromium.org/2638313002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:124: return
base::WrapUnique(new ServiceWorkerProviderHost(
MakeUnique is preferred

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

https://codereview.chromium.org/2638313002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:84: static
std::unique_ptr<ServiceWorkerProviderHost> CreateForTesting(
Can this be moved to a file like
service_worker/test/service_worker_test_utils.h? I'd like to avoid test-only
code in the release binary.

https://codereview.chromium.org/2638313002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:90:
ServiceWorkerDispatcherHost* dispatcher_host = nullptr);
The style guide discourages default arguments. Could we possibly avoid them here
with better named functions (CreateForServiceWorkerContext,
CreateWithDispatcherHost, etc?). Or should everyone just pass
ServiceWorkerProviderHostInfo struct, and we have helpers for creating that Info
struct?

[shimazu, 2017-01-26 09:25:02]

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

[commit-bot: I haz the power, 2017-01-26 09:25:17]

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

[shimazu, 2017-01-26 09:27:23]

Thanks for your comments. 
For comments I've replied to as ack, I'll fix them later.

https://codereview.chromium.org/2638313002/diff/140001/content/browser/servic...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/2638313002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_context_core.cc:281:
dispatcher_host->message_port_message_filter());
On 2017/01/26 05:15:42, falken wrote:
> Why does ServiceWorkerContextCore need dispatcher_hosts_ if
> EmbeddedWorkerRegistry also stores them? I think all we pass to
> AddChildProcessSender is ServiceWorkerDispatcherHosts. I'd rather not store it
> in two maps.

It's because I'm planning to use SWDispatcherHost to create SWProviderHost in
EWInstance or somewhere on a later patch. I think SWContextCore should keep such
objects and others should retrieve from the SWContenxtCore. 
I've updated this patch to remove the map owned by EWRegistry and let EWRegistry
send messages with asking SWContextCore everytime.

https://codereview.chromium.org/2638313002/diff/140001/content/browser/servic...
File content/browser/service_worker/service_worker_context_core.h (right):

https://codereview.chromium.org/2638313002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_context_core.h:73: using
DispatcherHostMap = IDMap<ServiceWorkerDispatcherHost*>;
On 2017/01/26 05:15:43, falken wrote:
> It feels misleading that the other *Map types here own their elements, and
> DispatcherhostMap doesn't. I would just type out
> IDMap<ServiceWorkerDispatcherHost*>. (I'm not really a fan of these Map
aliases
> in general, they don't really add clarify IMO.)

Makes sense. Done.

https://codereview.chromium.org/2638313002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_context_core.h:160: // through
them.
On 2017/01/26 05:15:43, falken wrote:
> It's a bit inconsistent that AddDispatcherHost doesn't take ownership of the
> pointer, but AddProviderHost does. Maybe say "The DispatcherHosts are not
owned
> by this class."

Done.

https://codereview.chromium.org/2638313002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_context_core.h:169:
ServiceWorkerProviderHostInfo provider_host_info);
On 2017/01/26 05:15:42, falken wrote:
> nit: Consider removing the function overloading by having ProviderHost provide
> the functions to create the host, and then all you need is
> AddProviderHost(std::unique_ptr<>).

Done.

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

https://codereview.chromium.org/2638313002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:124: return
base::WrapUnique(new ServiceWorkerProviderHost(
On 2017/01/26 05:15:43, falken wrote:
> MakeUnique is preferred

The constructor is private, so WrapUnique is used here:
https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-a...

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

https://codereview.chromium.org/2638313002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:84: static
std::unique_ptr<ServiceWorkerProviderHost> CreateForTesting(
On 2017/01/26 05:15:43, falken wrote:
> Can this be moved to a file like
> service_worker/test/service_worker_test_utils.h? I'd like to avoid test-only
> code in the release binary.

Acknowledged.

https://codereview.chromium.org/2638313002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:90:
ServiceWorkerDispatcherHost* dispatcher_host = nullptr);
On 2017/01/26 05:15:43, falken wrote:
> The style guide discourages default arguments. Could we possibly avoid them
here
> with better named functions (CreateForServiceWorkerContext,
> CreateWithDispatcherHost, etc?). Or should everyone just pass
> ServiceWorkerProviderHostInfo struct, and we have helpers for creating that
Info
> struct?

Acknowledged.

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

https://codereview.chromium.org/2638313002/diff/140001/content/common/service...
content/common/service_worker/service_worker.mojom:14: // |provider_id| is
unique within its child process. When this provider is
On 2017/01/26 02:40:23, horo wrote:
> These comments should be in service_worker_provider.mojom before
> ServiceWorkerProviderHostInfo.

Acknowledged.

[commit-bot: I haz the power, 2017-01-26 09:53:35]

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

[commit-bot: I haz the power, 2017-01-26 09:53:36]

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

[shimazu, 2017-02-03 00:51:19]

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

[commit-bot: I haz the power, 2017-02-03 00:52:02]

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-03 02:30:58]

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

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

Dry run: This issue passed the CQ dry run.

[shimazu, 2017-02-07 08:47:02]

Updated for the remaining. ptal again:)

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

https://codereview.chromium.org/2638313002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:84: static
std::unique_ptr<ServiceWorkerProviderHost> CreateForTesting(
On 2017/01/26 09:27:23, shimazu wrote:
> On 2017/01/26 05:15:43, falken wrote:
> > Can this be moved to a file like
> > service_worker/test/service_worker_test_utils.h? I'd like to avoid test-only
> > code in the release binary.
> 
> Acknowledged.

Done.

https://codereview.chromium.org/2638313002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:90:
ServiceWorkerDispatcherHost* dispatcher_host = nullptr);
On 2017/01/26 09:27:23, shimazu wrote:
> On 2017/01/26 05:15:43, falken wrote:
> > The style guide discourages default arguments. Could we possibly avoid them
> here
> > with better named functions (CreateForServiceWorkerContext,
> > CreateWithDispatcherHost, etc?). Or should everyone just pass
> > ServiceWorkerProviderHostInfo struct, and we have helpers for creating that
> Info
> > struct?
> 
> Acknowledged.

I've tried to add CreateForXXX to test_utils and used
ServcieWorkerProviderHostInfo everywhere.

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

https://codereview.chromium.org/2638313002/diff/140001/content/common/service...
content/common/service_worker/service_worker.mojom:14: // |provider_id| is
unique within its child process. When this provider is
On 2017/01/26 09:27:23, shimazu wrote:
> On 2017/01/26 02:40:23, horo wrote:
> > These comments should be in service_worker_provider.mojom before
> > ServiceWorkerProviderHostInfo.
> 
> Acknowledged.

Done.

[horo, 2017-02-08 02:14:11]

Please update the CL title and description.

https://codereview.chromium.org/2638313002/diff/220001/content/browser/servic...
File content/browser/service_worker/embedded_worker_test_helper.cc (right):

https://codereview.chromium.org/2638313002/diff/220001/content/browser/servic...
content/browser/service_worker/embedded_worker_test_helper.cc:80: };
nit: 
  DISALLOW_COPY_AND_ASSIGN(MockServiceWorkerDispatcherHost);

https://codereview.chromium.org/2638313002/diff/220001/content/browser/servic...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/2638313002/diff/220001/content/browser/servic...
content/browser/service_worker/service_worker_context_core.cc:311: return NULL;
nit: nullptr

https://codereview.chromium.org/2638313002/diff/220001/content/browser/servic...
File content/browser/service_worker/service_worker_context_core.h (right):

https://codereview.chromium.org/2638313002/diff/220001/content/browser/servic...
content/browser/service_worker/service_worker_context_core.h:168: void
AddProviderHost(int process_id,
Remove this.

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

https://codereview.chromium.org/2638313002/diff/220001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:84: enum class
FrameSecurityLevel { INSECURE, SECURE };
you can remove this.

https://codereview.chromium.org/2638313002/diff/220001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:384: const
FrameSecurityLevel parent_frame_security_level_;
"const bool is_parent_frame_secure;" is enough?

[falken, 2017-02-08 06:53:51]

https://codereview.chromium.org/2638313002/diff/220001/content/browser/servic...
File content/browser/service_worker/embedded_worker_registry.cc (right):

https://codereview.chromium.org/2638313002/diff/220001/content/browser/servic...
content/browser/service_worker/embedded_worker_registry.cc:195: // than
OnStopped so UMA doesn't record it as a normal stoppage.
So we expect the workers to have been stopped normally at the time when
"DetachProcess" is called, and this is forcibly reseting the state of zombie
workers, right? I think we need some better comments here.

https://codereview.chromium.org/2638313002/diff/220001/content/browser/servic...
File content/browser/service_worker/embedded_worker_registry.h (right):

https://codereview.chromium.org/2638313002/diff/220001/content/browser/servic...
content/browser/service_worker/embedded_worker_registry.h:91: void
DetachProcess(int process_id);
Can we have a better name than "Detach". Historically, "Detach" just meant an
emergency measure when "Stop" wasn't working. Should just be "RemoveProcess"?
The comment also doesn't explain well what "detach" means.

https://codereview.chromium.org/2638313002/diff/220001/content/browser/servic...
File content/browser/service_worker/embedded_worker_test_helper.h (right):

https://codereview.chromium.org/2638313002/diff/220001/content/browser/servic...
content/browser/service_worker/embedded_worker_test_helper.h:272: std::map<int,
scoped_refptr<ServiceWorkerDispatcherHost>> dispatcher_hosts_;
Can you comment what int is, like line 280 does?

https://codereview.chromium.org/2638313002/diff/220001/content/browser/servic...
File content/browser/service_worker/foreign_fetch_request_handler_unittest.cc
(left):

https://codereview.chromium.org/2638313002/diff/220001/content/browser/servic...
content/browser/service_worker/foreign_fetch_request_handler_unittest.cc:87:
context()->AddProviderHost(std::move(host));
I may be missing something but it looks like you can still call
CreateWindowTypeProviderHost here and then you don't need to call it in each
test.

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

https://codereview.chromium.org/2638313002/diff/220001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host_unittest.cc:75: bool
is_parent_frame_secure = true) {
Let's avoid the default param and add a
CreateProviderHostWithInsecureParentFrame instead.

https://codereview.chromium.org/2638313002/diff/220001/content/child/service_...
File content/child/service_worker/service_worker_network_provider.cc (left):

https://codereview.chromium.org/2638313002/diff/220001/content/child/service_...
content/child/service_worker/service_worker_network_provider.cc:160: if
(ServiceWorkerUtils::IsMojoForServiceWorkerEnabled()) {
BTW, do we still need this function to exist?

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

https://codereview.chromium.org/2638313002/diff/220001/content/common/service...
content/common/service_worker/service_worker.mojom:16: // Informs the browser
that a service worker is starting up in a provider.
nit: let's just remove "in a provider". A SW can only start up in a provider.

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

https://codereview.chromium.org/2638313002/diff/220001/content/common/service...
content/common/service_worker/service_worker_provider.mojom:26: //
|is_parent_frame_secure| is true.
I slightly prefer these comments to be in
	content/common/service_worker/service_worker_provider.h since it's hard to find
.mojom files in codesearch. Either way, one should link to the other.

Also, seems easier to read if the comments are directly in front of the fields,
instead of all at the top.

[falken, 2017-02-09 01:03:20]

https://codereview.chromium.org/2638313002/diff/220001/content/common/service...
File content/common/service_worker/service_worker_provider.h (right):

https://codereview.chromium.org/2638313002/diff/220001/content/common/service...
content/common/service_worker/service_worker_provider.h:12: struct
CONTENT_EXPORT ServiceWorkerProviderHostInfo {
nit: If this is the only thing meant for this file, should it just be named
"service_worker_provider_host_info.h" instead?

[shimazu, 2017-02-13 02:25:06]

Description was changed from

==========
Construct ServiceWorkerProviderHost in ServiceWorkerContextCore

ServiceWorkerProviders for the service workers currently have the mesasge
ordering issue between ProviderDestroyed and WorkerStopped. Associating the
message pipes correctly will fix that. I'm planning to bind the
ServiceWorkerProvider interfaces with the channel's pipe for pages and with
the EmbeddedWorkerInstanceClient's pipe for service workers.

This is the first patch to introduce the associated provider hosts.
This patch
 - adds ServiceWorkerProviderHostInfo to send the params to SWDispatcherHost
 - manages SWDispatcherHost in SWContextCore
 - creates SWProviderHost in SWContextCore
 - removes three legacy IPC messages
   - ServiceWorkerHostMsg_ProviderCreated
   - ServiceWorkerHostMsg_ProviderDestroyed
   - ServiceWorkerHostMsg_SetVersionId

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

to

==========
Manage ServiceWorkerDispatcherHost in ServiceWorkerContextCore

ServiceWorkerProviders for the service workers currently have the mesasge
ordering issue between ProviderDestroyed and WorkerStopped. Associating the
message pipes correctly will fix that. I'm planning to bind the
ServiceWorkerProvider interfaces with the channel's pipe for pages and with
the EmbeddedWorkerInstanceClient's pipe for service workers.

This is the first patch to introduce the associated provider hosts.
This patch
 - adds ServiceWorkerProviderHostInfo to send the params to SWDispatcherHost
 - manages SWDispatcherHost in SWContextCore
 - creates SWProviderHost by SWProviderHostInfo
 - removes three legacy IPC messages
   - ServiceWorkerHostMsg_ProviderCreated
   - ServiceWorkerHostMsg_ProviderDestroyed
   - ServiceWorkerHostMsg_SetVersionId

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

[shimazu, 2017-02-13 02:29:49]

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

[commit-bot: I haz the power, 2017-02-13 02:30:08]

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 02:32:21]

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

[commit-bot: I haz the power, 2017-02-13 02:32:23]

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-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[shimazu, 2017-02-13 03:15:51]

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

[commit-bot: I haz the power, 2017-02-13 03:16:08]

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 03:17:01]

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

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

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

[shimazu, 2017-02-13 03:25:56]

Updated the patch. 
PTAL again:)

https://codereview.chromium.org/2638313002/diff/220001/content/browser/servic...
File content/browser/service_worker/embedded_worker_registry.cc (right):

https://codereview.chromium.org/2638313002/diff/220001/content/browser/servic...
content/browser/service_worker/embedded_worker_registry.cc:195: // than
OnStopped so UMA doesn't record it as a normal stoppage.
On 2017/02/08 06:53:51, falken wrote:
> So we expect the workers to have been stopped normally at the time when
> "DetachProcess" is called, and this is forcibly reseting the state of zombie
> workers, right? I think we need some better comments here.

Yes, that's right.
Tried updating the comment.

https://codereview.chromium.org/2638313002/diff/220001/content/browser/servic...
File content/browser/service_worker/embedded_worker_registry.h (right):

https://codereview.chromium.org/2638313002/diff/220001/content/browser/servic...
content/browser/service_worker/embedded_worker_registry.h:91: void
DetachProcess(int process_id);
On 2017/02/08 06:53:51, falken wrote:
> Can we have a better name than "Detach". Historically, "Detach" just meant an
> emergency measure when "Stop" wasn't working. Should just be "RemoveProcess"?
> The comment also doesn't explain well what "detach" means.

Done.

https://codereview.chromium.org/2638313002/diff/220001/content/browser/servic...
File content/browser/service_worker/embedded_worker_test_helper.cc (right):

https://codereview.chromium.org/2638313002/diff/220001/content/browser/servic...
content/browser/service_worker/embedded_worker_test_helper.cc:80: };
On 2017/02/08 02:14:11, horo wrote:
> nit: 
>   DISALLOW_COPY_AND_ASSIGN(MockServiceWorkerDispatcherHost);

Done.

https://codereview.chromium.org/2638313002/diff/220001/content/browser/servic...
File content/browser/service_worker/embedded_worker_test_helper.h (right):

https://codereview.chromium.org/2638313002/diff/220001/content/browser/servic...
content/browser/service_worker/embedded_worker_test_helper.h:272: std::map<int,
scoped_refptr<ServiceWorkerDispatcherHost>> dispatcher_hosts_;
On 2017/02/08 06:53:51, falken wrote:
> Can you comment what int is, like line 280 does?

Done.

https://codereview.chromium.org/2638313002/diff/220001/content/browser/servic...
File content/browser/service_worker/foreign_fetch_request_handler_unittest.cc
(left):

https://codereview.chromium.org/2638313002/diff/220001/content/browser/servic...
content/browser/service_worker/foreign_fetch_request_handler_unittest.cc:87:
context()->AddProviderHost(std::move(host));
On 2017/02/08 06:53:51, falken wrote:
> I may be missing something but it looks like you can still call
> CreateWindowTypeProviderHost here and then you don't need to call it in each
> test.

Yes I can, but I thought that makes this test a bit complicated like
context()->RemoveProviderHost() should be called before AddProviderHost in
CreateServiceWorkerTypeProviderHost() if CreateWindowTypeProviderHost() calls
here. 
It's easier to understand if CreateWindowTypeProviderHost and
CreateServiceWorkerTypeProviderHost are symmetrical and do mostly the same thing
instead of calling CreateWindowTypeProviderHost by default.

What do you think?

https://codereview.chromium.org/2638313002/diff/220001/content/browser/servic...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/2638313002/diff/220001/content/browser/servic...
content/browser/service_worker/service_worker_context_core.cc:311: return NULL;
On 2017/02/08 02:14:11, horo wrote:
> nit: nullptr

Done.

https://codereview.chromium.org/2638313002/diff/220001/content/browser/servic...
File content/browser/service_worker/service_worker_context_core.h (right):

https://codereview.chromium.org/2638313002/diff/220001/content/browser/servic...
content/browser/service_worker/service_worker_context_core.h:168: void
AddProviderHost(int process_id,
On 2017/02/08 02:14:11, horo wrote:
> Remove this.

Done.

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

https://codereview.chromium.org/2638313002/diff/220001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:84: enum class
FrameSecurityLevel { INSECURE, SECURE };
On 2017/02/08 02:14:11, horo wrote:
> you can remove this.

Done.

https://codereview.chromium.org/2638313002/diff/220001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:384: const
FrameSecurityLevel parent_frame_security_level_;
On 2017/02/08 02:14:11, horo wrote:
> "const bool is_parent_frame_secure;" is enough?

Done.

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

https://codereview.chromium.org/2638313002/diff/220001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host_unittest.cc:75: bool
is_parent_frame_secure = true) {
On 2017/02/08 06:53:51, falken wrote:
> Let's avoid the default param and add a
> CreateProviderHostWithInsecureParentFrame instead.

Done.

https://codereview.chromium.org/2638313002/diff/220001/content/child/service_...
File content/child/service_worker/service_worker_network_provider.cc (left):

https://codereview.chromium.org/2638313002/diff/220001/content/child/service_...
content/child/service_worker/service_worker_network_provider.cc:160: if
(ServiceWorkerUtils::IsMojoForServiceWorkerEnabled()) {
On 2017/02/08 06:53:51, falken wrote:
> BTW, do we still need this function to exist?

Yes, we do. IsMojoForServiceWorkerEnabled is still used at
EmbeddedWorkerInstance::Stop(). I'll remove that on the later patch.

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

https://codereview.chromium.org/2638313002/diff/220001/content/common/service...
content/common/service_worker/service_worker.mojom:16: // Informs the browser
that a service worker is starting up in a provider.
On 2017/02/08 06:53:51, falken wrote:
> nit: let's just remove "in a provider". A SW can only start up in a provider.

Done.

https://codereview.chromium.org/2638313002/diff/220001/content/common/service...
File content/common/service_worker/service_worker_provider.h (right):

https://codereview.chromium.org/2638313002/diff/220001/content/common/service...
content/common/service_worker/service_worker_provider.h:12: struct
CONTENT_EXPORT ServiceWorkerProviderHostInfo {
On 2017/02/09 01:03:19, falken wrote:
> nit: If this is the only thing meant for this file, should it just be named
> "service_worker_provider_host_info.h" instead?

I'm planning to add ServiceWorkerProviderClientInfo to construct
ServiceWorkerProviderContext on later patch.
(SWProviderHostInfo in https://crrev.com/2627413002 will be SWProviderClientInfo
later.)

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

https://codereview.chromium.org/2638313002/diff/220001/content/common/service...
content/common/service_worker/service_worker_provider.mojom:26: //
|is_parent_frame_secure| is true.
On 2017/02/08 06:53:51, falken wrote:
> I slightly prefer these comments to be in
> 	content/common/service_worker/service_worker_provider.h since it's hard to
find
> .mojom files in codesearch. Either way, one should link to the other.
> 
> Also, seems easier to read if the comments are directly in front of the
fields,
> instead of all at the top.

Done.

[falken, 2017-02-13 05:59:24]

lgtm

https://codereview.chromium.org/2638313002/diff/220001/content/common/service...
File content/common/service_worker/service_worker_provider.h (right):

https://codereview.chromium.org/2638313002/diff/220001/content/common/service...
content/common/service_worker/service_worker_provider.h:12: struct
CONTENT_EXPORT ServiceWorkerProviderHostInfo {
On 2017/02/13 03:25:56, shimazu wrote:
> On 2017/02/09 01:03:19, falken wrote:
> > nit: If this is the only thing meant for this file, should it just be named
> > "service_worker_provider_host_info.h" instead?
> 
> I'm planning to add ServiceWorkerProviderClientInfo to construct
> ServiceWorkerProviderContext on later patch.
> (SWProviderHostInfo in https://crrev.com/2627413002 will be
SWProviderClientInfo
> later.)

If SWProviderHostInfo will become SWProviderClientInfo, then it sounds like this
file should be named "service_worker_provider_client_info.h" at that time. But
in this patch I think it we should just name it
"service_worker_provider_host_info.h" and then rename it when its content
changes.

https://codereview.chromium.org/2638313002/diff/260001/content/browser/servic...
File content/browser/service_worker/embedded_worker_registry.cc (right):

https://codereview.chromium.org/2638313002/diff/260001/content/browser/servic...
content/browser/service_worker/embedded_worker_registry.cc:194: // browser
process somehow.
I like how the existing comment explains why OnDetached is used. My previous
confusion is nothing mentioned that RemoveProcess should be called after the
workers have stopped.

I'd keep the previous comment and just add to the beginning that "RemoveProcess
is typically called after the running workers on the process have been stopped,
so if there is a running worker at this point somehow the worker thread has lost
contact with the browser process."

https://codereview.chromium.org/2638313002/diff/260001/content/browser/servic...
File content/browser/service_worker/embedded_worker_registry.h (right):

https://codereview.chromium.org/2638313002/diff/260001/content/browser/servic...
content/browser/service_worker/embedded_worker_registry.h:91: // |worker_map_|.
IMO it's best to avoid referencing private members in the documentation for a
public function.

This API may be confusing because there is no corresponding AddProcess or
AddWorker function. The reader wonders where these workers are coming from.

My suggestion: "Removes information about the service workers running on the
process and calls ServiceWorkerVersion::OnDetached() on each. Called when the
process is terminated. Under normal operation, the workers should already have
been stopped before the process is terminated, in which case this function does
nothing. But in some cases the process can be terminated unexpectedly or the
workers can fail to stop cleanly."

https://codereview.chromium.org/2638313002/diff/260001/content/browser/servic...
File content/browser/service_worker/link_header_support_unittest.cc (right):

https://codereview.chromium.org/2638313002/diff/260001/content/browser/servic...
content/browser/service_worker/link_header_support_unittest.cc:357:
CreateDocumentProviderHost(false);
nit: slightly prefer making a CreateInsecureDocumentProviderHost function
instead of using the default parameter. Otherwise, add /* is_parent_frame_secure
*/ comment.

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

https://codereview.chromium.org/2638313002/diff/260001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:175:
true);
Let's document /* is_parent_frame_secure */ or reintroduce the enum. Sorry for
this mess, I originally did both boolean and enum and now I'm seeing it's too
confusing.

[shimazu, 2017-02-13 07:04:11]

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

[commit-bot: I haz the power, 2017-02-13 07:04:31]

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

[horo, 2017-02-13 07:32:38]

lgtm

[shimazu, 2017-02-13 08:20:17]

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

[shimazu, 2017-02-13 08:20:19]

dcheng@chromium.org: Please review changes in _messages.h, mojom and
struct_traits.

kinuko@chromium.org: Please review changes in BUILD.gn (and scan other things if
possible)

https://codereview.chromium.org/2638313002/diff/220001/content/common/service...
File content/common/service_worker/service_worker_provider.h (right):

https://codereview.chromium.org/2638313002/diff/220001/content/common/service...
content/common/service_worker/service_worker_provider.h:12: struct
CONTENT_EXPORT ServiceWorkerProviderHostInfo {
On 2017/02/13 05:59:23, falken wrote:
> On 2017/02/13 03:25:56, shimazu wrote:
> > On 2017/02/09 01:03:19, falken wrote:
> > > nit: If this is the only thing meant for this file, should it just be
named
> > > "service_worker_provider_host_info.h" instead?
> > 
> > I'm planning to add ServiceWorkerProviderClientInfo to construct
> > ServiceWorkerProviderContext on later patch.
> > (SWProviderHostInfo in https://crrev.com/2627413002 will be
> SWProviderClientInfo
> > later.)
> 
> If SWProviderHostInfo will become SWProviderClientInfo, then it sounds like
this
> file should be named "service_worker_provider_client_info.h" at that time. But
> in this patch I think it we should just name it
> "service_worker_provider_host_info.h" and then rename it when its content
> changes.

That sounds more reasonable. Renamed:)

https://codereview.chromium.org/2638313002/diff/260001/content/browser/servic...
File content/browser/service_worker/embedded_worker_registry.cc (right):

https://codereview.chromium.org/2638313002/diff/260001/content/browser/servic...
content/browser/service_worker/embedded_worker_registry.cc:194: // browser
process somehow.
On 2017/02/13 05:59:24, falken wrote:
> I like how the existing comment explains why OnDetached is used. My previous
> confusion is nothing mentioned that RemoveProcess should be called after the
> workers have stopped.
> 
> I'd keep the previous comment and just add to the beginning that
"RemoveProcess
> is typically called after the running workers on the process have been
stopped,
> so if there is a running worker at this point somehow the worker thread has
lost
> contact with the browser process."

Done.

https://codereview.chromium.org/2638313002/diff/260001/content/browser/servic...
File content/browser/service_worker/embedded_worker_registry.h (right):

https://codereview.chromium.org/2638313002/diff/260001/content/browser/servic...
content/browser/service_worker/embedded_worker_registry.h:91: // |worker_map_|.
On 2017/02/13 05:59:24, falken wrote:
> IMO it's best to avoid referencing private members in the documentation for a
> public function.
> 
> This API may be confusing because there is no corresponding AddProcess or
> AddWorker function. The reader wonders where these workers are coming from.
> 
> My suggestion: "Removes information about the service workers running on the
> process and calls ServiceWorkerVersion::OnDetached() on each. Called when the
> process is terminated. Under normal operation, the workers should already have
> been stopped before the process is terminated, in which case this function
does
> nothing. But in some cases the process can be terminated unexpectedly or the
> workers can fail to stop cleanly."  

Thanks, it looks good!
Updated.

https://codereview.chromium.org/2638313002/diff/260001/content/browser/servic...
File content/browser/service_worker/link_header_support_unittest.cc (right):

https://codereview.chromium.org/2638313002/diff/260001/content/browser/servic...
content/browser/service_worker/link_header_support_unittest.cc:357:
CreateDocumentProviderHost(false);
On 2017/02/13 05:59:24, falken wrote:
> nit: slightly prefer making a CreateInsecureDocumentProviderHost function
> instead of using the default parameter. Otherwise, add /*
is_parent_frame_secure
> */ comment.

Thanks, I've added CreateInsecureDocumentProviderHost instead of the default
parameter.

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

https://codereview.chromium.org/2638313002/diff/260001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:175:
true);
On 2017/02/13 05:59:24, falken wrote:
> Let's document /* is_parent_frame_secure */ or reintroduce the enum. Sorry for
> this mess, I originally did both boolean and enum and now I'm seeing it's too
> confusing.

Oops, I forgot to add the note, thanks:)

[kinuko, 2017-02-14 02:25:38]

lgtm

https://codereview.chromium.org/2638313002/diff/300001/content/child/service_...
File content/child/service_worker/service_worker_network_provider.cc (right):

https://codereview.chromium.org/2638313002/diff/300001/content/child/service_...
content/child/service_worker/service_worker_network_provider.cc:130:
ServiceWorkerProviderHostInfo host_info(provider_id_, route_id, provider_type,
nit: I feel it might look slightly more consistent to call it 'provider_info' or
rename the mojo call to CreateProviderHost (so it becomes clearer 'we created
provider and send its info' or 'we want to create a provider host and send its
ctor info'), but up to you

[shimazu, 2017-02-14 07:58:14]

https://codereview.chromium.org/2638313002/diff/300001/content/child/service_...
File content/child/service_worker/service_worker_network_provider.cc (right):

https://codereview.chromium.org/2638313002/diff/300001/content/child/service_...
content/child/service_worker/service_worker_network_provider.cc:130:
ServiceWorkerProviderHostInfo host_info(provider_id_, route_id, provider_type,
On 2017/02/14 02:25:38, kinuko wrote:
> nit: I feel it might look slightly more consistent to call it 'provider_info'
or
> rename the mojo call to CreateProviderHost (so it becomes clearer 'we created
> provider and send its info' or 'we want to create a provider host and send its
> ctor info'), but up to you

Sounds good. I've done the former, thanks!

[dcheng, 2017-02-14 08:47:46]

Overall, looks good. Just a few questions (not all IPC related, sorry!) and a
few nits about making sure we're including all the headers we need.

https://codereview.chromium.org/2638313002/diff/300001/content/browser/servic...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/2638313002/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_context_core.cc:247:
dispatcher_hosts_(old_context->dispatcher_hosts_.release()),
Nit: std::move instead of release()

https://codereview.chromium.org/2638313002/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_context_core.cc:279:
dispatcher_hosts_->AddWithID(dispatcher_host, process_id);
I wonder: do we need to use IDMap if we always have a pre-set ID? Would it make
sense to just use std::map? Then we don't even need the unique_ptr -- we could
just std::move() the map itself.

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

https://codereview.chromium.org/2638313002/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:94: auto host =
base::WrapUnique(new ServiceWorkerProviderHost(
In general, base::WrapUnique(new T(...)) should be written auto x =
base::MakeUnique<T>(...);

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

https://codereview.chromium.org/2638313002/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host_unittest.cc:75:
std::unique_ptr<ServiceWorkerProviderHost> host =
#include <memory> for unique_ptr

https://codereview.chromium.org/2638313002/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host_unittest.cc:81:
context_->AddProviderHost(std::move(host));
#include <utility> for std::move

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

https://codereview.chromium.org/2638313002/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_test_utils.cc:19: return
ServiceWorkerProviderHost::Create(process_id, std::move(info), context,
Nit: #include <utility> for std::move

Also, std::move(context) since we're passing WeakPtr by value.

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

https://codereview.chromium.org/2638313002/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_test_utils.h:46:
std::unique_ptr<ServiceWorkerProviderHost> CreateProviderHostForWindow(
#include <memory> for unique_ptr

https://codereview.chromium.org/2638313002/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_test_utils.h:50:
base::WeakPtr<ServiceWorkerContextCore> context);
#include "base/memory/weak_ptr.h" for WeakPtr

https://codereview.chromium.org/2638313002/diff/300001/content/browser/servic...
File
content/browser/service_worker/service_worker_write_to_cache_job_unittest.cc
(left):

https://codereview.chromium.org/2638313002/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_write_to_cache_job_unittest.cc:297:
ServiceWorkerProviderHost::FrameSecurityLevel::SECURE,
To be honest, I preferred the enum over the bool here. =)

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

https://codereview.chromium.org/2638313002/diff/300001/content/common/service...
content/common/service_worker/service_worker_provider_host_info.h:14:
ServiceWorkerProviderHostInfo(ServiceWorkerProviderHostInfo&& other);
Is it necessary to make this move-only? The struct itself is pretty small it
seems, and moving is basically going to copy anyway.

https://codereview.chromium.org/2638313002/diff/300001/content/common/service...
content/common/service_worker/service_worker_provider_host_info.h:45: bool
is_parent_frame_secure;
Out of curiosity, is this something we can eventually remove? In theory, we
should be able to determine this information in the browser.

https://codereview.chromium.org/2638313002/diff/300001/content/common/service...
File content/common/service_worker/service_worker_provider_struct_traits.cc
(right):

https://codereview.chromium.org/2638313002/diff/300001/content/common/service...
content/common/service_worker/service_worker_provider_struct_traits.cc:19:
out->type = type;
Nit: it'd be OK to just ReadType() directly into the data member, to avoid a
stack temporary.

[shimazu, 2017-02-15 02:24:19]

Updated the patch, thanks for reviewing!
PTAL again.

falken@: Could you respond to the following comment? I'm not familiar with the
context when it's implemented.
https://codereview.chromium.org/2638313002/diff/300001/content/common/service...

https://codereview.chromium.org/2638313002/diff/300001/content/browser/servic...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/2638313002/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_context_core.cc:247:
dispatcher_hosts_(old_context->dispatcher_hosts_.release()),
On 2017/02/14 08:47:46, dcheng wrote:
> Nit: std::move instead of release()

Done.

https://codereview.chromium.org/2638313002/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_context_core.cc:279:
dispatcher_hosts_->AddWithID(dispatcher_host, process_id);
On 2017/02/14 08:47:46, dcheng wrote:
> I wonder: do we need to use IDMap if we always have a pre-set ID? Would it
make
> sense to just use std::map? Then we don't even need the unique_ptr -- we could
> just std::move() the map itself.

I assume it's just historical thing. Updated.
I'll create another patch to fix the remainings, thanks:)

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

https://codereview.chromium.org/2638313002/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:94: auto host =
base::WrapUnique(new ServiceWorkerProviderHost(
On 2017/02/14 08:47:46, dcheng wrote:
> In general, base::WrapUnique(new T(...)) should be written auto x =
> base::MakeUnique<T>(...);

That's because the constructor is private:
https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-a...

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

https://codereview.chromium.org/2638313002/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host_unittest.cc:75:
std::unique_ptr<ServiceWorkerProviderHost> host =
On 2017/02/14 08:47:46, dcheng wrote:
> #include <memory> for unique_ptr

Done.

https://codereview.chromium.org/2638313002/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host_unittest.cc:81:
context_->AddProviderHost(std::move(host));
On 2017/02/14 08:47:46, dcheng wrote:
> #include <utility> for std::move

Done.

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

https://codereview.chromium.org/2638313002/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_test_utils.cc:19: return
ServiceWorkerProviderHost::Create(process_id, std::move(info), context,
On 2017/02/14 08:47:46, dcheng wrote:
> Nit: #include <utility> for std::move
> 
> Also, std::move(context) since we're passing WeakPtr by value.

Done.

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

https://codereview.chromium.org/2638313002/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_test_utils.h:46:
std::unique_ptr<ServiceWorkerProviderHost> CreateProviderHostForWindow(
On 2017/02/14 08:47:46, dcheng wrote:
> #include <memory> for unique_ptr

Done.

https://codereview.chromium.org/2638313002/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_test_utils.h:50:
base::WeakPtr<ServiceWorkerContextCore> context);
On 2017/02/14 08:47:46, dcheng wrote:
> #include "base/memory/weak_ptr.h" for WeakPtr

Done.

https://codereview.chromium.org/2638313002/diff/300001/content/browser/servic...
File
content/browser/service_worker/service_worker_write_to_cache_job_unittest.cc
(left):

https://codereview.chromium.org/2638313002/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_write_to_cache_job_unittest.cc:297:
ServiceWorkerProviderHost::FrameSecurityLevel::SECURE,
On 2017/02/14 08:47:46, dcheng wrote:
> To be honest, I preferred the enum over the bool here. =)

I also feel so to tell the truth.
Is it okay to separate the patch to remove the booleans thoroughly?

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

https://codereview.chromium.org/2638313002/diff/300001/content/common/service...
content/common/service_worker/service_worker_provider_host_info.h:14:
ServiceWorkerProviderHostInfo(ServiceWorkerProviderHostInfo&& other);
On 2017/02/14 08:47:46, dcheng wrote:
> Is it necessary to make this move-only? The struct itself is pretty small it
> seems, and moving is basically going to copy anyway.

I'm planning to add AssociatedInterfacePtr in this struct on a later patch ( WIP
patch: https://crrev.com/2653493009), so this is not actually necessary to be
move-only. 
Sorry for confusing.
Would it be better to allow copying here and make this move-only on the later
patch?

https://codereview.chromium.org/2638313002/diff/300001/content/common/service...
content/common/service_worker/service_worker_provider_host_info.h:45: bool
is_parent_frame_secure;
On 2017/02/14 08:47:46, dcheng wrote:
> Out of curiosity, is this something we can eventually remove? In theory, we
> should be able to determine this information in the browser.

falken: do you know why? IIRC falken implemented that.

https://codereview.chromium.org/2638313002/diff/300001/content/common/service...
File content/common/service_worker/service_worker_provider_struct_traits.cc
(right):

https://codereview.chromium.org/2638313002/diff/300001/content/common/service...
content/common/service_worker/service_worker_provider_struct_traits.cc:19:
out->type = type;
On 2017/02/14 08:47:46, dcheng wrote:
> Nit: it'd be OK to just ReadType() directly into the data member, to avoid a
> stack temporary.

Done.

[shimazu, 2017-02-15 02:24:45]

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

[commit-bot: I haz the power, 2017-02-15 02:26:13]

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

[commit-bot: I haz the power, 2017-02-15 02:31:11]

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

[commit-bot: I haz the power, 2017-02-15 02:31:12]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  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-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  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_...)

[dcheng, 2017-02-15 04:29:35]

mojo lgtm

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

https://codereview.chromium.org/2638313002/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:94: auto host =
base::WrapUnique(new ServiceWorkerProviderHost(
On 2017/02/15 02:24:18, shimazu wrote:
> On 2017/02/14 08:47:46, dcheng wrote:
> > In general, base::WrapUnique(new T(...)) should be written auto x =
> > base::MakeUnique<T>(...);
> 
> That's because the constructor is private:
>
https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-a...

Ah oops. Thanks for pointing that out.

https://codereview.chromium.org/2638313002/diff/300001/content/browser/servic...
File
content/browser/service_worker/service_worker_write_to_cache_job_unittest.cc
(left):

https://codereview.chromium.org/2638313002/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_write_to_cache_job_unittest.cc:297:
ServiceWorkerProviderHost::FrameSecurityLevel::SECURE,
On 2017/02/15 02:24:19, shimazu wrote:
> On 2017/02/14 08:47:46, dcheng wrote:
> > To be honest, I preferred the enum over the bool here. =)
> 
> I also feel so to tell the truth.
> Is it okay to separate the patch to remove the booleans thoroughly?

Sounds good to me.

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

https://codereview.chromium.org/2638313002/diff/300001/content/common/service...
content/common/service_worker/service_worker_provider_host_info.h:14:
ServiceWorkerProviderHostInfo(ServiceWorkerProviderHostInfo&& other);
On 2017/02/15 02:24:19, shimazu wrote:
> On 2017/02/14 08:47:46, dcheng wrote:
> > Is it necessary to make this move-only? The struct itself is pretty small it
> > seems, and moving is basically going to copy anyway.
> 
> I'm planning to add AssociatedInterfacePtr in this struct on a later patch (
WIP
> patch: https://crrev.com/2653493009), so this is not actually necessary to be
> move-only. 
> Sorry for confusing.
> Would it be better to allow copying here and make this move-only on the later
> patch? 

Ah, OK. Let's just leave it as-is to avoid churn then.

[shimazu, 2017-02-15 06:19:39]

The CQ bit was checked by shimazu@chromium.org

[shimazu, 2017-02-15 06:19:39]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
horo@chromium.org, kinuko@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2638313002/#ps360001
(title: "rebase")

[commit-bot: I haz the power, 2017-02-15 06:20:04]

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-15 07:25:56]

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

[commit-bot: I haz the power, 2017-02-15 07:25:58]

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-02-16 00:28:33]

The CQ bit was checked by shimazu@chromium.org

[commit-bot: I haz the power, 2017-02-16 00:29: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, 2017-02-16 02:34:46]

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

[commit-bot: I haz the power, 2017-02-16 02:34:48]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[shimazu, 2017-02-16 09:03:42]

The CQ bit was checked by shimazu@chromium.org

[commit-bot: I haz the power, 2017-02-16 09:04:43]

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-16 10:48:05]

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

[commit-bot: I haz the power, 2017-02-16 10:48:08]

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

[shimazu, 2017-02-20 07:48:21]

The CQ bit was checked by shimazu@chromium.org

[commit-bot: I haz the power, 2017-02-20 07:48:46]

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-20 07:50:54]

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

[commit-bot: I haz the power, 2017-02-20 07:50:56]

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-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[shimazu, 2017-02-21 03:55:51]

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

[shimazu, 2017-02-21 03:55:59]

The CQ bit was unchecked by shimazu@chromium.org

[shimazu, 2017-02-21 03:56:07]

The CQ bit was checked by shimazu@chromium.org

[shimazu, 2017-02-21 03:56:08]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
kinuko@chromium.org, dcheng@chromium.org, horo@chromium.org
Link to the patchset: https://codereview.chromium.org/2638313002/#ps380001
(title: "Rebase")

[commit-bot: I haz the power, 2017-02-21 03:56:23]

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-21 05:30:11]

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

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

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

[shimazu, 2017-02-21 12:16:48]

The CQ bit was checked by shimazu@chromium.org

[commit-bot: I haz the power, 2017-02-21 12:16:58]

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-21 13:03:36]

CQ is committing da patch.

Bot data: {"patchset_id": 380001, "attempt_start_ts": 1487679408419260,
"parent_rev": "c183eaa3876b03b16c2c84e3dfb3b87b63071a5b", "commit_rev":
"3b85dfb581ef764c8a02dbd803f54d0bbab1f318"}

[commit-bot: I haz the power, 2017-02-21 13:04:10]

Description was changed from

==========
Manage ServiceWorkerDispatcherHost in ServiceWorkerContextCore

ServiceWorkerProviders for the service workers currently have the mesasge
ordering issue between ProviderDestroyed and WorkerStopped. Associating the
message pipes correctly will fix that. I'm planning to bind the
ServiceWorkerProvider interfaces with the channel's pipe for pages and with
the EmbeddedWorkerInstanceClient's pipe for service workers.

This is the first patch to introduce the associated provider hosts.
This patch
 - adds ServiceWorkerProviderHostInfo to send the params to SWDispatcherHost
 - manages SWDispatcherHost in SWContextCore
 - creates SWProviderHost by SWProviderHostInfo
 - removes three legacy IPC messages
   - ServiceWorkerHostMsg_ProviderCreated
   - ServiceWorkerHostMsg_ProviderDestroyed
   - ServiceWorkerHostMsg_SetVersionId

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

to

==========
Manage ServiceWorkerDispatcherHost in ServiceWorkerContextCore

ServiceWorkerProviders for the service workers currently have the mesasge
ordering issue between ProviderDestroyed and WorkerStopped. Associating the
message pipes correctly will fix that. I'm planning to bind the
ServiceWorkerProvider interfaces with the channel's pipe for pages and with
the EmbeddedWorkerInstanceClient's pipe for service workers.

This is the first patch to introduce the associated provider hosts.
This patch
 - adds ServiceWorkerProviderHostInfo to send the params to SWDispatcherHost
 - manages SWDispatcherHost in SWContextCore
 - creates SWProviderHost by SWProviderHostInfo
 - removes three legacy IPC messages
   - ServiceWorkerHostMsg_ProviderCreated
   - ServiceWorkerHostMsg_ProviderDestroyed
   - ServiceWorkerHostMsg_SetVersionId

BUG=668633, 676983, 629701

Review-Url: https://codereview.chromium.org/2638313002
Cr-Commit-Position: refs/heads/master@{#451745}
Committed:
https://chromium.googlesource.com/chromium/src/+/3b85dfb581ef764c8a02dbd803f5...
==========

[commit-bot: I haz the power, 2017-02-21 13:04:12]

Committed patchset #20 (id:380001) as
https://chromium.googlesource.com/chromium/src/+/3b85dfb581ef764c8a02dbd803f5...