Acquired Opera AdBlock code

This is the code of Opera AdBlock, which we acquired. It needs a full review.

[Felix Dahlke, 2012-10-04 08:28:05]

First some general observations:

- Indentation is mostly tabs, but a bit inconsistent. We'll want 2 space indents
at some point. I guess the earlier the better.
- Quite a bit of trailing whitespace, we should get rid of that while we're
fixing the indentation
- Single and double quotes are mixed, we'll want to use one kind everywhere
- Sometimes quotes around HTML attributes are missing. While this works and is
valid under some circumstances, the HTML spec recommends to always use quotes.
So do I.
- Opening braces are not on their own line, which isn't unusualy, but we'll
probably want to apply our commonly used style at some point. Changing this
isn't as urgent as changing indentation, because only a couple of lines will be
affected.

[Felix Dahlke, 2012-10-04 08:37:49]

More general observations:

- Lots of commented code everywhere. We can safely get rid of all that now that
the project is under version control.
- Another subtle style issue: No space before the opening paren for if/while/for
etc.

[Felix Dahlke, 2012-10-05 08:57:48]

A quick tour around the code base:

Extension files:
config.xml - Extension metadata, configuration and default preference values
index.html - Extension start file, initialises extension and UI
locales - Locale-specific files, most notably translated texts

Pages:
about.html and about/ - About page
help.html and help/ - Help page
options.html and options/ - Options page
common - Code used on multiple pages

Code:
files - Main code, the setup code is in background.js
includes - Injected scripts, will be executed on all or selected pages. There is
just one that injects CSS for element hiding on all pages right now.

Style:
design - All CSS files, used e.g. on pages
images - All image files

[Felix Dahlke, 2012-10-05 15:47:16]

http://codereview.adblockplus.org/8493027/diff/1/files/background.js
File files/background.js (right):

http://codereview.adblockplus.org/8493027/diff/1/files/background.js#newcode20
files/background.js:20: if(whitelist.check(event.data.url))
In all postMessage invocations, just the reply is different. Better
conditionally assign the reply and use it with a single postMessage call.
(Actually two: One here and one below)

http://codereview.adblockplus.org/8493027/diff/1/files/background.js#newcode26
files/background.js:26: if(css.status(key) && whitelist.check(event.data.url))
whitelist.check(event.data.url) should be stored in a variable, it's called
multiple times.

http://codereview.adblockplus.org/8493027/diff/1/files/background.js#newcode44
files/background.js:44: opera.extension.urlfilter.allow.add(event.data.rule);
See comment in parse.js on urlfilter.allow.

