Create ServiceWorkerProviderHost before starting worker

Create ServiceWorkerProviderHost before starting worker

Currently ServiceWorkerProviderHost for a service worker's context is bound with
the corresponing provider on the hosted worker when ProviderCreated and
SetHostedVersion messages are getting back after sending the StartWorker
message. This patch is to removes the round trip.
This patch changes the procedure of creating the SWProviderHost like the following.

1: Create a mock provider host when ServiceWorkerVersion::StartWorker is called.
2: Complete the rest of initialization for the provider host before sending
StartWorker message at ServiceWorkerProviderHost::CompleteStartWorkerPreparation.
3: Send the infomation about the provider host to the renderer as an argument of
EmbeddedWorkerInstanceClient::StartWorker.
4: Create ServiceWorkerNetworkProvider on the renderer when
ServiceWorkerContextClient is constructed.

This patch also sovles a weird ordering issue between ProviderDestroyed and
SetHostedVersion, and between OnStopped and ProviderDestroyed.

BUG=629701, 676983, 668633
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2779763004
Cr-Commit-Position: refs/heads/master@{#481391}
Committed: https://chromium.googlesource.com/chromium/src/+/1ebf3cb1014b2c85d4520e6dcb6a11d7001bc30f

[shimazu, 2017-03-28 08:01:33]

Description was changed from

==========
[WIP] Create ServiceWorkerProviderHost before starting worker

Currently ProviderHost for the worker's context is bound with the hosting worker
after StartWorker message is sent to the rednerer, and ProviderCreated and
SetHostedVersion messages are getting back. This patch removes these round trips
like the following steps.

1: Create a mock provider host when ServiceWorkerVersion::StartWorker is called.
2: Set the parameters of the provider host before sending StartWorker message to
the renderer.
3: Send the infomation about the provider host to the renderer.
4: Create ServiceWorkerNetworkProvider on the renderer when
ServiceWorkerContextClient is constructed.

This patch also sovles a weird ordering issue between ProviderDestroyed and
SetHostedVersion.

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

to

==========
[WIP] Create ServiceWorkerProviderHost before starting worker

Currently ProviderHost for the worker's context is bound with the hosting worker
after StartWorker message is sent to the rednerer, and ProviderCreated and
SetHostedVersion messages are getting back. This patch removes these round trips
like the following steps.

1: Create a mock provider host when ServiceWorkerVersion::StartWorker is called.
2: Set the parameters of the provider host before sending StartWorker message to
the renderer.
3: Send the infomation about the provider host to the renderer.
4: Create ServiceWorkerNetworkProvider on the renderer when
ServiceWorkerContextClient is constructed.

This patch also sovles a weird ordering issue between ProviderDestroyed and
SetHostedVersion.

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

[shimazu, 2017-05-16 09:31:34]

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

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-16 09:33:30]

The CQ bit was unchecked by shimazu@chromium.org

[shimazu, 2017-05-16 09:33:59]

Patchset #4 (id:60001) has been deleted

[shimazu, 2017-05-16 09:35:46]

The CQ bit was checked by shimazu@chromium.org

[shimazu, 2017-05-16 09:35:50]

The CQ bit was unchecked by shimazu@chromium.org

[shimazu, 2017-05-16 09:35:58]

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

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

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

This CL has an open dependency (Issue 2653493009 Patch 260001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[shimazu, 2017-05-17 05:24:42]

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

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

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

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

[commit-bot: I haz the power, 2017-05-17 05:31:37]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[shimazu, 2017-05-17 05:33:08]

Description was changed from

==========
[WIP] Create ServiceWorkerProviderHost before starting worker

Currently ProviderHost for the worker's context is bound with the hosting worker
after StartWorker message is sent to the rednerer, and ProviderCreated and
SetHostedVersion messages are getting back. This patch removes these round trips
like the following steps.

1: Create a mock provider host when ServiceWorkerVersion::StartWorker is called.
2: Set the parameters of the provider host before sending StartWorker message to
the renderer.
3: Send the infomation about the provider host to the renderer.
4: Create ServiceWorkerNetworkProvider on the renderer when
ServiceWorkerContextClient is constructed.

This patch also sovles a weird ordering issue between ProviderDestroyed and
SetHostedVersion.

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

to

==========
Create ServiceWorkerProviderHost before starting worker

Currently ServiceWorkerProviderHost for a service worker's context is bound with
the corresponing provider on the hosted worker when ProviderCreated and
SetHostedVersion messages are getting back after sending the StartWorker
message. This patch is to removes the round trip.
This patch changes the procedure of creating the SWProviderHost like the
following.

1: Create a mock provider host when ServiceWorkerVersion::StartWorker is called.
2: Set the parameters of the provider host before sending StartWorker message to
the renderer (ServiceWorkerProviderHost::CompleteStartWorkerPreparation).
3: Send the infomation about the provider host to the renderer as an argument of
EmbeddedWorkerInstanceClient::StartWorker.
4: Create ServiceWorkerNetworkProvider on the renderer when
ServiceWorkerContextClient is constructed.

This patch also sovles a weird ordering issue between ProviderDestroyed and
SetHostedVersion, and between OnStopped and ProviderDestroyed.

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

[shimazu, 2017-05-17 05:34:28]

Description was changed from

==========
Create ServiceWorkerProviderHost before starting worker

Currently ServiceWorkerProviderHost for a service worker's context is bound with
the corresponing provider on the hosted worker when ProviderCreated and
SetHostedVersion messages are getting back after sending the StartWorker
message. This patch is to removes the round trip.
This patch changes the procedure of creating the SWProviderHost like the
following.

1: Create a mock provider host when ServiceWorkerVersion::StartWorker is called.
2: Set the parameters of the provider host before sending StartWorker message to
the renderer (ServiceWorkerProviderHost::CompleteStartWorkerPreparation).
3: Send the infomation about the provider host to the renderer as an argument of
EmbeddedWorkerInstanceClient::StartWorker.
4: Create ServiceWorkerNetworkProvider on the renderer when
ServiceWorkerContextClient is constructed.

This patch also sovles a weird ordering issue between ProviderDestroyed and
SetHostedVersion, and between OnStopped and ProviderDestroyed.

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

to

==========
Create ServiceWorkerProviderHost before starting worker

Currently ServiceWorkerProviderHost for a service worker's context is bound with
the corresponing provider on the hosted worker when ProviderCreated and
SetHostedVersion messages are getting back after sending the StartWorker
message. This patch is to removes the round trip.
This patch changes the procedure of creating the SWProviderHost like the
following.

1: Create a mock provider host when ServiceWorkerVersion::StartWorker is called.
2: Complete the rest of initialization for the provider host before sending
StartWorker message at
ServiceWorkerProviderHost::CompleteStartWorkerPreparation.
3: Send the infomation about the provider host to the renderer as an argument of
EmbeddedWorkerInstanceClient::StartWorker.
4: Create ServiceWorkerNetworkProvider on the renderer when
ServiceWorkerContextClient is constructed.

This patch also sovles a weird ordering issue between ProviderDestroyed and
SetHostedVersion, and between OnStopped and ProviderDestroyed.

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

[shimazu, 2017-05-17 08:33:34]

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

[shimazu, 2017-05-17 08:33:34]

PTAL, thanks!

[shimazu, 2017-05-31 06:47:21]

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

[commit-bot: I haz the power, 2017-05-31 06:47: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-31 06:52:30]

Finally the patch which this depends on has been landed:
https://crrev.com/2653493009 :)
Could you start reviewing this CL?

[commit-bot: I haz the power, 2017-05-31 07:23:30]

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

[commit-bot: I haz the power, 2017-05-31 07:23:31]

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-06-01 08:12:35]

One round of comments

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

https://codereview.chromium.org/2779763004/diff/130001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:197:
CHECK(render_process_id == ChildProcessHost::kInvalidUniqueID);
CHECK_EQ

https://codereview.chromium.org/2779763004/diff/130001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:198: // Actual
thread id is set when the service worker context gets started.
this comment be also mention render proc id and be moved a line up.

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

https://codereview.chromium.org/2779763004/diff/130001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:68: //
ServiceWorkerNetworkProvider is created on the renderer.
This comment seems to need updating now.

https://codereview.chromium.org/2779763004/diff/130001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:274: // context.
It's pretty lame but this class historically calls it a provider host for a
"controller" or a provider host that's "hosting an running service worker".
E.g.,
IsHostToRunningServiceWorker. Let's be consistent with
SERVICE_WORKER_PROVIDER_FOR_CONTROLLER and call it "provider hosts for
controllers".

https://codereview.chromium.org/2779763004/diff/130001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:384: // for
providers for service worker contexts.
ditto

https://codereview.chromium.org/2779763004/diff/130001/content/browser/servic...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/2779763004/diff/130001/content/browser/servic...
content/browser/service_worker/service_worker_version.h:306:
ServiceWorkerProviderHost* running_controller() {
// The provider host hosting this version. Only valid while the version is
running.
provider_host()

https://codereview.chromium.org/2779763004/diff/130001/content/browser/servic...
content/browser/service_worker/service_worker_version.h:734: // be passed to the
context.
does "context" here mean ServiceWorkerContextCore? It reads like the "worker's
context" mentioned above.

https://codereview.chromium.org/2779763004/diff/130001/content/browser/servic...
content/browser/service_worker/service_worker_version.h:738:
base::WeakPtr<ServiceWorkerProviderHost> running_controller_provider_;
How about just pending_provider_host_ and provider_host_

[xiaofengzhang, 2017-06-06 03:22:22]

xiaofeng.zhang@intel.com changed reviewers:
+ xiaofeng.zhang@intel.com

[xiaofengzhang, 2017-06-06 03:22:23]

https://codereview.chromium.org/2779763004/diff/130001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2779763004/diff/130001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:1096:
std::move(pending_network_provider_));
I have a stupid question here:
CreateServiceWorkerNetworkProvider should be called in
WebEmbeddedWorkerImpl::DidFinishDocumentLoad which seems before the
StartWorker()?
But ServiceWorkerContextClient is constructed in StartWorker(), and
ServiceWorkerNetworkProvider is created in the constructor of
ServiceWorkerContextClient?

[shimazu, 2017-06-06 04:16:56]

Let me reply the question while I'm still fixing the failing unittests. 

==
Here is the progress of the fixes.

* BackgroundFetchServiceTest.Abort
  https://chromium-review.googlesource.com/c/523822/
* LinkHeaderServiceWorkerTest.InstallServiceWorker_FromWorkerWithoutControllees
  https://chromium-review.googlesource.com/c/523683/
* BackgroundSyncManagerTest.RegisterWithoutLiveSWRegistration
  Fixed on this CL.

These two tests are still under investigation.
* PaymentAppProviderTest.InvokePaymentAppTest
*
ForeignFetchRequestHandlerTest.InitializeHandler_TimeoutBehaviorForServiceWorker

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

https://codereview.chromium.org/2779763004/diff/130001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:197:
CHECK(render_process_id == ChildProcessHost::kInvalidUniqueID);
On 2017/06/01 08:12:34, falken wrote:
> CHECK_EQ

Done.

https://codereview.chromium.org/2779763004/diff/130001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:198: // Actual
thread id is set when the service worker context gets started.
On 2017/06/01 08:12:34, falken wrote:
> this comment be also mention render proc id and be moved a line up.

Done.

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

https://codereview.chromium.org/2779763004/diff/130001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:68: //
ServiceWorkerNetworkProvider is created on the renderer.
On 2017/06/01 08:12:34, falken wrote:
> This comment seems to need updating now.

Done.

https://codereview.chromium.org/2779763004/diff/130001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:274: // context.
On 2017/06/01 08:12:34, falken wrote:
> It's pretty lame but this class historically calls it a provider host for a
> "controller" or a provider host that's "hosting an running service worker".
> E.g.,
> IsHostToRunningServiceWorker. Let's be consistent with
> SERVICE_WORKER_PROVIDER_FOR_CONTROLLER and call it "provider hosts for
> controllers".

Done.

https://codereview.chromium.org/2779763004/diff/130001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:384: // for
providers for service worker contexts.
On 2017/06/01 08:12:34, falken wrote:
> ditto

Done.

https://codereview.chromium.org/2779763004/diff/130001/content/browser/servic...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/2779763004/diff/130001/content/browser/servic...
content/browser/service_worker/service_worker_version.h:306:
ServiceWorkerProviderHost* running_controller() {
On 2017/06/01 08:12:35, falken wrote:
> // The provider host hosting this version. Only valid while the version is
> running.
> provider_host()

Done.

https://codereview.chromium.org/2779763004/diff/130001/content/browser/servic...
content/browser/service_worker/service_worker_version.h:734: // be passed to the
context.
On 2017/06/01 08:12:34, falken wrote:
> does "context" here mean ServiceWorkerContextCore? It reads like the "worker's
> context" mentioned above.

Done.

https://codereview.chromium.org/2779763004/diff/130001/content/browser/servic...
content/browser/service_worker/service_worker_version.h:738:
base::WeakPtr<ServiceWorkerProviderHost> running_controller_provider_;
On 2017/06/01 08:12:35, falken wrote:
> How about just pending_provider_host_ and provider_host_

Done.

https://codereview.chromium.org/2779763004/diff/130001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2779763004/diff/130001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:1096:
std::move(pending_network_provider_));
On 2017/06/06 03:22:23, xiaofengzhang wrote:
> I have a stupid question here:
> CreateServiceWorkerNetworkProvider should be called in
> WebEmbeddedWorkerImpl::DidFinishDocumentLoad which seems before the
> StartWorker()?
> But ServiceWorkerContextClient is constructed in StartWorker(), and
> ServiceWorkerNetworkProvider is created in the constructor of
> ServiceWorkerContextClient?
> 

It might be a complicated part of starting up. 
EmbeddedWorkerClientImpl::StartWorker creates and starts WebEmbeddedWorkerImpl,
and WebEmbeddedWorkerImpl internally prepares a frame called a "shadow page".
DidFinishDocumentLoad is called when WebEmbeddedWorkerImpl::LoadShadowPage is
finished, and it means the shadow page gets ready. The shadow page is used for
loading the worker's script; it's main_script_loader_->LoadAsynchronously(...)
on this link:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebEmbedde...
.

[xiaofengzhang, 2017-06-06 05:19:45]

