Add support for 'serviceworkers' member #507

kenchris · 2016-10-20T14:36:28Z

First attempt at fixing #161

Where do we add the actual registration steps?

RobDolinMS · 2016-10-21T16:55:55Z

index.html

+        </p>
+        <div class="example">
+<pre class="">
+"serviceworkers": [{


Did you intend for this to be "serviceworker": { or "serviceworkers": [{ ?

plural as you can have multiple with different scopes. I am writing serviceworkers in one word to follow the naming used with the Link element "serviceworker" (as you can have multiple link elements)

I'm of the opinion also that there should only be 1 service worker in the manifest. Other service workers should be declared in the document(s) of the app.

Your own example used plural and an array :-)

RobDolinMS · 2016-10-21T17:00:29Z

Thanks for stepping-up here @kenchris.

I was thinking the serviceworkers object should also be added to the example near the top of the document.

RobDolinMS · 2016-10-24T15:56:34Z

index.html

+  "src": "sw.js",
+  "scope": "/foo",
+  "options": {}
+}


@kenchris - Should line 1971 be }] to match line 1967?

benfredwells · 2016-10-24T22:27:50Z

What is the purpose of adding this information? Is it intended that having a SW defined in the manifest would cause the SW to be registered, similar to calling navigator.serviceWorker.register in script?

If so it would be good to get this reviewed by some people familiar with SW registration.

kenchris · 2016-10-25T12:42:09Z

@benfredwells it is just like calling register manually or adding a service worker link tag. Jake already approved this approach.

kenchris · 2016-10-25T12:45:50Z

Can we make a decision on whether we want just one or multiple service workers in the manifest? @marcoscaceres @jakearchibald

I don't care strongly, but I would like to know so that I can change the WIP patch

marcoscaceres · 2016-10-25T13:10:04Z

Only 1 please.

kenchris · 2016-10-25T13:27:22Z

@jakearchibald what is your take?

RobDolinMS · 2016-10-25T19:53:05Z

@kenchris - After discussion with the Edge PM for Service Workers, Ali Alabas (@aliams) and others at MSFT/Edge, we believe a single serviceworker object listed in the web app manifest will address our use cases; but don’t object to having an array of serviceworkers. /cc @marcoscaceres @jakearchibald

benfredwells · 2016-10-25T22:41:08Z

@kenchris thanks for the explanation.

SW registration, updating (e.g. if you change the value in the manifest) and removal is complicated and has already had lots of thought put into it. Are you planning on including algorithms for declarative SW registrations, updates and removals in this spec? Maybe it would be better to just have the syntax for the declarations here, and get the SW spec updated to cover the notion of declarative registration in general, and point to that.

RobDolinMS · 2016-11-02T18:54:52Z

As far as the question about one ServiceWorker{ } or multiple ServiceWorkers[ ], what if we went with one to start and then looked for feedback / push-back from potential implementers?

/cc @marcoscaceres @kenchris @benfredwells @jakearchibald

kenchris · 2016-11-02T19:20:42Z

I talked to Jake, we will discuss it after the Chrome Dev Summit as he is currently quite busy. I will work on this after than then.

marcoscaceres · 2016-11-03T03:27:01Z

@kenchris, thanks for the update. Looking forward to @jakearchibald's input here. My position remains we should only have one (does enough work to make sure the application can be launched).

RobDolinMS · 2016-12-01T01:32:33Z

@kenchris Have you been able to sync with @jakearchibald about one ServiceWorker{ } or multiple ServiceWorkers[ ] ?

Summarizing positions:

@kenchris - prefers ServiceWorkers[ ] but "I don't care strongly"
@marcoscaceres - prefers ServiceWorker{ } - "only 1 please"
@aliams and @RobDolinMS - OK with either
@jakearchibald - TBD

kenchris · 2016-12-01T08:58:02Z

No, didn't get to talk unfortunately.

RobDolinMS · 2016-12-05T04:40:48Z

(I've sent an email ping to @jakearchibald with @marcoscaceres and @kenchris on CC)

jakearchibald · 2016-12-05T16:07:45Z

I'm happy to go with one for simplicity here. Additional SWs can be in the document as @marcoscaceres says, or perhaps even Link headers on the manifest response.

Apologies for the slow reply!

RobDolinMS · 2016-12-06T01:25:41Z

@marcoscaceres or @kenchris - Since @jakearchibald has given a thumbs-up to one serviceworker{ } would one of you want to modify the PR?

kenchris · 2016-12-06T08:53:22Z

Yes, I can work on this

kenchris · 2016-12-06T11:01:11Z

Now I have IPR issues as well, though I am in the Web Platforms WG as indicated by the Repository Manager

chaals · 2016-12-06T11:32:35Z

@siusin ping… re @kenchris access…

siusin · 2016-12-06T16:09:28Z

@kenchris -
Intel hasn't re-joined the group yet after we rechartered, so from an IPR perspective, the PRs would be flagged as non valid until the contributors sign up for the group again.

@chaals -
Of course, chairs can make a decision and get the PR merged in this case.

chaals · 2016-12-06T16:12:54Z

@siusin @kenchris I am happy that unless Intel fails to rejoin by December 15, they are currently treated as a member and have the effective commitment, so this should be marked as OK. But it seems something went funny with my access to ash-nazg to make it happen. Please go ahead and merge

siusin · 2016-12-06T16:19:18Z

Marked as non-substantive for IPR from ash-nazg.

RobDolinMS · 2016-12-07T00:35:53Z

Only 1 please.
'Looks like @kenchris has fixed the PR so there is only one serviceworker{ } and @siusin has gotten the IRP thumbs-up.

@marcoscaceres - Would you want to merge?

marcoscaceres · 2016-12-07T00:49:05Z

@RobDolinMS not yet :) This is far from done.

kenchris · 2016-12-08T09:16:08Z

Looking forward to comments from @marcoscaceres :-)

