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

updateViaCache is an attribute on ServiceWorkerRegistration that controls whether the browser cache (the HTTP cache) is used when performing an update check.

Service workers consist of a main script and possibly imported scripts. The service worker update check involves fetching the current scripts and comparing them to the installed scripts. This intent to ship concerns whether fetching the current scripts uses or bypasses the browser cache.

BUG=675540

[yuryu, 2017-03-23 13:49:42]

The CQ bit was checked by yuryu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 13:49:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 14:00:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-23 14:00:55]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[nhiroki, 2017-03-24 00:33:41]

nhiroki@chromium.org changed reviewers:
+ nhiroki@chromium.org

[nhiroki, 2017-03-24 00:33:42]

Thank you for working on this. Added unsolicited comments :)

> Should I split it into two or more? Changes in each file are not very big, but
they are scattered across many files.

I'm ok with one patch because changes are not complicated.

https://codereview.chromium.org/2771823002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_context_core.h (right):

https://codereview.chromium.org/2771823002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_context_core.h:199: bool
use_cache,
Enum would be better than boolean for readability[1]. We could define it in
service_worker_types.h, for example, as follows:

  struct CONTENT_EXPORT ServiceWorkerRegistrationOptions {
    enum class UseCache { DontUse, Use };
    // ...
    UseCache use_cache;
  };

[1] "Consider changing the function signature to replace a bool argument with an
enum argument. This will make the argument values self-describing."
https://google.github.io/styleguide/cppguide.html#Implementation_Comments

https://codereview.chromium.org/2771823002/diff/1/content/child/service_worke...
File content/child/service_worker/service_worker_registration_handle_reference.h
(right):

https://codereview.chromium.org/2771823002/diff/1/content/child/service_worke...
content/child/service_worker/service_worker_registration_handle_reference.h:44:
bool useCache() const { return info_.use_cache; }
use_cache() (we use the snake_case under content/)

https://codereview.chromium.org/2771823002/diff/1/content/common/service_work...
File content/common/service_worker/service_worker_types.h (right):

https://codereview.chromium.org/2771823002/diff/1/content/common/service_work...
content/common/service_worker/service_worker_types.h:226: bool use_cache;
|scope| and |use_cache| could be replaced with ServiceWorkerRegistrationOptions
(see also my following comment in this file)

https://codereview.chromium.org/2771823002/diff/1/content/common/service_work...
content/common/service_worker/service_worker_types.h:230: struct CONTENT_EXPORT
ServiceWorkerRegistrationAttributes {
"RegistrationAttributes" could sound a bit confusing. We use "VersionAttributes"
(see line 236) for updating ServiceWorker JS-object's attributes, but this
struct isn't used for such a purpose.

Since "script_url" is a required param of register() and others (pattern,
use_cache) are defined as optional params, how about excluding "script_url" from
this struct and then renaming this to ServiceWorkerRegistrationOptions? This
name would also be consistent with the spec:
https://w3c.github.io/ServiceWorker/#dictdef-registrationoptions

https://codereview.chromium.org/2771823002/diff/1/content/public/browser/serv...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/2771823002/diff/1/content/public/browser/serv...
content/public/browser/service_worker_context.h:55: // pattern, bool:
use_cache}) from a renderer, except that |pattern| is an
"bool: use_cache" => "useCache: use_cache" ?

[nhiroki, 2017-03-24 00:46:42]

nhiroki@chromium.org changed reviewers:
+ falken@chromium.org

[nhiroki, 2017-03-24 00:46:43]

+falken. I know this is still WIP and not ready for formal reviews, but early
comments would be helpful to save the cost of work (one line change on this
large CL may affect a bunch of files...)

[falken, 2017-03-24 04:06:20]

This looks reasonable to me and I agree with nhiroki's comments.

https://codereview.chromium.org/2771823002/diff/1/content/common/service_work...
File content/common/service_worker/service_worker_types.h (right):

