1187350 - service worker update() method should return a promise

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

Recent change to the spec:

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

Note, I'm marking this v1 since the issue is considered "milestone 1" for service workers.  We could reasonably delay this, though.

Comment 1

[(no longer active)]

Attachment: sketch for bug 1187350

This is a rough sketch for the fix.  There are two big things missing from this:

* We may end up releasing a promise on the wrong thread.  We would need to pass in an event target and pipe it all the way to release stuff on the correct thread.
* We should probably pass a pointer to the global object to ServiceWorkerUpdateFinishCallback and get rid of ServiceWorkerResolveWindowPromiseOnUpdateCallback::mWindow.  Plus, ServiceWorkerResolveWindowPromiseOnUpdaeCallback::UpdateFailed(const ErrorEventInit& aErrorDesc) should probably move whole-sale to the base class.

If someone wants to finish this up, please go ahead!

Comment 2

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

Attachment: MozReview Request: Bug 1187350 - update() should return a Promise. r?ehsan,catalinb

Bug 1187350 - update() should return a Promise. r?ehsan,catalinb

Comment 3

[Cătălin Badea (:catalinb)]

Comment on attachment 8648319 [details]
MozReview Request: Bug 1187350 - update() should return a Promise. r?ehsan,catalinb

https://reviewboard.mozilla.org/r/16201/#review14537

lgtm, just one comment

::: dom/workers/ServiceWorkerRegistration.cpp:374
(Diff revision 1)
> +    : mPromiseProxy(PromiseWorkerProxy::Create(aWorkerPrivate, aWorkerPromise))

This is not safe since PromiseWorkerProxy::Create can fail and there are no null checks when mPromiseProxy is used on the main thread. You should check this on the worker thread and don't dispatch the runnable if it failed.

Comment 4

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

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

Comment 5

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

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

Comment 6

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

Ehsan, could i get dom peer sign off on the webidl? Thanks!

Comment 7

[(no longer active)]

Comment on attachment 8648319 [details]
MozReview Request: Bug 1187350 - update() should return a Promise. r?ehsan,catalinb

https://reviewboard.mozilla.org/r/16201/#review14993

Ship It!

Comment 8

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

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/780809cead8968a0a90e3f913aa568dfa889a5f7
changeset:  780809cead8968a0a90e3f913aa568dfa889a5f7
user:       Nikhil Marathe <[e-mail]>
date:       Fri Aug 14 15:06:00 2015 -0700
description:
Bug 1187350 - update() should return a Promise. r=ehsan,catalinb

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/780809cead89