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

content/browser/service_worker/service_worker_register_job.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/service_worker/service_worker_register_job.h"
 
 #include <vector>
 
 #include "base/location.h"
 #include "base/single_thread_task_runner.h"
@@ skipping 327 matching lines @@
 void ServiceWorkerRegisterJob::OnStartWorkerFinished(
     ServiceWorkerStatusCode status) {
   if (status == SERVICE_WORKER_OK) {
     InstallAndContinue();
     return;
   }
 
   // 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());
     }
 
     ResolvePromise(SERVICE_WORKER_OK, std::string(), registration());
     Complete(status, "The updated worker is identical to the incumbent.");
     return;
   }
 
   // "If serviceWorker fails to start up..." then reject the promise with an
@@ skipping 169 matching lines @@
     if (host->IsHostToRunningServiceWorker())
       continue;
     if (!ServiceWorkerUtils::ScopeMatches(registration->pattern(),
                                           host->document_url()))
       continue;
     host->AddMatchingRegistration(registration);
   }
 }
 
 }  // namespace content