Bug 1275436 - Simplify firing push subscription change events. r=mt
authorKit Cambridge <kcambridge@mozilla.com>
Thu, 05 May 2016 09:12:35 -0700
changeset 339443 605c72e8e966af0ceab460f32d847280cb0a1e81
parent 339442 6153baa6401d8611015ae3fa0d589204286eadba
child 339444 5a00b354638d0806b86a75bf9f1ceca6d4abb0f0
push id6249
push userjlund@mozilla.com
push dateMon, 01 Aug 2016 13:59:36 +0000
treeherdermozilla-beta@bad9d4f5bf7e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmt
bugs1275436
milestone49.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1275436 - Simplify firing push subscription change events. r=mt Even if the event handler calls `subscribe()` or `getSubscription()`, the "readwrite" IDB transactions in `clearIf` and `forEachOrigin` should execute first. MozReview-Commit-ID: ETYGmnOIuag
dom/push/PushDB.jsm
dom/push/PushService.jsm
--- a/dom/push/PushDB.jsm
+++ b/dom/push/PushDB.jsm
@@ -211,49 +211,47 @@ this.PushDB.prototype = {
         },
         resolve,
         reject
       )
     );
   },
 
   /**
-   * Reduces all records associated with an origin to a single value.
+   * Iterates over all records associated with an origin.
    *
    * @param {String} origin The origin, matched as a prefix against the scope.
    * @param {String} originAttributes Additional origin attributes. Requires
    *  an exact match.
-   * @param {Function} callback A function with the signature `(result,
-   *  record, cursor)`, where `result` is the value returned by the previous
-   *  invocation, `record` is the registration, and `cursor` is an `IDBCursor`.
-   * @param {Object} [initialValue] The value to use for the first invocation.
-   * @returns {Promise} Resolves with the value of the last invocation.
+   * @param {Function} callback A function with the signature `(record,
+   *  cursor)`, called for each record. `record` is the registration, and
+   *  `cursor` is an `IDBCursor`.
+   * @returns {Promise} Resolves once all records have been processed.
    */
