Service Worker: Change the criteria for bumping the last update check time

Service Worker: Change the criteria for bumping the last update check time

As per the latest spec change, when an update successfully loaded a script
resource from the network (having bypassed the HTTP cache), the registration's
last update check time should be updated regardless of whether the installation
of the fetched resource succeeds or fails.

While the existing code updates the time stamp only when the subsequent
installation of the newly received script is successfully completed (or the
loaded script is byte-for-byte match to the incumbent script resource), this CL
makes it bump the time stamp as soon as the update succeeded.

This CL added a condition for checking whether the script resource was served
from the network:
if (net_request_->response_info().network_accessed &&
    !(net_request_->response_info().was_cached)) // This line is added.

Spec discussion: https://github.com/slightlyoff/ServiceWorker/issues/514#issuecomment-135641718

BUG=539709
Committed: https://crrev.com/2c7aaaf09eab313a07c425c474256f92957da931
Cr-Commit-Position: refs/heads/master@{#371485}

[jungkees, 2015-10-06 01:52:01]

jungkee.song@samsung.com changed reviewers:
+ jinho.bang@samsung.com

[jungkees, 2015-10-06 01:55:49]

jungkee.song@samsung.com changed reviewers:
+ falken@chromium.org, michaeln@chromium.org, nhiroki@chromium.org

[jungkees, 2015-10-06 02:02:26]

This CL is split out from https://codereview.chromium.org/1283273002/ to cover
the timestamp behavior change. PTAL

I removed TEST_F(ServiceWorkerJobTest, Update_BumpLastUpdateCheckTime) for now.
Could you give me a comment how and in which layer/file I can add a test for it
based on the changed behavior?

[jungkees, 2015-11-04 06:22:44]

This CL conforms to the result of the f2f discussion in TPAC:
https://github.com/slightlyoff/ServiceWorker/issues/514#issuecomment-151333617.

PTAL

[michaeln, 2015-11-09 23:19:10]

It makes sense to update the time when there's an application error during
installation event handling. Doing so will at least prevent repeated download of
the main script resource. (is that the motivation for updating the time prior to
completion of the update attempt)?

But I'm not so sure it make much sense to update the time if the install doesn't
complete for other reasons, such as the browser crashing or being quit prior to
installation completion. In those cases, it will not do the full check when it
should upon immediate restart. The saved time will indicate it just checked, yet
the version is stale.

https://codereview.chromium.org/1381153004/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/1381153004/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_register_job.cc:347:
context_->storage()->UpdateLastUpdateCheckTime(registration());
This block is reachable for first time registrations as well as updates of
existing registrations. For first time registrations, there is nothing to update
and this call will run into an error at some point.

[jungkees, 2015-11-10 02:43:16]

On 2015/11/09 23:19:10, michaeln wrote:
> It makes sense to update the time when there's an application error during
> installation event handling. Doing so will at least prevent repeated download
of
> the main script resource. (is that the motivation for updating the time prior
to
> completion of the update attempt)?
> 

Yes, that's one. Also, as the primary purpose of the update is to checking a new
version from the network, it aligns to the expected behavior where
registration.update() resolves before the install starts. (The mental model
distinguishes the update and the install as two separate internal states.)


> But I'm not so sure it make much sense to update the time if the install
doesn't
> complete for other reasons, such as the browser crashing or being quit prior
to
> installation completion. In those cases, it will not do the full check when it
> should upon immediate restart. The saved time will indicate it just checked,
yet
> the version is stale.
> 

Once a new version is retrieved, the update is done and it doesn't have to do
with the install regardless which errors prevented it from getting through. The
saved time is being checked in the next update to determine whether the UA has
to force update (bypassing the cache). But even in the case the force update is
not requested, the UA will hit the network again if the resource cannot be
obtained locally.

> it will not do the full check when it should upon immediate restart.

I'm not sure if I missed your point here. If so, please let me know.

[jungkees, 2015-11-10 02:45:17]

Setting |last_update_time| for the first time registration seems fine.
Checking/using the value for it seems to be guarded though. Left a comment.

https://codereview.chromium.org/1381153004/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/1381153004/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_register_job.cc:347:
context_->storage()->UpdateLastUpdateCheckTime(registration());
Executing this part of the code for the first time registration seems fine as
the first time registration also goes through an update. But as you pointed, I
think I need a guard for the first time registration here:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/se...
as the |last_update_time| shouldn't have been initialized by that time for the
first time registration.

[michaeln, 2015-11-10 21:17:36]

> I'm not sure if I missed your point here. If so, please let me know.

- swaccessed causing an update check
- new version is downloaded bypassing http cache (including writing into the
http cache)
- updatetime is updated
- system crashes
- system reboots
- swaccess causing an update check
- update check does not bypass cache and possibly accesses an old version in the
http cache
- old version is not updated

The spec doesn't recognize all conditions that an impl contends with. I don't
think that sequence of events is possible in the virtual environment of the spec
but could be in an in our impl.

What is the desired behavior for that scenario? I think the desired behavior and
the intent of the spec is to bypass caches in that case after system reboot. If
that's right, I think we could get the behavior by deferring writing the
updatetime to disk until after OnInstallFinished. If OnInstallFinished fails
update the check time then.  If install succeeds update the checktime atomically
with updating the registration to use the new version.

The impl distinguishes between updating values in memory (the data member of the
object) vs updating them on disk. I don't think the spec makes that kind of
distinction.

> Setting |last_update_time| for the first time registration seems fine.

Just a bit of unecessary disk IO to read the database only to discover there is
no such registrastion to update.

[jungkees, 2015-11-11 08:10:05]

> - swaccessed causing an update check
> - new version is downloaded bypassing http cache (including writing into the
> http cache)
> - updatetime is updated
> - system crashes
> - system reboots
> - swaccess causing an update check
> - update check does not bypass cache and possibly accesses an old version in
the
> http cache
> - old version is not updated
> 
> The spec doesn't recognize all conditions that an impl contends with. I don't
> think that sequence of events is possible in the virtual environment of the
spec
> but could be in an in our impl.
> 

Apart from optimizations, I believe the spec and the impl should match in
behavior. Considering the above scenario with the spec steps, we can think of
that running algorithm instance just aborts in the step where the crash occurs
leaving the internal states as are. Actually, it seems we are seeing the model
differently now.

> What is the desired behavior for that scenario? I think the desired behavior
and
> the intent of the spec is to bypass caches in that case after system reboot.

The desired behavior and the intent of the spec is to NOT bypass caches in that
case after system reboot. As the update before the system crash succeeded, the
version is not stale. The UA should use the cached script, if any, for the
install. If the dev intended to not allow caching the script, max-age header
greater than 0 should not be set in the response. The behavior the spec
contributors have agreed upon is
https://github.com/slightlyoff/ServiceWorker/issues/514#issuecomment-130664373.

> If
> that's right, I think we could get the behavior by deferring writing the
> updatetime to disk until after OnInstallFinished. If OnInstallFinished fails
> update the check time then.  If install succeeds update the checktime
atomically
> with updating the registration to use the new version.
> 
> The impl distinguishes between updating values in memory (the data member of
the
> object) vs updating them on disk. I don't think the spec makes that kind of
> distinction.
> 

Right. This is exactly an example where the impl optimizes the actual
operations. (The spec just assumed a persistent storage for the map.)

> > Setting |last_update_time| for the first time registration seems fine.
> 
> Just a bit of unecessary disk IO to read the database only to discover there
is
> no such registrastion to update.

When the first register succeeded fetching a script from the network (as well as
the subsequent successful updates), this disk IO seems required. As posed in the
abrupt system reboot scenario, it seems the last update time should be written
in a persistent storage in any case.

[michaeln, 2015-11-11 20:57:03]

> The desired behavior and the intent of the spec is to NOT bypass caches in
that
> case after system reboot. As the update before the system crash succeeded, the
> version is not stale. The UA should use the cached script, if any, for the
> install. If the dev intended to not allow caching the script, max-age header
> greater than 0 should not be set in the response. The behavior the spec
> contributors have agreed upon is
>
https://github.com/slightlyoff/ServiceWorker/issues/514#issuecomment-130664373.

I'm not sure what you mean by the update succeeded? The BYPASS_CACHE flag means
a response will not be retrieved or added to the http cache. The only persistent
artifact of the update prior to the crash was updating the 'last update time'.
Otherwise not a byte on disk has changed.

> When the first register succeeded fetching a script from the network (as well
as
> the subsequent successful updates), this disk IO seems required. As posed in
the
> abrupt system reboot scenario, it seems the last update time should be written
> in a persistent storage in any case.

But the disk IO here for an initial install results in no persistent artifacts
on disk. Nothing is being written, enough is read to know there is no such
registration in the database to update.

[michaeln, 2015-11-11 21:23:30]

>
https://github.com/slightlyoff/ServiceWorker/issues/514#issuecomment-130664373.
> 
> I'm not sure what you mean by the update succeeded? The BYPASS_CACHE flag
means
> a response will not be retrieved or added to the http cache. The only
persistent
> artifact of the update prior to the crash was updating the 'last update time'.
> Otherwise not a byte on disk has changed.

I was wrong about BYPASS_CACHE not having persistent artifacts. It will delete
any existing entry for the requested url and may create a new entry. So the http
cache is guaranteed to not contain an old entry in the case outlined, so the
update check after reboot will do the right thing.

> I'm not sure what you mean by the update succeeded?

I think I see, validating the network cache == update success.

[jungkees, 2015-11-12 11:25:25]

> I think I see, validating the network cache == update success.

Right. That's clear.

[falken, 2015-12-02 09:27:39]

Michael, it seems like your concerns are mostly resolved? If so, this patch
seems mostly OK for better conformance with the spec and avoiding repeatedly
hitting a server when there's a script error.

Jungkee, service_worker_job_unittest was not uploaded.

https://codereview.chromium.org/1381153004/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/1381153004/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_register_job.cc:347:
context_->storage()->UpdateLastUpdateCheckTime(registration());
On 2015/11/10 02:45:17, jungkees wrote:
> Executing this part of the code for the first time registration seems fine as
> the first time registration also goes through an update. But as you pointed, I
> think I need a guard for the first time registration here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/se...
> as the |last_update_time| shouldn't have been initialized by that time for the
> first time registration.

Michael is saying that for first time registrations, the registration hasn't
been written to disk yet. So UpdateLastUpdateCheckTime() will attempt to read
the registration and discover there is none. So it shouldn't be called in that
case.

[michaeln, 2015-12-04 00:37:13]

On 2015/12/02 09:27:39, falken (away back on Dec 4) wrote:
> Michael, it seems like your concerns are mostly resolved? If so, this patch
> seems mostly OK for better conformance with the spec and avoiding repeatedly
> hitting a server when there's a script error.

Yes, the hidden side effect of validating the http cache was not so obvious, but
given that side effect, i think the code does what its trying to do
 
> Jungkee, service_worker_job_unittest was not uploaded.
> 
>
https://codereview.chromium.org/1381153004/diff/1/content/browser/service_wor...
> File content/browser/service_worker/service_worker_register_job.cc (right):
> 
>
https://codereview.chromium.org/1381153004/diff/1/content/browser/service_wor...
> content/browser/service_worker/service_worker_register_job.cc:347:
> context_->storage()->UpdateLastUpdateCheckTime(registration());
> On 2015/11/10 02:45:17, jungkees wrote:
> > Executing this part of the code for the first time registration seems fine
as
> > the first time registration also goes through an update. But as you pointed,
I
> > think I need a guard for the first time registration here:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/se...
> > as the |last_update_time| shouldn't have been initialized by that time for
the
> > first time registration.
> 
> Michael is saying that for first time registrations, the registration hasn't
> been written to disk yet. So UpdateLastUpdateCheckTime() will attempt to read
> the registration and discover there is none. So it shouldn't be called in that
> case.

[falken, 2015-12-04 02:18:10]

A comment or even better a unit test for that side effect would be good.

[jungkees, 2015-12-04 11:46:38]

A question about checking whether a registration is stored in the database.

https://codereview.chromium.org/1381153004/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/1381153004/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_register_job.cc:347:
context_->storage()->UpdateLastUpdateCheckTime(registration());
Having inspected the code sequence for a first time registration, calling
UpdateLastUpdateCheckTime ends up doing no-op in database.cc without any
side-effect:

ServiceWorkerStorage::UpdateLastUpdateCheckTime
  base::IgnoreResult(&ServiceWorkerDatabase::UpdateLastCheckTime)
    status = ServiceWorkerDatabase::ReadRegistrationData
    if (status != STATUS_OK)
    return status;

Could you check if this is just fine as-is? If it still needs a guard before the
call, will it be a good idea to add a state like
ServiceWorkerRegistration.is_stored_? I couldn't use |is_deleted_| as it's
initialized to false. Is there any existing method to check whether a
registration is stored that I'm missing?

[falken, 2015-12-07 03:53:34]

https://codereview.chromium.org/1381153004/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/1381153004/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_register_job.cc:347:
context_->storage()->UpdateLastUpdateCheckTime(registration());
On 2015/12/04 11:46:38, jungkees wrote:
> Having inspected the code sequence for a first time registration, calling
> UpdateLastUpdateCheckTime ends up doing no-op in database.cc without any
> side-effect:
> 
> ServiceWorkerStorage::UpdateLastUpdateCheckTime
>   base::IgnoreResult(&ServiceWorkerDatabase::UpdateLastCheckTime)
>     status = ServiceWorkerDatabase::ReadRegistrationData
>     if (status != STATUS_OK)
>     return status;
> 
> Could you check if this is just fine as-is? If it still needs a guard before
the
> call, will it be a good idea to add a state like
> ServiceWorkerRegistration.is_stored_? I couldn't use |is_deleted_| as it's
> initialized to false. Is there any existing method to check whether a
> registration is stored that I'm missing?

Right, it's a no-op but it requires reading disk which is slow so it's best
avoided.

You could check if job_type_ is a REGISTER or UPDATE job. A REGISTER job would
not be written to disk yet, and an UPDATE job would.

[jungkees, 2015-12-07 07:02:09]

Avoided first time registrations write to disk.

> Jungkee, service_worker_job_unittest was not uploaded.

I'm again encountering 403 error when uploading that change, but confirmed the
change was actually uploaded. (Please check with the Unified diffs column.) If
it should be uploaded again, I'll do it this evening.

https://codereview.chromium.org/1381153004/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/1381153004/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_register_job.cc:347:
context_->storage()->UpdateLastUpdateCheckTime(registration());
Great. I made it only UPDATE_JOBs write to disk.

[falken, 2015-12-07 07:40:33]

I see the uploaded unittest file, but it's just removing the existing unit test.
 Can you add new unittests for the behavior change being introduced?

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

https://codereview.chromium.org/1381153004/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:354: // the
version having bypassed the network cache.
Please add a comment here about the side effect of BYPASS_CACHE we're relying on
here. BYPASS_CACHE will evict an existing cache entry, so it's ok to bump the
update check time.

https://codereview.chromium.org/1381153004/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:359: // Avoid
reading disk for first time registrations.
I think you can just remove this comment. It's clear you don't need to update
storage for a register job since it hasn't been written yet.

https://codereview.chromium.org/1381153004/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/1381153004/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:344:
!(net_request_->response_info().was_cached))
nit: I prefer to add braces to this if now that it's multi-lines

