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 179 matching lines @@
     filterNotifier.emit("subscription.removed", subscription);
   }
 
   /**
    * 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()];
+    disconnectSubscriptionFilters(subscription, oldFilters);
     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 34 matching lines @@
    * @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;
@@ skipping 402 matching lines @@
  *   subscription is disconnected from its own filters.
  */
 function disconnectSubscriptionFilters(subscription, filters)
 {
   if (!filterStorage.knownSubscriptions.has(subscription.url))
     return;
 
   for (let filter of filters || subscription.filters())
     filter.removeSubscription(subscription);
 }