-  reduceByOrigin: function(origin, originAttributes, callback, initialValue) {
+  forEachOrigin: function(origin, originAttributes, callback) {
     console.debug("forEachOrigin()");
 
     return new Promise((resolve, reject) =>
       this.newTxn(
         "readwrite",
         this._dbStoreName,
         (aTxn, aStore) => {
-          aTxn.result = initialValue;
+          aTxn.result = undefined;
 
           let index = aStore.index("identifiers");
           let range = IDBKeyRange.bound(
             [origin, originAttributes],
             [origin + "\x7f", originAttributes]
           );
           index.openCursor(range).onsuccess = event => {
             let cursor = event.target.result;
             if (!cursor) {
               return;
             }
-            let record = this.toPushRecord(cursor.value);
-            aTxn.result = callback(aTxn.result, record, cursor);
+            callback(this.toPushRecord(cursor.value), cursor);
             cursor.continue();
           };
         },
         resolve,
         reject
       )
     );
   },
--- a/dom/push/PushService.jsm
+++ b/dom/push/PushService.jsm
@@ -652,25 +652,22 @@ this.PushService = {
    *
    * We ignore expired registrations because they're already handled in other
    * code paths. Registrations that expired after exceeding their quotas are
    * evicted at startup, or on the next `idle-daily` event. Registrations that
    * expired because the user revoked the notification permission are evicted
    * once the permission is reinstated.
    */
   dropUnexpiredRegistrations: function() {
-    let subscriptionChanges = [];
     return this._db.clearIf(record => {
       if (record.isExpired()) {
         return false;
       }
-      subscriptionChanges.push(record);
+      this._notifySubscriptionChangeObservers(record);
       return true;
-    }).then(() => {
-      this.notifySubscriptionChanges(subscriptionChanges);
     });
   },
 
   _notifySubscriptionChangeObservers: function(record) {
     if (!record) {
       return;
     }
 
@@ -712,22 +709,16 @@ this.PushService = {
   updateRecordAndNotifyApp: function(aKeyID, aUpdateFunc) {
     return this._db.update(aKeyID, aUpdateFunc)
       .then(record => {
         this._notifySubscriptionChangeObservers(record);
         return record;
       });
   },
 
-  notifySubscriptionChanges: function(records) {
-    records.forEach(record => {
-      this._notifySubscriptionChangeObservers(record);
-    });
-  },
-
   ensureCrypto: function(record) {
     if (record.hasAuthenticationSecret() &&
         record.p256dhPublicKey &&
         record.p256dhPrivateKey) {
       return Promise.resolve(record);
     }
 
     let keygen = Promise.resolve([]);
@@ -1302,78 +1293,67 @@ this.PushService = {
     }
 
     return this._updatePermission(permission, data);
   },
 
   _clearPermissions() {
     console.debug("clearPermissions()");
 
-    let subscriptionModifications = [];
     return this._db.clearIf(record => {
       if (!record.quotaApplies()) {
         // Only drop registrations that are subject to quota.
         return false;
       }
       if (record.isExpired()) {
         // Fire subscription modified notifications for expired
-        // records after the IndexedDB transaction has committed.
-        subscriptionModifications.push(record);
+        // records.
+        gPushNotifier.notifySubscriptionModified(record.scope,
+                                                 record.principal);
       } else {
         // Drop unexpired registrations in the background.
         this._backgroundUnregister(record,
           Ci.nsIPushErrorReporter.UNSUBSCRIBE_PERMISSION_REVOKED);
       }
       return true;
-    }).then(() => {
-      subscriptionModifications.forEach(record =>
-        gPushNotifier.notifySubscriptionModified(record.scope, record.principal)
-      );
     });
   },
 
   _updatePermission: function(permission, type) {
     console.debug("updatePermission()");
 
     let isAllow = permission.capability ==
                   Ci.nsIPermissionManager.ALLOW_ACTION;
     let isChange = type == "added" || type == "changed";
 
     if (isAllow && isChange) {
       // Permission set to "allow". Drop all expired registrations for this
       // site, notify the associated service workers, and reset the quota
       // for active registrations.
-      return this._reduceByPrincipal(
+      return this._forEachPrincipal(
         permission.principal,
-        (subscriptionChanges, record, cursor) => {
-          this._permissionAllowed(subscriptionChanges, record, cursor);
-          return subscriptionChanges;
-        },
-        []
-      ).then(subscriptionChanges => {
-        this.notifySubscriptionChanges(subscriptionChanges);
-      });
+        (record, cursor) => this._permissionAllowed(record, cursor)
+      );
     } else if (isChange || (isAllow && type == "deleted")) {
       // Permission set to "block" or "always ask," or "allow" permission
       // removed. Expire all registrations for this site.
-      return this._reduceByPrincipal(
+      return this._forEachPrincipal(
         permission.principal,
-        (memo, record, cursor) => this._permissionDenied(record, cursor)
+        (record, cursor) => this._permissionDenied(record, cursor)
       );
     }
 
     return Promise.resolve();
   },
 
-  _reduceByPrincipal: function(principal, callback, initialValue) {
-    return this._db.reduceByOrigin(
+  _forEachPrincipal: function(principal, callback) {
+    return this._db.forEachOrigin(
       principal.URI.prePath,
       ChromeUtils.originAttributesToSuffix(principal.originAttributes),
-      callback,
-      initialValue
+      callback
     );
   },
 
   /**
    * The update function called for each registration record if the push
    * permission is revoked. We only expire the record so we can notify the
    * service worker as soon as the permission is reinstated. If we just
    * deleted the record, the worker wouldn't be notified until the next visit
@@ -1396,30 +1376,28 @@ this.PushService = {
     cursor.update(record);
   },
 
   /**
    * The update function called for each registration record if the push
    * permission is granted. If the record has expired, it will be dropped;
    * otherwise, its quota will be reset to the default value.
    *
-   * @param {Array} subscriptionChanges A list of records whose associated
-   *  service workers should be notified once the transaction has committed.
    * @param {PushRecord} record The record to update.
    * @param {IDBCursor} cursor The IndexedDB cursor.
    */
-  _permissionAllowed: function(subscriptionChanges, record, cursor) {
+  _permissionAllowed(record, cursor) {
     console.debug("permissionAllowed()");
 
     if (!record.quotaApplies()) {
       return;
     }
     if (record.isExpired()) {
       // If the registration has expired, drop and notify the worker
       // unconditionally.
-      subscriptionChanges.push(record);
+      this._notifySubscriptionChangeObservers(record);
       cursor.delete();
       return;
     }
     record.resetQuota();
     cursor.update(record);
   },
 };