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

This activates the ServiceWorker even if the same scope is already controlled by another active worker.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 1 • 9 years ago

Set this ni? request as we agree on IRC so Nikhil will provide some guidance about how to implement this.

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

From my reading of the spec, here is how this should be done. Note that implementation may introduce issues I didn't notice ;)

1) Add a mSkipWaitingFlag, initially false, to ServiceWorkerInfo. There should be a SetSkipWaitingFlag() that sets it to true, changing this value to false again is not allowed. This function and the flag should be main thread only.
2) In SWGlobalScope::SkipWaiting():
a) Dispatch a runnable to the main thread. This runnable will flip this flag for the worker's associated ServiceWorkerInfo. Catalin has added some infrastructure to allow you to find the right SWInfo in https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1130684&attachment=8592858
b) Just return a resolved promise.

3) You will need to modify register job ContinueAfterInstallEvent to implement steps 22, 24, 25.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 3 • 9 years ago

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #2)
> From my reading of the spec, here is how this should be done.

Thanks for the information.

Still WIP some parts already implemented.

I'll set the dependency on bug 1130684 per comment 2.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 4 • 9 years ago

Steps 1) and 2) implemented.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 5 • 9 years ago

Step 3 -more or less- implemented. Still WIP. Needs some tests.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 6 • 9 years ago

Rough implementation -with tests- of what you detailed at comment 2. I would need some feedback to polish this and move forward. Would you mind please? Thanks!

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

Comment on attachment 8598115 [details] [diff] [review]
v1

Review of attachment 8598115 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there.

If it isn't too much effort, I prefer reviewboard reviews + hg evolve :) But don't waste too much time on setup. This is fine too.

::: dom/workers/ServiceWorkerManager.cpp
@@ +856,5 @@
>      swm->InvalidateServiceWorkerRegistrationWorker(mRegistration,
>                                                     WhichServiceWorker::INSTALLING_WORKER | WhichServiceWorker::WAITING_WORKER);
>  
>      // FIXME(nsm): Bug 982711 Deal with activateImmediately.
>      NS_WARN_IF_FALSE(!aActivateImmediately, "Immediate activation using replace() is not supported yet");

Remove this.

@@ +869,5 @@
> +                                                           exitingWorker->CacheName());
> +        if (NS_FAILED(rv)) {
> +          NS_WARNING("Failed to purge the activating cache.");
> +        }
> +        swm->InvalidateServiceWorkerRegistrationWorker(mRegistration, WhichServiceWorker::ACTIVE_WORKER);

There is similar code in ::Activate, would you mind refactoring into a function PurgeActiveWorker() or something? Thanks!

@@ +3055,5 @@
>    return NS_OK;
>  }
>  
> +nsresult
> +ServiceWorkerManager::FlipSkipWaitingFlag(const nsCString& aScope,

const nsACString&

@@ +3063,5 @@
> +  if (!registration) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  nsRefPtr<ServiceWorkerInfo> newest = registration->Newest();

Won't we have to compare this to both installing and waiting (whichever is available). That may not be the newest.

::: dom/workers/ServiceWorkerManager.h
@@ +255,5 @@
>      MOZ_ASSERT(!aSpec.IsEmpty());
>      mScriptSpec = aSpec;
>    }
>  
> +  void SetSkipWaitingFlag(const bool aFlag)

No need for arg. Just SetSkipWaitingFlag()

@@ +268,1 @@
>    explicit ServiceWorkerInfo(ServiceWorkerRegistrationInfo* aReg,

Nit: while you are here, please remove this explicit, it is no longer necessary.

@@ +302,5 @@
> +  {
> +    AssertIsOnMainThread();
> +    return mSkipWaitingFlag;
> +  }
> +

Just for readability, please move this getter above and close to the setter.

@@ +435,5 @@
>    nsresult
>    ClaimClients(const nsCString& aScope, WorkerPrivate* aWorkerPrivate);
>  
> +  nsresult
> +  FlipSkipWaitingFlag(const nsCString& aScope, WorkerPrivate* aWorkerPrivate);

SetSkipWaitingFlag.

::: dom/workers/WorkerScope.cpp
@@ +485,5 @@
>    return mRegistration;
>  }
>  
> +namespace {
> + 

Nit: whitespace.

@@ +506,5 @@
> +  virtual bool
> +  WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) override;
> +
> +private:
> +  nsRefPtr<WorkerScopeSkipWaitingRunnable> mRunnable;

