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

Move installEvent.replace() onto ServiceWorkerClients #518

Closed
jakearchibald opened this issue Oct 22, 2014 · 14 comments
Closed

Move installEvent.replace() onto ServiceWorkerClients #518

jakearchibald opened this issue Oct 22, 2014 · 14 comments

Comments

@jakearchibald
Copy link
Contributor

Use Case comes from @paullewis, paraphrasing:

When a new SW version installs, I want to postMessage any in-scope pages so they can show an "update downloaded, click to show" banner. When this banner is clicked, the waiting worker is nudged, it calls replace() and the pages refresh themselves so they're loaded with the latest data.

reloadAll() would better suit this use case, but we're a while off specing that (it's huge and full of new things).

Paul's use case isn't possible because the install event has been and gone, so the opportunity to call replace() has passed. We already moved reloadAll() to ServiceWorkerClients before we punted on it, I think replace() makes more sense there too, renamed something like takeControl.

So, if a ServiceWorker calls clients.takeControl():

  1. If the ServiceWorker is installing, wait until install complete
  2. If the ServiceWorker is redundant, reject
  3. If the ServiceWorker is installed, activate it (removing any current active version)
  4. For each client
    1. If the result of running Match Scope algorithm, or its equivalent, with documentURL as its argument, is equal to this worker's registration, then
    2. Let client use this worker's registration as its service worker registration
@slightlyoff
Copy link
Contributor

I'm fine with this renaming/location.

@KenjiBaheux
Copy link
Collaborator

Tracked via http://crbug.com/430639 in Blink

@jakearchibald
Copy link
Contributor Author

@paullewis reads clients.takeControl() as "the clients take control", which is fair. Open to other names.

@paullewis
Copy link

I'm still of the persuasion that we should have a subject-verb-object approach, i.e. self.takeControl(), where clients is the implicit object given that's all the SW can take control of.

But if we're absolutely set that clients is the right place then perhaps a more clients.notifyChange() since that a) implies (to me at least) that the SW has replaced the previous one and b) presumably can / should / would send some event to the client side so it can react to the change, if applicable. Also we could not send the event if the client has a better scope-matched SW because it doesn't need to know.

@jakearchibald
Copy link
Contributor Author

It's more than just a notification, it's a (potentially) hostile takeover. clients.reloadAll will be the more interactive softly softly takeover.

I've updated the algorithm in the original post to be more accurate.

@paullewis
Copy link

OK, I can see notify might appear soft. Is there any reason why this has to live on clients rather than either event or the Service Worker itself? I'm still fighting the fact that clients.takeControl() or clients.reloadAll doesn't explicitly mention the Service Worker. Mayyyybe just me, but I kind of feel too much implicit gives way to ambiguity.

@jakearchibald
Copy link
Contributor Author

We moved it off InstallEvent based on your feedback. You had the idea of a "Update available" banner that, once clicked, caused the "take over" to happen. This would be clicked outside of the install event loop, so the install event object is the wrong place for it.

However, maybe the force takeover and force upgrade parts of this API need splitting out. I don't have any use-cases for using them separately, but they are two distinct actions.

  1. registration.[[something]] - steps 1-3 of the algorithm in the original post
  2. clients.[[something]] - step 4

@jakearchibald
Copy link
Contributor Author

I'm going to tackle steps 1-3 first, via a "skip waiting" flag on the worker, with ways to set this from the worker's global scope and the worker object itself.

@jakearchibald
Copy link
Contributor Author

Added the flag c839e33

@jakearchibald
Copy link
Contributor Author

I'm going for

  • self.skipWaiting() and
  • worker.skipWaiting()

Bikeshed away.

@jakearchibald
Copy link
Contributor Author

dcecb29 - specced skipWaiting(). @jungkees you may want to review, but do give feedback on anything I've gotten wrong so I don't make the same mistakes again :D

@jungkees
Copy link
Collaborator

jungkees commented Nov 7, 2014

@jakearchibald looks nice! Just added a few steps back which were added to solve #364: 6d2cc26. And changed a type from undefined to IDL any in Promise<T> skipWaiting();. Thanks!

@jungkees
Copy link
Collaborator

jungkees commented Jan 9, 2015

self.skipWaiting() is done. clients.claim() will be tracked in #586. Closing.

@jungkees jungkees closed this as completed Jan 9, 2015
@jungkees
Copy link
Collaborator

Clients.claim() has been specified: aeee6b9.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants