Add `ServiceWorkerRegistration.id` and supporting algorithm changes. (Fixes #1512) #1526

wanderview · 2020-07-31T21:33:24Z

This commit adds support for a registration id value. This will be used to uniquely identify registrations
instead of scope. This will make it easier for a developer to migrate a registration from one scope to
another.

The commit consists of the following changes:

Add a DOMString id and associated getter to the registration.
Add an origin to the internal registration type.
Move the scope internal representation to ServiceWorker and add a new getter.
Make the ServiceWorkerRegistration.scope return the oldest associated worker's scope.
Migrate the registration map to be keyed by the tuple (origin,id).
Migrate the job queue map to be keyed by the tuple (origin,id).
Adjust job equality checks to treat register jobs to account for new id and scope semantics.
Support changing the scope during register operations.
Reject register operations that change the scope to a value that is already in use by another registration.
Properly un-control clients whose URL no longer matches a new active worker's scope.

Preview | Diff

sideshowbarker · 2020-07-31T22:53:11Z

Hi Ben. There’s some W3C tooling that doesn’t yet recognize you on GitHub, but you can fix it by going to to https://www.w3.org/users/myprofile to log into your W3C account — and from there, follow the Connected Accounts link in the sidebar to connect your W3C account with your GitHub account.

ylafon · 2020-08-02T16:42:19Z

Marked as non substantive for IPR from ash-nazg.

wanderview · 2020-08-03T14:42:39Z

Thanks, I've connected my account.

jakearchibald

Mostly prose nits. The only real concern is the backwards compatibility break with reg.scope.

docs/index.bs

jakearchibald · 2020-08-21T10:04:40Z

docs/index.bs

@@ -208,9 +210,11 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
  <section dfn-for="service worker registration">
    <h3 id="service-worker-registration-concept">Service Worker Registration</h3>

-    A <dfn export id="dfn-service-worker-registration" for="">service worker registration</dfn> is a tuple of a [=service worker registration/scope url=] and a set of [=/service workers=], an <a>installing worker</a>, a <a>waiting worker</a>, and an <a>active worker</a>. A user agent *may* enable many [=/service worker registrations=] at a single origin so long as the [=service worker registration/scope url=] of the [=/service worker registration=] differs. A [=/service worker registration=] of an identical [=service worker registration/scope url=] when one already exists in the user agent causes the existing [=/service worker registration=] to be replaced.
+    A <dfn export id="dfn-service-worker-registration" for="">service worker registration</dfn> is a tuple of an [=environment settings object/origin=], an [=service worker registration/id=], and a set of [=/service workers=]; an <a>installing worker</a>, a <a>waiting worker</a>, and an <a>active worker</a>. A user agent *may* enable many [=/service worker registrations=] at a single origin so long as the [=service worker registration/id=] of the [=/service worker registration=] differs.


What do you think about just removing this paragraph? It doesn't seem to be saying anything that isn't already said elsewhere.

I'm not sure I can just remove it as it include a <dfn> tag for "service worker registration".

jakearchibald · 2020-08-21T10:10:05Z

docs/index.bs

-      :: A [=/service worker registration=]
+      :: A [=/service worker registration=] or null.
+
+      1. Run the following steps atomically.


I guess we have this a few places in the spec, but I don't actually know what it means.

docs/index.bs

jakearchibald · 2020-08-21T11:43:19Z

docs/index.bs

@@ -2797,6 +2862,10 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
      1. If |registration|'s [=active worker=] is not null, then:
          1. [=Terminate Service Worker|Terminate=] |registration|'s [=active worker=].
          1. Run the [=Update Worker State=] algorithm passing |registration|'s [=active worker=] and "`redundant`" as the arguments.
+      1. Let |oldUsingClients| be a [=list=] of [=/service worker clients=] who are <a>using</a> |registration|.
+
+      Note: We must get the list of clients prior to clearing the old <a>active worker</a> from |registration|.  Otherwise the [=/service worker client=] will not be considered to be <a>using</a> |registration| any more since |registration| will no longer be the <a>containing service worker registration</a>.


Good catch!

docs/index.bs

wanderview · 2020-08-21T15:06:13Z

Thanks for the review @jakearchibald! I will work on addressing this next week after I clear some other things off my TODO list.

annevk · 2020-10-12T10:34:35Z

docs/index.bs

@@ -683,6 +702,7 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
    </pre>
    <pre class="idl" id="registration-option-list-dictionary">
      dictionary RegistrationOptions {
+        DOMString id;


Please write a test with isolated surrogates to ensure implementations do not use UTF-8 to store these strings. (If you decide to change this to USVString to allow implementations to do that it's probably still worth writing a test.)

…ixes w3c#1512) This commit adds support for a registration `id` value. This will be used to uniquely identify registrations instead of `scope`. This will make it easier for a developer to migrate a registration from one `scope` to another. The commit consists of the following changes: * Add a DOMString `id` and associated getter to the registration. * Add an `origin` to the internal registration type. * Move the `scope` internal representation to `ServiceWorker` and add a new getter. * Make the `ServiceWorkerRegistration.scope` return the oldest associated worker's `scope`. * Migrate the registration map to be keyed by the tuple `(origin,id)`. * Migrate the job queue map to be keyed by the tuple `(origin,id)`. * Adjust job equality checks to treat register jobs to account for new id and scope semantics. * Support changing the scope during register operations. * Reject register operations that change the scope to a value that is already in use by another registration. * Properly un-control clients whose URL no longer matches a new active worker's scope.

Co-authored-by: Jake Archibald <jaffathecake@gmail.com>

wanderview · 2022-01-20T17:21:13Z

I think I've updated everything here finally. No need to rush to re-review, though. I'll be working on implementation for a while.

wanderview mentioned this pull request Jul 31, 2020

consider allowing a non-scope identifier for registrations #1512

Open

wanderview force-pushed the dev-id branch from 0b0f372 to eecef52 Compare August 7, 2020 21:10

wanderview mentioned this pull request Aug 10, 2020

bikeshed curl service is failing tobie/pr-preview#79

Closed

wanderview changed the title ~~DO NOT MERGE - Add id to ServiceWorkerRegistration. (Fixes #1512)~~ Add ServiceWorkerRegistration.id and supporting algorithm changes. (Fixes #1512) Aug 14, 2020

wanderview force-pushed the dev-id branch from 5b53aa3 to 9bef311 Compare August 14, 2020 21:22

wanderview marked this pull request as ready for review August 14, 2020 21:24

jakearchibald requested changes Aug 21, 2020

View reviewed changes

annevk reviewed Oct 12, 2020

View reviewed changes

Base automatically changed from master to main February 4, 2021 19:56

wanderview force-pushed the dev-id branch from 9bef311 to 012031e Compare January 14, 2022 19:42

wanderview and others added 7 commits January 14, 2022 14:48

Apply suggestions from code review

b693bf0

Co-authored-by: Jake Archibald <jaffathecake@gmail.com>

Remove unnecessary steps from unregister().

336b11c

Use this's relevant settings object instead of current settings object.

9a0fee9

Move scopeURL variable declaration.

7c65780

simplify Get Registration algorithm

b11f88e

note that scope and script can be null in jobs

5e057c6

add 'last relevant scope' to registration

6b92a44

expose ServiceWorker.scope

f73f5fd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `ServiceWorkerRegistration.id` and supporting algorithm changes. (Fixes #1512) #1526

Add `ServiceWorkerRegistration.id` and supporting algorithm changes. (Fixes #1512) #1526

wanderview commented Jul 31, 2020 •

edited by pr-preview bot

Loading

sideshowbarker commented Jul 31, 2020

ylafon commented Aug 2, 2020

wanderview commented Aug 3, 2020

jakearchibald left a comment

jakearchibald Aug 21, 2020

wanderview Jan 20, 2022

jakearchibald Aug 21, 2020

jakearchibald Aug 21, 2020

wanderview commented Aug 21, 2020

annevk Oct 12, 2020

wanderview commented Jan 20, 2022

Add ServiceWorkerRegistration.id and supporting algorithm changes. (Fixes #1512) #1526

Are you sure you want to change the base?

Add ServiceWorkerRegistration.id and supporting algorithm changes. (Fixes #1512) #1526

Conversation

wanderview commented Jul 31, 2020 • edited by pr-preview bot Loading

sideshowbarker commented Jul 31, 2020

ylafon commented Aug 2, 2020

wanderview commented Aug 3, 2020

jakearchibald left a comment

Choose a reason for hiding this comment

jakearchibald Aug 21, 2020

Choose a reason for hiding this comment

wanderview Jan 20, 2022

Choose a reason for hiding this comment

jakearchibald Aug 21, 2020

Choose a reason for hiding this comment

jakearchibald Aug 21, 2020

Choose a reason for hiding this comment

wanderview commented Aug 21, 2020

annevk Oct 12, 2020

Choose a reason for hiding this comment

wanderview commented Jan 20, 2022

Add `ServiceWorkerRegistration.id` and supporting algorithm changes. (Fixes #1512) #1526

Add `ServiceWorkerRegistration.id` and supporting algorithm changes. (Fixes #1512) #1526

wanderview commented Jul 31, 2020 •

edited by pr-preview bot

Loading