[falken, 2015-12-07 07:47:41]

I see now:
"I removed TEST_F(ServiceWorkerJobTest, Update_BumpLastUpdateCheckTime) for now.
Could you give me a comment how and in which layer/file I can add a test for it
based on the changed behavior?"

I think you can still do it in SeviceWorkerJobTest using
EmbeddedWorkerTestHelper to create the scenarios you want:

1) An update attempt for the same script that doesn't bypass the cache doesn't
bump the update time.
2) An update attempt for the same script that bypasses the cache bumps the
update time.
3) An update to a worker that loads successfully but fails to start up (syntax
error) bumps the update time.

[jungkees, 2015-12-15 13:07:46]

Just uploaded a new snapshot having addressed the comments and updated the unit
test. PTAL.

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

https://codereview.chromium.org/1381153004/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:354: // the
version having bypassed the network cache.
On 2015/12/07 07:40:33, falken wrote:
> Please add a comment here about the side effect of BYPASS_CACHE we're relying
on
> here. BYPASS_CACHE will evict an existing cache entry, so it's ok to bump the
> update check time.

Done.

https://codereview.chromium.org/1381153004/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:359: // Avoid
reading disk for first time registrations.
On 2015/12/07 07:40:33, falken wrote:
> I think you can just remove this comment. It's clear you don't need to update
> storage for a register job since it hasn't been written yet.