marcoscaceres · 2016-12-09T02:01:46Z

index.html

+        <p>
+          The <a><code>serviceworker</code> member</a> represents a service worker
+          registration. Other service worker registrations can be done, for instance
+          by a script; if these have different scopes they will be considered separate


We should probably not re-specify the rules that are in the service worker spec; we should reference them.

marcoscaceres · 2016-12-09T02:02:28Z

index.html

+        </p>
+        <p>
+          The <a><code>serviceworker</code> member</a> represents a service worker
+          registration. Other service worker registrations can be done, for instance


Link to registration definition in SW

marcoscaceres · 2016-12-09T02:03:49Z

index.html

+          with <var>serviceworker</var> and <var>manifest URL</var>.
+          </li>
+          <li>If <var>src</var> is <code>undefined</code>, or if the result of
+          running <a href="https://w3c.github.io/webappsec/specs/powerfulfeatures/#is-origin-trustworthy">Is origin potentially trustworthy</a>


Nit: throw in this URL in the dependencies section at the bottom of the document.

Won't the service worker registration attempt do this for us?

done, will need to check

marcoscaceres · 2016-12-09T02:06:01Z

index.html

+        <ol>
+          <li>Let <var>src</var> be the result of running the <a>steps
+          for processing the <code>src</code> member of a service worker</a>
+          with <var>serviceworker</var> and <var>manifest URL</var>.


Below, you are using let service worker be thus trashing this one? I don't think we need to send in a service worker here. Just process the members first, then "attempt to register the service worker"

Sorry, this comment is in the wrong place!

marcoscaceres · 2016-12-09T02:07:32Z

index.html

+        <p>
+          The <dfn>steps for processing the <code>serviceworker</code>
+          member</dfn> are given by the following algorithm. The algorithm
+          takes a <a>manifest</a> <var>manifest</var> and <a>serviceworker object</a>


Below, you are using let service worker be... thus trashing this one? I don't think we need to send in a service worker here. Just process the members first, then "attempt to register the service worker"

marcoscaceres · 2016-12-09T02:10:31Z

