ServiceWorker: Enqueue state change events until the worker thread gets ready

ServiceWorker: Enqueue state change events until the worker thread gets ready

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

Queued events are flushed out when the worker thread gets ready and
the provider host is notified of the thread id.

BUG=437677
TEST=https://codereview.chromium.org/861743002/

Committed: https://crrev.com/b0e43f64ce0dd2932d8dfac0f0c37b0ac64de3be
Cr-Commit-Position: refs/heads/master@{#313512}

[nhiroki, 2015-01-27 14:10:58]

Patchset #1 (id:1) has been deleted

[nhiroki, 2015-01-27 14:11:03]

Patchset #1 (id:20001) has been deleted

[nhiroki, 2015-01-27 14:11:10]

Patchset #1 (id:40001) has been deleted

[nhiroki, 2015-01-27 14:11:15]

Patchset #1 (id:50001) has been deleted

[nhiroki, 2015-01-27 14:11:21]

Patchset #1 (id:70001) has been deleted

[nhiroki, 2015-01-27 14:11:27]

Patchset #1 (id:90001) has been deleted

[nhiroki, 2015-01-27 15:35:25]

Patchset #1 (id:110001) has been deleted

[nhiroki, 2015-01-27 16:08:20]

Patchset #1 (id:130001) has been deleted

[nhiroki, 2015-01-27 16:17:13]

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

[nhiroki, 2015-01-27 16:17:37]

PTAL

[kinuko, 2015-01-28 07:13:12]

lgtm, nit comments only

https://codereview.chromium.org/855383006/diff/190001/content/browser/service...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/855383006/diff/190001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:672: return; 
// The provider has already been destroyed.
Does this happen for ServiceWorker?

https://codereview.chromium.org/855383006/diff/190001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/855383006/diff/190001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:66: // the worker
context gets started via SetReadyToSend().
I think part of this comment should be probably in the header file?

https://codereview.chromium.org/855383006/diff/190001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:68:
render_thread_id_ = kDocumentMainThreadId;
Doing the opposite might feel more natural /easier to understand (i.e.
initializing this with kDocumentMainThreadId, setting kInvalid if frame_id is
MSG_ROUTING_NONE)

https://codereview.chromium.org/855383006/diff/190001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/855383006/diff/190001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.h:165: // Called
from ServiceWorkerRegistrationHandle.
Can we put a bit more descriptive comments for these SendXXX()?
('Called from YYY' is not really informative, and it'd be better to also mention
that this may queue up messages)

https://codereview.chromium.org/855383006/diff/190001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.h:181: void
SetReadyToSend(int render_thread_id);
SetReadyToSendMessagesToRenderer ? (or ToWorker, maybe, as it's only used for
SWs)

[falken, 2015-01-28 13:42:09]

looks good to me too with kinuko's comments

typo in CL description: s/desing/design

https://codereview.chromium.org/855383006/diff/190001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/855383006/diff/190001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.h:48: typedef
ServiceWorkerProviderHost self;
comment: This pattern doesn't look idiomatic in Chromium codebase (outside some
Service Worker and sync file system classes :) ... is it really worth the level
of indirection... and is it even ok per style guide to have this lowercase type?
I'm not strongly opposed, just seems a bit off.

https://codereview.chromium.org/855383006/diff/190001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.h:181: void
SetReadyToSend(int render_thread_id);
On 2015/01/28 07:13:11, kinuko wrote:
> SetReadyToSendMessagesToRenderer ? (or ToWorker, maybe, as it's only used for
> SWs)

+1, it'd be nice to have a more descriptive name and summarize the gist of the
design doc in a comment here so the motivation is understood: events for the
worker are queued up until the worker thread id is known.

[nhiroki, 2015-01-28 13:43:22]

Thank you! Updated.

https://codereview.chromium.org/855383006/diff/190001/content/browser/service...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/855383006/diff/190001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:672: return; 
// The provider has already been destroyed.
On 2015/01/28 07:13:11, kinuko wrote:
> Does this happen for ServiceWorker?

Good point. ProviderDestroyed message always arrives after this
WorkerScriptLoaded message, so |provider_host| must be alive here. Removed this
check.

https://codereview.chromium.org/855383006/diff/190001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/855383006/diff/190001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:66: // the worker
context gets started via SetReadyToSend().
On 2015/01/28 07:13:11, kinuko wrote:
> I think part of this comment should be probably in the header file?

Done.

https://codereview.chromium.org/855383006/diff/190001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:68:
render_thread_id_ = kDocumentMainThreadId;
On 2015/01/28 07:13:11, kinuko wrote:
> Doing the opposite might feel more natural /easier to understand (i.e.
> initializing this with kDocumentMainThreadId, setting kInvalid if frame_id is
> MSG_ROUTING_NONE)

Done.

https://codereview.chromium.org/855383006/diff/190001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/855383006/diff/190001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.h:165: // Called
from ServiceWorkerRegistrationHandle.
On 2015/01/28 07:13:11, kinuko wrote:
> Can we put a bit more descriptive comments for these SendXXX()?
> ('Called from YYY' is not really informative, and it'd be better to also
mention
> that this may queue up messages)

Done.

https://codereview.chromium.org/855383006/diff/190001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.h:181: void
SetReadyToSend(int render_thread_id);
On 2015/01/28 07:13:11, kinuko wrote:
> SetReadyToSendMessagesToRenderer ? (or ToWorker, maybe, as it's only used for
> SWs)

Done.

[nhiroki, 2015-01-28 13:43:59]

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

[nhiroki, 2015-01-28 13:43:59]

+mkwst@, can you review embedded_worker_messages.h ? Thanks.

[Mike West, 2015-01-28 14:04:17]

IPC LGTM.

[nhiroki, 2015-01-28 14:08:30]

Thank you for reviewing. Addressed falken@'s comments.

https://codereview.chromium.org/855383006/diff/190001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/855383006/diff/190001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.h:48: typedef
ServiceWorkerProviderHost self;
On 2015/01/28 13:42:09, falken wrote:
> comment: This pattern doesn't look idiomatic in Chromium codebase (outside
some
> Service Worker and sync file system classes :) ... is it really worth the
level
> of indirection... and is it even ok per style guide to have this lowercase
type?
> I'm not strongly opposed, just seems a bit off.

To wrap the line at 80 columns, I just copied it from other sites without
thinking. This seems no longer necessary, so I just removed.

https://codereview.chromium.org/855383006/diff/190001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.h:181: void
SetReadyToSend(int render_thread_id);
On 2015/01/28 13:42:09, falken wrote:
> On 2015/01/28 07:13:11, kinuko wrote:
> > SetReadyToSendMessagesToRenderer ? (or ToWorker, maybe, as it's only used
for
> > SWs)
> 
> +1, it'd be nice to have a more descriptive name and summarize the gist of the
> design doc in a comment here so the motivation is understood: events for the
> worker are queued up until the worker thread id is known.

Updated.

[nhiroki, 2015-01-28 14:09:32]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2015-01-28 14:10:02]

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

[commit-bot: I haz the power, 2015-01-28 15:00:10]

Committed patchset #6 (id:250001)

[commit-bot: I haz the power, 2015-01-28 15:00:59]

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