Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fetch service worker scripts with "no-cache" by default #1020

Merged
merged 4 commits into from
Dec 12, 2016
Merged

Conversation

jungkees
Copy link
Collaborator

@jungkees jungkees commented Dec 6, 2016

This introduces service worker registration's use cache field and its
related APIs: options.useCache to register method and
registration.useCache for ServiceWorkerRegistration objects. This
changes the default cache mode of fetching SW scripts to "no-cache".
After the change, 24 hours limit and job's force bypass cache flag rules
are still enforced. register() method's options value set to
{ useCache: true } re-enables the previous default behavior provided
before this change.

Fixes #893 #894.

This introduces service worker registration's use cache field and its
related APIs: options.useCache to register method and
registration.useCache for ServiceWorkerRegistration objects. This
changes the default cache mode of fetching SW scripts to "no-cache".
After the change, 24 hours limit and job's force bypass cache flag rules
are still enforced. register() method's options value set to
{ useCache: true } re-enables the previous default behavior provided
before this change.

Fixes #893.
@jungkees
Copy link
Collaborator Author

jungkees commented Dec 6, 2016

/cc @wanderview

Copy link
Contributor

@jakearchibald jakearchibald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look good, but it needs to request imported scripts no-cache too.

@@ -187,8 +187,6 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe

A [=/service worker registration=] has an associated <dfn export id="dfn-scope-url">scope url</dfn> (a [=/URL=]).

A [=/service worker registration=] has an associated <dfn export id="dfn-registration-script-url">registering script url</dfn> (a [=/URL=]).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh, good catch!

@@ -1636,7 +1644,9 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
1. If the "<code>scope</code>" <a>target attribute</a> of the <code>Link</code> header is present, set |scopeURL| to the result of <a lt="URL parser">parsing</a> the "<code>scope</code>" <a>target attribute</a> with |scriptURL|.
1. Let |workerType| be the "<code>workertype</code>" <a>target attribute</a> of the <code>Link</code> header, or "<code>classic</code>" if no such attribute is present.
1. If |workerType| is not a valid {{WorkerType}} value, abort these steps.
1. Invoke [=Start Register=] with |scopeURL|, |scriptURL|, a new <a>promise</a>, null, |contextURL| and |workerType|.
1. Let |useCache| be the "<code>usecache</code>" <a>target attribute</a> of the <code>Link</code> header, or false if no such attribute is present.
1. If |useCache| is not a valid boolean value, abort these steps.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be an invalid value?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any other value except true or false. I copied this from other target attribute's value sanitization steps above. @mkruisselbrink, could you check if this step is needed and done in a right way?

@jungkees
Copy link
Collaborator Author

jungkees commented Dec 8, 2016

it needs to request imported scripts no-cache too.

Addressed. Note that job's force bypass cache flag is effective for the script executions in both Update and Install for now. Depending on #1021, there's a chance that the script execution in Install won't need this bypass cache flag.

@jakearchibald
Copy link
Contributor

I don't think updates are doing the right thing.

"Soft update" is never called with force bypass cache flag. "Update" 9.6 will only set force bypass cache for importscripts flag if force bypass cache flag is set, so it ignores use cache on the registration.

@jakearchibald
Copy link
Contributor

Same goes for "Install" - it doesn't pay attention to use cache on the registration when it comes to importScripts.

@@ -2500,7 +2527,7 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe

Else, continue the rest of these steps after the algorithm's asynchronous completion, with |script| being the asynchronous completion value.

1. If |newestWorker| is not null, |newestWorker|'s [=service worker/script url=] [=url/equals=] |job|'s [=job/script url=] with the *exclude fragments flag* set, and |script| is a byte-for-byte match with |newestWorker|'s <a>script resource</a>, then:
1. If |newestWorker| is not null, |newestWorker|'s [=service worker/script url=] [=url/equals=] |job|'s [=job/script url=] with the *exclude fragments flag* set, and |script|'s [=source text=] is a byte-for-byte match with |newestWorker|'s [=script resource=]'s [=source text=], if |script| is a [=classic script=], and |script|'s [=module record=]'s \[[ECMAScriptCode]] is a byte-for-byte match with |newestWorker|'s [=script resource=]'s [=module record=]'s \[[ECMAScriptCode]] otherwise, then:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bikeshed doesn't seem to pick up "module record"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed: whatwg/html#2157. And put module script specifier for module records: d2047e1.

@jungkees
Copy link
Collaborator Author

jungkees commented Dec 9, 2016

@jakearchibald, force bypass cache flag is for dev tools, debugging purposes, potential calls from other specs, etc. (See #753.) The registration's use cache field is being checked in Update and importScripts() as one of the conditions for "no-cache". In this change, I made the registration's use cache be set in Set Registration algorithm given job's use cache that's passed from Register requests.

@jakearchibald
Copy link
Contributor

Ah, I think I was looking at a commit before Apply cache mode "no-cache" to importScripts() requests. LGTM!

@jungkees jungkees merged commit 7deb238 into master Dec 12, 2016
@jungkees jungkees deleted the usecache branch December 12, 2016 07:06
alice pushed a commit to alice/html that referenced this pull request Jan 8, 2019
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants