Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API description - Part 1. #5

Merged
merged 1 commit into from
Nov 20, 2019
Merged

API description - Part 1. #5

merged 1 commit into from
Nov 20, 2019

Conversation

mugdhalakhani
Copy link
Owner

@beverloo PTAL.

spec/PeriodicBackgroundSync-index.bs Outdated Show resolved Hide resolved
spec/PeriodicBackgroundSync-index.bs Outdated Show resolved Hide resolved
spec/PeriodicBackgroundSync-index.bs Outdated Show resolved Hide resolved
spec/PeriodicBackgroundSync-index.bs Outdated Show resolved Hide resolved
spec/PeriodicBackgroundSync-index.bs Show resolved Hide resolved
spec/PeriodicBackgroundSync-index.bs Outdated Show resolved Hide resolved
spec/PeriodicBackgroundSync-index.bs Outdated Show resolved Hide resolved
spec/PeriodicBackgroundSync-index.bs Outdated Show resolved Hide resolved
spec/PeriodicBackgroundSync-index.bs Outdated Show resolved Hide resolved
spec/PeriodicBackgroundSync-index.bs Outdated Show resolved Hide resolved
spec/PeriodicBackgroundSync-index.bs Outdated Show resolved Hide resolved
spec/PeriodicBackgroundSync-index.bs Outdated Show resolved Hide resolved
spec/PeriodicBackgroundSync-index.bs Outdated Show resolved Hide resolved
spec/PeriodicBackgroundSync-index.bs Outdated Show resolved Hide resolved
spec/PeriodicBackgroundSync-index.bs Outdated Show resolved Hide resolved
1. Else:
1. If |timeSinceLastPeriodicSync| is greater than equal to |minIntervalAcrossOrigins|, set |timeToFire| to |delayForOrigin| + |now|.
1. Else, set <var>timeTillNextAllowedPeriodicSync</var> to |minIntervalAcrossOrigins| - |timeSinceLastPeriodicSync|. Set |timeToFire| to the maximum of |delayForOrigin| + |now|, and |timeTillNextAllowedPeriodicSync| + |now|.
1. Set the [=time to fire=] of |registration| to |timeToFire|.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

|registrations|'s [=time to fire=]

Also, I'd scope [=time to fire=] with the registration dfn, so it is accessible by using [=registration/time to fire=]

Copy link
Owner Author

@mugdhalakhani mugdhalakhani Nov 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re comment 1: done.
Re comment 2: In terms of how this is visible in the html, there's no difference. Also, if I do this, the same logic would apply to all other definitions under registration. Can you explain your reasoning? Are you worried these will be used outside of the spec?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's for maintainability

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. The spec is a lot less readable, so I'd still appreciate an example of how this improves maintainability.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same reason we use scopes and namespaces in code instead of having everything as a global variables. I'm sure you'll appreciate it if you have to amend sections of the spec a few months down the line :)

spec/PeriodicBackgroundSync-index.bs Outdated Show resolved Hide resolved
spec/PeriodicBackgroundSync-index.bs Show resolved Hide resolved
spec/PeriodicBackgroundSync-index.bs Outdated Show resolved Hide resolved
spec/PeriodicBackgroundSync-index.bs Outdated Show resolved Hide resolved
spec/PeriodicBackgroundSync-index.bs Outdated Show resolved Hide resolved
spec/PeriodicBackgroundSync-index.bs Outdated Show resolved Hide resolved
The user agent may also decide to retry each failed periodicSync event, capped to a user agent defined <dfn>maximum number of retries</dfn>.

A possible algorithm to calculate the [=time to fire=], <var>timeToFire</var> for |registration| would involve running these steps:
1. If this is the <dfn>first attempt of firing</dfn> the periodicSync event:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping

1. Else:
1. If |timeSinceLastPeriodicSync| is greater than equal to |minIntervalAcrossOrigins|, set |timeToFire| to |delayForOrigin| + |now|.
1. Else, set <var>timeTillNextAllowedPeriodicSync</var> to |minIntervalAcrossOrigins| - |timeSinceLastPeriodicSync|. Set |timeToFire| to the maximum of |delayForOrigin| + |now|, and |timeTillNextAllowedPeriodicSync| + |now|.
1. Set the [=time to fire=] of |registration| to |timeToFire|.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's for maintainability


The user agent MAY include factors such as user engagement with the origin to decide this time interval, allowing origins with high user engagement to update their web apps more often. In addition, the user agent SHOULD have per-origin and cross-origin caps on this interval, as recommended in [[#privacy]] and [[#resources]] sections.

The user agent may also decide to retry each failed periodicSync event, capped to a user agent defined <dfn>maximum number of retries</dfn>.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add a type here, the algorithm below would also benefit from types for variables

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

spec/PeriodicBackgroundSync-index.bs Outdated Show resolved Hide resolved
spec/PeriodicBackgroundSync-index.bs Outdated Show resolved Hide resolved
spec/PeriodicBackgroundSync-index.bs Outdated Show resolved Hide resolved
spec/PeriodicBackgroundSync-index.bs Outdated Show resolved Hide resolved
@mugdhalakhani
Copy link
Owner Author

@rayankans I've moved global constants to Constructs, and added scoped references and types to the algorithms. PTAL.

Copy link
Collaborator

@rayankans rayankans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM % Few minor comments & removing the periodicsync registration definition duplication

spec/PeriodicBackgroundSync-index.bs Outdated Show resolved Hide resolved
spec/PeriodicBackgroundSync-index.bs Outdated Show resolved Hide resolved
spec/PeriodicBackgroundSync-index.bs Outdated Show resolved Hide resolved
spec/PeriodicBackgroundSync-index.bs Outdated Show resolved Hide resolved

The <dfn>registration state</dfn> is one of <dfn>pending</dfn>, <dfn>waiting</dfn>, <dfn>firing</dfn>, or <dfn>reregisteredWhileFiring</dfn>. It is initially set to [=pending=].
The <dfn>options</dfn> are a dictionary containing {{options/minInterval}}, a long long. <dfn attribute for=options>minInterval</dfn> is used to specify the minimum interval, in milliseconds, at which the periodic synchronization should happen. {{options/minInterval}} is a suggestion to the user agent. The actual interval at which <code>periodicsync</code> events are fired MUST be greater than or equal to this.
Enclosing [=periodicsync registration/options=] in a dictionary allows this spec to be extended with more [=periodicsync registration/options=] in the future without adversely affecting existing usage.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't include this line

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a recommendation by Peter in a previous comment on this CL.


The options are a dictionary containing [=minInterval=], a long long. <dfn>minInterval</dfn> 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 <code>periodicsync</code> events are fired MUST be greater than or equal to this.
Enclosing options in a dictionary allows this spec to be extended with more options in the future without adversely affecting existing usage.
The <dfn>tag</dfn> is a {{DOMString}}. Within one [=list of periodicsync registrations=] each [=periodicsync registration=] MUST have a unique [=periodicsync registration/tag=]. Periodic Background Sync doesn't share namespace with Background Sync, so an [=origin can have registrations of both types with the same tag.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[=/string=]

Copy link
Owner Author

@mugdhalakhani mugdhalakhani Nov 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't need to include any default-links with DOMString, whereas with =/string= there's still ambiguity:
Arbitrarily chose https://drafts.csswg.org/css-values-4/#string
To auto-select one of the following refs, insert one of these lines into a

 block:
spec:infra; type:dfn; text:string
spec:css-values-4; type:dfn; text:string

spec/PeriodicBackgroundSync-index.bs Outdated Show resolved Hide resolved
spec/PeriodicBackgroundSync-index.bs Outdated Show resolved Hide resolved
spec/PeriodicBackgroundSync-index.bs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants