Update installed ServiceWorkers.

Update installed ServiceWorkers.

The cl defines a new job type for UPDATE_JOB and modifies the ServiceWorkerRegisterJob class to be able to perform this type of job in addition to the REGISTRATION_JOB. There is much in common between the two.

An update job is scheduled to run shortly after a page is loaded thru a service worker. The job checks for a new script resource and if found, goes thru the process of installing the new service worker and possibly activating it. Generally, activation has to be deferred until the page that was loaded closes. The update job sets up a deferred activation helper for that case prior to completing w/o having performed the activation.

BUG=371671
TEST=unittests

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

[michaeln, 2014-07-10 03:01:48]

not ready yet, but i think i'm close to having something work

[falken, 2014-07-10 03:45:32]

Great to hear this is almost working! I roughly looked over it, it all looks
reasonable to me.

https://codereview.chromium.org/380093002/diff/30001/content/browser/service_...
File content/browser/service_worker/service_worker_register_job.h (right):

https://codereview.chromium.org/380093002/diff/30001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.h:22: // Handles the
registration of a Service Worker.
we'll have to update this documentation

https://codereview.chromium.org/380093002/diff/30001/content/browser/service_...
File content/browser/service_worker/service_worker_register_job_base.h (right):

https://codereview.chromium.org/380093002/diff/30001/content/browser/service_...
content/browser/service_worker/service_worker_register_job_base.h:14: enum
RegistrationJobType { REGISTRATION, UNREGISTRATION, UPDATE_JOB };
bikeshed nit: "UPDATE" seems more consistent with the others

[nhiroki, 2014-07-10 09:14:14]

Looks good overall. I'm looking forward to playing with '.update' feature :)

https://codereview.chromium.org/380093002/diff/30001/content/browser/service_...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/380093002/diff/30001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:81: } else {
You may want to add "DCHECK_EQ(UPDATE_JOB, job_type_)".

https://codereview.chromium.org/380093002/diff/30001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:253:
ServiceWorkerStatusCode status) {
This may not have to receive |status| since the caller of this function has
already checked it.

[michaeln, 2014-07-14 23:02:30]

https://codereview.chromium.org/380093002/diff/30001/content/browser/service_...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/380093002/diff/30001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:253:
ServiceWorkerStatusCode status) {
On 2014/07/10 09:14:14, nhiroki wrote:
> This may not have to receive |status| since the caller of this function has
> already checked it. 

Done.

https://codereview.chromium.org/380093002/diff/30001/content/browser/service_...
File content/browser/service_worker/service_worker_register_job_base.h (right):

https://codereview.chromium.org/380093002/diff/30001/content/browser/service_...
content/browser/service_worker/service_worker_register_job_base.h:14: enum
RegistrationJobType { REGISTRATION, UNREGISTRATION, UPDATE_JOB };
On 2014/07/10 03:45:32, falken wrote:
> bikeshed nit: "UPDATE" seems more consistent with the others

it does, but that collides with the Phase enum also scope in the job class, how
about we postfix _JOB to all of these?

[michaeln, 2014-07-15 00:59:24]

hi, ptal i think this is ready for review now, i'll update cl description and
remove the [wip] name

[nhiroki, 2014-07-15 03:57:05]

lgtm

https://codereview.chromium.org/380093002/diff/180001/content/browser/service...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/380093002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:118: pattern_,
next_step);
nit: Maybe you can squash this line into the previous line.

https://codereview.chromium.org/380093002/diff/180001/content/browser/service...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/380093002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_version.h:88: protected:
style-nit: can you insert a blank line before 'protected:'?

[falken, 2014-07-15 11:23:09]

On 2014/07/15 00:59:24, michaeln wrote:
> hi, ptal i think this is ready for review now, i'll update cl description and
> remove the [wip] name

Sorry for the review delay, I've been gardening. I'll try to get to this tonight
or tomorrow.

[michaeln, 2014-07-16 00:32:54]

https://codereview.chromium.org/380093002/diff/180001/content/browser/service...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/380093002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:118: pattern_,
next_step);
On 2014/07/15 03:57:04, nhiroki wrote:
> nit: Maybe you can squash this line into the previous line.

Done.

