So, I *think* this should be doable without any observable behaviour change for existing API consumers. For the APIs where we currently expect an callback function to be passed in, if we don't get a callback passed in then we instead return a Promise. Or maybe we just always return a Promise regardless? Either way, this would cut down on code wrapping AddonManager API calls in functions that convert it to use promises. Also, pretty sure this is doable entirely from within AddonManager.jsm - without changes to individual providers.

Yeah I thought about this too and I think we can just always return a promise and just always pass any callback to then. One crazy thought I had was a slightly more advanced promise with a cancel function so say when the UI is closed it can cancel any pending promises and not have to deal with the possibilities of promises resolving after the UI is closed.

I'm conflicted (in general, and about this specific API) about having two-faced 'take callback + return promise' APIs; it does give us a gentler transition from one to the other, but it doubles the number of code paths we need to understand. I've spent a fair bit of time thinking about "cancellable promises" - the first hurdle to get across is that the promise you're holding could be at the end of a long chain including multiple complete, in-progress and not-started-yet operations, and the current Promise data structures don't know anything about their predecessors in the chain. The general case gets even worse, because it's possible for a promise chain to mix implementations - you could have 'DOM promise'.then('Toolkit promise').then('JQuery promise), though with care we can probably avoid needing to solve that for now. tl;dr I think the benefit of a Promise API to Addon Manager (and XPIProvider) outweighs my concern about multiple code paths, but I'm inclined to not touch the cancel code paths.

To be clear I would expect cancel() to reject the promise if it hasn't already been completed in some way so it would still uphold the guarantees expected, but I take the point that it may be too much. I think by making every method effectively: function getAddonByID(aId, aCallback) { let deferred = defer(); deferred.promise.then(aCallback); ... return deferred.promise; } we keep the differences between the codepaths minimal.

For what it's worth, I've been using cancellable promises implemented via WeakMaps, and I find it a highly useful feature. It would be nice if our promise implementation supported cancellation by default.

