Issue 2392/2393 - Created container for CSS property filters

lib/cssRules.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2015 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
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 /**
  * @fileOverview CSS property filtering implementation.
  */
 
-let {ElemHide: {getException}} = require("elemHide");

[kzar, 2015/10/24 17:14:51]

This results in a syntax error when building the script into adblockplus.js for
the Chrome extension. Changing it to `let {ElemHide} = require("elemHide");` and
later `ElemHide.getException(...` fixed it for me.

[Thomas Greiner, 2015/11/25 11:20:00]

Done. Since this is valid ES6 syntax, I'd consider this to be an issue with
JSHydra. However, I agree that for now I can make the change that you proposed
to avoid complications.

[kzar, 2015/11/25 13:12:26]

Thanks, yea I agree that it looks like more of a problem with JSHydra than your
code.

+let {ElemHide} = require("elemHide");
+let {Filter} =  require("filterClasses");
 
-/**
- * Lookup table, filters by their associated key
- * @type Object
- */
-let filterByKey = Object.create(null);
-
-/**
- * Lookup table, keys of the filters by filter text
- * @type Object
- */
-let keyByFilter = Object.create(null);

[Wladimir Palant, 2015/11/05 12:46:18]

The two maps filterByKey and keyByFilter seem to be completely unnecessary
overhead here - unlike with usual element hiding rules, the filters are never
referenced by key. If the point is merely excluding duplicate filters then a
single map filters would be sufficient (mapping filter text to true). Note that
actually storing the filter objects is unnecessary, these can be retrieved via
Filter.fromText() when needed.

[Thomas Greiner, 2015/11/25 11:20:00]

You're absolutely right.

I'd love to use `Set` for that since that's basically all the functionality
that's needed, except for the `getRulesForDomain` function, of course. However,
it's not supported on most platforms yet so yeah, an Object it is then.

+let filters = Object.create(null);
 
 /**
  * CSS rules component
  * @class
  */
 let CSSRules = exports.CSSRules =
 {
   /**
    * Removes all known filters
    */
   clear: function()
   {
-    filterByKey = Object.create(null);
-    keyByFilter = Object.create(null);
+    filters = Object.create(null);
   },
 
   /**
    * Add a new CSS property filter
    * @param {CSSPropertyFilter} filter
    */
   add: function(filter)
   {
-    if (filter.text in keyByFilter)
-      return;
-
-    let key;
-    do {
-      key = Math.random().toFixed(15).substr(5);
-    } while (key in filterByKey);
-
-    filterByKey[key] = filter;
-    keyByFilter[filter.text] = key;
+    filters[filter.text] = true;
   },
 
   /**
    * Removes a CSS property filter
    * @param {CSSPropertyFilter} filter
    */
   remove: function(filter)
   {
-    if (!(filter.text in keyByFilter))
-      return;
-
-    let key = keyByFilter[filter.text];
-    delete filterByKey[key];
-    delete keyByFilter[filter.text];
+    delete filters[filter.text];
   },
 
   /**
    * Returns a list of all rules active on a particular domain
    * @param {String} domain
-   * @param {Boolean} specificOnly

[kzar, 2015/11/04 13:03:20]

Aren't CSS property filters _always_ specific? We enforce that they have a
domain as they're inefficient right?
https://github.com/adblockplus/adblockplus/blob/master/lib/filterClasses.js#L933

[Thomas Greiner, 2015/11/25 11:20:00]

Done.

+   * @return {Array.<CSSPropertyFilter>}

[Wladimir Palant, 2015/11/26 12:52:46]

Nit: We usually use CSSPropertyFilter[] which is shorter and more like
JavaScript. For sets or maps the awkward template syntax isn't avoidable, here
it is.

[Thomas Greiner, 2015/11/30 13:51:16]

Done. Will include that change in the commit.

    */
-  getRulesForDomain: function(domain, specificOnly)
+  getRulesForDomain: function(domain)
   {
     let result = [];
-    let keys = Object.getOwnPropertyNames(filterByKey);
+    let keys = Object.getOwnPropertyNames(filters);
     for (let key of keys)
     {
-      let filter = filterByKey[key];
-      if (specificOnly && (!filter.domains || filter.domains[""]))

[kzar, 2015/11/02 19:33:45]

Use filter.isGeneric() instead?

[Wladimir Palant, 2015/11/05 12:46:19]

Actually, filters that are generic shouldn't even be added in the first place.
These should be rejected by the filter parser already so that the filter type
would be InvalidFilter.

[kzar, 2015/11/05 14:27:47]

Yea, I noticed that later, see my above comment :p

-        continue;
-
-      if (filter.isActiveOnDomain(domain) && !getException(filter, domain))
-        result.push(filter.selector);

[kzar, 2015/11/02 19:33:45]

Mind just pushing `filter` instead of `filter.selector`? It would be useful to
have access to `filter.regexpString`, `filter.selectorPrefix` and
`filter.selectorSuffix` for the message responder in #2396.

[Thomas Greiner, 2015/11/25 11:20:00]

Done. I must've overseen that those are required as well, thanks.

+      let filter = Filter.fromText(key);
+      if (filter.isActiveOnDomain(domain) && !ElemHide.getException(filter, domain))
+        result.push(filter);
     }

[Wladimir Palant, 2015/11/05 12:46:18]

Note: This loop is rather inefficient but that's ok for now - we don't expect
too many of these rules. We can make it more efficient when we make the ElemHide
code more efficient (https://issues.adblockplus.org/ticket/235).

     return result;
   }
 };