Issue 4067 - Make filter "keys" generated for element hiding filters numeric

Issue 4067 - Make filter "keys" generated for element hiding filters numeric

Repository: hg.adblockplus.org/adblockpluscore/

[kzar, 2016-05-24 12:36:49]

https://codereview.adblockplus.org/29342974/diff/29342975/lib/elemHide.js
File lib/elemHide.js (right):

https://codereview.adblockplus.org/29342974/diff/29342975/lib/elemHide.js#new...
lib/elemHide.js:333: if (!selector)
I'm curious why you added this check? Wont all element hiding filters have a
selector?

https://codereview.adblockplus.org/29342974/diff/29342975/lib/filterListener.js
File lib/filterListener.js (right):

https://codereview.adblockplus.org/29342974/diff/29342975/lib/filterListener....
lib/filterListener.js:238: for (let i = 0; i < len; i++, current = (current +
step) % len)
I didn't intuitively understand that this should work, so I tried it out on a
piece of paper. Unless I'm missing something it's not working right, I keep
getting the filters back in sequential order, just starting from a random index?

Take this example:

selectors: ["a", "b", "c", "d", "e", "f"]
len: 6
prime: 61
current: 3

3                  = 3
(3 + 61) % 6       = 4
(4 + 61) % 6       = 5
(5 + 61) % 6       = 0
(0 + 61) % 6       = 1
(1 + 61) % 6       = 2

[Wladimir Palant, 2016-05-24 12:49:50]

https://codereview.adblockplus.org/29342974/diff/29342975/lib/elemHide.js
File lib/elemHide.js (right):

https://codereview.adblockplus.org/29342974/diff/29342975/lib/elemHide.js#new...
lib/elemHide.js:333: if (!selector)
On 2016/05/24 12:36:49, kzar wrote:
> I'm curious why you added this check? Wont all element hiding filters have a
> selector?

From
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/...:

> The for...in loop statement will return all enumerable properties, including
those with non–integer names and those that are inherited.

Luckily, array properties like join() aren't enumerable and shouldn't show up
here. Still, I thought it's a good idea to verify that we actually have a filter
here, not some method.

https://codereview.adblockplus.org/29342974/diff/29342975/lib/filterListener.js
File lib/filterListener.js (right):

https://codereview.adblockplus.org/29342974/diff/29342975/lib/filterListener....
lib/filterListener.js:238: for (let i = 0; i < len; i++, current = (current +
step) % len)
On 2016/05/24 12:36:49, kzar wrote:
> Take this example:
> 
> selectors: ["a", "b", "c", "d", "e", "f"]
> len: 6
> prime: 61
> current: 3

That's because 61%6 happens to be 1. On the other hand, 47%6 is 5 so with this
prime you will get reverse order (3, 2, 1, 0, 5, 4). Either way, this is only an
issue if the array length is smaller than the prime used which shouldn't
normally be the case - our filter lists are considerably larger than the primes
used here.

So e.g. with len 7 and prime 3 you get (3, 6, 2, 5, 1, 4, 0).

[kzar, 2016-05-24 12:57:45]

LGTM

https://codereview.adblockplus.org/29342974/diff/29342975/lib/elemHide.js
File lib/elemHide.js (right):

https://codereview.adblockplus.org/29342974/diff/29342975/lib/elemHide.js#new...
lib/elemHide.js:333: if (!selector)
On 2016/05/24 12:49:50, Wladimir Palant wrote:
> On 2016/05/24 12:36:49, kzar wrote:
> > I'm curious why you added this check? Wont all element hiding filters have a
> > selector?
> 
> From
>
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/...:
> 
> > The for...in loop statement will return all enumerable properties, including
> those with non–integer names and those that are inherited.
> 
> Luckily, array properties like join() aren't enumerable and shouldn't show up
> here. Still, I thought it's a good idea to verify that we actually have a
filter
> here, not some method.

Acknowledged.

https://codereview.adblockplus.org/29342974/diff/29342975/lib/filterListener.js
File lib/filterListener.js (right):

https://codereview.adblockplus.org/29342974/diff/29342975/lib/filterListener....
lib/filterListener.js:238: for (let i = 0; i < len; i++, current = (current +
step) % len)
On 2016/05/24 12:49:50, Wladimir Palant wrote:
> On 2016/05/24 12:36:49, kzar wrote:
> > Take this example:
> > 
> > selectors: ["a", "b", "c", "d", "e", "f"]
> > len: 6
> > prime: 61
> > current: 3
> 
> That's because 61%6 happens to be 1. On the other hand, 47%6 is 5 so with this
> prime you will get reverse order (3, 2, 1, 0, 5, 4). Either way, this is only
an
> issue if the array length is smaller than the prime used which shouldn't
> normally be the case - our filter lists are considerably larger than the
primes
> used here.
> 
> So e.g. with len 7 and prime 3 you get (3, 6, 2, 5, 1, 4, 0).

Acknowledged.