https://codereview.chromium.org/380093002/diff/180001/content/browser/service...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/380093002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_version.h:88: protected:
On 2014/07/15 03:57:04, nhiroki wrote:
> style-nit: can you insert a blank line before 'protected:'?

Done.

[michaeln, 2014-07-16 01:41:04]

added a couple of unittest but nothing that really tests the update algo yet

[nhiroki, 2014-07-16 10:37:15]

LGTM2

[nhiroki, 2014-07-16 10:39:20]

https://codereview.chromium.org/380093002/diff/240001/content/browser/service...
File content/browser/service_worker/service_worker_registration_unittest.cc
(right):

https://codereview.chromium.org/380093002/diff/240001/content/browser/service...
content/browser/service_worker/service_worker_registration_unittest.cc:48: const
ServiceWorkerRegistrationInfo& info) {
Can you mark this with 'OVERRIDE'?

[falken, 2014-07-16 17:00:28]

Sorry, I didn't realize my gardening shift included today also so didn't have
time to review. Here are some starter comments, I'll review more fully tomorrow.

https://codereview.chromium.org/380093002/diff/240001/content/browser/service...
File content/browser/service_worker/service_worker_controllee_request_handler.cc
(right):

https://codereview.chromium.org/380093002/diff/240001/content/browser/service...
content/browser/service_worker/service_worker_controllee_request_handler.cc:123:
// TODO(michaeln): If  there's a waiting version, activate it instead of
nit: doublespace after If

https://codereview.chromium.org/380093002/diff/240001/content/browser/service...
content/browser/service_worker/service_worker_controllee_request_handler.cc:130:
// TODO(michaeln): if 'activating' wait until its activated before
grammar nit: it's

https://codereview.chromium.org/380093002/diff/240001/content/browser/service...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/380093002/diff/240001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:39:
scoped_ptr<DeferredActivationHelper> self_deletor(this);
It seems dangerous that this never dies if it never gets OnNoControllees, but I
guess it makes sense since it has to wait until the version is no longer
controlling something. Would it make sense for version to fire OnNoControllees
in its destructor to help mitigate risk of leaks? Or else DCHECK that it has no
controllees in its dtor?

https://codereview.chromium.org/380093002/diff/240001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:400:
ignore_result(deferred_activation.release());
I was a little confused by this. Is it a common idiom? It is equivalent to just
not using scoped_ptr, correct? 

// It will delete itself when done.
DeferredActivationHelper*  deferred_activation = new DeferredActivationHelper...

https://codereview.chromium.org/380093002/diff/240001/content/browser/service...
File content/browser/service_worker/service_worker_register_job_base.h (right):

https://codereview.chromium.org/380093002/diff/240001/content/browser/service...
content/browser/service_worker/service_worker_register_job_base.h:14: enum
RegistrationJobType { REGISTRATION, UNREGISTRATION, UPDATE_JOB };
_JOB suffix to all sounds good.

https://codereview.chromium.org/380093002/diff/240001/content/browser/service...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/380093002/diff/240001/content/browser/service...
content/browser/service_worker/service_worker_version.h:87: virtual void
OnNoControllees(ServiceWorkerVersion* version) {}
We'll have to readd actually firing OnNoControllees since it got removed in
https://codereview.chromium.org/377153003

[michaeln, 2014-07-16 19:45:21]

https://codereview.chromium.org/380093002/diff/240001/content/browser/service...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/380093002/diff/240001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:39:
scoped_ptr<DeferredActivationHelper> self_deletor(this);
On 2014/07/16 17:00:28, falken wrote:
> It seems dangerous that this never dies if it never gets OnNoControllees, but
I
> guess it makes sense since it has to wait until the version is no longer
> controlling something. Would it make sense for version to fire OnNoControllees
> in its destructor to help mitigate risk of leaks? Or else DCHECK that it has
no
> controllees in its dtor?

I'm wondering if it would be better for the reg object to handle this internally
instead of this wart watching from the outside. Calling OnNoControllees from a
dtor would be a bad idea, this method calls back into the reg and that method
internally addrefs... not a good idea when in the dtor.

reg->ActivateWaitingVersionWhenReady() kind of method

i didn't do that in this cl to so the reg jobs control flow is less impacted by
this change, i think this is something that can be tackled in later cls

https://codereview.chromium.org/380093002/diff/240001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:400:
ignore_result(deferred_activation.release());
On 2014/07/16 17:00:28, falken wrote:
> I was a little confused by this. Is it a common idiom? It is equivalent to
just
> not using scoped_ptr, correct? 
> 
> // It will delete itself when done.
> DeferredActivationHelper*  deferred_activation = new
DeferredActivationHelper...

functionally equivalent to ...

new DeferredActivationHelper(...);

https://codereview.chromium.org/380093002/diff/240001/content/browser/service...
File content/browser/service_worker/service_worker_register_job_base.h (right):

https://codereview.chromium.org/380093002/diff/240001/content/browser/service...
content/browser/service_worker/service_worker_register_job_base.h:14: enum
RegistrationJobType { REGISTRATION, UNREGISTRATION, UPDATE_JOB };
On 2014/07/16 17:00:28, falken wrote:
> _JOB suffix to all sounds good.

Done.

https://codereview.chromium.org/380093002/diff/240001/content/browser/service...
File content/browser/service_worker/service_worker_registration_unittest.cc
(right):

https://codereview.chromium.org/380093002/diff/240001/content/browser/service...
content/browser/service_worker/service_worker_registration_unittest.cc:48: const
ServiceWorkerRegistrationInfo& info) {
On 2014/07/16 10:39:20, nhiroki wrote:
> Can you mark this with 'OVERRIDE'?

Done.

https://codereview.chromium.org/380093002/diff/240001/content/browser/service...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/380093002/diff/240001/content/browser/service...
content/browser/service_worker/service_worker_version.h:87: virtual void
OnNoControllees(ServiceWorkerVersion* version) {}
On 2014/07/16 17:00:28, falken wrote:
> We'll have to readd actually firing OnNoControllees since it got removed in
> https://codereview.chromium.org/377153003

Done(ooops)

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

i'm working on some ServiceWorkerRegistrationJobTests (EmbeddedWorkerTestHelper
is immensely useful), i want to get some basic tests for this class in place in
this cl...