https://codereview.chromium.org/2771823002/diff/1/content/common/service_work...
content/common/service_worker/service_worker_types.h:230: struct CONTENT_EXPORT
ServiceWorkerRegistrationAttributes {
On 2017/03/24 00:33:42, nhiroki wrote:
> "RegistrationAttributes" could sound a bit confusing. We use
"VersionAttributes"
> (see line 236) for updating ServiceWorker JS-object's attributes, but this
> struct isn't used for such a purpose.
> 
> Since "script_url" is a required param of register() and others (pattern,
> use_cache) are defined as optional params, how about excluding "script_url"
from
> this struct and then renaming this to ServiceWorkerRegistrationOptions? This
> name would also be consistent with the spec:
> https://w3c.github.io/ServiceWorker/#dictdef-registrationoptions

+1, and then using this struct when possible when passing the information to
various function.

https://codereview.chromium.org/2771823002/diff/1/content/public/browser/serv...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/2771823002/diff/1/content/public/browser/serv...
content/public/browser/service_worker_context.h:71: bool use_cache,
Do we need to expose this use_cache to ServiceWorkerContext, or is it possible
to be only on ServiceWorkerContextCore? We may be able to save some callsites
passing false if it's just ContextCore that needs it.

Also, seems better if we can use ServiceWorkerRegistrationOptions here instead
of the individual paramenters.

[yuryu, 2017-03-24 04:45:55]

https://codereview.chromium.org/2771823002/diff/1/content/public/browser/serv...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/2771823002/diff/1/content/public/browser/serv...
content/public/browser/service_worker_context.h:71: bool use_cache,
On 2017/03/24 04:06:19, falken wrote:
> Do we need to expose this use_cache to ServiceWorkerContext, or is it possible
> to be only on ServiceWorkerContextCore? We may be able to save some callsites
> passing false if it's just ContextCore that needs it.

https://cs.chromium.org/chromium/src/content/browser/service_worker/link_head...

link_header_support.c uses ServiceWorkerContext, and it also needs control of
useCache (and WorkerType in the future). So I think we need to change it to use
ServiceWorkerRegistrationOptions too. Am I correct?

[falken, 2017-03-27 06:15:37]

https://codereview.chromium.org/2771823002/diff/1/content/public/browser/serv...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/2771823002/diff/1/content/public/browser/serv...
content/public/browser/service_worker_context.h:71: bool use_cache,
On 2017/03/24 04:45:55, yuryu wrote:
> On 2017/03/24 04:06:19, falken wrote:
> > Do we need to expose this use_cache to ServiceWorkerContext, or is it
possible
> > to be only on ServiceWorkerContextCore? We may be able to save some
callsites
> > passing false if it's just ContextCore that needs it.
> 
>
https://cs.chromium.org/chromium/src/content/browser/service_worker/link_head...
> 
> link_header_support.c uses ServiceWorkerContext, and it also needs control of
> useCache (and WorkerType in the future). So I think we need to change it to
use
> ServiceWorkerRegistrationOptions too. Am I correct?

Yes, you're right.

[yuryu, 2017-03-28 05:08:56]

https://codereview.chromium.org/2771823002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_context_core.h (right):

https://codereview.chromium.org/2771823002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_context_core.h:199: bool
use_cache,
On 2017/03/24 00:33:42, nhiroki wrote:
> Enum would be better than boolean for readability[1]. We could define it in
> service_worker_types.h, for example, as follows:
> 
>   struct CONTENT_EXPORT ServiceWorkerRegistrationOptions {
>     enum class UseCache { DontUse, Use };
>     // ...
>     UseCache use_cache;
>   };
> 
> [1] "Consider changing the function signature to replace a bool argument with
an
> enum argument. This will make the argument values self-describing."
> https://google.github.io/styleguide/cppguide.html#Implementation_Comments

Done.

https://codereview.chromium.org/2771823002/diff/1/content/child/service_worke...
File content/child/service_worker/service_worker_registration_handle_reference.h
(right):

https://codereview.chromium.org/2771823002/diff/1/content/child/service_worke...
content/child/service_worker/service_worker_registration_handle_reference.h:44:
bool useCache() const { return info_.use_cache; }
On 2017/03/24 00:33:42, nhiroki wrote:
> use_cache() (we use the snake_case under content/)

Done.

https://codereview.chromium.org/2771823002/diff/1/content/common/service_work...
File content/common/service_worker/service_worker_types.h (right):

https://codereview.chromium.org/2771823002/diff/1/content/common/service_work...
content/common/service_worker/service_worker_types.h:226: bool use_cache;
On 2017/03/24 00:33:42, nhiroki wrote:
> |scope| and |use_cache| could be replaced with
ServiceWorkerRegistrationOptions
> (see also my following comment in this file)

Done.

https://codereview.chromium.org/2771823002/diff/1/content/common/service_work...
content/common/service_worker/service_worker_types.h:230: struct CONTENT_EXPORT
ServiceWorkerRegistrationAttributes {
On 2017/03/24 04:06:19, falken wrote:
> On 2017/03/24 00:33:42, nhiroki wrote:
> > "RegistrationAttributes" could sound a bit confusing. We use
> "VersionAttributes"
> > (see line 236) for updating ServiceWorker JS-object's attributes, but this
> > struct isn't used for such a purpose.
> > 
> > Since "script_url" is a required param of register() and others (pattern,
> > use_cache) are defined as optional params, how about excluding "script_url"
> from
> > this struct and then renaming this to ServiceWorkerRegistrationOptions? This
> > name would also be consistent with the spec:
> > https://w3c.github.io/ServiceWorker/#dictdef-registrationoptions
> 
> +1, and then using this struct when possible when passing the information to
> various function.

Done.

https://codereview.chromium.org/2771823002/diff/1/content/public/browser/serv...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/2771823002/diff/1/content/public/browser/serv...
content/public/browser/service_worker_context.h:55: // pattern, bool:
use_cache}) from a renderer, except that |pattern| is an
On 2017/03/24 00:33:42, nhiroki wrote:
> "bool: use_cache" => "useCache: use_cache" ?

