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

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 244905ef69c213e2d69d80e35acbdb899aa413a2..f3a121e66337cd3d9cb85d24461410551fe575f4 100644
--- a/content/browser/service_worker/service_worker_register_job.cc
+++ b/content/browser/service_worker/service_worker_register_job.cc
@@ -350,6 +350,18 @@ void ServiceWorkerRegisterJob::UpdateAndContinue() {
 
 void ServiceWorkerRegisterJob::OnStartWorkerFinished(
     ServiceWorkerStatusCode status) {
+  // Bump the last update check time only when the register/update job fetched
+  // the version having bypassed the network cache. As bypassing the network
+  // cache will evict an existing cache entry, it is okay to bump the update
+  // check time.

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

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]

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.

+  if (new_version()->embedded_worker()->network_accessed_for_script() ||
+      new_version()->force_bypass_cache_for_scripts()) {
+    registration()->set_last_update_check(base::Time::Now());
+
+    if (job_type_ == UPDATE_JOB)
+      context_->storage()->UpdateLastUpdateCheckTime(registration());
+  }
+
   if (status == SERVICE_WORKER_OK) {
     InstallAndContinue();
     return;
@@ -357,16 +369,6 @@ 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) ||
-        new_version()->force_bypass_cache_for_scripts()) {
-      registration()->set_last_update_check(base::Time::Now());
-      context_->storage()->UpdateLastUpdateCheckTime(registration());
-    }
-
     ResolvePromise(SERVICE_WORKER_OK, std::string(), registration());
     Complete(status, "The updated worker is identical to the incumbent.");
     return;
@@ -431,7 +433,6 @@ void ServiceWorkerRegisterJob::OnInstallFinished(
   }
 
   SetPhase(STORE);
-  registration()->set_last_update_check(base::Time::Now());
   context_->storage()->StoreRegistration(
       registration(),
       new_version(),