Implement a service worker observer that signals when installation is complete |
|||||
Issue descriptionApp install banners use ServiceWorkerContext::CheckHasServiceWorker, but this is somewhat flaky as it depends on a registration either existing or being in progress at the time of the check. For example, jakearchibald.github.io/svgomg lazy-loads its service worker, so there isn't a registration in progress until after the page finishes loading (and the app banner flow runs). Instead, an observer could be added to ServiceWorkerContext. The app banner system can register as an observer and be notified when there is a service worker registration rather than relying on one being in progress when it decides to check.
,
Jul 4 2016
So there is already a ServiceWorkerContextObserver which lives in content/browser/service_worker and is attached to ServiceWorkerContextWrapper, which is exactly where I wanted to add the new observer. At first glance, the OnRegistrationStored method on that observer tells us when a new service worker is stored, which is what we want. We may still need to use CheckHasServiceWorker to check for existing registrations, but I think that is okay. Unfortunately, this existing observer cannot be used from chrome/, since it isn't in content/public. @falken, is there any way we can move the observer out of browser and into public? It looks like a pretty low level API... perhaps it could be split into a public section and a content-private one which inherits the public one?
,
Jul 4 2016
> It looks like a pretty low level API... perhaps it could be split into a public section and a content-private one which inherits the public one? This sounds reasonable to me especially as ServiceWorkerContext is in public. cc content OWNER kinuko@.
,
May 31 2017
Friendly nudge on this. I think as part of improving predictability of A2HS (issue 721881) we should pick this up. Is this something you have time for? If not, perhaps someone over here can look at it
,
May 31 2017
,
Jun 1 2017
Ping kinuko: do you have thoughts on the plan I proposed in c#2 (or suggestions for a more optimal way of listening for service worker registrations?) This will help us in installability with issues where sites lazily load a service worker some time after page load.
,
Jun 6 2017
falken/kinuko: I'd like to move ahead with moving things around here, so please let me know if you're (still) okay with the plan I outlined in c#2. :)
,
Jun 6 2017
Still seems fine to me to move ServiceWorkerContextObserver to content/public, (as a non-owner of content/public).
,
Jun 7 2017
After some more investigation, my tentative plan is as follows: 1. Rename ServiceWorkerContextObserver to ServiceWorkerContextCoreObserver 2. Add a new ServiceWorkerContextObserver to content/public 3. Make ServiceWorkerContextWrapper a ServiceWorkerContextCoreObserver. Fix up how it uses its observer_list_ (which is passed to ServiceWorkerContextCore every time it's restarted), i.e. move the DidDeleteAndStartOver notification to a method on ServiceWorkerContextCore 4. Add a new observer_list_ to ServiceWorkerContextWrapper which contains the content/public ServiceWorkerContextObserver. Introduce AddObserver/RemoveObserver methods to the ServiceWorkerContext public API 5. Override ServiceWorkerContextWrapper::OnRegistrationStored to send a Notify() to the public ServiceWorkerContextObserver objects, dropping implementation specific details like the registration id. This means that in future, any other currently-private but could be public observer methods could be proxied out in the same way. It means I don't need to totally refactor the existing observer (which has a number of subclasses), and it respects content's public / implementation divide.
,
Jun 9 2017
#9- yes that sounds reasonable to me too. Sorry for responding so slowly!
,
Jun 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/eadb0e90a6c4bc0e33c4436f8941b9534237b282 commit eadb0e90a6c4bc0e33c4436f8941b9534237b282 Author: dominickn <dominickn@chromium.org> Date: Tue Jun 13 03:23:37 2017 Rename ServiceWorkerContextObserver to ServiceWorkerContextCoreObserver. The existing observer is within content's implementation boundary, and actually observes a ServiceWorkerContextCore or a ServiceWorkerContextWrapper. Future CLs will make the observer only observe a ServiceWorkerContextCore to reflect its new name. This CL purely renames the ServiceWorkerContextObserver. This prepares for a ServiceWorkerContextObserver in content's public API (mirroring the public ServiceWorkerContext class). This new observer will allow classes in //chrome to observe events such as successful service worker registrations. BUG= 625051 Review-Url: https://codereview.chromium.org/2931033003 Cr-Commit-Position: refs/heads/master@{#478885} [modify] https://crrev.com/eadb0e90a6c4bc0e33c4436f8941b9534237b282/content/browser/BUILD.gn [modify] https://crrev.com/eadb0e90a6c4bc0e33c4436f8941b9534237b282/content/browser/background_sync/background_sync_manager.h [modify] https://crrev.com/eadb0e90a6c4bc0e33c4436f8941b9534237b282/content/browser/browsing_data/clear_site_data_throttle_browsertest.cc [modify] https://crrev.com/eadb0e90a6c4bc0e33c4436f8941b9534237b282/content/browser/devtools/protocol/service_worker_handler.cc [modify] https://crrev.com/eadb0e90a6c4bc0e33c4436f8941b9534237b282/content/browser/devtools/protocol/service_worker_handler.h [modify] https://crrev.com/eadb0e90a6c4bc0e33c4436f8941b9534237b282/content/browser/notifications/platform_notification_context_impl.h [modify] https://crrev.com/eadb0e90a6c4bc0e33c4436f8941b9534237b282/content/browser/notifications/platform_notification_context_unittest.cc [modify] https://crrev.com/eadb0e90a6c4bc0e33c4436f8941b9534237b282/content/browser/push_messaging/push_messaging_context.h [modify] https://crrev.com/eadb0e90a6c4bc0e33c4436f8941b9534237b282/content/browser/service_worker/service_worker_browsertest.cc [modify] https://crrev.com/eadb0e90a6c4bc0e33c4436f8941b9534237b282/content/browser/service_worker/service_worker_context_core.cc [modify] https://crrev.com/eadb0e90a6c4bc0e33c4436f8941b9534237b282/content/browser/service_worker/service_worker_context_core.h [rename] https://crrev.com/eadb0e90a6c4bc0e33c4436f8941b9534237b282/content/browser/service_worker/service_worker_context_core_observer.h [modify] https://crrev.com/eadb0e90a6c4bc0e33c4436f8941b9534237b282/content/browser/service_worker/service_worker_context_unittest.cc [modify] https://crrev.com/eadb0e90a6c4bc0e33c4436f8941b9534237b282/content/browser/service_worker/service_worker_context_watcher.cc [modify] https://crrev.com/eadb0e90a6c4bc0e33c4436f8941b9534237b282/content/browser/service_worker/service_worker_context_watcher.h [modify] https://crrev.com/eadb0e90a6c4bc0e33c4436f8941b9534237b282/content/browser/service_worker/service_worker_context_wrapper.cc [modify] https://crrev.com/eadb0e90a6c4bc0e33c4436f8941b9534237b282/content/browser/service_worker/service_worker_context_wrapper.h [modify] https://crrev.com/eadb0e90a6c4bc0e33c4436f8941b9534237b282/content/browser/service_worker/service_worker_internals_ui.cc [modify] https://crrev.com/eadb0e90a6c4bc0e33c4436f8941b9534237b282/content/browser/service_worker/service_worker_internals_ui.h
,
Jun 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f86ff4e07ecc10e97d9c677b44b1a46d91d1caf3 commit f86ff4e07ecc10e97d9c677b44b1a46d91d1caf3 Author: dominickn <dominickn@chromium.org> Date: Tue Jun 13 04:05:25 2017 Move all calls on ServiceWorkerContextCoreObserver to ServiceWorkerContextCore. Currently, ServiceWorkerContextWrapper holds a base::ObserverList of ServiceWorkerContextCoreObservers, which are passed to the wrapped ServiceWorkerContextCore upon creation. However, the OnStorageWiped call is only made by the wrapping class, not the wrapped class. This CL moves the OnStorageWiped observer method to be triggered by the wrapped ServiceWorkerContextCore object. This means all observer methods are triggered by that object. BUG= 625051 Review-Url: https://codereview.chromium.org/2925423002 Cr-Commit-Position: refs/heads/master@{#478897} [modify] https://crrev.com/f86ff4e07ecc10e97d9c677b44b1a46d91d1caf3/content/browser/service_worker/service_worker_context_core.cc [modify] https://crrev.com/f86ff4e07ecc10e97d9c677b44b1a46d91d1caf3/content/browser/service_worker/service_worker_context_core.h [modify] https://crrev.com/f86ff4e07ecc10e97d9c677b44b1a46d91d1caf3/content/browser/service_worker/service_worker_context_wrapper.cc [modify] https://crrev.com/f86ff4e07ecc10e97d9c677b44b1a46d91d1caf3/content/browser/service_worker/service_worker_context_wrapper.h
,
Jun 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5bce7f41747fd8e3918ae354977e655487237e4c commit 5bce7f41747fd8e3918ae354977e655487237e4c Author: dominickn <dominickn@chromium.org> Date: Thu Jun 15 01:27:15 2017 Add a ServiceWorkerContextObserver to content's public API. This CL introduces a ServiceWorkerContextObserver which may be used by classes outside of content. The observer is implemented by proxying the internal ServiceWorkerContextCoreObserver method calls via ServiceWorkerContextWrapper. This CL also exposes ServiceWorkerUtils::ScopeMatches on the public ServiceWorkerContext interface in preparation for crrev.com/2942513002, where the method is used to check if a manifest URL lies within the scope of a service worker. BUG= 625051 Review-Url: https://codereview.chromium.org/2934163003 Cr-Commit-Position: refs/heads/master@{#479568} [modify] https://crrev.com/5bce7f41747fd8e3918ae354977e655487237e4c/content/browser/service_worker/service_worker_context_wrapper.cc [modify] https://crrev.com/5bce7f41747fd8e3918ae354977e655487237e4c/content/browser/service_worker/service_worker_context_wrapper.h [modify] https://crrev.com/5bce7f41747fd8e3918ae354977e655487237e4c/content/public/browser/BUILD.gn [modify] https://crrev.com/5bce7f41747fd8e3918ae354977e655487237e4c/content/public/browser/service_worker_context.h [add] https://crrev.com/5bce7f41747fd8e3918ae354977e655487237e4c/content/public/browser/service_worker_context_observer.h
,
Jun 15 2017
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by falken@chromium.org
, Jul 4 2016