TEST_F(ServiceWorkerRegistrationJobTest, FirstTimeRegistration) {
}

TEST_F(ServiceWorkerRegistrationJobTest, Update_NoChangeDetected) {
}

TEST_F(ServiceWorkerRegistrationJobTest, Update_NewVersionFound) {
}

... and in later cls we can flesh out more cases and fix bugs in the process of
doing so.

[falken, 2014-07-17 06:33:43]

looks pretty good

https://codereview.chromium.org/380093002/diff/240001/content/browser/service...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/380093002/diff/240001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:39:
scoped_ptr<DeferredActivationHelper> self_deletor(this);
On 2014/07/16 19:45:21, michaeln wrote:
> On 2014/07/16 17:00:28, falken wrote:
> > It seems dangerous that this never dies if it never gets OnNoControllees,
but
> I
> > guess it makes sense since it has to wait until the version is no longer
> > controlling something. Would it make sense for version to fire
OnNoControllees
> > in its destructor to help mitigate risk of leaks? Or else DCHECK that it has
> no
> > controllees in its dtor?
> 
> I'm wondering if it would be better for the reg object to handle this
internally
> instead of this wart watching from the outside. Calling OnNoControllees from a
> dtor would be a bad idea, this method calls back into the reg and that method
> internally addrefs... not a good idea when in the dtor.
> 
> reg->ActivateWaitingVersionWhenReady() kind of method
> 
> i didn't do that in this cl to so the reg jobs control flow is less impacted
by
> this change, i think this is something that can be tackled in later cls

