Issue 2824 - Only consider ads subscriptions in chooseFilterSubscription

Issue 2824 - Only consider ads subscriptions in chooseFilterSubscription

[Sebastian Noack, 2017-10-03 00:24:41]

https://codereview.adblockplus.org/29562595/diff/29562596/lib/subscriptionIni...
File lib/subscriptionInit.js (right):

https://codereview.adblockplus.org/29562595/diff/29562596/lib/subscriptionIni...
lib/subscriptionInit.js:84: * Returns the default filter subscriptions to add.
I wouldn't insist on adding comments for functions that are not exported. But if
you feel like adding a comment for this function, at least make sure that it
contains any more information that its name already does:

  Finds the element for the default ad blocking filter subscription based
  on the UI language.

https://codereview.adblockplus.org/29562595/diff/29562596/lib/subscriptionIni...
lib/subscriptionInit.js:99: let prefix = Utils.checkLocalePrefixMatch(
Since this seems to be the only call to that function in the whole code base, we
should move it into this module as well, or perhaps even inline the logic:

  let prefixes = subscription.getAttribute("prefixes");
  let prefix = prefixes && prefixes.split(",").find(
    prefix => new RegExp("^" + prefix + "\\b").test(Utils.appLocale)
  );

https://codereview.adblockplus.org/29562595/diff/29562596/lib/subscriptionIni...
lib/subscriptionInit.js:104: if (subscriptionType != "ads")
This could be combined with the check below:

  if (prefix && subscriptionType != "ads")
  {
    ...
  }

[wspee, 2017-10-03 09:26:55]

https://codereview.adblockplus.org/29562595/diff/29562596/lib/subscriptionIni...
File lib/subscriptionInit.js (right):

https://codereview.adblockplus.org/29562595/diff/29562596/lib/subscriptionIni...
lib/subscriptionInit.js:84: * Returns the default filter subscriptions to add.
On 2017/10/03 00:24:40, Sebastian Noack wrote:
> I wouldn't insist on adding comments for functions that are not exported. But
if
> you feel like adding a comment for this function, at least make sure that it
> contains any more information that its name already does:
> 
>   Finds the element for the default ad blocking filter subscription based
>   on the UI language.

Done.

https://codereview.adblockplus.org/29562595/diff/29562596/lib/subscriptionIni...
lib/subscriptionInit.js:99: let prefix = Utils.checkLocalePrefixMatch(
On 2017/10/03 00:24:40, Sebastian Noack wrote:
> Since this seems to be the only call to that function in the whole code base,
we
> should move it into this module as well, or perhaps even inline the logic:
> 
>   let prefixes = subscription.getAttribute("prefixes");
>   let prefix = prefixes && prefixes.split(",").find(
>     prefix => new RegExp("^" + prefix + "\\b").test(Utils.appLocale)
>   );

Done.

https://codereview.adblockplus.org/29562595/diff/29562596/lib/subscriptionIni...
lib/subscriptionInit.js:104: if (subscriptionType != "ads")
On 2017/10/03 00:24:40, Sebastian Noack wrote:
> This could be combined with the check below:
> 
>   if (prefix && subscriptionType != "ads")
>   {
>     ...
>   }

Done.

[Sebastian Noack, 2017-10-03 10:01:27]

https://codereview.adblockplus.org/29562595/diff/29563577/lib/subscriptionIni...
File lib/subscriptionInit.js (right):

https://codereview.adblockplus.org/29562595/diff/29563577/lib/subscriptionIni...
lib/subscriptionInit.js:85: * on the users locale.
Typo: users => user's

[wspee, 2017-10-03 10:14:46]

https://codereview.adblockplus.org/29562595/diff/29563577/lib/subscriptionIni...
File lib/subscriptionInit.js (right):

https://codereview.adblockplus.org/29562595/diff/29563577/lib/subscriptionIni...
lib/subscriptionInit.js:85: * on the users locale.
On 2017/10/03 10:01:27, Sebastian Noack wrote:
> Typo: users => user's

Done.

[Sebastian Noack, 2017-10-03 10:25:14]

LGTM