Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(345)

Issue 29342974: Issue 4067 - Make filter "keys" generated for element hiding filters numeric (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 6 months ago by Wladimir Palant
Modified:
3 years, 6 months ago
Reviewers:
kzar
Visibility:
Public.

Description

Issue 4067 - Make filter "keys" generated for element hiding filters numeric Repository: hg.adblockplus.org/adblockpluscore/

Patch Set 1 #

Total comments: 6

Patch Set 2 : Fixed remaining addFilter calls #

Unified diffs Side-by-side diffs Delta from patch set Stats (+37 lines, -13 lines) Patch
M lib/elemHide.js View 4 chunks +8 lines, -9 lines 0 comments Download
M lib/filterListener.js View 1 3 chunks +29 lines, -4 lines 0 comments Download

Messages

Total messages: 4
Wladimir Palant
3 years, 6 months ago (2016-05-24 11:48:48 UTC) #1
kzar
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#newcode333 lib/elemHide.js:333: if (!selector) I'm curious why you added this check? ...
3 years, 6 months ago (2016-05-24 12:36:49 UTC) #2
Wladimir Palant
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#newcode333 lib/elemHide.js:333: if (!selector) On 2016/05/24 12:36:49, kzar wrote: > I'm ...
3 years, 6 months ago (2016-05-24 12:49:50 UTC) #3
kzar
3 years, 6 months ago (2016-05-24 12:57:45 UTC) #4
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5