index.html

+          <li>If <var>src</var> is <code>undefined</code>, or if the result of
+          running <a href="https://w3c.github.io/webappsec/specs/powerfulfeatures/#is-origin-trustworthy">Is origin potentially trustworthy</a>
+          with the origin of <var>src</var> is <code>Not Trusted</code>, return
+          <code>undefined</code>.


screwing up the security aspects of the registration is a pretty big error... so we should maybe pass this through and just allow service worker registration to fail (and kill the whole object then)

marcoscaceres · 2016-12-09T02:11:48Z

index.html

+          with the origin of <var>src</var> is <code>Not Trusted</code>, return
+          <code>undefined</code>.
+          </li>
+          <li>Otherwise, let <var>serviceworker</var> be an object with


nit: let <var>serviceworker</var> might confuse people into thinking this the actual service worker... maybe "Let registration properties be..."

ok, did something similar

marcoscaceres · 2016-12-09T02:13:14Z

index.html

+        <div class="example">
+          In the following example, the web application is listing
+          a service worker for the <code>/foo</code> scope:
+<pre class="">


nit: class=example?

ok, needs to be applied elsewhere in the spec

kenchris · 2016-12-12T14:09:59Z

Yes, any help on that is appreciated :-) I will see if I can get junkee to have a look and maybe help me.

jungkees · 2016-12-13T06:39:32Z

@kenchris, Start Register is a gate function that you can invoke to trigger a Register. Invoking Start Register makes a job and schedules it in the job queue defined in SW. Start Register is also supposed to take care of the security check and parsing the URLs for the given src and scope, etc. Basically, a register request is processed async given a promise (to Start Register).

I'm not quite sure, but you might want to invoke Start Register from steps for processing the serviceworker member I guess. Start Register returns nothing but the given promise to it resolves with a ~~service worker registration~~ ServiceWorkerRegistration object on success and rejects on failure. (See the example call site: In https://w3c.github.io/ServiceWorker/#link-element-processing, step 12 of the algorithm starting with "When a serviceworker link’s link element ..".)

This is basically what SW expects for a register request. I think we should look together if any changes are needed while you're working on this PR.

jungkees · 2016-12-13T06:44:15Z

/cc @mkruisselbrink, @jakearchibald, @slightlyoff

jungkees · 2016-12-13T13:48:59Z

index.html

+              <ul>
+                <li>Invoke <a>Start Register</a> with <var>scope</var> and <var>src</var>
+                members of the <var>service worker registration</var>, a new <var>promise</var>,
+                null, <a>manifest URL</a>, <code>"classic"</code> and <code>False</code>,


SW's RegistrationOptions dictionary has been extended: https://w3c.github.io/ServiceWorker/#dictdef-registrationoptions. I think both "classic" and "module" worker types should be covered and also useCache should be supported to be aligned with other registration paths. (useCache indicates whether the UA should set the cache mode to "no-cache" or "default" when fetching a service worker script from networks.)

jungkees · 2016-12-13T14:07:43Z

index.html

+              <ul>
+                <li>Invoke <a>Start Register</a> with <var>scope</var> and <var>src</var>
+                members of the <var>service worker registration</var>, a new <var>promise</var>,
+                null, <a>manifest URL</a>, <code>"classic"</code> and <code>False</code>,


For the client param to Start Register, I think the browsing context's Document's relevant settings object should be passed instead of null. The given client is used to queue a task later to manipulate promise settlement and parse a fetched script resources later.

OK, will look at that tomorrow then

jungkees

Service workers expect default values from scope, type, and use_cache members. Please see the comments.

jungkees · 2016-12-14T05:12:52Z

index.html

+          property to be <var>type</var>.
+          </li>
+          <li>If <var>type</var> is <code>undefined</code> abort these steps and
+          return <code>undefined</code>.


Service workers expect a default value for type member. If type is undefined here, please set the default value to "classic" when passing it to Start Register.

That is set in the sub algorithm. Undefined here means that the sub algorithm didn't get a valid value

