Issue 7015 - Serialize subscriptions for *exportData in one loop

lib/filterStorage.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 "use strict";
 
 /**
- * @fileOverview FilterStorage class responsible for managing user's
- *               subscriptions and filters.
+ * @fileOverview <code>filterStorage</code> object responsible for managing the
+ * user's subscriptions and filters.
  */
 
 const {IO} = require("io");
 const {Prefs} = require("prefs");
 const {Filter, ActiveFilter} = require("./filterClasses");
 const {Subscription, SpecialSubscription,
        ExternalSubscription} = require("./subscriptionClasses");
 const {filterNotifier} = require("./filterNotifier");
 const {INIParser} = require("./iniParser");
 
 /**
  * Version number of the filter storage file format.
  * @type {number}
  */
-let formatVersion = 5;
+const FORMAT_VERSION = 5;
 
 /**
- * This class reads user's filters from disk, manages them in memory
- * and writes them back.
- * @class
+ * {@link filterStorage} implementation.
  */
-let FilterStorage = exports.FilterStorage =
+class FilterStorage
 {
   /**
-   * Will be set to true after the initial loadFromDisk() call completes.
-   * @type {boolean}
-   */
-  initialized: false,
-
-  /**
-   * Version number of the patterns.ini format used.
+   * @hideconstructor
+   */
+  constructor()
+  {
+    /**
+     * Will be set to true after the initial {@link FilterStorage#loadFromDisk}
+     * call completes.
+     * @type {boolean}
+     */
+    this.initialized = false;
+
+    /**
+     * Will be set to <code>true</code> if no <code>patterns.ini</code> file
+     * exists.
+     * @type {boolean}
+     */
+    this.firstRun = false;
+
+    /**
+     * Map of properties listed in the filter storage file before the sections
+     * start. Right now this should be only the format version.
+     * @type {object}
+     */
+    this.fileProperties = Object.create(null);
+
+    /**
+     * Map of subscriptions already on the list, by their URL/identifier.
+     * @type {Map.<string,Subscription>}
+     */
+    this.knownSubscriptions = new Map();
+
+    /**
+     * Will be set to true if {@link FilterStorage#saveToDisk} is running
+     * (reentrance protection).
+     * @type {boolean}
+     * @private
+     */
+    this._saving = false;
+
+    /**
+     * Will be set to true if a {@link FilterStorage#saveToDisk} call arrives
+     * while {@link FilterStorage#saveToDisk} is already running (delayed
+     * execution).
+     * @type {boolean}
+     * @private
+     */
+    this._needsSave = false;
+  }
+
+  /**
+   * The version number of the <code>patterns.ini</code> format used.
    * @type {number}
    */
   get formatVersion()
   {
-    return formatVersion;
-  },
-
-  /**
-   * File containing the filter list
+    return FORMAT_VERSION;
+  }
+
+  /**
+   * The file containing the subscriptions.
    * @type {string}
    */
   get sourceFile()
   {
     return "patterns.ini";
-  },
-
-  /**
-   * Will be set to true if no patterns.ini file exists.
-   * @type {boolean}
-   */
-  firstRun: false,
-
-  /**
-   * Map of properties listed in the filter storage file before the sections
-   * start. Right now this should be only the format version.
-   */
-  fileProperties: Object.create(null),
-
-  /**
-   * Yields subscriptions containing all filters
+  }
+
+  /**
+   * Yields all subscriptions in the storage.
    * @yields {Subscription}
    */
   *subscriptions()
   {
     yield* this.knownSubscriptions.values();
-  },
-
-  /**
-   * Number of known subscriptions.
+  }
+
+  /**
+   * The number of subscriptions in the storage.
    * @type {number}
    */
   get subscriptionCount()
   {
     return this.knownSubscriptions.size;
-  },
-
-  /**
-   * Map of subscriptions already on the list, by their URL/identifier
-   * @type {Map.<string,Subscription>}
-   */
-  knownSubscriptions: new Map(),
+  }
 
   /**
    * Finds the filter group that a filter should be added to by default. Will
-   * return null if this group doesn't exist yet.
+   * return <code>null</code> if this group doesn't exist yet.
    * @param {Filter} filter
-   * @return {?SpecialSubscription}
+   * @returns {?SpecialSubscription}
    */
   getGroupForFilter(filter)
   {
     let generalSubscription = null;
-    for (let subscription of FilterStorage.knownSubscriptions.values())
+    for (let subscription of this.knownSubscriptions.values())
     {
       if (subscription instanceof SpecialSubscription && !subscription.disabled)
       {
         // Always prefer specialized subscriptions
         if (subscription.isDefaultFor(filter))
           return subscription;
 
         // If this is a general subscription - store it as fallback
         if (!generalSubscription &&
             (!subscription.defaults || !subscription.defaults.length))
         {
           generalSubscription = subscription;
         }
       }
     }
     return generalSubscription;
-  },
-
-  /**
-   * Adds a filter subscription to the list
-   * @param {Subscription} subscription filter subscription to be added
+  }
+
+  /**
+   * Adds a subscription to the storage.
+   * @param {Subscription} subscription The subscription to be added.
    */
   addSubscription(subscription)
   {
-    if (FilterStorage.knownSubscriptions.has(subscription.url))
-      return;
-
-    FilterStorage.knownSubscriptions.set(subscription.url, subscription);
-    addSubscriptionFilters(subscription);
+    if (this.knownSubscriptions.has(subscription.url))
+      return;
+
+    this.knownSubscriptions.set(subscription.url, subscription);
+    connectSubscriptionFilters(subscription);
 
     filterNotifier.emit("subscription.added", subscription);
-  },
-
-  /**
-   * Removes a filter subscription from the list
-   * @param {Subscription} subscription filter subscription to be removed
+  }
+
+  /**
+   * Removes a subscription from the storage.
+   * @param {Subscription} subscription The subscription to be removed.
    */
   removeSubscription(subscription)
   {
-    if (!FilterStorage.knownSubscriptions.has(subscription.url))
-      return;
-
-    removeSubscriptionFilters(subscription);
-
-    FilterStorage.knownSubscriptions.delete(subscription.url);
+    if (!this.knownSubscriptions.has(subscription.url))
+      return;
+
+    disconnectSubscriptionFilters(subscription);
+
+    this.knownSubscriptions.delete(subscription.url);
 
     // This should be the last remaining reference to the Subscription
     // object.
     Subscription.knownSubscriptions.delete(subscription.url);
 
     filterNotifier.emit("subscription.removed", subscription);
-  },
-
-  /**
-   * Replaces the list of filters in a subscription by a new list
-   * @param {Subscription} subscription filter subscription to be updated
-   * @param {Filter[]} filters new filter list
+  }
+
+  /**
+   * Replaces the list of filters in a subscription with a new list.
+   * @param {Subscription} subscription The subscription to be updated.
+   * @param {Array.<Filter>} filters The new list of filters.
    */
   updateSubscriptionFilters(subscription, filters)
   {
-    removeSubscriptionFilters(subscription);
-    let oldFilters = subscription.filters;
-    subscription.filters = filters;
-    addSubscriptionFilters(subscription);
+    let oldFilters = [...subscription.filters()];
+    disconnectSubscriptionFilters(subscription, oldFilters);
+    subscription.clearFilters();
+
+    for (let filter of filters)
+      subscription.addFilter(filter);
+
+    connectSubscriptionFilters(subscription, filters);
+
     filterNotifier.emit("subscription.updated", subscription, oldFilters);
-  },
-
-  /**
-   * Adds a user-defined filter to the list
+  }
+
+  /**
+   * Adds a user-defined filter to the storage.
    * @param {Filter} filter
-   * @param {SpecialSubscription} [subscription]
-   *   particular group that the filter should be added to
-   * @param {number} [position]
-   *   position within the subscription at which the filter should be added
+   * @param {?SpecialSubscription} [subscription] The subscription that the
+   *   filter should be added to.
+   * @param {number} [position] The position within the subscription at which
+   *   the filter should be added. If not specified, the filter is added at the
+   *   end of the subscription.
    */
   addFilter(filter, subscription, position)
   {
     if (!subscription)
     {
       for (let currentSubscription of filter.subscriptions())
       {
         if (currentSubscription instanceof SpecialSubscription &&
             !currentSubscription.disabled)
         {
           return;   // No need to add
         }
       }
-      subscription = FilterStorage.getGroupForFilter(filter);
+      subscription = this.getGroupForFilter(filter);
     }
     if (!subscription)
     {
       // No group for this filter exists, create one
       subscription = SpecialSubscription.createForFilter(filter);
       this.addSubscription(subscription);
       return;
     }
 
     if (typeof position == "undefined")
-      position = subscription.filters.length;
+      position = subscription.filterCount;
 
     filter.addSubscription(subscription);
-    subscription.filters.splice(position, 0, filter);
+    subscription.insertFilterAt(filter, position);
     filterNotifier.emit("filter.added", filter, subscription, position);
-  },
-
-  /**
-   * Removes a user-defined filter from the list
+  }
+
+  /**
+   * Removes a user-defined filter from the storage.
    * @param {Filter} filter
-   * @param {SpecialSubscription} [subscription] a particular filter group that
-   *      the filter should be removed from (if ommited will be removed from all
-   *      subscriptions)
-   * @param {number} [position]  position inside the filter group at which the
-   *      filter should be removed (if ommited all instances will be removed)
+   * @param {?SpecialSubscription} [subscription] The subscription that the
+   *   filter should be removed from. If not specified, the filter will be
+   *   removed from all subscriptions.
+   * @param {number} [position] The position within the subscription at which
+   *   the filter should be removed. If not specified, all instances of the
+   *   filter will be removed.
    */
   removeFilter(filter, subscription, position)
   {
     let subscriptions = (
       subscription ? [subscription] : filter.subscriptions()
     );
     for (let currentSubscription of subscriptions)
     {
       if (currentSubscription instanceof SpecialSubscription)
       {
         let positions = [];
         if (typeof position == "undefined")
         {
           let index = -1;
           do
           {
-            index = currentSubscription.filters.indexOf(filter, index + 1);
+            index = currentSubscription.searchFilter(filter, index + 1);
             if (index >= 0)
               positions.push(index);
           } while (index >= 0);
         }
         else
           positions.push(position);
 
         for (let j = positions.length - 1; j >= 0; j--)
         {
           let currentPosition = positions[j];
-          if (currentSubscription.filters[currentPosition] == filter)
+          let currentFilter = currentSubscription.filterAt(currentPosition);
+          if (currentFilter && currentFilter.text == filter.text)
           {
-            currentSubscription.filters.splice(currentPosition, 1);
-            if (currentSubscription.filters.indexOf(filter) < 0)
+            currentSubscription.deleteFilterAt(currentPosition);
+            if (currentSubscription.searchFilter(filter) < 0)
               filter.removeSubscription(currentSubscription);
             filterNotifier.emit("filter.removed", filter, currentSubscription,
                                 currentPosition);
           }
         }
       }
     }
-  },
-
-  /**
-   * Moves a user-defined filter to a new position
+  }
+
+  /**
+   * Moves a user-defined filter to a new position.
    * @param {Filter} filter
-   * @param {SpecialSubscription} subscription filter group where the filter is
-   *                                           located
-   * @param {number} oldPosition current position of the filter
-   * @param {number} newPosition new position of the filter
+   * @param {SpecialSubscription} subscription The subscription where the
+   *   filter is located.
+   * @param {number} oldPosition The current position of the filter.
+   * @param {number} newPosition The new position of the filter.
    */
   moveFilter(filter, subscription, oldPosition, newPosition)
   {
-    if (!(subscription instanceof SpecialSubscription) ||
-        subscription.filters[oldPosition] != filter)
-    {
-      return;
-    }
+    if (!(subscription instanceof SpecialSubscription))
+      return;
+
+    let currentFilter = subscription.filterAt(oldPosition);
+    if (!currentFilter || currentFilter.text != filter.text)
+      return;
 
     newPosition = Math.min(Math.max(newPosition, 0),
-                           subscription.filters.length - 1);
+                           subscription.filterCount - 1);
     if (oldPosition == newPosition)
       return;
 
