ServiceWorker: Expose registration within ServiceWorkerGlobalScope [2/3]

ServiceWorker: Expose registration within ServiceWorkerGlobalScope [2/3]

This CL supports the step 1 and 2 described in the desing doc:
https://docs.google.com/document/d/1qDGbMlwKOXxCRBlw9IirK8Qmna3JqvnO3Aogkfu9UJQ/edit?usp=sharing

- Makes ServiceWorkerProviderContext available on both main/worker threads
by the lock mechanism so that the context can retain a registration
reference for the worker until the worker thread gets ready.

- Creates registration object on the worker thread and sets it in
".registration" attribute on SWGlobalScope before evaluating the worker script.

[1] Blink: https://codereview.chromium.org/811793002/
[2] Chromium: THIS PATCH
[3] Blink: https://codereview.chromium.org/822593003/

BUG=437677
TEST=to be added by [3/3]

Committed: https://crrev.com/2d92972b8b5e90fb3036fe67a2fc8b61a12abb91
Cr-Commit-Position: refs/heads/master@{#313044}

[nhiroki, 2015-01-15 11:50:35]

Patchset #1 (id:1) has been deleted

[nhiroki, 2015-01-15 11:50:40]

Patchset #1 (id:20001) has been deleted

[nhiroki, 2015-01-15 11:50:47]

Patchset #1 (id:40001) has been deleted

[nhiroki, 2015-01-16 06:49:52]

Patchset #1 (id:60001) has been deleted

[nhiroki, 2015-01-19 01:55:17]

Patchset #1 (id:80001) has been deleted

[nhiroki, 2015-01-19 01:58:28]

Patchset #1 (id:100001) has been deleted

[nhiroki, 2015-01-19 01:58:49]

nhiroki@chromium.org changed reviewers:
+ falken@chromium.org, kinuko@chromium.org

[nhiroki, 2015-01-19 01:59:10]

Hi, can you review this? Thanks!

[nhiroki, 2015-01-19 06:27:04]

On 2015/01/19 01:59:10, nhiroki wrote:
> Hi, can you review this? Thanks!

Hmmm... this may need more modifications to support the step 3 described in the
CL description, so let me suspend this for now.

[nhiroki, 2015-01-20 10:57:03]

Let me resume this CL... can you take another look?

On 2015/01/19 06:27:04, nhiroki wrote:
> On 2015/01/19 01:59:10, nhiroki wrote:
> > Hi, can you review this? Thanks!
> 
> Hmmm... this may need more modifications to support the step 3 described in
the
> CL description, so let me suspend this for now.

I'll modify the step 3 ("forwarding the worker/registration events from the main
thread to the worker thread"). Instead of forwarding messages, the browser
process will directly send messages to the worker thread. Please see subsequent
CLs[1, 2] for more details. I'll update the design doc soon.

[1] https://codereview.chromium.org/825383004/
[2] https://codereview.chromium.org/855383006/

[kinuko, 2015-01-22 08:05:45]

lgtm

https://codereview.chromium.org/849163002/diff/140001/content/child/service_w...
File content/child/service_worker/service_worker_provider_context.h (right):

https://codereview.chromium.org/849163002/diff/140001/content/child/service_w...
content/child/service_worker/service_worker_provider_context.h:96: base::Lock
lock_;
It's unfortunate that we still need a lock while the fields aren't really racy
any more... I really hoped there could be a point where we can simply hand over
the context from main thread to worker thread.

https://codereview.chromium.org/849163002/diff/140001/content/renderer/servic...
File content/renderer/service_worker/embedded_worker_context_client.cc (right):

https://codereview.chromium.org/849163002/diff/140001/content/renderer/servic...
content/renderer/service_worker/embedded_worker_context_client.cc:444:
provider_context_->GetVersionAttributes();
It feels it's a bit odd to call two locked methods (registration() and
GetVersionAttributes()) separately, though it should be ok

[kinuko, 2015-01-22 08:14:00]

https://codereview.chromium.org/849163002/diff/140001/content/child/service_w...
File content/child/service_worker/service_worker_dispatcher.cc (right):

https://codereview.chromium.org/849163002/diff/140001/content/child/service_w...
content/child/service_worker/service_worker_dispatcher.cc:305: void
ServiceWorkerDispatcher::OnAssociateRegistrationWithServiceWorker(
I think there's a strong assumption that this should arrive before the worker
context starts (and in the current implementation it should be guaranteed as
this message should come before msgs for script loading), right?  Could we add a
brief comment about that?

[nhiroki, 2015-01-22 09:09:16]

Thank you for reviewing!

https://codereview.chromium.org/849163002/diff/140001/content/child/service_w...
File content/child/service_worker/service_worker_dispatcher.cc (right):

https://codereview.chromium.org/849163002/diff/140001/content/child/service_w...
content/child/service_worker/service_worker_dispatcher.cc:305: void
ServiceWorkerDispatcher::OnAssociateRegistrationWithServiceWorker(
On 2015/01/22 08:14:00, kinuko wrote:
> I think there's a strong assumption that this should arrive before the worker
> context starts (and in the current implementation it should be guaranteed as
> this message should come before msgs for script loading), right?

That's right.

Ideally, these info and attrs are pushed into EmbeddedWorkerMsg_StartWorker
message and directly handled by EmbeddedWorkerContextClient without the
indirection of ServiceWorkerProviderContext, but probably that needs a bit wide
and complex changes in the browser-side (how to get a registration handle before
creating SWNetworkProvider, etc). I'll revisit it later.

> Could we add a brief comment about that?

Added.

https://codereview.chromium.org/849163002/diff/140001/content/child/service_w...
File content/child/service_worker/service_worker_provider_context.h (right):

https://codereview.chromium.org/849163002/diff/140001/content/child/service_w...
content/child/service_worker/service_worker_provider_context.h:96: base::Lock
lock_;
On 2015/01/22 08:05:44, kinuko wrote:
> It's unfortunate that we still need a lock while the fields aren't really racy
> any more... I really hoped there could be a point where we can simply hand
over
> the context from main thread to worker thread.

Piggyback on StartWorker message could be a solution (see my other comment).
I'll try it later.

https://codereview.chromium.org/849163002/diff/140001/content/renderer/servic...
File content/renderer/service_worker/embedded_worker_context_client.cc (right):

https://codereview.chromium.org/849163002/diff/140001/content/renderer/servic...
content/renderer/service_worker/embedded_worker_context_client.cc:444:
provider_context_->GetVersionAttributes();
On 2015/01/22 08:05:44, kinuko wrote:
> It feels it's a bit odd to call two locked methods (registration() and
> GetVersionAttributes()) separately, though it should be ok

Agree. I'll make a followup patch (probably we can merge them?)

[nhiroki, 2015-01-22 09:10:46]

nhiroki@chromium.org changed reviewers:
+ mkwst@chromium.org

[nhiroki, 2015-01-22 09:10:47]

+mkwst@: Can you take a look at service_worker_message.h? Thanks!

[Mike West, 2015-01-22 10:11:27]

IPC LGTM.

[nhiroki, 2015-01-22 10:21:55]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2015-01-22 10:22:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/849163002/180001

[commit-bot: I haz the power, 2015-01-22 10:31:39]

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

[commit-bot: I haz the power, 2015-01-22 10:31:41]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[nhiroki, 2015-01-22 11:28:44]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2015-01-22 11:28:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/849163002/200001

[nhiroki, 2015-01-22 12:05:47]

The CQ bit was unchecked by nhiroki@chromium.org

[nhiroki, 2015-01-22 17:22:18]

Patchset #6 (id:220001) has been deleted

[nhiroki, 2015-01-22 23:02:22]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2015-01-22 23:05:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/849163002/240001

[commit-bot: I haz the power, 2015-01-23 00:28:13]

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

[commit-bot: I haz the power, 2015-01-23 00:28:14]

Try jobs failed on following builders:
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nhiroki, 2015-01-26 04:36:00]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2015-01-26 04:36:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/849163002/280001

[commit-bot: I haz the power, 2015-01-26 05:32:19]

Committed patchset #8 (id:280001)

[commit-bot: I haz the power, 2015-01-26 05:33:07]

Patchset 8 (id:??) landed as
https://crrev.com/2d92972b8b5e90fb3036fe67a2fc8b61a12abb91
Cr-Commit-Position: refs/heads/master@{#313044}