Done.

https://codereview.chromium.org/1381153004/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/1381153004/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:344:
!(net_request_->response_info().was_cached))
On 2015/12/07 07:40:33, falken wrote:
> nit: I prefer to add braces to this if now that it's multi-lines

Done.

[jungkees, 2015-12-15 13:11:00]

service_worker_job_unittest was uploaded, but side-by-side diffs doesn't show
again. I again encountered a 403 error for this file. Sorry about that.

[falken, 2015-12-16 08:27:48]

I think this is looking good.

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

https://codereview.chromium.org/1381153004/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_job_unittest.cc:805: 
A bit ambiguous of a name. "set_force_start_worker_failure" is a bit clearer.

https://codereview.chromium.org/1381153004/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_job_unittest.cc:875: 
nit: braces for the bodies of these

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

https://codereview.chromium.org/1381153004/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:356: // check
time.
I think the second sentence of this comment would be hard to understand without
the context of this code review. I would say something like: "We assume that the
BYPASS_CACHE flag evicts an existing cache entry, so even if the update
ultimately failed for whatever reason, we know the version in the HTTP cache is
not stale, so it's OK to bump the update check time."

[jungkees, 2015-12-16 08:59:20]

Uploaded a new snapshot. PTAL. Thanks for review!

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

https://codereview.chromium.org/1381153004/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:356: // check
time.
Thanks for the comment. I used the words as-is except changing "the update
ultimately failed" to "the successive install ultimately failed". I don't think
it matters much, but the update can fail without updating the time stamp (due to
network error, MIME type violation, etc). In other words, once the time stamp
was bumped, we can say the update (phase) succeeded.

