This includes allowing Workers and SharedWorkers to access registrations via register(), getRegistrations() etc. and allowing the ServiceWorkerGlobalScope to access it's registration.

Does this include intercepting network requests generated from workers? Or is that a separate bug? Or does it already work?

That is 1032521.

This adds the WebIDL changes and introduces relevant C++ classes. The first step is to support update(), unregister() and the access to scope.

This patch exposes ServiceWorkerRegistration (and ServiceWorker to satisfy constraints) to workers. For now, a null registration is returned in the worker. Please ignore that I'm removing the visibility prefs, I'll add a patch that reintroduces them. It was just easier to develop with them removed.

ServiceWorkerGlobalScope::Registration() now returns initialized registration.

Am I using PromiseWorkerProxy correctly?

Splits up the listener interface to which SWM can hold rawptrs so that registration objects living on the worker thread can use awkward proxies to listen to these events.

Comment on attachment 8591119 [details] [diff] [review] Patch 1 - Introduce ServiceWorkerRegistration{Base,MainThread,WorkerThread} Review of attachment 8591119 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/ServiceWorkerManager.cpp @@ +2034,5 @@ > { > AssertIsOnMainThread(); > nsAutoCString scope = NS_ConvertUTF16toUTF8(aScope); > > // TODO: this is very very bad: I guess we can remove this comment. ::: dom/workers/ServiceWorkerRegistration.cpp @@ +33,3 @@ > NS_INTERFACE_MAP_END_INHERITING(DOMEventTargetHelper) > > +NS_IMPL_CYCLE_COLLECTION_INHERITED(ServiceWorkerRegistrationBase, DOMEventTargetHelper, mCCDummy); 80chars. @@ +38,1 @@ > const nsAString& aScope) indentation @@ +180,5 @@ > } > > +void > +ServiceWorkerRegistrationMainThread::InvalidateWorkerReference(WhichServiceWorker aWhichOnes) > +{ AssertIsOnMainTHread(); @@ +243,1 @@ > { AssertIsMainThread(); @@ +253,1 @@ > { AssertIsMainThread(); @@ +322,5 @@ > +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(ServiceWorkerRegistrationWorkerThread) > +NS_INTERFACE_MAP_END_INHERITING(ServiceWorkerRegistrationBase) > + > +NS_IMPL_CYCLE_COLLECTION_INHERITED(ServiceWorkerRegistrationWorkerThread, ServiceWorkerRegistrationBase, > + mCCDummy); In theory mCCDummy is CCed by the base class. Remove it. ::: dom/workers/ServiceWorkerRegistration.h @@ +59,5 @@ > + // DOMEventTargethelper > + virtual void DisconnectFromOwner() override; > + > +protected: > + ~ServiceWorkerRegistrationBase(); virtual @@ +78,5 @@ > +class ServiceWorkerRegistrationMainThread final : public ServiceWorkerRegistrationBase > +{ > +public: > + NS_DECL_ISUPPORTS_INHERITED > + NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(ServiceWorkerRegistrationMainThread, ServiceWorkerRegistrationBase) 80chars max? @@ +126,5 @@ > +class ServiceWorkerRegistrationWorkerThread final : public ServiceWorkerRegistrationBase > +{ > +public: > + NS_DECL_ISUPPORTS_INHERITED > + NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(ServiceWorkerRegistrationWorkerThread, ServiceWorkerRegistrationBase) 80 chars @@ +136,5 @@ > + > + void > + Update() > + { > + MOZ_CRASH("FIXME"); next patches I guess. @@ +165,5 @@ > +private: > + ~ServiceWorkerRegistrationWorkerThread() > + {} > + > + nsCOMPtr<nsISupports> mCCDummy; This already exist in the base class.

Comment on attachment 8591120 [details] [diff] [review] Patch 2 - Expose to workers Review of attachment 8591120 [details] [diff] [review]: ----------------------------------------------------------------- I guess you forgot to uncomment something... or it's in another patch. ::: dom/webidl/ServiceWorkerRegistration.webidl @@ -7,5 @@ > * http://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html > * > */ > > -[Pref="dom.serviceWorkers.enabled", No pref anymore? ::: dom/workers/WorkerScope.cpp @@ +480,5 @@ > + //if (!mRegistration) { > + // mRegistration = new ServiceWorkerRegistrationWorkerThread(this); > + //} > + > + return mRegistration; This will crash. because you are returning a nullptr without an ErrorResult. ::: dom/workers/test/serviceworkers/test_serviceworker_interfaces.js @@ +157,5 @@ > // IMPORTANT: Do not change this list without review from a DOM peer! > "ServiceWorker", > // IMPORTANT: Do not change this list without review from a DOM peer! > + "ServiceWorkerRegistration", > +// IMPORTANT: Do not change this list without review from a DOM peer! alphabetic order. It goes under ServiceWorkerGlobalScope.

Comment on attachment 8591129 [details] [diff] [review] Patch 3 - move event listeners to main thread class Review of attachment 8591129 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/ServiceWorkerRegistration.cpp @@ +107,5 @@ > +// registered. > +void > +ServiceWorkerRegistrationMainThread::StartListeningForEvents() > +{ > + AssertIsOnMainThread(); MOZ_ASSERT(!mListeningForEvents); ::: dom/workers/ServiceWorkerRegistration.h @@ +59,2 @@ > protected: > + ~ServiceWorkerRegistrationBase() virtual

Comment on attachment 8591133 [details] [diff] [review] Patch 4 - Implement ServiceWorkerRegistration.update() on worker Review of attachment 8591133 [details] [diff] [review]: ----------------------------------------------------------------- r+ also to the other patch. ::: dom/workers/ServiceWorkerRegistration.cpp @@ +390,5 @@ > + MOZ_ASSERT(worker); > + worker->AssertIsOnWorkerThread(); > +#endif > + nsCOMPtr<nsIRunnable> r = new UpdateRunnable(mScope); > + MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_DispatchToMainThread(r))); I spoke with smaug about this pattern. He told me that in theory, this is wrong because DispatchToMainThread could fail. What about if we start doing: if (NS_FAILED(NS_DispatchToMainThread(r))) { NS_WARNING("Failed to dispatch ... "); } ::: dom/workers/WorkerScope.cpp @@ +478,5 @@ > ServiceWorkerGlobalScope::Registration() > { > + if (!mRegistration) { > + mRegistration = new ServiceWorkerRegistrationWorkerThread(mScope); > + } here we are :)

Comment on attachment 8591137 [details] [diff] [review] Patch 5 - Implement ServiceWorkerRegistration.unregister() on worker Review of attachment 8591137 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/ServiceWorkerRegistration.cpp @@ +556,5 @@ > + return nullptr; > + } > + > + nsRefPtr<StartUnregisterRunnable> r = new StartUnregisterRunnable(worker, promise, mScope); > + MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_DispatchToMainThread(r))); see the previous comment.

