ServiceWorker: Carve out methods of ServiceWorkerProviderContext to delegate classes

ServiceWorker: Carve out methods of ServiceWorkerProviderContext to delegate classes

This is a refactoring CL for ServiceWorkerProviderContext.

ServiceWorkerProviderContext is used for a variety of things:

For a controlled document, it's used for keeping a controller alive until
ServiceWorkerContainer is created. For ServiceWorkerGlobalScope, it's used for
keeping an associated registration and its versions alive until the worker
context is initialized. This mixed use impairs the readability.

This CL carves them out to delegate classes to improve readability. In addition,
this removes a lock to protect service worker versions accessed from both the
main thread and the worker thread (this access pattern can happen only when a
provider context is created for ServiceWorkerGlobalScope). Actually the lock was
not necessary because ownership of the versions are transferred from the main
thread to the worker thread after registration association is done.

BUG=532098
Committed: https://crrev.com/cd2a5a60f93a81f8cec4171f048de2e54250b0f2
Cr-Commit-Position: refs/heads/master@{#352807}

[nhiroki, 2015-09-17 07:54:52]

Patchset #1 (id:1) has been deleted

[nhiroki, 2015-09-17 07:55:01]

Patchset #1 (id:20001) has been deleted

[nhiroki, 2015-09-17 07:55:11]

Patchset #1 (id:40001) has been deleted

[nhiroki, 2015-09-17 07:55:17]

Patchset #1 (id:60001) has been deleted

[nhiroki, 2015-09-17 07:55:24]

Patchset #1 (id:80001) has been deleted

[nhiroki, 2015-09-17 07:55:33]

Patchset #1 (id:100001) has been deleted

[nhiroki, 2015-09-17 07:55:41]

Patchset #1 (id:120001) has been deleted

[nhiroki, 2015-09-17 07:55:48]

Patchset #1 (id:140001) has been deleted

[nhiroki, 2015-09-17 08:14:17]

Patchset #1 (id:160001) has been deleted

[nhiroki, 2015-09-17 08:18:59]

Patchset #1 (id:180001) has been deleted

[nhiroki, 2015-09-17 08:22:00]

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

[nhiroki, 2015-09-17 08:22:01]

PTAL when you have time (just a refactoring CL)

[nhiroki, 2015-09-28 01:44:34]

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

[nhiroki, 2015-09-28 01:44:34]

-kinuko@ and +falken@ because she might be busy now.

falken@, can you review this?

[kinuko (google), 2015-09-28 02:24:14]

Sorry hiroki for slow review. One minor nit I noticed: probably 'curve out' ->
'carve out' ?

It looks ProviderContext's role significantly varies for controller and
controllee.  Would we be able to try having a better class comment for
ServiceWorkerProviderContext to summarize the current situation?

[nhiroki, 2015-09-29 02:08:49]

Thank you for reviewing!

On 2015/09/28 02:24:14, kinuko (google) wrote:
> Sorry hiroki for slow review. One minor nit I noticed: probably 'curve out' ->
> 'carve out' ?

Oh... fixed.

> It looks ProviderContext's role significantly varies for controller and
> controllee.  Would we be able to try having a better class comment for
> ServiceWorkerProviderContext to summarize the current situation?

Added comments.

[falken, 2015-10-07 06:39:13]

Looks like a nice refactoring.

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

https://codereview.chromium.org/1344783002/diff/240001/content/child/service_...
content/child/service_worker/service_worker_provider_context.h:31: //  - For
controllees, this is used for keeping the controller service worker
nit: keeping the <noun> alive

https://codereview.chromium.org/1344783002/diff/240001/content/child/service_...
content/child/service_worker/service_worker_provider_context.h:33: //  - For
controllers, this is used for keeping the associated registration and
nit: ditto

https://codereview.chromium.org/1344783002/diff/240001/content/child/service_...
content/child/service_worker/service_worker_provider_context.h:34: //    its
versions until controller's ServiceWorkerGlobalScope is created.
Thanks for the comments. Can you also add comments about the lifetime of this
class? Once the SWContainer or SWGlobalScope are created, does this class still
stay alive (and keep refptrs to the things it keeps alive) or does it die
because it's no longer needed?

https://codereview.chromium.org/1344783002/diff/240001/content/child/service_...
content/child/service_worker/service_worker_provider_context.h:45: // Called
from ServiceWorkerDispatcher on the main thread.
nit: We should either annotate each function with "called on the ___ thread" or
say "all methods are called on the main thread" in the class comment, but not do
both. It erodes trust in the comment.

[nhiroki, 2015-10-07 07:50:38]

Thank you for reviewing! Updated.

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

https://codereview.chromium.org/1344783002/diff/240001/content/child/service_...
content/child/service_worker/service_worker_provider_context.h:31: //  - For
controllees, this is used for keeping the controller service worker
On 2015/10/07 06:39:13, falken wrote:
> nit: keeping the <noun> alive

Done.

https://codereview.chromium.org/1344783002/diff/240001/content/child/service_...
content/child/service_worker/service_worker_provider_context.h:33: //  - For
controllers, this is used for keeping the associated registration and
On 2015/10/07 06:39:13, falken wrote:
> nit: ditto

Done.

https://codereview.chromium.org/1344783002/diff/240001/content/child/service_...
content/child/service_worker/service_worker_provider_context.h:34: //    its
versions until controller's ServiceWorkerGlobalScope is created.
On 2015/10/07 06:39:13, falken wrote:
> Thanks for the comments. Can you also add comments about the lifetime of this
> class? Once the SWContainer or SWGlobalScope are created, does this class
still
> stay alive (and keep refptrs to the things it keeps alive) or does it die
> because it's no longer needed?

Done.

https://codereview.chromium.org/1344783002/diff/240001/content/child/service_...
content/child/service_worker/service_worker_provider_context.h:45: // Called
from ServiceWorkerDispatcher on the main thread.
On 2015/10/07 06:39:13, falken wrote:
> nit: We should either annotate each function with "called on the ___ thread"
or
> say "all methods are called on the main thread" in the class comment, but not
do
> both. It erodes trust in the comment.

Point taken. Removed.

[falken, 2015-10-07 07:55:48]

lgtm

[nhiroki, 2015-10-07 08:36:06]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2015-10-07 08:37:55]

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

[commit-bot: I haz the power, 2015-10-07 09:25:39]

Committed patchset #5 (id:280001)

[commit-bot: I haz the power, 2015-10-07 09:26:30]

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