Noissue - Make the options page respond to subscription title and homepage changes

Noissue - Make the options page respond to subscription title and homepage changes

[Sebastian Noack, 2016-04-05 19:09:54]

https://codereview.adblockplus.org/29339395/diff/29339396/background.js
File background.js (left):

https://codereview.adblockplus.org/29339395/diff/29339396/background.js#oldco...
background.js:141: this.title = "Subscription " + url;
Otherwise, any subscription update would cause the title provided by
subscriptions.xml to get overridden with this one when running with the
standalone mock implementation.

https://codereview.adblockplus.org/29339395/diff/29339396/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29339395/diff/29339396/messageResponder.js...
messageResponder.js:64: var key = keys[i];
As per the comment in background.js we don't set the title anymore, to avoid
overriding it. However, that requires an addional check here. Otherwise the
title would still be overridden with undefined.

[Thomas Greiner, 2016-04-06 13:51:18]

Just two comments about minor stuff.

https://codereview.adblockplus.org/29339395/diff/29339396/options.js
File options.js (right):

https://codereview.adblockplus.org/29339395/diff/29339396/options.js#newcode447
options.js:447: for (var property in subscription)
Detail: I know that this is valid JavaScript and I'm aware that we don't have
anything in that regard in the coding styles but it's a bit strange seeing this
Python-looking block in JavaScript because usually you would put braces around
blocks that include multiple lines.

It might be comparable to putting multiple statements into a single line (e.g.
`foo(), bar();`) which we would usually avoid even though it is valid JavaScript
and our coding style allows it.

https://codereview.adblockplus.org/29339395/diff/29339396/options.js#newcode448
options.js:448: if (property == "title" && subscription.url in
recommendationsMap)
Detail: I noticed that there's a variable `subscriptionsUrl` above. Feel free to
decide whether you want to reuse (and rename) or remove it.

[Sebastian Noack, 2016-04-06 17:26:30]

https://codereview.adblockplus.org/29339395/diff/29339396/options.js
File options.js (right):

https://codereview.adblockplus.org/29339395/diff/29339396/options.js#newcode447
options.js:447: for (var property in subscription)
On 2016/04/06 13:51:18, Thomas Greiner wrote:
> Detail: I know that this is valid JavaScript and I'm aware that we don't have
> anything in that regard in the coding styles but it's a bit strange seeing
this
> Python-looking block in JavaScript because usually you would put braces around
> blocks that include multiple lines.
> 
> It might be comparable to putting multiple statements into a single line (e.g.
> `foo(), bar();`) which we would usually avoid even though it is valid
JavaScript
> and our coding style allows it.

Done.

https://codereview.adblockplus.org/29339395/diff/29339396/options.js#newcode448
options.js:448: if (property == "title" && subscription.url in
recommendationsMap)
On 2016/04/06 13:51:18, Thomas Greiner wrote:
> Detail: I noticed that there's a variable `subscriptionsUrl` above. Feel free
to
> decide whether you want to reuse (and rename) or remove it.

Done.

https://codereview.adblockplus.org/29339395/diff/29339519/options.js
File options.js (right):

https://codereview.adblockplus.org/29339395/diff/29339519/options.js#newcode919
options.js:919: getPref("subscriptions_exceptionsurl", function(value)
Please ignore this change. It's not part of the patch, but due to rebasing.

[Thomas Greiner, 2016-04-06 18:06:05]

LGTM