Crawler frontend

The initial implementation of the crawler frontend. In this state, it can only be used from the UI (the ABP menu).

[Wladimir Palant, 2012-09-21 15:36:18]

Concerning defaults/prefs.js (impossible to comment on empty files): if you
don't have any preferences then you don't need to keep that file (and the entire
defaults/ directory) around.

http://codereview.adblockplus.org/8402021/diff/1/chrome/content/crawler.js
File chrome/content/crawler.js (right):

http://codereview.adblockplus.org/8402021/diff/1/chrome/content/crawler.js#ne...
chrome/content/crawler.js:6: 
Site-note: I don't think that this manual UI will be all too useful beyond your
initial testing, I consider this temporary code. Screenshotter creates a new
Firefox profile, installs Adblock Plus and itself into that profile
automatically and runs Firefox then - this extension needs to run the same way.
Starting it in a random Firefox profile, potentially with other extensions and
non-default browser settings just isn't very useful.

http://codereview.adblockplus.org/8402021/diff/1/chrome/content/crawler.js#ne...
chrome/content/crawler.js:29: control.setAttribute("value", control.value);
Are you trying to make the field values persist? Unfortunately, this won't
happen automatically if you change attributes manually. You need to add:

  document.persist(control.id, "value");

http://codereview.adblockplus.org/8402021/diff/1/chrome/content/crawler.js#ne...
chrome/content/crawler.js:35: return !crawling;
Maybe show a message why the window cannot be closed right now?

http://codereview.adblockplus.org/8402021/diff/1/chrome/content/crawler.js#ne...
chrome/content/crawler.js:54: let dialog =
document.getElementById("abpcrawler-crawler");
How about using document.documentElement here?

