Introduce Extend Service Worker Lifetime algorithm #1010

jungkees · 2016-11-22T09:46:56Z

This replaces the steps that monkeypatched the extension of the service
worker's lifetime with an algorithm that is explicitly invoked after the
steps that dispatch the extendable events.

This also changes to use "create an event" algorithm in DOM spec.

This replaces the steps that monkeypatched the extension of the service worker's lifetime with an algorithm that is explicitly invoked after the steps that dispatch the extendable events. This also changes to use "create an event" algorithm in DOM spec.

jungkees · 2016-11-22T09:49:06Z

@annevk, could you review it?

annevk · 2016-11-22T09:55:14Z

docs/index.bs

@@ -168,7 +168,7 @@ spec: rfc7231; urlPrefix: https://tools.ietf.org/html/rfc7231
    <p>A <a href="#dfn-service-worker">service worker</a> has an associated <dfn id="dfn-type">type</dfn> which is either "<code>classic</code>" or "<code>module</code>". Unless stated otherwise, it is "<code>classic</code>".</p>
    <p>A <a href="#dfn-service-worker">service worker</a> has an associated <dfn id="dfn-containing-service-worker-registration">containing service worker registration</dfn> (a <a href="#dfn-service-worker-registration">service worker registration</a>), which contains itself.</p>
    <p>A <a href="#dfn-service-worker">service worker</a> has an associated <dfn id="dfn-service-worker-id">id</dfn> (an opaque string), which uniquely identifies itself during the lifetime of its <a href="#dfn-containing-service-worker-registration">containing service worker registration</a>.</p>
-    <p>A <a href="#dfn-service-worker">service worker</a> is dispatched a set of <dfn id="dfn-lifecycle-events">lifecycle events</dfn>, <a href="#service-worker-global-scope-install-event">install</a> and <a href="#service-worker-global-scope-activate-event">activate</a>, and <dfn id="dfn-functional-events">functional events</dfn> including <a href="#service-worker-global-scope-fetch-event">fetch</a>.</p>
+    <p>A <a href="#dfn-service-worker">service worker</a> is dispatched a set of <dfn id="dfn-lifecycle-events">lifecycle events</dfn>, <a href="#service-worker-global-scope-install-event">install</a> and <a href="#service-worker-global-scope-activate-event">activate</a>, and <dfn id="dfn-functional-events" for="">functional events</dfn> including <a href="#service-worker-global-scope-fetch-event">fetch</a>.</p>


This should not need for="".

You should also try to migrate away from relying on href="#..." and instead use the linking mechanism of Bikeshed that does not require href but uses lt (or just the contents).

Without the for="", it generates no dfn found error. So I fixed this specific dfn in the follow-up patch by removing href="#..." entirely.

migrate away from relying on href="#..."

Yes. I'll do this cleanup soon as a separate work.

annevk · 2016-11-22T10:00:17Z

docs/index.bs

@@ -3173,8 +3158,10 @@ spec: rfc7231; urlPrefix: https://tools.ietf.org/html/rfc7231
      <li>Invoke <a href="#run-service-worker-algorithm">Run Service Worker</a> algorithm with <var>installingWorker</var> as the argument.</li>
      <li><a>Queue a task</a> <var>task</var> to run the following substeps:
        <ol>
-          <li>Create a trusted event <var>e</var> that uses the {{InstallEvent}} interface, with the event type <code><a href="#service-worker-global-scope-install-event">install</a></code>, which does not bubble and is not cancelable.</li>
+          <li><a>Create an event</a> <var>e</var> with the {{InstallEvent}} interface.</li>


I think this phrasing is a bit confusing. "Let e be the result of ..." would be clearer.

annevk · 2016-11-22T10:01:01Z

docs/index.bs

@@ -3173,8 +3158,10 @@ spec: rfc7231; urlPrefix: https://tools.ietf.org/html/rfc7231
      <li>Invoke <a href="#run-service-worker-algorithm">Run Service Worker</a> algorithm with <var>installingWorker</var> as the argument.</li>
      <li><a>Queue a task</a> <var>task</var> to run the following substeps:
        <ol>
