Issue 7094 - Encapsulate management of subscription filters

lib/subscriptionClasses.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 22 matching lines @@
  * @param {string} [title]  title of the filter subscription
  * @constructor
  */
 function Subscription(url, title)
 {
   this.url = url;
 
   this._filterText = [];
   this._filters = [];
 
-  this._filterTextSet = new Set();
-
   if (title)
     this._title = title;
 
   Subscription.knownSubscriptions.set(url, this);
 }
 exports.Subscription = Subscription;
 
 Subscription.prototype =
 {
   /**
    * Download location of the subscription
    * @type {string}
    */
   url: null,
 
   /**
    * Type of the subscription
    * @type {?string}
    */
   type: null,
 
   /**
    * Filter text contained in the filter subscription.
    * @type {Array.<string>}
    * @private
    */
   _filterText: null,

[hub, 2018/11/19 17:47:49]

shouldn't this be named `_filtersText` since it is many filters?

Also would be consistent with `_filters`

[Manish Jethani, 2018/11/20 00:19:39]

I thought about calling it _filtersText but that sounds wrong in English. Maybe
_filterTextList or _filterTextArray? Though I find it clear enough to just call
it _filterText, since text is both singular and plural and it maps nicely to the
generator function filterText()

[hub, 2018/11/20 19:30:19]

my bad. let's leave it at that.

 
   /**
    * {@link Filter} objects corresponding to the subscription's filter text.
    * @type {Array.<Filter>}
    * @private
    */
   _filters: null,
-
-  /**
-   * Set of filter text contained in the filter subscription, used for faster
-   * lookup.
-   * @type {Set.<string>}
-   * @private
-   */
-  _filterTextSet: null,
 
   _title: null,
   _fixedTitle: false,
   _disabled: false,
 
   /**
    * Title of the filter subscription
    * @type {string}
    */
   get title()
@@ skipping 87 matching lines @@
   },
 
   /**
    * Returns the 0-based index of the given filter.
    * @param {Filter} filter
    * @param {number} [fromIndex] The index from which to start the search.
    * @return {number}
    */
   searchFilter(filter, fromIndex = 0)
   {
     return this._filterText.indexOf(filter.text, fromIndex);

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

Note: We look up a filter always by the text, not by the filter object, which
can be temporary and so the references can keep changing.

   },
 
   /**
-   * Checks whether the subscription contains the given filter.
-   * @param {Filter} filter
-   * @return {boolean}
-   */
-  hasFilter(filter)

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

We will use this in this patch: https://codereview.adblockplus.org/29935568/

-  {
-    return this._filterTextSet.has(filter.text);
-  },
-
-  /**
    * Removes all filters from the subscription.
    */
   clearFilters()
   {
     this._filterText = [];
     this._filters = [];
-
-    this._filterTextSet.clear();
   },
 
   /**
    * Adds a filter to the subscription.
    * @param {Filter} filter
    */
   addFilter(filter)
   {
     this._filterText.push(filter.text);
     this._filters.push(filter);
-
-    this._filterTextSet.add(filter.text);
   },
 
   /**
    * Inserts a filter into the subscription.
    * @param {Filter} filter
    * @param {number} index The index at which to insert the filter.
    */
   insertFilterAt(filter, index)
   {
     this._filterText.splice(index, 0, filter.text);
     this._filters.splice(index, 0, filter);
-
-    this._filterTextSet.add(filter.text);
   },
 
   /**
    * Deletes a filter from the subscription.
    * @param {number} index The index at which to delete the filter.
    */
   deleteFilterAt(index)
   {
-    let items = this._filterText.splice(index, 1);
-
+    // Ignore index if out of bounds on the negative side, for consistency.
+    if (index < 0)
+      return;
+
+    this._filterText.splice(index, 1);
     this._filters.splice(index, 1);
-
-    // Performance note: A subscription can contain the same filter multiple
-    // times. We can only delete the text from the set if all occurrences of
-    // the filter have been removed. This makes deletion an expensive
-    // operation. It is only needed when a user-defined filter is removed,
-    // which is relatively rare. In comparison, filters must be looked up in
-    // subscriptions a lot more often; therefore, it makes sense to maintain
-    // the set at this cost. An alternative here would be to use a map instead
-    // of a set, with the value of each entry being the reference count, but
-    // this would make addition more expensive thus slowing down the loading of
-    // the initial subscriptions.
-    if (items.length > 0 && this._filterText.indexOf(items[0]) == -1)
-      this._filterTextSet.delete(items[0]);
   },
 
   /**
    * Serializes the subscription for writing out on disk.
    * @yields {string}
    */
   *serialize()
   {
     let {url, type, _title, _fixedTitle, _disabled} = this;
 
@@ skipping 447 matching lines @@
     if (errors)
       yield "errors=" + errors;
     if (version)
       yield "version=" + version;
     if (requiredVersion)
       yield "requiredVersion=" + requiredVersion;
     if (downloadCount)
       yield "downloadCount=" + downloadCount;
   }
 });