Periodic background sync spec feedback

[jakearchibald]

cc @mugdhalakhani

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#in-the-background

The periodicSync event is considered to run in the background if

The prose here suggests that the definition "in the background" is owned by "the periodicSync event", but the definition isn't "for" anything. Either attach the definition to something, or rename the definition so the context is clear (eg "a background periodic sync").

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#in-the-background

whose frame type is top-level or auxiliary exist

Nit: use markup to make it clear that "top-level" and "auxiliary" are values, as in https://w3c.github.io/ServiceWorker/#dfn-service-worker-client-frame-type.

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#in-the-background

The definition of "in the background" seems odd to me. A "nested" frame type can currently run as much JavaScript as it wants, but we don't consider it to be in the background. Either the definition needs to change, or the condition needs to include nested frame types.

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#concepts

The user agent is considered to be online if the user agent…

This is written as if 'online' is a definition, but it's linking to another document.

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#periodic-background-sync-opportunity

A periodic Background Sync opportunity allows periodic synchronization

This definition seems vague and is only used in the example. It probably isn't needed.

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#firing-a-periodicsync-event

In parallel, fire functional event "periodicSync" using

Event names are usually lowercase. Eg "periodicsync".

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#periodicsync-processing-queue

This is queuing all period sync actions across all origins, is that level of synchronisation required? Would it be enough to create a queue per service worker registration?

---

The spec refers to "periodicsync" throughout. I would change this to "periodic sync" unless there's some reason the space should be omitted, such as event/property names.

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#constructs

A service worker registration has an associated list of periodicsync registrations whose element type is a periodicsync registration.

Nit: I would call this "active periodic sync registrations". This means you can link to the definition of list, since it isn't part of the definition name.

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#periodic-sync-registration

A periodicsync registration is a tuple

Given that the order of items isn't important, I don't think this should be a tuple. Just say "A periodic sync registration consists of:".

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#periodicsync-registration-service-worker-registration

A service worker registration, which is the service worker registration associated with the context object the PeriodicSyncManager belongs to.

There's no context object here, so you can't use it. Just say it's a service worker registration, and set it when you do have a context object.

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#periodicsync-registration-tag

A tag, which is a DOMString. Within one list of periodicsync registrations each periodicsync registration MUST have a unique tag. Periodic Background Sync doesn’t share namespace with Background Sync, so an origin can have registrations of both types with the same tag.

The uniqueness constraint isn't enforced here, so it should be a note, or not mentioned here at all.

It seems like the "list of periodicsync registrations" should be a map, which would help enforce this uniqueness constraint, as with https://wicg.github.io/background-fetch/#service-worker-registration-active-background-fetches.

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#periodicsync-registration-options

options, which is a dictionary containing minInterval. Enclosing options in a dictionary allows this spec to be extended with more options in the future without adversely affecting existing usage.

minInterval (a long long), which is used to specify the minimum interval, in milliseconds, at which the periodic synchronization should happen. minInterval is a suggestion to the user agent. The actual interval at which periodicSync events are fired MUST be greater than or equal to this.

It seems like an error to have both of these, especially as minInterval is never set. I would remove options and just use minInterval. I would also call it "min interval" or "minimum interval".

Enclosing options in a dictionary allows this spec to be extended with more options in the future without adversely affecting existing usage.

Nit: Text like the above should be a note.

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#periodicsync-registration-registration-state

A registration state, which is one of pending, firing, or reregisteredWhileFiring. It is initially set to pending.

This is owned by the registration, so I'd just call it "state". It's ok to make the state values definitions, but they should be owned by "state", not the grandparent registration. Otherwise, make them values like https://fetch.spec.whatwg.org/#concept-https-state-value.

If reregisteredWhileFiring continues to be a definition, give it spaces.

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#constants

minimum interval for any origin, a long long, that represents the minimum gap between periodicSync events for any given origin, and,

minimum interval across origins, a long long, that represents the minimum gap between periodicSync events across all origins.

I can't figure out the difference between these.

Also, since these definitions are global, their naming should be more specific. Eg "minimum periodic background sync interval across origins".

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#maximum-number-of-retries

The user agent MAY define a maximum number of retries

This is a "may" but no default is provided, and references don't check to see if it was set. I guess this should have a default of "infinity"?

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#global-state

Global state

It isn't clear to me what the difference is between "global state" and things that are defined as being "the user agent has" - aren't they equally global?

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#time-the-last-periodicsync-event-was-fired

A user agent SHOULD keep track of the time the last periodicSync event was fired

It isn't clear at a glance what this is used for, and why a user agent may or may not record it. Maybe add a note?

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#history-leaking

The fetch requests are HTTPS so the request contents will not be leaked

They're leaked to the destination of the fetch, which is also a concern.

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#periodicsyncmanager-interface

A PeriodicSyncManager has a service worker registration (a service worker registration).

The "service worker registration" here is defined as an attribute, but it shouldn't be. See https://wicg.github.io/background-fetch/#backgroundfetchmanager-service-worker-registration.

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#extensions-to-serviceworkerregistration

initially a new PeriodicSyncManager whose service worker registration is the context object's service worker registration.

There isn't a context object at this point. There's a nice bit of shorthand that can help here:

