EmbeddedWorker, browser side code

EmbeddedWorker, browser side code

Adding two new classes, EmbeddedWorkerRegistry and EmbeddedWorkerInstance.
Technically they can be implemented within ServiceWorkerContextCore and
ServiceWorkerVersion, but here I wrote them in separate classes for logical
separation.

* EmbeddedWorkerRegistry
-- Hangs off the ServiceWorkerContextCore
-- Acts as a stub between MessageFilter and each EmbeddedWorkerInstance
-- Sends/receives messages to/from each EmbeddedWorker in child process
-- Mostly talks only with MessageFilter and EmbeddedWorkerInstance

* EmbeddedWorkerInstance
-- ServiceWorkerVersion will own it (1:1 relation)
-- Provides an interface to interact (Start/Stop/Execute) with an EmbeddedWorker

BUG=313530
TBR=jochen

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

[kinuko, 2013-11-27 05:25:10]

Here're some of the browser side code. I'm adding two classes, their overview is
in description so you may want to review it before the code. Either class is
intended to be small/thin, but to hide some minor abstraction. Can you review?

[alecflett, 2013-12-05 05:18:42]

small, addressable nits. lgtm with those.

https://codereview.chromium.org/85023003/diff/160001/content/browser/service_...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/85023003/diff/160001/content/browser/service_...
content/browser/service_worker/embedded_worker_instance.cc:44:
registry_->StopWorker(process_id_, embedded_worker_id_);
set status to STOPPING?

https://codereview.chromium.org/85023003/diff/160001/content/browser/service_...
content/browser/service_worker/embedded_worker_instance.cc:54: status_ =
STOPPED;
DCHECK(status_ == STOPPING)?

https://codereview.chromium.org/85023003/diff/160001/content/browser/service_...
content/browser/service_worker/embedded_worker_instance.cc:86:
DCHECK(max_ref_iter != process_refs_.end());
Won't this be true if there are no entries in process_refs_ ? i.e. if the map is
empty, then max_ref_iter never gets reset to a valid process?

(Or is is guaranteed that AddProcessReference is called first? if so, perhaps
also DCHECK(ProcessRefMap.size() > 0) at the start of the function?

https://codereview.chromium.org/85023003/diff/160001/content/browser/service_...
File content/browser/service_worker/embedded_worker_instance.h (right):

https://codereview.chromium.org/85023003/diff/160001/content/browser/service_...
content/browser/service_worker/embedded_worker_instance.h:47: // Called back
from Registry.
Can you just explain when the registry calls these. and that they allow
state/status transition?

https://codereview.chromium.org/85023003/diff/160001/content/browser/service_...
File content/browser/service_worker/service_worker_registration_unittest.cc
(right):

https://codereview.chromium.org/85023003/diff/160001/content/browser/service_...
content/browser/service_worker/service_worker_registration_unittest.cc:58: const
int version_1_id = 1L;
(I suspect these will need to be long long / int64 to compile?)

https://codereview.chromium.org/85023003/diff/160001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (left):

https://codereview.chromium.org/85023003/diff/160001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:15:
ServiceWorkerVersion::~ServiceWorkerVersion() { DCHECK(is_shutdown_); }
I've been 'git cl format'ing my CL's before review/landing, which is how some of
this stuff ends up on one line. I am intending to keep doing that (I can't be
bothered with formatting issues :)) so that may result in some churn on these
lines.

https://codereview.chromium.org/85023003/diff/160001/content/browser/service_...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/85023003/diff/160001/content/browser/service_...
content/browser/service_worker/service_worker_version.h:61: int version_id);
int64?

[kinuko, 2013-12-09 14:07:42]

Thx, updated.

https://codereview.chromium.org/85023003/diff/160001/content/browser/service_...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/85023003/diff/160001/content/browser/service_...
content/browser/service_worker/embedded_worker_instance.cc:44:
registry_->StopWorker(process_id_, embedded_worker_id_);
On 2013/12/05 05:18:42, alecflett wrote:
> set status to STOPPING?

