Issue 1853 - Moved filter generation into background page

Issue 1853 - Moved filter generation into background page

[kzar, 2015-01-26 10:35:16]

http://codereview.adblockplus.org/6174977720057856/diff/5741031244955648/incl...
File include.postload.js (right):

http://codereview.adblockplus.org/6174977720057856/diff/5741031244955648/incl...
include.postload.js:487: 
Why remove the comments explaining that we're highlighting the elements yellow /
red? I found those useful when first trying to understand the code last time.

[Sebastian Noack, 2015-01-26 10:44:07]

http://codereview.adblockplus.org/6174977720057856/diff/5741031244955648/incl...
File include.postload.js (right):

http://codereview.adblockplus.org/6174977720057856/diff/5741031244955648/incl...
include.postload.js:487: 
On 2015/01/26 10:35:16, kzar wrote:
> Why remove the comments explaining that we're highlighting the elements yellow
/
> red? I found those useful when first trying to understand the code last time.

A while ago we agreed to don't spam code with comments that explain what's
already obvious. Calling functions called highlightElement(s) should be clear
enough that those elements are highlighted here. And the color doesn't matter to
understand the logic. Also the color is given in the code below as well.

[kzar, 2015-01-26 10:49:55]

http://codereview.adblockplus.org/6174977720057856/diff/5741031244955648/incl...
File include.postload.js (right):

http://codereview.adblockplus.org/6174977720057856/diff/5741031244955648/incl...
include.postload.js:487: 
On 2015/01/26 10:44:07, Sebastian Noack wrote:
> On 2015/01/26 10:35:16, kzar wrote:
> > Why remove the comments explaining that we're highlighting the elements
yellow
> /
> > red? I found those useful when first trying to understand the code last
time.
> 
> A while ago we agreed to don't spam code with comments that explain what's
> already obvious. Calling functions called highlightElement(s) should be clear
> enough that those elements are highlighted here. And the color doesn't matter
to
> understand the logic. Also the color is given in the code below as well.

These comments were useful and directly helped me to understand the code when I
was doing the block elements in iFrames ticket.

[Sebastian Noack, 2015-01-26 11:08:14]

http://codereview.adblockplus.org/6174977720057856/diff/5741031244955648/incl...
File include.postload.js (right):

http://codereview.adblockplus.org/6174977720057856/diff/5741031244955648/incl...
include.postload.js:487: 
On 2015/01/26 10:49:55, kzar wrote:
> On 2015/01/26 10:44:07, Sebastian Noack wrote:
> > On 2015/01/26 10:35:16, kzar wrote:
> > > Why remove the comments explaining that we're highlighting the elements
> yellow
> > /
> > > red? I found those useful when first trying to understand the code last
> time.
> > 
> > A while ago we agreed to don't spam code with comments that explain what's
> > already obvious. Calling functions called highlightElement(s) should be
clear
> > enough that those elements are highlighted here. And the color doesn't
matter
> to
> > understand the logic. Also the color is given in the code below as well.
> 
> These comments were useful and directly helped me to understand the code when
I
> was doing the block elements in iFrames ticket.

Those comments doesn't add any information that aren't already given in those
two lines of code.

[kzar, 2015-01-26 14:21:28]

http://codereview.adblockplus.org/6174977720057856/diff/5741031244955648/incl...
File include.postload.js (right):

http://codereview.adblockplus.org/6174977720057856/diff/5741031244955648/incl...
include.postload.js:487: 
On 2015/01/26 11:08:14, Sebastian Noack wrote:
> On 2015/01/26 10:49:55, kzar wrote:
> > On 2015/01/26 10:44:07, Sebastian Noack wrote:
> > > On 2015/01/26 10:35:16, kzar wrote:
> > > > Why remove the comments explaining that we're highlighting the elements
> > yellow
> > > /
> > > > red? I found those useful when first trying to understand the code last
> > time.
> > > 
> > > A while ago we agreed to don't spam code with comments that explain what's
> > > already obvious. Calling functions called highlightElement(s) should be
> clear
> > > enough that those elements are highlighted here. And the color doesn't
> matter
> > to
> > > understand the logic. Also the color is given in the code below as well.
> > 
> > These comments were useful and directly helped me to understand the code
when
> I
> > was doing the block elements in iFrames ticket.
> 
> Those comments doesn't add any information that aren't already given in those
> two lines of code.

Yes they do. These two lines do not explain that anything is being highlighted
yellow and they only explain something is being highlighted red if you go and
look up the hex code #fd1708.

Also I found they gave a useful clue / implied some context that currentElement
was the thing that the user had last clicked on - before you understand the code
"current element" could be the element you last hovered over or pretty much
anything.

But either way, why isn't the fact that I personally found these comments useful
- just the other day - enough? Having a couple of comments here costs us
nothing. What's the point of arguing about them?

[Sebastian Noack, 2015-01-26 15:11:34]

http://codereview.adblockplus.org/6174977720057856/diff/5741031244955648/incl...
File include.postload.js (right):

http://codereview.adblockplus.org/6174977720057856/diff/5741031244955648/incl...
include.postload.js:487: 
On 2015/01/26 14:21:28, kzar wrote:
> On 2015/01/26 11:08:14, Sebastian Noack wrote:
> > On 2015/01/26 10:49:55, kzar wrote:
> > > On 2015/01/26 10:44:07, Sebastian Noack wrote:
> > > > On 2015/01/26 10:35:16, kzar wrote:
> > > > > Why remove the comments explaining that we're highlighting the
elements
> > > yellow
> > > > /
> > > > > red? I found those useful when first trying to understand the code
last
> > > time.
> > > > 
> > > > A while ago we agreed to don't spam code with comments that explain
what's
> > > > already obvious. Calling functions called highlightElement(s) should be
> > clear
> > > > enough that those elements are highlighted here. And the color doesn't
> > matter
> > > to
> > > > understand the logic. Also the color is given in the code below as well.
> > > 
> > > These comments were useful and directly helped me to understand the code
> when
> > I
> > > was doing the block elements in iFrames ticket.
> > 
> > Those comments doesn't add any information that aren't already given in
those
> > two lines of code.
> 
> Yes they do. These two lines do not explain that anything is being highlighted
> yellow

Actually nothing is highlighted yellow here. highlightElements() uses just
another shade of red. This comment is outdated, another reason to get rid of it.
And if you tell me that the comment helped you to understand the code, that
merely means that you didn't understand the code but were misguided by the
comment, since the code is doing something different.

> and they only explain something is being highlighted red if you go and
> look up the hex code #fd1708.

It's quite obvious, since the red octet is significantly larger than the green
and blue. So if you understand the RGB format you don't have to lookup anything
to know that this color is almost pure red. But as I said the color shouldn't
matter to understand the logic. It's actually a hack to use hard-coded colors.

> Also I found they gave a useful clue / implied some context that
currentElement
> was the thing that the user had last clicked on

No, the line in clickHide_mouseOver() where currentElement is set to
event.target gives you the clue that currentElement is the element the user
clicked on. It's irrelevant for the code we have here.

[kzar, 2015-01-26 17:26:39]

I don't want to argue about those comments any more, even though I believe there
should be a comment there still, I don't think it's worth our time to argue
about.

Otherwise this LGTM, I like how the filter generation is separated, after
looking through some of the other reviews it seems to make things simpler.

[kzar, 2015-03-02 18:47:59]

LGTM

[Wladimir Palant, 2015-03-02 20:20:25]

LGTM