[falken, 2015-12-17 13:23:43]

Sorry for the late review, I was off today.

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

https://codereview.chromium.org/1381153004/diff/60001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.h:102: virtual void
OnStartWorkerFailed(int embedded_worker_id,
Sorry to nitpick a bit late in the game... this function is quite different than
the rest in this class, which just do the success thing. Tests that want custom
implementation are supposed to subclass this class.

The name is a bit off too, these are designed to be called in response to a
message from the browser, the browser wouldn't send a "OnStartWorkerFailed"
message. The failure hasn't occurred yet until the function runs.

So I think this custom implementation should be done in a subclass of
EmbeddedWorkerTestHelper.

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

https://codereview.chromium.org/1381153004/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_job_unittest.cc:878: 
This class should just override OnStartWorker to do what it wants.

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

https://codereview.chromium.org/1381153004/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:355: //
BYPASS_CACHE flag evicts an existing cache entry, so even if the successive
nit: "successive" seems confusing. I'd just remove the word.

[jungkees, 2015-12-23 02:53:40]

Patchset #5 (id:80001) has been deleted

[jungkees, 2015-12-23 03:08:58]

Sorry for the late response. I just uploaded a new snapshot having rebased and
addressed the comments. PTAL.

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

https://codereview.chromium.org/1381153004/diff/60001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.h:102: virtual void
OnStartWorkerFailed(int embedded_worker_id,
I'd tried subclassing EmbeddedWorkerTestHelper to override OnStartWorker for the
fail-to-start-worker case. But that needed OnStartWorker to extend the parameter
list (|success| to be passed to SimulateWorkerScriptEvaluated) and involved some
changes in other call sites as such.

So, I instead gave a try with subclassing UpdateJobTestHelper and override
OnStartWorker in that class which simulate the fail-to-start-worker depending on
the value its |force_start_worker_failure_|.

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

https://codereview.chromium.org/1381153004/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:355: //
BYPASS_CACHE flag evicts an existing cache entry, so even if the successive
On 2015/12/17 13:23:43, falken wrote:
> nit: "successive" seems confusing. I'd just remove the word.

Done.

[falken, 2016-01-05 04:21:16]

https://codereview.chromium.org/1381153004/diff/120001/content/browser/servic...
File content/browser/service_worker/embedded_worker_test_helper.h (right):

https://codereview.chromium.org/1381153004/diff/120001/content/browser/servic...
content/browser/service_worker/embedded_worker_test_helper.h:78: }
How about a GetNextThreadId() instead that takes care of incrementing?

https://codereview.chromium.org/1381153004/diff/120001/content/browser/servic...
content/browser/service_worker/embedded_worker_test_helper.h:82: std::map<int,
int64> embedded_worker_id_service_worker_version_id_map() {
int64_t

https://codereview.chromium.org/1381153004/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_job_unittest.cc (right):

https://codereview.chromium.org/1381153004/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_job_unittest.cc:832: 
rm comment that repeats the code

https://codereview.chromium.org/1381153004/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_job_unittest.cc:894: 
init to false here instead of in the ctor for consistency with update_found_

https://codereview.chromium.org/1381153004/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_job_unittest.cc:913: 
"int64" -> "int64_t" and "uint64" -> "uint64_t" throughout this file.

https://codereview.chromium.org/1381153004/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_job_unittest.cc:947: 
I'd rather not repeat all the code in OnStartWorker up to here. How about
UpdateJobTestHelper just has a force_start_worker_failure_ member instead making
a subclass for it?

https://codereview.chromium.org/1381153004/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_job_unittest.cc:1085: 
This doesn't actually test the new behavior. The helper is using kNoChangeOrigin
which means it bails out with ERROR_EXISTS in OnStartWorker.
set_force_start_worker_failure makes no difference.

[jungkees, 2016-01-06 02:57:55]

I uploaded a new snapshot having addressed the comments. PTAL.

https://codereview.chromium.org/1381153004/diff/120001/content/browser/servic...
File content/browser/service_worker/embedded_worker_test_helper.h (right):

https://codereview.chromium.org/1381153004/diff/120001/content/browser/servic...
content/browser/service_worker/embedded_worker_test_helper.h:78: }
Yes, that'd be better. Done.

https://codereview.chromium.org/1381153004/diff/120001/content/browser/servic...
content/browser/service_worker/embedded_worker_test_helper.h:82: std::map<int,
int64> embedded_worker_id_service_worker_version_id_map() {
On 2016/01/05 04:21:16, falken wrote:
> int64_t

Done.

https://codereview.chromium.org/1381153004/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_job_unittest.cc (right):

https://codereview.chromium.org/1381153004/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_job_unittest.cc:832: 
On 2016/01/05 04:21:16, falken wrote:
> rm comment that repeats the code

Done.

https://codereview.chromium.org/1381153004/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_job_unittest.cc:894: 
On 2016/01/05 04:21:16, falken wrote:
> init to false here instead of in the ctor for consistency with update_found_

Done.

https://codereview.chromium.org/1381153004/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_job_unittest.cc:913: 
On 2016/01/05 04:21:16, falken wrote:
> "int64" -> "int64_t" and "uint64" -> "uint64_t" throughout this file.

Done.

https://codereview.chromium.org/1381153004/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_job_unittest.cc:947: 
Agreed. Done.

https://codereview.chromium.org/1381153004/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_job_unittest.cc:1085: 
Sorry for having missed that. I updated the second and the third cases using
kNewVersionOrigin. Note that I removed ASSERT_FALSE(update_found_); in
OnUpdateFound to run StartUpdate() multiple times. This change ensures other
Update_* tests do not break.

[falken, 2016-01-06 05:22:08]

https://codereview.chromium.org/1381153004/diff/140001/content/browser/servic...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/1381153004/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_register_job.cc:364: if (job_type_
== UPDATE_JOB)
Ick, I made a mistake. The registration could be on disk for a REGISTER_JOB that
found an existing registration.

I'm not sure there's an accurate way to tell if SWRegistration is stored. I
think maybe checking if there's a waiting/active version works. I really don't
want to add an is_stored flag to that class, the three curently flags
is_deleted, is_uninstalling, is_uninstalled are already a mess.

[jungkees, 2016-01-06 05:45:29]

Addressed the comment on changing the condition to check if the SWRegistration
is stored.

https://codereview.chromium.org/1381153004/diff/140001/content/browser/servic...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/1381153004/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_register_job.cc:364: if (job_type_
== UPDATE_JOB)
> think maybe checking if there's a waiting/active version works.