-          <li>Create a trusted event <var>e</var> that uses the {{InstallEvent}} interface, with the event type <code><a href="#service-worker-global-scope-install-event">install</a></code>, which does not bubble and is not cancelable.</li>
+          <li><a>Create an event</a> <var>e</var> with the {{InstallEvent}} interface.</li>
+          <li>Initialize <var>e</var>’s {{Event/type}} attribute to "<code>install</code>".</li>
          <li><a>Dispatch</a> <var>e</var> at <var>installingWorker</var>'s <a>environment settings object</a>'s <a for="environment settings object">global object</a> <var>globalObject</var>.</li>


Shouldn't this be "relevant global object" rather than "environment settings object's global object"?

installingWorker is not a platform object but a service worker. So it seems we can't get a "relevant global object" from it?

I guess not. Does a service worker have an environment settings object as slot? This should probably link to it.

Ah.. It doesn't. It seems better to define a slot for the global object rather than the environment settings object. I don't have any other use of service worker's environment settings object apart from getting the global object. Also what'd be a good name for it? The service worker's ___ global object. Incumbent? But I think just "global object" would also be fine as a service worker won't have multiple global objects at the same time. Any suggestion?

Yeah, just do <dfn for="service worker">global object</dfn> and set it somewhere when creating the service worker.

Great. Was working on exactly that way.

annevk · 2016-11-22T10:02:19Z

docs/index.bs

+      <li>Let <var>extendLifetimePromises</var> be an empty array.</li>
+      <li>Run the following substeps <a>in parallel</a>:
+        <ol>
+          <li><em>SetupPromiseArray</em>: Set <var>extendLifetimePromises</var> to a copy of <var>event</var>'s <a>extend lifetime promises</a>.</li>


I thought we had a better way of doing this? Just letting the promises decrease a counter.

Right. That's what we discussed during the TPAC. My plan is to do #931 (comment) as a separate work. (This particular PR is for #931 (comment)).

Okay. I guess moving it and then changing the approach is fine.

annevk · 2016-11-22T10:03:20Z

docs/index.bs

-          <li>Let the {{FetchEvent/request}} attribute of <var>e</var> be initialized to <var>r</var>.</li>
+          <li><a>Create an event</a> <var>e</var> with the {{FetchEvent}} interface.</li>
+          <li>Initialize <var>e</var>’s {{Event/type}} attribute to "<code>fetch</code>".</li>
+          <li>Initialize <var>e</var>’s {{Event/cancelable}} attribute to true.</li>


Why do you make it cancelable? That seems like a change.

You're not handling the return value of the dispatch algorithm though, so how does it matter?

Note that fetch and foreignfetch are cancelable. When they're canceled without calling respondWith(), it returns network error. See #51 (comment).

FYI. install and activate are not cancelable as you pointed.

It just seems that per the original text they were not cancelable. Shouldn't that change be separate or pointed out somehow?

Sorry, I had to be clearer. They were cancelable in the original text too. (I presume the previous text in DOM wanted me to write it'd be cancelable by default if not explicitly called it's not cancelable? Ignore me if it was not.)

