Issue 6957 - Add generateStyleSheetForDomain function to ElemHide module

test/elemHide.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 27 matching lines @@
       sandboxedRequire("../lib/elemHide"),
     {ElemHideExceptions} = sandboxedRequire("../lib/elemHideExceptions"),
     {Filter} = sandboxedRequire("../lib/filterClasses")
   );
 
   callback();
 };
 
 function normalizeSelectors(selectors)
 {
-  // getSelectorsForDomain is currently allowed to return duplicate selectors
-  // for performance reasons, so we need to remove duplicates here.
+  // generateStyleSheetForDomain is currently allowed to return duplicate
+  // selectors for performance reasons, so we need to remove duplicates here.
   return selectors.slice().sort().filter((selector, index, sortedSelectors) =>

[Manish Jethani, 2018/09/20 12:27:24]

Somewhat unrelated, but we should make a copy and sort the copy, otherwise it
might be confusing if the tests starts to fail because the caller doesn't know
that the array is being modified.

[Jon Sonesen, 2018/09/20 20:10:20]

Acknowledged.

Just curious, since slice returns a shallow copy are there still existing
references to the duplicates in the original selectors array and does that
impact performance?

[Manish Jethani, 2018/09/21 10:53:05]

The selectors themselves are immutable string objects (strings in JS are
immutable), so it should be OK to make a shallow copy.

Make a copy itself would affect performance slightly (whatever it costs to make
a copy), but over here it doesn't matter since it's a unit test.

[Jon Sonesen, 2018/09/23 18:01:59]

Yeah I figured since it is test code the importance is minimal also, thanks for
explaining :)

   {
     return index == 0 || selector != sortedSelectors[index - 1];
   });
 }
 
 function testResult(test, domain, expectedSelectors, specificOnly)
 {
   let normalizedExpectedSelectors = normalizeSelectors(expectedSelectors);
-
-  test.deepEqual(
-    normalizeSelectors(ElemHide.getSelectorsForDomain(domain, specificOnly)),
-    normalizedExpectedSelectors
-  );
 
   let {code, selectors} =
     ElemHide.generateStyleSheetForDomain(domain, specificOnly);
 
   test.deepEqual(normalizeSelectors(selectors), normalizedExpectedSelectors);
 
   // Make sure each expected selector is in the actual CSS code.
   for (let selector of normalizedExpectedSelectors)
   {
     test.ok(code.includes(selector + ", ") ||
             code.includes(selector + " {display: none !important;}\n"));
   }

[Jon Sonesen, 2018/09/20 20:10:20]

I am sort of confused by the set literal `{code, selectors}` Do is hold two
Boolean values which are the result of first the deep equal test and then the
test.ok?

Other than wanting the clarification, can the for loop do without the curly
braces? Or is the reasoning behind them that the test line is broken up so the
braces help encapsulate it?

[Manish Jethani, 2018/09/21 10:53:05]

This is called a destructuring assignment [1]

[1]:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/D...

Essentially the function returns an object containing `code` and `selectors`
properties, and we directly extract those properties into local variables.
There's no technical reason to use the braces, but Dave made a "rule" (more like
a convention, it's not an ESLint rule) that if a block spans multiple lines then
it should be surrounded by braces. I'm just sticking to this convention. I
personally don't care too much about this.

[Jon Sonesen, 2018/09/23 18:01:59]

I do think the convention is helpful tbh so I was curious, perhaps it could be
added to the eslint-eyeo-config package? I think it's easy enough to remember
but would be nice to have my linter plugin tell me hehe. 

 }
 
-exports.testGetSelectorsForDomain = function(test)
+exports.testGenerateStyleSheetForDomain = function(test)
 {
   let addFilter = filterText => ElemHide.add(Filter.fromText(filterText));
   let removeFilter = filterText => ElemHide.remove(Filter.fromText(filterText));
   let addException =
     filterText => ElemHideExceptions.add(Filter.fromText(filterText));
   let removeException =
     filterText => ElemHideExceptions.remove(Filter.fromText(filterText));
 
   testResult(test, "", []);
 
@@ skipping 214 matching lines @@
 
   let selectors = new Array(50000).map((element, index) => ".s" + index);
 
   test.equal((createStyleSheet(selectors).match(/\n/g) || []).length,
              Math.ceil(50000 / selectorGroupSize),
              "Style sheet should be split up into rules with at most " +
              selectorGroupSize + " selectors each");
 
   test.done();
 };