On 2017/06/06 04:16:56, shimazu wrote:
> Let me reply the question while I'm still fixing the failing unittests. 
> 
> ==
> Here is the progress of the fixes.
> 
> * BackgroundFetchServiceTest.Abort
>   https://chromium-review.googlesource.com/c/523822/
> *
LinkHeaderServiceWorkerTest.InstallServiceWorker_FromWorkerWithoutControllees
>   https://chromium-review.googlesource.com/c/523683/
> * BackgroundSyncManagerTest.RegisterWithoutLiveSWRegistration
>   Fixed on this CL.
> 
> These two tests are still under investigation.
> * PaymentAppProviderTest.InvokePaymentAppTest
> *
>
ForeignFetchRequestHandlerTest.InitializeHandler_TimeoutBehaviorForServiceWorker
> 
>
https://codereview.chromium.org/2779763004/diff/130001/content/browser/servic...
> File content/browser/service_worker/service_worker_provider_host.cc (right):
> 
>
https://codereview.chromium.org/2779763004/diff/130001/content/browser/servic...
> content/browser/service_worker/service_worker_provider_host.cc:197:
> CHECK(render_process_id == ChildProcessHost::kInvalidUniqueID);
> On 2017/06/01 08:12:34, falken wrote:
> > CHECK_EQ
> 
> Done.
> 
>
https://codereview.chromium.org/2779763004/diff/130001/content/browser/servic...
> content/browser/service_worker/service_worker_provider_host.cc:198: // Actual
> thread id is set when the service worker context gets started.
> On 2017/06/01 08:12:34, falken wrote:
> > this comment be also mention render proc id and be moved a line up.
> 
> Done.
> 
>
https://codereview.chromium.org/2779763004/diff/130001/content/browser/servic...
> File content/browser/service_worker/service_worker_provider_host.h (right):
> 
>
https://codereview.chromium.org/2779763004/diff/130001/content/browser/servic...
> content/browser/service_worker/service_worker_provider_host.h:68: //
> ServiceWorkerNetworkProvider is created on the renderer.
> On 2017/06/01 08:12:34, falken wrote:
> > This comment seems to need updating now.
> 
> Done.
> 
>
https://codereview.chromium.org/2779763004/diff/130001/content/browser/servic...
> content/browser/service_worker/service_worker_provider_host.h:274: // context.
> On 2017/06/01 08:12:34, falken wrote:
> > It's pretty lame but this class historically calls it a provider host for a
> > "controller" or a provider host that's "hosting an running service worker".
> > E.g.,
> > IsHostToRunningServiceWorker. Let's be consistent with
> > SERVICE_WORKER_PROVIDER_FOR_CONTROLLER and call it "provider hosts for
> > controllers".
> 
> Done.
> 
>
https://codereview.chromium.org/2779763004/diff/130001/content/browser/servic...
> content/browser/service_worker/service_worker_provider_host.h:384: // for
> providers for service worker contexts.
> On 2017/06/01 08:12:34, falken wrote:
> > ditto
> 
> Done.
> 
>
https://codereview.chromium.org/2779763004/diff/130001/content/browser/servic...
> File content/browser/service_worker/service_worker_version.h (right):
> 
>
https://codereview.chromium.org/2779763004/diff/130001/content/browser/servic...
> content/browser/service_worker/service_worker_version.h:306:
> ServiceWorkerProviderHost* running_controller() {
> On 2017/06/01 08:12:35, falken wrote:
> > // The provider host hosting this version. Only valid while the version is
> > running.
> > provider_host()
> 
> Done.
> 
>
https://codereview.chromium.org/2779763004/diff/130001/content/browser/servic...
> content/browser/service_worker/service_worker_version.h:734: // be passed to
the
> context.
> On 2017/06/01 08:12:34, falken wrote:
> > does "context" here mean ServiceWorkerContextCore? It reads like the
"worker's
> > context" mentioned above.
> 
> Done.
> 
>
https://codereview.chromium.org/2779763004/diff/130001/content/browser/servic...
> content/browser/service_worker/service_worker_version.h:738:
> base::WeakPtr<ServiceWorkerProviderHost> running_controller_provider_;
> On 2017/06/01 08:12:35, falken wrote:
> > How about just pending_provider_host_ and provider_host_
> 
> Done.
> 
>
https://codereview.chromium.org/2779763004/diff/130001/content/renderer/servi...
> File content/renderer/service_worker/service_worker_context_client.cc (right):
> 
>
https://codereview.chromium.org/2779763004/diff/130001/content/renderer/servi...
> content/renderer/service_worker/service_worker_context_client.cc:1096:
> std::move(pending_network_provider_));
> On 2017/06/06 03:22:23, xiaofengzhang wrote:
> > I have a stupid question here:
> > CreateServiceWorkerNetworkProvider should be called in
> > WebEmbeddedWorkerImpl::DidFinishDocumentLoad which seems before the
> > StartWorker()?
> > But ServiceWorkerContextClient is constructed in StartWorker(), and
> > ServiceWorkerNetworkProvider is created in the constructor of
> > ServiceWorkerContextClient?
> > 
> 
> It might be a complicated part of starting up. 
> EmbeddedWorkerClientImpl::StartWorker creates and starts
WebEmbeddedWorkerImpl,
> and WebEmbeddedWorkerImpl internally prepares a frame called a "shadow page".
> DidFinishDocumentLoad is called when WebEmbeddedWorkerImpl::LoadShadowPage is
> finished, and it means the shadow page gets ready. The shadow page is used for
> loading the worker's script; it's main_script_loader_->LoadAsynchronously(...)
> on this link:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebEmbedde...
> .

Very clear! I understand the related code now, thanks a lot.

[shimazu, 2017-06-06 07:21:14]

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

[commit-bot: I haz the power, 2017-06-06 07:21: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-06-06 07:31:04]

Fixed all unittests. PTAnL

[commit-bot: I haz the power, 2017-06-06 07:52:57]

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

[commit-bot: I haz the power, 2017-06-06 07:52:58]

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-06-06 14:06:34]

Another round, sorry for the iteration time. Do you see any way to split up this
CL into smaller chunks, for easier reviewing/landing? I know it's difficult
because it's mostly unit test changes. But any small chunks could help this go
more smoothly.

https://codereview.chromium.org/2779763004/diff/210001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager_unittest.cc
(right):

https://codereview.chromium.org/2779763004/diff/210001/content/browser/backgr...
content/browser/background_sync/background_sync_manager_unittest.cc:469:
ASSERT_TRUE(sw_registration_1_->HasOneRef());
Why ASSERT? Use ASSERT for fatal errors when it wouldn't make sense to continue
(we'd crash or something) if false. It looks like EXPECT is more suitable here.

https://codereview.chromium.org/2779763004/diff/210001/content/browser/paymen...
File content/browser/payments/payment_app_content_unittest_base.cc (right):

https://codereview.chromium.org/2779763004/diff/210001/content/browser/paymen...
content/browser/payments/payment_app_content_unittest_base.cc:134:
EXPECT_TRUE(registration);
ASSERT, since the other EXPECTs don't make sense if false (we'd crash)

https://codereview.chromium.org/2779763004/diff/210001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance.h (right):

https://codereview.chromium.org/2779763004/diff/210001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.h:289: // renderer side.
Not sure I understand. Does this mean the browser side should never disconnect
the pipe, and instead it's the renderer's responsibility to disconnect it?

If so, I'd suggest:
"The browser process should not disconnect the pipe, because other associated
interfaces may be using it. The renderer process will disconnect the pipe when
appropriate."

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

https://codereview.chromium.org/2779763004/diff/210001/content/browser/servic...
content/browser/service_worker/embedded_worker_test_helper.h:118: // Registers a
mojo endpoint object derived from
while you're here: Mojo.

https://codereview.chromium.org/2779763004/diff/210001/content/browser/servic...
content/browser/service_worker/embedded_worker_test_helper.h:125: // to the map.
This should be called before ServiceWorkerDispatcher::Init
ServiceWorkerDispatcherHost::Init?

https://codereview.chromium.org/2779763004/diff/210001/content/browser/servic...
File content/browser/service_worker/service_worker_context_unittest.cc (right):

https://codereview.chromium.org/2779763004/diff/210001/content/browser/servic...
content/browser/service_worker/service_worker_context_unittest.cc:234: //
reference to the ServiceWorkerVersion.
This comment is nice for understanding the change in the test, but I'm not sure
it'll be useful when reading the code for an explanation of why something you
didn't expect didn't happen.

The original test called StopWorker because no one had a reference to
ServiceWorkerVersion so it got destructed and called Stop()?

That's kind of weird, I would have thought the ServiceWorkerRegistration held a
reference to the SWVersion?

https://codereview.chromium.org/2779763004/diff/210001/content/browser/servic...
content/browser/service_worker/service_worker_context_unittest.cc:339: //
worker.
ditto: not sure this comment is worth leaving in the codebase

Why was this previously causing the worker to stop? The registration isn't
holding a reference to the version?

https://codereview.chromium.org/2779763004/diff/210001/content/browser/servic...
content/browser/service_worker/service_worker_context_unittest.cc:616: // The
provider_id is not fixed because it's unique beyond tests.
"unique beyond tests" means that other tests influence the value?

https://codereview.chromium.org/2779763004/diff/210001/content/browser/servic...
content/browser/service_worker/service_worker_context_unittest.cc:727: //
registered worker should have taken no 0.
"no 0" means "number 0"? Recommend ID 0.

https://codereview.chromium.org/2779763004/diff/210001/content/browser/servic...
content/browser/service_worker/service_worker_context_unittest.cc:772: // The
new context should take over next handle ids. No 2 should have been
s/No/Number or ID

(No sounds like the opposite of "yes")

https://codereview.chromium.org/2779763004/diff/210001/content/browser/servic...
File content/browser/service_worker/service_worker_dispatcher_host.h (right):

https://codereview.chromium.org/2779763004/diff/210001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host.h:82: 
Now that this is public let's clean it up a little.

https://codereview.chromium.org/2779763004/diff/210001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host.h:86:
ServiceWorkerRegistrationObjectInfo* info,
out_info

https://codereview.chromium.org/2779763004/diff/210001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host.h:87:
ServiceWorkerVersionAttributes* attrs);
out_attrs

https://codereview.chromium.org/2779763004/diff/210001/content/browser/servic...
File content/browser/service_worker/service_worker_job_unittest.cc (right):

https://codereview.chromium.org/2779763004/diff/210001/content/browser/servic...
content/browser/service_worker/service_worker_job_unittest.cc:50: 
// A dispatcher host that holds on to all registered ServiceWorkerHandles and
ServiceWorkerRegistrationHandles.

https://codereview.chromium.org/2779763004/diff/210001/content/browser/servic...
content/browser/service_worker/service_worker_job_unittest.cc:51: class
KeepHandleDispatcherHost : public ServiceWorkerDispatcherHost {
s/Handle/Handles

https://codereview.chromium.org/2779763004/diff/210001/content/browser/servic...
content/browser/service_worker/service_worker_job_unittest.cc:402: // context.
KeepHandleDispatcherHost will stores the handles.
s/stores/store

https://codereview.chromium.org/2779763004/diff/210001/content/browser/servic...
content/browser/service_worker/service_worker_job_unittest.cc:468: // context.
KeepHandleDispatcherHost will stores the handles.
s/stores/store

[shimazu, 2017-06-12 03:58:17]

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

[commit-bot: I haz the power, 2017-06-12 03:58:32]

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-06-12 04:12:07]

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

[commit-bot: I haz the power, 2017-06-12 04:12:09]

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

Thanks for your comments!
I've tried to split a couple of chunks off from this CL. 

- https://codereview.chromium.org/2933793002/
- https://codereview.chromium.org/2932273002/

This CL is still big, but I hope it helps you review the CLs.

https://codereview.chromium.org/2779763004/diff/210001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager_unittest.cc
(right):

https://codereview.chromium.org/2779763004/diff/210001/content/browser/backgr...
content/browser/background_sync/background_sync_manager_unittest.cc:469:
ASSERT_TRUE(sw_registration_1_->HasOneRef());
On 2017/06/06 14:06:33, falken wrote:
> Why ASSERT? Use ASSERT for fatal errors when it wouldn't make sense to
continue
> (we'd crash or something) if false. It looks like EXPECT is more suitable
here.

That's because if |sw_registration_1| isn't the only reference to the
registration, the following line to release the registration doesn't make sense.
That means the following Register() and |callback_status_| should be different
of the expectation. 
However I feel I don't exactly understand when ASSERT should be used... Is
EXPECT suitable in this case?

https://codereview.chromium.org/2779763004/diff/210001/content/browser/paymen...
File content/browser/payments/payment_app_content_unittest_base.cc (right):

https://codereview.chromium.org/2779763004/diff/210001/content/browser/paymen...
content/browser/payments/payment_app_content_unittest_base.cc:134:
EXPECT_TRUE(registration);
On 2017/06/06 14:06:33, falken wrote:
> ASSERT, since the other EXPECTs don't make sense if false (we'd crash)

We seems not to be able to use ASSERT in functions whose return type is not
void. 
Do you have any typical ways to handle the situation?
Returning nullptr and handle that on the caller is one possible way, but I feel
it's a bit tough because all of callers should have a null check.

https://codereview.chromium.org/2779763004/diff/210001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance.h (right):

https://codereview.chromium.org/2779763004/diff/210001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.h:289: // renderer side.
On 2017/06/06 14:06:33, falken wrote:
> Not sure I understand. Does this mean the browser side should never disconnect
> the pipe, and instead it's the renderer's responsibility to disconnect it?
> 
> If so, I'd suggest:
> "The browser process should not disconnect the pipe, because other associated
> interfaces may be using it. The renderer process will disconnect the pipe when
> appropriate."

Yes, that's correct. Updated, thanks!

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

https://codereview.chromium.org/2779763004/diff/210001/content/browser/servic...
content/browser/service_worker/embedded_worker_test_helper.h:118: // Registers a
mojo endpoint object derived from
On 2017/06/06 14:06:33, falken wrote:
> while you're here: Mojo.

Done.

https://codereview.chromium.org/2779763004/diff/210001/content/browser/servic...
content/browser/service_worker/embedded_worker_test_helper.h:125: // to the map.
This should be called before ServiceWorkerDispatcher::Init
On 2017/06/06 14:06:33, falken wrote:
> ServiceWorkerDispatcherHost::Init?

Done.

https://codereview.chromium.org/2779763004/diff/210001/content/browser/servic...
File content/browser/service_worker/service_worker_context_unittest.cc (right):

https://codereview.chromium.org/2779763004/diff/210001/content/browser/servic...
content/browser/service_worker/service_worker_context_unittest.cc:234: //
reference to the ServiceWorkerVersion.
On 2017/06/06 14:06:33, falken wrote:
> This comment is nice for understanding the change in the test, but I'm not
sure
> it'll be useful when reading the code for an explanation of why something you
> didn't expect didn't happen.

Thanks, I agree that this comment doesn't describe the behavior itself. Removed
the comments.
> 
> The original test called StopWorker because no one had a reference to
> ServiceWorkerVersion so it got destructed and called Stop()?
> 
> That's kind of weird, I would have thought the ServiceWorkerRegistration held
a
> reference to the SWVersion?

Yes, I think releasing SWVersion triggers Stop().
ServiceWorkerRegisterJob tentatively has the reference to the registration which
are about to be registered, but it would be released at the end of the job. The
reference to registration is usually kept by  SWRegistrationHandle which is
created on the association, but previously it's not created on this test. 
After browser-side creation of SWPH, the provider is created before the service
worker starts. The relevant SWRegistrationHandle and SWHandles are created at
the time.

