Re: [w3c/ServiceWorker] Minor improvements around importScripts. (#1380)

jakearchibald approved this pull request.

Only minor nits. Fix em & merge 😄 

> @@ -2067,12 +2067,11 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe

             * The [=current global object=]'s [=force bypass cache for importscripts flag=] is set.
             * |registration|'s [=last update check time=] is not null and the time difference in seconds calculated by the current time minus |registration|’s [=last update check time=] is greater than 86400.
         1. Let |response| be the result of [=fetch|fetching=] |request|.
-        1. Set |response| to |response|'s [=unsafe response=].
-        1. If |response|’s [=response/cache state=] is not "`local`", set |registration|’s [=service worker registration/last update check time=] to the current time.
-        1. [=Extract a MIME type=] from the |response|'s [=response/header list=]. If this MIME type (ignoring parameters) is not a [=JavaScript MIME type=], return a [=network error=].
-        1. If |response|'s [=response/type=] is not "`error`", and |response|'s [=response/status=] is an <a>ok status</a>, then:
-            1. [=map/Set=] |serviceWorker|'s [=script resource map=][|request|'s [=request/url=]] to |response|.
-            1. Set |serviceWorker|'s [=classic scripts imported flag=].
+        1. Let |unsafeResponse| be |response|'s [=unsafe response=].
+        1. If |unsafeResponse|’s [=response/cache state=] is not "`local`", set |registration|’s [=service worker registration/last update check time=] to the current time.

I think checking the |response| is fine here. The cache state isn't filtered.

> @@ -2067,12 +2067,11 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe

             * The [=current global object=]'s [=force bypass cache for importscripts flag=] is set.
             * |registration|'s [=last update check time=] is not null and the time difference in seconds calculated by the current time minus |registration|’s [=last update check time=] is greater than 86400.
         1. Let |response| be the result of [=fetch|fetching=] |request|.
-        1. Set |response| to |response|'s [=unsafe response=].
-        1. If |response|’s [=response/cache state=] is not "`local`", set |registration|’s [=service worker registration/last update check time=] to the current time.
-        1. [=Extract a MIME type=] from the |response|'s [=response/header list=]. If this MIME type (ignoring parameters) is not a [=JavaScript MIME type=], return a [=network error=].
-        1. If |response|'s [=response/type=] is not "`error`", and |response|'s [=response/status=] is an <a>ok status</a>, then:
-            1. [=map/Set=] |serviceWorker|'s [=script resource map=][|request|'s [=request/url=]] to |response|.
-            1. Set |serviceWorker|'s [=classic scripts imported flag=].
+        1. Let |unsafeResponse| be |response|'s [=unsafe response=].
+        1. If |unsafeResponse|’s [=response/cache state=] is not "`local`", set |registration|’s [=service worker registration/last update check time=] to the current time.
+        1. If |unsafeResponse| is a [=network error import script=], then return a [=network error=].

Nit: for readability, rename "network error import script" to something like "bad import script response".

> @@ -2067,12 +2067,11 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe

             * The [=current global object=]'s [=force bypass cache for importscripts flag=] is set.
             * |registration|'s [=last update check time=] is not null and the time difference in seconds calculated by the current time minus |registration|’s [=last update check time=] is greater than 86400.
         1. Let |response| be the result of [=fetch|fetching=] |request|.
-        1. Set |response| to |response|'s [=unsafe response=].
-        1. If |response|’s [=response/cache state=] is not "`local`", set |registration|’s [=service worker registration/last update check time=] to the current time.
-        1. [=Extract a MIME type=] from the |response|'s [=response/header list=]. If this MIME type (ignoring parameters) is not a [=JavaScript MIME type=], return a [=network error=].
-        1. If |response|'s [=response/type=] is not "`error`", and |response|'s [=response/status=] is an <a>ok status</a>, then:
-            1. [=map/Set=] |serviceWorker|'s [=script resource map=][|request|'s [=request/url=]] to |response|.
-            1. Set |serviceWorker|'s [=classic scripts imported flag=].
+        1. Let |unsafeResponse| be |response|'s [=unsafe response=].
+        1. If |unsafeResponse|’s [=response/cache state=] is not "`local`", set |registration|’s [=service worker registration/last update check time=] to the current time.
+        1. If |unsafeResponse| is a [=network error import script=], then return a [=network error=].

Since this is the only line that needs the unsafe response, you could get rid of the variable here, and have the "bad import script response" steps extract the unsafe response.

> @@ -2199,6 +2198,14 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe

 
   A <dfn id="dfn-scope-to-job-queue-map">scope to job queue map</dfn> is an <a>ordered map</a> where the keys are [=service worker registration/scope urls=], [=URL serializer|serialized=], and the values are [=job queues=].
 
+  A <dfn id="dfn-network-error-import-script">network error import script</dfn> is a [=/response=] for which any of the following conditions are met:
+
+    * |response|'s [=response/type=] is "`error`"
+    * |response|'s [=response/status=] is not an [=ok status=]
+    * The result of [=Extract a MIME type|extracting a MIME type=] from |response|'s [=response/header list=] is not a [=JavaScript MIME type=]
+
+        Note: Keep this definition in sync with [=fetch a classic worker-imported script=].

Ideally we should move this definition to HTML, but I think it's fine for now.

> @@ -2451,22 +2458,25 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe

           1. If |response|'s [=response/cache state=] is not "`local`", set |registration|'s [=last update check time=] to the current time.
           1. If |newestWorker| is null, or |newestWorker|'s [=script resource map=][|request|'s [=request/url=]]'s [=response/body=] is not byte-for-byte identical with |response|'s [=response/body=], set |hasUpdatedResources| to true.
           1. Else if |newestWorker|'s [=classic scripts imported flag=] is set, then:
-              1. [=map/For each=] |url| → |storedResponse| of |newestWorker|'s [=script resource map=]:
-                  1. Let |request| be a new [=/request=] whose [=request/url=] is |url|, [=request/client=] is |job|'s [=job/client=], [=request/destination=] is "`script`", [=request/parser metadata=] is "`not parser-inserted`", [=request/synchronous flag=] is set, and whose [=request/use-URL-credentials flag=] is set.
-                  1. Set |request|'s [=request/cache mode=] to "`no-cache`" if any of the following are true:
+
+              Note: The following checks to see if an imported script has been updated, since the main script has not changed.
+
+              1. [=map/For each=] |importUrl| → |storedResponse| of |newestWorker|'s [=script resource map=]:
+                  1. If |importUrl| is |request|'s [=request/url=], then continue.

good catch

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/w3c/ServiceWorker/pull/1380#pullrequestreview-201577929