Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Description • 10 years ago

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

	Andrea Marchesini [:baku] Assignee
	Comment 1 • 10 years ago

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.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 2 • 10 years ago

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?

	Andrea Marchesini [:baku] Assignee
	Comment 3 • 10 years ago

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.

	Andrea Marchesini [:baku] Assignee
	Comment 4 • 10 years ago

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

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 6 • 10 years ago

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.

	Andrea Marchesini [:baku] Assignee
	Comment 7 • 10 years ago

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

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 8 • 10 years ago

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|?

	Andrea Marchesini [:baku] Assignee
	Comment 9 • 10 years ago

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.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 10 • 10 years ago

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.

	Andrea Marchesini [:baku] Assignee
	Comment 11 • 10 years ago

> 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.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 13 • 10 years ago

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().

	Andrea Marchesini [:baku] Assignee
	Comment 14 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=5315dbdd3397

https://hg.mozilla.org/integration/mozilla-inbound/rev/7266964a7e27

	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/7266964a7e27