-    subscription.filters.splice(oldPosition, 1);
-    subscription.filters.splice(newPosition, 0, filter);
+    subscription.deleteFilterAt(oldPosition);
+    subscription.insertFilterAt(filter, newPosition);
     filterNotifier.emit("filter.moved", filter, subscription, oldPosition,
                         newPosition);
-  },
-
-  /**
-   * Increases the hit count for a filter by one
+  }
+
+  /**
+   * Increases the hit count for a filter by one.
    * @param {Filter} filter
    */
   increaseHitCount(filter)
   {
     if (!Prefs.savestats || !(filter instanceof ActiveFilter))
       return;
 
     filter.hitCount++;
     filter.lastHit = Date.now();
-  },
-
-  /**
-   * Resets hit count for some filters
-   * @param {Filter[]} filters  filters to be reset, if null all filters will
-   *                            be reset
+  }
+
+  /**
+   * Resets hit count for some filters.
+   * @param {?Array.<Filter>} [filters] The filters to be reset. If not
+   *   specified, all filters will be reset.
    */
   resetHitCounts(filters)
   {
     if (!filters)
       filters = Filter.knownFilters.values();
     for (let filter of filters)
     {
       filter.hitCount = 0;
       filter.lastHit = 0;
     }
-  },
+  }
 
   /**
    * @callback TextSink
    * @param {string?} line
    */
 
   /**
    * Allows importing previously serialized filter data.
-   * @param {boolean} silent
-   *    If true, no "load" notification will be sent out.
-   * @return {TextSink}
-   *    Function to be called for each line of data. Calling it with null as
-   *    parameter finalizes the import and replaces existing data. No changes
-   *    will be applied before finalization, so import can be "aborted" by
-   *    forgetting this callback.
+   * @param {boolean} silent If <code>true</code>, no "load" notification will
+   *   be sent out.
+   * @returns {TextSink} The function to be called for each line of data.
+   *   Calling it with <code>null</code> as the argument finalizes the import
+   *   and replaces existing data. No changes will be applied before
+   *   finalization, so import can be "aborted" by forgetting this callback.
    */
   importData(silent)
   {
     let parser = new INIParser();
     return line =>
     {
       parser.process(line);
       if (line === null)
       {
         let knownSubscriptions = new Map();
         for (let subscription of parser.subscriptions)
           knownSubscriptions.set(subscription.url, subscription);
 
         this.fileProperties = parser.fileProperties;
         this.knownSubscriptions = knownSubscriptions;
         Filter.knownFilters = parser.knownFilters;
         Subscription.knownSubscriptions = parser.knownSubscriptions;
 
         if (!silent)
           filterNotifier.emit("load");
       }
     };
-  },
-
-  /**
-   * Loads all subscriptions from the disk.
-   * @return {Promise} promise resolved or rejected when loading is complete
+  }
+
+  /**
+   * Loads all subscriptions from disk.
+   * @returns {Promise} A promise resolved or rejected when loading is complete.
    */
   loadFromDisk()
   {
     let tryBackup = backupIndex =>
     {
       return this.restoreBackup(backupIndex, true).then(() =>
       {
         if (this.knownSubscriptions.size == 0)
           return tryBackup(backupIndex + 1);
       }).catch(error =>
@@ skipping 22 matching lines @@
       });
     }).catch(error =>
     {
       Cu.reportError(error);
       return tryBackup(1);
     }).then(() =>
     {
       this.initialized = true;
       filterNotifier.emit("load");
     });
