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

How to create registration objects #1445

Merged
merged 3 commits into from
Jul 1, 2019
Merged

Conversation

jakearchibald
Copy link
Contributor

@jakearchibald jakearchibald commented Jun 28, 2019

As I did with service worker objects, I made it more formal with registrations too.

I'll merge to v1 once we're happy with this.


Preview | Diff

@jakearchibald jakearchibald requested a review from mfalken June 28, 2019 15:06
Copy link
Member

@mfalken mfalken left a comment

Choose a reason for hiding this comment

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

lgtm! Just some nits (feel free to ignore if they don't sound right).

docs/index.bs Outdated
1. If |objectMap|[|registration|] does not [=map/exist=], then:
1. Let |registrationObject| be a new {{ServiceWorkerRegistration}} in |environment|'s [=environment settings object/Realm=].
1. Set |registrationObject|'s [=ServiceWorkerRegistration/service worker registration=] to |registration|.
1. Set |registrationObject|'s {{ServiceWorkerRegistration/installing}} attribute to null if |registration|'s [=service worker registration/installing worker=] is null, otherwise the result of [=getting the service worker object=] that represents |registration|'s [=service worker registration/installing worker=] in |environment|.
Copy link
Member

Choose a reason for hiding this comment

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

I think this is a run-on sentence. I was once told the spec convention is "to X, if Y, and Z otherwise".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh really? I kinda hate that form since you have to read on to realise it's an "otherwise". But I guess the real problem here is it's too long for a one-liner. I'll split it up.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, splitting it up is good too. I was just remembering whatwg/html#3891 (comment). I think just adding "and" before "otherwise" would work as well too: "if Y, and otherwise Z".

docs/index.bs Outdated
</section>
</section>

A {{ServiceWorkerRegistration}} has a <dfn for="ServiceWorkerRegistration">service worker registration</dfn> (a [=/service worker registration=]).
Copy link
Member

Choose a reason for hiding this comment

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

Could this sentence go above this section? Since the algorithm above refers to this definition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes. No idea why I put it here.

docs/index.bs Outdated
1. Resolve |promise| with the {{ServiceWorkerRegistration}} object which represents |registration|.
1. Else:
1. Resolve |promise| with undefined.
1. If |registration| is null, resolve |promise| with undefined, else resolve |promise| with the result of [=getting the service worker registration object=] that represents |registration| in |promise|'s [=relevant settings object=].
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this if/else construction is conventional. How about splitting it to two statements or else doing the: "Resolve |promise| with undefined, if |registration| is null, and the result of getting the registration... otherwise."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll split it into two steps

@jakearchibald jakearchibald merged commit 7c07a47 into master Jul 1, 2019
@jakearchibald jakearchibald deleted the reg-object-creation branch July 1, 2019 08:55
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.

2 participants