Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(445)

Issue 29587717: Issue 5926 - Fix missing string errors (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 5 months ago by saroyanm
Modified:
2 years, 5 months ago
Reviewers:
ire
Visibility:
Public.

Description

This is a regression introduced by -> https://codereview.adblockplus.org/29572786/

Patch Set 1 : #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+1 line, -7 lines) Patch
M desktop-options.js View 1 chunk +1 line, -7 lines 4 comments Download

Messages

Total messages: 5
saroyanm
@Ire can you please have a look.
2 years, 5 months ago (2017-10-24 18:12:57 UTC) #1
ire
LGTM. One NIT/Suggestion which you're free to take or ignore :) https://codereview.adblockplus.org/29587717/diff/29587720/desktop-options.js File desktop-options.js (right): ...
2 years, 5 months ago (2017-10-25 09:12:52 UTC) #2
saroyanm
https://codereview.adblockplus.org/29587717/diff/29587720/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29587717/diff/29587720/desktop-options.js#newcode511 desktop-options.js:511: if (subscription.recommended != "ads") On 2017/10/25 09:12:52, ire wrote: ...
2 years, 5 months ago (2017-10-25 13:42:32 UTC) #3
saroyanm
https://codereview.adblockplus.org/29587717/diff/29587720/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29587717/diff/29587720/desktop-options.js#newcode511 desktop-options.js:511: if (subscription.recommended != "ads") On 2017/10/25 13:42:32, saroyanm wrote: ...
2 years, 5 months ago (2017-10-25 13:55:18 UTC) #4
ire
2 years, 5 months ago (2017-10-26 09:22:34 UTC) #5
Thanks!

LGTM again :)

https://codereview.adblockplus.org/29587717/diff/29587720/desktop-options.js
File desktop-options.js (right):

https://codereview.adblockplus.org/29587717/diff/29587720/desktop-options.js#...
desktop-options.js:511: if (subscription.recommended != "ads")
On 2017/10/25 13:42:32, saroyanm wrote:
> On 2017/10/25 09:12:52, ire wrote:
> > NIT/Suggestion: I think just checking that the type is no ads may be
encompass
> > more than we want (in the future). Assuming that a subscription could be
added
> > with a different type than ads, that doesn't necessarily mean it should be
> part
> > of this list. I think it would be clearer to check against a list of the
> > recommended names/types e.g. 
> > 
> > let recommendedSubscriptions = ["EasyPrivacy", "Fanboy's Social Blocking
> List"]
> > if (recommendedSubscriptions.includes(subscription.originalTitle))
> 
> Thanks for bringing this up, yes and no: Currently we do recommend(In the
> subscriptions.xml) Languages filter lists which are of type "ads" and two
other,
> which in this case "EasyPrivacy", "Fanboy's Social Blocking List", so with
> current logic whatever is not of type "ads" goes to the "PRIVACY & SECURITY"
> section, that was initial intention of current implementation if there were
more
> we might also consider changing the section title, but this was changed and in
> future we will also have custom titles for some other special filter lists
like
> ("Adblock warning removal list, Spam and etc.") which are shown in the More
> Filters section, but introduction of that logic will require change in the
> current implementation.
> 
> The current implementation that you see here I tried to make it consistent
with
> the current logic we have, see for example ->
> https://hg.adblockplus.org/adblockplusui/file/tip/desktop-options.js#l388 
> 
> So current logic says, what is not of type ads and recommended, treat
> differently - Move to the Privacy & Secutiry section (ex.: Recommendation
> section) and add custom titles to it.
> 
> In order to addd custom title to other filter lists we will need to add them
to
> the subscrioptions.xml as well, but in that case this logic will be changed to
> the one you have mentioned, or we might introduce additional attribute which
> will differentiate the "Privacy" related subscriptions.

Thanks very much for the explanation. I understand now and your implementation
looks like the most suitable for this case. If/when we decide to change the rule
of "whatever is not of type "ads" goes to the "PRIVACY & SECURITY" section",
then we can update this as well.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5