service worker scaffolding

ServiceWorker scaffolding.

Refactor the 'context' class to have a thread-safe refcounted wrapper for use by higher level chromium code, and a single-threaded nonrefcounted core for use in the service worker lib. Added explicit Init() and Shutdown() methods on the wrapper class which are used by StoragePartitionImpl to get things started and cleaned up.

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

[michaeln, 2013-11-07 00:28:37]

changes like we've talked about...

i'm waiting for alec's change to make its way thru the CQ and then i can
rebase...
https://codereview.chromium.org/26442004/

[kinuko, 2013-11-07 03:02:59]

k, I think this looks good.  Some nits (nits only)

https://codereview.chromium.org/61023005/diff/90001/content/browser/service_w...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/61023005/diff/90001/content/browser/service_w...
content/browser/service_worker/service_worker_context_core.cc:22: path_ =
user_data_directory.Append(kServiceWorkerDirectory);
I know this is coming from existing code, but one question: does this line mean
we may have a conditional like if (path_.empty()) { ... } in the code body?  I
feel just initializing path_ without checking empty() is fine (or even
preferable).

https://codereview.chromium.org/61023005/diff/90001/content/browser/service_w...
File content/browser/service_worker/service_worker_context_core.h (right):

https://codereview.chromium.org/61023005/diff/90001/content/browser/service_w...
content/browser/service_worker/service_worker_context_core.h:27: : public
base::SupportsWeakPtr<ServiceWorkerContextCore> {
We're intermittently having 'kill SupportsWeakPtr' threads on chromium-dev.  I
think our usecase is fine, but maybe better note a comment about why we're using
this and this weakptr's visibility is basically 'private' within the service
worker lib?

https://codereview.chromium.org/61023005/diff/90001/content/browser/service_w...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/61023005/diff/90001/content/browser/service_w...
content/browser/service_worker/service_worker_dispatcher_host.cc:24:
context_wrapper_ = NULL;
Hmm... ok I see.

https://codereview.chromium.org/61023005/diff/90001/content/browser/service_w...
File content/browser/service_worker/service_worker_dispatcher_host.h (right):

https://codereview.chromium.org/61023005/diff/90001/content/browser/service_w...
content/browser/service_worker/service_worker_dispatcher_host.h:41:
scoped_refptr<ServiceWorkerContextWrapper> context_wrapper_;
Maybe worth adding some comment to note that this wrapper is used only during
initialization alive only until OnFilterAdded() is called?

[alecflett, 2013-11-07 22:46:55]

I'm not sure why the Wrapper class is working so hard to make the ContextCore
class lazily-created? Why can't it just create it in its constructor, so that it
doesn't have to hold on to a bunch of duplicate member variables? I feel like
there is a lot of complexity around stuff like OnFilterAdded()?

[michaeln, 2013-11-07 23:24:53]

On 2013/11/07 22:46:55, alecflett wrote:
> I'm not sure why the Wrapper class is working so hard to make the ContextCore
> class lazily-created? Why can't it just create it in its constructor, so that
it
> doesn't have to hold on to a bunch of duplicate member variables? I feel like
> there is a lot of complexity around stuff like OnFilterAdded()?

I don't like the busy-ness either. It's arranged like that so the core class
really is single threaded, including its construction. I'll nix the lazy
construction and new it on the UI thread and give up on that "really" part.

https://codereview.chromium.org/61023005/diff/90001/content/browser/service_w...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/61023005/diff/90001/content/browser/service_w...
content/browser/service_worker/service_worker_context_core.cc:22: path_ =
user_data_directory.Append(kServiceWorkerDirectory);
On 2013/11/07 03:02:59, kinuko wrote:
> I know this is coming from existing code, but one question: does this line
mean
> we may have a conditional like if (path_.empty()) { ... } in the code body?  I
> feel just initializing path_ without checking empty() is fine (or even
> preferable).

We will be testing if (path_.empty()), that determines whether things get
written to disk or not.

https://codereview.chromium.org/61023005/diff/90001/content/browser/service_w...
File content/browser/service_worker/service_worker_context_core.h (right):

https://codereview.chromium.org/61023005/diff/90001/content/browser/service_w...
content/browser/service_worker/service_worker_context_core.h:27: : public
base::SupportsWeakPtr<ServiceWorkerContextCore> {
On 2013/11/07 03:02:59, kinuko wrote:
> We're intermittently having 'kill SupportsWeakPtr' threads on chromium-dev.  I
> think our usecase is fine, but maybe better note a comment about why we're
using
> this and this weakptr's visibility is basically 'private' within the service
> worker lib? 

Was just reading thru one of those threads. I'm picking WeakPtrs (and
SupportsWeakPtr specificically) as the least of the evil options available for
some defensive programming. I'm choosing to make this central class
SupportsWeakPtrs to defend against very unexpected behavior that can happen up
in higher level chrome/content code. In spirit, the central class *should*
outlive everything, but in practice user profiles can get deleted prior to
related child processes and ipc channels having been terminated/cleaned up.
Pathological cases turn up.

We'll have to code defensively to test if context_ prior to handling ipc
messages. It might be nice to put that test in the method containing the message
map? I'll look into that.

[alecflett, 2013-11-07 23:51:40]

On 2013/11/07 23:24:53, michaeln wrote:

> We will be testing if (path_.empty()), that determines whether things get
> written to disk or not.
> 

+1 

> Was just reading thru one of those threads. I'm picking WeakPtrs (and
> SupportsWeakPtr specificically) as the least of the evil options available for
> some defensive programming. I'm choosing to make this central class
> SupportsWeakPtrs to defend against very unexpected behavior that can happen up
> in higher level chrome/content code. In spirit, the central class *should*
> outlive everything, but in practice user profiles can get deleted prior to
> related child processes and ipc channels having been terminated/cleaned up.
> Pathological cases turn up.