Yep, makes sense to me.

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:24: // Helper
class for the [[Update]] algo.
Maybe add a lightweight comment about what it does. "Activates the waiting
version when the current active version no longer controls a document."

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:287: // This
function corresponds to the spec's _Update algorithm.
nit: now it's [[Update]] algorithm

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:493:
SERVICE_WORKER_OK, registration(), current_version);
this can fit on one line

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
File content/browser/service_worker/service_worker_register_job.h (right):

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
content/browser/service_worker/service_worker_register_job.h:23: // subsequent
update of existing regisrtaions.
nit: registrations

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
File content/browser/service_worker/service_worker_register_job_base.h (right):

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
content/browser/service_worker/service_worker_register_job_base.h:9: 
I think some comment is still useful. "Base class for register/update/unregister
jobs managed by ServiceWorkerJobCoordinator."?

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
File content/browser/service_worker/service_worker_registration.h (right):

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
content/browser/service_worker/service_worker_registration.h:23: // This class
represents a service worker regiratration. The
nit: registration

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
content/browser/service_worker/service_worker_registration.h:25: //
registration. It's refcounted to facillitate multiple controllees
nit: facilitate

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
content/browser/service_worker/service_worker_registration.h:27: // corresponds
to navigator.serviceWorker.registgration.
nit: registration

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
content/browser/service_worker/service_worker_registration.h:67: // Sets the
corresposding version attribute and resets the position
nit: corresponding

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
File content/browser/service_worker/service_worker_registration_unittest.cc
(right):

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
content/browser/service_worker/service_worker_registration_unittest.cc:60:
scoped_refptr<ServiceWorkerRegistration>  observed_registration_;
double space

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
File content/browser/service_worker/service_worker_storage.cc (right):

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
content/browser/service_worker/service_worker_storage.cc:77: class CompareHelper
: public base::RefCounted<CompareHelper> {
would ResponseComparer be a more descriptive name? Maybe a comment like
"Compares two responses and returns the result to a CompareCallback" is good
too.

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
content/browser/service_worker/service_worker_storage.cc:146: if
(++(completion_count_) != 2)
nit: those parens look unnecessary to me

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
content/browser/service_worker/service_worker_storage.cc:150:
completion_callback_.Run(SERVICE_WORKER_OK, -1);
nit: this seems another reason that "bool equals" instead of "int
compare_result" would be better. as written, the compare value is like memcmp
except if the sizes are different, seems arbitrary.

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
content/browser/service_worker/service_worker_storage.cc:176: if
(++(completion_count_) != 2) {
nit: i prefer removing those parens

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
content/browser/service_worker/service_worker_storage.cc:181: if (result !=
previous_result_) {
I was confused at first. I see that lhs and rhs must be the same size or we'd
abort after reading Info. But it's not possible for a single call to ReadData to
sometimes read less data than it can? I.e., if the remaining data is 10*N bytes,
and I do ReadData(N), it'll always read N?

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
content/browser/service_worker/service_worker_storage.cc:913: }
nit: these braces aren't neeeded

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
File content/browser/service_worker/service_worker_storage.h (right):

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
content/browser/service_worker/service_worker_storage.h:56:
void(ServiceWorkerStatusCode status, int compare_result)>
bikeshedding: I think we don't care about sort order... would "bool equal" be
better?  In either case, a comment above CompareScriptResources about the
meaning would be good.

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
File content/browser/service_worker/service_worker_storage_unittest.cc (right):

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
content/browser/service_worker/service_worker_storage_unittest.cc:115:
base::Bind(&OnIOComplete, &rv));
now this fits on one line

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
content/browser/service_worker/service_worker_storage_unittest.cc:133: "HTTP/1.0
200 HONKYDORY\0Content-Length: 5\0\0";
this fits on one line

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
content/browser/service_worker/service_worker_storage_unittest.cc:985: "HTTP/1.0
200 HONKYDORY\0\0";
fits on one line

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
content/browser/service_worker/service_worker_version.h:87: virtual void
OnNoControllees(ServiceWorkerVersion* version) {}
Can we add a comment "Called when a version that controlled a page no longer
controls any pages"? I remember jsbell had a question about this.

[michaeln, 2014-07-18 03:40:49]

I've added two basic tests without any complicating aspects. I like how the test
harness works out for this. It should not be to hard to work on more complicated
test cases and verify things in detail.

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:287: // This
function corresponds to the spec's _Update algorithm.
On 2014/07/17 06:33:41, falken wrote:
> nit: now it's [[Update]] algorithm

Done.

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:493:
SERVICE_WORKER_OK, registration(), current_version);
On 2014/07/17 06:33:41, falken wrote:
> this can fit on one line

