Closed
Bug 1160527
Opened 9 years ago
Closed 8 years ago
Other ServiceWorker events should inherit from ExtendableEvent
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: bkelly, Assigned: nsm)
References
Details
(Keywords: dev-doc-complete)
Attachments
(1 file, 1 obsolete file)
5.88 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
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•9 years ago
|
Assignee: nobody → ehsan
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
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•8 years ago
|
||
Bug 1160527 - Patch 1 - Support PushEvent.waitUntil(). r=baku.
Attachment #8614387 -
Flags: review?(amarchesini)
Assignee | ||
Comment 3•8 years ago
|
||
Assignee: ehsan → nsm.nikhil
Attachment #8614387 -
Attachment is obsolete: true
Attachment #8614387 -
Flags: review?(amarchesini)
Attachment #8615712 -
Flags: review?(amarchesini)
Updated•8 years ago
|
Comment 4•8 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; }
Attachment #8615712 -
Flags: review?(amarchesini) → review+
Updated•8 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 5•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3835361fac6f
Comment 6•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3835361fac6f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment 7•8 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
Keywords: dev-doc-needed → dev-doc-complete
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•