When install request has been implemented, there was no promise support. So that you had to send back a custom event, here `webappsEvent` in order communicate the asynchronous result for this given installation. We should switch to promise now that the gecko18 branch is frozen.

This patch is on top of bug 914604 that adds various tests for the actor.

Comment on attachment 802313 [details] [diff] [review] Use promise for async install request instead of sending events. r=past Review of attachment 802313 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! ::: toolkit/devtools/server/actors/webapps.js @@ +192,5 @@ > > delete aApp.manifest; > + aDefer.resolve({ appId: aId, path: aDir.path }); > + > + // Keep sending this event for compatility with old clients I don't believe we care about supporting old clients. The only scenario we want to support is a recent devtools client targeting an old server (probably embedded in a phone). @@ +265,5 @@ > installHostedApp: function wa_actorInstallHosted(aDir, aId, aReceipts, > aManifest, aMetadata) { > debug("installHostedApp"); > let self = this; > + let defer = promise.defer(); Nit: the object created by defer() is commonly called deferred, as we routinely use nouns for objects and verbs for functions. In this case you would also use aDeferred for the formal argument. @@ +512,5 @@ > + appDir.remove(true); > + } catch(e) {} > + return { error: "badParameterType", > + message: "hosted app file and manifest/metadata fields " + > + "are missing" Trailing whitespace.

(In reply to Panos Astithas [:past] from comment #3) > ::: toolkit/devtools/server/actors/webapps.js > @@ +192,5 @@ > > > > delete aApp.manifest; > > + aDefer.resolve({ appId: aId, path: aDir.path }); > > + > > + // Keep sending this event for compatility with old clients > > I don't believe we care about supporting old clients. The only scenario we > want to support is a recent devtools client targeting an old server > (probably embedded in a phone). CCing Dave, in case I'm misrepresenting our current policy.

I think should assume that developers are using the latest browser on the same channel as the thing they're debugging. So a nightly b2g build can assume an up-to-date nightly. Once something hits release, the latest release desktop client should work on it. So since this is only on nightly and will move to aurora on desktop concurrent with/before it gets to aurora on b2g, I don't think we need extra compat here.

(In reply to Panos Astithas [:past] from comment #3) > Comment on attachment 802313 [details] [diff] [review] > I don't believe we care about supporting old clients. The only scenario we > want to support is a recent devtools client targeting an old server > (probably embedded in a phone). Actually I agree with that with most other features being exposed by the remote protocol, except this one, the install method is already being used by external dependencies. Like b2g remote addon from fabrice that has been forked since by some external contributors.

Like this tool: https://github.com/arcturus/node-firefoxos-cli But having look at its source, it wasn't listening for the webappsEvent... So if you think we shouldn't care I can drop this compatibility layer and try to message projects I see using install request.

Comment on attachment 803008 [details] [diff] [review] Removed compatibility code and renamed all defer to deferred Review of attachment 803008 [details] [diff] [review]: ----------------------------------------------------------------- I modified some other pieces of webapps actor that was using defer instead of deferred. Also I had to modify some new piece of code, tests, that were listening for webappsEvent.

Comment on attachment 803008 [details] [diff] [review] Removed compatibility code and renamed all defer to deferred Review of attachment 803008 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/devtools/server/actors/webapps.js @@ +143,5 @@ > this._actorPool = null; > this.conn = null; > }, > > + _registerApp: function wa_actorRegisterApp(aDefer, aApp, aId, aDir) { aDefer -> aDeferred @@ +203,5 @@ > aDir.remove(true); > }); > }, > > + _sendError: function wa_actorSendError(aDefer, aMsg, aId) { Same here.

Note for checkin, it needs to be landed after bug 914604, which is also ready to land.