More scaffolding, add class ServiceWorkerProviderHost.

More scaffolding, add class ServiceWorkerProviderHost.

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

[michaeln, 2013-11-15 23:13:38]

ptal, another stubbed out class added to the mix

[kinuko, 2013-11-18 05:57:24]

lgtm % a few nits + one question (not directly related to this CL)

https://codereview.chromium.org/70533005/diff/1/content/child/service_worker/...
File content/child/service_worker/web_service_worker_provider_impl.cc (right):

https://codereview.chromium.org/70533005/diff/1/content/child/service_worker/...
content/child/service_worker/web_service_worker_provider_impl.cc:39: new
ServiceWorkerHostMsg_ProviderCreated(provider_id_));
Not directly related to this CL, but let me write this question here-- when we
create a new WorkerGlobalScope it needs to know the parent context's
ContentSecurityPolicy (usually can be obtained by
Document::contentSecurityPolicy).  Can I assume that  the information will be
registered via the ProviderHost too (so that it'll be available in the browser)?

https://codereview.chromium.org/70533005/diff/130001/content/browser/service_...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/70533005/diff/130001/content/browser/service_...
content/browser/service_worker/service_worker_context_core.cc:34:
ProcessToProvidersMap::iterator it1 = providers_map_.find(process_id);
const_iterator

https://codereview.chromium.org/70533005/diff/130001/content/browser/service_...
content/browser/service_worker/service_worker_context_core.cc:37:
ServiceWorkerProviderHostMap::iterator it2 = it1->second->find(provider_id);
ditto

https://codereview.chromium.org/70533005/diff/130001/content/browser/service_...
File content/browser/service_worker/service_worker_context_core.h (right):

https://codereview.chromium.org/70533005/diff/130001/content/browser/service_...
content/browser/service_worker/service_worker_context_core.h:42:
ServiceWorkerProviderHostMap* map);
nit: could this be const ptr ?

https://codereview.chromium.org/70533005/diff/130001/content/browser/service_...
content/browser/service_worker/service_worker_context_core.h:49: typedef
std::map<int, ServiceWorkerProviderHostMap*> ProcessToProvidersMap;
ditto

https://codereview.chromium.org/70533005/diff/130001/content/browser/service_...
File content/browser/service_worker/service_worker_dispatcher_host.h (right):

https://codereview.chromium.org/70533005/diff/130001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.h:54:
ServiceWorkerProviderHostMap providers_;
nit: can you comment this instance owns provider hosts (for the process)? (since
it's not obvious only from types)

https://codereview.chromium.org/70533005/diff/130001/content/browser/service_...
File content/browser/service_worker/service_worker_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/70533005/diff/130001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:88:
EXPECT_EQ(1UL, dispatcher_host->sent_messages_.size());
nit: ASSERT_EQ may be better to avoid crash in line 89

https://codereview.chromium.org/70533005/diff/130001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:111:
EXPECT_EQ(1UL, dispatcher_host->sent_messages_.size());
ditto

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

https://codereview.chromium.org/70533005/diff/130001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.h:24: typedef
std::map<int, ServiceWorkerProviderHost*> ServiceWorkerProviderHostMap;
This one's primarily for id to host.. correct?

[michaeln, 2013-11-18 20:18:17]

https://codereview.chromium.org/70533005/diff/130001/content/browser/service_...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/70533005/diff/130001/content/browser/service_...
content/browser/service_worker/service_worker_context_core.cc:34:
ProcessToProvidersMap::iterator it1 = providers_map_.find(process_id);
On 2013/11/18 05:57:24, kinuko wrote:
> const_iterator

Done.

https://codereview.chromium.org/70533005/diff/130001/content/browser/service_...
File content/browser/service_worker/service_worker_context_core.h (right):

https://codereview.chromium.org/70533005/diff/130001/content/browser/service_...
content/browser/service_worker/service_worker_context_core.h:42:
ServiceWorkerProviderHostMap* map);
On 2013/11/18 05:57:24, kinuko wrote:
> nit: could this be const ptr ?

Could be? Is the contract your suggesting that this class never alter the
'providerhosts'? I'm not sure that's a good contract. I think having
GetProviderHost return a const ptr would not be helpful as i expect the caller
of that method will want/need to invoke non-const methods.

https://codereview.chromium.org/70533005/diff/130001/content/browser/service_...
File content/browser/service_worker/service_worker_dispatcher_host.h (right):

