Ben Kelly [:bkelly, not reviewing] Reporter
	Description • 9 years ago

The ServiceWorker spec has changed such that FetchEvent and other functional events now inherit from ExtendableEvent.  The purpose of this is to provide waitUntil() on all events.

We should implement this and the requisite logic to support waitUntil() on these events.

Note, MessageEvent is not currently spec'd to inherit from ExtendableEvent, but I wrote this issue to possibly fix that:

  https://github.com/slightlyoff/ServiceWorker/issues/690

See bug 1134841 for some of the annoying intermittents that can happen from early worker shutdown.

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

Note, the MessageEvent has been fixed in the spec:

  https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#extendablemessage-event-section

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

Bug 1160527 - Patch 1 - Support PushEvent.waitUntil(). r=baku.

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

Comment on attachment 8615712 [details] [diff] [review]
patch

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

Sorry if it took so long to review this code.

::: dom/workers/ServiceWorkerManager.cpp
@@ +1453,5 @@
>    PendingOperation* opt = mPendingOperations.AppendElement();
>    opt->mRunnable = aRunnable;
>  }
>  
> +namespace {

Don't we already have an anonymous namespace? Can we merge them?

@@ +1460,5 @@
> +{
> +  nsMainThreadPtrHandle<ServiceWorker> mServiceWorker;
> +
> +  virtual
> +  ~KeepAliveHandler()

same line?

@@ +1503,5 @@
> +  WidgetEvent* internalEvent = aEvent->GetInternalNSEvent();
> +
> +  ErrorResult result;
> +  result = aWorkerScope->DispatchDOMEvent(nullptr, aEvent, nullptr, nullptr);
> +  if (!result.Failed() && !internalEvent->mFlags.mExceptionHasBeenRisen) {

can you do a quick return here?

if (result.Failed() || ... ) {
  return nullptr;
}

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/3835361fac6f

	Carsten Book [:Tomcat]
	Comment 6 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/3835361fac6f

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

So ExtendableEvent is here:

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

And the other functional event types are here:

https://developer.mozilla.org/en-US/docs/Web/API/FetchEvent
https://developer.mozilla.org/en-US/docs/Web/API/InstallEvent
https://developer.mozilla.org/en-US/docs/Web/API/NotificationEvent
https://developer.mozilla.org/en-US/docs/Web/API/PeriodicSyncEvent
https://developer.mozilla.org/en-US/docs/Web/API/SyncEvent

Does this look ok?

I also a note to the relevant release notes page:

https://developer.mozilla.org/en-US/Firefox/Releases/41#Service_Workers