https://heycam.github.io/webidl/#SameObject

Now you can move the creation of the PeriodicSyncManager into the getter, where you'll have access to the context object.

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#dom-periodicsyncmanager-register

The register(tag, options) method, when invoked, MUST return a new promise promise and run the following steps:

This form of shorthand (return & run steps) only really makes sense if the steps are happening elsewhere, eg in parallel. However, that isn't happening here.

Another symptom of the same problem:

If serviceWorkerRegistration’s active worker is null, reject promise with an InvalidStateError and abort these steps.

You're accessing service worker registration state on the main thread, which isn't allowed. Maybe all these steps should happen on the periodic sync processing queue?

---

Nit: if you tell bikeshed which parts of your spec are algorithms, you get extra validation, and highlighting of vars on click. Eg https://wicg.github.io/background-fetch/#background-fetch-manager-fetch.

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#dom-periodicsyncmanager-register

Let serviceWorkerRegistration be the PeriodicSyncManager's associated

There's no "the PeriodicSyncManager" here. I think you want "the context object".

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#caculate-time-to-fire

A possible algorithm to calculate the time to fire…

It makes me nervous that a browser could do random things here can be spec compliant. Are none of the steps here required? Certain parts like incrementing the count of retries seems like it should be defined rather than suggested.

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#caculate-time-to-fire

Let origin represent service worker registration's origin. Let penalty be a user agent defined penalty to account for the level of user engagement with the origin. Calculate minimumIntervalForOrigin as penalty*minimum interval for any origin.

This should be three separate steps. Variables should be defined with "Let foo be (value)" - avoid things like "Calculate".

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#caculate-time-to-fire

Let origin represent service worker registration

Which service worker registration?

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#caculate-time-to-fire

Set delayForOrigin to the multiple of

"Set" is for updating existing variables, but this looks like a new variable.

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#caculate-time-to-fire

Let timeTillScheduledEventForOrigin, a number, be the time till the next scheduled periodicSync event for origin, if any, null otherwise.

"the time till the next scheduled" isn't defined.

---

If timeTillScheduledEventForOrigin - delayForOrigin is greater than or equal to minIntervalForOrigin, abort these substeps.

Just use "steps", unless you mean something more granular than aborting the algorithm. If it's something more granular, it isn't clear which bit should be aborted.

---

Set the time to fire of registration to timeToFire.

Nit: I think it's easier to read as "Set registration's time to fire to timeToFire".

---

Increment count of retries.

If count of retries

You need to be clear about which registration you're talking about here.

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#dom-periodicsyncmanager-register

Schedule processing for registration.

"registration" should be "newRegistration" I think.

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#schedule-delayed-processing

Schedule firing a periodicSync event for registration as soon as the the device is online at or after registration’s time to fire.

This seems a bit hand-wavey to me. I'd spell it out a bit more. Wait until the target time, then wait some more if the user is offline.

Also, what if the background sync is unregistered during this time?

---

In general, I can't figure out how unregistering works. I see that it removes the registration from the list, but it doesn't seem to stop it scheduling.

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#periodicSync-event

You can (and should) define the constructor as a method. See: https://wicg.github.io/background-fetch/#background-fetch-event

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#firing-periodicsync-events

Whenever the user agent changes to online, it SHOULD fire a periodicSync event for each periodicsync registration whose registration state is pending and time to fire is now or in the past.

This seems like an additional scheduler to the one mentioned in https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#schedule-delayed-processing. There should only be one.

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#dom-periodicsyncmanager-register

If the currentRegistration’s options is different from options:

Define what "different" means.

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#ref-for-periodicsync-registration-reregisteredwhilefiring

Else, if currentRegistration’s registration state is firing, set serviceWorkerRegistration’s registration state to reregisteredWhileFiring.

"reregisteredWhileFiring" isn't referenced anywhere else. What's it for?

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#dom-periodicsyncmanager-register

Set currentRegistration’s associated registration state to pending.

The sync may be mid-firing at this point, making it possible to have two overlapping sync events for the same registration. That doesn't seem intentional.

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#dom-periodicsyncmanager-gettags

Let serviceWorkerRegistration be the PeriodicSyncManager's associated…

Which PeriodicSyncManager? I think you want the context object.

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#dom-periodicsyncmanager-unregister

Also needs to use the context object.

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#dom-periodicsyncmanager-unregister

If serviceWorkerRegistration’s active worker is null, reject promise with an InvalidStateError and abort these steps.

You can't access the service worker registration from the main thread like this.

Also, does it make sense to reject here? If there's no active worker, then there's no periodic sync with that name, so unregistration won't be a failure as such.

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#periodicSync-event

The PeriodicSyncEvent interface represents a firing periodicsync registration.

The PeriodicSyncEvent may still be in reference when the periodic sync is pending, so this statement isn't really true.

---

https://wicg.github.io/BackgroundSync/spec/PeriodicBackgroundSync-index.html#firing-a-periodicsync-event

In parallel, fire functional event "periodicSync"

I'm not sure what the benefit of "in parallel" is there. It doesn't look like this is ever called on the main thread.

---

Check the bikeshed errors, there are some ambiguous references. Also, promises have moved to webidl: https://heycam.github.io/webidl/#resolve.