https://codereview.chromium.org/2779763004/diff/210001/content/browser/servic...
content/browser/service_worker/service_worker_context_unittest.cc:339: //
worker.
On 2017/06/06 14:06:33, falken wrote:
> ditto: not sure this comment is worth leaving in the codebase
> 
> Why was this previously causing the worker to stop? The registration isn't
> holding a reference to the version?

Done.

https://codereview.chromium.org/2779763004/diff/210001/content/browser/servic...
content/browser/service_worker/service_worker_context_unittest.cc:616: // The
provider_id is not fixed because it's unique beyond tests.
On 2017/06/06 14:06:33, falken wrote:
> "unique beyond tests" means that other tests influence the value?

Yes, the provider id is unique because it's saved as static. Should it be
initialized when SWContextCore is initialized, for example?

https://codereview.chromium.org/2779763004/diff/210001/content/browser/servic...
content/browser/service_worker/service_worker_context_unittest.cc:727: //
registered worker should have taken no 0.
On 2017/06/06 14:06:33, falken wrote:
> "no 0" means "number 0"? Recommend ID 0.

Done.

https://codereview.chromium.org/2779763004/diff/210001/content/browser/servic...
content/browser/service_worker/service_worker_context_unittest.cc:772: // The
new context should take over next handle ids. No 2 should have been
On 2017/06/06 14:06:33, falken wrote:
> s/No/Number or ID
> 
> (No sounds like the opposite of "yes")

Thanks. I felt the same when I read the comment again..

https://codereview.chromium.org/2779763004/diff/210001/content/browser/servic...
File content/browser/service_worker/service_worker_dispatcher_host.h (right):

https://codereview.chromium.org/2779763004/diff/210001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host.h:82: 
On 2017/06/06 14:06:33, falken wrote:
> Now that this is public let's clean it up a little.

Acknowledged.

https://codereview.chromium.org/2779763004/diff/210001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host.h:86:
ServiceWorkerRegistrationObjectInfo* info,
On 2017/06/06 14:06:34, falken wrote:
> out_info

Done.

https://codereview.chromium.org/2779763004/diff/210001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host.h:87:
ServiceWorkerVersionAttributes* attrs);
On 2017/06/06 14:06:33, falken wrote:
> out_attrs

Done.

https://codereview.chromium.org/2779763004/diff/210001/content/browser/servic...
File content/browser/service_worker/service_worker_job_unittest.cc (right):

https://codereview.chromium.org/2779763004/diff/210001/content/browser/servic...
content/browser/service_worker/service_worker_job_unittest.cc:50: 
On 2017/06/06 14:06:34, falken wrote:
> // A dispatcher host that holds on to all registered ServiceWorkerHandles and
> ServiceWorkerRegistrationHandles.

Done.

https://codereview.chromium.org/2779763004/diff/210001/content/browser/servic...
content/browser/service_worker/service_worker_job_unittest.cc:51: class
KeepHandleDispatcherHost : public ServiceWorkerDispatcherHost {
On 2017/06/06 14:06:34, falken wrote:
> s/Handle/Handles

Done.

https://codereview.chromium.org/2779763004/diff/210001/content/browser/servic...
content/browser/service_worker/service_worker_job_unittest.cc:402: // context.
KeepHandleDispatcherHost will stores the handles.
On 2017/06/06 14:06:34, falken wrote:
> s/stores/store

Done.

https://codereview.chromium.org/2779763004/diff/210001/content/browser/servic...
content/browser/service_worker/service_worker_job_unittest.cc:468: // context.
KeepHandleDispatcherHost will stores the handles.
On 2017/06/06 14:06:34, falken wrote:
> s/stores/store

Done.

[falken, 2017-06-12 07:01:29]

https://codereview.chromium.org/2779763004/diff/210001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager_unittest.cc
(right):

https://codereview.chromium.org/2779763004/diff/210001/content/browser/backgr...
content/browser/background_sync/background_sync_manager_unittest.cc:469:
ASSERT_TRUE(sw_registration_1_->HasOneRef());
On 2017/06/12 06:08:12, shimazu wrote:
> On 2017/06/06 14:06:33, falken wrote:
> > Why ASSERT? Use ASSERT for fatal errors when it wouldn't make sense to
> continue
> > (we'd crash or something) if false. It looks like EXPECT is more suitable
> here.
> 
> That's because if |sw_registration_1| isn't the only reference to the
> registration, the following line to release the registration doesn't make
sense.
> That means the following Register() and |callback_status_| should be different
> of the expectation. 
> However I feel I don't exactly understand when ASSERT should be used... Is
> EXPECT suitable in this case?

Ah I see what you mean. I agree ASSERT is OK here.

https://codereview.chromium.org/2779763004/diff/210001/content/browser/paymen...
File content/browser/payments/payment_app_content_unittest_base.cc (right):

https://codereview.chromium.org/2779763004/diff/210001/content/browser/paymen...
content/browser/payments/payment_app_content_unittest_base.cc:134:
EXPECT_TRUE(registration);
On 2017/06/12 06:08:12, shimazu wrote:
> On 2017/06/06 14:06:33, falken wrote:
> > ASSERT, since the other EXPECTs don't make sense if false (we'd crash)
> 
> We seems not to be able to use ASSERT in functions whose return type is not
> void. 
> Do you have any typical ways to handle the situation?
> Returning nullptr and handle that on the caller is one possible way, but I
feel
> it's a bit tough because all of callers should have a null check. 

Ah didn't know that. I guess we just use EXPECT here then.

https://codereview.chromium.org/2779763004/diff/250001/content/common/service...
File content/common/service_worker/embedded_worker_start_params.h (right):

https://codereview.chromium.org/2779763004/diff/250001/content/common/service...
content/common/service_worker/embedded_worker_start_params.h:28:
ServiceWorkerProviderHost* controller_provider;
It seems weird that content/common has a message with ServiceWorkerProviderHost
in it, since that's a browser process concept.

Does controller_provider need to live in this struct?

[shimazu, 2017-06-13 00:04:43]

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

[commit-bot: I haz the power, 2017-06-13 00:05: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-06-13 00:12:29]

The CQ bit was unchecked by shimazu@chromium.org

[shimazu, 2017-06-13 00:24:05]

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

[commit-bot: I haz the power, 2017-06-13 00:24:24]

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-06-13 01:33:15]

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

[commit-bot: I haz the power, 2017-06-13 01:33:16]

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

[kinuko, 2017-06-13 06:21:39]

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

[kinuko, 2017-06-13 06:21:40]

Haven't looked into the tests yet, but looking good!

https://codereview.chromium.org/2779763004/diff/270001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/2779763004/diff/270001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.cc:600: process_id(),
&provider_client_info);
This feels a bit puzzling to me as well... params having a ptr to ProviderHost
looks wrong.

I think one of the design changes we could possibly make at this point is just
to let EmbeddedWorkerInstance have a back pointer to the SWV, which could make
things simpler...

(Alternatively we could pass along the ProviderHost pointer in addition to the
param too)

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

https://codereview.chromium.org/2779763004/diff/270001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:44: return
g_next_navigation_provider_id--;
let's rename this to g_next_browser_provided_provider_id

https://codereview.chromium.org/2779763004/diff/270001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:134:
std::unique_ptr<ServiceWorkerProviderHost> host =
nit: auto host =

is fine

https://codereview.chromium.org/2779763004/diff/270001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:139:
SERVICE_WORKER_PROVIDER_FOR_CONTROLLER, true),
nit: true /* are_ancestors_secure */

[shimazu, 2017-06-14 07:22:58]

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

[commit-bot: I haz the power, 2017-06-14 07:23:11]

Dry run: CQ is trying da patch.

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

[shimazu, 2017-06-14 07:24:40]

Updated, PTAnL:)

https://codereview.chromium.org/2779763004/diff/250001/content/common/service...
File content/common/service_worker/embedded_worker_start_params.h (right):

https://codereview.chromium.org/2779763004/diff/250001/content/common/service...
content/common/service_worker/embedded_worker_start_params.h:28:
ServiceWorkerProviderHost* controller_provider;
On 2017/06/12 07:01:28, falken wrote:
> It seems weird that content/common has a message with
ServiceWorkerProviderHost
> in it, since that's a browser process concept.
> 
> Does controller_provider need to live in this struct?

Actually it was because I could avoid lots of changes in unittests. It's not
necessary to be owned by StartParams, but I originally thought
ServiceWorkerProvider's info could be an parameter of it.

https://codereview.chromium.org/2779763004/diff/270001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/2779763004/diff/270001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.cc:600: process_id(),
&provider_client_info);
On 2017/06/13 06:21:40, kinuko wrote:
> This feels a bit puzzling to me as well... params having a ptr to ProviderHost
> looks wrong.
> 
> I think one of the design changes we could possibly make at this point is just
> to let EmbeddedWorkerInstance have a back pointer to the SWV, which could make
> things simpler...
> 
> (Alternatively we could pass along the ProviderHost pointer in addition to the
> param too)

Done.

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

https://codereview.chromium.org/2779763004/diff/270001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:44: return
g_next_navigation_provider_id--;
On 2017/06/13 06:21:40, kinuko wrote:
> let's rename this to g_next_browser_provided_provider_id

Done.

https://codereview.chromium.org/2779763004/diff/270001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:134:
std::unique_ptr<ServiceWorkerProviderHost> host =
On 2017/06/13 06:21:40, kinuko wrote:
> nit: auto host =
> 
> is fine

Done.

https://codereview.chromium.org/2779763004/diff/270001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:139:
SERVICE_WORKER_PROVIDER_FOR_CONTROLLER, true),
On 2017/06/13 06:21:40, kinuko wrote:
> nit: true /* are_ancestors_secure */

Done.

[commit-bot: I haz the power, 2017-06-14 07:58:16]

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

[commit-bot: I haz the power, 2017-06-14 07:58:17]

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

[falken, 2017-06-14 08:10:35]

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance.h (right):

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.h:300: 
// The provider host that is hosting this running worker.

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance_unittest.cc
(right):

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance_unittest.cc:68:
binding_.Bind(mojo::MakeRequest(&(*provider_client_info)->host_ptr_info));
this line is pretty cryptic. can you break it up with named variables?

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
File content/browser/service_worker/service_worker_context_unittest.cc (right):

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
content/browser/service_worker/service_worker_context_unittest.cc:612: // The
provider_id is not fixed because it's unique beyond tests.
The "unique beyond tests" is still confusing.

"Since the provider host is created via
CreateProviderHostForServiceWorkerContext, the provider_id is not a fixed
number."

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

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:223:
helper_->mock_render_process_id());
Can you help me understand why we use dispatcher_host_->render_process_id_ in
line 218 and helper_->mock_render_process_id() in line 223?

If these are different, a comment explaining why would be helpful.

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:845:
EXPECT_NE(SERVICE_WORKER_ERROR_TIMEOUT, status);
this is redundant with line 846, no?

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

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:201:
CHECK_EQ(ChildProcessHost::kInvalidUniqueID, render_process_id);
Is there a reason this is a CHECK isntead of a DCHECK? It doesn't look like a
security issue if this is violated.

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:353:
DCHECK_EQ(EmbeddedWorkerStatus::STOPPED, version->running_status());
does DCHECK(!running_hosted_version_) also hold?

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:698:
DCHECK(dispatcher_host);
Is it possible the process crashed by the time we got here?

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:721:
base::Bind(&RemoveProviderHost, context_, process_id, provider_id()));
If the process already crashed, does the error handler get caleld?

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

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:62: // and the
renderer. 1) When it's for a document or worker (i.e., a service
"created on the browser and the renderer" sounds like it can be created in the
renderer process.

How about:

A ServiceWorkerProviderHost is created in the following situations:
1) When it's for a document or worker...
2) When it's for a service worker context...
3) When PlzNavigate is turned on...

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:101: // about the
ServiceWorkerProviderHost is passed with the StartWorker message.
The use of "context" here is confusing since the |context| param is not the
worker's context. Overall "context" is a really overloaded term for service
workers.

Recommend changing this to "PreCreateForHostedVersion".

// Creates a ServiceWorkerProviderHost for hosting a running service worker.

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:283: // Completes
initialization of provider hosts for controllers.
When is this called?

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:286:
mojom::ServiceWorkerProviderClientInfoPtr* provider_client_info);
Does this mutate |provider_client_info|? If so, |out_provider_client_info| or
maybe just |out_info|

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
content/browser/service_worker/service_worker_version.h:741: // Keeps track of
the provider hosting worker's context for this version.
"hosting this running service worker"

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

https://codereview.chromium.org/2779763004/diff/290001/content/common/service...
content/common/service_worker/service_worker_provider.mojom:12: // This is
passed on launching a worker's context.
launching a service worker.

[falken, 2017-06-14 08:30:19]

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

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:283: // Completes
initialization of provider hosts for controllers.
On 2017/06/14 08:10:35, falken wrote:
> When is this called?

(I mean to add something to the documentation like this is called after a
process is allocated for the worker.)

[kinuko, 2017-06-14 08:37:40]

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance.h (right):

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.h:112: // to start the
worker.
Please document |provider_host|.

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance_unittest.cc
(right):

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance_unittest.cc:49:
MockServiceWorkerProviderHost(base::WeakPtr<ServiceWorkerContextCore> context)
nit: explicit

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

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:101: // about the
ServiceWorkerProviderHost is passed with the StartWorker message.
On 2017/06/14 08:10:35, falken wrote:
> The use of "context" here is confusing since the |context| param is not the
> worker's context. Overall "context" is a really overloaded term for service
> workers.
> 
> Recommend changing this to "PreCreateForHostedVersion".
> 
> // Creates a ServiceWorkerProviderHost for hosting a running service worker.

Or PreCreateForController? (The version is actually not yet hosted yet...)

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:864:
context()->AddProviderHost(std::move(pending_provider_host_));
I have a feeling that SetHostedVersion should be called here rather than in
PreCreate method, as we're temporarily creating a cyclic reference until this
point... wdyt?

https://codereview.chromium.org/2779763004/diff/290001/content/child/service_...
File content/child/service_worker/service_worker_network_provider.h (right):

https://codereview.chromium.org/2779763004/diff/290001/content/child/service_...
content/child/service_worker/service_worker_network_provider.h:64:
ServiceWorkerNetworkProvider(mojom::ServiceWorkerProviderClientInfoPtr info);
nit: explicit

[shimazu, 2017-06-14 09:35:21]

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

[commit-bot: I haz the power, 2017-06-14 09:35:38]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-06-14 10:31:09]

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

[commit-bot: I haz the power, 2017-06-14 10:31:11]

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

[shimazu, 2017-06-16 04:10:08]

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

[commit-bot: I haz the power, 2017-06-16 04:10:30]

Dry run: CQ is trying da patch.

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

[shimazu, 2017-06-16 04:18:44]

