Implement updateViaCache flag and no-cache by default for main service worker scripts

content/browser/service_worker/service_worker_context_request_handler.cc

 // Copyright 2014 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_context_request_handler.h"
 
 #include "base/time/time.h"
 #include "content/browser/service_worker/service_worker_context_core.h"
 #include "content/browser/service_worker/service_worker_provider_host.h"
 #include "content/browser/service_worker/service_worker_read_from_cache_job.h"
@@ skipping 173 matching lines @@
   if (resource_id == kInvalidServiceWorkerResourceId) {
     *out_status = CreateJobStatus::ERROR_OUT_OF_RESOURCE_IDS;
     return nullptr;
   }
 
   // Bypass the browser cache for initial installs and update checks after 24
   // hours have passed.
   int extra_load_flags = 0;
   base::TimeDelta time_since_last_check =
       base::Time::Now() - registration->last_update_check();
-  if (time_since_last_check > kServiceWorkerScriptMaxCacheAge ||
+  bool should_bypass_cache =
+      (is_main_script && registration->update_via_cache() !=
+                             blink::WebServiceWorkerUpdateViaCache::kAll) ||
+      registration->update_via_cache() ==
+          blink::WebServiceWorkerUpdateViaCache::kNone;

[nhiroki, 2017/08/04 09:24:50]

Would it be possible to move this policy decision to a separate function like
DetermineIfShouldBypassCache() as follows and add a unit test that covers all
possible combinations?

bool DetermineIfShouldBypassCache(
    bool is_main_script,
    blink::WebServiceWorkerUpdateViaCache cache_mode) {
  if (cache_mode == blink::WebServiceWorkerUpdateViaCache::kNone)
    return true;
  if (is_main_script && blink::WebServiceWorkerUpdateViaCache::kImported)
    return true;
  return false;
}

[yuryu, 2017/08/17 07:36:35]

I think it makes sense to extract the part to another method as it is fairly
complex already, but I'm not sure if we can/should test functions with internal
linkage.

+  if (should_bypass_cache ||
+      time_since_last_check > kServiceWorkerScriptMaxCacheAge ||
       version_->force_bypass_cache_for_scripts()) {
     extra_load_flags = net::LOAD_BYPASS_CACHE;
   }
 
   ServiceWorkerVersion* stored_version = registration->waiting_version()
                                              ? registration->waiting_version()
                                              : registration->active_version();
   int64_t incumbent_resource_id = kInvalidServiceWorkerResourceId;
   if (is_main_script) {
     if (stored_version && stored_version->script_url() == request->url()) {
       incumbent_resource_id =
           stored_version->script_cache_map()->LookupResourceId(request->url());
     }
     version_->embedded_worker()->OnURLJobCreatedForMainScript();
   }
   *out_status = incumbent_resource_id == kInvalidServiceWorkerResourceId
                     ? CreateJobStatus::WRITE_JOB
                     : CreateJobStatus::WRITE_JOB_WITH_INCUMBENT;
   return new ServiceWorkerWriteToCacheJob(
       request, network_delegate, resource_type_, context_, version_.get(),
       extra_load_flags, resource_id, incumbent_resource_id);
 }
 
 }  // namespace content