Handle Fetch step 21.1. (https://w3c.github.io/ServiceWorker/#on-fetch-request-algorithm) checks the value set by event's canceled flag.

DOM always defaulted to not cancelable.

OK. The previous text was wrong. It didn't specify it's cancelable.

annevk · 2016-11-22T10:04:04Z

docs/index.bs

-          <li>Let the {{ForeignFetchEvent/origin}} attribute of <var>e</var> be initialized to the  <a lt="Unicode serialization of an origin">Unicode serialization</a> of <var>request</var>'s <a for=request>origin</a>.</li>
+          <li><a>Create an event</a> <var>e</var> with the {{ForeignFetchEvent}} interface.</li>
+          <li>Initialize <var>e</var>’s {{Event/type}} attribute to "<code>foreignfetch</code>".</li>
+          <li>Initialize <var>e</var>’s {{Event/cancelable}} attribute to true.</li>


Explained above.

annevk · 2016-11-22T10:04:39Z

docs/index.bs

@@ -3582,8 +3602,9 @@ spec: rfc7231; urlPrefix: https://tools.ietf.org/html/rfc7231
    <ol>
      <li><a>Assert</a>: a <a>Record</a> with the \[[value]] equals to <var>registration</var> is contained in <a href="#dfn-scope-to-registration-map">scope to registration map</a>.</li>
      <li><a>Assert</a>: <var>registration</var>'s <a href="#dfn-active-worker">active worker</a> is not null.</li>
+      <li>Let <var>event</var> be the <a for="/">functional event</a> for which this algorithm was invoked.</li>


Shouldn't this be passed in as variable?

While I agree with you, event here is apparent here, and it needs changes to caller if we'd want to pass the event as an argument. Would it be better?

It would be clearer I think. In code you would also have to pass it.

Added an argument, event. Please update the notifications API accordingly when it's landed.

annevk · 2016-11-22T10:47:05Z

docs/index.bs

@@ -168,7 +168,7 @@ spec: rfc7231; urlPrefix: https://tools.ietf.org/html/rfc7231
    <p>A <a href="#dfn-service-worker">service worker</a> has an associated <dfn id="dfn-type">type</dfn> which is either "<code>classic</code>" or "<code>module</code>". Unless stated otherwise, it is "<code>classic</code>".</p>
    <p>A <a href="#dfn-service-worker">service worker</a> has an associated <dfn id="dfn-containing-service-worker-registration">containing service worker registration</dfn> (a <a href="#dfn-service-worker-registration">service worker registration</a>), which contains itself.</p>
    <p>A <a href="#dfn-service-worker">service worker</a> has an associated <dfn id="dfn-service-worker-id">id</dfn> (an opaque string), which uniquely identifies itself during the lifetime of its <a href="#dfn-containing-service-worker-registration">containing service worker registration</a>.</p>
-    <p>A <a href="#dfn-service-worker">service worker</a> is dispatched a set of <dfn id="dfn-lifecycle-events">lifecycle events</dfn>, <a href="#service-worker-global-scope-install-event">install</a> and <a href="#service-worker-global-scope-activate-event">activate</a>, and <dfn id="dfn-functional-events" for="">functional events</dfn> including <a href="#service-worker-global-scope-fetch-event">fetch</a>.</p>
+    <p>A <a href="#dfn-service-worker">service worker</a> is dispatched a set of <dfn id="dfn-lifecycle-events">lifecycle events</dfn>, <a href="#service-worker-global-scope-install-event">install</a> and <a href="#service-worker-global-scope-activate-event">activate</a>, and <dfn export>functional events</dfn> including <a href="#service-worker-global-scope-fetch-event">fetch</a>.</p>


You should keep the ID, no? Otherwise you change the ID to "functional-events".

Yes yes. Indeed, I was worrying about the refs being changed by the bikeshed cleanups. Now I understand how you'd not break the x-refs from the existing contents. Just missed this simple thing..

Will get it back in the follow-up patch on your additional comments.

…s global object

jungkees · 2016-11-23T10:23:57Z

@annevk, do you think it's good to merge now?

annevk · 2016-11-23T11:41:38Z

I think so.

jungkees assigned annevk Nov 22, 2016

annevk requested changes Nov 22, 2016

View reviewed changes

Address comments; more discussion needed

3429a86

annevk reviewed Nov 22, 2016

View reviewed changes

Add event argument to Handle Functional Event; define service worker'…

9f040ec

…s global object

jungkees merged commit c2a518b into master Nov 23, 2016

jungkees deleted the extendableevent-fix branch November 23, 2016 13:42

Introduce Extend Service Worker Lifetime algorithm #1010

Introduce Extend Service Worker Lifetime algorithm #1010

Conversation

jungkees commented Nov 22, 2016

jungkees commented Nov 22, 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

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

jungkees commented Nov 23, 2016

annevk commented Nov 23, 2016