Closed
Bug 1131327
Opened 9 years ago
Closed 9 years ago
Expose ServiceWorkerRegistration on Workers
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: nsm, Assigned: nsm)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete)
Attachments
(10 files)
25.65 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
8.58 KB,
patch
|
nsm
:
review+
|
Details | Diff | Splinter Review |
7.43 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
9.09 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
13.76 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
Patch 6 - SWM holds ServiceWorkerRegistrationListener for updatefound and invalidation notifications
17.27 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
17.42 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
1.43 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
2.77 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
1.86 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
This includes allowing Workers and SharedWorkers to access registrations via register(), getRegistrations() etc. and allowing the ServiceWorkerGlobalScope to access it's registration.
Assignee | ||
Updated•9 years ago
|
Blocks: ServiceWorkers-B2G
Comment 1•9 years ago
|
||
Does this include intercepting network requests generated from workers? Or is that a separate bug? Or does it already work?
Flags: needinfo?(nsm.nikhil)
Assignee | ||
Comment 3•9 years ago
|
||
This adds the WebIDL changes and introduces relevant C++ classes. The first step is to support update(), unregister() and the access to scope.
Attachment #8591119 -
Flags: review?(amarchesini)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → nsm.nikhil
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•9 years ago
|
||
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.
Attachment #8591120 -
Flags: review?(amarchesini)
Assignee | ||
Updated•9 years ago
|
Attachment #8591119 -
Attachment is obsolete: true
Attachment #8591119 -
Flags: review?(amarchesini)
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8591129 -
Flags: review?(amarchesini)
Assignee | ||
Updated•9 years ago
|
Attachment #8591120 -
Attachment is obsolete: true
Attachment #8591120 -
Flags: review?(amarchesini)
Assignee | ||
Comment 6•9 years ago
|
||
ServiceWorkerGlobalScope::Registration() now returns initialized registration.
Attachment #8591133 -
Flags: review?(amarchesini)
Assignee | ||
Updated•9 years ago
|
Attachment #8591129 -
Attachment is obsolete: true
Attachment #8591129 -
Flags: review?(amarchesini)
Assignee | ||
Comment 7•9 years ago
|
||
Am I using PromiseWorkerProxy correctly?
Attachment #8591137 -
Flags: review?(catalin.badea392)
Assignee | ||
Updated•9 years ago
|
Attachment #8591133 -
Attachment is obsolete: true
Attachment #8591133 -
Flags: review?(amarchesini)
Assignee | ||
Comment 8•9 years ago
|
||
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.
Attachment #8591140 -
Flags: review?(amarchesini)
Assignee | ||
Updated•9 years ago
|
Attachment #8591137 -
Attachment is obsolete: true
Attachment #8591137 -
Flags: review?(catalin.badea392)
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #8591143 -
Flags: review?(amarchesini)
Assignee | ||
Updated•9 years ago
|
Attachment #8591140 -
Attachment is obsolete: true
Attachment #8591140 -
Flags: review?(amarchesini)
Assignee | ||
Updated•9 years ago
|
Attachment #8591119 -
Attachment is obsolete: false
Attachment #8591119 -
Flags: review?(amarchesini)
Assignee | ||
Updated•9 years ago
|
Attachment #8591120 -
Attachment is obsolete: false
Attachment #8591120 -
Flags: review?(amarchesini)
Assignee | ||
Updated•9 years ago
|
Attachment #8591129 -
Attachment is obsolete: false
Attachment #8591129 -
Flags: review?(amarchesini)
Assignee | ||
Updated•9 years ago
|
Attachment #8591133 -
Attachment is obsolete: false
Attachment #8591133 -
Flags: review?(amarchesini)
Assignee | ||
Updated•9 years ago
|
Attachment #8591137 -
Attachment is obsolete: false
Attachment #8591137 -
Flags: review?(amarchesini)
Assignee | ||
Updated•9 years ago
|
Attachment #8591140 -
Attachment is obsolete: false
Attachment #8591140 -
Flags: review?(amarchesini)
Comment 10•9 years ago
|
||
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.
Attachment #8591119 -
Flags: review?(amarchesini) → review+
Comment 11•9 years ago
|
||
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.
Attachment #8591120 -
Flags: review?(amarchesini) → review-
Comment 12•9 years ago
|
||
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
Attachment #8591129 -
Flags: review?(amarchesini) → review+
Comment 13•9 years ago
|
||
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 :)
Attachment #8591133 -
Flags: review?(amarchesini) → review+
Comment 14•9 years ago
|
||
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.
Attachment #8591137 -
Flags: review?(amarchesini) → review+
Comment 15•9 years ago
|
||
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?
Attachment #8591140 -
Flags: review?(amarchesini) → review+
Comment 16•9 years ago
|
||
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.
Attachment #8591143 -
Flags: review?(amarchesini) → review+
Comment 17•9 years ago
|
||
mListener is fully thread-safe, correct?
Assignee | ||
Comment 18•9 years ago
|
||
Comment on attachment 8591120 [details] [diff] [review] Patch 2 - Expose to workers Review of attachment 8591120 [details] [diff] [review]: ----------------------------------------------------------------- r+ based on comment 13
Attachment #8591120 -
Flags: review- → review+
Assignee | ||
Comment 19•9 years ago
|
||
(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.
Assignee | ||
Comment 20•9 years ago
|
||
Attachment #8592526 -
Flags: review?(amarchesini)
Assignee | ||
Comment 21•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=79051357d533
Updated•9 years ago
|
Attachment #8592526 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 22•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d10640e791b7
Assignee | ||
Comment 23•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b8928516ca5
Assignee | ||
Comment 24•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9abc4936b8a5
Assignee | ||
Comment 25•9 years ago
|
||
Attachment #8594263 -
Flags: review?(amarchesini)
Assignee | ||
Comment 26•9 years ago
|
||
Attachment #8594264 -
Flags: review?(amarchesini)
Assignee | ||
Comment 27•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7594f19460f
Updated•9 years ago
|
Attachment #8594264 -
Flags: review?(amarchesini) → review+
Updated•9 years ago
|
Attachment #8594263 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 28•9 years ago
|
||
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 29•9 years ago
|
||
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
Comment 30•9 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•9 years ago
|
Keywords: dev-doc-needed
Comment 31•9 years ago
|
||
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
Keywords: dev-doc-needed → dev-doc-complete
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•