Change PusheService.jsm to use Promise

This is a separate issue from PushServiceHttp2 so I have opened a new bug.

Comment on attachment 8603230 [details] [diff] [review] bug_1162414_v1.patch Review of attachment 8603230 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/push/PushService.jsm @@ +1382,5 @@ > // Fires a push-register system message to all applications that have > // registration. > _notifyAllAppsRegister: function() { > debug("notifyAllAppsRegister()"); > return new Promise((resolve, reject) => { Remove this. @@ +1384,5 @@ > _notifyAllAppsRegister: function() { > debug("notifyAllAppsRegister()"); > return new Promise((resolve, reject) => { > // records are objects describing the registration as stored in IndexedDB. > + this._db.getAllChannelIDs() Prefix |return| to this and you'll achieve the same thing. @@ +1400,5 @@ > + scope > + ); > + globalMM.broadcastAsyncMessage('pushsubscriptionchanged', scope); > + } > + resolve(); Then you don't need to do this. @@ +1401,5 @@ > + ); > + globalMM.broadcastAsyncMessage('pushsubscriptionchanged', scope); > + } > + resolve(); > + }, reject); and this @@ +1642,5 @@ > + // about the reply. > + this._send("unregister", {channelID: record.channelID}); > + deferred.resolve(); > + }, err => fail(err)); > + },err => fail(err)); Nit: space between , and err @@ +1667,5 @@ > ); > }, > > _clearAll: function _clearAll() { > return new Promise((resolve, reject) => { The same pattern again, you can get rid of the outer promise and return the inner one. @@ +1675,3 @@ > }); > }, > Could you get rid of this trailing space while you are here. Thanks! @@ +1677,5 @@ > > /** > * Called on message from the child process > */ > _registration: function(aPageRecord) { This can be written as: function(aPageRecord) { if (!aPageRecord.scope) { return Promise.reject("Database error"); } return this._db.getByScope(...) .then(... existing code) } and then get rid of the calls to resolve and empty _ => reject.

Comment on attachment 8603230 [details] [diff] [review] bug_1162414_v1.patch Review of attachment 8603230 [details] [diff] [review]: ----------------------------------------------------------------- I don't think that this goes far enough, but I can't see all of the code that might be affected. ::: dom/push/PushService.jsm @@ +428,5 @@ > + // just for it > + if (this._ws) { > + debug("Had a connection, so telling the server"); > + this._send("unregister", {channelID: records.channelID}); > + } Rethrow here or the error will be swallowed. (Not enough context in splinter to know if this is OK; maybe you should consider using review board.) @@ +1624,5 @@ > }; > > if (!aPageRecord.scope) { > fail("NotFoundError"); > return deferred.promise; This worries me a little. You shouldn't need to be using a Promise.defer() anywhere any more.

/r/8483 - Bug 1162414 - Change PushService.jsm db to use promise. r=:nsm r=:mt Pull down this commit: hg pull -r def9a15a2221d96d6006ac7538a08b85ab3e2f39 https://reviewboard-hg.mozilla.org/gecko/

Comment on attachment 8604132 [details] MozReview Request: bz://1162414/:dragana https://reviewboard.mozilla.org/r/8481/#review7165 ::: dom/push/PushService.jsm:1638 (Diff revision 1) > + Extra line

Dragana, I just noticed that you have prefixed all the names with ':'. It should be r=mt on the comment, not r=:mt. Also, check your reviewboard configuration. I have: [mozilla] ircnick = mt

can we get a try run for this ?

/r/8883 - Bug 1162414 - Change PushService.jsm db to use promise. r=nsm r=mt /r/8885 - Bug 1150812 - split PushService - separate webSocket part. r=nsm, r=mt /r/8887 - Bug 1150812 - split PushService - make the separation more common for webSocket and http2. r=nsm, r=mt /r/8889 - Bug 1150812 - adapt xpcshell test and add mochi tests for WebSocket version. r=nsm, r=mt /r/8891 - Bug 1150812 - Add Http2 Push service. r=nsm, r=mt /r/8893 - Bug 1150812 - xcpshell test for PushService with http2. r=nsm, r=mt Pull down these commits: hg pull -r bb6c468afbae24342decf6d5d9ef6931a079d0e1 https://reviewboard-hg.mozilla.org/gecko/

Comment on attachment 8607015 [details] MozReview Request: bz://1162414/dragana wrong bug