Understood. I saw you return "classic" when no value is given. It looks good.

jungkees · 2016-12-14T05:17:45Z

index.html

+          property to be <var>use cache</var>.
+          </li>
+          <li>If <var>use cache</var> is <code>undefined</code> abort these steps
+          and return <code>undefined</code>.


Likewise, If use_cache is undefined here, please set the default value to false when passing it to Start Register.

Note that there's an ongoing discussion about the expected default behavior in consider fetching service worker scripts with no-cache by default ServiceWorker#893. So, there's a chance SW may require true as default depending on the discussion there.

same, ok Please keep me up to date :)

Same as above. I saw you return false when no value is given. It looks good.

jungkees · 2016-12-14T05:23:39Z

index.html

          </li>
          <li>Set <var>registration</var>'s <code>src</code> property to be
          <var>src</var>.
          </li>
-          <li>Let <var>type</var> be the result of running the <a>steps
+          <li>Let <var>scope</var> be the result of running the <a>steps


Similar to type and use_cache, SW expects a default value for scope too. If scope is undefined, please give null to Start Register as a default value. That makes Start Register set the scope to the same location as the given script url.

When the value is undefined here, it means that there was a parsing error (check sub algorithms), and I thus don't attempt to register at all.

I understood the intention. Please check if it actually doesn't attempt to register in that case (i.e. when the sub algorithm returns undefined.) I left a comment here: #507 (comment).

jungkees · 2016-12-14T14:54:37Z

index.html

+          <var>src</var>.
+          </li>
+          <li>Let <var>scope</var> be the result of running the <a>steps
+          for processing the <code>scope</code> member of a service worker</a>


When this algorithm returns undefined, the steps to install the web application gets to pass nothing to Start Register. In this case, shouldn't there be an error handling such that Start Register is not invoked?

Let me check that, it should not be invoked no

<li>If <a>obtaining the manifest</a> succeeds, and the <var>service worker registration</var> of <a>manifest</a> is not <code>undefined</code>, a user agent can at this point...

It is not invoked if it fails

That looks good.

jungkees · 2016-12-14T15:06:31Z

index.html

+              <li>Return <code>undefined</code>.
+              </li>
+            </ol>
+          <li>Return <var>type</var>.


Shouldn't the return value be value.[[Value]] instead?

We have
Let type be Type(value). above. That is how @marcoscaceres did it elsewhere. I think that should be fine. Why the value.[[Value]]? looks like a private slot

Ah.. I got it. It has to do with the prefix ! for an ECMAScript abstract operation. So, the value here is the actual value for this member already. But then it still needs to return value instead of type. Referring to other cases, it should return Trim(value) here I suppose.

right, fixed

jungkees · 2016-12-14T15:06:50Z

index.html

+              <li>Otherwise, return False</li>
+            </ol>
+          </li>
+          <li>Return <var>use_cache</var>.


Shouldn't the return value be value.[[Value]] instead?

It seems it should return value here. See "steps for processing the prefer_related_applications member" as an example.

jungkees · 2016-12-15T15:01:35Z

@kenchris, when the above comments are addressed, I think the shape of the hooks between Web App Manifest and Service Workers would be OK. For the details of the spec language here, I defer to you and @marcoscaceres. I'll continue to help, of course.

marcoscaceres

Maybe I missed it, but did you also add processing the manifest member to: https://www.w3.org/TR/appmanifest/#processing

marcoscaceres · 2016-12-28T06:45:23Z

index.html

+                members of the <var>registration</var>,
+                </li>
+              </ol>
+              in which case the state of the settled <var>promise</var> determine whether the


nit: determines

marcoscaceres · 2016-12-28T06:47:00Z

index.html

+                <code>Document</code>'s
+                <a href="https://html.spec.whatwg.org/multipage/webappapis.html#relevant-settings-object">
+                relevant settings object</a>, or <code>null</code> if unavailable.
+                <li>Invoke <a>Start Register</a> with <var>scope</var> and <var>src</var>


q: should this be "Let registration be the result of Start Registration".

The way this is spec'ed is a bit strange - I was expecting some kind of flow with the resulting promise... but it all seems to magically result in a registration.

I am not sure how to best solve this. Maybe you could solve in a follow up?

marcoscaceres · 2016-12-28T06:49:00Z

index.html

+        </h3>
+        <p>
+          The <dfn><code>serviceworker</code> member</dfn> describes a
+          service worker as defined in [[!SERVICE-WORKERS-1]].


Nit: Is there some unversioned reference we can use here instead of V1?

Not as far as I could see. @jungkees ?

marcoscaceres · 2016-12-28T06:49:32Z

index.html

+        <p>
+          The <a><code>serviceworker</code> member</a> represents a <a
+          href="https://w3c.github.io/ServiceWorker/#service-worker-registration-concept">
+          service worker registration</a>. Other service worker registrations can be done, for instance


The text "Other service worker registrations can be done" should be in a non-normative note.

Also, you are redefining the rules - which is a bit dangerous. That should point to the SW spec, like:

"For rules on how SWs interact, then see section X of Service Workers"

Made it a note now. @jungkees can you point me to the best place to refer to?

marcoscaceres · 2016-12-28T06:51:20Z

index.html

+          The <dfn>steps for processing the <code>serviceworker</code>
+          member</dfn> are given by the following algorithm. The algorithm
+          takes a <a>manifest</a> <var>manifest</var>. This algorithm returns a
+          registration object <var>registration</var>, which can be <code>undefined</code>.


nit: link to registration object?

marcoscaceres · 2016-12-28T06:59:51Z

index.html

+        <p>
+          The <dfn data-lt="serviceworker-scope"><code>scope</code> member</dfn> of a
+          <a>serviceworker object</a> is the service worker's associated
+          <a href="https://w3c.github.io/ServiceWorker/#dfn-scope-url">scope URL</a>.


nit, scope URL should be defined at the end of the document, so not to include the link to the SW spec here.

marcoscaceres · 2016-12-28T07:01:39Z

index.html

+          service worker</dfn> are given by the following algorithm. The algorithm takes
+          a <a>serviceworker object</a> <var>registration</var>, and a <a>URL</a> <var>manifest
+          URL</var>, which is the <a>URL</a> from which the
+          <var>manifest</var> was fetched. This algorithm will return a


Hmm... what if the manifest redirected during fetch... we might need to clarify that somewhere.

Can you help with that? :-)

marcoscaceres · 2016-12-28T07:03:30Z

index.html

+          </li>
+          <li>If <a>Trim</a>(value) is the empty string, then:
+            <ol>
+              <li>Issue a developer warning that the type is unsupported.


This seems odd... you are returning undefined here, but "classic" above?

I.e., should we recover to "classic" in error cases for backwards compat?

That is how it works for Link element. What do you think @jungkees ?

marcoscaceres · 2016-12-28T07:05:23Z

index.html

+              <li>Return <code>undefined</code>.
+              </li>
+            </ol>
+          <li>Otherwise, <a>Trim</a>(<var>value</var>) and return the result.


Also, should should this be restricted to just "classic" and "module"?

Yes, "classic", "module" or undefined. Everything else should fail the whole registration as it does with link element

marcoscaceres · 2016-12-28T07:06:00Z

index.html

+              developer warning that the type is unsupported and
+              return <code>undefined</code>.
+              </li>
+              <li>Otherwise, return False</li>


nit: return <code>false</code>

kenchris · 2016-12-28T11:58:49Z

Maybe I missed it, but did you also add processing the manifest member to: https://www.w3.org/TR/appmanifest/#processing

Yes, I did

kenchris · 2017-01-17T10:04:57Z

So I can go ahead and merge?

marcoscaceres · 2017-01-17T11:13:31Z

Yeah, why not :) ship it

…

On 17 Jan 2017, at 9:04 pm, Kenneth Rohde Christiansen ***@***.***> wrote: So I can go ahead and merge? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