https://codereview.chromium.org/70533005/diff/130001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.h:54:
ServiceWorkerProviderHostMap providers_;
On 2013/11/18 05:57:24, kinuko wrote:
> nit: can you comment this instance owns provider hosts (for the process)?
(since
> it's not obvious only from types)

Done.

https://codereview.chromium.org/70533005/diff/130001/content/browser/service_...
File content/browser/service_worker/service_worker_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/70533005/diff/130001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:88:
EXPECT_EQ(1UL, dispatcher_host->sent_messages_.size());
On 2013/11/18 05:57:24, kinuko wrote:
> nit: ASSERT_EQ may be better to avoid crash in line 89

done

i'm really not sure what the best practice is here? anticipating what unmet
expectations will lead to a crash and treating them as assertions seems hard to
get right?

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

https://codereview.chromium.org/70533005/diff/130001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.h:24: typedef
std::map<int, ServiceWorkerProviderHost*> ServiceWorkerProviderHostMap;
On 2013/11/18 05:57:24, kinuko wrote:
> This one's primarily for id to host.. correct?

yes, added a comment

[alecflett, 2013-11-18 23:40:21]

Overall I don't fully understand why the dispatcher host needs to own the map -
it seems like it makes coordinating creation/destruction of the context +
dispatcher host more complicated.

On a related note, do we need explicit destruction messages or is it sufficient
for the dispatcher host to just see when the channel arrives / goes away (i.e.
OnChannelConnected / OnChannelClosing or what have you) I'm not as familiar with
some of the startup/shutdown issues, so I don't know if there are good reasons
not to use those.

https://codereview.chromium.org/70533005/diff/390001/content/browser/service_...
File content/browser/service_worker/service_worker_context_core.h (right):

https://codereview.chromium.org/70533005/diff/390001/content/browser/service_...
content/browser/service_worker/service_worker_context_core.h:42:
ServiceWorkerProviderHostMap* map);
It seems strange that the dispatcher host is providing (and owning) the map - it
seems like this should all be done within the Context, and perhaps the context
just needs to know of the arrival/destruction of processes.

https://codereview.chromium.org/70533005/diff/390001/content/browser/service_...
File content/browser/service_worker/service_worker_dispatcher_host.h (right):

https://codereview.chromium.org/70533005/diff/390001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.h:35: friend class
BrowserThread;
Why is BrowserThread a friend?

[kinuko, 2013-11-19 00:55:56]

https://codereview.chromium.org/70533005/diff/130001/content/browser/service_...
File content/browser/service_worker/service_worker_context_core.h (right):

https://codereview.chromium.org/70533005/diff/130001/content/browser/service_...
content/browser/service_worker/service_worker_context_core.h:42:
ServiceWorkerProviderHostMap* map);
On 2013/11/18 20:18:17, michaeln wrote:
> On 2013/11/18 05:57:24, kinuko wrote:
> > nit: could this be const ptr ?
> 
> Could be? Is the contract your suggesting that this class never alter the
> 'providerhosts'? I'm not sure that's a good contract. I think having
> GetProviderHost return a const ptr would not be helpful as i expect the caller
> of that method will want/need to invoke non-const methods.

Does this class alter the given map?  Probably you're talking about this class
(or this class's customers) going to call non-const methods of providerhost's
while I was concerned if this class alters the map?

[michaeln, 2013-11-19 01:14:26]

> OnChannelConnected / OnChannelClosing

I'm relying on the ctor/dtor for that so there's no opportunity for the object
to be in a partially torn down state and there's no dependence on those
particular overrides being called even.

https://codereview.chromium.org/70533005/diff/390001/content/browser/service_...
File content/browser/service_worker/service_worker_context_core.h (right):

https://codereview.chromium.org/70533005/diff/390001/content/browser/service_...
content/browser/service_worker/service_worker_context_core.h:42:
ServiceWorkerProviderHostMap* map);
On 2013/11/18 23:40:22, alecflett wrote:
> It seems strange that the dispatcher host is providing (and owning) the map -
it
> seems like this should all be done within the Context, and perhaps the context
> just needs to know of the arrival/destruction of processes.

Seems like 6 vs half-a-dozen?

https://codereview.chromium.org/70533005/diff/390001/content/browser/service_...
File content/browser/service_worker/service_worker_dispatcher_host.h (right):

https://codereview.chromium.org/70533005/diff/390001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.h:35: friend class
BrowserThread;
On 2013/11/18 23:40:22, alecflett wrote:
> Why is BrowserThread a friend?

for BrowserThread::DeleteOnIOThread::Destruct(this)

[alecflett, 2013-11-19 02:00:51]