Done.

https://codereview.chromium.org/2771823002/diff/1/content/public/browser/serv...
content/public/browser/service_worker_context.h:71: bool use_cache,
On 2017/03/27 06:15:37, falken wrote:
> On 2017/03/24 04:45:55, yuryu wrote:
> > On 2017/03/24 04:06:19, falken wrote:
> > > Do we need to expose this use_cache to ServiceWorkerContext, or is it
> possible
> > > to be only on ServiceWorkerContextCore? We may be able to save some
> callsites
> > > passing false if it's just ContextCore that needs it.
> > 
> >
>
https://cs.chromium.org/chromium/src/content/browser/service_worker/link_head...
> > 
> > link_header_support.c uses ServiceWorkerContext, and it also needs control
of
> > useCache (and WorkerType in the future). So I think we need to change it to
> use
> > ServiceWorkerRegistrationOptions too. Am I correct?
> 
> Yes, you're right.

Acknowledged.

[yuryu, 2017-03-28 05:09:08]

The CQ bit was checked by yuryu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 05:09:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-28 05:13:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-28 05:13:25]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[yuryu, 2017-03-28 06:13:38]

The CQ bit was checked by yuryu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 06:13:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-28 06:35:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-28 06:35:47]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[nhiroki, 2017-03-28 06:49:52]

Looks good overall.

Can you flesh out the CL description? A CL that launches a new feature typically
contains links to the spec, the dashboard entry (chromestatus.com), and the
intent-to-implement-and-ship.

In addition, you might need to tweak layout tests that depend on the current
cache behavior (ie., useCache==true). IIRC, following tests depend on it:

- http/tests/serviceworker/update-served-from-cache.html
(resources/update-served-from-cache-worker.php)
- http/tests/serviceworker/update.html (resources/update-worker.php)
- http/tests/serviceworker/ServiceWorkerGlobalScope/update.html
(ServiceWorkerGlobalScope/resources/update-worker.php)

https://codereview.chromium.org/2771823002/diff/20001/content/browser/service...
File content/browser/service_worker/link_header_support_unittest.cc (right):