Comment on attachment 8591140 [details] [diff] [review] Patch 6 - SWM holds ServiceWorkerRegistrationListener for updatefound and invalidation notifications Review of attachment 8591140 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/ServiceWorkerManager.cpp @@ +705,1 @@ > mRegistration); If I was bent I would indent this like this: nsCOMPtr<nsIRunnable> upr = NS_NewRunnableMethodWithArg<ServiceWorkerRegistrationInfo*>( swm, &ServiceWorkerManager::FireUpdateFoundOnServiceWorkerRegistrations, mRegistration); ::: dom/workers/ServiceWorkerRegistration.h @@ +187,5 @@ > > nsCOMPtr<nsISupports> mCCDummy; > }; > > +class ServiceWorkerRegistrationProxy final What's this?

Comment on attachment 8591143 [details] [diff] [review] Patch 7 - Fire updatefound on worker registration Review of attachment 8591143 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/ServiceWorkerRegistration.cpp @@ +555,5 @@ > + void > + InvalidateWorkers(WhichServiceWorker aWhichOnes) override > + { > + AssertIsOnMainThread(); > + // FIXME(nsm); next patches? @@ +763,5 @@ > +ServiceWorkerRegistrationWorkerThread::ReleaseListener(Reason aReason) > +{ > + // Can't assert worker thread here since the worker may have gone away. > + MOZ_ASSERT(!NS_IsMainThread()); > + if (mListener) { if (!mListener) { return; } @@ +769,5 @@ > + > + if (aReason == RegistrationIsGoingAway) { > + nsRefPtr<AsyncStopListeningRunnable> r = > + new AsyncStopListeningRunnable(mListener); > + MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_DispatchToMainThread(r))); same comment as the previous patches. ::: dom/workers/ServiceWorkerRegistration.h @@ +186,5 @@ > > + bool > + Notify(JSContext* aCx, workers::Status aStatus) override > + { > + ReleaseListener(WorkerIsGoingAway); Do we want to have a boolean to do not call ReleaseListener several times? This Notify() receives 4 different calls... ::: dom/workers/test/serviceworkers/test_workerupdatefoundevent.html @@ +4,5 @@ > +--> > +<!DOCTYPE HTML> > +<html> > +<head> > + <title>Bug XXXXXXX - Test ServiceWorkerRegistration.onupdatefound on ServiceWorker</title> Correct bug ID ::: dom/workers/test/serviceworkers/updatefoundevent.html @@ +1,1 @@ > +<!DOCTYPE html> same here. ::: dom/workers/test/serviceworkers/worker_updatefoundevent.js @@ +1,1 @@ > +onactivate = function(e) { We should add a header with the license.

mListener is fully thread-safe, correct?

Comment on attachment 8591120 [details] [diff] [review] Patch 2 - Expose to workers Review of attachment 8591120 [details] [diff] [review]: ----------------------------------------------------------------- r+ based on comment 13

(In reply to Andrea Marchesini (:baku) from comment #16) > Comment on attachment 8591143 [details] [diff] [review] > Patch 7 - Fire updatefound on worker registration > > Review of attachment 8591143 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/workers/ServiceWorkerRegistration.cpp > @@ +555,5 @@ > > + void > > + InvalidateWorkers(WhichServiceWorker aWhichOnes) override > > + { > > + AssertIsOnMainThread(); > > + // FIXME(nsm); > > next patches? > This requires ServiceWorker to be exposed to workers, which I'm going to do in another patch. So for now I'm going to have {.installing,.waiting,.active} return null in this patch and follow up. > > ::: dom/workers/ServiceWorkerRegistration.h > @@ +186,5 @@ > > > > + bool > > + Notify(JSContext* aCx, workers::Status aStatus) override > > + { > > + ReleaseListener(WorkerIsGoingAway); > > Do we want to have a boolean to do not call ReleaseListener several times? > This Notify() receives 4 different calls... mListener is the boolean.

https://hg.mozilla.org/integration/mozilla-inbound/rev/01ebdcc64821 https://hg.mozilla.org/integration/mozilla-inbound/rev/6d4972508d3b https://hg.mozilla.org/integration/mozilla-inbound/rev/09c5b97b9186 https://hg.mozilla.org/integration/mozilla-inbound/rev/3fe905b66249 https://hg.mozilla.org/integration/mozilla-inbound/rev/3c2774add913 https://hg.mozilla.org/integration/mozilla-inbound/rev/cc0295e4472a https://hg.mozilla.org/integration/mozilla-inbound/rev/5e31bd95ab8c https://hg.mozilla.org/integration/mozilla-inbound/rev/40e66684151c https://hg.mozilla.org/integration/mozilla-inbound/rev/b24b2eb809c3 https://hg.mozilla.org/integration/mozilla-inbound/rev/fc091f913859

Comment on attachment 8591143 [details] [diff] [review] Patch 7 - Fire updatefound on worker registration >diff --git a/dom/workers/ServiceWorkerRegistration.cpp b/dom/workers/ServiceWorkerRegistration.cpp >--- a/dom/workers/ServiceWorkerRegistration.cpp >+++ b/dom/workers/ServiceWorkerRegistration.cpp >+class WorkerListener final : public ServiceWorkerRegistrationListener >+{ [...] >+public: >+ NS_INLINE_DECL_THREADSAFE_REFCOUNTING(WorkerListener) This was missing an 'override' annotation, which causes clang 3.6 & newer to spam a "-Winconsistent-missing-override" build warning, which busts --enable-warnings-as-errors builds (with clang 3.6+). (This is for AddRef/Release overriding decls for these methods in ServiceWorkerRegistrationListener.) I pushed a followup to add that keyword, with blanket r+ that ehsan granted me for fixes of this sort over in bug 1126447 comment 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/5676a6c15258

https://hg.mozilla.org/mozilla-central/rev/01ebdcc64821 https://hg.mozilla.org/mozilla-central/rev/6d4972508d3b https://hg.mozilla.org/mozilla-central/rev/09c5b97b9186 https://hg.mozilla.org/mozilla-central/rev/3fe905b66249 https://hg.mozilla.org/mozilla-central/rev/3c2774add913 https://hg.mozilla.org/mozilla-central/rev/cc0295e4472a https://hg.mozilla.org/mozilla-central/rev/5e31bd95ab8c https://hg.mozilla.org/mozilla-central/rev/40e66684151c https://hg.mozilla.org/mozilla-central/rev/b24b2eb809c3 https://hg.mozilla.org/mozilla-central/rev/fc091f913859 https://hg.mozilla.org/mozilla-central/rev/5676a6c15258

I've updated relevant ref pages to mention that this functionality is available in workers: https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerRegistration https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerRegistration/scope https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerRegistration/installing https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerRegistration/waiting https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerRegistration/active https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerRegistration/update https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerRegistration/unregister https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerRegistration/onupdatefound https://developer.mozilla.org/en-US/Firefox/Releases/40#Web_Workers https://developer.mozilla.org/en-US/docs/Web/API/Worker/Functions_and_classes_available_to_workers#APIs_available_in_workers A quick tech review would be great. Thanks