On 2013/11/19 01:14:26, michaeln wrote:
> > OnChannelConnected / OnChannelClosing
> 
> I'm relying on the ctor/dtor for that so there's no opportunity for the object
> to be in a partially torn down state and there's no dependence on those
> particular overrides being called even.
> 
>
https://codereview.chromium.org/70533005/diff/390001/content/browser/service_...
> File content/browser/service_worker/service_worker_context_core.h (right):
> 
>
https://codereview.chromium.org/70533005/diff/390001/content/browser/service_...
> content/browser/service_worker/service_worker_context_core.h:42:
> ServiceWorkerProviderHostMap* map);
> On 2013/11/18 23:40:22, alecflett wrote:
> > It seems strange that the dispatcher host is providing (and owning) the map
-
> it
> > seems like this should all be done within the Context, and perhaps the
context
> > just needs to know of the arrival/destruction of processes.
> 
> Seems like 6 vs half-a-dozen?

I can't say I agree, it seems very different to me. I think what I take issue
with is that ServiceWorkerContext has a get-like accessor:
GetProviderHost() but the API provides no obvious equivalent setter (aside from
assigning/removing the map) - i.e. looking at the API, it's not obvious to me
that the map is altered during the lifetime of ServiceWorkerContextCore.

One dependency I was trying to maintain was that ServiceWorkerContext[Core]
doesn't depend on ServiceworkerDispatcherHost.. it doesn't depend on it
explicitly here, but it this does introduce an implicit contract that the caller
to Attach*/Detach* is the one altering the map.. it also requires that the
caller to those methods outlive ServiceWorkerContext which seems strange to me.

> 
>
https://codereview.chromium.org/70533005/diff/390001/content/browser/service_...
> File content/browser/service_worker/service_worker_dispatcher_host.h (right):
> 
>
https://codereview.chromium.org/70533005/diff/390001/content/browser/service_...
> content/browser/service_worker/service_worker_dispatcher_host.h:35: friend
class
> BrowserThread;
> On 2013/11/18 23:40:22, alecflett wrote:
> > Why is BrowserThread a friend?
> 
> for BrowserThread::DeleteOnIOThread::Destruct(this)

[michaeln, 2013-11-19 23:10:27]

> but the API provides no obvious equivalent setter

Yes, thats intentional. I expect there will be many callers of the getter...
whereas there is no need for a setter.

> One dependency I was trying to maintain was that ServiceWorkerContext
> doesn't depend on ServiceWorkerDispatcherHost

Me too, and this CL maintains that. 

> it also requires that the caller to those methods outlive ServiceWorkerContext
> which seems strange to me.

There is no such requirement? In the usual case, the context outlives everything
else.

[michaeln, 2013-11-20 23:23:24]

I've swapped things around such that the context now "owns" the provider hosts.

[alecflett, 2013-11-21 00:54:23]

lgtm

[kinuko, 2013-11-21 02:29:31]

The new code still lgtm2

[michaeln, 2013-11-21 22:37:07]

@brettw, git-cl suggested you do an owner review

please see patch set 10 for diffs, git-cl upload yakked on uploading 11?

[brettw, 2013-11-21 22:40:46]

Shared stuff lgtm

[michaeln, 2013-11-21 22:45:48]

thnx brett

@jln, git-cl also suggested that you do an owner review for the messages.h file

[jln (very slow on Chromium), 2013-11-21 23:09:29]

On 2013/11/21 22:45:48, michaeln wrote:
> thnx brett
> 
> @jln, git-cl also suggested that you do an owner review for the messages.h
file

content/common/service_worker_messages.h lgtm

[kinuko, 2013-11-22 02:03:19]

Ready to submit? I'm clicking CQ

[commit-bot: I haz the power, 2013-11-22 02:04:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaeln@chromium.org/70533005/840001

[commit-bot: I haz the power, 2013-11-22 03:27:34]

Retried try job too often on android_aosp for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao...

[kinuko, 2013-11-22 04:13:09]

https://codereview.chromium.org/70533005/diff/840001/content/browser/service_...
File content/browser/service_worker/service_worker_context_core.h (right):

https://codereview.chromium.org/70533005/diff/840001/content/browser/service_...
content/browser/service_worker/service_worker_context_core.h:10: #include
"base/memory/weak_ptr.h"
looks like you need to include base/memory/scoped_ptr.h here

[commit-bot: I haz the power, 2013-11-22 21:12:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaeln@chromium.org/70533005/1240001

[commit-bot: I haz the power, 2013-11-22 21:15:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaeln@chromium.org/70533005/1330001

[commit-bot: I haz the power, 2013-11-22 23:37:10]

Change committed as 236872