https://codereview.chromium.org/2771823002/diff/20001/content/browser/service...
content/browser/service_worker/link_header_support_unittest.cc:115: false /*
use_cache* */, 1L,
There is an extra * ("use_cache*")

https://codereview.chromium.org/2771823002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_registration.h (right):

https://codereview.chromium.org/2771823002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_registration.h:199: const bool
use_cache_;
Why don't you use ServiceWorkerRegistrationOptions::UseCache?

https://codereview.chromium.org/2771823002/diff/20001/content/common/service_...
File content/common/service_worker/service_worker_types.cc (right):

https://codereview.chromium.org/2771823002/diff/20001/content/common/service_...
content/common/service_worker/service_worker_types.cc:143: : scope(scope),
use_cache(UseCache::DontUse) {}
Delegating constructor would work here:

ServiceWorkerRegistrationOptions::ServiceWorkerRegistrationOptions(
    const GURL& scope)
    : ServiceWorkerRegistrationOptions(scope, UseCache::DontUse) {}

https://codereview.chromium.org/2771823002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerContainerTest.cpp
(right):

https://codereview.chromium.org/2771823002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerContainerTest.cpp:361:
EXPECT_EQ(false, stubProvider.useCache());
Can you use EXPECT_FALSE?

  EXPECT_FALSE(stubProvider.useCache());

https://codereview.chromium.org/2771823002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerContainerTest.cpp:407:
EXPECT_EQ(true, stubProvider.useCache());
EXPECT_TRUE(stubProvider.useCache());

[yuryu, 2017-03-28 07:07:02]

On 2017/03/28 06:49:52, nhiroki wrote:
> Can you flesh out the CL description? A CL that launches a new feature
typically
> contains links to the spec, the dashboard entry (http://chromestatus.com), and
the
> intent-to-implement-and-ship.

I'll send an intent-to-implement-and-ship and update the description
accordingly.

> In addition, you might need to tweak layout tests that depend on the current
> cache behavior (ie., useCache==true). IIRC, following tests depend on it:

Will update them too.

https://codereview.chromium.org/2771823002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_registration.h (right):

https://codereview.chromium.org/2771823002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_registration.h:199: const bool
use_cache_;
On 2017/03/28 06:49:51, nhiroki wrote:
> Why don't you use ServiceWorkerRegistrationOptions::UseCache?

I thought it would make sense to align the type with ServiceWorkerRegistration
in the spec
https://w3c.github.io/ServiceWorker/#serviceworkerregistration-interface

If this alignment is not necessary, I'll change it to
ServiceWorkerRegistrationOptions::UseCache.

[nhiroki, 2017-03-28 07:21:50]

https://codereview.chromium.org/2771823002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_registration.h (right):

https://codereview.chromium.org/2771823002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_registration.h:199: const bool
use_cache_;
On 2017/03/28 07:07:02, yuryu wrote:
> On 2017/03/28 06:49:51, nhiroki wrote:
> > Why don't you use ServiceWorkerRegistrationOptions::UseCache?
> 
> I thought it would make sense to align the type with ServiceWorkerRegistration
> in the spec
> https://w3c.github.io/ServiceWorker/#serviceworkerregistration-interface
> 
> If this alignment is not necessary, I'll change it to
> ServiceWorkerRegistrationOptions::UseCache.

IMO, we don't have to make the implementation completely aligned with the spec
if the implementation is much clearer and the user-facing behavior is not
changed.

[falken, 2017-04-05 01:01:08]

Heads-up that the spec here is changing:
https://github.com/w3c/ServiceWorker/issues/893

I think we should update the patch in the direction of the decision there. For
the purpose of moving this forward, it might make sense to land the patch behind
a flag and then enable it by default once the spec has settled.

[yuryu, 2017-04-05 02:25:24]

Thanks for the update. I'll implement it as a boolean behind a flag.

The new proposal: https://github.com/w3c/ServiceWorker/issues/1104

[yuryu, 2017-06-21 12:37:01]

The CQ bit was checked by yuryu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-21 12:37:14]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yuryu, 2017-06-21 12:40:49]

