ABP/Android JavaScript code

The only private code is start.js and XMLHTTPRequest.jsm, everything else does not need to be reviewed.

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

Just found a few mall issues.

http://codereview.adblockplus.org/8482109/diff/1/assets/js/XMLHttpRequest.jsm
File assets/js/XMLHttpRequest.jsm (right):

http://codereview.adblockplus.org/8482109/diff/1/assets/js/XMLHttpRequest.jsm...
assets/js/XMLHttpRequest.jsm:81: if (rHeader.match(new RegExp(header, "i")))
I think we need to quote the header to use it in a regex, it may lead to weird
errors otherwise. Here's an example quote function that looks good to me:
http://stackoverflow.com/a/2593661

Alternatively, you could just convert both values to lower case before comparing
them.

http://codereview.adblockplus.org/8482109/diff/1/assets/js/start.js
File assets/js/start.js (right):

http://codereview.adblockplus.org/8482109/diff/1/assets/js/start.js#newcode15
assets/js/start.js:15: requestHost = requestHost.replace(/\.+$/, "");
Bit of a nit pick, but how about having a function removeTrailingDots(string)?
Would remove duplication and make the comment above redundant.

http://codereview.adblockplus.org/8482109/diff/1/assets/js/start.js#newcode312
assets/js/start.js:312: return (this.currentIndex < this.lines.length);
This check is done twice, temp variable?

[Andrey Novikov, 2012-11-12 09:33:12]

http://codereview.adblockplus.org/8482109/diff/1/assets/js/XMLHttpRequest.jsm
File assets/js/XMLHttpRequest.jsm (right):

http://codereview.adblockplus.org/8482109/diff/1/assets/js/XMLHttpRequest.jsm...
assets/js/XMLHttpRequest.jsm:81: if (rHeader.match(new RegExp(header, "i")))
On 2012/11/09 14:59:16, Felix H. Dahlke wrote:
> I think we need to quote the header to use it in a regex, it may lead to weird
> errors otherwise. Here's an example quote function that looks good to me:
> http://stackoverflow.com/a/2593661
> 
> Alternatively, you could just convert both values to lower case before
comparing
> them.

Done.

http://codereview.adblockplus.org/8482109/diff/1/assets/js/start.js
File assets/js/start.js (right):

http://codereview.adblockplus.org/8482109/diff/1/assets/js/start.js#newcode15
assets/js/start.js:15: requestHost = requestHost.replace(/\.+$/, "");
On 2012/11/09 14:59:16, Felix H. Dahlke wrote:
> Bit of a nit pick, but how about having a function removeTrailingDots(string)?
> Would remove duplication and make the comment above redundant.

Done.

http://codereview.adblockplus.org/8482109/diff/1/assets/js/start.js#newcode312
assets/js/start.js:312: return (this.currentIndex < this.lines.length);
On 2012/11/09 14:59:16, Felix H. Dahlke wrote:
> This check is done twice, temp variable?

No, currentIndex is plusplused after first check.

[Felix Dahlke, 2012-11-13 07:53:14]

Found another small issue.

http://codereview.adblockplus.org/8482109/diff/1/assets/js/start.js
File assets/js/start.js (right):

http://codereview.adblockplus.org/8482109/diff/1/assets/js/start.js#newcode312
assets/js/start.js:312: return (this.currentIndex < this.lines.length);
On 2012/11/12 09:33:12, Andrey Novikov wrote:
> On 2012/11/09 14:59:16, Felix H. Dahlke wrote:
> > This check is done twice, temp variable?
> 
> No, currentIndex is plusplused after first check.

OK, didn't notice.

http://codereview.adblockplus.org/8482109/diff/7001/assets/js/XMLHttpRequest.jsm
File assets/js/XMLHttpRequest.jsm (right):

http://codereview.adblockplus.org/8482109/diff/7001/assets/js/XMLHttpRequest....
assets/js/XMLHttpRequest.jsm:80: for (var rHeader in this.responseHeaders)
You might want to make sure that you're not iterating over any prototype
properties here, like this:

for (var rHeader in this.responseHeaders)
  if (this.responseHeaders.hasOwnProperty(rHeader)
      && rHeader.toLowerCase() == lcHeader)
    returnedHeaders.push(this.responseHeaders[rHeader]);

But it does not seem likely that any prototype property would coincide with a
header field name.

[Andrey Novikov, 2012-11-13 09:44:54]

http://codereview.adblockplus.org/8482109/diff/7001/assets/js/XMLHttpRequest.jsm
File assets/js/XMLHttpRequest.jsm (right):

http://codereview.adblockplus.org/8482109/diff/7001/assets/js/XMLHttpRequest....
assets/js/XMLHttpRequest.jsm:80: for (var rHeader in this.responseHeaders)
On 2012/11/13 07:53:14, Felix H. Dahlke wrote:
> You might want to make sure that you're not iterating over any prototype
> properties here, like this:
> 
> for (var rHeader in this.responseHeaders)
>   if (this.responseHeaders.hasOwnProperty(rHeader)
>       && rHeader.toLowerCase() == lcHeader)
>     returnedHeaders.push(this.responseHeaders[rHeader]);
> 
> But it does not seem likely that any prototype property would coincide with a
> header field name.

Done.

[Felix Dahlke, 2012-11-13 09:45:37]

LGTM