PTAL again.

// though a couple of win compilation are failing... I'm now looking.

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance.h (right):

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.h:112: // to start the
worker.
On 2017/06/14 08:37:40, kinuko wrote:
> Please document |provider_host|.

Done.

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.h:300: 
On 2017/06/14 08:10:34, falken wrote:
> // The provider host that is hosting this running worker.

Done.

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance_unittest.cc
(right):

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance_unittest.cc:68:
binding_.Bind(mojo::MakeRequest(&(*provider_client_info)->host_ptr_info));
On 2017/06/14 08:10:34, falken wrote:
> this line is pretty cryptic. can you break it up with named variables?

Done.

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
File content/browser/service_worker/service_worker_context_unittest.cc (right):

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
content/browser/service_worker/service_worker_context_unittest.cc:612: // The
provider_id is not fixed because it's unique beyond tests.
On 2017/06/14 08:10:34, falken wrote:
> The "unique beyond tests" is still confusing.
> 
> "Since the provider host is created via
> CreateProviderHostForServiceWorkerContext, the provider_id is not a fixed
> number."

Thanks, I missed to fix this. Updated.

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

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:223:
helper_->mock_render_process_id());
On 2017/06/14 08:10:34, falken wrote:
> Can you help me understand why we use dispatcher_host_->render_process_id_ in
> line 218 and helper_->mock_render_process_id() in line 223?
> 
> If these are different, a comment explaining why would be helpful.

Thanks, these two should be the same. Updated.

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:845:
EXPECT_NE(SERVICE_WORKER_ERROR_TIMEOUT, status);
On 2017/06/14 08:10:34, falken wrote:
> this is redundant with line 846, no?
Yes, that's right. Removed.

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

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:201:
CHECK_EQ(ChildProcessHost::kInvalidUniqueID, render_process_id);
On 2017/06/14 08:10:34, falken wrote:
> Is there a reason this is a CHECK isntead of a DCHECK? It doesn't look like a
> security issue if this is violated.

Done.

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:353:
DCHECK_EQ(EmbeddedWorkerStatus::STOPPED, version->running_status());
On 2017/06/14 08:10:35, falken wrote:
> does DCHECK(!running_hosted_version_) also hold?

Sounds good!

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:698:
DCHECK(dispatcher_host);
On 2017/06/14 08:10:35, falken wrote:
> Is it possible the process crashed by the time we got here?

Yes, I think so. 
Between EmbeddedWorkerInstance::StartTask::OnAllocatedProcess and here, there
are two (IO -> UI -> IO) thread hops. The renderer is possible to crash during
that.

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:721:
base::Bind(&RemoveProviderHost, context_, process_id, provider_id()));
On 2017/06/14 08:10:34, falken wrote:
> If the process already crashed, does the error handler get caleld?

Yes, I think so. It'll be triggered when calling StartWorker() on the Mojo
interface.

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

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:62: // and the
renderer. 1) When it's for a document or worker (i.e., a service
On 2017/06/14 08:10:35, falken wrote:
> "created on the browser and the renderer" sounds like it can be created in the
> renderer process.
> 
> How about:
> 
> A ServiceWorkerProviderHost is created in the following situations:
> 1) When it's for a document or worker...
> 2) When it's for a service worker context...
> 3) When PlzNavigate is turned on...

Yeah, that looks great! Updated the comment.

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:101: // about the
ServiceWorkerProviderHost is passed with the StartWorker message.
On 2017/06/14 08:37:40, kinuko wrote:
> On 2017/06/14 08:10:35, falken wrote:
> > The use of "context" here is confusing since the |context| param is not the
> > worker's context. Overall "context" is a really overloaded term for service
> > workers.
> > 
> > Recommend changing this to "PreCreateForHostedVersion".
> > 
> > // Creates a ServiceWorkerProviderHost for hosting a running service worker.
> 
> Or PreCreateForController? (The version is actually not yet hosted yet...)

Done.

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:283: // Completes
initialization of provider hosts for controllers.
On 2017/06/14 08:30:18, falken wrote:
> On 2017/06/14 08:10:35, falken wrote:
> > When is this called?
> 
> (I mean to add something to the documentation like this is called after a
> process is allocated for the worker.)

Done.

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:286:
mojom::ServiceWorkerProviderClientInfoPtr* provider_client_info);
On 2017/06/14 08:10:35, falken wrote:
> Does this mutate |provider_client_info|? If so, |out_provider_client_info| or
> maybe just |out_info|

Done.

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:864:
context()->AddProviderHost(std::move(pending_provider_host_));
On 2017/06/14 08:37:40, kinuko wrote:
> I have a feeling that SetHostedVersion should be called here rather than in
> PreCreate method, as we're temporarily creating a cyclic reference until this
> point... wdyt?

That's good point...
I tried to fix it by 

- moving SetHostedVersionId into CompleteStartWorkerPreparation
- having the provider host in ProviderClientInfoGetter which is newly introduced
on ps#18
- passing the getter to EWInstance::Start instead of provider_host
- using AddProviderHost and CompleteStartWorkerPreparation at the same function

I hope this makes things clearer. 
EWInstance doesn't have the provider host explicitly, and SWVersion doesn't have
the strong reference to the provider host. The references are safe if
ProviderClientInfoGetter ensures the cyclic reference doesn't happen.

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
content/browser/service_worker/service_worker_version.h:741: // Keeps track of
the provider hosting worker's context for this version.
On 2017/06/14 08:10:35, falken wrote:
> "hosting this running service worker"
> 

Done.

https://codereview.chromium.org/2779763004/diff/290001/content/child/service_...
File content/child/service_worker/service_worker_network_provider.h (right):

https://codereview.chromium.org/2779763004/diff/290001/content/child/service_...
content/child/service_worker/service_worker_network_provider.h:64:
ServiceWorkerNetworkProvider(mojom::ServiceWorkerProviderClientInfoPtr info);
On 2017/06/14 08:37:40, kinuko wrote:
> nit: explicit

Done.

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

https://codereview.chromium.org/2779763004/diff/290001/content/common/service...
content/common/service_worker/service_worker_provider.mojom:12: // This is
passed on launching a worker's context.
On 2017/06/14 08:10:35, falken wrote:
> launching a service worker.

Done.

[shimazu, 2017-06-16 04:21:39]

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

[commit-bot: I haz the power, 2017-06-16 04:22:03]

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-06-16 06:41:43]

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

[commit-bot: I haz the power, 2017-06-16 06:41:45]

Dry run: This issue passed the CQ dry run.

[kinuko, 2017-06-16 09:33:50]

