Other ServiceWorker events should inherit from ExtendableEvent

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
2 months ago

People

(Reporter: bkelly, Assigned: nsm)

Tracking

({dev-doc-complete})

unspecified
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.

Updated

4 years ago
Assignee: nobody → ehsan
Note, the MessageEvent has been fixed in the spec:

  https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#extendablemessage-event-section
Status: NEW → ASSIGNED
Bug 1160527 - Patch 1 - Support PushEvent.waitUntil(). r=baku.
Attachment #8614387 - Flags: review?(amarchesini)
Posted patch patchSplinter Review
Assignee: ehsan → nsm.nikhil
Attachment #8614387 - Attachment is obsolete: true
Attachment #8614387 - Flags: review?(amarchesini)
Attachment #8615712 - Flags: review?(amarchesini)

Updated

4 years ago
Blocks: ServiceWorkers-postv1
No longer blocks: ServiceWorkers-v1
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;
}
Attachment #8615712 - Flags: review?(amarchesini) → review+
https://hg.mozilla.org/mozilla-central/rev/3835361fac6f
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41

Updated

4 years ago
Depends on: 1180274
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.