http://codereview.adblockplus.org/8402021/diff/1/chrome/content/crawler.js#ne...
chrome/content/crawler.js:57: Crawler.crawl(backendUrl, parallelTabs,
window.opener, function()
Of course we could have the condition that window.opener is null or
window.opener.closed is true - the user closed that browser window already. Add
a check and an error message maybe?

http://codereview.adblockplus.org/8402021/diff/1/chrome/content/crawler.xul
File chrome/content/crawler.xul (right):

http://codereview.adblockplus.org/8402021/diff/1/chrome/content/crawler.xul#n...
chrome/content/crawler.xul:29: <label control="backend-url" value="Backend
URL:"/>
Indentation issues here: you are mixing 2 spaces and 4 spaces indentation.

http://codereview.adblockplus.org/8402021/diff/1/chrome/locale/en-US/meta.pro...
File chrome/locale/en-US/meta.properties (right):

http://codereview.adblockplus.org/8402021/diff/1/chrome/locale/en-US/meta.pro...
chrome/locale/en-US/meta.properties:8: description.short=Collects the elements
blocked by a specified filter.
I don't think we will ever publish that extension on addons.mozilla.org :)
Diagnostics used to be available there for a while but here it is safe to remove
description.short and description.long.

http://codereview.adblockplus.org/8402021/diff/1/lib/client.js
File lib/client.js (right):

http://codereview.adblockplus.org/8402021/diff/1/lib/client.js#newcode9
lib/client.js:9: {
How about request.addEventListener("load", callback.bind(null, request))?
Function.bind() makes lots of closures unnecessary which is good for performance
and code readability.

http://codereview.adblockplus.org/8402021/diff/1/lib/client.js#newcode19
lib/client.js:19: formData.append("file", new window.File(filePath));
If you use window.File then you can also use window.FormData instead of going
the complicated route. Too bad that File apparently isn't available via XPCOM.
Then again, it is probably possible to "steal" File and FormData from a
JavaScript module the same way we do it for atob - see
buildtools/bootstrap.js.tmpl.

http://codereview.adblockplus.org/8402021/diff/1/lib/client.js#newcode26
lib/client.js:26: request.addEventListener("load", function()
Same as above, better use Function.bind().

http://codereview.adblockplus.org/8402021/diff/1/lib/crawler.js
File lib/crawler.js (right):

http://codereview.adblockplus.org/8402021/diff/1/lib/crawler.js#newcode4
lib/crawler.js:4: {
This is unnecessary - module in lib/ directory can just use require() to get to
other modules of the same extension.

http://codereview.adblockplus.org/8402021/diff/1/lib/crawler.js#newcode19
lib/crawler.js:19: return Cu.import("chrome://adblockplus-modules/content/" +
This is unnecessary - Diagnostics is currently compatible with old Adblock Plus
versions (ones that used JSM modules), this extension doesn't need to do that.

http://codereview.adblockplus.org/8402021/diff/1/lib/crawler.js#newcode38
lib/crawler.js:38: if (url)
I think that you can remove that check - an nsIURI is always a valid URL. And
invalid URLs don't even result in a call to processNode().

There is a special case: for calls with type == Policy.type.ELEMHIDE the
location parameter is a filter (element hiding hits don't have a real location).
So you probably want to sent location.text then.

http://codereview.adblockplus.org/8402021/diff/1/lib/crawler.js#newcode72
lib/crawler.js:72: siteTabs[aLocation.spec] = site;
Ok, that's a hack. What if two webpages in different tabs redirect to the same
URL?

Associating data with the <browser> element should definitely work and it is the
cleaner/more reliable approach.

http://codereview.adblockplus.org/8402021/diff/1/lib/crawler.js#newcode84
lib/crawler.js:84: sites = sites.slice(1);
let site = sites.shift()?

http://codereview.adblockplus.org/8402021/diff/1/lib/main.js
File lib/main.js (right):

http://codereview.adblockplus.org/8402021/diff/1/lib/main.js#newcode9
lib/main.js:9: let {Prefs} = require("prefs");
You aren't using prefs so you should remove that line (loading that module will
initialize it and make it attempt to load defaults/prefs.js).

http://codereview.adblockplus.org/8402021/diff/1/lib/main.js#newcode16
lib/main.js:16: "mail:messageWindow": true,
I think that you only know navigator:browser - meaning Firefox. The other two
types are only used in Thunderbird.

http://codereview.adblockplus.org/8402021/diff/1/lib/storage.js
File lib/storage.js (right):

http://codereview.adblockplus.org/8402021/diff/1/lib/storage.js#newcode32
lib/storage.js:32: let line = JSON.stringify(data) + "\n";
Looking at that again - you shouldn't need the converter if you use
https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIJSON#en....
You can write the newline character separately via outputStream.write("\n", 1).

http://codereview.adblockplus.org/8402021/diff/1/prefs.js
File prefs.js (right):

http://codereview.adblockplus.org/8402021/diff/1/prefs.js#newcode1
prefs.js:1: pref("extensions.abpwatcher.startwatching_key", "Accel Shift M,
Accel Shift F4");
Looks like unused file?

[Wladimir Palant, 2012-09-21 17:48:01]

Just to make sure there are no misunderstandings: nsIJSON is not the same thing
as the JSON object available to web pages. It is an internally used interface
that is mostly superseded now - but it still offers ways to work with streams
that the new JSON object doesn't have.

[Felix Dahlke, 2012-09-24 14:11:30]

I've made your proposed changes, except those discussed below.

> Site-note: I don't think that this manual UI will be all too useful beyond
your
> initial testing, I consider this temporary code. Screenshotter creates a new
> Firefox profile, installs Adblock Plus and itself into that profile
> automatically and runs Firefox then - this extension needs to run the same
way.
> Starting it in a random Firefox profile, potentially with other extensions and
> non-default browser settings just isn't very useful.

Yes, that's the plan, but I didn't want to wait too long with the review. I
delayed the running via a script, because I thought it would be harder to debug
when started that way.

>
http://codereview.adblockplus.org/8402021/diff/1/chrome/content/crawler.js#ne...
> chrome/content/crawler.js:29: control.setAttribute("value", control.value);
> Are you trying to make the field values persist? Unfortunately, this won't
> happen automatically if you change attributes manually. You need to add:
> 
>   document.persist(control.id, "value");

Just tried that, but it doesn't seem to do the same thing as setAttribute.

> Then again, it is probably possible to "steal" File and FormData from a
> JavaScript module the same way we do it for atob - see
> buildtools/bootstrap.js.tmpl.

It would be nice not having to pass window around, but I don't really understand
where atob is coming from in bootstrap.js.tmpl.

> http://codereview.adblockplus.org/8402021/diff/1/lib/crawler.js#newcode72
> lib/crawler.js:72: siteTabs[aLocation.spec] = site;
> Ok, that's a hack. What if two webpages in different tabs redirect to the same
> URL?
> 
> Associating data with the <browser> element should definitely work and it is
the
> cleaner/more reliable approach.

Agreed, that needs work. I'll first solve the smaller review issues and then
work on this.

> http://codereview.adblockplus.org/8402021/diff/1/lib/storage.js
> File lib/storage.js (right):
> 
> http://codereview.adblockplus.org/8402021/diff/1/lib/storage.js#newcode32
> lib/storage.js:32: let line = JSON.stringify(data) + "\n";
> Looking at that again - you shouldn't need the converter if you use
>
https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIJSON#en....

This variant definitely makes the code nicer, but I wasn't able to get it
working. encodeToStream() would always cause "NS_BASE_STREAM_CLOSED" errors.

[Felix Dahlke, 2012-09-24 14:22:05]

As I said, I've addressed all issues except the location.href hack for
differentiating sites. I'll try and find a better solution for that in the mean
time.

[Wladimir Palant, 2012-09-25 09:40:39]

http://codereview.adblockplus.org/8402021/diff/12001/lib/client.js
File lib/client.js (right):

http://codereview.adblockplus.org/8402021/diff/12001/lib/client.js#newcode15
lib/client.js:15: formData.append("file", new window.File(filePath));
As discussed on IRC: we can avoid using the window here. This requires modifying
buildtools/bootstrap.js.tmpl so that it gets File from the scope of a JavaScript
module and add it to the scope of our modules (similar to the way it is
currently done for atob). And FormData can be constructed via XPCOM the way
you've done it originally.

http://codereview.adblockplus.org/8402021/diff/12001/lib/crawler.js
File lib/crawler.js (right):

http://codereview.adblockplus.org/8402021/diff/12001/lib/crawler.js#newcode58
lib/crawler.js:58: siteTabs[aLocation.spec] = site;
Taking over comment from previous review: this needs to be done properly,
attaching data to the <browser> element should work.

[Felix Dahlke, 2012-09-25 15:14:31]

I've addressed all issues now.

[Wladimir Palant, 2012-09-25 16:51:29]

http://codereview.adblockplus.org/8402021/diff/24001/lib/crawler.js
File lib/crawler.js (right):

http://codereview.adblockplus.org/8402021/diff/24001/lib/crawler.js#newcode31
lib/crawler.js:31: let site = siteTabs.get(browser);
I would probably add a few more checks here. The three lines above failing if
ABP and Crawler are the only extensions installed is unexpected - but it isn't
impossible. Meaning: the chrome window doesn't necessarily have a gBrowser
property (e.g. a non-browser window making a request), the tabbrowser doesn't
necessarily know the document (e.g. a request in a browser sidebar) and we don't
necessarily know the browser (e.g. a browser belonging to a pop-up window that
was somehow opened despite the pop-up blocker).

http://codereview.adblockplus.org/8402021/diff/24001/lib/crawler.js#newcode62
lib/crawler.js:62: siteTabs.set(browser, site);
Why are we doing this in the progress listener and not at the top of loadSite?
We already know the browser there, onLocationChange() shouldn't be necessary.

http://codereview.adblockplus.org/8402021/diff/24001/lib/crawler.js#newcode102
lib/crawler.js:102: siteTabs = new WeakMap();
You should null out this variable when you are done, to make sure the map can be
garbage collected (the usual trouble with global variables).

[Felix Dahlke, 2012-09-26 08:26:00]

Okay, I've addressed all your comments.

[Wladimir Palant, 2012-09-26 11:02:00]

LGTM