lgtm (haven't thoroughly reviewed all the tests though)

works for me.

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

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:102: //
StartWorker message.
It feels a bit unclear what the second sentence is talking about, maybe
something like 'Information about this provider host is passed down to the
service worker via StartWorker message' is what you mean?

(I assume falken@ could give better advice here!)

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:1496:
base::BindOnce(&CompleteProviderHostPreperation, base::Unretained(this),
Maybe comment about why Unretained is fine (e.g. per ownership), or could
possibly use weak_factory_.GetWeakPtr()

[shimazu, 2017-06-16 13:58:10]

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

[shimazu, 2017-06-16 13:58:12]

dcheng@:
PTAL at changes in mojom and typemap files

[falken, 2017-06-16 15:44:47]

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

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:698:
DCHECK(dispatcher_host);
On 2017/06/16 04:18:43, shimazu wrote:
> On 2017/06/14 08:10:35, falken wrote:
> > Is it possible the process crashed by the time we got here?
> 
> Yes, I think so. 
> Between EmbeddedWorkerInstance::StartTask::OnAllocatedProcess and here, there
> are two (IO -> UI -> IO) thread hops. The renderer is possible to crash during
> that.

If the process crashed, I think dispatcher_host would be nullptr so this DCHECK
would fail.

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

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:67: // 2) When
it's for a service worker context, the provider host is created on
"for a running service worker" for consistency with the above paragraphs

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:102: //
StartWorker message.
On 2017/06/16 09:33:50, kinuko wrote:
> It feels a bit unclear what the second sentence is talking about, maybe
> something like 'Information about this provider host is passed down to the
> service worker via StartWorker message' is what you mean?
> 
> (I assume falken@ could give better advice here!)

Sounds good.

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:203:
mojom::ServiceWorkerProviderClientInfoPtr CompleteProviderHostPreperation(
typo: Preparation

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
File content/browser/service_worker/service_worker_version_unittest.cc (right):

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
content/browser/service_worker/service_worker_version_unittest.cc:710: //
Hosting provider has the reference to the |version_|.
The comment doesn't really explain the 'why' though.

// Destruct |version_| by releasing all references, including the provider
host's.

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

https://codereview.chromium.org/2779763004/diff/370001/content/child/service_...
content/child/service_worker/service_worker_network_provider.cc:223:
dispatcher->OnAssociateRegistration(-1, info->provider_id, info->registration,
this -1 is not used right, since we are bypassing ServiceWorkerMessageFilter?

Can you add a comment like /* unused thread_id */

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

https://codereview.chromium.org/2779763004/diff/370001/content/common/service...
content/common/service_worker/service_worker_provider.mojom:13: struct
ServiceWorkerProviderClientInfo {
Can you help me understand this terminology?

A ServiceWorkerProvider means a ServiceWorkerNetworkProvider (something that
makes resource requests that the browser process wants to intercept).

Adding "Client" is Mojo terminology that means it's on the renderer side or
something? But SWNProvider is already on the renderer side.

It feels like a better name should exist here. 

I would suggest ServiceWorkerNetworkProviderInfo, but this struct is restricted
to SWNP for service worker execution contexts (i.e., "controllers" in
SWProviderHost language, which is pretty bad since SWs don't have be
controllers).

maybe ServiceWorkerNetworkProviderInfoForStartWorker?

https://codereview.chromium.org/2779763004/diff/370001/content/common/service...
content/common/service_worker/service_worker_provider.mojom:16: bool
is_parent_frame_secure;
I don't think |type| and |is_parent_frame_secure| should be needed. If this
struct is only used for starting a service worker, |type| must be CONTROLLER and
|is_parent_frame_secure| has no meaning, since the service worker does not have
a parent frame.

https://codereview.chromium.org/2779763004/diff/370001/content/common/service...
content/common/service_worker/service_worker_provider.mojom:18: // currently to
be launched.
This terminology is a little inaccurate because the registration doesn't get
launched.

|registration| and |attributes| are information about the service worker's
registration.

[falken, 2017-06-16 15:56:31]

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

https://codereview.chromium.org/2779763004/diff/370001/content/common/service...
content/common/service_worker/service_worker_provider.mojom:18: // currently to
be launched.
On 2017/06/16 15:44:47, falken wrote:
> This terminology is a little inaccurate because the registration doesn't get
> launched.
> 
> |registration| and |attributes| are information about the service worker's
> registration.

And add "used to populate ServiceWorkerGlobalScope#registration"

[kinuko, 2017-06-19 03:20:07]

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

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:698:
DCHECK(dispatcher_host);
On 2017/06/16 15:44:46, falken wrote:
> On 2017/06/16 04:18:43, shimazu wrote:
> > On 2017/06/14 08:10:35, falken wrote:
> > > Is it possible the process crashed by the time we got here?
> > 
> > Yes, I think so. 
> > Between EmbeddedWorkerInstance::StartTask::OnAllocatedProcess and here,
there
> > are two (IO -> UI -> IO) thread hops. The renderer is possible to crash
during
> > that.
> 
> If the process crashed, I think dispatcher_host would be nullptr so this
DCHECK
> would fail.

I have a question, if it's possible that process crash could be propagated to
the IO thread before here could it already have called CallDetach on client_
error and reset client_?  Is it still safe to call client_ method in
SendStartWorker?

[kinuko, 2017-06-19 03:27:56]

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

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:102: //
StartWorker message.
On 2017/06/16 15:44:46, falken wrote:
> On 2017/06/16 09:33:50, kinuko wrote:
> > It feels a bit unclear what the second sentence is talking about, maybe
> > something like 'Information about this provider host is passed down to the
> > service worker via StartWorker message' is what you mean?
> > 
> > (I assume falken@ could give better advice here!)
> 
> Sounds good.

(Also feel free to use some other term than 'controller' if we can come up with
better, more consistent terminology... I agree that 'controller' could be also
confusing esp. when it's not really controlling)

[shimazu, 2017-06-19 03:52:11]

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

[commit-bot: I haz the power, 2017-06-19 03:52:24]

Dry run: CQ is trying da patch.

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

[shimazu, 2017-06-19 03:53:31]

Thanks for your reviews!
Uploaded ps#20. PTAnL.

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

https://codereview.chromium.org/2779763004/diff/290001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:698:
DCHECK(dispatcher_host);
On 2017/06/19 03:20:07, kinuko wrote:
> On 2017/06/16 15:44:46, falken wrote:
> > On 2017/06/16 04:18:43, shimazu wrote:
> > > On 2017/06/14 08:10:35, falken wrote:
> > > > Is it possible the process crashed by the time we got here?
> > > 
> > > Yes, I think so. 
> > > Between EmbeddedWorkerInstance::StartTask::OnAllocatedProcess and here,
> there
> > > are two (IO -> UI -> IO) thread hops. The renderer is possible to crash
> during
> > > that.
> > 
> > If the process crashed, I think dispatcher_host would be nullptr so this
> DCHECK
> > would fail.
> 
> I have a question, if it's possible that process crash could be propagated to
> the IO thread before here could it already have called CallDetach on client_
> error and reset client_?  Is it still safe to call client_ method in
> SendStartWorker?

Ah, sorry, I didn't understand what falken pointed out.
If the process crashed before reaching here, CallDetach is called and StartTask
is stopped at the moment. That means SendStartWorker which invokes this method
isn't called. Otherwise, dispatcher_host should be found here.

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

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:67: // 2) When
it's for a service worker context, the provider host is created on
On 2017/06/16 15:44:46, falken wrote:
> "for a running service worker" for consistency with the above paragraphs

Done.

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:102: //
StartWorker message.
On 2017/06/16 15:44:46, falken wrote:
> On 2017/06/16 09:33:50, kinuko wrote:
> > It feels a bit unclear what the second sentence is talking about, maybe
> > something like 'Information about this provider host is passed down to the
> > service worker via StartWorker message' is what you mean?
> > 
> > (I assume falken@ could give better advice here!)
> 
> Sounds good.

Thanks, done.

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:203:
mojom::ServiceWorkerProviderClientInfoPtr CompleteProviderHostPreperation(
On 2017/06/16 15:44:46, falken wrote:
> typo: Preparation

Done.

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:1496:
base::BindOnce(&CompleteProviderHostPreperation, base::Unretained(this),
On 2017/06/16 09:33:50, kinuko wrote:
> Maybe comment about why Unretained is fine (e.g. per ownership), or could
> possibly use weak_factory_.GetWeakPtr()

Done.

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
File content/browser/service_worker/service_worker_version_unittest.cc (right):

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
content/browser/service_worker/service_worker_version_unittest.cc:710: //
Hosting provider has the reference to the |version_|.
On 2017/06/16 15:44:46, falken wrote:
> The comment doesn't really explain the 'why' though.
> 
> // Destruct |version_| by releasing all references, including the provider
> host's.

Done.

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

https://codereview.chromium.org/2779763004/diff/370001/content/child/service_...
content/child/service_worker/service_worker_network_provider.cc:223:
dispatcher->OnAssociateRegistration(-1, info->provider_id, info->registration,
On 2017/06/16 15:44:46, falken wrote:
> this -1 is not used right, since we are bypassing ServiceWorkerMessageFilter?
> 
> Can you add a comment like /* unused thread_id */

Yes, that's right.
Added, thanks.

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

https://codereview.chromium.org/2779763004/diff/370001/content/common/service...
content/common/service_worker/service_worker_provider.mojom:13: struct
ServiceWorkerProviderClientInfo {
On 2017/06/16 15:44:47, falken wrote:
> Can you help me understand this terminology?
> 
> A ServiceWorkerProvider means a ServiceWorkerNetworkProvider (something that
> makes resource requests that the browser process wants to intercept).
> 
> Adding "Client" is Mojo terminology that means it's on the renderer side or
> something? But SWNProvider is already on the renderer side.
> 
> It feels like a better name should exist here. 
> 
> I would suggest ServiceWorkerNetworkProviderInfo, but this struct is
restricted
> to SWNP for service worker execution contexts (i.e., "controllers" in
> SWProviderHost language, which is pretty bad since SWs don't have be
> controllers).
> 
> maybe ServiceWorkerNetworkProviderInfoForStartWorker?

I originally thought that "ServiceWorkerProviderClient" is SWNetworkProvider on
the renderer and "ServiceWorkerProviderHost" is SWProviderHost on the
browser-side. Info means the information to be passed on each constructor. 
I agree that better naming should be here, but actually I was confused by the
naming around providers. 
Could you help me understand the variant of providers:
ServiceWorker"Network"Provider, ServiceWorkerProvider"Context" and
ServiceWorkerProvider"Host"?

In the aspect of mojom interface, there are only two concepts:
ServiceWorkerProvider(Client) <-> ServiceWorkerProviderHost. I think no
"Network"Provider in this layer. On the other hand, there are three relevant
classes in //content/renderer and //content/browser: SWNetworkProvider,
SWProviderContext and SWProviderHost. I assume that the counterpart of
SWProviderHost is SWNetworkProvider, but I felt it's a bit difficult to read the
relationship..
I actually tried to avoid "SWNetworkProvider" in the mojom interfaces to
simplify it. 
Do you have any good ideas for it?

https://codereview.chromium.org/2779763004/diff/370001/content/common/service...
content/common/service_worker/service_worker_provider.mojom:16: bool
is_parent_frame_secure;
On 2017/06/16 15:44:46, falken wrote:
> I don't think |type| and |is_parent_frame_secure| should be needed. If this
> struct is only used for starting a service worker, |type| must be CONTROLLER
and
> |is_parent_frame_secure| has no meaning, since the service worker does not
have
> a parent frame.

Yes, that's right.
Removed.

https://codereview.chromium.org/2779763004/diff/370001/content/common/service...
content/common/service_worker/service_worker_provider.mojom:18: // currently to
be launched.
On 2017/06/16 15:56:30, falken wrote:
> On 2017/06/16 15:44:47, falken wrote:
> > This terminology is a little inaccurate because the registration doesn't get
> > launched.
> > 
> > |registration| and |attributes| are information about the service worker's
> > registration.
> 
> And add "used to populate ServiceWorkerGlobalScope#registration"

Done.

[falken, 2017-06-19 05:21:43]

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

https://codereview.chromium.org/2779763004/diff/370001/content/common/service...
content/common/service_worker/service_worker_provider.mojom:13: struct
ServiceWorkerProviderClientInfo {
On 2017/06/19 03:53:30, shimazu wrote:
> On 2017/06/16 15:44:47, falken wrote:
> > Can you help me understand this terminology?
> > 
> > A ServiceWorkerProvider means a ServiceWorkerNetworkProvider (something that
> > makes resource requests that the browser process wants to intercept).
> > 
> > Adding "Client" is Mojo terminology that means it's on the renderer side or
> > something? But SWNProvider is already on the renderer side.
> > 
> > It feels like a better name should exist here. 
> > 
> > I would suggest ServiceWorkerNetworkProviderInfo, but this struct is
> restricted
> > to SWNP for service worker execution contexts (i.e., "controllers" in
> > SWProviderHost language, which is pretty bad since SWs don't have be
> > controllers).
> > 
> > maybe ServiceWorkerNetworkProviderInfoForStartWorker?
> 
> I originally thought that "ServiceWorkerProviderClient" is SWNetworkProvider
on
> the renderer and "ServiceWorkerProviderHost" is SWProviderHost on the
> browser-side. Info means the information to be passed on each constructor. 
> I agree that better naming should be here, but actually I was confused by the
> naming around providers. 
> Could you help me understand the variant of providers:
> ServiceWorker"Network"Provider, ServiceWorkerProvider"Context" and
> ServiceWorkerProvider"Host"?
> 
> In the aspect of mojom interface, there are only two concepts:
> ServiceWorkerProvider(Client) <-> ServiceWorkerProviderHost. I think no
> "Network"Provider in this layer. On the other hand, there are three relevant
> classes in //content/renderer and //content/browser: SWNetworkProvider,
> SWProviderContext and SWProviderHost. I assume that the counterpart of
> SWProviderHost is SWNetworkProvider, but I felt it's a bit difficult to read
the
> relationship..
> I actually tried to avoid "SWNetworkProvider" in the mojom interfaces to
> simplify it. 
> Do you have any good ideas for it?

These are great questions. I'm hoping kinuko@ or michaeln@ can provide history
here.

I appreciate trying to simplify the terminology. I think naming is
super-important and we should be consistent and clear and mojofication is the
perfect place to get it right.

I prefer to avoid "Client" to mean something on the renderer, because the word
"Client" is already taken in a lot of places. Particularly for
ServiceWorkerProviderHost itself, we talk about "client" to mean a service
worker client as in ServiceWorkerGlobalScope#clients. We also use it to just
mean a class that uses another class, like ServiceWorkerContextClient.

In most of the Chromium codebase, XXX and XXXHost is a pair that means XXX is on
the renderer, and XXXHost is on the browser process.

Here's my understanding:
ServiceWorkerNetworkProvider: Something on the renderer that attaches to any
execution context that can make resource requests. These are service worker
clients: documents and workers; and service workers themselves. 

ServiceWorkerProviderContext: SWNetworkProvider HAS_A SWProviderContext. This is
an abstract class helper that lets us have different behavior between SW clients
and SW themselves. I think the meaning of "Context" here is to emphasize the
different behavior based on the type execution context (client or SW).

ServiceWorkerProviderHost: The browser-side of ServiceWorkerNetworkProvider. The
naming asymmetry has historical reasons.

I suspect originally NetworkProvider was supposed to just be about resource
requests, but it expanded to include registration association to implement
navigator.serviceWorker.controller and self.registration.

If we're keeping the ServiceWorkerNetworkProvider naming, I still think this
should be ServiceWorkerNetworkProviderInfoForStartWorker because it's the params
to the ServiceWorkerNetworkProvider ctor when we're starting a worker. If we're
going to say ServiceWorkerNetworkProvider is misnamed and should just be
ServiceWorkerProvider, then we could call this
ServiceWorkerProviderInfoForStartWorker and rename SWNP separately.

[commit-bot: I haz the power, 2017-06-19 05:37:38]

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

[commit-bot: I haz the power, 2017-06-19 05:37:39]

Dry run: This issue passed the CQ dry run.

[shimazu, 2017-06-19 05:53:50]

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

[commit-bot: I haz the power, 2017-06-19 05:54:04]

Dry run: CQ is trying da patch.

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

[shimazu, 2017-06-19 06:00:40]

Thanks a lot for the explanation!
I prefer the plan of changing SWNetworkProvider to SWProvider and align the
struct's name with that.
kinuko@: do you have any thoughts on that?

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

https://codereview.chromium.org/2779763004/diff/370001/content/common/service...
content/common/service_worker/service_worker_provider.mojom:13: struct
ServiceWorkerProviderClientInfo {
On 2017/06/19 05:21:43, falken wrote:
> On 2017/06/19 03:53:30, shimazu wrote:
> > On 2017/06/16 15:44:47, falken wrote:
> > > Can you help me understand this terminology?
> > > 
> > > A ServiceWorkerProvider means a ServiceWorkerNetworkProvider (something
that
> > > makes resource requests that the browser process wants to intercept).
> > > 
> > > Adding "Client" is Mojo terminology that means it's on the renderer side
or
> > > something? But SWNProvider is already on the renderer side.
> > > 
> > > It feels like a better name should exist here. 
> > > 
> > > I would suggest ServiceWorkerNetworkProviderInfo, but this struct is
> > restricted
> > > to SWNP for service worker execution contexts (i.e., "controllers" in
> > > SWProviderHost language, which is pretty bad since SWs don't have be
> > > controllers).
> > > 
> > > maybe ServiceWorkerNetworkProviderInfoForStartWorker?
> > 
> > I originally thought that "ServiceWorkerProviderClient" is SWNetworkProvider
> on
> > the renderer and "ServiceWorkerProviderHost" is SWProviderHost on the
> > browser-side. Info means the information to be passed on each constructor. 
> > I agree that better naming should be here, but actually I was confused by
the
> > naming around providers. 
> > Could you help me understand the variant of providers:
> > ServiceWorker"Network"Provider, ServiceWorkerProvider"Context" and
> > ServiceWorkerProvider"Host"?
> > 
> > In the aspect of mojom interface, there are only two concepts:
> > ServiceWorkerProvider(Client) <-> ServiceWorkerProviderHost. I think no
> > "Network"Provider in this layer. On the other hand, there are three relevant
> > classes in //content/renderer and //content/browser: SWNetworkProvider,
> > SWProviderContext and SWProviderHost. I assume that the counterpart of
> > SWProviderHost is SWNetworkProvider, but I felt it's a bit difficult to read
> the
> > relationship..
> > I actually tried to avoid "SWNetworkProvider" in the mojom interfaces to
> > simplify it. 
> > Do you have any good ideas for it?
> 
> These are great questions. I'm hoping kinuko@ or michaeln@ can provide history
> here.
> 
> I appreciate trying to simplify the terminology. I think naming is
> super-important and we should be consistent and clear and mojofication is the
> perfect place to get it right.
> 
> I prefer to avoid "Client" to mean something on the renderer, because the word
> "Client" is already taken in a lot of places. Particularly for
> ServiceWorkerProviderHost itself, we talk about "client" to mean a service
> worker client as in ServiceWorkerGlobalScope#clients. We also use it to just
> mean a class that uses another class, like ServiceWorkerContextClient.
> 
> In most of the Chromium codebase, XXX and XXXHost is a pair that means XXX is
on
> the renderer, and XXXHost is on the browser process.
> 
> Here's my understanding:
> ServiceWorkerNetworkProvider: Something on the renderer that attaches to any
> execution context that can make resource requests. These are service worker
> clients: documents and workers; and service workers themselves. 
> 
> ServiceWorkerProviderContext: SWNetworkProvider HAS_A SWProviderContext. This
is
> an abstract class helper that lets us have different behavior between SW
clients
> and SW themselves. I think the meaning of "Context" here is to emphasize the
> different behavior based on the type execution context (client or SW).
> 
> ServiceWorkerProviderHost: The browser-side of ServiceWorkerNetworkProvider.
The
> naming asymmetry has historical reasons.
> 
> I suspect originally NetworkProvider was supposed to just be about resource
> requests, but it expanded to include registration association to implement
> navigator.serviceWorker.controller and self.registration.
> 
> If we're keeping the ServiceWorkerNetworkProvider naming, I still think this
> should be ServiceWorkerNetworkProviderInfoForStartWorker because it's the
params
> to the ServiceWorkerNetworkProvider ctor when we're starting a worker. If
we're
> going to say ServiceWorkerNetworkProvider is misnamed and should just be
> ServiceWorkerProvider, then we could call this
> ServiceWorkerProviderInfoForStartWorker and rename SWNP separately.

Thanks, ServiceWorkerProviderInfoForStartWorker sounds good for me. 
Changed the name of the struct to ServiceWorkerProviderInfoForStartWorker.

[kinuko, 2017-06-19 06:42:42]

On 2017/06/19 05:21:43, falken wrote:
>
https://codereview.chromium.org/2779763004/diff/370001/content/common/service...
> File content/common/service_worker/service_worker_provider.mojom (right):
> 
>
https://codereview.chromium.org/2779763004/diff/370001/content/common/service...
> content/common/service_worker/service_worker_provider.mojom:13: struct
> ServiceWorkerProviderClientInfo {
> On 2017/06/19 03:53:30, shimazu wrote:
> > On 2017/06/16 15:44:47, falken wrote:
> > > Can you help me understand this terminology?
> > > 
> > > A ServiceWorkerProvider means a ServiceWorkerNetworkProvider (something
that
> > > makes resource requests that the browser process wants to intercept).
> > > 
> > > Adding "Client" is Mojo terminology that means it's on the renderer side
or
> > > something? But SWNProvider is already on the renderer side.
> > > 
> > > It feels like a better name should exist here. 
> > > 
> > > I would suggest ServiceWorkerNetworkProviderInfo, but this struct is
> > restricted
> > > to SWNP for service worker execution contexts (i.e., "controllers" in
> > > SWProviderHost language, which is pretty bad since SWs don't have be
> > > controllers).
> > > 
> > > maybe ServiceWorkerNetworkProviderInfoForStartWorker?
> > 
> > I originally thought that "ServiceWorkerProviderClient" is SWNetworkProvider
> on
> > the renderer and "ServiceWorkerProviderHost" is SWProviderHost on the
> > browser-side. Info means the information to be passed on each constructor. 
> > I agree that better naming should be here, but actually I was confused by
the
> > naming around providers. 
> > Could you help me understand the variant of providers:
> > ServiceWorker"Network"Provider, ServiceWorkerProvider"Context" and
> > ServiceWorkerProvider"Host"?
> > 
> > In the aspect of mojom interface, there are only two concepts:
> > ServiceWorkerProvider(Client) <-> ServiceWorkerProviderHost. I think no
> > "Network"Provider in this layer. On the other hand, there are three relevant
> > classes in //content/renderer and //content/browser: SWNetworkProvider,
> > SWProviderContext and SWProviderHost. I assume that the counterpart of
> > SWProviderHost is SWNetworkProvider, but I felt it's a bit difficult to read
> the
> > relationship..
> > I actually tried to avoid "SWNetworkProvider" in the mojom interfaces to
> > simplify it. 
> > Do you have any good ideas for it?
> 
> These are great questions. I'm hoping kinuko@ or michaeln@ can provide history
> here.
> 
> I appreciate trying to simplify the terminology. I think naming is
> super-important and we should be consistent and clear and mojofication is the
> perfect place to get it right.
> 
> I prefer to avoid "Client" to mean something on the renderer, because the word
> "Client" is already taken in a lot of places. Particularly for
> ServiceWorkerProviderHost itself, we talk about "client" to mean a service
> worker client as in ServiceWorkerGlobalScope#clients. We also use it to just
> mean a class that uses another class, like ServiceWorkerContextClient.
> 
> In most of the Chromium codebase, XXX and XXXHost is a pair that means XXX is
on
> the renderer, and XXXHost is on the browser process.
> 
> Here's my understanding:
> ServiceWorkerNetworkProvider: Something on the renderer that attaches to any
> execution context that can make resource requests. These are service worker
> clients: documents and workers; and service workers themselves. 
> 
> ServiceWorkerProviderContext: SWNetworkProvider HAS_A SWProviderContext. This
is
> an abstract class helper that lets us have different behavior between SW
clients
> and SW themselves. I think the meaning of "Context" here is to emphasize the
> different behavior based on the type execution context (client or SW).
> 
> ServiceWorkerProviderHost: The browser-side of ServiceWorkerNetworkProvider.
The
> naming asymmetry has historical reasons.
> 
> I suspect originally NetworkProvider was supposed to just be about resource
> requests, but it expanded to include registration association to implement
> navigator.serviceWorker.controller and self.registration.
> 
> If we're keeping the ServiceWorkerNetworkProvider naming, I still think this
> should be ServiceWorkerNetworkProviderInfoForStartWorker because it's the
params
> to the ServiceWorkerNetworkProvider ctor when we're starting a worker. If
we're
> going to say ServiceWorkerNetworkProvider is misnamed and should just be
> ServiceWorkerProvider, then we could call this
> ServiceWorkerProviderInfoForStartWorker and rename SWNP separately.

One complicating bit is that we also have WebServiceWorkerProvider (and Impl),
that's implementing provider for navigator.serviceWorker container. 
(Web)ServiceWorkerProvider(Impl) is mainly for script access while
(Web)ServiceWorkerNetworkProvider is for request interception, and they share
one ServiceWorkerProviderContext.  (Originally there was only one single
ServiceWorkerProvider in the renderer side but we split it into two for
script-part and network-part, and that's where diversion from the browser-side
terminology happened)

We could rename things around, e.g.
- (Web)ServiceWorkerProvider(Impl)  to something else (e.g. SWContainerProvider
or something like that)
- SWNP to ServiceWorkerProvider (to match it with SWPH)
- WebServiceWorkerNetworkProvider can be more generic fetch/loading context
interface (that's a brief plan in loading side)

but I doubt we want to touch these in this patch.  Regarding the provider-info
naming in this patch I can live with either idea, while I agree that using both
'Client' and 'Host' terminologies felt a bit confusing.

(By the way what made us have two mojom files around these,
service_worker_provider.mojom and service_worker_provider_interfaces.mojom? 
This is adding another confusion to me.)

[commit-bot: I haz the power, 2017-06-19 07:40:02]

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

[commit-bot: I haz the power, 2017-06-19 07:40:03]

Dry run: This issue passed the CQ dry run.

[falken, 2017-06-19 07:50:40]

Agree we don't need a big renaming of ServiceWorkerNetworkProvider etc in this
patch, but want to set things in the right direction.

+1 about the two mojom files... that could also be fixed outside the patch if
appropriate.

Sorry for all the bikeshedding but I think naming is very important in a complex
codebase.

https://codereview.chromium.org/2779763004/diff/410001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/2779763004/diff/410001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.cc:445:
ProviderClientInfoGetter provider_client_info_getter,
rename

https://codereview.chromium.org/2779763004/diff/410001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance.h (right):

https://codereview.chromium.org/2779763004/diff/410001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.h:76: int)>;
Rername to ProviderInfoGetter

int /* process_id */

https://codereview.chromium.org/2779763004/diff/410001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.h:115: // to start the
worker. |provider_client_info_getter| will be used when this
"|provider_info_getter| is called when this instance allocates..."

https://codereview.chromium.org/2779763004/diff/410001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.h:118:
ProviderClientInfoGetter provider_client_info_getter,
Rename to provider_info_getter

https://codereview.chromium.org/2779763004/diff/410001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.h:306:
ProviderClientInfoGetter provider_client_info_getter_;
ProviderInfoGetter provider_info_getter

https://codereview.chromium.org/2779763004/diff/410001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance_unittest.cc
(right):

https://codereview.chromium.org/2779763004/diff/410001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance_unittest.cc:126:
std::move(params), CreateProviderClientInfoGetter(),
CreateProviderInfoGetter

https://codereview.chromium.org/2779763004/diff/410001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance_unittest.cc:145:
mojom::ServiceWorkerProviderInfoForStartWorkerPtr CreateProviderClientInfo(
rename

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

https://codereview.chromium.org/2779763004/diff/410001/content/browser/servic...
content/browser/service_worker/service_worker_test_utils.h:66: void
BindWithProviderClientInfo(
BindWithProviderInfo

https://codereview.chromium.org/2779763004/diff/410001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/2779763004/diff/410001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:211:
context->AddProviderHost(std::move(provider_host));
Could context be null?

If not, we could DCHECK(context) and add a comment why it's never null.

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

https://codereview.chromium.org/2779763004/diff/410001/content/common/service...
content/common/service_worker/service_worker_provider.mojom:12: // This is
passed on launching a service worker.
"for the constructor of ServiceWorkerNetworkProvider used for starting a service
worker" and delete the final sentence.

[shimazu, 2017-06-19 10:41:25]

There are two mojom files because I wanted to put the interfaces defined in
service_worker_provider_interfaces.mojom in the structs defined in
service_worker_provider.mojom.

https://codereview.chromium.org/2779763004/diff/410001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/2779763004/diff/410001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.cc:445:
ProviderClientInfoGetter provider_client_info_getter,
On 2017/06/19 07:50:40, falken wrote:
> rename

Done.

https://codereview.chromium.org/2779763004/diff/410001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance.h (right):

https://codereview.chromium.org/2779763004/diff/410001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.h:76: int)>;
On 2017/06/19 07:50:40, falken wrote:
> Rername to ProviderInfoGetter
> 
> int /* process_id */
> 

Done.

https://codereview.chromium.org/2779763004/diff/410001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.h:115: // to start the
worker. |provider_client_info_getter| will be used when this
On 2017/06/19 07:50:40, falken wrote:
> "|provider_info_getter| is called when this instance allocates..."

Done.

https://codereview.chromium.org/2779763004/diff/410001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.h:118:
ProviderClientInfoGetter provider_client_info_getter,
On 2017/06/19 07:50:40, falken wrote:
> Rename to provider_info_getter

Done.

https://codereview.chromium.org/2779763004/diff/410001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.h:306:
ProviderClientInfoGetter provider_client_info_getter_;
On 2017/06/19 07:50:40, falken wrote:
> ProviderInfoGetter provider_info_getter

Done.

https://codereview.chromium.org/2779763004/diff/410001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance_unittest.cc
(right):

https://codereview.chromium.org/2779763004/diff/410001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance_unittest.cc:126:
std::move(params), CreateProviderClientInfoGetter(),
On 2017/06/19 07:50:40, falken wrote:
> CreateProviderInfoGetter

Done.

https://codereview.chromium.org/2779763004/diff/410001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance_unittest.cc:145:
mojom::ServiceWorkerProviderInfoForStartWorkerPtr CreateProviderClientInfo(
On 2017/06/19 07:50:40, falken wrote:
> rename

Done.

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

https://codereview.chromium.org/2779763004/diff/410001/content/browser/servic...
content/browser/service_worker/service_worker_test_utils.h:66: void
BindWithProviderClientInfo(
On 2017/06/19 07:50:40, falken wrote:
> BindWithProviderInfo

Done.

https://codereview.chromium.org/2779763004/diff/410001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/2779763004/diff/410001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:211:
context->AddProviderHost(std::move(provider_host));
On 2017/06/19 07:50:40, falken wrote:
> Could context be null?
> 
> If not, we could DCHECK(context) and add a comment why it's never null.

Done.

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

https://codereview.chromium.org/2779763004/diff/410001/content/common/service...
content/common/service_worker/service_worker_provider.mojom:12: // This is
passed on launching a service worker.
On 2017/06/19 07:50:40, falken wrote:
> "for the constructor of ServiceWorkerNetworkProvider used for starting a
service
> worker" and delete the final sentence.

Done.

[falken, 2017-06-20 04:59:35]

LGTM

https://codereview.chromium.org/2779763004/diff/430001/content/browser/servic...
File content/browser/service_worker/service_worker_dispatcher_host.h (right):

https://codereview.chromium.org/2779763004/diff/430001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host.h:82: 
// Gets or creates the registration and version handles appropriate for
representing |registration| inside of |provider_host|. Sets |out_info| and
|out_attrs| accordingly for these handles.

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

https://codereview.chromium.org/2779763004/diff/430001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:683:
DCHECK_EQ(SERVICE_WORKER_PROVIDER_FOR_CONTROLLER, provider_type());
This is redundant with 680. I prefer 683 because we'd get a better error message
when it fails.

https://codereview.chromium.org/2779763004/diff/430001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:684:
DCHECK(is_parent_frame_secure());
I prefer deleting this DCHECK because is_parent_frame_secure() should be
irrelevant when !IsProviderForClient().

I think we should make this member a base::Optional or actually consider
splitting SWProviderHost into different classes for SW clients or SW execution
contexts. But that is definitely future work.

https://codereview.chromium.org/2779763004/diff/430001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:707: //
Initialize provider_client_info.
s/provider_client_info/provider_info

https://codereview.chromium.org/2779763004/diff/430001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:708:
mojom::ServiceWorkerProviderInfoForStartWorkerPtr provider_client_info =
s/provider_client_info/provider_info

https://codereview.chromium.org/2779763004/diff/430001/content/browser/servic...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/2779763004/diff/430001/content/browser/servic...
content/browser/service_worker/service_worker_version.h:745: // the running
context.
s/has the running context/is running

https://codereview.chromium.org/2779763004/diff/430001/content/child/service_...
File content/child/service_worker/service_worker_network_provider.h (right):

https://codereview.chromium.org/2779763004/diff/430001/content/child/service_...
content/child/service_worker/service_worker_network_provider.h:60: bool
is_parent_frame_secure);
// This is for service worker clients.

[shimazu, 2017-06-20 08:04:05]

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

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

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-06-20 08:06:45]

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

[commit-bot: I haz the power, 2017-06-20 08:06:48]

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

[dcheng, 2017-06-20 09:35:39]

https://codereview.chromium.org/2779763004/diff/450001/content/browser/servic...
File content/browser/service_worker/service_worker_job_unittest.cc (right):

https://codereview.chromium.org/2779763004/diff/450001/content/browser/servic...
content/browser/service_worker/service_worker_job_unittest.cc:406: new
KeepHandlesDispatcherHost(
Consider using auto dispatcher_host =
base::MakeRefCounted<KeepHandlesDispatcherHost>(...);

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

https://codereview.chromium.org/2779763004/diff/450001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:287: // careful
not to create a reference cycle.
Can we change this function to take a scoped_refptr<ServiceWorkerVersion>
hosted_version if it takes a reference?

(Followup is fine if it's easier)

https://codereview.chromium.org/2779763004/diff/450001/content/common/service...
File
content/common/service_worker/service_worker_installed_scripts_manager.mojom
(right):

https://codereview.chromium.org/2779763004/diff/450001/content/common/service...
content/common/service_worker/service_worker_installed_scripts_manager.mojom:9:
interface ServiceWorkerInstalledScriptsManager {
Please add a short interface-level comment (about what processes this
communciates between--i.e. browser -> renderer or renderer->browser), and what
the purpose is.

https://codereview.chromium.org/2779763004/diff/450001/content/common/service...
File content/common/service_worker/service_worker_types.mojom (right):

https://codereview.chromium.org/2779763004/diff/450001/content/common/service...
content/common/service_worker/service_worker_types.mojom:26: struct
ServiceWorkerVersionAttributes;
Please add some comments for these new structs as well to explain their purpose.

https://codereview.chromium.org/2779763004/diff/450001/content/renderer/servi...
File
content/renderer/service_worker/web_service_worker_installed_scripts_manager_impl.cc
(right):

https://codereview.chromium.org/2779763004/diff/450001/content/renderer/servi...
content/renderer/service_worker/web_service_worker_installed_scripts_manager_impl.cc:19:
}
Nit:  put  // namespace here ot make it easier to spot the end of a block.

https://codereview.chromium.org/2779763004/diff/450001/content/renderer/servi...
content/renderer/service_worker/web_service_worker_installed_scripts_manager_impl.cc:64:
mojo::ScopedDataPipeConsumerHandle meta_data) {}
Is this a "will implement in the future" method?

https://codereview.chromium.org/2779763004/diff/450001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerThread.cpp
(right):

https://codereview.chromium.org/2779763004/diff/450001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerThread.cpp:62:
return installed_scripts_manager_.get();
I'm probably missing something, but what code actually constructs this new
member?

[shimazu, 2017-06-20 10:47:08]

Patchset #23 (id:450001) has been deleted

[shimazu, 2017-06-20 10:48:17]

Ahh, so sorry, ps#23 is not for this CL. I wrongly uploaded that. 
PTAL at ps#22, thanks!

On 2017/06/20 09:35:39, dcheng wrote:
>
https://codereview.chromium.org/2779763004/diff/450001/content/browser/servic...
> File content/browser/service_worker/service_worker_job_unittest.cc (right):
> 
>
https://codereview.chromium.org/2779763004/diff/450001/content/browser/servic...
> content/browser/service_worker/service_worker_job_unittest.cc:406: new
> KeepHandlesDispatcherHost(
> Consider using auto dispatcher_host =
> base::MakeRefCounted<KeepHandlesDispatcherHost>(...);
> 
>
https://codereview.chromium.org/2779763004/diff/450001/content/browser/servic...
> File content/browser/service_worker/service_worker_provider_host.h (right):
> 
>
https://codereview.chromium.org/2779763004/diff/450001/content/browser/servic...
> content/browser/service_worker/service_worker_provider_host.h:287: // careful
> not to create a reference cycle.
> Can we change this function to take a scoped_refptr<ServiceWorkerVersion>
> hosted_version if it takes a reference?
> 
> (Followup is fine if it's easier)
> 
>
https://codereview.chromium.org/2779763004/diff/450001/content/common/service...
> File
> content/common/service_worker/service_worker_installed_scripts_manager.mojom
> (right):
> 
>
https://codereview.chromium.org/2779763004/diff/450001/content/common/service...
>
content/common/service_worker/service_worker_installed_scripts_manager.mojom:9:
> interface ServiceWorkerInstalledScriptsManager {
> Please add a short interface-level comment (about what processes this
> communciates between--i.e. browser -> renderer or renderer->browser), and what
> the purpose is.
> 
>
https://codereview.chromium.org/2779763004/diff/450001/content/common/service...
> File content/common/service_worker/service_worker_types.mojom (right):
> 
>
https://codereview.chromium.org/2779763004/diff/450001/content/common/service...
> content/common/service_worker/service_worker_types.mojom:26: struct
> ServiceWorkerVersionAttributes;
> Please add some comments for these new structs as well to explain their
purpose.
> 
>
https://codereview.chromium.org/2779763004/diff/450001/content/renderer/servi...
> File
>
content/renderer/service_worker/web_service_worker_installed_scripts_manager_impl.cc
> (right):
> 
>
https://codereview.chromium.org/2779763004/diff/450001/content/renderer/servi...
>
content/renderer/service_worker/web_service_worker_installed_scripts_manager_impl.cc:19:
> }
> Nit:  put  // namespace here ot make it easier to spot the end of a block.
> 
>
https://codereview.chromium.org/2779763004/diff/450001/content/renderer/servi...
>
content/renderer/service_worker/web_service_worker_installed_scripts_manager_impl.cc:64:
> mojo::ScopedDataPipeConsumerHandle meta_data) {}
> Is this a "will implement in the future" method?
> 
>
https://codereview.chromium.org/2779763004/diff/450001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerThread.cpp
> (right):
> 
>
https://codereview.chromium.org/2779763004/diff/450001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerThread.cpp:62:
> return installed_scripts_manager_.get();
> I'm probably missing something, but what code actually constructs this new
> member?

[dcheng, 2017-06-20 17:23:25]

On 2017/06/20 10:48:17, shimazu wrote:
> Ahh, so sorry, ps#23 is not for this CL. I wrongly uploaded that. 
> PTAL at ps#22, thanks!

I see. I'm re-reviewing now--but rather than deleting the bad patchset next
time, please just upload the correct one on top next time. It would have made
the re-review much simpler.

[dcheng, 2017-06-20 21:09:48]

Btw, it'll make the followup review simpler if there's no rebase between
responding to the comments, so I can just review the diff

(It looks like the latest PS is also not uploaded, since falken/kinuko had some
comments on PS22 which are marked as done, but I don't see anywhere)

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
File content/browser/service_worker/service_worker_context_unittest.cc (right):

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
content/browser/service_worker/service_worker_context_unittest.cc:616: new
ServiceWorkerRegistration(GURL("http://www.example.com/test/"),
Consider using base::MakeRefCounted here and below

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host.cc:974:
DCHECK_NE(SERVICE_WORKER_PROVIDER_FOR_CONTROLLER, info.type);
We shouldn't be DCHECKing things in IPC handlers--same comment for line 967,
especially on the browser side. It's hard for me to tell if this is OK, but from
what I can tell, it's possible for a bad renderer to violate this assumption,
right?

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

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:39: // Provider
host for navigation with PlzNavigate or service worker's context is
add "when" after "or", i.e. this should read "or when a service worker's context
is created"

(With PlzNavigate, will we always take this path?)

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:139: context,
nullptr));
Nit: use std::move() if passing WeakPtr by value.

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:203:
CHECK(render_process_id != ChildProcessHost::kInvalidUniqueID ||
Does this need to be a CHECK? The ctor is private, so all our factory creators
should be doing the right thing, right? So it seems like DCHECK is more
appropriate here.

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

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:290:
ServiceWorkerVersion* hosted_version);
Earlier comment about passing this as a scoped_refptr<ServiceWorkerVersion>
still applies -- it makes it obvious ownership is being retained here.

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

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
content/browser/service_worker/service_worker_test_utils.cc:32:
mojom::ServiceWorkerProviderClientInfo* info) {
FWIW, I think it's a little surprising that this is passed by pointer rather
than by value, but this is test code, so I don't feel strongly.

Passing move-only types by value makes it clear that the thing you're calling
consumes members from it, whereas passing by pointer makes it unclear what
happens to it.

https://codereview.chromium.org/2779763004/diff/370001/content/common/service...
File content/common/service_worker/service_worker_types.mojom (right):

https://codereview.chromium.org/2779763004/diff/370001/content/common/service...
content/common/service_worker/service_worker_types.mojom:26: struct
ServiceWorkerVersionAttributes;
Previous comment about documenting this struct remains (or at least provide a
pointer to where these are defined)

https://codereview.chromium.org/2779763004/diff/430001/content/browser/servic...
File content/browser/service_worker/service_worker_job_unittest.cc (right):

https://codereview.chromium.org/2779763004/diff/430001/content/browser/servic...
content/browser/service_worker/service_worker_job_unittest.cc:406: new
KeepHandlesDispatcherHost(
Comment from PS2 about using base::MakeRefCounted<> still applies here.

[shimazu, 2017-06-21 03:11:27]

Thanks a lot for your comments!
Uploaded ps#23 with addressing falken@ and dcheng@'s comments.

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
File content/browser/service_worker/service_worker_context_unittest.cc (right):

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
content/browser/service_worker/service_worker_context_unittest.cc:616: new
ServiceWorkerRegistration(GURL("http://www.example.com/test/"),
On 2017/06/20 21:09:47, dcheng wrote:
> Consider using base::MakeRefCounted here and below

Done.

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host.cc:974:
DCHECK_NE(SERVICE_WORKER_PROVIDER_FOR_CONTROLLER, info.type);
On 2017/06/20 21:09:47, dcheng wrote:
> We shouldn't be DCHECKing things in IPC handlers--same comment for line 967,
> especially on the browser side. It's hard for me to tell if this is OK, but
from
> what I can tell, it's possible for a bad renderer to violate this assumption,
> right?

Sorry, I probably don't understand what is the point of your concern. 
Could you tell me why we shouldn't be DCHECKing in IPC handlers and what's the
"bad renderer"? Basically this DCHECK won't be failed unless we don't put any
bugs in, so failing DCHECK indicates something is broken.
Also, I don't understand the difference between DCHECKing in IPC handlers and in
methods used by IPC handlers. Should both be restricted? or Should DCHECKing in
IPC handlers directly be avoided?

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

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:39: // Provider
host for navigation with PlzNavigate or service worker's context is
On 2017/06/20 21:09:47, dcheng wrote:
> add "when" after "or", i.e. this should read "or when a service worker's
context
> is created"
> 
> (With PlzNavigate, will we always take this path?)

Done.
Yes, PlzNavigate always assigns ids for provider hosts on the browser.

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:139: context,
nullptr));
On 2017/06/20 21:09:47, dcheng wrote:
> Nit: use std::move() if passing WeakPtr by value.

Done.

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:203:
CHECK(render_process_id != ChildProcessHost::kInvalidUniqueID ||
On 2017/06/20 21:09:47, dcheng wrote:
> Does this need to be a CHECK? The ctor is private, so all our factory creators
> should be doing the right thing, right? So it seems like DCHECK is more
> appropriate here.

I agree DCHECK is more appropriate here. 
I didn't change it because I guessed the reason why CHECKs related to
PlzNavigate are not DCHECKs is to track the causes of failures.

Changed to DCHECK since I have no longer seen any failures related to this
CHECK.

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

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.h:290:
ServiceWorkerVersion* hosted_version);
On 2017/06/20 21:09:47, dcheng wrote:
> Earlier comment about passing this as a scoped_refptr<ServiceWorkerVersion>
> still applies -- it makes it obvious ownership is being retained here.

Done.

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

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
content/browser/service_worker/service_worker_test_utils.cc:32:
mojom::ServiceWorkerProviderClientInfo* info) {
On 2017/06/20 21:09:47, dcheng wrote:
> FWIW, I think it's a little surprising that this is passed by pointer rather
> than by value, but this is test code, so I don't feel strongly.
> 
> Passing move-only types by value makes it clear that the thing you're calling
> consumes members from it, whereas passing by pointer makes it unclear what
> happens to it.

Thanks. I agree that passing move-only object by value is straightforward.
But in this case, I think it's the same pattern with "request =
mojo::MakeRequest(&ptr)". Is this weird?

https://codereview.chromium.org/2779763004/diff/370001/content/common/service...
File content/common/service_worker/service_worker_types.mojom (right):

https://codereview.chromium.org/2779763004/diff/370001/content/common/service...
content/common/service_worker/service_worker_types.mojom:26: struct
ServiceWorkerVersionAttributes;
On 2017/06/20 21:09:47, dcheng wrote:
> Previous comment about documenting this struct remains (or at least provide a
> pointer to where these are defined)

Done.

https://codereview.chromium.org/2779763004/diff/430001/content/browser/servic...
File content/browser/service_worker/service_worker_dispatcher_host.h (right):

https://codereview.chromium.org/2779763004/diff/430001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host.h:82: 
On 2017/06/20 04:59:34, falken wrote:
> // Gets or creates the registration and version handles appropriate for
> representing |registration| inside of |provider_host|. Sets |out_info| and
> |out_attrs| accordingly for these handles.

Done.

https://codereview.chromium.org/2779763004/diff/430001/content/browser/servic...
File content/browser/service_worker/service_worker_job_unittest.cc (right):

https://codereview.chromium.org/2779763004/diff/430001/content/browser/servic...
content/browser/service_worker/service_worker_job_unittest.cc:406: new
KeepHandlesDispatcherHost(
On 2017/06/20 21:09:48, dcheng wrote:
> Comment from PS2 about using base::MakeRefCounted<> still applies here.

Done.

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

https://codereview.chromium.org/2779763004/diff/430001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:683:
DCHECK_EQ(SERVICE_WORKER_PROVIDER_FOR_CONTROLLER, provider_type());
On 2017/06/20 04:59:35, falken wrote:
> This is redundant with 680. I prefer 683 because we'd get a better error
message
> when it fails.

Done.

https://codereview.chromium.org/2779763004/diff/430001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:684:
DCHECK(is_parent_frame_secure());
On 2017/06/20 04:59:35, falken wrote:
> I prefer deleting this DCHECK because is_parent_frame_secure() should be
> irrelevant when !IsProviderForClient().
> 
> I think we should make this member a base::Optional or actually consider
> splitting SWProviderHost into different classes for SW clients or SW execution
> contexts. But that is definitely future work.

Done.

https://codereview.chromium.org/2779763004/diff/430001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:707: //
Initialize provider_client_info.
On 2017/06/20 04:59:34, falken wrote:
> s/provider_client_info/provider_info

Done.

https://codereview.chromium.org/2779763004/diff/430001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:708:
mojom::ServiceWorkerProviderInfoForStartWorkerPtr provider_client_info =
On 2017/06/20 04:59:35, falken wrote:
> s/provider_client_info/provider_info

Done.

https://codereview.chromium.org/2779763004/diff/430001/content/browser/servic...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/2779763004/diff/430001/content/browser/servic...
content/browser/service_worker/service_worker_version.h:745: // the running
context.
On 2017/06/20 04:59:35, falken wrote:
> s/has the running context/is running

Done.

https://codereview.chromium.org/2779763004/diff/430001/content/child/service_...
File content/child/service_worker/service_worker_network_provider.h (right):

https://codereview.chromium.org/2779763004/diff/430001/content/child/service_...
content/child/service_worker/service_worker_network_provider.h:60: bool
is_parent_frame_secure);
On 2017/06/20 04:59:35, falken wrote:
> // This is for service worker clients.

Done.

[falken, 2017-06-21 03:26:21]

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host.cc:974:
DCHECK_NE(SERVICE_WORKER_PROVIDER_FOR_CONTROLLER, info.type);
On 2017/06/21 03:11:26, shimazu wrote:
> On 2017/06/20 21:09:47, dcheng wrote:
> > We shouldn't be DCHECKing things in IPC handlers--same comment for line 967,
> > especially on the browser side. It's hard for me to tell if this is OK, but
> from
> > what I can tell, it's possible for a bad renderer to violate this
assumption,
> > right?
> 
> Sorry, I probably don't understand what is the point of your concern. 
> Could you tell me why we shouldn't be DCHECKing in IPC handlers and what's the
> "bad renderer"? Basically this DCHECK won't be failed unless we don't put any
> bugs in, so failing DCHECK indicates something is broken.
> Also, I don't understand the difference between DCHECKing in IPC handlers and
in
> methods used by IPC handlers. Should both be restricted? or Should DCHECKing
in
> IPC handlers directly be avoided?

In Chromium the renderer process is considered untrusted. It's possible the
render process was hacked and sends bad IPC messages.

See
https://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc

Actually a long time ago, this said "DCHECK for IPC messages is fine", which I
always found strange, but it looks updated since then.

[shimazu, 2017-06-21 05:20:15]

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host.cc:974:
DCHECK_NE(SERVICE_WORKER_PROVIDER_FOR_CONTROLLER, info.type);
On 2017/06/21 03:26:20, falken wrote:
> On 2017/06/21 03:11:26, shimazu wrote:
> > On 2017/06/20 21:09:47, dcheng wrote:
> > > We shouldn't be DCHECKing things in IPC handlers--same comment for line
967,
> > > especially on the browser side. It's hard for me to tell if this is OK,
but
> > from
> > > what I can tell, it's possible for a bad renderer to violate this
> assumption,
> > > right?
> > 
> > Sorry, I probably don't understand what is the point of your concern. 
> > Could you tell me why we shouldn't be DCHECKing in IPC handlers and what's
the
> > "bad renderer"? Basically this DCHECK won't be failed unless we don't put
any
> > bugs in, so failing DCHECK indicates something is broken.
> > Also, I don't understand the difference between DCHECKing in IPC handlers
and
> in
> > methods used by IPC handlers. Should both be restricted? or Should DCHECKing
> in
> > IPC handlers directly be avoided?
> 
> In Chromium the renderer process is considered untrusted. It's possible the
> render process was hacked and sends bad IPC messages.
> 
> See
>
https://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc
> 
> Actually a long time ago, this said "DCHECK for IPC messages is fine", which I
> always found strange, but it looks updated since then.

That makes sense!
So we should use IF statements and bad_message::ReceivedBadMessage for checking
inputs of IPCs to ensure the inputs are valid because DCKECK won't work in
release builds, right?

Updated, thanks!

[falken, 2017-06-21 05:28:25]

On 2017/06/21 05:20:15, shimazu wrote:
>
https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
> File content/browser/service_worker/service_worker_dispatcher_host.cc (right):
> 
>
https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
> content/browser/service_worker/service_worker_dispatcher_host.cc:974:
> DCHECK_NE(SERVICE_WORKER_PROVIDER_FOR_CONTROLLER, info.type);
> On 2017/06/21 03:26:20, falken wrote:
> > On 2017/06/21 03:11:26, shimazu wrote:
> > > On 2017/06/20 21:09:47, dcheng wrote:
> > > > We shouldn't be DCHECKing things in IPC handlers--same comment for line
> 967,
> > > > especially on the browser side. It's hard for me to tell if this is OK,
> but
> > > from
> > > > what I can tell, it's possible for a bad renderer to violate this
> > assumption,
> > > > right?
> > > 
> > > Sorry, I probably don't understand what is the point of your concern. 
> > > Could you tell me why we shouldn't be DCHECKing in IPC handlers and what's
> the
> > > "bad renderer"? Basically this DCHECK won't be failed unless we don't put
> any
> > > bugs in, so failing DCHECK indicates something is broken.
> > > Also, I don't understand the difference between DCHECKing in IPC handlers
> and
> > in
> > > methods used by IPC handlers. Should both be restricted? or Should
DCHECKing
> > in
> > > IPC handlers directly be avoided?
> > 
> > In Chromium the renderer process is considered untrusted. It's possible the
> > render process was hacked and sends bad IPC messages.
> > 
> > See
> >
>
https://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc
> > 
> > Actually a long time ago, this said "DCHECK for IPC messages is fine", which
I
> > always found strange, but it looks updated since then.
> 
> That makes sense!
> So we should use IF statements and bad_message::ReceivedBadMessage for
checking
> inputs of IPCs to ensure the inputs are valid because DCKECK won't work in
> release builds, right?
> 
> Updated, thanks!

Yep, that lg to me.

[dcheng, 2017-06-21 05:45:06]

ipc lgtm, thanks!

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host.cc:974:
DCHECK_NE(SERVICE_WORKER_PROVIDER_FOR_CONTROLLER, info.type);
On 2017/06/21 05:20:13, shimazu wrote:
> On 2017/06/21 03:26:20, falken wrote:
> > On 2017/06/21 03:11:26, shimazu wrote:
> > > On 2017/06/20 21:09:47, dcheng wrote:
> > > > We shouldn't be DCHECKing things in IPC handlers--same comment for line
> 967,
> > > > especially on the browser side. It's hard for me to tell if this is OK,
> but
> > > from
> > > > what I can tell, it's possible for a bad renderer to violate this
> > assumption,
> > > > right?
> > > 
> > > Sorry, I probably don't understand what is the point of your concern. 
> > > Could you tell me why we shouldn't be DCHECKing in IPC handlers and what's
> the
> > > "bad renderer"? Basically this DCHECK won't be failed unless we don't put
> any
> > > bugs in, so failing DCHECK indicates something is broken.
> > > Also, I don't understand the difference between DCHECKing in IPC handlers
> and
> > in
> > > methods used by IPC handlers. Should both be restricted? or Should
DCHECKing
> > in
> > > IPC handlers directly be avoided?
> > 
> > In Chromium the renderer process is considered untrusted. It's possible the
> > render process was hacked and sends bad IPC messages.
> > 
> > See
> >
>
https://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc
> > 
> > Actually a long time ago, this said "DCHECK for IPC messages is fine", which
I
> > always found strange, but it looks updated since then.
> 
> That makes sense!
> So we should use IF statements and bad_message::ReceivedBadMessage for
checking
> inputs of IPCs to ensure the inputs are valid because DCKECK won't work in
> release builds, right?
> 
> Updated, thanks!
> 

Basically, it's two things:
- We want to make sure we handle invalid inputs from IPC.
- We also don't want to crash on invalid input from an IPC (since this would
kill Chrome).

It is occasionally OK to DCHECK in IPC message handlers, if we're asserting
about state that's not related to the IPC at all--but in general, DCHECK/CHECK
in IPC message handlers will be viewed with some suspicion =)

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

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
content/browser/service_worker/service_worker_test_utils.cc:32:
mojom::ServiceWorkerProviderClientInfo* info) {
On 2017/06/21 03:11:26, shimazu wrote:
> On 2017/06/20 21:09:47, dcheng wrote:
> > FWIW, I think it's a little surprising that this is passed by pointer rather
> > than by value, but this is test code, so I don't feel strongly.
> > 
> > Passing move-only types by value makes it clear that the thing you're
calling
> > consumes members from it, whereas passing by pointer makes it unclear what
> > happens to it.
> 
> Thanks. I agree that passing move-only object by value is straightforward.
> But in this case, I think it's the same pattern with "request =
> mojo::MakeRequest(&ptr)". Is this weird?

I guess it's a matter of semantics. With MakeRequest, I expect it to modify
|ptr| but I don't expect it to consume (and possibly null out) the value in
|ptr|. This contrasts with this helper, which is essentially consuming the
information passed in |info|.

(I don't feel strongly about this, it's just a general suggestion)

[shimazu, 2017-06-21 05:59:20]

The CQ bit was checked by shimazu@chromium.org

[shimazu, 2017-06-21 05:59:21]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
falken@chromium.org
Link to the patchset: https://codereview.chromium.org/2779763004/#ps490001
(title: "Use bad_message::ReceivedBadMessage instead of DCHECK")

[commit-bot: I haz the power, 2017-06-21 05:59:44]

CQ is trying da patch.

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

[shimazu, 2017-06-21 06:04:07]

The CQ bit was unchecked by shimazu@chromium.org

[shimazu, 2017-06-21 06:27:15]

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

https://codereview.chromium.org/2779763004/diff/370001/content/browser/servic...
content/browser/service_worker/service_worker_test_utils.cc:32:
mojom::ServiceWorkerProviderClientInfo* info) {
On 2017/06/21 05:45:06, dcheng wrote:
> On 2017/06/21 03:11:26, shimazu wrote:
> > On 2017/06/20 21:09:47, dcheng wrote:
> > > FWIW, I think it's a little surprising that this is passed by pointer
rather
> > > than by value, but this is test code, so I don't feel strongly.
> > > 
> > > Passing move-only types by value makes it clear that the thing you're
> calling
> > > consumes members from it, whereas passing by pointer makes it unclear what
> > > happens to it.
> > 
> > Thanks. I agree that passing move-only object by value is straightforward.
> > But in this case, I think it's the same pattern with "request =
> > mojo::MakeRequest(&ptr)". Is this weird?
> 
> I guess it's a matter of semantics. With MakeRequest, I expect it to modify
> |ptr| but I don't expect it to consume (and possibly null out) the value in
> |ptr|. This contrasts with this helper, which is essentially consuming the
> information passed in |info|.
> 
> (I don't feel strongly about this, it's just a general suggestion)

I see, unless the ProviderClientInfo is used on the caller side, it seems
simpler to pass the info. 
Updated.

[shimazu, 2017-06-21 06:28:09]

The CQ bit was checked by shimazu@chromium.org

[shimazu, 2017-06-21 06:28:10]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
kinuko@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2779763004/#ps510001
(title: "Pass the param of BindWithProviderInfo by value instead of pointer")

[commit-bot: I haz the power, 2017-06-21 06:28:30]

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-06-21 06:38:50]

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

[commit-bot: I haz the power, 2017-06-21 06:38:53]

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-06-21 08:27:50]

On 2017/06/21 06:38:53, commit-bot: I haz the power wrote:
> 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...)

Hmm...
https://crrev.com/2941623002 changed the expectation of enums.xml not to have
"type" attribute on the enums, but several metrics have it.
Reported a bug: https://crbug.com/735364

[kinuko, 2017-06-22 01:14:30]

On 2017/06/21 08:27:50, shimazu wrote:
> On 2017/06/21 06:38:53, commit-bot: I haz the power wrote:
> > 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...)
> 
> Hmm...
> https://crrev.com/2941623002 changed the expectation of enums.xml not to have
> "type" attribute on the enums, but several metrics have it.
> Reported a bug: https://crbug.com/735364

Any update on this? If you're absolutely sure the enums.xml is right (it looks)
could we just land this with NOPRESUBMIT?  Looks like this case applies:

https://dev.chromium.org/developers/testing/commit-queue
"This should only be used when there's a bug in the PRESUBMIT scripts. Please
check that there's a bug filed against the bad script, and if there isn't, file
one."

[kinuko, 2017-06-22 01:15:24]

On 2017/06/22 01:14:30, kinuko wrote:
> On 2017/06/21 08:27:50, shimazu wrote:
> > On 2017/06/21 06:38:53, commit-bot: I haz the power wrote:
> > > 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...)
> > 
> > Hmm...
> > https://crrev.com/2941623002 changed the expectation of enums.xml not to
have
> > "type" attribute on the enums, but several metrics have it.
> > Reported a bug: https://crbug.com/735364
> 
> Any update on this? If you're absolutely sure the enums.xml is right (it
looks)
> could we just land this with NOPRESUBMIT?  Looks like this case applies:
> 
> https://dev.chromium.org/developers/testing/commit-queue
> "This should only be used when there's a bug in the PRESUBMIT scripts. Please
> check that there's a bug filed against the bad script, and if there isn't,
file
> one."

(CC-ing isherman@ too so that he can take a look if necessary)

[shimazu, 2017-06-22 01:19:26]

On 2017/06/22 01:15:24, kinuko wrote:
> On 2017/06/22 01:14:30, kinuko wrote:
> > On 2017/06/21 08:27:50, shimazu wrote:
> > > On 2017/06/21 06:38:53, commit-bot: I haz the power wrote:
> > > > 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...)
> > > 
> > > Hmm...
> > > https://crrev.com/2941623002 changed the expectation of enums.xml not to
> have
> > > "type" attribute on the enums, but several metrics have it.
> > > Reported a bug: https://crbug.com/735364
> > 
> > Any update on this? If you're absolutely sure the enums.xml is right (it
> looks)
> > could we just land this with NOPRESUBMIT?  Looks like this case applies:
> > 
> > https://dev.chromium.org/developers/testing/commit-queue
> > "This should only be used when there's a bug in the PRESUBMIT scripts.
Please
> > check that there's a bug filed against the bad script, and if there isn't,
> file
> > one."
> 
> (CC-ing isherman@ too so that he can take a look if necessary)

https://crrev.com/2945333002 has been tried but not landed due to a patch error.
I'll try NOPRESUBMIT=true, thanks!

[shimazu, 2017-06-22 01:20:03]

Description was changed from

==========
Create ServiceWorkerProviderHost before starting worker

Currently ServiceWorkerProviderHost for a service worker's context is bound with
the corresponing provider on the hosted worker when ProviderCreated and
SetHostedVersion messages are getting back after sending the StartWorker
message. This patch is to removes the round trip.
This patch changes the procedure of creating the SWProviderHost like the
following.

1: Create a mock provider host when ServiceWorkerVersion::StartWorker is called.
2: Complete the rest of initialization for the provider host before sending
StartWorker message at
ServiceWorkerProviderHost::CompleteStartWorkerPreparation.
3: Send the infomation about the provider host to the renderer as an argument of
EmbeddedWorkerInstanceClient::StartWorker.
4: Create ServiceWorkerNetworkProvider on the renderer when
ServiceWorkerContextClient is constructed.

This patch also sovles a weird ordering issue between ProviderDestroyed and
SetHostedVersion, and between OnStopped and ProviderDestroyed.

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

to

==========
Create ServiceWorkerProviderHost before starting worker

Currently ServiceWorkerProviderHost for a service worker's context is bound with
the corresponing provider on the hosted worker when ProviderCreated and
SetHostedVersion messages are getting back after sending the StartWorker
message. This patch is to removes the round trip.
This patch changes the procedure of creating the SWProviderHost like the
following.

1: Create a mock provider host when ServiceWorkerVersion::StartWorker is called.
2: Complete the rest of initialization for the provider host before sending
StartWorker message at
ServiceWorkerProviderHost::CompleteStartWorkerPreparation.
3: Send the infomation about the provider host to the renderer as an argument of
EmbeddedWorkerInstanceClient::StartWorker.
4: Create ServiceWorkerNetworkProvider on the renderer when
ServiceWorkerContextClient is constructed.

This patch also sovles a weird ordering issue between ProviderDestroyed and
SetHostedVersion, and between OnStopped and ProviderDestroyed.

BUG=629701,676983,668633
NOPRESUBMIT=true
==========

[shimazu, 2017-06-22 01:20:19]

The CQ bit was checked by shimazu@chromium.org

[commit-bot: I haz the power, 2017-06-22 01:20:37]

CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-06-22 01:21:34]

CQ is committing da patch.

Bot data: {"patchset_id": 510001, "attempt_start_ts": 1498094419739470,
"parent_rev": "c8af805863f40f75271df364e73ad02c2f6a18f2", "commit_rev":
"1ebf3cb1014b2c85d4520e6dcb6a11d7001bc30f"}

[commit-bot: I haz the power, 2017-06-22 01:21:49]

Description was changed from

==========
Create ServiceWorkerProviderHost before starting worker

Currently ServiceWorkerProviderHost for a service worker's context is bound with
the corresponing provider on the hosted worker when ProviderCreated and
SetHostedVersion messages are getting back after sending the StartWorker
message. This patch is to removes the round trip.
This patch changes the procedure of creating the SWProviderHost like the
following.

1: Create a mock provider host when ServiceWorkerVersion::StartWorker is called.
2: Complete the rest of initialization for the provider host before sending
StartWorker message at
ServiceWorkerProviderHost::CompleteStartWorkerPreparation.
3: Send the infomation about the provider host to the renderer as an argument of
EmbeddedWorkerInstanceClient::StartWorker.
4: Create ServiceWorkerNetworkProvider on the renderer when
ServiceWorkerContextClient is constructed.

This patch also sovles a weird ordering issue between ProviderDestroyed and
SetHostedVersion, and between OnStopped and ProviderDestroyed.

BUG=629701,676983,668633
NOPRESUBMIT=true
==========

to

==========
Create ServiceWorkerProviderHost before starting worker

Currently ServiceWorkerProviderHost for a service worker's context is bound with
the corresponing provider on the hosted worker when ProviderCreated and
SetHostedVersion messages are getting back after sending the StartWorker
message. This patch is to removes the round trip.
This patch changes the procedure of creating the SWProviderHost like the
following.

1: Create a mock provider host when ServiceWorkerVersion::StartWorker is called.
2: Complete the rest of initialization for the provider host before sending
StartWorker message at
ServiceWorkerProviderHost::CompleteStartWorkerPreparation.
3: Send the infomation about the provider host to the renderer as an argument of
EmbeddedWorkerInstanceClient::StartWorker.
4: Create ServiceWorkerNetworkProvider on the renderer when
ServiceWorkerContextClient is constructed.

This patch also sovles a weird ordering issue between ProviderDestroyed and
SetHostedVersion, and between OnStopped and ProviderDestroyed.

BUG=629701,676983,668633
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2779763004
Cr-Commit-Position: refs/heads/master@{#481391}
Committed:
https://chromium.googlesource.com/chromium/src/+/1ebf3cb1014b2c85d4520e6dcb6a...
==========

[commit-bot: I haz the power, 2017-06-22 01:21:52]

Committed patchset #25 (id:510001) as
https://chromium.googlesource.com/chromium/src/+/1ebf3cb1014b2c85d4520e6dcb6a...