I tried it with this.

[jungkees, 2016-01-19 03:48:15]

falken@, I think this CL is ready. Could you check it?

[michaeln, 2016-01-19 19:20:28]

driveby comments/questions

https://codereview.chromium.org/1381153004/diff/160001/content/browser/servic...
File content/browser/service_worker/service_worker_register_job.cc (left):

https://codereview.chromium.org/1381153004/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_register_job.cc:436:
registration()->set_last_update_check(base::Time::Now());
Is there any chance we can be here w/o having set the datamember in
OnStartWorkerFinished? Would it hurt to set the value to a slightly newer value
here?

https://codereview.chromium.org/1381153004/diff/160001/content/browser/servic...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/1381153004/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_register_job.cc:364: if
(registration()->waiting_version() || registration()->active_version())
if job_type_ == UPDATE_JOB works as the condition here, that might be a little
clearer

[jungkees, 2016-01-20 05:38:11]

I updated the comments. Thanks for review.

https://codereview.chromium.org/1381153004/diff/160001/content/browser/servic...
File content/browser/service_worker/service_worker_register_job.cc (left):

https://codereview.chromium.org/1381153004/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_register_job.cc:436:
registration()->set_last_update_check(base::Time::Now());
On 2016/01/19 19:20:28, michaeln wrote:
> Is there any chance we can be here w/o having set the datamember in
> OnStartWorkerFinished?

