Comment on attachment 8456568 [details] [diff] [review] WIP: ServiceWorkerClients and Client interfaces with getServiced(). Nikhil, could you please take a look at the patch and give me some early feedback?

Comment on attachment 8456568 [details] [diff] [review] WIP: ServiceWorkerClients and Client interfaces with getServiced(). Review of attachment 8456568 [details] [diff] [review]: ----------------------------------------------------------------- Excellent! ::: dom/bindings/Bindings.conf @@ +230,5 @@ > }, > > +'Client': { > + 'nativeType': 'mozilla::dom::workers::Client', > + 'headerFile': 'mozilla/dom/workers/bindings/Client.h', If this is only available on ServiceWorkerGlobalScope, you should add |workers: True| and use the '_workers' form of namespace so that it isn't exposed on the main thread. @@ +1069,5 @@ > > +'ServiceWorkerClients': { > + 'nativeType': 'mozilla::dom::workers::ServiceWorkerClients', > + 'headerFile': 'mozilla/dom/workers/bindings/ServiceWorkerClients.h', > +}, same for this. ::: dom/workers/Client.h @@ +20,5 @@ > + > +namespace workers { > + > +class Client MOZ_FINAL : public nsISupports, > + public nsWrapperCache { Nit: { on newline. @@ +23,5 @@ > +class Client MOZ_FINAL : public nsISupports, > + public nsWrapperCache { > +public: > + NS_DECL_CYCLE_COLLECTING_ISUPPORTS > + NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(Client) This doesn't hold an jsvals, so don't need to be SCRIPT_HOLDER. ::: dom/workers/ServiceWorkerClients.cpp @@ +62,5 @@ > + } > + mPromise->MaybeResolve(ret); > + > + // release the reference on the worker thread. > + nsRefPtr<Promise> kungFu = mPromise.forget(); This isn't a kungFuDeathGrip :) I'd call it something different. @@ +75,5 @@ > + nsCString mDomain; > + nsCString mScope; > +public: > + explicit GetServicedRunnable(WorkerPrivate* aWorkerPrivate, > + nsRefPtr<Promise> aPromise, Promise* aPromise? @@ +83,5 @@ > + mPromise(aPromise), > + mDomain(aDomain), > + mScope(aScope) > + { > + } I'm assuming you intend to do AddFeature/RemoveFeature before landing this. ::: dom/workers/ServiceWorkerClients.h @@ +25,5 @@ > +class ServiceWorkerClients MOZ_FINAL : public nsISupports, > + public nsWrapperCache { > +public: > + NS_DECL_CYCLE_COLLECTING_ISUPPORTS > + NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(ServiceWorkerClients) Same here about SCRIPT_HOLDER. ::: dom/workers/ServiceWorkerManager.cpp @@ +1245,5 @@ > +ServiceWorkerManager::GetServicedClients(const nsCString& aDomain, > + const nsCString& aScope, nsTArray<uint64_t>* controlledDocuments) > +{ > + nsRefPtr<ServiceWorkerDomainInfo> domainInfo; > + mDomainMap.Get(aDomain, getter_AddRefs(domainInfo)); You can now use GetDomainInfo(). Although not strictly required here, let's keep accesses to mDomainMap in one location. @@ +1247,5 @@ > +{ > + nsRefPtr<ServiceWorkerDomainInfo> domainInfo; > + mDomainMap.Get(aDomain, getter_AddRefs(domainInfo)); > + > + nsRefPtr<ServiceWorkerRegistration> registration = domainInfo->GetRegistration(aScope); Add an assertion that this can never be null.

Close the worker right after dispatching GetServiced to test that the promise is released gracefully.

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #3) I made all the changes you requested, except for the SCRIPT_HOLDER as we've discussed offline. Try run: https://tbpl.mozilla.org/?tree=Try&rev=7767ee6b112a Try run with getServiced closing the worker right after the first dispatch (test_kill_worker attachment): https://tbpl.mozilla.org/?tree=Try&rev=dd74942841d8

Comment on attachment 8459855 [details] [diff] [review] ServiceWorkers: ServiceWorkerClients and Client interfaces with getServiced() Review of attachment 8459855 [details] [diff] [review]: ----------------------------------------------------------------- Kyle, please have a look at this patch - the PromiseHolder / WorkerFeature part in particular.

Comment on attachment 8459981 [details] [diff] [review] Patch 3 - Implement client.postMessage and add tests for getServiced(). Review of attachment 8459981 [details] [diff] [review]: ----------------------------------------------------------------- Please also add a test that enumerates the array of clients and postmessages to all of them and ensure that messages are received. r=me with the below changes. ::: dom/webidl/Client.webidl @@ +14,1 @@ > void postMessage(any message, DOMString targetOrigin, https://github.com/slightlyoff/ServiceWorker/issues/190 ? Please remove targetorigin. ::: dom/workers/Client.cpp @@ +91,5 @@ > + if (!window) { > + return NS_ERROR_FAILURE; > + } > + > + AutoSafeJSContext cx; Please use AutoJSAPI. @@ +99,5 @@ > + return NS_OK; > + } > +}; > + > +void Client::PostMessage(JSContext* aCx, JS::Handle<JS::Value> aMessage, Do get a once over from khuey/bent (feedback?) on this method. It seems like you distilled much of the worker messaging stuff, into what is required only for worker thread to main thread messaging. Good! ::: dom/workers/test/serviceworkers/message_posting_worker.js @@ +2,5 @@ > + self.clients.getServiced().then(function(res) { > + if (!res.length) { > + dump("ERROR: no clients are currently controlled.\n"); > + } > + res[0].postMessage(e.data, "*"); Indentation. ::: dom/workers/test/serviceworkers/test_get_serviced_advanced.html @@ +15,5 @@ > +<pre id="test"></pre> > +<script class="testbody" type="text/javascript"> > + > + function start() { > + opened = []; Just define this outside of the function. Do use var. @@ +17,5 @@ > + > + function start() { > + opened = []; > + return navigator.serviceWorker.register("get_serviced_worker_advanced.js").then(function(e) { > + worker = e; Same two comments here as in test_post_message.html. ::: dom/workers/test/serviceworkers/test_post_message.html @@ +16,5 @@ > +<script class="testbody" type="text/javascript"> > + > + function start() { > + magic_value = "MAGIC_VALUE_123"; > + return navigator.serviceWorker.register("message_posting_worker.js").then(function(e) { Please pass a scope localized to the dom/workers/test/serviceworkers directory at least :) Also unregister() the worker in runTest() below. Nit: Call the parameter sw instead of e. @@ +17,5 @@ > + > + function start() { > + magic_value = "MAGIC_VALUE_123"; > + return navigator.serviceWorker.register("message_posting_worker.js").then(function(e) { > + worker = e; Please declare worker in the global scope before using it here. @@ +48,5 @@ > + .then(function(e) { > + SimpleTest.finish(); > + }).catch(function(e) { > + ok(false, "Some test failed with error " + e); > + SimpleTest.finish(); You can avoid the duplicate SimpleTest.finish() by using .then(testPostMessage) .catch(function(e) { ok(false, ...); }) .then(SimpleTest.finish)

Comment on attachment 8459874 [details] [diff] [review] Patch 2 - Add postMessage to service workers. Review of attachment 8459874 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the [Throws] removed. ::: dom/webidl/ServiceWorker.webidl @@ +19,5 @@ > attribute EventHandler onstatechange; > + > + // FIXME(catalinb): Temporary fix needed for testing service workers. > + [Throws] > + void postMessage(any message, optional sequence<Transferable> transferable); The worker spec does not define postMessage as throwing. See my comment below. ::: dom/workers/ServiceWorker.cpp @@ +49,5 @@ > +ServiceWorker::PostMessage(JSContext* aCx, JS::Handle<JS::Value> aMessage, > + const Optional<Sequence<JS::Value>>& aTransferable, > + ErrorResult& aRv) > +{ > + WorkerPrivate* workerPrivate = GetWorkerPrivate(); Please add an assertion for workerPrivate. @@ +50,5 @@ > + const Optional<Sequence<JS::Value>>& aTransferable, > + ErrorResult& aRv) > +{ > + WorkerPrivate* workerPrivate = GetWorkerPrivate(); > + workerPrivate->PostMessage(aCx, aMessage, aTransferable, aRv); You can just create an |ErrorResult result;| on the stack here and pass it to PostMessage.

Comment on attachment 8459855 [details] [diff] [review] ServiceWorkers: ServiceWorkerClients and Client interfaces with getServiced() Review of attachment 8459855 [details] [diff] [review]: ----------------------------------------------------------------- Kyle/Ben should look at the Promise keep alive. You'll need dom peer review on the webidl. ::: dom/webidl/Client.webidl @@ +9,5 @@ > + */ > + > +interface Client { > + readonly attribute unsigned long id; > + void postMessage(any message, DOMString targetOrigin, remove targetorigin here too. ::: dom/workers/Client.h @@ +35,5 @@ > + } > + > + uint32_t Id() const > + { > + // XXXcatalinb the id exposed to JS should be mangled. In discussion with catalin, this comment is no longer relevant. ::: dom/workers/ServiceWorkerClients.cpp @@ +202,5 @@ > + > + return promise.forget(); > +} > + > +already_AddRefed<Promise> ServiceWorkerClients::ReloadAll() Please add a FIXME comment with Bug number to actually implement this. ::: dom/workers/ServiceWorkerManager.cpp @@ +1252,5 @@ > + const nsCString& aScope, nsTArray<uint64_t>* controlledDocuments) > +{ > + nsRefPtr<ServiceWorkerDomainInfo> domainInfo = GetDomainInfo(aScriptURL); > + > + nsRefPtr<ServiceWorkerRegistration> registration = domainInfo->GetRegistration(aScope); You can just use SWM::GetRegistration(aScope) since aScope is a full URL. That way you don't need the aScriptURL argument.

Comment on attachment 8459855 [details] [diff] [review] ServiceWorkers: ServiceWorkerClients and Client interfaces with getServiced() Review of attachment 8459855 [details] [diff] [review]: ----------------------------------------------------------------- It's only ok to call Clean() on the worker thread (since you're going to call RemoveWorkerFeature). Please add an assertion for that. Checking mClean on the main thread is unnecessary. If the worker has shut down already ResolveWorkerPromiseRunnable will fail to dispatch, and if it shuts down between dispatch and running that event the runnable will be canceled and discarded. Getting rid of that check also lets you get rid of the lock. I didn't look at this exhaustively. ::: dom/workers/ServiceWorkerClients.cpp @@ +60,5 @@ > + } > + > + Promise* Get() const > + { > + return mPromise.get(); You should be able to just return mPromise, I would think. @@ +107,5 @@ > +public: > + ResolvePromiseWorkerRunnable(WorkerPrivate* aWorkerPrivate, > + PromiseHolder* aPromiseHolder, > + nsAutoPtr<nsTArray<uint64_t> > aValue) > + : WorkerRunnable(aWorkerPrivate, WorkerThreadUnchangedBusyCount), This causes script to run, so it needs to be WorkerThreadModifyBusyCount.

Comment on attachment 8466486 [details] [diff] [review] Patch 1 - ServiceWorkers: ServiceWorkerClients and Client interfaces with getServiced() Review of attachment 8466486 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/Bindings.conf @@ +238,5 @@ > 'nativeType': 'mozilla::dom::workers::ChromeWorkerPrivate', > }, > > +'Client': { > + 'workers': True, This is not needed. @@ +1073,5 @@ > 'headerFile': 'mozilla/dom/ServiceWorkerContainer.h', > }, > > +'ServiceWorkerClients': { > + 'workers': True, Remove this. ::: dom/webidl/Client.webidl @@ +6,5 @@ > + * The origin of this IDL file is > + * http://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html > + * > + */ > + spec is changed. This is called ServiceWorkerClient. Use: [Exposed=ServiceWorker] to expose the object to worker. ::: dom/webidl/ServiceWorkerClients.webidl @@ +6,5 @@ > + * The origin of this IDL file is > + * http://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html > + * > + */ > + [Exposed=ServiceWorker] ::: dom/workers/Client.h @@ +6,5 @@ > + > +#ifndef mozilla_dom_workers_client_h > +#define mozilla_dom_workers_client_h > + > +#include "mozilla/dom/ClientBinding.h" Remove this. @@ +9,5 @@ > + > +#include "mozilla/dom/ClientBinding.h" > + > +#include "nsISupportsImpl.h" > +#include "nsIGlobalObject.h" do you need this? @@ +40,5 @@ > + } > + > + void PostMessage(JSContext* aCx, JS::Handle<JS::Value> aMessage, > + const Optional<Sequence<JS::Value>>& aTransferable) > + { This comes with the other patches, I guess. @@ +49,5 @@ > + return mOwner; > + } > + > + JSObject* WrapObject(JSContext* cx) MOZ_OVERRIDE > + { Move this to the .cpp file. @@ +58,5 @@ > + ~Client() > + { > + } > + > + nsISupports* mOwner; nsCOMPtr<nsISupports> mOwner; @@ +66,5 @@ > +} // namespace workers > +} // namespace dom > +} // namespace mozilla > + > +#endif // mozilla_dom_workers_client_h ::: dom/workers/ServiceWorkerClients.cpp @@ +7,5 @@ > +#include "ServiceWorkerManager.h" > +#include "WorkerScope.h" > +#include "Client.h" > + > +#include "nsDOMString.h" DO you need this? @@ +8,5 @@ > +#include "WorkerScope.h" > +#include "Client.h" > + > +#include "nsDOMString.h" > +#include "nsPIDOMWindow.h" this is not needed and probably RuntimeService.h can be removed as well. @@ +20,5 @@ > +using namespace mozilla::dom::workers; > + > +NS_IMPL_CYCLE_COLLECTING_ADDREF(ServiceWorkerClients) > +NS_IMPL_CYCLE_COLLECTING_RELEASE(ServiceWorkerClients) > +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(ServiceWorkerClients) NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(ServiceWorkerClients, mWorkerScope) @@ +35,5 @@ > +} > + > +// Helper class used for passing the promise between threads while > +// keeping the worker alive. > +class PromiseHolder : public WorkerFeature 1. MOZ_FINAL 2. put this class in an anonymous namespace. But most important, probably what you really want is just PromiseWorkerProxy. Take a look at ./dom/promise/PromiseWorkerProxy.h @@ +37,5 @@ > +// Helper class used for passing the promise between threads while > +// keeping the worker alive. > +class PromiseHolder : public WorkerFeature > +{ > + friend class GetServicedRunnable; Why this? @@ +42,5 @@ > + > + NS_INLINE_DECL_THREADSAFE_REFCOUNTING(PromiseHolder) > + > +public: > + explicit PromiseHolder(WorkerPrivate* aWorkerPrivate, explicit is not needed if you have 2 params. @@ +97,5 @@ > + > + bool mClean; > +}; > + > +class ResolvePromiseWorkerRunnable : public WorkerRunnable anonymous namespace + MOZ_FINAL @@ +104,5 @@ > + nsAutoPtr<nsTArray<uint64_t> > mValue; > +public: > + ResolvePromiseWorkerRunnable(WorkerPrivate* aWorkerPrivate, > + PromiseHolder* aPromiseHolder, > + nsAutoPtr<nsTArray<uint64_t> > aValue) >> @@ +116,5 @@ > + { > + MOZ_ASSERT(aWorkerPrivate); > + aWorkerPrivate->AssertIsOnWorkerThread(); > + > + nsTArray<nsRefPtr<Client> > ret; >> @@ +117,5 @@ > + MOZ_ASSERT(aWorkerPrivate); > + aWorkerPrivate->AssertIsOnWorkerThread(); > + > + nsTArray<nsRefPtr<Client> > ret; > + for (size_t i = 0; i < mValue.get()->Length(); i++) { is get() needed? @@ +162,5 @@ > + return NS_OK; > + } > +}; > + > +already_AddRefed<Promise> ServiceWorkerClients::GetServiced() I guess you should mark this method as [Throws] in the the WebIDL file. Then use the ErrorResult object to propagate errors. @@ +167,5 @@ > +{ > + WorkerPrivate* workerPrivate = GetCurrentThreadWorkerPrivate(); > + MOZ_ASSERT(workerPrivate); > + > + DOMString scope; nsString scope; @@ +175,5 @@ > + > + ErrorResult rv; > + nsRefPtr<Promise> promise = Promise::Create(global, rv); > + if (rv.Failed()) { > + return nullptr; Once you have the ErrorResult object, this will be: nsRefPtr<Promise> promise = Promise::Create(global, aRv); if (NS_WARN_IF(aRv.Failed())) { return nullptr; } @@ +179,5 @@ > + return nullptr; > + } > + > + nsRefPtr<GetServicedRunnable> r = > + new GetServicedRunnable(workerPrivate, promise, NS_ConvertUTF16toUTF8(scope.AsAString())); NS_ConvertUTF16toUTF8(scope) @@ +180,5 @@ > + } > + > + nsRefPtr<GetServicedRunnable> r = > + new GetServicedRunnable(workerPrivate, promise, NS_ConvertUTF16toUTF8(scope.AsAString())); > + rv = NS_DispatchToMainThread(r); nsresult rv = NS_DispatchToMainThread(r); if (NS_WARN_IF(NS_FAILED(rv)) { ... @@ +190,5 @@ > + return promise.forget(); > +} > + > +// FIXME(catalinb): Bug 1045257 - Implement ReloadAll > +already_AddRefed<Promise> ServiceWorkerClients::ReloadAll() Also this method should have a ErrorResult param. Add [Throws] in the WebIDL. ::: dom/workers/ServiceWorkerClients.h @@ +6,5 @@ > + > +#ifndef mozilla_dom_workers_serviceworkerclients_h > +#define mozilla_dom_workers_serviceworkerclients_h > + > +#include "mozilla/dom/ServiceWorkerClientsBinding.h" remove this @@ +9,5 @@ > + > +#include "mozilla/dom/ServiceWorkerClientsBinding.h" > + > +#include "nsISupportsImpl.h" > +#include "nsIGlobalObject.h" do you need this? @@ +22,5 @@ > + > +class ServiceWorkerGlobalScope; > + > +class ServiceWorkerClients MOZ_FINAL : public nsISupports, > + public nsWrapperCache { { in a new line. @@ +34,5 @@ > + already_AddRefed<Promise> ReloadAll(); > + > + JSObject* WrapObject(JSContext* cx) > + { > + return ServiceWorkerClientsBinding_workers::Wrap(cx, this); in the cpp file. @@ +36,5 @@ > + JSObject* WrapObject(JSContext* cx) > + { > + return ServiceWorkerClientsBinding_workers::Wrap(cx, this); > + } > + nsISupports* GetParentObject() const; new line before nsISupports. Change this in: ServiceWorkerGlobalScope* GetParentObject() const { return mWorkerScope; } @@ +43,5 @@ > + ~ServiceWorkerClients() > + { > + } > + > + ServiceWorkerGlobalScope* mWorkerScope; nsRefPtr ::: dom/workers/ServiceWorkerManager.cpp @@ +1410,5 @@ > > aScope = NS_ConvertUTF8toUTF16(r->mScope); > return NS_OK; > } > all of this goes into an anonymous namespace. ::: dom/workers/ServiceWorkerManager.h @@ +315,5 @@ > static already_AddRefed<ServiceWorkerManager> > GetInstance(); > > + NS_IMETHODIMP > + GetServicedClients(const nsCString& aScope, You don't need this. Just change nsIServiceWorkerManager.idl

I'll wait for a new version of patch 1 before reviewing patch 2 and 3. Is it ok?

(In reply to Andrea Marchesini (:baku) from comment #19) > I'll wait for a new version of patch 1 before reviewing patch 2 and 3. Is it > ok? Yes, I'll also update patches 2 and 3 based on your feedback for the first patch. I have another version for patch 1 (as per your comments), but I want to go through the code one more time before uploading. I have to work on my intern presentation right now. :D

Comment on attachment 8471249 [details] [diff] [review] Patch 1(v2) - ServiceWorkers: ServiceWorkerClients and Client interfaces with getServiced() I have addressed the issues you mentioned, with a few exceptions: (In reply to Andrea Marchesini (:baku) from comment #18) > @@ +35,5 @@ > > +} > > + > > +// Helper class used for passing the promise between threads while > > +// keeping the worker alive. > > +class PromiseHolder : public WorkerFeature > > But most important, probably what you really want is just > PromiseWorkerProxy. Take a look at ./dom/promise/PromiseWorkerProxy.h I've already looked at PromiseWorkerProxy. The reason I can't use it is because it requires creating a second promise on the main thread, which is not possible since there is no available global object on the main thread for sw. > @@ +167,5 @@ > > +{ > > + WorkerPrivate* workerPrivate = GetCurrentThreadWorkerPrivate(); > > + MOZ_ASSERT(workerPrivate); > > + > > + DOMString scope; > > nsString scope; The getter for the scope requires a DOMString and the compiler complains. > ::: dom/workers/ServiceWorkerManager.h > @@ +315,5 @@ > > static already_AddRefed<ServiceWorkerManager> > > GetInstance(); > > > > + NS_IMETHODIMP > > + GetServicedClients(const nsCString& aScope, > > You don't need this. Just change nsIServiceWorkerManager.idl I removed the NS_IMETHODIMP return type since it wasn't really being used. I've kept the declaration in the .h for the following reasons: 1. The method is only used from c++ code and I'm using the singleton instance getter for ServiceWorkerManager which returns the exact SWM type. 2. Converting the arguments to IDL types will add unnecessary conversions (or maybe I missing something): there is no IDL equivalent for nsTArray<uint64_t>. Also, in ServiceWorkerClients, because the reference to the parent object is now a strong reference, I had to add cycle collecting info for ServiceWorkerGlobalScope.

Comment on attachment 8471249 [details] [diff] [review] Patch 1(v2) - ServiceWorkers: ServiceWorkerClients and Client interfaces with getServiced() Review of attachment 8471249 [details] [diff] [review]: ----------------------------------------------------------------- Good. Can I see it again with this comments fixed? Thanks. ::: dom/webidl/ServiceWorkerClient.webidl @@ +9,5 @@ > + */ > + > +[Exposed=ServiceWorker] > +interface ServiceWorkerClient { > + readonly attribute unsigned long id; no postMessage? I guess it's in another patch. ::: dom/workers/ServiceWorkerClient.cpp @@ +20,5 @@ > + NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY > + NS_INTERFACE_MAP_ENTRY(nsISupports) > +NS_INTERFACE_MAP_END > + > +JSObject* ServiceWorkerClient::WrapObject(JSContext* cx) JSObject* Ser... aCx) ::: dom/workers/ServiceWorkerClient.h @@ +40,5 @@ > + { > + return mOwner; > + } > + > + JSObject* WrapObject(JSContext* cx) MOZ_OVERRIDE; aCx ::: dom/workers/ServiceWorkerClients.cpp @@ +13,5 @@ > + > +#include "mozilla/dom/Promise.h" > +#include "mozilla/dom/ServiceWorkerClientsBinding.h" > + > +using namespace mozilla; I guess this is just for ErrorResult, so just do: using mozilla::ErrorResult; @@ +33,5 @@ > + MOZ_ASSERT(mWorkerScope); > + SetIsDOMBinding(); > +} > + > +JSObject* ServiceWorkerClients::WrapObject(JSContext* cx) JSObject* ServiceWorkerClients::WrapObject(JSContext* aCx) @@ +111,5 @@ > + > +public: > + ResolvePromiseWorkerRunnable(WorkerPrivate* aWorkerPrivate, > + PromiseHolder* aPromiseHolder, > + nsAutoPtr<nsTArray<uint64_t>> aValue) nsAutoPtr<...>& @@ +172,5 @@ > +}; > + > +} // anonymous namespace > + > +already_AddRefed<Promise> ServiceWorkerClients::GetServiced(ErrorResult& aRv) returnType Class::Method() everywhere in any CPP file. @@ +175,5 @@ > + > +already_AddRefed<Promise> ServiceWorkerClients::GetServiced(ErrorResult& aRv) > +{ > + WorkerPrivate* workerPrivate = GetCurrentThreadWorkerPrivate(); > + MOZ_ASSERT(workerPrivate); Add mWorkerPrivate->AssertIsOnWorkerThread(); @@ +184,5 @@ > + nsRefPtr<Promise> promise = Promise::Create(mWorkerScope, aRv); > + if (NS_WARN_IF(aRv.Failed())) { > + return nullptr; > + } > + you want to add the feature at this point because the worker can be CC/GCed before the runnable is executed. Create the PromiseHolder obj here. ::: dom/workers/ServiceWorkerClients.h @@ +33,5 @@ > + > + already_AddRefed<Promise> GetServiced(ErrorResult& aRv); > + already_AddRefed<Promise> ReloadAll(ErrorResult& aRv); > + > + JSObject* WrapObject(JSContext* cx) MOZ_OVERRIDE; aCx ::: dom/workers/ServiceWorkerManager.cpp @@ +1433,5 @@ > + ServiceWorkerRegistration* aRegistration, > + void* aData) > +{ > + FilterRegistrationData* data = static_cast<FilterRegistrationData*>(aData); > + if (data->mRegistration == aRegistration) { if (data->mRegistration != aRegistration) { return PL_DHASH_NEXT; } @@ +1435,5 @@ > +{ > + FilterRegistrationData* data = static_cast<FilterRegistrationData*>(aData); > + if (data->mRegistration == aRegistration) { > + nsCOMPtr<nsIDocument> document = do_QueryInterface(aKey); > + if (!document->GetInnerWindow()) { document can be null. @@ +1447,5 @@ > + > +} // anonymous namespace > + > +void > +ServiceWorkerManager::GetServicedClients(const nsCString& aScope, nsTArray<uint64_t>* aControlledDocuments) indent in a new line the second param. ::: dom/workers/WorkerScope.h @@ +197,5 @@ > { > // FIXME(nsm): Bug 982728 > } > > + already_AddRefed<ServiceWorkerClients> ServiceWorkerClients* @@ +201,5 @@ > + already_AddRefed<ServiceWorkerClients> > + Clients() { > + if (!mClients) { > + mClients = new ServiceWorkerClients(this); > + MOZ_ASSERT(mClients); remove this assert. "new" doesn't fail. @@ +203,5 @@ > + if (!mClients) { > + mClients = new ServiceWorkerClients(this); > + MOZ_ASSERT(mClients); > + } > + return mClients;

Comment on attachment 8471259 [details] [diff] [review] Patch 2(v2) - Add postMessage to service workers. Review of attachment 8471259 [details] [diff] [review]: ----------------------------------------------------------------- I think this patch should change ServiceWorkerClient and not ServiceWorker. This is what the spec says. ::: dom/webidl/ServiceWorker.webidl @@ +21,5 @@ > readonly attribute ServiceWorkerState state; > attribute EventHandler onstatechange; > + > + [Throws] > + void postMessage(any message, optional sequence<Transferable> transferable); This goes in ServiceWorkerClient... right?

Comment on attachment 8471261 [details] [diff] [review] Patch 3(v2) - Implement client.postMessage and add tests for getServiced(). Review of attachment 8471261 [details] [diff] [review]: ----------------------------------------------------------------- I would like to see a test for: 1. postMessage with DOMFile/DOMBlob, and maybe add also normal object, booleans, strings, arrays. 2. postMessage with transferable objects. ArrayBuffers are transferable and check that, after the postMessage, the arrayBuffer is empty from 1 side, and fully populated on the other side. A separated patch for this is ok. Smaug, can you take a look at how the MessageEvent is dispatched? ::: dom/workers/ServiceWorkerClient.cpp @@ +34,5 @@ > } > > +namespace { > + > +class ServiceWorkerPostMessageRunnable MOZ_FINAL : public nsRunnable ServiceWorkerClientPostMessageRunnable? @@ +38,5 @@ > +class ServiceWorkerPostMessageRunnable MOZ_FINAL : public nsRunnable > +{ > + uint64_t mId; > + JSAutoStructuredCloneBuffer mBuffer; > + nsTArray<nsCOMPtr<nsISupports> > mClonedObjects; >> @@ +43,5 @@ > + > +public: > + ServiceWorkerPostMessageRunnable(uint64_t aId, > + JSAutoStructuredCloneBuffer&& aData, > + nsTArray<nsCOMPtr<nsISupports> >& aClonedObjects) >> @@ +49,5 @@ > + mBuffer(Move(aData)) > + { > + mClonedObjects.SwapElements(aClonedObjects); > + } > + DispatchDOMEvent should be private. @@ +51,5 @@ > + mClonedObjects.SwapElements(aClonedObjects); > + } > + > + bool DispatchDOMEvent(JSContext* aCx, nsGlobalWindow* aTargetWindow) > + { AssertIsOnMainThread(); @@ +59,5 @@ > + clonedObjects.SwapElements(mClonedObjects); > + > + JS::Rooted<JS::Value> messageData(aCx); > + if (!mBuffer.read(aCx, &messageData, > + WorkerStructuredCloneCallbacks(false))) { why false? You want to dispatch events to the main-thread so this must be true. Add a mochitest where you pass a DOMBlob object. You should test also the transferable obj list. @@ +77,5 @@ > + if (NS_FAILED(rv)) { > + xpc::Throw(aCx, rv); > + return false; > + } > + I think you can just do this: event->SetTrusted(true); bool status = false; aTargetWindow->DispatchEvent(event, &status); if (!status) { ... I'll ask smaug a feedback about this. @@ +112,5 @@ > +}; > + > +} // anonymous namespace > + > +void ServiceWorkerClient::PostMessage(JSContext* aCx, JS::Handle<JS::Value> aMessage, void ServiceWorkerClient:...

Comment on attachment 8471261 [details] [diff] [review] Patch 3(v2) - Implement client.postMessage and add tests for getServiced(). >+ nsRefPtr<MessageEvent> event = new MessageEvent(aTargetWindow, nullptr, nullptr); >+ nsresult rv = >+ event->InitMessageEvent(NS_LITERAL_STRING("message"), >+ false /* non-bubbling */, >+ true /* cancelable */, Is this really supposed to be cancelable? Yet you don't do anything if preventDefault() is actually called >+ event->SetTrusted(true); >+ WidgetEvent* internalEvent = event->GetInternalNSEvent(); >+ >+ nsEventStatus status = nsEventStatus_eIgnore; >+ EventDispatcher::Dispatch(static_cast<nsPIDOMWindow*>(aTargetWindow), >+ presContext, >+ internalEvent, >+ static_cast<dom::Event*>(event.get()), >+ &status); Just do bool dummy; aTargetWindow->DispatchEvent(event, &dummy);

(In reply to Andrea Marchesini (:baku) from comment #27) > Comment on attachment 8471259 [details] [diff] [review] > Patch 2(v2) - Add postMessage to service workers. > > Review of attachment 8471259 [details] [diff] [review]: > ----------------------------------------------------------------- > > I think this patch should change ServiceWorkerClient and not ServiceWorker. > This is what the spec says. > > ::: dom/webidl/ServiceWorker.webidl > @@ +21,5 @@ > > readonly attribute ServiceWorkerState state; > > attribute EventHandler onstatechange; > > + > > + [Throws] > > + void postMessage(any message, optional sequence<Transferable> transferable); > > This goes in ServiceWorkerClient... right? No. The spec specifies that ServiceWorker inherits from Worker which has the postMessage method. With our current implementation, SW inherits from EventTarget because the inheritance was an open issue [1]. I need serviceWorker.postMessage for running the tests, so I decided to add the definition to the SW interface. Now that the issue has been decided, we should switch to inheriting from Worker, I guess. Either way, the implementation should be the same, only the webidl that's exposing postMessage will change. I will open a separate bug for it, but do you think we can land this and fix the webidl after? [1] https://github.com/slightlyoff/ServiceWorker/issues/189

Comment on attachment 8471259 [details] [diff] [review] Patch 2(v2) - Add postMessage to service workers. Review of attachment 8471259 [details] [diff] [review]: ----------------------------------------------------------------- Ok. Thanks for your comment. File a bug please. r=me ::: dom/webidl/ServiceWorker.webidl @@ +21,5 @@ > readonly attribute ServiceWorkerState state; > attribute EventHandler onstatechange; > + > + [Throws] > + void postMessage(any message, optional sequence<Transferable> transferable); File a follow up and write here the bug ID.

Comment on attachment 8473539 [details] [diff] [review] Patch 1(v3) - Service Worker: ServiceWorkerClients and Client interfaces with getServiced() Updated with the requested changes, with some minor comments: (In reply to Andrea Marchesini (:baku) from comment #26) > ::: dom/workers/ServiceWorkerClients.cpp > @@ +13,5 @@ > > + > > +#include "mozilla/dom/Promise.h" > > +#include "mozilla/dom/ServiceWorkerClientsBinding.h" > > + > > +using namespace mozilla; > > I guess this is just for ErrorResult, so just do: > > using mozilla::ErrorResult; It's also needed for the js (AutoJSContext) context used in dispatching the runnable back to the worker thread. > @@ +184,5 @@ > > + nsRefPtr<Promise> promise = Promise::Create(mWorkerScope, aRv); > > + if (NS_WARN_IF(aRv.Failed())) { > > + return nullptr; > > + } > > + > > you want to add the feature at this point because the worker can be CC/GCed > before the runnable is executed. > Create the PromiseHolder obj here. Well, the PromiseHolder is created in the constructor of GetServicedRunnable, which is called right after the if statement. So, moving the PromiseHolder out won't make any difference. I personally prefer the current code, because it's similar to PromiseWorkerProxy.

Comment on attachment 8473545 [details] [diff] [review] Patch 2(v3) - Add postMessage to service workers. Added comment for follow-up.

Comment on attachment 8473547 [details] [diff] [review] Patch 3(v3) - Implement client.postMessage and add tests for getServiced(). Made all the requested changes. (In reply to Olli Pettay [:smaug] from comment #29) > Comment on attachment 8471261 [details] [diff] [review] > Patch 3(v2) - Implement client.postMessage and add tests for getServiced(). > > >+ nsRefPtr<MessageEvent> event = new MessageEvent(aTargetWindow, nullptr, nullptr); > >+ nsresult rv = > >+ event->InitMessageEvent(NS_LITERAL_STRING("message"), > >+ false /* non-bubbling */, > >+ true /* cancelable */, > Is this really supposed to be cancelable? Yet you don't do anything if > preventDefault() is actually called I checked the spec, and it shouldn't be cancelable - just changed the flag.

Comment on attachment 8473548 [details] [diff] [review] Patch 4 - Extensive testing of postMessage. WIP. I only wrote a test for basic types + blob and file.

Comment on attachment 8473547 [details] [diff] [review] Patch 3(v3) - Implement client.postMessage and add tests for getServiced(). Review of attachment 8473547 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/ServiceWorkerClient.cpp @@ +114,5 @@ > +} // anonymous namespace > + > +void > +ServiceWorkerClient::PostMessage(JSContext* aCx, JS::Handle<JS::Value> aMessage, > + const Optional<Sequence<JS::Value>>& aTransferable, indentation ::: dom/workers/test/serviceworkers/get_serviced_worker_enumerate.js @@ +1,4 @@ > +onmessage = function() { > + self.clients.getServiced().then(function(result) { > + dump("result: " + result.length + "\n\n"); > + dump("catalinn\n\n"); mm... remove these messages. ::: dom/workers/test/serviceworkers/message_posting_worker.js @@ +1,4 @@ > +onmessage = function(e) { > + self.clients.getServiced().then(function(res) { > + if (!res.length) { > + dump("ERROR: no clients are currently controlled.\n"); We should report this error properly.

Handle AddFeature failure gracefully. Add call to getServiced() in onclose. More thread assertions.

Changes to test. I'm not happy with not using ready. Maybe we should wait for that.

Comment on attachment 8478030 [details] [diff] [review] Patch 1 - interdiff from v3 Review of attachment 8478030 [details] [diff] [review]: ----------------------------------------------------------------- lgtm ::: dom/workers/ServiceWorkerManager.cpp @@ +1981,5 @@ > class MOZ_STACK_CLASS FilterRegistrationData > { > public: > FilterRegistrationData(nsTArray<uint64_t>* aDocuments, > + ServiceWorkerRegistrationInfo* aRegistration) indentation

Comment on attachment 8478118 [details] [diff] [review] interdiff from Patch 3(v3) Review of attachment 8478118 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/test/serviceworkers/test_get_serviced_advanced.html @@ +46,5 @@ > } > }); > + var w; > + setTimeout(function() { > + w = window.open("sw_clients/service_worker_controlled.html"); this should be an iframe but it's ok for this patch.

The earlier patch passed a nsTArray where the WorkerStructuredCloneCallbacks expect a WorkerStructuredCloneClosure, obviously leading to all sorts of bad things.

Comment on attachment 8478643 [details] interdiff from Patch 4(v3) Review of attachment 8478643 [details]: ----------------------------------------------------------------- ::: dom/workers/ServiceWorkerClient.cpp @@ +54,5 @@ > + bool rv = mBuffer.write(aCx, aValue, aTransferredValue, aCallbacks, > + &mClosure); > + // This hashtable has to be empty because it could contain MessagePort > + // objects that cannot be freed on a different thread. > + mClosure.mTransferredPorts.Clear(); This is not required in this runnable, will get rid of it.

Comment on attachment 8478643 [details] interdiff from Patch 4(v3) WorkerStructuredCloneClosure is from MessagePort :(

(In reply to Andrea Marchesini (:baku) from comment #18) > Comment on attachment 8466486 [details] [diff] [review] > Patch 1 - ServiceWorkers: ServiceWorkerClients and Client interfaces with > getServiced() > > Review of attachment 8466486 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/bindings/Bindings.conf > @@ +238,5 @@ > > 'nativeType': 'mozilla::dom::workers::ChromeWorkerPrivate', > > }, > > > > +'Client': { > > + 'workers': True, > > This is not needed. > > @@ +1073,5 @@ > > 'headerFile': 'mozilla/dom/ServiceWorkerContainer.h', > > }, > > > > +'ServiceWorkerClients': { > > + 'workers': True, > > Remove this. > > ::: dom/webidl/Client.webidl > @@ +6,5 @@ > > + * The origin of this IDL file is > > + * http://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html > > + * > > + */ > > + > > spec is changed. This is called ServiceWorkerClient. > > Use: [Exposed=ServiceWorker] to expose the object to worker. Andrea, it seems Exposed=ServiceWorker doesn't yet do what we want it to do. test_interfaces.html keeps complaining. Even if I try just Exposed=Worker. I think we don't want to generate main thread bindings at all for now. What this bug needs is two interfaces ServiceWorkerClient(s) that are solely exposed on ServiceWorkerGlobalScope. AFAIK there is no way to accomplish that right now except using bindings.conf. Boris, is that right?

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #53) > https://tbpl.mozilla.org/?tree=Try&rev=2a757b8cae02 Nikhil, thank you for taking over. Unfortunately, I won't be able to continue the work on SW until the beginning of September - I'm on a tight schedule with a school project. I'll ping you once I'm done with it. Assigning the bug to you in the meantime.

> AFAIK there is no way to accomplish that right now except using bindings.conf. Other way around. You can't restrict to only ServiceWorkerGlobalScope (but not other worker scopes) using Bindings.conf, since Bindings.conf has no concept of different worker scopes. What you want to do for now, until bug 1048699 is fixed, is something like [Exposed=Workers,Func=something] that will enforce visibility only in service workers. Perhaps restoring the ServiceWorkerGlobalScope::Visible function that attachment 8464540 [details] [diff] [review] removed? Or better yet, just fix bug 1048699. ;)

https://hg.mozilla.org/integration/mozilla-inbound/rev/9244a16c3258 https://hg.mozilla.org/integration/mozilla-inbound/rev/05acfcc7a341 https://hg.mozilla.org/integration/mozilla-inbound/rev/095ca68e5d38 https://hg.mozilla.org/integration/mozilla-inbound/rev/547076d4a357

I had to back these out for apparently breaking mochitest-4 on various platforms: https://hg.mozilla.org/integration/mozilla-inbound/rev/d323ede35f89 https://tbpl.mozilla.org/php/getParsedLog.php?id=47265533&tree=Mozilla-Inbound

It's very likely the failures were intermittents as both are known bugs. Andrea landed a fix for Bug 1060517. This patch is completely unrelated to the failures. Trying this again.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2771520aa1b9 https://hg.mozilla.org/integration/mozilla-inbound/rev/ebe7add5dd11 https://hg.mozilla.org/integration/mozilla-inbound/rev/9d397edb8e9a https://hg.mozilla.org/integration/mozilla-inbound/rev/9674f68df2e5

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #61) > It's very likely the failures were intermittents as both are known bugs. > Andrea landed a fix for Bug 1060517. This patch is completely unrelated to > the failures. Trying this again. More like perma-fails hitting consistently across all platforms. And guess what, it still is. Backed out again. Please verify that this is green on Try before burning the tree a third time. https://hg.mozilla.org/integration/mozilla-inbound/rev/185126a2a4e9 https://tbpl.mozilla.org/php/getParsedLog.php?id=47362920&tree=Mozilla-Inbound

It seems like the test_scopes.html test for the toplevel scope leads to some ordering issue that causes failures. I've done a new try push with that specific test disabled (not the entire test_scopes.html). If this goes green, I'll land these patches. It is quite possible that the ordering changes will be fixed by bug 1066822. In either case I'll file a followup to enable that test when landing this.

https://hg.mozilla.org/projects/maple/rev/20bbe32b1e3e https://hg.mozilla.org/projects/maple/rev/2462a26c50ff https://hg.mozilla.org/projects/maple/rev/2ad50f15b2d8 https://hg.mozilla.org/projects/maple/rev/79327fd9e5a4

Pushed to maple for now. Disabling that test did not work, seems like something more insidious. I'm going to work on bug 1066822 and see if that helps.

This should help fix the b2g build bustages that resulted from the patches in comment 76. https://hg.mozilla.org/projects/maple/rev/a040c47e5db9

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d5f35227e58e Besides the build failing on Windows, it looks fine.

Rebased and fixed windows build bustage.

I've rebased the patch set and fixed the build from breaking on windows. Patch 1.2 is ehsan's fix from maple: https://hg.mozilla.org/projects/maple/rev/a040c47e5db9. patch 1.1, 1.2: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a9b20d24e95b patch 2: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=69bee59e97ef whole patch set: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ceec68e1820a

Comment on attachment 8512141 [details] [diff] [review] Part 1.3: Fix crash caused by a race in PromiseHolder. I think we can try to land 1.1, 1.2 and 1.3 Here are the try runs: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=55a9b2deec3e https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b570fa5d25a2

Comment on attachment 8512141 [details] [diff] [review] Part 1.3: Fix crash caused by a race in PromiseHolder. Review of attachment 8512141 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Please land all relevant patches on m-c, and this fix on maple. ::: dom/promise/Promise.cpp @@ +1375,5 @@ > { > MOZ_ASSERT(NS_IsMainThread()); > > MutexAutoLock lock(mCleanUpLock); > + This file doesn't need to change. ::: dom/workers/ServiceWorkerClients.cpp @@ +53,5 @@ > PromiseHolder(WorkerPrivate* aWorkerPrivate, > Promise* aPromise) > : mWorkerPrivate(aWorkerPrivate), > mPromise(aPromise), > + mCleanUpLock("cleanUpLock"), name this "PromiseHolder cleanUpLock" or similar. @@ +219,5 @@ > } > > + nsRefPtr<PromiseHolder> promiseHolder = new PromiseHolder(workerPrivate, > + promise); > + if (!promiseHolder->Get()) { Please rename Get() to GetPromise().

Please land patches 1.1, 1.2 and 1.3. The patches have all been previously r+-ed and contain only small changes or have been rebased.

https://hg.mozilla.org/integration/mozilla-inbound/rev/062c0a4db943 https://hg.mozilla.org/integration/mozilla-inbound/rev/26cd857611b8 https://hg.mozilla.org/integration/mozilla-inbound/rev/0ae1b3474b22

sorry had to back this out since i guess this caused https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3350702&repo=mozilla-inbound

(In reply to Carsten Book [:Tomcat] from comment #95) > sorry had to back this out since i guess this caused > https://treeherder.mozilla.org/ui/logviewer. > html#?job_id=3350702&repo=mozilla-inbound Yes, this was causing the failures. I want to give it another go. I've updated patch 1.3 to remove a deprecated feature from the test which I think was the reason for the assert. Try results (I've queued 2 full runs because the assert happens intermittently): https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2674a2ae79a0 https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ab05c20ba1d9 Please land 1.1, 1.2 and 1.3 again.

Actually, lets wait on this. I believe there is still a race condition and its just less likely to occur.

Comment on attachment 8513793 [details] [diff] [review] Patch 1.4 - Fix second crash scenario caused by a race in PromiseHolder. Review of attachment 8513793 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me. Andrea, could you take a look at this patch and at patch 1.3. ::: dom/workers/ServiceWorkerClients.cpp @@ +232,5 @@ > + // Use a control runnable to release the runnable on the worker thread. > + nsRefPtr<ReleasePromiseRunnable> releaseRunnable = > + new ReleasePromiseRunnable(mWorkerPrivate, mPromiseHolder); > + > + if (!releaseRunnable->Dispatch(nullptr)) { Use the same cx you used above. @@ +233,5 @@ > + nsRefPtr<ReleasePromiseRunnable> releaseRunnable = > + new ReleasePromiseRunnable(mWorkerPrivate, mPromiseHolder); > + > + if (!releaseRunnable->Dispatch(nullptr)) { > + NS_RUNTIMEABORT("Failed to dispatch control runnable."); "PromiseHolder control runnable" or "ReleasePromiseRunnable"

Comment on attachment 8516119 [details] [diff] [review] Patch 1.5 - Update getServiced test and fix an assert when the registration has been removed. Review of attachment 8516119 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/test/serviceworkers/test_get_serviced.html @@ +23,3 @@ > function simpleRegister() { > + return navigator.serviceWorker.register("get_serviced_worker.js", > + { scope: "./sw_clients/" }).then((swr) => registration = swr); Nit: Could you move the .then() to a new line.

Comment on attachment 8514656 [details] [diff] [review] Patch 1.4 - Fix second crash scenario caused by a race in PromiseHolder. Review of attachment 8514656 [details] [diff] [review]: ----------------------------------------------------------------- Looks ok but I want to ask a quick feedback to bent when he is online. I'll write a comment in a few hours. ::: dom/workers/ServiceWorkerClients.cpp @@ +222,5 @@ > swm->GetServicedClients(mScope, result); > nsRefPtr<ResolvePromiseWorkerRunnable> r = > new ResolvePromiseWorkerRunnable(mWorkerPrivate, mPromiseHolder, result); > > AutoSafeJSContext cx; We should use AutoJSAPI instead. But it's not an issue for this bug.

Comment on attachment 8514656 [details] [diff] [review] Patch 1.4 - Fix second crash scenario caused by a race in PromiseHolder. Review of attachment 8514656 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/ServiceWorkerClients.cpp @@ +222,5 @@ > swm->GetServicedClients(mScope, result); > nsRefPtr<ResolvePromiseWorkerRunnable> r = > new ResolvePromiseWorkerRunnable(mWorkerPrivate, mPromiseHolder, result); > > AutoSafeJSContext cx; We should use AutoJSAPI instead. But it's not an issue for this bug.

Please land patches 1.1 through 1.5.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed211a407dde https://hg.mozilla.org/integration/mozilla-inbound/rev/736c29dc81ce https://hg.mozilla.org/integration/mozilla-inbound/rev/7fabdf3714a5 https://hg.mozilla.org/integration/mozilla-inbound/rev/5f914e69229c https://hg.mozilla.org/integration/mozilla-inbound/rev/a1a0c1573df1

https://hg.mozilla.org/mozilla-central/rev/ed211a407dde https://hg.mozilla.org/mozilla-central/rev/736c29dc81ce https://hg.mozilla.org/mozilla-central/rev/7fabdf3714a5 https://hg.mozilla.org/mozilla-central/rev/5f914e69229c https://hg.mozilla.org/mozilla-central/rev/a1a0c1573df1

Reopening since the postMessage bits aren't landed yet.

Catalin, any updates on this? Any patches we could start on review would be good.

This includes the client.postMessage implementation from Patch 3 and Patch 4.

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #112) > Catalin, any updates on this? Any patches we could start on review would be > good. Tryrun for the two patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=342cf7afd5cc I will follow up with a patch that stubs the new API and then patches for individual features. getServiced/getAll/get is still missing tests on m-i, I will add those along with the updated implementation. Cheers!

This patch is a bit cumbersome to read. It does the following: 1. s/getServiced/getAll, s/ServiceWorkerClient(s)/Client(s) 2. add stubbed options parameter to getAll and return error for anything other than the default options.

Try run for Patch 2 and Patch 3 with correct tests for patch 3: https://treeherder.mozilla.org/#/jobs?repo=try&revision=37cae710ea5d

Comment on attachment 8559533 [details] [diff] [review] Patch 2 - Add postMessage to service worker. Review of attachment 8559533 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/ServiceWorker.h @@ +62,5 @@ > } > > +#ifdef PostMessage > +#undef PostMessage > +#endif WorkerPrivate.cpp seems to do this only on WIN_XP. Could you do that here too?

Comment on attachment 8560141 [details] [diff] [review] Patch 3 - Implement client.postMessage. Review of attachment 8560141 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/ServiceWorkerClient.h @@ +11,5 @@ > #include "nsWrapperCache.h" > > namespace mozilla { > + > +class ErrorResult; include ErrorResult @@ +17,5 @@ > namespace dom { > > class Promise; > +template<typename T> class Optional; > +template<typename T> class Sequence; Just import BindingDeclarations.h

Comment on attachment 8561588 [details] [diff] [review] Patch 2 - Add postMessage to service worker. The webidl change was reviewed by :amarchesini in a prevous patch.

Comment on attachment 8559533 [details] [diff] [review] Patch 2 - Add postMessage to service worker. Review of attachment 8559533 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/ServiceWorker.cpp @@ +77,5 @@ > +{ > + WorkerPrivate* workerPrivate = GetWorkerPrivate(); > + MOZ_ASSERT(workerPrivate); > + > + workerPrivate->PostMessage(aCx, aMessage, aTransferable, aRv); The spec says this one is should throw InvalidAccessError if state is redundant. Please attach a follow up patch.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b105df291221 https://hg.mozilla.org/integration/mozilla-inbound/rev/bec72ec0f5ad

Comment on attachment 8561588 [details] [diff] [review] Patch 2 - Add postMessage to service worker. Review of attachment 8561588 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/webidl/ServiceWorker.webidl @@ +18,5 @@ > readonly attribute ServiceWorkerState state; > > attribute EventHandler onstatechange; > + > + // FIXME(catalinb): Should inherit this from Worker. Do we have a bug for that? Write the bug id here.

https://hg.mozilla.org/mozilla-central/rev/b105df291221 https://hg.mozilla.org/mozilla-central/rev/bec72ec0f5ad

(In reply to Andrea Marchesini (:baku) from comment #131) > Comment on attachment 8561588 [details] [diff] [review] > Patch 2 - Add postMessage to service worker. > > Review of attachment 8561588 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/webidl/ServiceWorker.webidl > @@ +18,5 @@ > > readonly attribute ServiceWorkerState state; > > > > attribute EventHandler onstatechange; > > + > > + // FIXME(catalinb): Should inherit this from Worker. > > Do we have a bug for that? Write the bug id here. Bug 1131347

Comment on attachment 8561780 [details] [diff] [review] Patch 3.1 Throw InvalidStateError when ServiceWorkerState == Redundant. https://hg.mozilla.org/integration/mozilla-inbound/rev/408f0108ff53

https://hg.mozilla.org/mozilla-central/rev/38fe830e9bde https://hg.mozilla.org/mozilla-central/rev/6983a75e87c9