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 60dc34b42a1a9f823e7b994bd18abcfc19aa3dfd..2cfe701cb99ddffe360e45f08d7cac450110eed1 100644
--- a/content/browser/service_worker/service_worker_register_job.cc
+++ b/content/browser/service_worker/service_worker_register_job.cc
@@ -340,6 +340,13 @@ void ServiceWorkerRegisterJob::UpdateAndContinue() {
 
 void ServiceWorkerRegisterJob::OnStartWorkerFinished(
     ServiceWorkerStatusCode status) {
+  // Only bump the last check time when we've bypassed the browser cache.
+  if (new_version()->embedded_worker()->network_accessed_for_script() ||
+      new_version()->force_bypass_cache_for_scripts()) {
+    registration()->set_last_update_check(base::Time::Now());
+    context_->storage()->UpdateLastUpdateCheckTime(registration());

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

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:45:17]

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.

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

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.

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

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]

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]

Great. I made it only UPDATE_JOBs write to disk.

+  }
+
   if (status == SERVICE_WORKER_OK) {
     InstallAndContinue();
     return;
@@ -347,16 +354,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;
@@ -421,7 +418,6 @@ void ServiceWorkerRegisterJob::OnInstallFinished(
   }
 
   SetPhase(STORE);
-  registration()->set_last_update_check(base::Time::Now());
   context_->storage()->StoreRegistration(
       registration(),
       new_version(),