GrosSacASac · 2017-12-29T14:51:09Z

If I understand correctly, the service worker declaration inside the manifest has no consequence unless the web app is installed without directly visting the web app url.(with an web app store ?)

I ask because I have an app where the service worker is only installed after the users checks the checkbox USE OFFLINE. I don't want the manifest to install the service worker on normal page load without user consent first.

I came here because https://www.w3.org/TR/appmanifest/#serviceworker-member words are confusing.

marcoscaceres · 2018-01-23T05:14:02Z

@GrosSacASac, moved your issue here: #640 so we don't lose track of it.

RobDolinMS reviewed Oct 21, 2016

View reviewed changes

RobDolinMS reviewed Oct 24, 2016

View reviewed changes

Add support for 'serviceworker' member

9e5567a

kenchris force-pushed the sw-member branch from f9d9715 to 9e5567a Compare December 6, 2016 10:59

marcoscaceres reviewed Dec 9, 2016

View reviewed changes

marcoscaceres requested changes Dec 9, 2016

View reviewed changes

Review fixes after Marcos' comments

4a5f762

Add Service Worker registration, with some help from Jungkee

e97ab3e

jungkees reviewed Dec 13, 2016

View reviewed changes

Add support for 'type' and 'use_cache' when registering a service worker

2524071

jungkees suggested changes Dec 14, 2016

View reviewed changes

A few changes based on feedback by Jungkee

3df8f7a

jungkees reviewed Dec 14, 2016

View reviewed changes

Clean up spec text a bit

5d37374

kenchris force-pushed the sw-member branch from 31f798c to 5d37374 Compare December 15, 2016 09:13

Add client to Start Register

5c867ad

Fix two return values pointed out by Jungkee

4fc48b1

marcoscaceres requested changes Dec 28, 2016

View reviewed changes

Fix comments from Marcos

77bdd10

marcoscaceres approved these changes Jan 17, 2017

View reviewed changes

kenchris merged commit 1b26460 into w3c:gh-pages Jan 17, 2017

jakearchibald mentioned this pull request Jan 17, 2017

Payment app identifier to manifest filename mapping w3c/payment-handler#48

Closed

anssiko mentioned this pull request Jan 24, 2017

Integration with service workers #161

Closed

This pull request was closed.

Add support for 'serviceworkers' member #507

Add support for 'serviceworkers' member #507

Conversation

kenchris commented Oct 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RobDolinMS commented Oct 21, 2016

RobDolinMS Oct 24, 2016 • edited Loading

Choose a reason for hiding this comment

benfredwells commented Oct 24, 2016

kenchris commented Oct 25, 2016

kenchris commented Oct 25, 2016

marcoscaceres commented Oct 25, 2016

kenchris commented Oct 25, 2016

RobDolinMS commented Oct 25, 2016

benfredwells commented Oct 25, 2016

RobDolinMS commented Nov 2, 2016

kenchris commented Nov 2, 2016

marcoscaceres commented Nov 3, 2016

RobDolinMS commented Dec 1, 2016

kenchris commented Dec 1, 2016

RobDolinMS commented Dec 5, 2016

jakearchibald commented Dec 5, 2016

RobDolinMS commented Dec 6, 2016

kenchris commented Dec 6, 2016

kenchris commented Dec 6, 2016

chaals commented Dec 6, 2016

siusin commented Dec 6, 2016

chaals commented Dec 6, 2016

siusin commented Dec 6, 2016

RobDolinMS commented Dec 7, 2016

marcoscaceres commented Dec 7, 2016

kenchris commented Dec 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenchris commented Dec 12, 2016

jungkees commented Dec 13, 2016 • edited Loading

jungkees commented Dec 13, 2016

Choose a reason for hiding this comment

jungkees Dec 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jungkees left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RobDolinMS Oct 24, 2016 •

edited

Loading

jungkees commented Dec 13, 2016 •

edited

Loading

jungkees Dec 13, 2016 •

edited

Loading