Issue 1528 - Implemented backend for general tab of new options page

See http://codereview.adblockplus.org/6088024630755328/ for the UI review for the general tab.

There are two methods not implemented which do exist in the current options page because they were not included in the design:
- `Utils.checkLocalePrefixMatch` which is used for highlighting the subscription matching the browser locale
- `Synchronizer.isExecuting` which is used as part of the download progress indicator in the current options page

[saroyanm, 2015-01-26 19:44:06]

http://codereview.adblockplus.org/4864767881641984/diff/5629499534213120/back...
File background.js (right):

http://codereview.adblockplus.org/4864767881641984/diff/5629499534213120/back...
background.js:129: var subscription = Subscription.fromURL("~user~786254");
Subscription is not defined here, this needs to be changed to:
modules.subscriptionClasses.Subscription.fromURL("~user~786254");

http://codereview.adblockplus.org/4864767881641984/diff/5629499534213120/back...
background.js:140: var subscription = Subscription.fromURL("~user~786254");
Subscription is not defined here, this needs to be changed to:
modules.subscriptionClasses.Subscription.fromURL("~user~786254");

[Thomas Greiner, 2015-01-27 13:01:07]

I noticed that I forgot to test the filter management within the test
environment so I rewrote the subscription management code to be able to keep the
state of a subscription.

http://codereview.adblockplus.org/4864767881641984/diff/5629499534213120/back...
File background.js (right):

http://codereview.adblockplus.org/4864767881641984/diff/5629499534213120/back...
background.js:129: var subscription = Subscription.fromURL("~user~786254");
On 2015/01/26 19:44:06, saroyanm wrote:
> Subscription is not defined here, this needs to be changed to:
> modules.subscriptionClasses.Subscription.fromURL("~user~786254");

Done.

http://codereview.adblockplus.org/4864767881641984/diff/5629499534213120/back...
background.js:140: var subscription = Subscription.fromURL("~user~786254");
On 2015/01/26 19:44:06, saroyanm wrote:
> Subscription is not defined here, this needs to be changed to:
> modules.subscriptionClasses.Subscription.fromURL("~user~786254");

Done.

[saroyanm, 2015-01-27 15:37:12]

On 2015/01/27 13:01:07, Thomas Greiner wrote:
> I noticed that I forgot to test the filter management within the test
> environment so I rewrote the subscription management code to be able to keep
the
> state of a subscription.
> 
>
http://codereview.adblockplus.org/4864767881641984/diff/5629499534213120/back...
> File background.js (right):
> 
>
http://codereview.adblockplus.org/4864767881641984/diff/5629499534213120/back...
> background.js:129: var subscription = Subscription.fromURL("~user~786254");
> On 2015/01/26 19:44:06, saroyanm wrote:
> > Subscription is not defined here, this needs to be changed to:
> > modules.subscriptionClasses.Subscription.fromURL("~user~786254");
> 
> Done.
> 
>
http://codereview.adblockplus.org/4864767881641984/diff/5629499534213120/back...
> background.js:140: var subscription = Subscription.fromURL("~user~786254");
> On 2015/01/26 19:44:06, saroyanm wrote:
> > Subscription is not defined here, this needs to be changed to:
> > modules.subscriptionClasses.Subscription.fromURL("~user~786254");
> 
> Done.

LGTM

[Thomas Greiner, 2015-01-30 18:15:17]

Updated the review to add the "app.get/addonVersion" endpoint (as promised at
http://codereview.adblockplus.org/6088024630755328/diff/5689792285114368/opti...).
I also fixed "app.listen/addSubscription" and made it possible to test those two
within the test environment.

[Felix Dahlke, 2015-05-28 20:35:41]

Looks good all in all, a few comments. I have yet to look more closely into the
options page frontend, but I might suggest something along the lines of getting
more logic into the backend, let's see.

http://codereview.adblockplus.org/4864767881641984/diff/5631943370604544/back...
File background.js (right):

http://codereview.adblockplus.org/4864767881641984/diff/5631943370604544/back...
background.js:77: SpecialSubscription: function(url) {
Nit: Opening brace on the next line?

http://codereview.adblockplus.org/4864767881641984/diff/5631943370604544/back...
background.js:104: subscriptions = subscriptions.reduce(function(obj,
subscription)
Can we do something less complex here? Took a while for me to understand the
code because of this. How about properly initialising subscriptions to begin
with rather than changing it like this on first use?

http://codereview.adblockplus.org/4864767881641984/diff/5631943370604544/back...
background.js:110: Object.defineProperty(modules.filterStorage.FilterStorage,
"knownSubscriptions", {
Nit: Opening brace on the next line?

http://codereview.adblockplus.org/4864767881641984/diff/5631943370604544/back...
background.js:140: var subscription =
modules.filterStorage.FilterStorage.knownSubscriptions["~user~786254"];
Seeing this used a few times - how about a "constant"?

http://codereview.adblockplus.org/4864767881641984/diff/5631943370604544/back...
background.js:229: addon: "adblockplus",
Shouldn't this be `addonName`?

http://codereview.adblockplus.org/4864767881641984/diff/5631943370604544/ext/...
File ext/background.js (right):

http://codereview.adblockplus.org/4864767881641984/diff/5631943370604544/ext/...
ext/background.js:82: callback();
Isn't the callback supposed to be invoked after the options page is opened?

http://codereview.adblockplus.org/4864767881641984/diff/5631943370604544/mess...
File messageResponder.js (right):

http://codereview.adblockplus.org/4864767881641984/diff/5631943370604544/mess...
messageResponder.js:29: var filterClasses = require("filterClasses");
Nit: adblockpluschrome typically uses `with` for this IIRC, nicer IMO. But it's
fine by me as it is.

http://codereview.adblockplus.org/4864767881641984/diff/5631943370604544/mess...
messageResponder.js:47: var convertSubscription = convertObject.bind(null,
["disabled", "downloadStatus",
Nit: We might as well go for 80 columns if we wrap here :P

http://codereview.adblockplus.org/4864767881641984/diff/5631943370604544/mess...
messageResponder.js:180: case "filters.blocked":
Nit: `filters.blocking` maybe?

[Thomas Greiner, 2015-06-08 16:12:43]

http://codereview.adblockplus.org/4864767881641984/diff/5631943370604544/back...
File background.js (right):

http://codereview.adblockplus.org/4864767881641984/diff/5631943370604544/back...
background.js:77: SpecialSubscription: function(url) {
On 2015/05/28 20:35:41, Felix H. Dahlke wrote:
> Nit: Opening brace on the next line?

Done.

http://codereview.adblockplus.org/4864767881641984/diff/5631943370604544/back...
background.js:104: subscriptions = subscriptions.reduce(function(obj,
subscription)
On 2015/05/28 20:35:41, Felix H. Dahlke wrote:
> Can we do something less complex here? Took a while for me to understand the
> code because of this. How about properly initialising subscriptions to begin
> with rather than changing it like this on first use?

Done.

http://codereview.adblockplus.org/4864767881641984/diff/5631943370604544/back...
background.js:110: Object.defineProperty(modules.filterStorage.FilterStorage,
"knownSubscriptions", {
On 2015/05/28 20:35:41, Felix H. Dahlke wrote:
> Nit: Opening brace on the next line?

Done.

http://codereview.adblockplus.org/4864767881641984/diff/5631943370604544/back...
background.js:140: var subscription =
modules.filterStorage.FilterStorage.knownSubscriptions["~user~786254"];
On 2015/05/28 20:35:41, Felix H. Dahlke wrote:
> Seeing this used a few times - how about a "constant"?

Done.

http://codereview.adblockplus.org/4864767881641984/diff/5631943370604544/back...
background.js:229: addon: "adblockplus",
On 2015/05/28 20:35:41, Felix H. Dahlke wrote:
> Shouldn't this be `addonName`?

Done. Thanks for pointing out this inconsistency.

Note that currently only `addonVersion` is used but I still added it for
completeness' sake. So we could even get rid of it if you prefer that.

http://codereview.adblockplus.org/4864767881641984/diff/5631943370604544/ext/...
File ext/background.js (right):

http://codereview.adblockplus.org/4864767881641984/diff/5631943370604544/ext/...
ext/background.js:82: callback();
On 2015/05/28 20:35:41, Felix H. Dahlke wrote:
> Isn't the callback supposed to be invoked after the options page is opened?

That's not necessarily correct. It will also be called if the options page is
already open. So if you are on the options page this is always true and
shouldn't lead to a new page being opened.

I added a basic check now to clarify that and also changed the URL since we now
actually have an options page we can open. :)

http://codereview.adblockplus.org/4864767881641984/diff/5631943370604544/mess...
File messageResponder.js (right):

http://codereview.adblockplus.org/4864767881641984/diff/5631943370604544/mess...
messageResponder.js:29: var filterClasses = require("filterClasses");
On 2015/05/28 20:35:41, Felix H. Dahlke wrote:
> Nit: adblockpluschrome typically uses `with` for this IIRC, nicer IMO. But
it's
> fine by me as it is.

"Using with is not recommended, and is forbidden in ECMAScript 5 strict mode.
The recommended alternative is to assign the object whose properties you want to
access to a temporary variable."

see
https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Statements/with

Note that the "need for `with`" will be gone as soon as Chrome/Opera/Safari
implement destructuring assignments which will allow us to write:

var {Filter, BlockingFilter} = require("filterClasses");

http://codereview.adblockplus.org/4864767881641984/diff/5631943370604544/mess...
messageResponder.js:47: var convertSubscription = convertObject.bind(null,
["disabled", "downloadStatus",
On 2015/05/28 20:35:41, Felix H. Dahlke wrote:
> Nit: We might as well go for 80 columns if we wrap here :P

Done.

http://codereview.adblockplus.org/4864767881641984/diff/5631943370604544/mess...
messageResponder.js:180: case "filters.blocked":
On 2015/05/28 20:35:41, Felix H. Dahlke wrote:
> Nit: `filters.blocking` maybe?

This part was added by Wladimir and would require firstRun.js to be adapted
accordingly. I just moved it to have the cases be sorted alphabetically. :)

http://codereview.adblockplus.org/4864767881641984/diff/5732694713434112/back...
File background.js (right):

http://codereview.adblockplus.org/4864767881641984/diff/5732694713434112/back...
background.js:232: knownSubscriptions[subscriptionUrl] =
modules.subscriptionClasses.Subscription.fromURL(subscriptionUrl);
FYI: This is basically the same what I did with `Array.reduce()` before.

[Felix Dahlke, 2015-06-09 13:04:46]

LGTM

http://codereview.adblockplus.org/4864767881641984/diff/5631943370604544/mess...
File messageResponder.js (right):

http://codereview.adblockplus.org/4864767881641984/diff/5631943370604544/mess...
messageResponder.js:29: var filterClasses = require("filterClasses");
On 2015/06/08 16:12:43, Thomas Greiner wrote:
> On 2015/05/28 20:35:41, Felix H. Dahlke wrote:
> > Nit: adblockpluschrome typically uses `with` for this IIRC, nicer IMO. But
> it's
> > fine by me as it is.
> 
> "Using with is not recommended, and is forbidden in ECMAScript 5 strict mode.
> The recommended alternative is to assign the object whose properties you want
to
> access to a temporary variable."
> 
> see
> https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Statements/with
> 
> Note that the "need for `with`" will be gone as soon as Chrome/Opera/Safari
> implement destructuring assignments which will allow us to write:
> 
> var {Filter, BlockingFilter} = require("filterClasses");

Oh, I wasn't aware that with isn't working in strict mode - then never mind of
course.

http://codereview.adblockplus.org/4864767881641984/diff/5631943370604544/mess...
messageResponder.js:180: case "filters.blocked":
On 2015/06/08 16:12:43, Thomas Greiner wrote:
> On 2015/05/28 20:35:41, Felix H. Dahlke wrote:
> > Nit: `filters.blocking` maybe?
> 
> This part was added by Wladimir and would require firstRun.js to be adapted
> accordingly. I just moved it to have the cases be sorted alphabetically. :)

Na, let's leave it alone then, not really worth changing existing code over.