Adding ABP core modules to ABP/Opera

Adding ABP core modules to ABP/Opera

[Felix Dahlke, 2012-10-09 14:16:59]

According to Wladimir, most of this code is generated. Only the following files
are to be reviewed:

- lib/matcher.js
- lib/adblockplus_compat.js (IO module)
- background.js (just importOldData())

On that note, I think it would be brilliant if we could move all generated code
into into a special subdirectory.

[Wladimir Palant, 2012-10-09 14:28:43]

On 2012/10/09 14:16:59, Felix H. Dahlke wrote:
> According to Wladimir, most of this code is generated.

Not generated but generic :)
It's code I copied verbatim from Adblock Plus for Chrome. The long-term solution
is probably to simply generate the Opera extension from the same repository, the
differences are just too few.

[Felix Dahlke, 2012-10-09 14:31:20]

On 2012/10/09 14:28:43, Wladimir Palant wrote:
> Not generated but generic :)
> It's code I copied verbatim from Adblock Plus for Chrome. The long-term
solution
> is probably to simply generate the Opera extension from the same repository,
the
> differences are just too few.

Ah okay, then it doesn't really need its own sub directory yet.

[Felix Dahlke, 2012-10-10 12:11:22]

Done with the review.

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

http://codereview.adblockplus.org/8483154/diff/1/background.js#newcode80
background.js:80: // TODO: Import custom filters?
Yes, I think we should do that, but it might get hairy. IIRC, the custom filters
are stored in Opera's format, and custom element hiding was just plain CSS.

http://codereview.adblockplus.org/8483154/diff/1/button.js
File button.js (right):