No, I don't think so.

> Would it hurt to set the value to a slightly newer value here?

But yes because there's a case where the data member is updated in
OnStartWorkerFinished but doesn't call InstallAndContinue. (That is, the update
job resolves immediately without advancing to install even though the network
cache was validated.)

https://codereview.chromium.org/1381153004/diff/160001/content/browser/servic...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/1381153004/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_register_job.cc:364: if
(registration()->waiting_version() || registration()->active_version())
I discussed with falken@ and tried so, but later he found that the condition (if
job_type_ == UPDATE_JOB) doesn't seem to cover all the cases:
https://codereview.chromium.org/1381153004/#msg30

[falken, 2016-01-20 09:31:54]

I think this looks good unless Michael has objections. But when run locally (I
wanted to verify the test path) with asserts enabled I get an assert failure: 
[7006:7006:0120/182055:6213987689906:FATAL:service_worker_registration.cc(49)]
Check failed: !listeners_.might_have_observers().

[1/1] ServiceWorkerJobTest.Update_BumpLastUpdateCheckTime (CRASHED)
1 test crashed:
    ServiceWorkerJobTest.Update_BumpLastUpdateCheckTime
(../../content/browser/service_worker/service_worker_job_unittest.cc:984)

I think you need to cleanly remove the listener from the first registration you
use.

https://codereview.chromium.org/1381153004/diff/160001/content/browser/servic...
File content/browser/service_worker/service_worker_register_job.cc (left):

https://codereview.chromium.org/1381153004/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_register_job.cc:436:
registration()->set_last_update_check(base::Time::Now());
On 2016/01/20 05:38:11, jungkees wrote:
> On 2016/01/19 19:20:28, michaeln wrote:
> > Is there any chance we can be here w/o having set the datamember in
> > OnStartWorkerFinished?
> 
> No, I don't think so.
> 
> > Would it hurt to set the value to a slightly newer value here?
> 
> But yes because there's a case where the data member is updated in
> OnStartWorkerFinished but doesn't call InstallAndContinue. (That is, the
update
> job resolves immediately without advancing to install even though the network
> cache was validated.)

I guess it'd only be possible if network_accessed_for_script or
force_bypass_cache_for_scripts was wrong. Adding it in both places wouldn't
really hurt except for being a bit redundant. How about doing:

if (new_version()->embedded_worker()->network_accessed_for_script() ||
    new_version()->force_bypass_cache_for_scripts()) {
  ...
} else {
  DCHECK_NE(status, SERVICE_WORKER_OK);
}

if (status == SERVICE_WORKER_OK)
  InstallAndContinue();

This asserts you couldn't get to OnInstallFinished without setting the update
check time.

https://codereview.chromium.org/1381153004/diff/160001/content/browser/servic...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/1381153004/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_register_job.cc:364: if
(registration()->waiting_version() || registration()->active_version())
On 2016/01/20 05:38:11, jungkees wrote:
> I discussed with falken@ and tried so, but later he found that the condition
(if
> job_type_ == UPDATE_JOB) doesn't seem to cover all the cases:
> https://codereview.chromium.org/1381153004/#msg30

Right. I was then worried about whether waiting_version || active_version really
holds iff there is a stored version. For example, SWRegistration::DeleteVersion
does some sketchy unsetting of the versions and might not cleanup the stored
registration properly. But I think in that case, we'd update the check time when
the installing version reaches OnInstallFinished.
(I'm meaning to move DeleteVersion into a job to avoid race conditions with
ongoing jobs).

