Service Worker: Change last update check location and HTTP cache bypass rule (2/2)

content/browser/service_worker/service_worker_register_job.cc

Index: content/browser/service_worker/service_worker_register_job.cc
diff --git a/content/browser/service_worker/service_worker_register_job.cc b/content/browser/service_worker/service_worker_register_job.cc
index 16a192858271e92bc9532362686253dc9bb308fb..ee8887a1b4dfdb3fe89ecb1c18830e02632ca7e2 100644
--- a/content/browser/service_worker/service_worker_register_job.cc
+++ b/content/browser/service_worker/service_worker_register_job.cc
@@ -345,10 +345,7 @@ void ServiceWorkerRegisterJob::OnStartWorkerFinished(
   // The updated worker is identical to the incumbent.
   if (status == SERVICE_WORKER_ERROR_EXISTS) {
     // Only bump the last check time when we've bypassed the browser cache.
-    base::TimeDelta time_since_last_check =
-        base::Time::Now() - registration()->last_update_check();
-    if (time_since_last_check > base::TimeDelta::FromHours(
-                                    kServiceWorkerScriptMaxCacheAgeInHours) ||
+    if (new_version()->embedded_worker()->network_accessed_for_script() ||

[nhiroki, 2015/08/24 07:57:51]

This could break some content_unittests?

[jungkees, 2015/08/25 09:36:51]

Ah yes.. I found ServiceWorkerJobTest.Update_BumpLastUpdateCheckTime needs some
changes. I'll work on that.

BTW, I noticed there seems to be a discrepancy in using the last update time
between the spec and the implementation. Currently the spec does not update the
registration's last update time internal slot when the UA hits the network
bypassing HTTP cache but the script is byte-by-byte identical (i.e. 304 case.)
But this line of code updates the value in this case. I think it needs to be
sorted out to update the code.

[nhiroki, 2015/08/28 09:19:55]

Thank you for clarifying the spec. The latest spec looks good[1]. I'll take
another look when this CL is ready to review :)

[1]
https://github.com/slightlyoff/ServiceWorker/commit/569863f3acd0f5d79beab9d8c...

[jungkees, 2015/09/04 12:56:57]

As per the latest spec change, I moved this part to
ServiceWorkerWriteToCacheJob::OnResponseStarted
(service_worker_write_to_cache_job.cc), and also removed
ServiceWorkerJobTest.Update_BumpLastUpdateCheckTime and created
ServiceWorkerWriteToCacheJobTest.Update_BumpLastUpdateCheckTime.

         new_version()->force_bypass_cache_for_scripts()) {

[nhiroki, 2015/08/24 07:57:51]

This second condition seems no longer necessary because the first condition
contains it.

[jungkees, 2015/08/25 09:36:51]

Yes. I'll remove this second condition.

       registration()->set_last_update_check(base::Time::Now());
       context_->storage()->UpdateLastUpdateCheckTime(registration());