http://codereview.adblockplus.org/8493027/diff/1/files/background.js#newcode58
files/background.js:58:
if(window.navigator.language.substring(0,2).toLowerCase() == 'ru') {
This is something we'll want to do for all locales, not just Russian.

http://codereview.adblockplus.org/8493027/diff/1/files/background.js#newcode70
files/background.js:70: if(css.status(key) && getTime() - css.time(key) >
preferences.int('interval')) { //If the list is enabled
getTime() and preferences.int('interval') should be stored in a variable - both
are used multiple times.

http://codereview.adblockplus.org/8493027/diff/1/files/background.js#newcode81
files/background.js:81: window.setTimeout('updateAll()',
preferences.int('interval') / 12); //Verifies required updates every 1/12 of the
update interval
Better use window.setTimeout(updateAll, ...), passing a string to setTimeout is
deprecated.

http://codereview.adblockplus.org/8493027/diff/1/files/lists.js
File files/lists.js (right):

http://codereview.adblockplus.org/8493027/diff/1/files/lists.js#newcode12
files/lists.js:12: if(add)
Shouldn't this use parse.opera()? It has the very same logic. It's commented out
right now though.

http://codereview.adblockplus.org/8493027/diff/1/files/lists.js#newcode37
files/lists.js:37: opera.extension.urlfilter.allow.add(content[i]);
See comment in parse.js on urlfilter.allow.

http://codereview.adblockplus.org/8493027/diff/1/files/lists.js#newcode44
files/lists.js:44: if(useMessage)
From what I've seen, useMessage is always true. Why parameterise?

http://codereview.adblockplus.org/8493027/diff/1/files/lists.js#newcode56
files/lists.js:56: if(useMessage)
As in add, seems to always be true.

http://codereview.adblockplus.org/8493027/diff/1/files/lists.js#newcode71
files/lists.js:71: success = false;
We can just return false here, avoiding the success variable and unnecessary
iterations.

http://codereview.adblockplus.org/8493027/diff/1/files/lists.js#newcode84
files/lists.js:84: value =
download(sources.url(key)).replace(/(^|\n);.*?(\n|$)/gm,
'$1').split('[exclude]')[1].split('\n'); //Pre-remove comments to save space
This line definitely deserves its own function, with multiple lines.

It won't be able to handle ill-formatted filters like this. Or download
problems. And why is everything before the exclude section ignored?

http://codereview.adblockplus.org/8493027/diff/1/files/lists.js#newcode86
files/lists.js:86: value =
download(sources.url(key)).replace(/^\[adblock.*\n/im,
'').replace(/(^|\n)(!|##|#@#).*?(\n|$)/gm, '$1').split('\n'); //Pre-remove
comments and unsupported to save space
As above, this deserves its own function and multiple lines. I haven't checked
the whole code base at this point, but it seems like element hiding isn't
supported (apart from including e.g. fanboy's pre generated CSS).

http://codereview.adblockplus.org/8493027/diff/1/files/lists.js#newcode94
files/lists.js:94: else
else is not necessary here. In fact, the return value doesn't seem to be used
anywhere.

http://codereview.adblockplus.org/8493027/diff/1/files/lists.js#newcode97
files/lists.js:97: status: function(key) {
How about "enabled" instead of "status"?  Status implies it's more than a
boolean IMO.

http://codereview.adblockplus.org/8493027/diff/1/files/lists.js#newcode105
files/lists.js:105: return true;
The function will always return true no matter what. Probably not necessary.

http://codereview.adblockplus.org/8493027/diff/1/files/parse.js
File files/parse.js (right):

http://codereview.adblockplus.org/8493027/diff/1/files/parse.js#newcode10
files/parse.js:10: adblock: function(content, add) {
This function has a lot of code duplication, we should reduce that.

http://codereview.adblockplus.org/8493027/diff/1/files/parse.js#newcode18
files/parse.js:18: opera.extension.urlfilter.allow.add(url, option);
If we're using urlfilter.allow, filters with "adblock format" (e.g. Easylist)
will not work on Opera < 12.10. But even if we check for its presence to avoid
an exception, whitelisting wouldn't work, and that means we would have many
false positives.

http://codereview.adblockplus.org/8493027/diff/1/files/parse.js#newcode32
files/parse.js:32: opera.extension.urlfilter.allow.add(url);
See above.

http://codereview.adblockplus.org/8493027/diff/1/files/parse.js#newcode57
files/parse.js:57: 'all': opera.extension.urlfilter.RESOURCE_SCRIPT |
opera.extension.urlfilter.RESOURCE_IMAGE |
opera.extension.urlfilter.RESOURCE_STYLESHEET |
opera.extension.urlfilter.RESOURCE_OBJECT |
We should generate this 'all' mask from the flags above.

http://codereview.adblockplus.org/8493027/diff/1/files/parse.js#newcode66
files/parse.js:66: if(element.match(/^domain/i) == 0) { //TESTED
What is "TESTED" supposed to mean? That this expression evaluates to true when
element starts with "domain"?  I don't think so, match() returns an array, which
isn't equal to 0, type inference or not. I don't think this condition is ever
entered.

http://codereview.adblockplus.org/8493027/diff/1/files/parse.js#newcode67
files/parse.js:67: var domains = special[i].split('=')[1].split(',');
What if there is no "="? The above should match for /^domain=/ to avoid issues
with incorrect filters.

http://codereview.adblockplus.org/8493027/diff/1/files/parse.js#newcode69
files/parse.js:69: if(domains[j].match(/^~/)) //Don't apply on this website
This could be made more succinct by setting the option attribute conditionally.

http://codereview.adblockplus.org/8493027/diff/1/files/parse.js#newcode75
files/parse.js:75: else if(!element.match(/^elemhide/i)) { //No support for
ElemHide - TESTED
"TESTED" again? What about bogus or unsupported options? Whitelisting options
would be more robust.

http://codereview.adblockplus.org/8493027/diff/1/files/parse.js#newcode76
files/parse.js:76: if(!element.match(/^~/i)) {
Case insensitive matching makes no sense for "~".

http://codereview.adblockplus.org/8493027/diff/1/files/parse.js#newcode81
files/parse.js:81: option['resources'] += this.map[element];
That's not how a bit flag is added to a mask ("|="). This won't work.

http://codereview.adblockplus.org/8493027/diff/1/files/parse.js#newcode84
files/parse.js:84: option['resources'] -= this.map[element];
That's not how a bit flag is removed from a bit mask ("^="). This won't work.

http://codereview.adblockplus.org/8493027/diff/1/files/parse.js#newcode89
files/parse.js:89: wildcard: function(url) { //TESTED
"TESTED" again?
There's a bit of duplication here and it's not easy to read, but the function
does seem to work fine.

http://codereview.adblockplus.org/8493027/diff/1/files/sources.js
File files/sources.js (right):

http://codereview.adblockplus.org/8493027/diff/1/files/sources.js#newcode5
files/sources.js:5:
'fanboy':'https://www.fanboy.co.nz/adblock/opera/urlfilter.ini',
How about making this more cohesive by storing style, name and license together
with the URL in an object? It's a bit spread out right now.

http://codereview.adblockplus.org/8493027/diff/1/files/sources.js#newcode48
files/sources.js:48: for (var i in this.fanboy) { value[i] = this.fanboy[i]; }
IMO this would look a bit cleaner if the loop over the lists wasn't unrolled.
(i.e. nested loops)

[Felix Dahlke, 2012-10-05 15:50:49]

I've done a partial review, all the files I've commented on are fully reviewed.
It seem to be the most relevant ones.

Some relevations:
- Code quality is pretty bad, especially in parse.js which is arguably the most
important module
- Element hiding does not work (except for directly included precompiled CSS, as
provided by Fanboy)
- Options in filters do not work

And one naming nit pick:
Functions are hardly ever named by words, which makes it harder to make sense of
them. For instance: There's "options()" instead of "parseOptions()". I didn't
comment on all individual cases, there are a lot of those.

[Felix Dahlke, 2012-10-08 05:31:46]

Another observation: No function is ever private. That's not a very big problem,
but something we should change when bringing the code base up to our standards.
If something can be private, it should. Makes the module easier to understand
and harder to misuse.

[Felix Dahlke, 2012-10-08 05:57:02]

http://codereview.adblockplus.org/8493027/diff/1/files/preferences.js
File files/preferences.js (right):

http://codereview.adblockplus.org/8493027/diff/1/files/preferences.js#newcode3
files/preferences.js:3: if(value == undefined) //Gets
That's a bit dangerous, undefined could in fact have been defined. Better:
typeof value === "undefined"

http://codereview.adblockplus.org/8493027/diff/1/files/preferences.js#newcode9
files/preferences.js:9: if(value == undefined) { //Gets
See comment about the same code above.

http://codereview.adblockplus.org/8493027/diff/1/files/preferences.js#newcode10
files/preferences.js:10: switch(widget.preferences.getItem(key)) {
How about return key === "true" instead?

http://codereview.adblockplus.org/8493027/diff/1/files/preferences.js#newcode27
files/preferences.js:27: if(value == undefined) { //Gets
See comment about the same code above.

http://codereview.adblockplus.org/8493027/diff/1/files/preferences.js#newcode29
files/preferences.js:29: if(value == NaN)
No, this doesn't work. Use isNan().

http://codereview.adblockplus.org/8493027/diff/1/files/preferences.js#newcode30
files/preferences.js:30: return 0; //On error return 0
I think it's dangerous to return 0 in case of an error - is there never a
situation where we want to distinguish a not stored value from the explicit
value 0?

http://codereview.adblockplus.org/8493027/diff/1/files/preferences.js#newcode38
files/preferences.js:38: if(value == undefined) { //Gets
See comment about the same code above.

http://codereview.adblockplus.org/8493027/diff/1/files/preferences.js#newcode40
files/preferences.js:40: if(value == NaN)
See above, use isNan()

http://codereview.adblockplus.org/8493027/diff/1/files/preferences.js#newcode41
files/preferences.js:41: return 0; //On error return 0
See above, not sure if this is a good idea.

http://codereview.adblockplus.org/8493027/diff/1/files/preferences.js#newcode49
files/preferences.js:49: if(value == undefined) { //Gets
See comment about the same code above.

http://codereview.adblockplus.org/8493027/diff/1/files/preferences.js#newcode52
files/preferences.js:52: if(value.indexOf("\n") > -1)
This whole if/else can be replaced by: return value.split("\n")

http://codereview.adblockplus.org/8493027/diff/1/files/preferences.js#newcode58
files/preferences.js:58: return new Array(); //Returns an empty array when no
value exists
Shorter: return []

http://codereview.adblockplus.org/8493027/diff/1/files/preferences.js#newcode66
files/preferences.js:66: switch(widget.preferences.getItem(key)) {
This switch can be replaced with: widget.preferences.setItem(key, item ===
'true' ? 'false' : 'true')

http://codereview.adblockplus.org/8493027/diff/1/files/preferences.js#newcode75
files/preferences.js:75: exist: function(key) { //Says if a preference exists
This will return false for a numerical value set to 0, see above. Considering
that, we can just write: return !!widget.preferences.getItem(key).

This also avoids getItem being called twice.

[Felix Dahlke, 2012-10-08 06:00:50]

Uploaded a new change set by the guy. He's touched some of the files I've
reviewed already so I'll have to review the diffs.

[Felix Dahlke, 2012-10-10 12:53:43]

Since we've decided to drop most of the code and port ABP instead, the following
files will not be reviewed here, even if they already have comments:

- Everything in files/
- config.xml
- index.html

[Felix Dahlke, 2012-10-10 12:55:33]

On 2012/10/10 12:53:43, Felix H. Dahlke wrote:
> Since we've decided to drop most of the code and port ABP instead, the
following
> files will not be reviewed here, even if they already have comments:
> 
> - Everything in files/
> - config.xml
> - index.html

includes/cssFilter.js also won't be reviewed.

[Felix Dahlke, 2012-10-10 14:24:03]

Actually, since we're going to port the UI from Chrome, I think we can close
this review.