I think we're more or less all on board with WeakPtr... I'm new to this
particular part of the conversation but it sounds like folks were leaning
towards only using WeakPtrFactory, if only because it doesn't burden the
class-which-will-be-held-weak with SupportsWeakPtr.

I don't have strong feelings either way, but if we *are* worried about code
churn, it sounds like there might some day be interest in migrating code to
WeakPtrFactory, and we could avoid that today by switching to it. (But this has
zero impact on my review here - we can leave that conversion to someone else if
they care enough about it)

> 
> We'll have to code defensively to test if context_ prior to handling ipc
> messages. It might be nice to put that test in the method containing the
message
> map? I'll look into that.

The defensive test seems fine to me. Because we're at the IPC boundary we can do
DCHECK + defensive check so we can at least catch bad uses in tests.

[kinuko, 2013-11-08 02:08:01]

https://codereview.chromium.org/61023005/diff/90001/content/browser/service_w...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/61023005/diff/90001/content/browser/service_w...
content/browser/service_worker/service_worker_context_core.cc:22: path_ =
user_data_directory.Append(kServiceWorkerDirectory);
On 2013/11/07 23:24:53, michaeln wrote:
> On 2013/11/07 03:02:59, kinuko wrote:
> > I know this is coming from existing code, but one question: does this line
> mean
> > we may have a conditional like if (path_.empty()) { ... } in the code body? 
I
> > feel just initializing path_ without checking empty() is fine (or even
> > preferable).
> 
> We will be testing if (path_.empty()), that determines whether things get
> written to disk or not.

Ok... I generally prefer explicit flag for behavior control (having implicit
behavior makes things harder), but if we want to take it easy can we explicitly
document it in the header?

https://codereview.chromium.org/61023005/diff/90001/content/browser/service_w...
File content/browser/service_worker/service_worker_context_core.h (right):

https://codereview.chromium.org/61023005/diff/90001/content/browser/service_w...
content/browser/service_worker/service_worker_context_core.h:27: : public
base::SupportsWeakPtr<ServiceWorkerContextCore> {
On 2013/11/07 23:24:53, michaeln wrote:
> On 2013/11/07 03:02:59, kinuko wrote:
> > We're intermittently having 'kill SupportsWeakPtr' threads on chromium-dev. 
I
> > think our usecase is fine, but maybe better note a comment about why we're
> using
> > this and this weakptr's visibility is basically 'private' within the service
> > worker lib? 
> 
> Was just reading thru one of those threads. I'm picking WeakPtrs (and
> SupportsWeakPtr specificically) as the least of the evil options available for
> some defensive programming. I'm choosing to make this central class
> SupportsWeakPtrs to defend against very unexpected behavior that can happen up
> in higher level chrome/content code. In spirit, the central class *should*
> outlive everything, but in practice user profiles can get deleted prior to
> related child processes and ipc channels having been terminated/cleaned up.
> Pathological cases turn up.

We can also take destruction observer pattern or propagate Shutdown down to
child classes (though these patterns also have their own downside). I'm
wondering if we'll have cases where child classes want to know the timing the
context goes away and make some actions (e.g. prepare for shutting down).
Thoughts?

> We'll have to code defensively to test if context_ prior to handling ipc
> messages. It might be nice to put that test in the method containing the
message
> map? I'll look into that.

Sounds good.

[michaeln, 2013-11-09 00:23:33]

> I'm not sure why the Wrapper class is working so hard to make the ContextCore
> class lazily-created? Why can't it just create it in its constructor, so that
it
> doesn't have to hold on to a bunch of duplicate member variables? I feel like
> there is a lot of complexity around stuff like OnFilterAdded()?

I've reworked things to get rid of the dup member vars.

[michaeln, 2013-11-09 00:24:00]

ptal

https://codereview.chromium.org/61023005/diff/90001/content/browser/service_w...
File content/browser/service_worker/service_worker_context_core.h (right):

https://codereview.chromium.org/61023005/diff/90001/content/browser/service_w...
content/browser/service_worker/service_worker_context_core.h:27: : public
base::SupportsWeakPtr<ServiceWorkerContextCore> {
> We can also take destruction observer pattern or propagate Shutdown down to
> child classes (though these patterns also have their own downside). I'm
> wondering if we'll have cases where child classes want to know the timing the
> context goes away and make some actions (e.g. prepare for shutting down).
> Thoughts?

If we ever have the need to observe destruction and take action at that time, we
can write code to do that. I don't anticipate we'll need that.

[kinuko, 2013-11-09 09:39:56]

lgtm

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

https://codereview.chromium.org/61023005/diff/510001/content/browser/service_...
content/browser/service_worker/service_worker_context_core.cc:21: 
nit: extra empty line

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

https://codereview.chromium.org/61023005/diff/510001/content/browser/service_...
content/browser/service_worker/service_worker_context_core.h:31:
ServiceWorkerContextCore(const base::FilePath& user_data_directory,
can you comment that giving empty user_data_directory means it doesn't store
data on disk?

https://codereview.chromium.org/61023005/diff/510001/content/browser/service_...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/61023005/diff/510001/content/browser/service_...
content/browser/service_worker/service_worker_context_wrapper.cc:30:
context_core_.reset(
Should this check DCHECK(!context_core_) before resetting?

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

https://codereview.chromium.org/61023005/diff/510001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.cc:34: int
render_process_id) {
Oh, we don't use this process_id yet..

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

https://codereview.chromium.org/61023005/diff/510001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.h:20:
ServiceWorkerDispatcherHost(int render_process_id);
explicit

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

https://codereview.chromium.org/61023005/diff/510001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:38:
context_ = context->AsWeakPtr();
Hmm.. ok.

[michaeln, 2013-11-11 16:40:24]

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

https://codereview.chromium.org/61023005/diff/510001/content/browser/service_...
content/browser/service_worker/service_worker_context_core.cc:21: 
On 2013/11/09 09:39:56, kinuko (OOO until Nov 13) wrote:
> nit: extra empty line

Done.

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

https://codereview.chromium.org/61023005/diff/510001/content/browser/service_...
content/browser/service_worker/service_worker_context_core.h:31:
ServiceWorkerContextCore(const base::FilePath& user_data_directory,
On 2013/11/09 09:39:56, kinuko (OOO until Nov 13) wrote:
> can you comment that giving empty user_data_directory means it doesn't store
> data on disk?

Done.

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

https://codereview.chromium.org/61023005/diff/510001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.cc:34: int
render_process_id) {
On 2013/11/09 09:39:56, kinuko (OOO until Nov 13) wrote:
> Oh, we don't use this process_id yet..

i was surprised by that too

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

https://codereview.chromium.org/61023005/diff/510001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.h:20:
ServiceWorkerDispatcherHost(int render_process_id);
On 2013/11/09 09:39:56, kinuko (OOO until Nov 13) wrote:
> explicit

Done.

[alecflett, 2013-11-11 18:59:02]

lgtm as well, with one minor query

https://codereview.chromium.org/61023005/diff/360001/content/browser/service_...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/61023005/diff/360001/content/browser/service_...
content/browser/service_worker/service_worker_context_wrapper.cc:23: if
(!BrowserThread::CurrentlyOn(BrowserThread::IO)) {
Since, as it exists today, it will always be called on the IO thread, it seems
like it would be a bug to make it created on another thread?

I'm starting to wonder what "other" thread is ever going to be calling Init()?
Can we DCHECK that this called on the right thread for now, and then if code
evolves to create the this on another thread, we add the if() ?

[michaeln, 2013-11-11 20:56:37]

https://codereview.chromium.org/61023005/diff/360001/content/browser/service_...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/61023005/diff/360001/content/browser/service_...
content/browser/service_worker/service_worker_context_wrapper.cc:23: if
(!BrowserThread::CurrentlyOn(BrowserThread::IO)) {
On 2013/11/11 18:59:03, alecflett wrote:
> Since, as it exists today, it will always be called on the IO thread...

But the wrapper is created by StoragePartitionImpl::Create which happens on the
UI thread. Also fyi, ServiceWorkerDispatcherHost instances are created on the UI
thread too.

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

@jam, can you do a content owner review?

[jam, 2013-11-11 23:28:15]

https://codereview.chromium.org/61023005/diff/510002/content/public/browser/s...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/61023005/diff/510002/content/public/browser/s...
content/public/browser/service_worker_context.h:14: // which will come later.
can you explain what this class will be used for, and how chrome will use it? in
general, i don't like adding new stuff to content/public until i see the usage
of it in chrome, because then it's impossible to figure out if this needs to be
exposed or not.

[michaeln, 2013-11-12 00:04:03]

https://codereview.chromium.org/61023005/diff/510002/content/public/browser/s...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/61023005/diff/510002/content/public/browser/s...
content/public/browser/service_worker_context.h:14: // which will come later.
On 2013/11/11 23:28:16, jam wrote:
> can you explain what this class will be used for, and how chrome will use it?
in
> general, i don't like adding new stuff to content/public until i see the usage
> of it in chrome, because then it's impossible to figure out if this needs to
be
> exposed or not.

Sure, this class is similar to a number of classes that exist in the public api.
For each storage sort of system, there's a distinct context class that provides
an api for use by chrome's contentsettings/ui. The public interface to those
systems is narrowly defined to provide the list of things and their names and
such, DOMStorageContext is a good example (although that class also exposes apis
to get sessionStorage to participate in chromes sessionRestore stuff).

This class will eventually have interfaces that contentsettings can use to
enumerate ServiceWorkers registered in different origins and Caches defined by
those things.

[michaeln, 2013-11-12 00:06:09]

On 2013/11/12 00:04:03, michaeln wrote:
>
https://codereview.chromium.org/61023005/diff/510002/content/public/browser/s...
> File content/public/browser/service_worker_context.h (right):
> 
>
https://codereview.chromium.org/61023005/diff/510002/content/public/browser/s...
> content/public/browser/service_worker_context.h:14: // which will come later.
> On 2013/11/11 23:28:16, jam wrote:
> > can you explain what this class will be used for, and how chrome will use
it?
> in
> > general, i don't like adding new stuff to content/public until i see the
usage
> > of it in chrome, because then it's impossible to figure out if this needs to
> be
> > exposed or not.
> 
> Sure, this class is similar to a number of classes that exist in the public
api.
> For each storage sort of system, there's a distinct context class that
provides
> an api for use by chrome's contentsettings/ui. The public interface to those
> systems is narrowly defined to provide the list of things and their names and
> such, DOMStorageContext is a good example (although that class also exposes
apis
> to get sessionStorage to participate in chromes sessionRestore stuff).
> 
> This class will eventually have interfaces that contentsettings can use to
> enumerate ServiceWorkers registered in different origins and Caches defined by
> those things.

I can defer adding this empty interface, but it helps to explain why some of the
other classes are named as they are.

[jam, 2013-11-12 01:47:34]

On 2013/11/12 00:06:09, michaeln wrote:
> On 2013/11/12 00:04:03, michaeln wrote:
> >
>
https://codereview.chromium.org/61023005/diff/510002/content/public/browser/s...
> > File content/public/browser/service_worker_context.h (right):
> > 
> >
>
https://codereview.chromium.org/61023005/diff/510002/content/public/browser/s...
> > content/public/browser/service_worker_context.h:14: // which will come
later.
> > On 2013/11/11 23:28:16, jam wrote:
> > > can you explain what this class will be used for, and how chrome will use
> it?
> > in
> > > general, i don't like adding new stuff to content/public until i see the
> usage
> > > of it in chrome, because then it's impossible to figure out if this needs
to
> > be
> > > exposed or not.
> > 
> > Sure, this class is similar to a number of classes that exist in the public
> api.
> > For each storage sort of system, there's a distinct context class that
> provides
> > an api for use by chrome's contentsettings/ui. The public interface to those
> > systems is narrowly defined to provide the list of things and their names
and
> > such, DOMStorageContext is a good example (although that class also exposes
> apis
> > to get sessionStorage to participate in chromes sessionRestore stuff).
> > 
> > This class will eventually have interfaces that contentsettings can use to
> > enumerate ServiceWorkers registered in different origins and Caches defined
by
> > those things.
> 
> I can defer adding this empty interface, but it helps to explain why some of
the
> other classes are named as they are.

yes, please add it when it's used from chrome. or you can add it, but internally
in content/browser, until it's used by chrome

[michaeln, 2013-11-12 19:17:47]

> yes, please add it when it's used from chrome. or you can add it, but
internally
> in content/browser, until it's used by chrome

ok

[michaeln, 2013-11-12 22:19:37]

@jam, want to take another look?

[commit-bot: I haz the power, 2013-11-12 23:36:10]

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

[michaeln, 2013-11-13 02:07:00]

> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/michaeln%40chromium.org/61023005/910001

yup, i regret clicking that cq checkbox yet again :(

[commit-bot: I haz the power, 2013-11-13 02:07:02]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-11-13 04:26:01]

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

[commit-bot: I haz the power, 2013-11-13 05:05:19]

Change committed as 234756