Issue 6856 - Remove FilterStorage.moveSubscription

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
@@ skipping 94 matching lines @@
 
   /**
    * 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())

[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.

+    for (let subscription of FilterStorage.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))
@@ skipping 237 matching lines @@
   /**
    * Loads all subscriptions from the disk.
    * @return {Promise} promise resolved or rejected when loading is complete
    */
   loadFromDisk()
   {
     let tryBackup = backupIndex =>
     {
       return this.restoreBackup(backupIndex, true).then(() =>
       {
-        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.

+        if (this.knownSubscriptions.size == 0)
           return tryBackup(backupIndex + 1);
       }).catch(error =>
       {
         // Give up
       });
     };
 
     return IO.statFile(this.sourceFile).then(statData =>
     {
       if (!statData.exists)
       {
         this.firstRun = true;
         return;
       }
 
       let parser = this.importData(true);
       return IO.readFromFile(this.sourceFile, parser).then(() =>
       {
         parser(null);
-        if (this.subscriptionCount == 0)
+        if (this.knownSubscriptions.size == 0)
         {
           // No filter subscriptions in the file, this isn't right.
           throw new Error("No data in the file");
         }
       });
     }).catch(error =>
     {
       Cu.reportError(error);
       return tryBackup(1);
     }).then(() =>
@@ skipping 33 matching lines @@
       return this.saveToDisk();
     });
   },
 
   /**
    * Generator serializing filter data and yielding it line by line.
    */
   *exportData()
   {
     // Do not persist external subscriptions
-    let subscriptions = [...this.subscriptions()].filter(
-      s => !(s instanceof ExternalSubscription)
-    );
+    let subscriptions = [];
+    for (let subscription of this.subscriptions())
+    {
+      if (!(subscription instanceof ExternalSubscription))
+        subscriptions.push(subscription);
+    }
 
     yield "# Adblock Plus preferences";
     yield "version=" + formatVersion;
 
     let saved = new Set();
     let buf = [];
 
     // Save subscriptions
     for (let subscription of subscriptions)
     {
@@ skipping 187 matching lines @@
  * @param {Subscription} subscription filter subscription to be removed
  */
 function removeSubscriptionFilters(subscription)
 {
   if (!FilterStorage.knownSubscriptions.has(subscription.url))
     return;
 
   for (let filter of subscription.filters)
     filter.removeSubscription(subscription);
 }