Issue 7094 - Encapsulate management of subscription filters

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 180 matching lines @@
   }
 
   /**
    * 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)
   {
     disconnectSubscriptionFilters(subscription);
-    let oldFilters = subscription.filters;
-    subscription.filters = filters;
-    connectSubscriptionFilters(subscription);
+
+    let oldFilters = [...subscription.filters()];
+    subscription.clearFilters();
+    for (let filter of filters)
+      subscription.addFilter(filter);
+
+    connectSubscriptionFilters(subscription, filters);

[Manish Jethani, 2018/11/17 21:41:09]

The reason we're passing the filters array to the method here is because we
don't know if the subscription will hold on to the filter objects. Right now it
does hold on to them, but in subsequent patches we'll make some of the objects
"ephemeral" so the subscription will simply keep the text and discard the
object.

     filterNotifier.emit("subscription.updated", subscription, oldFilters);
   }
 
   /**
    * Adds a user-defined filter to the storage.
    * @param {Filter} filter
    * @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
@@ skipping 15 matching lines @@
     }
     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 storage.
    * @param {Filter} filter
    * @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 = (

[Manish Jethani, 2018/11/17 21:41:09]

Note: Both removeFilter() and moveFilter() can probably rewritten in a better
way, but I want to minimize the changes in this patch. Much of the code here
should actually be in the Subscription class, now that we're encapsulating
everything, but it is not strictly necessary for what we're trying to accomplish
with these changes.

       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.
    * @param {Filter} 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)
-    {
+    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.
    * @param {Filter} filter
    */
   increaseHitCount(filter)
   {
@@ skipping 141 matching lines @@
    * @yields {string}
    */
   *exportData()
   {
     // Do not persist external subscriptions
     let subscriptions = [];
     for (let subscription of this.subscriptions())
     {
       if (!(subscription instanceof ExternalSubscription) &&
           !(subscription instanceof SpecialSubscription &&
-            subscription.filters.length == 0))
+            subscription.filterCount == 0))
       {
         subscriptions.push(subscription);
       }
     }
 
     yield "# Adblock Plus preferences";
     yield "version=" + this.formatVersion;
 
     let saved = new Set();
 
     // Save subscriptions
     for (let subscription of subscriptions)
     {
       yield* subscription.serialize();
       yield* subscription.serializeFilters();
     }
 
     // Save filter data
     for (let subscription of subscriptions)
     {
-      for (let filter of subscription.filters)
+      for (let filter of subscription.filters())
       {
         if (!saved.has(filter.text))
         {
           yield* filter.serialize();
           saved.add(filter.text);
         }
       }
     }
   }
 
@@ skipping 129 matching lines @@
  * back to disk.
  */
 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)
+function connectSubscriptionFilters(subscription, filters)
 {
   if (!filterStorage.knownSubscriptions.has(subscription.url))
     return;
 
-  for (let filter of subscription.filters)
+  for (let filter of filters || subscription.filters())
     filter.addSubscription(subscription);
 }
 
 /**
  * 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 disconnectSubscriptionFilters(subscription)
+function disconnectSubscriptionFilters(subscription, filters)
 {
   if (!filterStorage.knownSubscriptions.has(subscription.url))
     return;
 
-  for (let filter of subscription.filters)
+  for (let filter of filters || subscription.filters())
     filter.removeSubscription(subscription);
 }