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

Make Service Worker registration work properly (don't use outdated steps) #792

Merged
merged 4 commits into from Nov 12, 2019

Conversation

mgiuca
Copy link
Collaborator

@mgiuca mgiuca commented Sep 13, 2019

This change (choose one):

  • Breaks existing normative behavior (please add label "breaking")
  • Adds new normative requirements
  • Adds new normative recommendations or optional items
  • Makes only editorial changes (only changes informative sections, or
    changes normative sections without changing behavior)
  • Is a "chore" (metadata, formatting, fixing warnings, etc).

Commit message:

Make Service Worker registration work properly (don't use outdated steps).

Closes #789.


Preview | Diff

index.html Outdated
A {{Document}} may either be <dfn>installable</dfn> or not. The initial
state of a document is not <a>installable</a>.
</p>
<p>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this PR have unrelated changes? It seems so

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ugh, yes, this is because GitHub doesn't respect upstream branches in Git. This PR is dependent on #790 which hasn't landed yet, and I don't think there's any way for GitHub to show just the relevant changes.

Please look at this delta.

@mgiuca
Copy link
Collaborator Author

mgiuca commented Sep 13, 2019

@marcoscaceres While I'm on a roll with Manifest...

Edit: Please look at this delta.

  1. I need a bit of help with ReSpec. (General ReSpec comments appreciated; I've been trying out the new (?) |var|.{{Method}} syntax.) I tried to add a reference to RegistrationOptions.type using "for="RegistrationOptions"" (the "for" clause working well for all the other methods and members). But I get a conflict with the other definition of ECMAScript's Type() function. I thought "for" was supposed to disambiguate it by saying "this type is for RegistrationOptions only" but it still conflicts.
  2. This is a bit nasty but it's in the current text too: (the old text reads: "the state of the settled promise determines whether the installation succeeded or not") --- the fact that if service worker registration fails, the whole installation is aborted, even though service worker registration is optional. So some user agents will not bother to register a (failing) SW, thus successfully installing, and others will try to register the SW and fail the install. Is that "okay" (essentially making a best effort to check, but you don't have to check). Or should we succeed the install even if SW registration fails, for consistency across user agents?

@mgiuca mgiuca mentioned this pull request Sep 20, 2019
5 tasks
@mgiuca mgiuca force-pushed the service-worker-start-register branch from 5a56e10 to 7a551e3 Compare September 26, 2019 06:51
@mgiuca
Copy link
Collaborator Author

mgiuca commented Sep 26, 2019

The upstream PR has now merged, so this PR's delta is correct.

Copy link
Member

@marcoscaceres marcoscaceres left a comment

Choose a reason for hiding this comment

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

Approved with caveat the the service worker stuff will be deleted right after (as decided in #800).

@kenchris
Copy link
Collaborator

Well Jeffrey (talked at TPAC) says to please not do that as he is going to use that innWeb Packaging which uses web app manifest

@kenchris
Copy link
Collaborator

@jyasskin could you comment?

@marcoscaceres
Copy link
Member

Sure, but the multi-implementer problem remains. Not sure at this point we want to support this in Gecko, but need to check if WebKit supports it.

@jyasskin
Copy link
Member

jyasskin commented Oct 2, 2019

We might use the serviceworker field to have navigation to a web package automatically install its service worker before the start page loads, but I think we can re-add the field if/when we do that. It won't hurt us to remove it until then.

@marcoscaceres
Copy link
Member

Ok, let's proceed with the plan to remove it... we can always bring it back.

@marcoscaceres marcoscaceres merged commit 6708bc8 into w3c:gh-pages Nov 12, 2019
@mgiuca
Copy link
Collaborator Author

mgiuca commented Nov 14, 2019

Oops, thanks for closing this out.

christianliebel pushed a commit to christianliebel/manifest that referenced this pull request May 27, 2020
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.

Service worker installation uses an algorithm (Start Register) that doesn't exist in SW spec
4 participants