Issue 6856 - Remove FilterStorage.moveSubscription

lib/filterStorage.js

Index: lib/filterStorage.js
===================================================================
--- a/lib/filterStorage.js
+++ b/lib/filterStorage.js
@@ -75,37 +75,49 @@
 
   /**
    * 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),
 
   /**
-   * List of filter subscriptions containing all filters
-   * @type {Subscription[]}
+   * Yields subscriptions containing all filters
+   * @yields {Subscription}
    */
-  subscriptions: [],
+  *subscriptions()
+  {
+    yield* this.knownSubscriptions.values();
+  },
+
+  /**
+   * Number of known subscriptions.
+   * @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.
    * @param {Filter} filter
    * @return {?SpecialSubscription}
    */
   getGroupForFilter(filter)
   {
     let generalSubscription = null;
-    for (let subscription of FilterStorage.subscriptions)
+    for (let subscription of FilterStorage.subscriptions())

[Manish Jethani, 2018/10/01 10:40:45]

Since this is within the FilterStorage object, maybe it's better to directly
access the knownSubscriptions object here? (It is private to the object, but
code within the object should be able to access it directly)

So like `FilterStorage.knownSubscriptions.values()` instead of
`FilterStorage.subscriptions()` is what I was thinking.

[Jon Sonesen, 2018/10/01 15:40:57]

Yeah, I think you are right. I had the same thought while writing this but
figured to just stick with the previous convention to see what happened in
review. Also, convention was my only argument for not changing. Which kinda
makes less sense now. Although (and I hadn't thought of this previously) are we
hurting ourselves performance wise?
As far as I have read lists are much faster for access and iteration, is this
wrong?

[Manish Jethani, 2018/10/01 16:12:52]

I'm not sure, but it shouldn't hurt if we just avoid an additional function call
either, unless doing so makes the code significantly less readable.
For random access, yes of course. For iteration, probably, although I'm not sure
how much in practice. For subscription lists, when I tested things out for #6916
[1], it seems iteration had become ~25% slower when it was a generator. We may
have a similar impact here, but adjusted for the savings in memory and
simplification of the code I think it's worth it.

[1]: https://issues.adblockplus.org/ticket/6916#Performanceimpact

[Jon Sonesen, 2018/10/02 16:23:48]

Done.

     {
       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
@@ -123,75 +135,40 @@
    * Adds a filter subscription to the list
    * @param {Subscription} subscription filter subscription to be added
    */
   addSubscription(subscription)
   {
     if (FilterStorage.knownSubscriptions.has(subscription.url))
       return;
 
-    FilterStorage.subscriptions.push(subscription);
     FilterStorage.knownSubscriptions.set(subscription.url, subscription);
     addSubscriptionFilters(subscription);
 
     filterNotifier.emit("subscription.added", subscription);
   },
 
   /**
    * Removes a filter subscription from the list
    * @param {Subscription} subscription filter subscription to be removed
    */
   removeSubscription(subscription)
   {
-    for (let i = 0; i < FilterStorage.subscriptions.length; i++)
-    {
-      if (FilterStorage.subscriptions[i].url == subscription.url)
-      {
-        removeSubscriptionFilters(subscription);
-
-        FilterStorage.subscriptions.splice(i--, 1);
-        FilterStorage.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);
-        return;
-      }
-    }
-  },
-
-  /**
-   * Moves a subscription in the list to a new position.
-   * @param {Subscription} subscription filter subscription to be moved
-   * @param {Subscription} [insertBefore] filter subscription to insert before
-   *        (if omitted the subscription will be put at the end of the list)
-   */
-  moveSubscription(subscription, insertBefore)
-  {
-    let currentPos = FilterStorage.subscriptions.indexOf(subscription);
-    if (currentPos < 0)
+    if (!FilterStorage.knownSubscriptions.has(subscription.url))
       return;
 
-    let newPos = -1;
-    if (insertBefore)
-      newPos = FilterStorage.subscriptions.indexOf(insertBefore);
+    removeSubscriptionFilters(subscription);
 
-    if (newPos < 0)
-      newPos = FilterStorage.subscriptions.length;
+    FilterStorage.knownSubscriptions.delete(subscription.url);
 
-    if (currentPos < newPos)
-      newPos--;
-    if (currentPos == newPos)
-      return;
+    // This should be the last remaining reference to the Subscription
+    // object.
+    Subscription.knownSubscriptions.delete(subscription.url);
 
-    FilterStorage.subscriptions.splice(currentPos, 1);
-    FilterStorage.subscriptions.splice(newPos, 0, subscription);
-    filterNotifier.emit("subscription.moved", subscription);
+    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
    */
   updateSubscriptionFilters(subscription, filters)
@@ -368,17 +345,16 @@
       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.subscriptions = parser.subscriptions;

[Manish Jethani, 2018/10/01 10:53:13]

Note: An implication of this change is that a patterns.ini (the filter that
contains the subscriptions serialized to disk) with duplicate subscriptions will
be read as having only one subscription, the first occurrence. I have to think
about the implications of this, but I'll make a note in the Trac ticket.

[Jon Sonesen, 2018/10/01 15:40:57]

Acknowledged.

         this.knownSubscriptions = knownSubscriptions;
         Filter.knownFilters = parser.knownFilters;
         Subscription.knownSubscriptions = parser.knownSubscriptions;
 
         if (!silent)
           filterNotifier.emit("load");
       }
     };
@@ -389,17 +365,17 @@
    * @return {Promise} promise resolved or rejected when loading is complete
    */
   loadFromDisk()
   {
     let tryBackup = backupIndex =>
     {
       return this.restoreBackup(backupIndex, true).then(() =>
       {
-        if (this.subscriptions.length == 0)
+        if (this.subscriptionCount == 0)

[Manish Jethani, 2018/10/01 10:40:45]

Here (and below) too I think it's probably better to just access
`this.knownSubscriptions.size` directly.

[Jon Sonesen, 2018/10/01 15:40:57]

Acknowledged.

[Jon Sonesen, 2018/10/02 16:23:48]

Done.

           return tryBackup(backupIndex + 1);
       }).catch(error =>
       {
         // Give up
       });
     };
 
     return IO.statFile(this.sourceFile).then(statData =>
@@ -409,17 +385,17 @@
         this.firstRun = true;
         return;
       }
 
       let parser = this.importData(true);
       return IO.readFromFile(this.sourceFile, parser).then(() =>
       {
         parser(null);
-        if (this.subscriptions.length == 0)
+        if (this.subscriptionCount == 0)
         {
           // No filter subscriptions in the file, this isn't right.
           throw new Error("No data in the file");
         }
       });
     }).catch(error =>
     {
       Cu.reportError(error);
@@ -463,17 +439,17 @@
   },
 
   /**
    * Generator serializing filter data and yielding it line by line.
    */
   *exportData()
   {
     // Do not persist external subscriptions
-    let subscriptions = this.subscriptions.filter(
+    let subscriptions = [...this.subscriptions()].filter(
       s => !(s instanceof ExternalSubscription)
     );
 
     yield "# Adblock Plus preferences";
     yield "version=" + formatVersion;
 
     let saved = new Set();
     let buf = [];