I've rebased and made changes, so the delta contains unrelated changes...
Now I need to add/fix some tests, but basically the main work is done.

Intent to implement and ship is under review here:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/RwjXGTAbuuA

[yuryu, 2017-06-21 12:41:58]

Description was changed from

==========
Implement useCache flag and no-cache by default for service worker scripts

BUG=
==========

to

==========
updateViaCache is an attribute on ServiceWorkerRegistration that controls
whether the browser cache (the HTTP cache) is used when performing an update
check.

Service workers consist of a main script and possibly imported scripts. The
service worker update check involves fetching the current scripts and comparing
them to the installed scripts. This intent to ship concerns whether fetching the
current scripts uses or bypasses the browser cache.

BUG=675540
==========

[commit-bot: I haz the power, 2017-06-21 13:22:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-21 13:22:06]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[nhiroki, 2017-06-21 14:08:35]

Partially commented.

https://codereview.chromium.org/2771823002/diff/80001/android_webview/tools/s...
File
android_webview/tools/system_webview_shell/test/data/webexposed/global-interface-listing-expected.txt
(right):

https://codereview.chromium.org/2771823002/diff/80001/android_webview/tools/s...
android_webview/tools/system_webview_shell/test/data/webexposed/global-interface-listing-expected.txt:1953:
getter useCache
s/useCache/updateViaCache/

https://codereview.chromium.org/2771823002/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_database.h (right):

https://codereview.chromium.org/2771823002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_database.h:27: #include
"url/origin.h"
Can you include "WebServiceWorkerUpdateViaCache.h"?

https://codereview.chromium.org/2771823002/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_database.proto (right):

https://codereview.chromium.org/2771823002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_database.proto:58: optional uint32
update_via_cache = 14;
How about using Enum?
https://developers.google.com/protocol-buffers/docs/proto#enum

[nhiroki, 2017-06-21 15:04:59]

https://codereview.chromium.org/2771823002/diff/80001/content/browser/service...
File content/browser/service_worker/link_header_support.cc (right):

