Closed Bug 1162414 Opened 5 years ago Closed 5 years ago

Change PushService db to use promise

Categories

(Core :: DOM: Push Notifications, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: dragana, Assigned: dragana)

References

Details

Attachments

(1 file, 4 obsolete files)

Change PusheService.jsm to use Promise
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
Blocks: 1150812
Attached patch bug_1162414_v1.patch (obsolete) — Splinter Review
This is a separate issue from PushServiceHttp2 so I have opened a new bug.
Attachment #8602573 - Flags: review?(nsm.nikhil)
Attached patch bug_1162414_v1.patch (obsolete) — Splinter Review
Attachment #8602573 - Attachment is obsolete: true
Attachment #8602573 - Flags: review?(nsm.nikhil)
Attachment #8603230 - Flags: review?(nsm.nikhil)
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.
Attachment #8603230 - Flags: review?(nsm.nikhil) → review+
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/
Attachment #8604132 - Flags: review?(martin.thomson)
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
Attachment #8604132 - Flags: review?(martin.thomson) → review+
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
Attachment #8603230 - Attachment is obsolete: true
Attachment #8604132 - Attachment is obsolete: true
Attachment #8604823 - Flags: review+
Keywords: checkin-needed
can we get a try run for this ?
Flags: needinfo?(dd.mozilla)
Keywords: checkin-needed
Keywords: checkin-needed
/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/
Attachment #8607015 - Flags: review?(nsm.nikhil)
Attachment #8607015 - Flags: review?(martin.thomson)
Comment on attachment 8607015 [details]
MozReview Request: bz://1162414/dragana

wrong bug
Attachment #8607015 - Attachment is obsolete: true
Attachment #8607015 - Flags: review?(nsm.nikhil)
Attachment #8607015 - Flags: review?(martin.thomson)
https://hg.mozilla.org/mozilla-central/rev/551237426e90
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.