You want to pass around a PromiseWorkerProxy.

@@ +513,5 @@
> +
> +class WorkerScopeSkipWaitingRunnable final : public nsRunnable
> +{
> +  WorkerPrivate* mWorkerPrivate;
> +  // Worker thread only.

This comment doesn't work since nothing prevents this runnable from being destroyed on the main thread, leading to the promise being released on the main thread, where Release() will crash due to thread safety assertions. See PromiseWorkerProxy, which you may safely hold using a refptr.

@@ +548,5 @@
> +
> +    nsresult rv = swm->FlipSkipWaitingFlag(mScope, mWorkerPrivate);
> +
> +    nsRefPtr<SkipWaitingResultRunnable> runnable =
> +      new SkipWaitingResultRunnable(mWorkerPrivate, this, rv);

According to spec, the promise may never fail, so don't bother passing this error. Always resolve the promise.

@@ +582,5 @@
> +    return nullptr;
> +  }
> +
> +  nsRefPtr<WorkerScopeSkipWaitingRunnable> runnable =
> +    new WorkerScopeSkipWaitingRunnable(mWorkerPrivate,

Here and for future reference, it is UNSAFE to pass a worker private to the main thread (or any other thread apart from the worker thread itself) and hold it as a rawptr without significant other effort (look into WorkerFeature if you are interested). Since here you only care about ID of the worker, just pass the ID.

@@ +583,5 @@
> +  }
> +
> +  nsRefPtr<WorkerScopeSkipWaitingRunnable> runnable =
> +    new WorkerScopeSkipWaitingRunnable(mWorkerPrivate,
> +                                       promise,

You will have to use PromiseWorkerProxy since this promise lives on the worker but is being passed to the main thread.

@@ +585,5 @@
> +  nsRefPtr<WorkerScopeSkipWaitingRunnable> runnable =
> +    new WorkerScopeSkipWaitingRunnable(mWorkerPrivate,
> +                                       promise,
> +                                       NS_ConvertUTF16toUTF8(mScope));
> +  NS_DispatchToMainThread(runnable);

if (NS_WARN_IF(NS_FAILED(NS_Dispatch...))) {
  promise->MaybeReject(NS_ERROR_DOM_ABORT_ERR);
}

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 8 • 9 years ago

Addresses -more or less- the review comments made at comment 7. I'll request review once I set up the MozReview stuff.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 9 • 9 years ago

/r/8095 - Bug XXXXXXX: Enable some prefs for testing. r=me
/r/8097 - Bug 1130684 - Implement Service Worker clients.claim.
/r/8099 - Bug 1131352 - Add ServiceWorkerGlobalScope skipWaiting(). r=nsm

Pull down these commits:

hg pull -r 8fd5e00451f5ed4e54aa54624c8c29d942796962 https://reviewboard-hg.mozilla.org/gecko/

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 10 • 9 years ago

Comment on attachment 8601014 [details]
MozReview Request: bz://1131352/jaoo

/r/8095 - Bug XXXXXXX: Enable some prefs for testing. r=me
/r/8097 - Bug 1130684 - Implement Service Worker clients.claim.
/r/8099 - Bug 1131352 - Add ServiceWorkerGlobalScope skipWaiting(). r=nsm

Pull down these commits:

hg pull -r 8fd5e00451f5ed4e54aa54624c8c29d942796962 https://reviewboard-hg.mozilla.org/gecko/

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 11 • 9 years ago

Comment on attachment 8601014 [details]
MozReview Request: bz://1131352/jaoo

/r/8095 - Bug XXXXXXX: Enable some prefs for testing. r=me
/r/8097 - Bug 1130684 - Implement Service Worker clients.claim.
/r/8099 - Bug 1131352 - Add ServiceWorkerGlobalScope skipWaiting(). r=nsm

Pull down these commits:

hg pull -r 8fd5e00451f5ed4e54aa54624c8c29d942796962 https://reviewboard-hg.mozilla.org/gecko/

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 12 • 9 years ago

https://reviewboard.mozilla.org/r/8099/#review6985

::: dom/workers/ServiceWorkerManager.h:439
(Diff revision 1)
> +  SetSkipWaitingFlag(const nsCString& aScope, const uint64_t aServiceWorkerID);

I kept nsCString for keeping it as the other scope parameters at the other functions.

::: dom/workers/WorkerScope.cpp:557
(Diff revision 1)
> +      new SkipWaitingResultRunnable(workerPrivate, mPromiseProxy, rv);

You said to not deal with any error as the promise sould resolve always. Does it mean I should not use the SkipWaitingResultRunnable runnable?

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 13 • 9 years ago

Comment on attachment 8601014 [details]
MozReview Request: bz://1131352/jaoo

/r/8095 - Bug XXXXXXX: Enable some prefs for testing. r=me
/r/8097 - Bug 1130684 - Implement Service Worker clients.claim.
/r/8099 - Bug 1131352 - Add ServiceWorkerGlobalScope skipWaiting(). r=nsm

Pull down these commits:

hg pull -r 8fd5e00451f5ed4e54aa54624c8c29d942796962 https://reviewboard-hg.mozilla.org/gecko/

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

https://reviewboard.mozilla.org/r/8099/#review7025

This is really good. I'd like to see 2.2.1 implemented before i r+.

::: dom/workers/ServiceWorkerManager.h:262
(Diff revision 1)
> -  explicit ServiceWorkerInfo(ServiceWorkerRegistrationInfo* aReg,
> +  bool

Nit: In this file/struct we seem to have return type on the same line as function name.

::: dom/workers/ServiceWorkerManager.h:439
(Diff revision 1)
> +  SetSkipWaitingFlag(const nsCString& aScope, const uint64_t aServiceWorkerID);

Nit: const doesn't matter for the uint64_t since it is passed by value.

::: dom/workers/ServiceWorkerManager.cpp:1167
(Diff revision 1)
> +  nsRefPtr<ServiceWorkerInfo> exitingWorker = mActiveWorker;

exitingWorker = mActiveWorker.forget()
otherwise mActiveWorker will stay set.

::: dom/workers/ServiceWorkerManager.cpp:1186
(Diff revision 1)
>    nsRefPtr<ServiceWorkerInfo> exitingWorker = mActiveWorker;

Nit: exitingWorker is no longer needed here.

::: dom/workers/ServiceWorkerManager.cpp:1194
(Diff revision 1)
>    if (exitingWorker) {

Unconditionally call PurgeActiveWorker here since that checks.

::: dom/workers/ServiceWorkerManager.cpp:1189
(Diff revision 1)
>    swm->InvalidateServiceWorkerRegistrationWorker(this, WhichServiceWorker::WAITING_WORKER | WhichServiceWorker::ACTIVE_WORKER);

Only call with flag WAITING_WORKER since Purge() will call with Active. Also move this to after the call to PurgeActiveWorker.

::: dom/workers/WorkerScope.cpp:594
(Diff revision 1)
> +    return promise.forget();

Resolve the promise before aborting.

::: dom/workers/WorkerScope.cpp:559
(Diff revision 1)
> +    AutoSafeJSContext cx;

Use AutoJSAPI

AutoJSAPI jsapi;
jsapi.Init();
runnable->Dispatch(jsapi.cx())

::: dom/workers/WorkerScope.cpp:551
(Diff revision 1)
> +    WorkerPrivate* workerPrivate = mPromiseProxy->GetWorkerPrivate();

Since you are implementing custom behaviour rather than just adding the proxy as a promise handler, you'll need to grab the clean up lock from the proxy here, then while the lock is acquired, check if it already cleaned up. in that case abort. Otherwise you can safely GetWorkerPrivate()

::: dom/workers/ServiceWorkerManager.cpp:3091
(Diff revision 1)
> +

Step 2.2.1 seems to be missing where if there is a valid waiting worker and it's state is installed, we try to activate immediately.

::: dom/workers/WorkerScope.cpp:598
(Diff revision 1)
> +    new WorkerScopeSkipWaitingRunnable(mWorkerPrivate->ServiceWorkerID(),

Ah! Sorry, I didn't realize that once we pass a PromiseWorkerProxy, we can actually get a pointer to the workerprivate on the main thread (safely, see comment above). So you don't need to pass the ID around everywhere.

::: dom/workers/test/serviceworkers/test_skip_waiting.html:52
(Diff revision 1)
> +  function checkWhetherItSkipedWaiting(swr) {

Nit: skipped

::: dom/workers/test/serviceworkers/test_skip_waiting.html:80
(Diff revision 1)
> +      .then(checkWhetherItSkipedWaiting)

I think you need to have the message listener on the window ready to go before you call register() so we don't miss messages in some weird edge case. Why not move the checkWhetherItSkippedWaiting() code into register()? The registration=swr can be set in a then() handler to the actual n.s.register() call. Or you don't really have to bother since the scope is unique.

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

https://reviewboard.mozilla.org/r/8099/#review7027

> I kept nsCString for keeping it as the other scope parameters at the other functions.

ok

> You said to not deal with any error as the promise sould resolve always. Does it mean I should not use the SkipWaitingResultRunnable runnable?

you will still need the runnable, but you don't need to pass the nsresult to it.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 16 • 9 years ago

Comment on attachment 8601014 [details]
MozReview Request: bz://1131352/jaoo

/r/8095 - Bug 1130684 - Implement Service Worker clients.claim.
/r/8097 - Bug 1131352 - Add ServiceWorkerGlobalScope skipWaiting(). r=nsm

Pull down these commits:

hg pull -r 54f2eb8c8481948fe1bd12306345b8bc17f46069 https://reviewboard-hg.mozilla.org/gecko/

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 17 • 9 years ago

https://reviewboard.mozilla.org/r/8097/#review7065

::: dom/workers/ServiceWorkerManager.cpp:3087
(Diff revision 2)
> +    }

The if statement above implements 2.2.1. What use case is there for it? I don't see any use case for it.

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

https://reviewboard.mozilla.org/r/8097/#review7089

::: dom/workers/ServiceWorkerManager.cpp:3086
(Diff revision 2)
> +      registration->Activate();

Please call TryToActivate()

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

https://reviewboard.mozilla.org/r/8097/#review7091

> The if statement above implements 2.2.1. What use case is there for it? I don't see any use case for it.

you are right. just make it use TryToActivate().

This is required for when a waiting worker is just sitting around, but then something happens (say the current active worker postMessage's to it) and the waiting worker calls skipWaiting(). In that case we want to immediately have it replace the active worker.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 20 • 9 years ago

Comment on attachment 8601014 [details]
MozReview Request: bz://1131352/jaoo

/r/8095 - Bug 1130684 - Implement Service Worker clients.claim.
/r/8097 - Bug 1131352 - Add ServiceWorkerGlobalScope skipWaiting(). r=nsm

Pull down these commits:

hg pull -r dfec39f4ee82c900682cc6e800acd4ef4fc7740f https://reviewboard-hg.mozilla.org/gecko/

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 21 • 9 years ago

Comment on attachment 8601014 [details]
MozReview Request: bz://1131352/jaoo

/r/8095 - Bug 1130684 - Implement Service Worker clients.claim.
/r/8097 - Bug 1131352 - Add ServiceWorkerGlobalScope skipWaiting(). r=nsm

Pull down these commits:

hg pull -r dfec39f4ee82c900682cc6e800acd4ef4fc7740f https://reviewboard-hg.mozilla.org/gecko/

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

Hi Jose, any reason this hasn't landed yet?

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 23 • 9 years ago

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #22)
> Hi Jose, any reason this hasn't landed yet?

It depends on Catalin's and he has not landed his patch yet.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 24 • 9 years ago

I'll land this as soon as possible. Probably today or tomorrow (Monday).

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 25 • 9 years ago

Comment on attachment 8601014 [details]
MozReview Request: bz://1131352/jaoo

/r/8095 - Bug 1131352 - Add ServiceWorkerGlobalScope skipWaiting(). r=nsm

Pull down this commit:

hg pull -r c7ab9fcc8770b4ef2ace49807dac58efff1cd03d https://reviewboard-hg.mozilla.org/gecko/

	Ben Kelly [:bkelly, not reviewing]
	Comment 26 • 9 years ago

I think this is something we need for v1.  The largest site I know using SW is pinterest and they depend on skipWaiting().  The trained-to-thrill demo also depends on it.

Looks like this is going to land soon, though, so shouldn't be a problem.  Just wanted to note that SW sites seem to be depending on this feature.

	Ryan VanderMeulen [:RyanVM]
	Comment 27 • 9 years ago

Please provide a Try link for this.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 28 • 9 years ago

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #27)
> Please provide a Try link for this.

See the try results in the mozreview link.

	Ryan VanderMeulen [:RyanVM]
	Comment 29 • 9 years ago

In the future, it's much easier to just add a link when setting checkin-needed instead of expecting people to go hunting around for it. Also, checkin-needed is the fast way to get your patch landed rather than ni?ing an individual person. Multiple people look for checkin-needed at different times of day instead of just me whenever I happen to be free.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 30 • 9 years ago

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #29)
> In the future, it's much easier to just add a link when setting
> checkin-needed instead of expecting people to go hunting around for it.
> Also, checkin-needed is the fast way to get your patch landed rather than
> ni?ing an individual person. Multiple people look for checkin-needed at
> different times of day instead of just me whenever I happen to be free.

I ni?'ed you because you asked for the link. Sorry if I did wrong.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 31 • 9 years ago

Try results are very bad. There is a compile error (weird). I'll handle it and request checkin-needed again. Sorry for the noise.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 32 • 9 years ago

Let's see how this looks after applying the codegen patch :bz provided.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=da4149d09544

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 33 • 9 years ago

Comment on attachment 8601014 [details]
MozReview Request: bz://1131352/jaoo

/r/8095 - Bug 1131352 - Add ServiceWorkerGlobalScope skipWaiting(). r=nsm
/r/9167 - Bug 1131352 - Fix codegen issue. r=nsm

Pull down these commits:

hg pull -r 1fedcf47bd879a8edd4e1615df02fe313473720b https://reviewboard-hg.mozilla.org/gecko/

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 34 • 9 years ago

Everything seems to be fine. Let's land this then (Latest try results at comment 32).

	Ryan VanderMeulen [:RyanVM]
	Comment 35 • 9 years ago

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #30)
> I ni?'ed you because you asked for the link. Sorry if I did wrong.

No worries :-). It's a team policy, so any of the sheriffs looking at checkin-neededs would want to see a Try link.

	Ryan VanderMeulen [:RyanVM]
	Comment 36 • 9 years ago

That said, this can't land until the webidl changes get DOM peer review.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 37 • 9 years ago

Comment on attachment 8601014 [details]
MozReview Request: bz://1131352/jaoo

/r/8095 - Bug 1131352 - Add ServiceWorkerGlobalScope skipWaiting(). r=nsm
/r/9167 - Bug 1131352 - Fix codegen issue. r=nsm

Pull down these commits:

hg pull -r 1fedcf47bd879a8edd4e1615df02fe313473720b https://reviewboard-hg.mozilla.org/gecko/

	Andrea Marchesini [:baku]
	Comment 38 • 9 years ago

jaoo, can you send me a normal patch to review? reviewboard doesn't support persona login yet.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 39 • 9 years ago

Carrying out r=nsm.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 41 • 9 years ago

Comment on attachment 8608721 [details] [diff] [review]
Part 2: Fix codegen issue

:bz, :peterv, could you -either of you guys- have a look a it?. :bz, you know about if ;). We don't need codegen to generate a genericMethod when the only method in the binding is a genericPromiseReturningMethod otherwise the compiler complains about it not being used. Thanks!

	Boris Zbarsky [:bzbarsky]
	Comment 42 • 9 years ago

Comment on attachment 8608721 [details] [diff] [review]
Part 2: Fix codegen issue

I wrote this patch, so me reviewing it would be quite odd.  ;)

Also, I assume this should be part 1 and the other patch part 2, so we don't end up with busted builds during bisects.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 43 • 9 years ago

:peterv, could you have a look a it?. We don't need codegen to generate a genericMethod when the only method in the binding is a genericPromiseReturningMethod otherwise the compiler complains about it not being used. Thanks!

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 44 • 9 years ago

Carrying out r=nsm

	Andrea Marchesini [:baku]
	Comment 45 • 9 years ago

Comment on attachment 8609843 [details] [diff] [review]
Part 2: Add ServiceWorkerGlobalScope skipWaiting()

Review of attachment 8609843 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/ServiceWorkerManager.cpp
@@ +1332,5 @@
> +  nsRefPtr<ServiceWorkerInfo> exitingWorker = mActiveWorker.forget();
> +  if (!exitingWorker)
> +    return;
> +
> +  // FIXME(jaoo): Wait for exitingWorker to finish and terminate.

write here the follow-up bug ID.

::: dom/workers/WorkerScope.cpp
@@ +581,5 @@
> +  mWorkerPrivate->AssertIsOnWorkerThread();
> +  MOZ_ASSERT(mWorkerPrivate->IsServiceWorker());
> +
> +  nsRefPtr<Promise> promise = Promise::Create(this, aRv);
> +  if (aRv.Failed()) {

NS_WARN_IF

@@ +597,5 @@
> +  nsRefPtr<WorkerScopeSkipWaitingRunnable> runnable =
> +    new WorkerScopeSkipWaitingRunnable(promiseProxy,
> +                                       NS_ConvertUTF16toUTF8(mScope));
> +
> +  aRv = NS_DispatchToMainThread(runnable);

nsresult rv = NS_DispatchToMainThread(runnable);
if (NS_WARN_IF(NS_FAILED(rv))) {
  promise->MaybeReject(NS_ERROR_DOM_ABORT_ERR);
}

If you return the promise, then you must return a !aRv.Failed()

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 46 • 9 years ago

Address review comments made at comment 45.

Carrying out r=nsm,baku

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 47 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae61cc08ccab

Let's see how try looks and land this is everything is fine.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 48 • 9 years ago

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #47)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae61cc08ccab
> 
> Let's see how try looks and land this is everything is fine.

Looking good, lets land it.

	Pulsebot
	Comment 49 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/cbfd275dd604
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cdcc90a4738

	Pulsebot
	Comment 50 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/91bba46e795a

	Ryan VanderMeulen [:RyanVM]
	Comment 51 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/cbfd275dd604
https://hg.mozilla.org/mozilla-central/rev/8cdcc90a4738
https://hg.mozilla.org/mozilla-central/rev/91bba46e795a

	Chris Mills [:cmills]
	Comment 52 • 9 years ago

We've got this covered at :

https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerGlobalScope/skipWaiting

A tech review would be nice. Thanks!

	Jean-Yves Perrier [:teoli]
	Comment 53 • 9 years ago

I've updated the compat table, it is in Gecko 41 not 32. 
Added a line in https://developer.mozilla.org/en-US/Firefox/Releases/41#InterfacesAPIsDOM