1160527 - Other ServiceWorker events should inherit from ExtendableEvent

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Note, the MessageEvent has been fixed in the spec:

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

Comment 2

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: MozReview Request: Bug 1160527 - Patch 1 - Support PushEvent.waitUntil(). r=baku.

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

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: patch

Comment 4

[Andrea Marchesini [:baku]]

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;
}

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Comment 6

[Carsten Book [:Tomcat]]

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

Comment 7

[Chris Mills [:cmills]]

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