Done.

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
File content/browser/service_worker/service_worker_register_job_base.h (right):

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
content/browser/service_worker/service_worker_register_job_base.h:9: 
On 2014/07/17 06:33:41, falken wrote:
> I think some comment is still useful. "Base class for
register/update/unregister
> jobs managed by ServiceWorkerJobCoordinator."? 

pet peevish, but i think we have too many comments stating the obvious (like the
comment on the GetType() method which  says returns the type of this job)

when there are too many comments that aren't needed, the comments that are
needed go unseen and unread because the readers expect them to not be useful

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
File content/browser/service_worker/service_worker_registration_unittest.cc
(right):

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
content/browser/service_worker/service_worker_registration_unittest.cc:60:
scoped_refptr<ServiceWorkerRegistration>  observed_registration_;
On 2014/07/17 06:33:42, falken wrote:
> double space

Done.

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
File content/browser/service_worker/service_worker_storage.cc (right):

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
content/browser/service_worker/service_worker_storage.cc:77: class CompareHelper
: public base::RefCounted<CompareHelper> {
On 2014/07/17 06:33:42, falken wrote:
> would ResponseComparer be a more descriptive name? Maybe a comment like
> "Compares two responses and returns the result to a CompareCallback" is good
> too.

Done (classname but not comment since the name indicates its function)

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
content/browser/service_worker/service_worker_storage.cc:146: if
(++(completion_count_) != 2)
On 2014/07/17 06:33:42, falken wrote:
> nit: those parens look unnecessary to me

Done.

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
content/browser/service_worker/service_worker_storage.cc:150:
completion_callback_.Run(SERVICE_WORKER_OK, -1);
On 2014/07/17 06:33:42, falken wrote:
> nit: this seems another reason that "bool equals" instead of "int
> compare_result" would be better. as written, the compare value is like memcmp
> except if the sizes are different, seems arbitrary.

Done.

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
content/browser/service_worker/service_worker_storage.cc:176: if
(++(completion_count_) != 2) {
On 2014/07/17 06:33:42, falken wrote:
> nit: i prefer removing those parens

Done.

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
content/browser/service_worker/service_worker_storage.cc:181: if (result !=
previous_result_) {
On 2014/07/17 06:33:42, falken wrote:
> I was confused at first. I see that lhs and rhs must be the same size or we'd
> abort after reading Info. But it's not possible for a single call to ReadData
to
> sometimes read less data than it can? I.e., if the remaining data is 10*N
bytes,
> and I do ReadData(N), it'll always read N?

Good questin, I'm not sure if it's guaranteed to always return the the amount
you asked for or less as you hit eof? This doesn't look like a safe assumption,
i've added a todo.

(also i have another todo about doing this comparison as data is being retrieved
via the network instead of writing to disk and the comparing which would make
this method uneeded altogether)

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
content/browser/service_worker/service_worker_storage.cc:913: }
On 2014/07/17 06:33:42, falken wrote:
> nit: these braces aren't neeeded

Done.

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
File content/browser/service_worker/service_worker_storage_unittest.cc (right):

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
content/browser/service_worker/service_worker_storage_unittest.cc:115:
base::Bind(&OnIOComplete, &rv));
On 2014/07/17 06:33:43, falken wrote:
> now this fits on one line

Done.

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
content/browser/service_worker/service_worker_storage_unittest.cc:133: "HTTP/1.0
200 HONKYDORY\0Content-Length: 5\0\0";
On 2014/07/17 06:33:43, falken wrote:
> this fits on one line

Done.

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
content/browser/service_worker/service_worker_storage_unittest.cc:985: "HTTP/1.0
200 HONKYDORY\0\0";
On 2014/07/17 06:33:43, falken wrote:
> fits on one line

Done.

[falken, 2014-07-18 05:13:25]

LGTM

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
File content/browser/service_worker/service_worker_register_job_base.h (right):

https://codereview.chromium.org/380093002/diff/300001/content/browser/service...
content/browser/service_worker/service_worker_register_job_base.h:9: 
On 2014/07/18 03:40:48, michaeln wrote:
> On 2014/07/17 06:33:41, falken wrote:
> > I think some comment is still useful. "Base class for
> register/update/unregister
> > jobs managed by ServiceWorkerJobCoordinator."? 
> 
> pet peevish, but i think we have too many comments stating the obvious (like
the
> comment on the GetType() method which  says returns the type of this job)
> 
> when there are too many comments that aren't needed, the comments that are
> needed go unseen and unread because the readers expect them to not be useful

Yep I definitely agree about unneeded comments. I find class comments useful
though, especially for stating (even briefly) the relationship between classes:
this "job" is the same "job" in the name JobCoordinator. When I started on SW I
wished more class comments like that existed.

The GetType() comment I agree is a bit silly and normally would have omitted it.
But I try to add a comment for functions that are part of a public interface
that other classes must implement.

Anyway, it's up to you here.

https://codereview.chromium.org/380093002/diff/360001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.cc (right):

https://codereview.chromium.org/380093002/diff/360001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.cc:59:
OnResumeAfterDownloadStub);
We should put the semicolon on all or none... grep shows no semi-colon is more
common.