https://codereview.chromium.org/2771823002/diff/80001/content/browser/service...
content/browser/service_worker/link_header_support.cc:36:
blink::WebServiceWorkerUpdateViaCache ParseUpdateViaCache(
Can you add a link to
https://html.spec.whatwg.org/multipage/semantics.html#attr-link-updateviacache ?

https://codereview.chromium.org/2771823002/diff/80001/content/browser/service...
content/browser/service_worker/link_header_support.cc:45: if
(base::EqualsCaseInsensitiveASCII(cache, "never"))
never -> none?
https://w3c.github.io/ServiceWorker/#dfn-update-via-cache

https://codereview.chromium.org/2771823002/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_context_request_handler.cc
(right):

https://codereview.chromium.org/2771823002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_context_request_handler.cc:212:
bool no_update_via_cache =
|no_update_via_cache| sounds a bit confusing. How about |should_bypass_cache|?

https://codereview.chromium.org/2771823002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_context_request_handler.cc:215:
(!is_main_script && registration->update_via_cache() ==
|!is_main_script| would be unnecessary.

[yuryu, 2017-07-20 10:15:12]

https://codereview.chromium.org/2771823002/diff/20001/content/browser/service...
File content/browser/service_worker/link_header_support_unittest.cc (right):

https://codereview.chromium.org/2771823002/diff/20001/content/browser/service...
content/browser/service_worker/link_header_support_unittest.cc:115: false /*
use_cache* */, 1L,
On 2017/03/28 06:49:51, nhiroki wrote:
> There is an extra * ("use_cache*")

Acknowledged.

https://codereview.chromium.org/2771823002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_registration.h (right):

https://codereview.chromium.org/2771823002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_registration.h:199: const bool
use_cache_;
On 2017/03/28 07:21:50, nhiroki wrote:
> On 2017/03/28 07:07:02, yuryu wrote:
> > On 2017/03/28 06:49:51, nhiroki wrote:
> > > Why don't you use ServiceWorkerRegistrationOptions::UseCache?
> > 
> > I thought it would make sense to align the type with
ServiceWorkerRegistration
> > in the spec
> > https://w3c.github.io/ServiceWorker/#serviceworkerregistration-interface
> > 
> > If this alignment is not necessary, I'll change it to
> > ServiceWorkerRegistrationOptions::UseCache.
> 
> IMO, we don't have to make the implementation completely aligned with the spec
> if the implementation is much clearer and the user-facing behavior is not
> changed.

Acknowledged.

https://codereview.chromium.org/2771823002/diff/20001/content/common/service_...
File content/common/service_worker/service_worker_types.cc (right):

https://codereview.chromium.org/2771823002/diff/20001/content/common/service_...
content/common/service_worker/service_worker_types.cc:143: : scope(scope),
use_cache(UseCache::DontUse) {}
On 2017/03/28 06:49:51, nhiroki wrote:
> Delegating constructor would work here:
> 
> ServiceWorkerRegistrationOptions::ServiceWorkerRegistrationOptions(
>     const GURL& scope)
>     : ServiceWorkerRegistrationOptions(scope, UseCache::DontUse) {}

Done.

https://codereview.chromium.org/2771823002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerContainerTest.cpp
(right):

https://codereview.chromium.org/2771823002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerContainerTest.cpp:361:
EXPECT_EQ(false, stubProvider.useCache());
On 2017/03/28 06:49:51, nhiroki wrote:
> Can you use EXPECT_FALSE?
> 
>   EXPECT_FALSE(stubProvider.useCache());

Acknowledged.

https://codereview.chromium.org/2771823002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerContainerTest.cpp:407:
EXPECT_EQ(true, stubProvider.useCache());
On 2017/03/28 06:49:51, nhiroki wrote:
> EXPECT_TRUE(stubProvider.useCache());

Acknowledged.

https://codereview.chromium.org/2771823002/diff/80001/android_webview/tools/s...
File
android_webview/tools/system_webview_shell/test/data/webexposed/global-interface-listing-expected.txt
(right):

https://codereview.chromium.org/2771823002/diff/80001/android_webview/tools/s...
android_webview/tools/system_webview_shell/test/data/webexposed/global-interface-listing-expected.txt:1953:
getter useCache
On 2017/06/21 14:08:35, nhiroki wrote:
> s/useCache/updateViaCache/

Done.

https://codereview.chromium.org/2771823002/diff/80001/content/browser/service...
File content/browser/service_worker/link_header_support.cc (right):

https://codereview.chromium.org/2771823002/diff/80001/content/browser/service...
content/browser/service_worker/link_header_support.cc:36:
blink::WebServiceWorkerUpdateViaCache ParseUpdateViaCache(
On 2017/06/21 15:04:58, nhiroki wrote:
> Can you add a link to
> https://html.spec.whatwg.org/multipage/semantics.html#attr-link-updateviacache
?

Done.

https://codereview.chromium.org/2771823002/diff/80001/content/browser/service...
content/browser/service_worker/link_header_support.cc:45: if
(base::EqualsCaseInsensitiveASCII(cache, "never"))
On 2017/06/21 15:04:58, nhiroki wrote:
> never -> none?
> https://w3c.github.io/ServiceWorker/#dfn-update-via-cache

Done.

https://codereview.chromium.org/2771823002/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_context_request_handler.cc
(right):

https://codereview.chromium.org/2771823002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_context_request_handler.cc:212:
bool no_update_via_cache =
On 2017/06/21 15:04:59, nhiroki wrote:
> |no_update_via_cache| sounds a bit confusing. How about |should_bypass_cache|?

Done.

https://codereview.chromium.org/2771823002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_context_request_handler.cc:215:
(!is_main_script && registration->update_via_cache() ==
On 2017/06/21 15:04:59, nhiroki wrote:
> |!is_main_script| would be unnecessary.

I intended to reflect the spec, where algorithms for main scripts and imported
scripts are separately written.
https://w3c.github.io/ServiceWorker/#importscripts
But I agree, it is not necessary.

https://codereview.chromium.org/2771823002/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_database.proto (right):

https://codereview.chromium.org/2771823002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_database.proto:58: optional uint32
update_via_cache = 14;
On 2017/06/21 14:08:35, nhiroki wrote:
> How about using Enum?
> https://developers.google.com/protocol-buffers/docs/proto#enum

Done.

[yuryu, 2017-07-20 10:15:31]

The CQ bit was checked by yuryu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-20 10:15:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-20 11:09:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-20 11:09:04]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[nhiroki, 2017-07-21 04:39:46]

Partially reviewed. I'll take a closer look later.

https://codereview.chromium.org/2771823002/diff/100001/content/browser/servic...
File
content/browser/service_worker/service_worker_context_request_handler_unittest.cc
(right):

https://codereview.chromium.org/2771823002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_context_request_handler_unittest.cc:69:
SetUpServiceWorker();
All tests seem to explicitly call SetUpServiceWorker() or
SetUpServiceWorkerWithOptions(). How about stopping calling SetUpServiceWorker()
in SetUp()?

https://codereview.chromium.org/2771823002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_context_request_handler_unittest.cc:89:
blink::WebServiceWorkerUpdateViaCache::kImports);
How about using default value? Default value in production code is basically
discouraged, but it's not in test code :)

https://codereview.chromium.org/2771823002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_context_request_handler_unittest.cc:94:
// A new unstored registration/version.
I feel this comment is not so useful for reading code.

https://codereview.chromium.org/2771823002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerRegistration.cpp
(right):

https://codereview.chromium.org/2771823002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerRegistration.cpp:98:
return "never";
"never" -> "none"?

https://codereview.chromium.org/2771823002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerRegistration.cpp:99:
default:
I'd prefer to avoid 'default' as much as possible because 'default' disables the
compiler's coverage check when add a new enum value. Also, can you add
NOTRACHED() check like this?

    case WebServiceWorkerUpdateViaCache::kUnknown:
    case WebServiceWorkerUpdateViaCache::kLast:
      break;
  }
  NOTREACHED();
  return "imports";

[yuryu, 2017-08-03 04:19:48]

The CQ bit was checked by yuryu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-03 04:19:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yuryu, 2017-08-03 04:20:05]