[michaeln, 2016-01-21 20:55:31]

https://codereview.chromium.org/1381153004/diff/160001/content/browser/servic...
File content/browser/service_worker/service_worker_register_job.cc (left):

https://codereview.chromium.org/1381153004/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_register_job.cc:436:
registration()->set_last_update_check(base::Time::Now());
> This asserts you couldn't get to OnInstallFinished without setting the update
> check time.

Right, an assurance of some kind about having a meaningfully set value is the
gist of my question.

DCHECK(!reg->last_update_check().is_null());

https://codereview.chromium.org/1381153004/diff/160001/content/browser/servic...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/1381153004/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_register_job.cc:366: }
else { there's an assumption here that last_update_check() is set to a non-empty
value }

how about add that test to the main condition on line 360?

if (network_accessed_for_script() ||
    force_bypass_cache_for_scripts() ||
    reg->last_update_check().is_null())

[jungkees, 2016-01-22 05:22:27]

I uploaded a new snapshot having addressed those comments. PTAL. Thanks.

falken@,
> I think you need to cleanly remove the listener from the first registration
you
use.

Addressed. Thanks for spotting this.

https://codereview.chromium.org/1381153004/diff/160001/content/browser/servic...
File content/browser/service_worker/service_worker_register_job.cc (left):

https://codereview.chromium.org/1381153004/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_register_job.cc:436:
registration()->set_last_update_check(base::Time::Now());
> Right, an assurance of some kind about having a meaningfully set value is the
> gist of my question.
> 
> DCHECK(!reg->last_update_check().is_null());

Okay. To assure that, I added this assertion as well as
(reg->last_update_check().is_null()) check in the main condition on line 360.

https://codereview.chromium.org/1381153004/diff/160001/content/browser/servic...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/1381153004/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_register_job.cc:366: }
On 2016/01/21 20:55:31, michaeln wrote:
> else { there's an assumption here that last_update_check() is set to a
non-empty
> value }
> 
> how about add that test to the main condition on line 360?
> 
> if (network_accessed_for_script() ||
>     force_bypass_cache_for_scripts() ||
>     reg->last_update_check().is_null())

Done.

[michaeln, 2016-01-22 19:48:13]

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

https://codereview.chromium.org/1381153004/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_register_job.cc:368:
DCHECK(!registration()->last_update_check().is_null());
This dcheck is very useless here. If the time is null the if condition will be
met and the else clause can't possibly be entered.

https://codereview.chromium.org/1381153004/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_register_job.cc:441:
SetPhase(STORE);
If you're going to add the dcheck, please put it here.

It might be helpful here, if somehow this method gets entered w/o having that
time set (so we skipped OnStartWorkerFinished somehow), it would trip and we'd
find the bug faster.

In current code, the time is undoubtedly set upon return. With your new code,
that's probably not true in all cases (some tests might need to be updated to
set the time). I'd vote to make sure the time is set in all cases so we don't
have the possibility of installed registrations with null time values.

[jungkees, 2016-01-25 20:03:01]

I uploaded a new snapshot having addressed the comment. PTAL

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

https://codereview.chromium.org/1381153004/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_register_job.cc:368:
DCHECK(!registration()->last_update_check().is_null());
I moved the dcheck as suggested.

https://codereview.chromium.org/1381153004/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_register_job.cc:441:
SetPhase(STORE);
On 2016/01/22 19:48:13, michaeln wrote:
> If you're going to add the dcheck, please put it here.
> 
> It might be helpful here, if somehow this method gets entered w/o having that
> time set (so we skipped OnStartWorkerFinished somehow), it would trip and we'd
> find the bug faster.
> 

Yes, I moved the dcheck here.

> In current code, the time is undoubtedly set upon return. With your new code,
> that's probably not true in all cases (some tests might need to be updated to
> set the time). I'd vote to make sure the time is set in all cases so we don't
> have the possibility of installed registrations with null time values.

I agree we have to make sure the time value for an installed registration won't
be null here. But I doubt we have a path here without getting through
OnStartWorkerFinished. If we detect such an error case, the condition
(new_version()->embedded_worker()->network_accessed_for_script() ||
new_version()->force_bypass_cache_for_scripts()) has to be rechecked I believe.
Also the very reason that this change suggests is aligning the behavior to the
spec change which says the time shouldn't be updated depending on conditions.

[michaeln, 2016-01-25 23:24:38]

thnx... lgtm!

[jungkees, 2016-01-26 01:18:18]

The CQ bit was checked by jungkee.song@samsung.com