https://codereview.chromium.org/380093002/diff/360001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.h (right):

https://codereview.chromium.org/380093002/diff/360001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.h:70: int
mock_render_process_id() const { return mock_render_process_id_ ;}
space before semicolon

https://codereview.chromium.org/380093002/diff/360001/content/browser/service...
File content/browser/service_worker/service_worker_job_unittest.cc (right):

https://codereview.chromium.org/380093002/diff/360001/content/browser/service...
content/browser/service_worker/service_worker_job_unittest.cc:876: const char
kHttpHeaders[] =  "HTTP/1.0 200 HONKYDORY\0\0";
nit: double space after =

https://codereview.chromium.org/380093002/diff/360001/content/browser/service...
content/browser/service_worker/service_worker_job_unittest.cc:1016: 
nit: extra newline

https://codereview.chromium.org/380093002/diff/360001/content/browser/service...
content/browser/service_worker/service_worker_job_unittest.cc:1069: //
oldredundant,
nit: old.redundant

https://codereview.chromium.org/380093002/diff/360001/content/browser/service...
content/browser/service_worker/service_worker_job_unittest.cc:1097: }
nice tests!

https://codereview.chromium.org/380093002/diff/360001/content/browser/service...
File content/browser/service_worker/service_worker_registration.h (right):

https://codereview.chromium.org/380093002/diff/360001/content/browser/service...
content/browser/service_worker/service_worker_registration.h:23: // This class
represents a service worker regiratration. The
"registration" (and other comments from the previous review on this file. I'm
sorry about my spelling nits... I know it's not so critical...)

[michaeln, 2014-07-18 19:19:24]

https://codereview.chromium.org/380093002/diff/360001/content/browser/service...
File content/browser/service_worker/service_worker_registration.h (right):

https://codereview.chromium.org/380093002/diff/360001/content/browser/service...
content/browser/service_worker/service_worker_registration.h:23: // This class
represents a service worker regiratration. The
On 2014/07/18 05:13:25, falken wrote:
> "registration" (and other comments from the previous review on this file. I'm
> sorry about my spelling nits... I know it's not so critical...)

thnx for pointing them out, please continue to do so

[michaeln, 2014-07-18 19:29:11]

The CQ bit was checked by michaeln@chromium.org

[commit-bot: I haz the power, 2014-07-18 19:32:37]

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

[commit-bot: I haz the power, 2014-07-18 22:05:30]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)

[commit-bot: I haz the power, 2014-07-18 22:10:23]

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

[commit-bot: I haz the power, 2014-07-18 22:10:24]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[michaeln, 2014-07-18 22:40:48]

The CQ bit was checked by michaeln@chromium.org

[commit-bot: I haz the power, 2014-07-18 22:42:09]

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

[commit-bot: I haz the power, 2014-07-19 02:04:09]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-07-19 02:08:12]

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

[commit-bot: I haz the power, 2014-07-19 02:08:13]

Try jobs failed on following builders:
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)

[michaeln, 2014-07-19 07:01:00]

The CQ bit was checked by michaeln@chromium.org

[commit-bot: I haz the power, 2014-07-19 07:01:09]

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

[commit-bot: I haz the power, 2014-07-19 12:46:45]

Change committed as 284329