(In reply to Kris Maglione [:kmag] from comment #4) > For what it's worth, I've been using cancellable promises implemented via > WeakMaps, and I find it a highly useful feature. It would be nice if our > promise implementation supported cancellation by default. That sounds like something good to file in "async tooling" for when Yoric is back?

(In reply to Georg Fritzsche [:gfritzsche] from comment #5) > (In reply to Kris Maglione [:kmag] from comment #4) > > For what it's worth, I've been using cancellable promises implemented via > > WeakMaps, and I find it a highly useful feature. It would be nice if our > > promise implementation supported cancellation by default. > > That sounds like something good to file in "async tooling" for when Yoric is > back? The FxOS Device storage people are working on an AbortablePromise implementation (for DOM Promises) in bug 1035060, but I don't know of anyone currently working on adding cancellation to Promise.jsm. The current state-of-the-draft spec discussion appears to be https://github.com/promises-aplus/cancellation-spec/issues/6

The cancellation spec seems to be something that we can implement on top of either Promise.jsm or DOM Promise. I believe that we should wait until we have finally ported Promise.jsm on top of DOM Promise before we proceed in this direction.

Removing [good second bug], as this isn't actionable yet (see comment 7).

Well, we could implement cancellation independent of Promises (that's what we have for our callback-based APIs, for the operations we support cancelling), but I agree it would be nice to have a clean way to cancel in-progress AddonManager operations. That said, I think we should take it in steps: implement plain Promises first and then add cancel support.

Ok!

It is not a complete patch. But I will like to have a feedback on the approach I am using. :)

Comment on attachment 8516202 [details] [diff] [review] 987512_AddonManagerToUsePromise.patch > if (typeof aCallback != "function") > throw Components.Exception("aCallback must be a function", > Cr.NS_ERROR_INVALID_ARG); This needs ` || aCallback != null` >+ p.then((value) => saeCall(aCallback, aAddon), null); This needs `if (aCallback != null)`. Aside from the typo, though, something like the following would be preferable: if (aCallback != null) { p.then(value => { safeCall(aCallback, aAddon) }) .then(null, Cu.reportError); } This also needs tests to ensure that both forms behave as expected. In the future, it would be a good idea to test your code before requesting feedback.

Comment on attachment 8516202 [details] [diff] [review] 987512_AddonManagerToUsePromise.patch Review of attachment 8516202 [details] [diff] [review]: ----------------------------------------------------------------- This seems ok. I'm not entrirely happy with it, as there's a lot of boilerplate between needing both Promise and AsyncObjectCaller. But I don't see a way to avoid that, other than re-thinking AsyncObjectCaller (which has been discussed, but it's not a simple fix).

Akshendra: Are you still working on this?

Sorry for the delay, have been busy lately with my academics. Will submit a patch within this week.

Blair, I am having troubles writing tests. Can you hint me to a test for some insight on this. Thanks.

(In reply to Akshendra Pratap Singh from comment #16) > I am having troubles writing tests. Can you hint me to a test for some > insight on this. You'll want an XPCShell test for this (https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests). Have a look at test_general.js (http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_general.js) and base your test on that - we don't need anything overly complex here.

Hey Akshendra, did you manage to get these tests written?

Sorry but I could not manage to write the tests.

Comment on attachment 8819043 [details] Bug 987512: Part 1 - Support both promises and callbacks in the AddonManager API. https://reviewboard.mozilla.org/r/98906/#review99492 ::: toolkit/mozapps/extensions/AddonManager.jsm:226 (Diff revision 1) > +function promiseOrCallback(promise, callback) { > + if (!callback) > + return promise; > + > + if (typeof callback !== "function") > + throw Components.Exception("aCallback must be a function", It's called `callback` in the function signature and `aCallback` in the exception, pick one? Preferably the former

Comment on attachment 8819044 [details] Bug 987512: Part 2 - Remove promise wrappers from AddonTestUtils. https://reviewboard.mozilla.org/r/98908/#review99532

Comment on attachment 8819045 [details] Bug 987512: Part 3 - Support promises in getInstallForFile and getInstallForURL. https://reviewboard.mozilla.org/r/98910/#review99534

Comment on attachment 8819046 [details] Bug 987512: Part 4 - Remove manual AddonManager promise wrappers from test code. https://reviewboard.mozilla.org/r/98912/#review99536

Comment on attachment 8819047 [details] Bug 987512: Part 5 - Remove manual AddonManager promise wrappers. https://reviewboard.mozilla.org/r/98914/#review99538

Comment on attachment 8819043 [details] Bug 987512: Part 1 - Support both promises and callbacks in the AddonManager API. https://reviewboard.mozilla.org/r/98906/#review99492 > It's called `callback` in the function signature and `aCallback` in the exception, pick one? Preferably the former I went with aCallback because it's still called aCallback in the method signatures in all of the docs. But I suppose I have to update the docs, anyway, so...

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa05269ea601f52c430cc95404e8646c76b2776c Bug 987512: Part 1 - Support both promises and callbacks in the AddonManager API. r=rhelmer https://hg.mozilla.org/integration/mozilla-inbound/rev/5087537f8766732337eeaab0a8bee9b58281c3e9 Bug 987512: Part 2 - Remove promise wrappers from AddonTestUtils. r=rhelmer https://hg.mozilla.org/integration/mozilla-inbound/rev/1dcf79ea901134abe0d967ee95d788c42640e6a2 Bug 987512: Part 3 - Support promises in getInstallForFile and getInstallForURL. r=rhelmer https://hg.mozilla.org/integration/mozilla-inbound/rev/807adcac28b1b0fb763366a21a7fc149f7a3a57a Bug 987512: Part 4 - Remove manual AddonManager promise wrappers from test code. r=rhelmer https://hg.mozilla.org/integration/mozilla-inbound/rev/32ca7dcd1be5a152e49b7a57e78a3b2bb766a423 Bug 987512: Part 5 - Remove manual AddonManager promise wrappers. r=rhelmer

getInstallForURL now calls its callback asynchronously, where it previously apparently always called it synchronously. This broke a couple of tests that relied on the old behavior, so it may break some add-ons as well.

(In reply to Kris Maglione [:kmag] from comment #32) > getInstallForURL now calls its callback asynchronously, where it previously > apparently always called it synchronously. This broke a couple of tests that > relied on the old behavior, so it may break some add-ons as well. I checked DXR and didn't find any add-ons that depend on this behavior, so perhaps it's not worth worrying about.

Noted in: https://developer.mozilla.org/en-US/Firefox/Releases/53 and basic documentation added to: https://developer.mozilla.org/en-US/Add-ons/Add-on_Manager/AddonManager It could use a bit more documentation, but that page is currently broken due to the AMInterface template apparently disappearing, so it will have to wait until that is fixed.

https://hg.mozilla.org/mozilla-central/rev/fa05269ea601 https://hg.mozilla.org/mozilla-central/rev/5087537f8766 https://hg.mozilla.org/mozilla-central/rev/1dcf79ea9011 https://hg.mozilla.org/mozilla-central/rev/807adcac28b1 https://hg.mozilla.org/mozilla-central/rev/32ca7dcd1be5