[commit-bot: I haz the power, 2016-01-26 01:22:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1381153004/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1381153004/200001

[commit-bot: I haz the power, 2016-01-26 01:29:19]

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

[commit-bot: I haz the power, 2016-01-26 01:29:20]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[jungkees, 2016-01-26 01:56:12]

The CQ bit was checked by jungkee.song@samsung.com

[commit-bot: I haz the power, 2016-01-26 01:56:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1381153004/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1381153004/200001

[commit-bot: I haz the power, 2016-01-26 02:01:42]

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

[commit-bot: I haz the power, 2016-01-26 02:01:43]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[jungkees, 2016-01-26 03:15:54]

The CQ bit was checked by jungkee.song@samsung.com

[commit-bot: I haz the power, 2016-01-26 03:16:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1381153004/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1381153004/200001

[commit-bot: I haz the power, 2016-01-26 03:25:01]

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

[commit-bot: I haz the power, 2016-01-26 03:25:02]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[jungkees, 2016-01-26 08:14:41]

I rebased it having resolving a conflict. PTAL

[falken, 2016-01-26 08:32:56]

lgtm

[jungkees, 2016-01-26 08:34:40]

The CQ bit was checked by jungkee.song@samsung.com

[jungkees, 2016-01-26 08:34:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeln@chromium.org
Link to the patchset: https://codereview.chromium.org/1381153004/#ps220001
(title: "Rebase")

[commit-bot: I haz the power, 2016-01-26 08:34:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1381153004/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1381153004/220001

[commit-bot: I haz the power, 2016-01-26 10:03:39]

Description was changed from

==========
Service Worker: Change the criteria for bumping the last update check time

As per the latest spec change, when an update successfully loaded a script
resource from the network (having bypassed the HTTP cache), the registration's
last update check time should be updated regardless of whether the installation
of the fetched resource succeeds or fails.

While the existing code updates the time stamp only when the subsequent
installation of the newly received script is successfully completed (or the
loaded script is byte-for-byte match to the incumbent script resource), this CL
makes it bump the time stamp as soon as the update succeeded.

This CL added a condition for checking whether the script resource was served
from the network:
if (net_request_->response_info().network_accessed &&
    !(net_request_->response_info().was_cached)) // This line is added.

Spec discussion:
https://github.com/slightlyoff/ServiceWorker/issues/514#issuecomment-135641718

BUG=539709
==========

to

==========
Service Worker: Change the criteria for bumping the last update check time

As per the latest spec change, when an update successfully loaded a script
resource from the network (having bypassed the HTTP cache), the registration's
last update check time should be updated regardless of whether the installation
of the fetched resource succeeds or fails.

While the existing code updates the time stamp only when the subsequent
installation of the newly received script is successfully completed (or the
loaded script is byte-for-byte match to the incumbent script resource), this CL
makes it bump the time stamp as soon as the update succeeded.

This CL added a condition for checking whether the script resource was served
from the network:
if (net_request_->response_info().network_accessed &&
    !(net_request_->response_info().was_cached)) // This line is added.

Spec discussion:
https://github.com/slightlyoff/ServiceWorker/issues/514#issuecomment-135641718

BUG=539709
==========

[commit-bot: I haz the power, 2016-01-26 10:03:40]

Committed patchset #11 (id:220001)

[commit-bot: I haz the power, 2016-01-26 10:05:12]

Description was changed from

==========
Service Worker: Change the criteria for bumping the last update check time

As per the latest spec change, when an update successfully loaded a script
resource from the network (having bypassed the HTTP cache), the registration's
last update check time should be updated regardless of whether the installation
of the fetched resource succeeds or fails.

While the existing code updates the time stamp only when the subsequent
installation of the newly received script is successfully completed (or the
loaded script is byte-for-byte match to the incumbent script resource), this CL
makes it bump the time stamp as soon as the update succeeded.

This CL added a condition for checking whether the script resource was served
from the network:
if (net_request_->response_info().network_accessed &&
    !(net_request_->response_info().was_cached)) // This line is added.

Spec discussion:
https://github.com/slightlyoff/ServiceWorker/issues/514#issuecomment-135641718

BUG=539709
==========

to

==========
Service Worker: Change the criteria for bumping the last update check time

As per the latest spec change, when an update successfully loaded a script
resource from the network (having bypassed the HTTP cache), the registration's
last update check time should be updated regardless of whether the installation
of the fetched resource succeeds or fails.

While the existing code updates the time stamp only when the subsequent
installation of the newly received script is successfully completed (or the
loaded script is byte-for-byte match to the incumbent script resource), this CL
makes it bump the time stamp as soon as the update succeeded.

This CL added a condition for checking whether the script resource was served
from the network:
if (net_request_->response_info().network_accessed &&
    !(net_request_->response_info().was_cached)) // This line is added.

Spec discussion:
https://github.com/slightlyoff/ServiceWorker/issues/514#issuecomment-135641718

BUG=539709

Committed: https://crrev.com/2c7aaaf09eab313a07c425c474256f92957da931
Cr-Commit-Position: refs/heads/master@{#371485}
==========

[commit-bot: I haz the power, 2016-01-26 10:05:13]

Patchset 11 (id:??) landed as
https://crrev.com/2c7aaaf09eab313a07c425c474256f92957da931
Cr-Commit-Position: refs/heads/master@{#371485}