Wire provider_id into scriptable API provider to start listening events

Wire provider_id into scriptable API provider to start listening events for the document

- Wire provider_id to Scriptable-API ServiceWorkerProvider
- Send AddScriptClient/RemoveScriptClient message which is sent when navigator.serviceWorker is accessed (or detached)

We can use the clients information when we dispatch events to
controllee documents.

BUG=349919
TEST=will add browser tests after one more patch in blink

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255919

[kinuko, 2014-03-06 15:06:27]

Can you review? (I can add browser test if this looks good to you)

[michaeln, 2014-03-06 20:24:29]

https://codereview.chromium.org/182863004/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/182863004/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.h:44: const
std::set<int>& client_thread_ids() const { return client_thread_ids_; }
Shouldn't there only be one script client thread? This corresponds to the
WebCore context thread on which a Document/Worker is running and there is only
one of those.

I can see how we might get multiple instances of WebServiceWorkerProviderImpl
(depending), but they'll all be on the same "apartment thread" so to speak.

If that's not right, can you explain how we'll see multiple thread_ids here.

https://codereview.chromium.org/182863004/diff/20001/content/child/service_wo...
File content/child/service_worker/web_service_worker_provider_impl.h (right):

https://codereview.chromium.org/182863004/diff/20001/content/child/service_wo...
content/child/service_worker/web_service_worker_provider_impl.h:26: //
TODO(kinuko): This should be given the provider_id from the network
can remove the todo

[kinuko, 2014-03-07 00:22:23]

https://codereview.chromium.org/182863004/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/182863004/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.h:44: const
std::set<int>& client_thread_ids() const { return client_thread_ids_; }
On 2014/03/06 20:24:29, michaeln wrote:
> Shouldn't there only be one script client thread? This corresponds to the
> WebCore context thread on which a Document/Worker is running and there is only
> one of those.
> 
> I can see how we might get multiple instances of WebServiceWorkerProviderImpl
> (depending), but they'll all be on the same "apartment thread" so to speak.

In the CL the Add/Remove messages are sent from thread-specific dispatcher that
is created by WebServiceWorkerProviderImpl, and browser can directly send
messages to those script providers from the browser (where we can use these
thread IDs), that is my assumption.  If I rename this 'client' to 'script
provider threads' or something else will that become clearer, or probably you're
assuming different model?

[michaeln, 2014-03-07 01:04:51]

> In the CL the Add/Remove messages are sent from thread-specific dispatcher
that
> is created by WebServiceWorkerProviderImpl, and browser can directly send
> messages to those script providers from the browser (where we can use these
> thread IDs), that is my assumption.  If I rename this 'client' to 'script
> provider threads' or something else will that become clearer, or probably
you're
> assuming different model?

A providerID identifies a one-per-scriptcontext specific construct (per
document). Is there a case where two different WebServiceWorkerProviderImpls
live on two different threads for a single document?

Maybe your thinking of when dedicated workers are wired up to the scriptable api
and they glom onto their parents providerID?

[kinuko, 2014-03-07 01:06:18]

On 2014/03/07 01:04:51, michaeln wrote:
> > In the CL the Add/Remove messages are sent from thread-specific dispatcher
> that
> > is created by WebServiceWorkerProviderImpl, and browser can directly send
> > messages to those script providers from the browser (where we can use these
> > thread IDs), that is my assumption.  If I rename this 'client' to 'script
> > provider threads' or something else will that become clearer, or probably
> you're
> > assuming different model?
> 
> A providerID identifies a one-per-scriptcontext specific construct (per
> document). Is there a case where two different WebServiceWorkerProviderImpls
> live on two different threads for a single document?
> 
> Maybe your thinking of when dedicated workers are wired up to the scriptable
api
> and they glom onto their parents providerID?

Yes, that's what I was thinking.

[michaeln, 2014-03-07 01:26:22]

>> Maybe your thinking of when dedicated workers are wired up to
>> the scriptable api and they glom onto their parents providerID?
>
> Yes, that's what I was thinking.

Ok, got it. Maybe add a comment about how we can have multiple clients on
different threads when the main page contains dedicated workers.

https://codereview.chromium.org/182863004/diff/20001/content/child/service_wo...
File content/child/service_worker/service_worker_dispatcher.cc (right):

https://codereview.chromium.org/182863004/diff/20001/content/child/service_wo...
content/child/service_worker/service_worker_dispatcher.cc:93: client_ = client;
This class is used on the main thread where we'll have multiple script clients
for different documents so a singular client_ doesn't look right.

https://codereview.chromium.org/182863004/diff/20001/content/child/service_wo...
File content/child/service_worker/web_service_worker_provider_impl.cc (right):

https://codereview.chromium.org/182863004/diff/20001/content/child/service_wo...
content/child/service_worker/web_service_worker_provider_impl.cc:25:
WebServiceWorkerProviderImpl::~WebServiceWorkerProviderImpl() {
Do we need to send the reset message here to get the backend to forget about
this thread_id or are we guaranteed to have seen a setClient(NULL) before we get
here?

[michaeln, 2014-03-07 01:33:58]

https://codereview.chromium.org/182863004/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/182863004/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.h:34: // from
ServiceWorker to the document corresponding to this provider.
This comment could be massaged to mention the document and any of its dedicated
workers that may be listening for SW related events.

[kinuko, 2014-03-07 02:02:17]

https://codereview.chromium.org/182863004/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/182863004/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.h:34: // from
ServiceWorker to the document corresponding to this provider.
On 2014/03/07 01:33:58, michaeln wrote:
> This comment could be massaged to mention the document and any of its
dedicated
> workers that may be listening for SW related events.

Done.

https://codereview.chromium.org/182863004/diff/20001/content/child/service_wo...
File content/child/service_worker/service_worker_dispatcher.cc (right):

https://codereview.chromium.org/182863004/diff/20001/content/child/service_wo...
content/child/service_worker/service_worker_dispatcher.cc:93: client_ = client;
On 2014/03/07 01:26:23, michaeln wrote:
> This class is used on the main thread where we'll have multiple script clients
> for different documents so a singular client_ doesn't look right.

Oops, that's right, I have focused too much on worker cases. Fixed.

https://codereview.chromium.org/182863004/diff/20001/content/child/service_wo...
File content/child/service_worker/web_service_worker_provider_impl.cc (right):

https://codereview.chromium.org/182863004/diff/20001/content/child/service_wo...
content/child/service_worker/web_service_worker_provider_impl.cc:25:
WebServiceWorkerProviderImpl::~WebServiceWorkerProviderImpl() {
On 2014/03/07 01:26:23, michaeln wrote:
> Do we need to send the reset message here to get the backend to forget about
> this thread_id or are we guaranteed to have seen a setClient(NULL) before we
get
> here?

Done.  Would be better to ensure that.

[michaeln, 2014-03-07 02:24:09]

lgtm

https://codereview.chromium.org/182863004/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/182863004/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.cc:188: if
(!provider_host) {
This might not be indicative of a bad message depending on blink thread shutdown
ordering? I don't know how much we can trust blink about this?

https://codereview.chromium.org/182863004/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.cc:201: if
(!provider_host) {
ditto can we trust blink on this?

https://codereview.chromium.org/182863004/diff/40001/content/child/service_wo...
File content/child/service_worker/service_worker_dispatcher.cc (right):

https://codereview.chromium.org/182863004/diff/40001/content/child/service_wo...
content/child/service_worker/service_worker_dispatcher.cc:37: client_(NULL) {
ditto stale

https://codereview.chromium.org/182863004/diff/40001/content/child/service_wo...
File content/child/service_worker/service_worker_dispatcher.h (right):

https://codereview.chromium.org/182863004/diff/40001/content/child/service_wo...
content/child/service_worker/service_worker_dispatcher.h:85:
blink::WebServiceWorkerProviderClient* client_;
stale line to be deleted

[kinuko, 2014-03-07 02:37:13]

Thanks, updated and also removed stale TODO / debug logs.

https://codereview.chromium.org/182863004/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/182863004/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.cc:188: if
(!provider_host) {
On 2014/03/07 02:24:09, michaeln wrote:
> This might not be indicative of a bad message depending on blink thread
shutdown
> ordering? I don't know how much we can trust blink about this?

Hmm, good point.  Changed to just return in null case.

https://codereview.chromium.org/182863004/diff/40001/content/child/service_wo...
File content/child/service_worker/service_worker_dispatcher.cc (right):

https://codereview.chromium.org/182863004/diff/40001/content/child/service_wo...
content/child/service_worker/service_worker_dispatcher.cc:37: client_(NULL) {
On 2014/03/07 02:24:09, michaeln wrote:
> ditto stale

Done.

https://codereview.chromium.org/182863004/diff/40001/content/child/service_wo...
File content/child/service_worker/service_worker_dispatcher.h (right):

https://codereview.chromium.org/182863004/diff/40001/content/child/service_wo...
content/child/service_worker/service_worker_dispatcher.h:85:
blink::WebServiceWorkerProviderClient* client_;
On 2014/03/07 02:24:09, michaeln wrote:
> stale line to be deleted

Done.

[kinuko, 2014-03-07 02:40:24]

creis@: could you do owner review for content/renderer/render_frame_impl.cc?

tsepez@: could you do security review for service_worker_messages.h change?

[Tom Sepez, 2014-03-07 17:48:25]

Messages LGTM.

https://codereview.chromium.org/182863004/diff/80001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/182863004/diff/80001/content/renderer/render_...
content/renderer/render_frame_impl.cc:1017: int provider_id = provider
style nit: ? and : at end of previous lines.

[Charlie Reis, 2014-03-07 18:16:13]

render_frame_impl.cc LGTM with Tom's nit.

[kinuko, 2014-03-10 05:02:17]

https://codereview.chromium.org/182863004/diff/80001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/182863004/diff/80001/content/renderer/render_...
content/renderer/render_frame_impl.cc:1017: int provider_id = provider
On 2014/03/07 17:48:26, Tom Sepez wrote:
> style nit: ? and : at end of previous lines.

Done.

[kinuko, 2014-03-10 05:03:14]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2014-03-10 05:03:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/182863004/100001

[commit-bot: I haz the power, 2014-03-10 06:47:22]

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

[commit-bot: I haz the power, 2014-03-10 06:47:23]

Retried try job too often on win_rel for step(s) telemetry_perf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[kinuko, 2014-03-10 07:46:44]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2014-03-10 07:46:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/182863004/100001

[commit-bot: I haz the power, 2014-03-10 12:18:35]

Change committed as 255919