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.

+
+/**
+ * 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.

+
+/**
+ * CSS rules component
+ * @class
+ */
+let CSSRules = exports.CSSRules =
+{
+  /**
+   * Removes all known filters
+   */
+  clear: function()
+  {
+    filterByKey = Object.create(null);
+    keyByFilter = 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;
+  },
+
+  /**
+   * 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];
+  },
+
+  /**
+   * 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.

+   */
+  getRulesForDomain: function(domain, specificOnly)
+  {
+    let result = [];
+    let keys = Object.getOwnPropertyNames(filterByKey);
+    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.

+    }

[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;
+  }
+};