-  },
-
-  /**
-   * Constructs the file name for a patterns.ini backup.
-   * @param {number} backupIndex
-   *    number of the backup file (1 being the most recent)
-   * @return {string} backup file name
+  }
+
+  /**
+   * Constructs the file name for a <code>patterns.ini</code> backup.
+   * @param {number} backupIndex Number of the backup file (1 being the most
+   *   recent).
+   * @returns {string} Backup file name.
    */
   getBackupName(backupIndex)
   {
     let [name, extension] = this.sourceFile.split(".", 2);
     return (name + "-backup" + backupIndex + "." + extension);
-  },
+  }
 
   /**
    * Restores an automatically created backup.
-   * @param {number} backupIndex
-   *    number of the backup to restore (1 being the most recent)
-   * @param {boolean} silent
-   *    If true, no "load" notification will be sent out.
-   * @return {Promise} promise resolved or rejected when restoring is complete
+   * @param {number} backupIndex Number of the backup to restore (1 being the
+   *   most recent).
+   * @param {boolean} silent If <code>true</code>, no "load" notification will
+   *   be sent out.
+   * @returns {Promise} A promise resolved or rejected when restoration is
+   *   complete.
    */
   restoreBackup(backupIndex, silent)
   {
     let backupFile = this.getBackupName(backupIndex);
     let parser = this.importData(silent);
     return IO.readFromFile(backupFile, parser).then(() =>
     {
       parser(null);
       return this.saveToDisk();
     });
-  },
+  }
 
   /**
    * Generator serializing filter data and yielding it line by line.
+   * @yields {string}
    */
   *exportData()
   {
     yield "# Adblock Plus preferences";
-    yield "version=" + formatVersion;
+    yield "version=" + this.formatVersion;
 
     let saved = new Set();
-    let buf = [];
 
     // Save subscriptions
     for (let subscription of this.subscriptions())
     {
       // Do not persist external subscriptions
-      if (subscription instanceof ExternalSubscription)
-        continue;
-
-      yield "";
-      subscription.serialize(buf);
-
-      if (subscription.filters.length)
-      {
-        buf.push("", "[Subscription filters]");
-        subscription.serializeFilters(buf);
-      }
-
-      for (let line of buf)
-        yield line;
-
-      buf.splice(0);

[Sebastian Noack, 2018/10/03 16:33:24]

While optimizing this code, you might want consider using `buf = []` which is
used to be much faster than splice(). I didn't benchmark it recently though, so
you might want to double check on recent browser versions.

-
-      // Save filter data
-      for (let filter of subscription.filters)
-      {
+      if (!(subscription instanceof ExternalSubscription) &&
+          !(subscription instanceof SpecialSubscription &&
+            subscription.filters.length == 0))
+      {
+        yield* subscription.serialize();
+        yield* subscription.serializeFilters();
+      }
+
+    // Save filter data
+    for (let subscription of subscriptions)
+    {
+      for (let filter of subscription.filters())
+      {
+        // Save filter data
         if (!saved.has(filter.text))
         {
-          filter.serialize(buf);
+          yield* filter.serialize();
           saved.add(filter.text);
-          for (let line of buf)
-            yield line;
-          buf.splice(0);
         }
       }
     }
-  },
-
-  /**
-   * Will be set to true if saveToDisk() is running (reentrance protection).
-   * @type {boolean}
-   */
-  _saving: false,
-
-  /**
-   * Will be set to true if a saveToDisk() call arrives while saveToDisk() is
-   * already running (delayed execution).
-   * @type {boolean}
-   */
-  _needsSave: false,
-
-  /**
-   * Saves all subscriptions back to disk
-   * @return {Promise} promise resolved or rejected when saving is complete
+  }
+
+  /**
+   * Saves all subscriptions back to disk.
+   * @returns {Promise} A promise resolved or rejected when saving is complete.
    */
   saveToDisk()
   {
     if (this._saving)
     {
       this._needsSave = true;
       return;
     }
 
     this._saving = true;
@@ skipping 63 matching lines @@
       Cu.reportError(error);
     }).then(() =>
     {
       this._saving = false;
       if (this._needsSave)
       {
         this._needsSave = false;
         this.saveToDisk();
       }
     });
-  },
+  }
 
   /**
    * @typedef FileInfo
    * @type {object}
    * @property {number} index
    * @property {number} lastModified
    */
 
   /**
    * Returns a promise resolving in a list of existing backup files.
-   * @return {Promise.<FileInfo[]>}
+   * @returns {Promise.<Array.<FileInfo>>}
    */
   getBackupFiles()
   {
     let backups = [];
 
     let checkBackupFile = index =>
     {
       return IO.statFile(this.getBackupName(index)).then(statData =>
       {
         if (!statData.exists)
           return backups;
 
         backups.push({
           index,
           lastModified: statData.lastModified
         });
         return checkBackupFile(index + 1);
       }).catch(error =>
       {
         // Something went wrong, return whatever data we got so far.
         Cu.reportError(error);
         return backups;
       });
     };
 
     return checkBackupFile(1);
   }
-};
+}
 
 /**
- * Joins subscription's filters to the subscription without any notifications.
- * @param {Subscription} subscription
- *   filter subscription that should be connected to its filters
+ * Reads the user's filters from disk, manages them in memory, and writes them
+ * back to disk.
  */
-function addSubscriptionFilters(subscription)
+let filterStorage = new FilterStorage();
+
+exports.filterStorage = filterStorage;
+
+/**
+ * Connects a subscription to its filters without any notifications.
+ * @param {Subscription} subscription The subscription that should be
+ *   connected to its filters.
+ * @param {?Array.<Filter>} [filters] A list of filters to which the
+ *   subscription should be connected. If this is not given, the subscription
+ *   is connected to its own filters.
+ */
+function connectSubscriptionFilters(subscription, filters)
 {
-  if (!FilterStorage.knownSubscriptions.has(subscription.url))
+  if (!filterStorage.knownSubscriptions.has(subscription.url))
     return;
 
-  for (let filter of subscription.filters)
+  for (let filter of filters || subscription.filters())
     filter.addSubscription(subscription);
 }
 
 /**
- * Removes subscription's filters from the subscription without any
- * notifications.
- * @param {Subscription} subscription filter subscription to be removed
+ * Disconnects a subscription from its filters without any notifications.
+ * @param {Subscription} subscription The subscription that should be
+ *   disconnected from its filters.
+ * @param {?Array.<Filter>} [filters] A list of filters from which the
+ *   subscription should be disconnected. If this is not given, the
+ *   subscription is disconnected from its own filters.
  */
-function removeSubscriptionFilters(subscription)
+function disconnectSubscriptionFilters(subscription, filters)
 {
-  if (!FilterStorage.knownSubscriptions.has(subscription.url))
+  if (!filterStorage.knownSubscriptions.has(subscription.url))
     return;
 
-  for (let filter of subscription.filters)
+  for (let filter of filters || subscription.filters())
     filter.removeSubscription(subscription);
 }