http://codereview.adblockplus.org/8483154/diff/1/button.js#newcode13
button.js:13: click: function() {
This is just an alias for update() that's just used once, below. We can remove
this and just use update() instead.

http://codereview.adblockplus.org/8483154/diff/1/button.js#newcode44
button.js:44: icon: '/images/icon18.png',
update() is called soon, overwriting the icon property, so we don't need to set
it here. Unless createItem() demands it.

http://codereview.adblockplus.org/8483154/diff/1/index.html
File index.html (right):

http://codereview.adblockplus.org/8483154/diff/1/index.html#newcode12
index.html:12: <script src="filterListener.js"></script>
Note from Wladimir: This include should be removed.

http://codereview.adblockplus.org/8483154/diff/1/lib/adblockplus_compat.js
File lib/adblockplus_compat.js (right):

http://codereview.adblockplus.org/8483154/diff/1/lib/adblockplus_compat.js#ne...
lib/adblockplus_compat.js:113: IO: {
Opening brace not on its own line?

http://codereview.adblockplus.org/8483154/diff/1/lib/adblockplus_compat.js#ne...
lib/adblockplus_compat.js:133: if ("spec" in file &&
/^defaults\b/.test(file.spec))
The first check is not necessary, .test(undefined) will return false.

http://codereview.adblockplus.org/8483154/diff/1/lib/adblockplus_compat.js#ne...
lib/adblockplus_compat.js:164: window.localStorage[path] = data.join("\n") +
"\n";
Why not use this.lineBreak?

http://codereview.adblockplus.org/8483154/diff/1/lib/adblockplus_compat.js#ne...
lib/adblockplus_compat.js:175: this.readFromFile(fromFile, false, {
Shouldn't the opening brace be on its own line?

http://codereview.adblockplus.org/8483154/diff/1/lib/adblockplus_compat.js#ne...
lib/adblockplus_compat.js:200: window.localStorage[newName + "/lastModified"] =
window.localStorage[path + "/lastModified"] || 0;
How about a function that sets both localStorage[x] and localStorage[x +
"/lastModified"]? lastModified could be optional and default to 0. Could also be
used in writeFile() above.

http://codereview.adblockplus.org/8483154/diff/1/lib/adblockplus_compat.js#ne...
lib/adblockplus_compat.js:209: delete window.localStorage[path];
How about a function to delete both localStorage[x] and localStorage[x +
"/lastModified"]? Could also be used in renameFile() above.

http://codereview.adblockplus.org/8483154/diff/1/lib/matcher.js
File lib/matcher.js (right):

http://codereview.adblockplus.org/8483154/diff/1/lib/matcher.js#newcode27
lib/matcher.js:27: _generateRule: function(filter)
I'd love it if the code segments below comments ("Convert anchors, ...",
"Translate type options" etc.) would be extracted to functions. Probably just a
matter of taste, but I find small functions with descriptive names easier to
understand than larger ones with comments. I've also given Andrey that feedback
a lot - it's just my opinion, nothing I insist on.

http://codereview.adblockplus.org/8483154/diff/1/lib/matcher.js#newcode48
lib/matcher.js:48: if (rule.text.substr(0, 1) == "|")
This value is used twice, so how about storing it in a variable? I'd also prefer
[0] to .substr(0, 1):

var firstChar = rule.text[0];
if (firstChar == "|")
...

Only a readability aid, we can of course rely on the idempotency of [] and
substr().

http://codereview.adblockplus.org/8483154/diff/1/lib/matcher.js#newcode57
lib/matcher.js:57: if (rule.text.substr(-1) == "|")
As above, how about storing this in a variable lastChar?

http://codereview.adblockplus.org/8483154/diff/1/lib/matcher.js#newcode69
lib/matcher.js:69: resources = resources | urlfilter["RESOURCE_" + type];
How about:
resources |= urlfilter["RESOURCE_" + type];

http://codereview.adblockplus.org/8483154/diff/1/lib/matcher.js#newcode108
lib/matcher.js:108: if (filter.text == "http://")
Why just check for the string "http://" here? Wouldn't "foobar" or anything
that's not a valid filter be equally worth reporting?

http://codereview.adblockplus.org/8483154/diff/1/lib/matcher.js#newcode129
lib/matcher.js:129: opera.extension.urlfilter[rule.type].remove(rule);
Shouldn't this be "remove(text)"?

[Felix Dahlke, 2012-10-10 15:37:06]

On 2012/10/10 12:11:22, Felix H. Dahlke wrote:
http://codereview.adblockplus.org/8483154/diff/1/lib/adblockplus_compat.js#ne...
> lib/adblockplus_compat.js:113: IO: {
> Opening brace not on its own line?
> 
>
http://codereview.adblockplus.org/8483154/diff/1/lib/adblockplus_compat.js#ne...
> lib/adblockplus_compat.js:175: this.readFromFile(fromFile, false, {
> Shouldn't the opening brace be on its own line?

Forget these two remarks, our code base consistently places opening braces for
object literals on the same line.

[Felix Dahlke, 2012-10-11 07:54:28]

http://codereview.adblockplus.org/8483154/diff/1/index.html
File index.html (right):

http://codereview.adblockplus.org/8483154/diff/1/index.html#newcode12
index.html:12: <script src="filterListener.js"></script>
On 2012/10/10 12:11:22, Felix H. Dahlke wrote:
> Note from Wladimir: This include should be removed.

Done, while I was at it.

http://codereview.adblockplus.org/8483154/diff/1/lib/adblockplus_compat.js
File lib/adblockplus_compat.js (right):

http://codereview.adblockplus.org/8483154/diff/1/lib/adblockplus_compat.js#ne...
lib/adblockplus_compat.js:113: IO: {
On 2012/10/10 12:11:22, Felix H. Dahlke wrote:
> Opening brace not on its own line?

Nevermind.

http://codereview.adblockplus.org/8483154/diff/1/lib/adblockplus_compat.js#ne...
lib/adblockplus_compat.js:133: if ("spec" in file &&
/^defaults\b/.test(file.spec))
On 2012/10/10 12:11:22, Felix H. Dahlke wrote:
> The first check is not necessary, .test(undefined) will return false.

Nevermind.

http://codereview.adblockplus.org/8483154/diff/1/lib/adblockplus_compat.js#ne...
lib/adblockplus_compat.js:138: executeFirstRunActions();
This function cannot be called from here.

[Felix Dahlke, 2012-10-11 07:55:57]

Please ignore my comments on button.js, I'll fix the issues while I'm working on
the UI.

[Wladimir Palant, 2012-10-11 09:36:26]

http://codereview.adblockplus.org/8483154/diff/1/button.js
File button.js (right):

http://codereview.adblockplus.org/8483154/diff/1/button.js#newcode13
button.js:13: click: function() {
On 2012/10/10 12:11:22, Felix H. Dahlke wrote:
> This is just an alias for update() that's just used once, below. We can remove
> this and just use update() instead.

I didn't change this file, merely renamed it because I thought that we might
want to use that code. It is currently unused however, it has to be taken care
of when the UI is migrated - not sure we will keep it at all then.

http://codereview.adblockplus.org/8483154/diff/1/lib/adblockplus_compat.js
File lib/adblockplus_compat.js (right):

http://codereview.adblockplus.org/8483154/diff/1/lib/adblockplus_compat.js#ne...
lib/adblockplus_compat.js:133: if ("spec" in file &&
/^defaults\b/.test(file.spec))
On 2012/10/10 12:11:22, Felix H. Dahlke wrote:
> The first check is not necessary, .test(undefined) will return false.

Still, I don't feel comfortable running regular expressions in undefined values.
I better check first whether this actually is a URL - it also makes clear that
it might not be one.

http://codereview.adblockplus.org/8483154/diff/1/lib/matcher.js
File lib/matcher.js (right):

http://codereview.adblockplus.org/8483154/diff/1/lib/matcher.js#newcode69
lib/matcher.js:69: resources = resources | urlfilter["RESOURCE_" + type];
I thought that JavaScript doesn't support |= but it seems that I was mistaken -
looks like I was thinking about ||= that is indeed not supported.

http://codereview.adblockplus.org/8483154/diff/1/lib/matcher.js#newcode108
lib/matcher.js:108: if (filter.text == "http://")
This is just debugging code I forgot :)

[Felix Dahlke, 2012-10-11 13:10:02]

Looks good. But I've discovered two issues in practice, see below. I wouldn't
mind addressing those while I'm at it though.

http://codereview.adblockplus.org/8483154/diff/8003/background.js
File background.js (right):

http://codereview.adblockplus.org/8483154/diff/8003/background.js#newcode70
background.js:70: Synchronizer.execute(subscription);
This line causes an exception.

http://codereview.adblockplus.org/8483154/diff/8003/lib/adblockplus_compat.js
File lib/adblockplus_compat.js (right):

http://codereview.adblockplus.org/8483154/diff/8003/lib/adblockplus_compat.js...
lib/adblockplus_compat.js:147: executeFirstRunActions();
This function cannot be called from here.

[Felix Dahlke, 2012-10-11 14:50:25]

On 2012/10/11 13:10:02, Felix H. Dahlke wrote:
> http://codereview.adblockplus.org/8483154/diff/8003/background.js
> File background.js (right):
> 
> http://codereview.adblockplus.org/8483154/diff/8003/background.js#newcode70
> background.js:70: Synchronizer.execute(subscription);
> This line causes an exception.

Doesn't happen anymore, for some reason.

> http://codereview.adblockplus.org/8483154/diff/8003/lib/adblockplus_compat.js
> File lib/adblockplus_compat.js (right):
> 
>
http://codereview.adblockplus.org/8483154/diff/8003/lib/adblockplus_compat.js...
> lib/adblockplus_compat.js:147: executeFirstRunActions();
> This function cannot be called from here.

Wladimir has fixed it, no need to upload a new patch set.

So LGTM.