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

Add State to Service Worker Clients. #37

Merged
merged 6 commits into from
Aug 7, 2019
Merged

Conversation

dtapuska
Copy link
Contributor

@dtapuska dtapuska commented Jul 16, 2019

As discussed on w3c/ServiceWorker#1442

Unfortunately adoption of page-lifecycle is yet to be formally
supported by other vendors so we need to monkey patch this in the
page lifecycle spec.

@dtapuska
Copy link
Contributor Author

@jakearchibald as discussed offline.
@domenic let me know what you think if we can put this in the page lifecycle spec for now.

spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated
};
</pre>

A {{Client}} object has an associated <dfn id="dfn-service-worker-client-state" for="Client">state</dfn>, which is one of {{ClientState}} attribute value.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nothing ever sets this value. That seems bad.

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 skipped adjusting the Create Client algorithms where state is passed in.. I've now added them so it should be clearer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, this is clearer. The remaining thing is to add two subsections of "Algorithms" that explains how you modify "Create Window Client" and "Create Client" to take the new argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

spec.bs Show resolved Hide resolved
@domenic
Copy link
Collaborator

domenic commented Jul 31, 2019

So the remaining big issue is workers. This PR operates on all clients, but "Get Client Lifecycle State" consults the responsible document, which only exists for window environment settings objects. (See whatwg/html#4335 and in particular whatwg/html#4335 (comment).)

Maybe this is as simple as using https://wicg.github.io/page-lifecycle/#dedicatedworkerglobalscope-owning-document for dedicated workers. And I think clients can only be dedicated workers or windows, right? So that'd work?

dtapuska and others added 4 commits July 31, 2019 12:36
As discussed on w3c/ServiceWorker#1442

Unfortunately adoption of page-lifecycle is yet to be formally
supported by other vendors so we need to monkey patch this in the
page lifecycle spec.
- Remove "Service Worker:" prefix. We should remove the "HTML:" prefix for HTML modifications in a separate commit.
- Match service worker section titles
- Use Markdown-style headings instead of mixing in HTML and Markdown
- Service worker client internal slots are like "lifecycle state" not "lifecycleState"
- Fix section headings for matchAll/openWindow; those are methods, not algorithms
- Nest sections properly (e.g. Clients stuff goes under Clients)
They were used but not turned on, so they were appearing raw.
- Change definition of frozen to using owning document
@dtapuska
Copy link
Contributor Author

dtapuska commented Aug 1, 2019

So the remaining big issue is workers. This PR operates on all clients, but "Get Client Lifecycle State" consults the responsible document, which only exists for window environment settings objects. (See whatwg/html#4335 and in particular whatwg/html#4335 (comment).)

Maybe this is as simple as using https://wicg.github.io/page-lifecycle/#dedicatedworkerglobalscope-owning-document for dedicated workers. And I think clients can only be dedicated workers or windows, right? So that'd work?

I've adjusted it back to what I wrote before Jake asked me to change it to be the resposible document. (See w3c/ServiceWorker@a5bc071#r303010890) I prefered using owning document anyways.

Copy link
Collaborator

@domenic domenic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the patience!

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