Still working to fix tests

https://codereview.chromium.org/2771823002/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_database.h (right):

https://codereview.chromium.org/2771823002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_database.h:27: #include
"url/origin.h"
On 2017/06/21 14:08:35, nhiroki wrote:
> Can you include "WebServiceWorkerUpdateViaCache.h"?

Done.

https://codereview.chromium.org/2771823002/diff/100001/content/browser/servic...
File
content/browser/service_worker/service_worker_context_request_handler_unittest.cc
(right):

https://codereview.chromium.org/2771823002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_context_request_handler_unittest.cc:69:
SetUpServiceWorker();
On 2017/07/21 04:39:46, nhiroki wrote:
> All tests seem to explicitly call SetUpServiceWorker() or
> SetUpServiceWorkerWithOptions(). How about stopping calling
SetUpServiceWorker()
> in SetUp()?

Actually these SetUpServiceWorker calls were not necessary so removing them
instead.

https://codereview.chromium.org/2771823002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_context_request_handler_unittest.cc:89:
blink::WebServiceWorkerUpdateViaCache::kImports);
On 2017/07/21 04:39:46, nhiroki wrote:
> How about using default value? Default value in production code is basically
> discouraged, but it's not in test code :)

Merged SetUpServiceWorker and SetUpServiceWorkerWithOptions and make the
argument mandatory as the only call with the kImports value is in SetUp().

https://codereview.chromium.org/2771823002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_context_request_handler_unittest.cc:94:
// A new unstored registration/version.
On 2017/07/21 04:39:46, nhiroki wrote:
> I feel this comment is not so useful for reading code.

Removed.

https://codereview.chromium.org/2771823002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerRegistration.cpp
(right):

https://codereview.chromium.org/2771823002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerRegistration.cpp:98:
return "never";
On 2017/07/21 04:39:46, nhiroki wrote:
> "never" -> "none"?

Done.

https://codereview.chromium.org/2771823002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerRegistration.cpp:99:
default:
On 2017/07/21 04:39:46, nhiroki wrote:
> I'd prefer to avoid 'default' as much as possible because 'default' disables
the
> compiler's coverage check when add a new enum value. Also, can you add
> NOTRACHED() check like this?
> 
>     case WebServiceWorkerUpdateViaCache::kUnknown:
>     case WebServiceWorkerUpdateViaCache::kLast:
>       break;
>   }
>   NOTREACHED();
>   return "imports";

Done.

[commit-bot: I haz the power, 2017-08-03 04:24:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-08-03 04:24:05]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[yuryu, 2017-08-03 04:51:45]

The CQ bit was checked by yuryu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-03 04:51:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-03 05:08:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-08-03 05:08:25]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yuryu, 2017-08-03 08:04:56]

The CQ bit was checked by yuryu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-03 08:05:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-03 09:31:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-08-03 09:31:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yuryu, 2017-08-04 07:34:01]

The CQ bit was checked by yuryu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-04 07:34:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-04 07:38:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-08-04 07:38:16]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nhiroki, 2017-08-04 09:24:51]

Looking good.

https://codereview.chromium.org/2771823002/diff/180001/content/browser/servic...
File content/browser/service_worker/service_worker_context_request_handler.cc
(right):

https://codereview.chromium.org/2771823002/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_context_request_handler.cc:198:
blink::WebServiceWorkerUpdateViaCache::kNone;
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;
}

https://codereview.chromium.org/2771823002/diff/180001/content/browser/servic...
File content/browser/service_worker/service_worker_database.proto (right):

https://codereview.chromium.org/2771823002/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_database.proto:27: UNKNOWN = 0;
I wonder if we don't have to have UNKNOWN type because we shouldn't write a
registration data to the DB if its cache type is "UNKNOWN".

https://codereview.chromium.org/2771823002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerContainer.cpp
(right):

https://codereview.chromium.org/2771823002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerContainer.cpp:71:
WebServiceWorkerUpdateViaCache ParseUpdateViaCache(const String& value) {
Can you sync this implementation with ParseUpdateViaCache() in
HTMLLinkElement.cpp?

[nhiroki, 2017-08-07 03:55:13]

I'll be OOO until Aug 14. Feel free to land this without my stamp after you get
falken@'s stamp :)

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

https://codereview.chromium.org/2771823002/diff/180001/content/browser/servic...
File content/browser/service_worker/service_worker_context_request_handler.cc
(right):

https://codereview.chromium.org/2771823002/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_context_request_handler.cc:198:
blink::WebServiceWorkerUpdateViaCache::kNone;
On 2017/08/04 09:24:50, nhiroki wrote:
> 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;
> }

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.

https://codereview.chromium.org/2771823002/diff/180001/content/browser/servic...
File content/browser/service_worker/service_worker_database.proto (right):

https://codereview.chromium.org/2771823002/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_database.proto:27: UNKNOWN = 0;
On 2017/08/04 09:24:50, nhiroki wrote:
> I wonder if we don't have to have UNKNOWN type because we shouldn't write a
> registration data to the DB if its cache type is "UNKNOWN".

Done.

https://codereview.chromium.org/2771823002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerContainer.cpp
(right):

https://codereview.chromium.org/2771823002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerContainer.cpp:71:
WebServiceWorkerUpdateViaCache ParseUpdateViaCache(const String& value) {
On 2017/08/04 09:24:50, nhiroki wrote:
> Can you sync this implementation with ParseUpdateViaCache() in
> HTMLLinkElement.cpp?

Done.

[yuryu, 2017-08-17 07:46:22]

The CQ bit was checked by yuryu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-17 07:46:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-17 07:49:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-08-17 07:49:49]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)