Issue 2668 - Use new subscription types in options page

Note that while the feature strings are now in a common translation file, I decided to not use those for the first-run page yet to avoid complications with existing translations.

I'll create a separate ticket about that which should be tackled as soon as #2834 is resolved.

[Thomas Greiner, 2015-09-25 15:43:22]

https://codereview.adblockplus.org/29328672/diff/29328673/options.js
File options.js (right):

https://codereview.adblockplus.org/29328672/diff/29328673/options.js#newcode321
options.js:321: var type = recommendation.type.replace(/\W/g, "_");
I needed to replace "-" with "_" for "common_feature_anti_adblock_title" so to
make it consistent with buildtools I also applied the same method in the block
above: https://hg.adblockplus.org/buildtools/file/tip/packagerChrome.py#l233

[saroyanm, 2015-11-10 16:47:25]

Just a few comments.

https://codereview.adblockplus.org/29328672/diff/29328673/options.js
File options.js (right):

https://codereview.adblockplus.org/29328672/diff/29328673/options.js#newcode321
options.js:321: var type = recommendation.type.replace(/\W/g, "_");
On 2015/09/25 15:43:22, Thomas Greiner wrote:
> I needed to replace "-" with "_" for "common_feature_anti_adblock_title" so to
> make it consistent with buildtools I also applied the same method in the block
> above: https://hg.adblockplus.org/buildtools/file/tip/packagerChrome.py#l233

Nit: What is the reason to assign the result to variable if you are not reusing
it ?

https://codereview.adblockplus.org/29328672/diff/29328673/subscriptions.xml
File subscriptions.xml (left):

https://codereview.adblockplus.org/29328672/diff/29328673/subscriptions.xml#o...
subscriptions.xml:122: homepage="https://easylist.adblockplus.org/"/>
What is the reason of removing this subscriptions ? Couldn't find reference in
the ticket. Also noticed some URLs are updated I assume it's intentional.

[Thomas Greiner, 2015-11-10 19:54:09]

https://codereview.adblockplus.org/29328672/diff/29328673/options.js
File options.js (right):

https://codereview.adblockplus.org/29328672/diff/29328673/options.js#newcode321
options.js:321: var type = recommendation.type.replace(/\W/g, "_");
On 2015/11/10 16:47:25, saroyanm wrote:
> On 2015/09/25 15:43:22, Thomas Greiner wrote:
> > I needed to replace "-" with "_" for "common_feature_anti_adblock_title" so
to
> > make it consistent with buildtools I also applied the same method in the
block
> > above: https://hg.adblockplus.org/buildtools/file/tip/packagerChrome.py#l233
> 
> Nit: What is the reason to assign the result to variable if you are not
reusing
> it ?

Just a personal preference to improve readability since the variable name
provides context to what this value is supposed to represent. I can replace
those two lines with the following if you prefer that:

subscription.title = ext.i18n.getMessage("common_feature_"
  + recommendation.type.replace(/\W/g, "_") + "_title");

https://codereview.adblockplus.org/29328672/diff/29328673/subscriptions.xml
File subscriptions.xml (left):

https://codereview.adblockplus.org/29328672/diff/29328673/subscriptions.xml#o...
subscriptions.xml:122: homepage="https://easylist.adblockplus.org/"/>
On 2015/11/10 16:47:25, saroyanm wrote:
> What is the reason of removing this subscriptions ? Couldn't find reference in
> the ticket.

This subscription is still in there.

> Also noticed some URLs are updated I assume it's intentional.

It's a rebased version of subscriptions.xml as we're currently shipping it with
the extension (that's why some URLs are different, etc.) but with the addition
of one subscription for each of the new subscription type.

[saroyanm, 2015-11-11 10:47:57]

LGTM

https://codereview.adblockplus.org/29328672/diff/29328673/options.js
File options.js (right):

https://codereview.adblockplus.org/29328672/diff/29328673/options.js#newcode321
options.js:321: var type = recommendation.type.replace(/\W/g, "_");
On 2015/11/10 19:54:09, Thomas Greiner wrote:
> On 2015/11/10 16:47:25, saroyanm wrote:
> > On 2015/09/25 15:43:22, Thomas Greiner wrote:
> > > I needed to replace "-" with "_" for "common_feature_anti_adblock_title"
so
> to
> > > make it consistent with buildtools I also applied the same method in the
> block
> > > above:
https://hg.adblockplus.org/buildtools/file/tip/packagerChrome.py#l233
> > 
> > Nit: What is the reason to assign the result to variable if you are not
> reusing
> > it ?
> 
> Just a personal preference to improve readability since the variable name
> provides context to what this value is supposed to represent. I can replace
> those two lines with the following if you prefer that:
> 
> subscription.title = ext.i18n.getMessage("common_feature_"
>   + recommendation.type.replace(/\W/g, "_") + "_title");

I see, well while nothing related in coding styles and I can see similar
implementation in the file feel free to keep it as it is right now.