These will be proxies to the main thread equivalents and implemented after ServiceWorkerManager.

I need a feedback about this: Which scope should I use? At the moment I used the mScope from the ServiceWorkerGlobalScope but the spec says the prefix of the scope.

Comment on attachment 8479100 [details] [diff] [review] unregister.patch Review of attachment 8479100 [details] [diff] [review]: ----------------------------------------------------------------- What we discussed about PromiseWorkerProxy. Other than that, this looks good. As for scope, using mScope is the way to go. The spec language is confusing, but I think path prefix should be path - if scope is 'http://example.com/foo' then '/foo'. Of course this is irrelevant to our implementation since we are using absolute URL scopes everywhere. ::: dom/workers/ServiceWorkerManager.cpp @@ -1038,1 @@ > nsCOMPtr<nsIGlobalObject> sgo = GetEntryGlobal(); Does this resolve to a sane object since the stack is clean due to being called async?

In order to test this new feature I need to post messages from the ServiceWorker to the window. ServiceWorker::postMessage() method is implemented in bug 982726.

This doesn't contain a mochitest. It will be included in a separated patch.

Comment on attachment 8481232 [details] [diff] [review] unregister.patch Review of attachment 8481232 [details] [diff] [review]: ----------------------------------------------------------------- Dropping review until Andrea verifies that if the worker goes away, resolving the promise does not lead to a crash or other issues. Everything else looks good at a glance. ::: dom/interfaces/base/nsIServiceWorkerManager.idl @@ +5,5 @@ > > #include "domstubs.idl" > > interface nsIDocument; > +interface nsIGlobalObject; Nit: This seems to be unused.

Tested locally. We need bug 982726 to be landed in order to write a proper mochitest.

Comment on attachment 8486414 [details] [diff] [review] unregister.patch Review of attachment 8486414 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/interfaces/base/nsIServiceWorkerManager.idl @@ +31,5 @@ > nsISupports register(in DOMString aScope, in DOMString aScriptURI); > > /** > + * Unregister an existing ServiceWorker registration for `aScope`. > + * It keeps alive the aCallback until the operation is conclused. keeps aCallback alive until the operation is concluded. ::: dom/workers/ServiceWorkerManager.cpp @@ +244,5 @@ > // worker and queues up a runnable to resolve the update promise once the > // worker has successfully been parsed. > class ServiceWorkerUpdateInstance MOZ_FINAL : public nsISupports > { > + nsRefPtr<ServiceWorkerRegistrationInfo> mRegistration; Not part of this patch. ::: dom/workers/ServiceWorkerRegistration.cpp @@ +145,4 @@ > already_AddRefed<Promise> > ServiceWorkerRegistration::Unregister(ErrorResult& aRv) > { > + nsCOMPtr<nsIGlobalObject> go = GetEntryGlobal(); Just replace all this entry global stuff with GetOwner() now that these checks have moved to registration. ::: dom/workers/WorkerScope.cpp @@ +13,3 @@ > #include "mozilla/dom/FunctionBinding.h" > +#include "mozilla/dom/Promise.h" > +#include "mozilla/dom/PromiseWorkerProxy.h" This header is not needed any more. @@ +409,5 @@ > + State mState; > + bool mValue; > +}; > + > +class UnregisterRunnable MOZ_FINAL : public WorkerMainThreadRunnable We don't need to block the worker while Unregister() is called on the main thread. So I think we can get away with just using a normal nsRunnable here. @@ +411,5 @@ > +}; > + > +class UnregisterRunnable MOZ_FINAL : public WorkerMainThreadRunnable > + , public nsIServiceWorkerUnregisterCallback > + , public WorkerFeature While I'm uncomfortable having a runnable be a WorkerFeature and a callback, it doesn't seem unsafe. Please run it by khuey/bent once though to make sure there aren't lifetime edge cases with this sort of setup. Could you add a comment to the top that the lifetime of this runnable is actually controlled by SWM::Unregister() and UnregisterResultRunnable, so that it is acceptable to use it as a WorkerFeature. @@ +415,5 @@ > + , public WorkerFeature > +{ > + nsRefPtr<Promise> mWorkerPromise; > + nsString mScope; > + bool mCleanUp; Nit: Call this mCleanedUp. @@ +450,5 @@ > + NS_IMETHODIMP > + UnregisterSucceeded(bool aState) MOZ_OVERRIDE > + { > + AssertIsOnMainThread(); > + This code path can be skipped if mCleanedUp is true. The dispatch will fail anyway, but perhaps it is good to save the allocation when we have the relevant info. @@ +462,5 @@ > + NS_IMETHODIMP > + UnregisterFailed() MOZ_OVERRIDE > + { > + AssertIsOnMainThread(); > + Ditto. @@ +472,5 @@ > + } > + > + void > + CleanUp(JSContext* aCx) > + { Nit: Worker thread assertion. @@ +547,5 @@ > + mWorkerPrivate->AssertIsOnWorkerThread(); > + MOZ_ASSERT(mWorkerPrivate->IsServiceWorker()); > + > + nsRefPtr<Promise> promise = > + Promise::Create(mWorkerPrivate->GlobalScope(), aRv); Just use |this|?

another quick glance? Actually the reason why the RegistrationInfo is nsRefPtr in the runnable is because the registration must be kept alive when the worker unregisters itself.

Comment on attachment 8486720 [details] [diff] [review] unregister.patch Review of attachment 8486720 [details] [diff] [review]: ----------------------------------------------------------------- You haven't incorporated the last 5 suggestions of the previous review. The RefPtr is fine.

> This code path can be skipped if mCleanedUp is true. The dispatch will fail > anyway, but perhaps it is good to save the allocation when we have the > relevant info. I would not do this because otherwise we need to protect mCleanedUp with a mutex. At the moment mCleanedUp is used only in the worker thread.

Comment on attachment 8486790 [details] [diff] [review] unregister.patch Review of attachment 8486790 [details] [diff] [review]: ----------------------------------------------------------------- Could you rename this bug to have only unregister() and file another bug for update().

https://tbpl.mozilla.org/?tree=Try&rev=5315dbdd3397 https://hg.mozilla.org/integration/mozilla-inbound/rev/7266964a7e27