Yup, done.

https://codereview.chromium.org/85023003/diff/160001/content/browser/service_...
content/browser/service_worker/embedded_worker_instance.cc:54: status_ =
STOPPED;
On 2013/12/05 05:18:42, alecflett wrote:
> DCHECK(status_ == STOPPING)?

This could also happen in STARTING or RUNNING (i.e. not only by Stop()), when
the worker script has failed to load or the worker closed itself (the latter
sounds weird in service worker context but it looks it's possible as far as it
derives from WorkerGlobalScope)

https://codereview.chromium.org/85023003/diff/160001/content/browser/service_...
content/browser/service_worker/embedded_worker_instance.cc:86:
DCHECK(max_ref_iter != process_refs_.end());
On 2013/12/05 05:18:42, alecflett wrote:
> Won't this be true if there are no entries in process_refs_ ? i.e. if the map
is
> empty, then max_ref_iter never gets reset to a valid process?
> 
> (Or is is guaranteed that AddProcessReference is called first? if so, perhaps
> also DCHECK(ProcessRefMap.size() > 0) at the start of the function?

Currently this instance code has some assumptions like: the caller (likely the
ServiceWorkerVersion) doesn't call Start() or Stop() in a wrong state, or
doesn't call Start() when no corresponding ProviderHost's around.

The latter assumption might not be obvious though, so I've made this method and
Start() fail when no process's available.

https://codereview.chromium.org/85023003/diff/160001/content/browser/service_...
File content/browser/service_worker/embedded_worker_instance.h (right):

https://codereview.chromium.org/85023003/diff/160001/content/browser/service_...
content/browser/service_worker/embedded_worker_instance.h:47: // Called back
from Registry.
On 2013/12/05 05:18:42, alecflett wrote:
> Can you just explain when the registry calls these. and that they allow
> state/status transition?

Done.

I assume eventually SWVersion will implement EmbeddedWorkerInstanceObserver and
will listen to these upcalls from its corresponding embedded worker.

https://codereview.chromium.org/85023003/diff/160001/content/browser/service_...
File content/browser/service_worker/service_worker_registration_unittest.cc
(right):

https://codereview.chromium.org/85023003/diff/160001/content/browser/service_...
content/browser/service_worker/service_worker_registration_unittest.cc:58: const
int version_1_id = 1L;
On 2013/12/05 05:18:42, alecflett wrote:
> (I suspect these will need to be long long / int64 to compile?)

Done.

https://codereview.chromium.org/85023003/diff/160001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (left):

https://codereview.chromium.org/85023003/diff/160001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:15:
ServiceWorkerVersion::~ServiceWorkerVersion() { DCHECK(is_shutdown_); }
On 2013/12/05 05:18:42, alecflett wrote:
> I've been 'git cl format'ing my CL's before review/landing, which is how some
of
> this stuff ends up on one line. I am intending to keep doing that (I can't be
> bothered with formatting issues :)) so that may result in some churn on these
> lines.

Ok, reverted.

https://codereview.chromium.org/85023003/diff/160001/content/browser/service_...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/85023003/diff/160001/content/browser/service_...
content/browser/service_worker/service_worker_version.h:61: int version_id);
On 2013/12/05 05:18:42, alecflett wrote:
> int64?

Good catch, done.

[kinuko, 2013-12-10 11:53:26]

Added decent unittests and fixed some bugs.  Also moved some
EmbeddedWorkerInstance methods to private and friend'ed EmbeddedWorkerRegistry
to tighten up the public methods.

[kinuko, 2013-12-11 03:15:20]

Ok, I'm landing this. Feel free to shout out even after it lands, I can revisit
then.

[kinuko, 2013-12-11 03:16:49]

TBR=jochen for content/content_{browser,tests}.gypi changes (adding a few files
+ test).

[commit-bot: I haz the power, 2013-12-11 03:17:15]

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

[commit-bot: I haz the power, 2013-12-11 06:05:36]

Change committed as 240030