Issue 29478597: Issue 5326 - General tab (HTML, strings and functionality) (Closed)
|
Created:
July 3, 2017, 3:43 p.m. by saroyanm Modified:
Aug. 23, 2017, 12:40 p.m. Reviewers:
Thomas Greiner Visibility:
Public. |
DescriptionThis review contains HTML/Semantics, strings and functional implementation excluding "Acceptable Ads notification" of General tab according to the specification ( https://bitbucket.org/adblockplus/spec/src/6eb0fc6e906a60ac76d93cf1ba9ae770e01f88df/spec/abp/options-page.md?at=master&fileviewer=file-view-default#markdown-header-general-tab ).
Patch Set 1 : #
Total comments: 109
Patch Set 2 : #
Total comments: 36
Patch Set 3 : #
Total comments: 13
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : rebase #
MessagesTotal messages: 19
Ready for review https://codereview.adblockplus.org/29478597/diff/29498701/background.js File background.js (right): https://codereview.adblockplus.org/29478597/diff/29498701/background.js#newco... background.js:106: subscriptions_exceptionsurl_privacy: "https://easylist-downloads.adblockplus.org/exceptionrules-privacy.txt" The URL is subject to change, as soon we will know exact URL. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:617: case "enable-aa": Adding acceptableAds multiple times, causes the "All filters list" to add acceptableAds multiple times. This still needs to be fixed. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:628: case "enable-privacy-aa": The "Acceptable Ads notification" I think will be better to implement in separate review, to keep current one small and I still have to figure some implementation details. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:1223: if (!navigator.doNotTrack) I couldn't find way to listen for "do not track" change. So if user will change the browser setting having options page opened, it will require the user to refresh the page to hide the message.
My main focus was on the JavaScript so I only did a rough review of the HTML and CSS parts since those will be tackled by the separate style review anyway. https://codereview.adblockplus.org/29478597/diff/29498701/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29478597/diff/29498701/locale/en-US/new-op... locale/en-US/new-options.json:58: "options_aa_header": { Detail: It'd be great if we could stay away from abbreviations because not everyone might be aware that "aa" here stands for "Acceptable Ads" so using more descriptive words such as "acceptable" could help make the code more readable. https://codereview.adblockplus.org/29478597/diff/29498701/locale/en-US/new-op... locale/en-US/new-options.json:60: "message": "ACCEPTABLE ADS" Detail: I thought we agreed on transforming headlines to upper-case via CSS rather than defining them as such in here. https://codereview.adblockplus.org/29478597/diff/29498701/locale/en-US/new-op... locale/en-US/new-options.json:76: "message": "<strong>Note:</strong> The ads collect data about your browsing habits <strong>_not_</strong> Adblock Plus." Detail: I assume that the underscores in "<strong>_not_</strong>" were unintentional because they're used in Markdown to emphasize text. https://codereview.adblockplus.org/29478597/diff/29498701/locale/en-US/new-op... locale/en-US/new-options.json:88: "message": "**Note:** You have **Do Not Track (DNT)** disabled in your Chrome settings. For this feature to work properly, please enable **DNT** in your browser preferences. <a>Find out how to turn on DNT</a>." Detail: We're not using Markdown syntax so please convert it into HTML. It seems that the spec generally mixes HTML with Markdown for some reason to mark up the strings so we should make that consistent. https://codereview.adblockplus.org/29478597/diff/29498701/locale/en-US/new-op... locale/en-US/new-options.json:128: "message": "+ add a languages" Typo: Replace "languages" with "language" https://codereview.adblockplus.org/29478597/diff/29498701/locale/en-US/new-op... locale/en-US/new-options.json:134: "message": "Malicious software (or 'malware') is software that is intended to gain access to and damage your computer. Computer viruses, worms, spyware and Trojan horses are all forms of malware." Detail: "(or 'malware')" is not using the same type of quotes as defined in the spec. https://codereview.adblockplus.org/29478597/diff/29498701/locale/en-US/new-op... locale/en-US/new-options.json:140: "message": "The social media buttons (or icons) on the websites that you visit allow social media networks to build a profile of you based on your browsing habits - even when you don’t click on them. Hide these buttons to protect your profile." Detail: Replace "’" with "'" https://codereview.adblockplus.org/29478597/diff/29498701/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29478597/diff/29498701/new-options.html#ne... new-options.html:106: <input data-action="enable-aa" type="radio" name="acceptable-ads" value="tracking"> Detail: I noticed that you created three messages ("block-all", "enable-aa" and "enable-privacy-aa"") which do pretty much the same thing (i.e. change the status of Acceptable Ads). Therefore I'd recommend grouping them together into a single message and determine what to do based on the value. For that I'd recommend using more useful values such as "ads", "ads,privacy", "privacy" and "". Thereby we'd also avoid using the word "tracking" because it implies that it's about allowing tracking rather than allowing ads. e.g. <input data-action="switch-acceptable" type="radio" name="acceptable-ads" value="ads,privacy"> https://codereview.adblockplus.org/29478597/diff/29498701/new-options.html#ne... new-options.html:115: <p id="no-dnt" class="i18n_options_aa_no_dnt_notification"></p> Suggestion: Can we drop the "no" in "no-dnt" and "options_aa_no_dnt_notification"? Because it could make it easier to understand and there's no other DNT-related notification that we have anyway. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.html#ne... new-options.html:349: <!-- Change language subscription --> This is pretty much exactly the same dialog as the one above. The only difference is the button action so it'd be great if you could avoid this duplication. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js File new-options.js (left): https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#oldc... new-options.js:1278: tooltip.className = "flip-" + flip; How are you planning to handle tooltip orientation in the future? https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:90: if (item.url === acceptableAdsUrl) Do we even need this special handling of Acceptable Ads subscriptions? We introduced it in #3907 but only because it its URL showed up but that may no longer be the case. So unless this still occurs or we need the subscription title to be translatable, we can get rid of this check. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:91: return getMessage("options_aa_tracking_label"); Detail: Let's be consistent with the naming scheme to clarify if things belong together. In this case you have `acceptableAdsUrl` and `acceptableAdsPrivacyUrl` so why not name the message IDs accordingly as "options_acceptableAds_label" and "options_acceptableAds_privacy_label". https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:144: if (tooltip && tooltip.hasAttribute("data-tooltip")) Detail: The second condition is redundant because `tooltip` is an element that matches the selector `[data-tooltip]`. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:146: if (item.recommended) According to the spec, not all subscriptions that have a tooltip are also recommended. Recommended: - Subscriptions of type "ads" - Fanboy's Annoyances - EasyPrivacy Tooltips: - Adware filter - Malware Domains - Spam404 - Adblock Warning Removal List So we need to either change the implementation or the spec. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:239: item.url == acceptableAdsPrivacyUrl) && Detail: This is already the third time that you're doing this check so let's create a function for that instead. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:357: switchSingleEntryControl: true Detail: This property appears to be unused. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:398: if (moreSubscriptions.indexOf(subscription.recommended) >= 0 && This is the opposite of what the spec says: "All filter lists that a user subscribes to which are not one of Adblock Plus's recommended filter lists" So none of the recommended filter lists should appear in the "More Filters" section. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:411: if (privacySubscriptions.indexOf(subscription.recommended) >= 0) Detail: You're only using this variable here and we don't expect to change its value anytime soon so why not just replace it with `if (subscription.recommended == "privacy" || subscription.recommended == "security"` to simplify the code? https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:411: if (privacySubscriptions.indexOf(subscription.recommended) >= 0) Detail: This naming doesn't make sense. You're putting "privacy subscriptions" into a "security collection". The section is called "Privacy & Security" so I don't think either is representative of this section so it'd be great if we could come up with a more generic and more accurate name (e.g. "protection"). Otherwise we could always fall back to "additional", "more" or "other". https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:433: return; Doesn't this mean that neither of the Acceptable Ads subscriptions will show up in any of the subscription lists? https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:596: setDntNotification(false); Placing this here may cause inconsistent behavior because it's not guaranteed that the extension will actually succeed in downloading, parsing and storing both filter lists. Therefore this needs to be placed in `onSubscriptionMessage()` which is where we get notified that the subscription has been installed successfully. On that note, please keep in mind that we also need to reset the value of the radio buttons if the subscription installation fails. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:604: url: subscriptionToChange This variable seems redundant because you should be able to determine that at runtime since it's the only language subscription that the user has installed. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:610: break; What if one of those two actions fails? In theory we could end up with no filter list being installed at all. So far we've never had two actions that were dependent on each other so we'd have to implement some kind of transactions or rethink our approach. Obviously, the same problems applies to switching Acceptable Ads filter lists. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:617: case "enable-aa": On 2017/07/26 20:56:50, saroyanm wrote: > Adding acceptableAds multiple times, causes the "All filters list" to add > acceptableAds multiple times. This still needs to be fixed. Acknowledged. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:628: case "enable-privacy-aa": On 2017/07/26 20:56:50, saroyanm wrote: > The "Acceptable Ads notification" I think will be better to implement in > separate review, to keep current one small and I still have to figure some > implementation details. Acknowledged. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:1094: addSubscription({ This looks odd. The subscription is not installed so why should we add it to the UI? https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:1099: // Load user subscriptions This code doesn't depend on the value of `urlPrivacy` so why do we need to wait for it before we can load user subscriptions? https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:1218: "[name='acceptable-ads'][value='tracking']").checked = true; Coding Style: "When an if statement, an else statement or a loop spans over more than one line always enclose it with braces." Same applies to the similar statement below. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:1218: "[name='acceptable-ads'][value='tracking']").checked = true; I noticed that you're only changing the value of `<input name="acceptable-ads">` when a subscription gets added but since the case exists where none of them is enabled we should also react to when a subscription gets removed. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:1219: if (subscription.url == acceptableAdsPrivacyUrl) Detail: This condition will never be `true` if the one above is so let's make this an `else if` instead to clarify that relation. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:1223: if (!navigator.doNotTrack) On 2017/07/26 20:56:50, saroyanm wrote: > I couldn't find way to listen for "do not track" change. So if user will change > the browser setting having options page opened, it will require the user to > refresh the page to hide the message. Users need to move away from the options page to change the setting so it should be sufficient to check only after they return to the page (e.g. using Page Visibility API). https://codereview.adblockplus.org/29478597/diff/29498701/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29478597/diff/29498701/skin/new-options.cs... skin/new-options.css:104: #dialog-body .table button[role="checkbox"] Detail: Why are those duplicated selectors for checkboxes necessary? On first glance it appears to work without it. https://codereview.adblockplus.org/29478597/diff/29498701/skin/new-options.cs... skin/new-options.css:381: .table li:first-child:nth-last-child(2) [data-single='hidden'] Detail: It's a nice trick to select an element if it's the only one but it's not easy to understand because you'd have to know that the last element will always be the `<template>` element. Therefore let's use `li:first-of-type:last-of-type` instead because it makes clear that we're only selecting the element if it's both the first and last `<li>`. https://codereview.adblockplus.org/29478597/diff/29498701/skin/new-options.cs... skin/new-options.css:386: .table li:first-child:nth-last-child(2) [data-single='visible'] Suggestion: You could move that selector into the rule above using `li:not(:first-of-type:last-of-type)` to avoid having to know whether it's supposed to be a block or an inline element.
https://codereview.adblockplus.org/29478597/diff/29498701/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29478597/diff/29498701/locale/en-US/new-op... locale/en-US/new-options.json:58: "options_aa_header": { On 2017/08/09 18:14:47, Thomas Greiner wrote: > Detail: It'd be great if we could stay away from abbreviations because not > everyone might be aware that "aa" here stands for "Acceptable Ads" so using more > descriptive words such as "acceptable" could help make the code more readable. Agree. https://codereview.adblockplus.org/29478597/diff/29498701/locale/en-US/new-op... locale/en-US/new-options.json:60: "message": "ACCEPTABLE ADS" On 2017/08/09 18:14:47, Thomas Greiner wrote: > Detail: I thought we agreed on transforming headlines to upper-case via CSS > rather than defining them as such in here. We did, I think I already have this here/forgot to change. Will update. https://codereview.adblockplus.org/29478597/diff/29498701/locale/en-US/new-op... locale/en-US/new-options.json:76: "message": "<strong>Note:</strong> The ads collect data about your browsing habits <strong>_not_</strong> Adblock Plus." On 2017/08/09 18:14:47, Thomas Greiner wrote: > Detail: I assume that the underscores in "<strong>_not_</strong>" were > unintentional because they're used in Markdown to emphasize text. Right, will change. https://codereview.adblockplus.org/29478597/diff/29498701/locale/en-US/new-op... locale/en-US/new-options.json:88: "message": "**Note:** You have **Do Not Track (DNT)** disabled in your Chrome settings. For this feature to work properly, please enable **DNT** in your browser preferences. <a>Find out how to turn on DNT</a>." On 2017/08/09 18:14:47, Thomas Greiner wrote: > Detail: We're not using Markdown syntax so please convert it into HTML. > > It seems that the spec generally mixes HTML with Markdown for some reason to > mark up the strings so we should make that consistent. Copy-past error, thanks. https://codereview.adblockplus.org/29478597/diff/29498701/locale/en-US/new-op... locale/en-US/new-options.json:128: "message": "+ add a languages" On 2017/08/09 18:14:47, Thomas Greiner wrote: > Typo: Replace "languages" with "language" Acknowledged. https://codereview.adblockplus.org/29478597/diff/29498701/locale/en-US/new-op... locale/en-US/new-options.json:134: "message": "Malicious software (or 'malware') is software that is intended to gain access to and damage your computer. Computer viruses, worms, spyware and Trojan horses are all forms of malware." On 2017/08/09 18:14:47, Thomas Greiner wrote: > Detail: "(or 'malware')" is not using the same type of quotes as defined in the > spec. Acknowledged. https://codereview.adblockplus.org/29478597/diff/29498701/locale/en-US/new-op... locale/en-US/new-options.json:140: "message": "The social media buttons (or icons) on the websites that you visit allow social media networks to build a profile of you based on your browsing habits - even when you don’t click on them. Hide these buttons to protect your profile." On 2017/08/09 18:14:47, Thomas Greiner wrote: > Detail: Replace "’" with "'" Acknowledged. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29478597/diff/29498701/new-options.html#ne... new-options.html:106: <input data-action="enable-aa" type="radio" name="acceptable-ads" value="tracking"> On 2017/08/09 18:14:48, Thomas Greiner wrote: > Detail: I noticed that you created three messages ("block-all", "enable-aa" and > "enable-privacy-aa"") which do pretty much the same thing (i.e. change the > status of Acceptable Ads). > > Therefore I'd recommend grouping them together into a single message and > determine what to do based on the value. For that I'd recommend using more > useful values such as "ads", "ads,privacy", "privacy" and "". > Thereby we'd also avoid using the word "tracking" because it implies that it's > about allowing tracking rather than allowing ads. > > e.g. > <input > data-action="switch-acceptable" > type="radio" > name="acceptable-ads" > value="ads,privacy"> I'll do in your suggested way. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.html#ne... new-options.html:115: <p id="no-dnt" class="i18n_options_aa_no_dnt_notification"></p> On 2017/08/09 18:14:48, Thomas Greiner wrote: > Suggestion: Can we drop the "no" in "no-dnt" and > "options_aa_no_dnt_notification"? Because it could make it easier to understand > and there's no other DNT-related notification that we have anyway. Acknowledged. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.html#ne... new-options.html:349: <!-- Change language subscription --> On 2017/08/09 18:14:48, Thomas Greiner wrote: > This is pretty much exactly the same dialog as the one above. The only > difference is the button action so it'd be great if you could avoid this > duplication. agree https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js File new-options.js (left): https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#oldc... new-options.js:1278: tooltip.className = "flip-" + flip; On 2017/08/09 18:14:48, Thomas Greiner wrote: > How are you planning to handle tooltip orientation in the future? Not yet, sure, but I think it make sense to re-implement, we don't need current solution anymore as beforehand we had two columns layout, while here only one in the main content. I will need to do bit of research before implementing the tooltips. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:90: if (item.url === acceptableAdsUrl) On 2017/08/09 18:14:49, Thomas Greiner wrote: > Do we even need this special handling of Acceptable Ads subscriptions? > > We introduced it in #3907 but only because it its URL showed up but that may no > longer be the case. So unless this still occurs or we need the subscription > title to be translatable, we can get rid of this check. I think that we want the title to be translated, also the title is slightly different in spec it's Allow "some"... the word "some" is missing in the original title. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:91: return getMessage("options_aa_tracking_label"); On 2017/08/09 18:14:49, Thomas Greiner wrote: > Detail: Let's be consistent with the naming scheme to clarify if things belong > together. In this case you have `acceptableAdsUrl` and `acceptableAdsPrivacyUrl` > so why not name the message IDs accordingly as "options_acceptableAds_label" and > "options_acceptableAds_privacy_label". Agree, I'll change. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:144: if (tooltip && tooltip.hasAttribute("data-tooltip")) On 2017/08/09 18:14:48, Thomas Greiner wrote: > Detail: The second condition is redundant because `tooltip` is an element that > matches the selector `[data-tooltip]`. Well spotted. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:146: if (item.recommended) On 2017/08/09 18:14:48, Thomas Greiner wrote: > According to the spec, not all subscriptions that have a tooltip are also > recommended. > > Recommended: > - Subscriptions of type "ads" > - Fanboy's Annoyances > - EasyPrivacy > > Tooltips: > - Adware filter > - Malware Domains > - Spam404 > - Adblock Warning Removal List > > So we need to either change the implementation or the spec. I agree, make sense to first check if the string for tooltip exists. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:239: item.url == acceptableAdsPrivacyUrl) && On 2017/08/09 18:14:50, Thomas Greiner wrote: > Detail: This is already the third time that you're doing this check so let's > create a function for that instead. agree. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:357: switchSingleEntryControl: true On 2017/08/09 18:14:48, Thomas Greiner wrote: > Detail: This property appears to be unused. right, will remove. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:398: if (moreSubscriptions.indexOf(subscription.recommended) >= 0 && On 2017/08/09 18:14:50, Thomas Greiner wrote: > This is the opposite of what the spec says: "All filter lists that a user > subscribes to which are not one of Adblock Plus's recommended filter lists" > > So none of the recommended filter lists should appear in the "More Filters" > section. Hmmm, good point.. I'll update this. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:411: if (privacySubscriptions.indexOf(subscription.recommended) >= 0) On 2017/08/09 18:14:49, Thomas Greiner wrote: > Detail: This naming doesn't make sense. You're putting "privacy subscriptions" > into a "security collection". > > The section is called "Privacy & Security" so I don't think either is > representative of this section so it'd be great if we could come up with a more > generic and more accurate name (e.g. "protection"). Otherwise we could always > fall back to "additional", "more" or "other". Good point. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:433: return; On 2017/08/09 18:14:50, Thomas Greiner wrote: > Doesn't this mean that neither of the Acceptable Ads subscriptions will show up > in any of the subscription lists? I think I tried to be consistent with previous implementation as we separately were adding acceptable ads subscription into "custom" collection, but not using current function.. I think we should be safe to move that implementation here.. I'm not sure why we were adding to custom collection separately, I'll check if that change will have unexpected implication. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:596: setDntNotification(false); On 2017/08/09 18:14:48, Thomas Greiner wrote: > Placing this here may cause inconsistent behavior because it's not guaranteed > that the extension will actually succeed in downloading, parsing and storing > both filter lists. > > Therefore this needs to be placed in `onSubscriptionMessage()` which is where we > get notified that the subscription has been installed successfully. > > On that note, please keep in mind that we also need to reset the value of the > radio buttons if the subscription installation fails. Noted https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:604: url: subscriptionToChange On 2017/08/09 18:14:50, Thomas Greiner wrote: > This variable seems redundant because you should be able to determine that at > runtime since it's the only language subscription that the user has installed. agree. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:610: break; On 2017/08/09 18:14:49, Thomas Greiner wrote: > What if one of those two actions fails? In theory we could end up with no filter > list being installed at all. So far we've never had two actions that were > dependent on each other so we'd have to implement some kind of transactions or > rethink our approach. > > Obviously, the same problems applies to switching Acceptable Ads filter lists. We still have a button for adding subscription, I think it make sense to keep robust error handling with notification, to the separate issue. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:1094: addSubscription({ On 2017/08/09 18:14:50, Thomas Greiner wrote: > This looks odd. The subscription is not installed so why should we add it to the > UI? Agree it's odd :) Will fix. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:1099: // Load user subscriptions On 2017/08/09 18:14:49, Thomas Greiner wrote: > This code doesn't depend on the value of `urlPrivacy` so why do we need to wait > for it before we can load user subscriptions? There are checks involved dependant on acceptableAdsPrivacyUrl URL, before adding subscription it's required in this case to know the URL for privacy friendly Acceptable Ads list. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:1218: "[name='acceptable-ads'][value='tracking']").checked = true; On 2017/08/09 18:14:49, Thomas Greiner wrote: > I noticed that you're only changing the value of `<input name="acceptable-ads">` > when a subscription gets added but since the case exists where none of them is > enabled we should also react to when a subscription gets removed. Agree. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:1219: if (subscription.url == acceptableAdsPrivacyUrl) On 2017/08/09 18:14:48, Thomas Greiner wrote: > Detail: This condition will never be `true` if the one above is so let's make > this an `else if` instead to clarify that relation. agree. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:1223: if (!navigator.doNotTrack) On 2017/08/09 18:14:49, Thomas Greiner wrote: > On 2017/07/26 20:56:50, saroyanm wrote: > > I couldn't find way to listen for "do not track" change. So if user will > change > > the browser setting having options page opened, it will require the user to > > refresh the page to hide the message. > > Users need to move away from the options page to change the setting so it should > be sufficient to check only after they return to the page (e.g. using Page > Visibility API). This is a good idea, but I think would be better to handle that separately. If you agree I'll create a separate issue for that, not to block current review with that implementation. https://codereview.adblockplus.org/29478597/diff/29498701/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29478597/diff/29498701/skin/new-options.cs... skin/new-options.css:104: #dialog-body .table button[role="checkbox"] On 2017/08/09 18:14:50, Thomas Greiner wrote: > Detail: Why are those duplicated selectors for checkboxes necessary? On first > glance it appears to work without it. I'll recheck, but I almost sure I did it because of specificity https://codereview.adblockplus.org/29478597/diff/29498701/skin/new-options.cs... skin/new-options.css:381: .table li:first-child:nth-last-child(2) [data-single='hidden'] On 2017/08/09 18:14:51, Thomas Greiner wrote: > Detail: It's a nice trick to select an element if it's the only one but it's not > easy to understand because you'd have to know that the last element will always > be the `<template>` element. > > Therefore let's use `li:first-of-type:last-of-type` instead because it makes > clear that we're only selecting the element if it's both the first and last > `<li>`. I'll try that. https://codereview.adblockplus.org/29478597/diff/29498701/skin/new-options.cs... skin/new-options.css:386: .table li:first-child:nth-last-child(2) [data-single='visible'] On 2017/08/09 18:14:50, Thomas Greiner wrote: > Suggestion: You could move that selector into the rule above using > `li:not(:first-of-type:last-of-type)` to avoid having to know whether it's > supposed to be a block or an inline element. Acknowledged.
https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:90: if (item.url === acceptableAdsUrl) On 2017/08/14 14:00:10, saroyanm wrote: > On 2017/08/09 18:14:49, Thomas Greiner wrote: > > Do we even need this special handling of Acceptable Ads subscriptions? > > > > We introduced it in #3907 but only because it its URL showed up but that may > no > > longer be the case. So unless this still occurs or we need the subscription > > title to be translatable, we can get rid of this check. > > I think that we want the title to be translated, also the title is slightly > different in spec it's Allow "some"... the word "some" is missing in the > original title. What's so bad about showing the original (i.e. untranslated) title that's defined in the filter list itself? It's only going to show up in the Advanced tab anyway AFAIK. There we also show the original titles for all the other filter lists. We may want to think about changing the title inside the filter list because "Allow non-intrusive advertising" is not an actual name but a description. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:433: return; On 2017/08/14 14:00:09, saroyanm wrote: > On 2017/08/09 18:14:50, Thomas Greiner wrote: > > Doesn't this mean that neither of the Acceptable Ads subscriptions will show > up > > in any of the subscription lists? > > I think I tried to be consistent with previous implementation as we separately > were adding acceptable ads subscription into "custom" collection, but not using > current function.. I think we should be safe to move that implementation here.. > I'm not sure why we were adding to custom collection separately, I'll check if > that change will have unexpected implication. Ok, thanks. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:610: break; On 2017/08/14 14:00:10, saroyanm wrote: > On 2017/08/09 18:14:49, Thomas Greiner wrote: > > What if one of those two actions fails? In theory we could end up with no > filter > > list being installed at all. So far we've never had two actions that were > > dependent on each other so we'd have to implement some kind of transactions or > > rethink our approach. > > > > Obviously, the same problems applies to switching Acceptable Ads filter lists. > > We still have a button for adding subscription, I think it make sense to keep > robust error handling with notification, to the separate issue. That's fine with me as long as we make sure that this gets tackled so that we not only communicate such errors to the user but also properly recover from them. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:1099: // Load user subscriptions On 2017/08/14 14:00:09, saroyanm wrote: > On 2017/08/09 18:14:49, Thomas Greiner wrote: > > This code doesn't depend on the value of `urlPrivacy` so why do we need to > wait > > for it before we can load user subscriptions? > > There are checks involved dependant on acceptableAdsPrivacyUrl URL, before > adding subscription it's required in this case to know the URL for privacy > friendly Acceptable Ads list. Which checks in particular are you referring to? https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:1223: if (!navigator.doNotTrack) On 2017/08/14 14:00:10, saroyanm wrote: > On 2017/08/09 18:14:49, Thomas Greiner wrote: > > On 2017/07/26 20:56:50, saroyanm wrote: > > > I couldn't find way to listen for "do not track" change. So if user will > > change > > > the browser setting having options page opened, it will require the user to > > > refresh the page to hide the message. > > > > Users need to move away from the options page to change the setting so it > should > > be sufficient to check only after they return to the page (e.g. using Page > > Visibility API). > > This is a good idea, but I think would be better to handle that separately. If > you agree I'll create a separate issue for that, not to block current review > with that implementation. Ok, sounds good. https://codereview.adblockplus.org/29478597/diff/29498701/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29478597/diff/29498701/skin/new-options.cs... skin/new-options.css:104: #dialog-body .table button[role="checkbox"] On 2017/08/14 14:00:11, saroyanm wrote: > On 2017/08/09 18:14:50, Thomas Greiner wrote: > > Detail: Why are those duplicated selectors for checkboxes necessary? On first > > glance it appears to work without it. > > I'll recheck, but I almost sure I did it because of specificity Acknowledged.
New patch uploaded. Ready for review. https://codereview.adblockplus.org/29478597/diff/29498701/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29478597/diff/29498701/locale/en-US/new-op... locale/en-US/new-options.json:58: "options_aa_header": { On 2017/08/09 18:14:47, Thomas Greiner wrote: > Detail: It'd be great if we could stay away from abbreviations because not > everyone might be aware that "aa" here stands for "Acceptable Ads" so using more > descriptive words such as "acceptable" could help make the code more readable. I used acceptabe_ads as already used that in some other places, let me know if you want me to shorten this. https://codereview.adblockplus.org/29478597/diff/29498701/locale/en-US/new-op... locale/en-US/new-options.json:60: "message": "ACCEPTABLE ADS" On 2017/08/14 14:00:08, saroyanm wrote: > On 2017/08/09 18:14:47, Thomas Greiner wrote: > > Detail: I thought we agreed on transforming headlines to upper-case via CSS > > rather than defining them as such in here. > > We did, I think I already have this here/forgot to change. Will update. Done. https://codereview.adblockplus.org/29478597/diff/29498701/locale/en-US/new-op... locale/en-US/new-options.json:76: "message": "<strong>Note:</strong> The ads collect data about your browsing habits <strong>_not_</strong> Adblock Plus." On 2017/08/14 14:00:07, saroyanm wrote: > On 2017/08/09 18:14:47, Thomas Greiner wrote: > > Detail: I assume that the underscores in "<strong>_not_</strong>" were > > unintentional because they're used in Markdown to emphasize text. > > Right, will change. Done. https://codereview.adblockplus.org/29478597/diff/29498701/locale/en-US/new-op... locale/en-US/new-options.json:88: "message": "**Note:** You have **Do Not Track (DNT)** disabled in your Chrome settings. For this feature to work properly, please enable **DNT** in your browser preferences. <a>Find out how to turn on DNT</a>." On 2017/08/14 14:00:08, saroyanm wrote: > On 2017/08/09 18:14:47, Thomas Greiner wrote: > > Detail: We're not using Markdown syntax so please convert it into HTML. > > > > It seems that the spec generally mixes HTML with Markdown for some reason to > > mark up the strings so we should make that consistent. > > Copy-past error, thanks. Done. https://codereview.adblockplus.org/29478597/diff/29498701/locale/en-US/new-op... locale/en-US/new-options.json:128: "message": "+ add a languages" On 2017/08/14 14:00:08, saroyanm wrote: > On 2017/08/09 18:14:47, Thomas Greiner wrote: > > Typo: Replace "languages" with "language" > > Acknowledged. Done. https://codereview.adblockplus.org/29478597/diff/29498701/locale/en-US/new-op... locale/en-US/new-options.json:134: "message": "Malicious software (or 'malware') is software that is intended to gain access to and damage your computer. Computer viruses, worms, spyware and Trojan horses are all forms of malware." On 2017/08/14 14:00:07, saroyanm wrote: > On 2017/08/09 18:14:47, Thomas Greiner wrote: > > Detail: "(or 'malware')" is not using the same type of quotes as defined in > the > > spec. > > Acknowledged. Done. https://codereview.adblockplus.org/29478597/diff/29498701/locale/en-US/new-op... locale/en-US/new-options.json:140: "message": "The social media buttons (or icons) on the websites that you visit allow social media networks to build a profile of you based on your browsing habits - even when you don’t click on them. Hide these buttons to protect your profile." On 2017/08/14 14:00:08, saroyanm wrote: > On 2017/08/09 18:14:47, Thomas Greiner wrote: > > Detail: Replace "’" with "'" > > Acknowledged. Done. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29478597/diff/29498701/new-options.html#ne... new-options.html:106: <input data-action="enable-aa" type="radio" name="acceptable-ads" value="tracking"> On 2017/08/09 18:14:48, Thomas Greiner wrote: > Detail: I noticed that you created three messages ("block-all", "enable-aa" and > "enable-privacy-aa"") which do pretty much the same thing (i.e. change the > status of Acceptable Ads). > > Therefore I'd recommend grouping them together into a single message and > determine what to do based on the value. For that I'd recommend using more > useful values such as "ads", "ads,privacy", "privacy" and "". > Thereby we'd also avoid using the word "tracking" because it implies that it's > about allowing tracking rather than allowing ads. > > e.g. > <input > data-action="switch-acceptable" > type="radio" > name="acceptable-ads" > value="ads,privacy"> Done, I used none, for the block all option value, to match the value using CSS selectors easily. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.html#ne... new-options.html:115: <p id="no-dnt" class="i18n_options_aa_no_dnt_notification"></p> On 2017/08/14 14:00:08, saroyanm wrote: > On 2017/08/09 18:14:48, Thomas Greiner wrote: > > Suggestion: Can we drop the "no" in "no-dnt" and > > "options_aa_no_dnt_notification"? Because it could make it easier to > understand > > and there's no other DNT-related notification that we have anyway. > > Acknowledged. Done. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.html#ne... new-options.html:349: <!-- Change language subscription --> On 2017/08/09 18:14:48, Thomas Greiner wrote: > This is pretty much exactly the same dialog as the one above. The only > difference is the button action so it'd be great if you could avoid this > duplication. Done. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:90: if (item.url === acceptableAdsUrl) On 2017/08/15 17:10:29, Thomas Greiner wrote: > On 2017/08/14 14:00:10, saroyanm wrote: > > On 2017/08/09 18:14:49, Thomas Greiner wrote: > > > Do we even need this special handling of Acceptable Ads subscriptions? > > > > > > We introduced it in #3907 but only because it its URL showed up but that may > > no > > > longer be the case. So unless this still occurs or we need the subscription > > > title to be translatable, we can get rid of this check. > > > > I think that we want the title to be translated, also the title is slightly > > different in spec it's Allow "some"... the word "some" is missing in the > > original title. > > What's so bad about showing the original (i.e. untranslated) title that's > defined in the filter list itself? It's only going to show up in the Advanced > tab anyway AFAIK. There we also show the original titles for all the other > filter lists. > > We may want to think about changing the title inside the filter list because > "Allow non-intrusive advertising" is not an actual name but a description. Nevermind I totally agree, I thought that this also applies to the title in the General tab, but now I see that it was about the Advanced tab. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:91: return getMessage("options_aa_tracking_label"); On 2017/08/14 14:00:09, saroyanm wrote: > On 2017/08/09 18:14:49, Thomas Greiner wrote: > > Detail: Let's be consistent with the naming scheme to clarify if things belong > > together. In this case you have `acceptableAdsUrl` and > `acceptableAdsPrivacyUrl` > > so why not name the message IDs accordingly as "options_acceptableAds_label" > and > > "options_acceptableAds_privacy_label". > > Agree, I'll change. Done. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:144: if (tooltip && tooltip.hasAttribute("data-tooltip")) On 2017/08/09 18:14:48, Thomas Greiner wrote: > Detail: The second condition is redundant because `tooltip` is an element that > matches the selector `[data-tooltip]`. Done. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:146: if (item.recommended) On 2017/08/09 18:14:48, Thomas Greiner wrote: > According to the spec, not all subscriptions that have a tooltip are also > recommended. > > Recommended: > - Subscriptions of type "ads" > - Fanboy's Annoyances > - EasyPrivacy > > Tooltips: > - Adware filter > - Malware Domains > - Spam404 > - Adblock Warning Removal List > > So we need to either change the implementation or the spec. Done. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:239: item.url == acceptableAdsPrivacyUrl) && On 2017/08/09 18:14:50, Thomas Greiner wrote: > Detail: This is already the third time that you're doing this check so let's > create a function for that instead. Done. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:357: switchSingleEntryControl: true On 2017/08/09 18:14:48, Thomas Greiner wrote: > Detail: This property appears to be unused. Done. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:398: if (moreSubscriptions.indexOf(subscription.recommended) >= 0 && On 2017/08/09 18:14:50, Thomas Greiner wrote: > This is the opposite of what the spec says: "All filter lists that a user > subscribes to which are not one of Adblock Plus's recommended filter lists" > > So none of the recommended filter lists should appear in the "More Filters" > section. Done. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:411: if (privacySubscriptions.indexOf(subscription.recommended) >= 0) On 2017/08/09 18:14:49, Thomas Greiner wrote: > Detail: This naming doesn't make sense. You're putting "privacy subscriptions" > into a "security collection". > > The section is called "Privacy & Security" so I don't think either is > representative of this section so it'd be great if we could come up with a more > generic and more accurate name (e.g. "protection"). Otherwise we could always > fall back to "additional", "more" or "other". Done. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:411: if (privacySubscriptions.indexOf(subscription.recommended) >= 0) On 2017/08/09 18:14:50, Thomas Greiner wrote: > Detail: You're only using this variable here Not anymore https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:433: return; On 2017/08/14 14:00:09, saroyanm wrote: > On 2017/08/09 18:14:50, Thomas Greiner wrote: > > Doesn't this mean that neither of the Acceptable Ads subscriptions will show > up > > in any of the subscription lists? > > I think I tried to be consistent with previous implementation as we separately > were adding acceptable ads subscription into "custom" collection, but not using > current function.. I think we should be safe to move that implementation here.. > I'm not sure why we were adding to custom collection separately, I'll check if > that change will have unexpected implication. I did updated the implementation on how subscriptions are added and updated, I guess now implementation makes more sense. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:596: setDntNotification(false); On 2017/08/09 18:14:48, Thomas Greiner wrote: > Placing this here may cause inconsistent behavior because it's not guaranteed > that the extension will actually succeed in downloading, parsing and storing > both filter lists. > > Therefore this needs to be placed in `onSubscriptionMessage()` which is where we > get notified that the subscription has been installed successfully. > > On that note, please keep in mind that we also need to reset the value of the > radio buttons if the subscription installation fails. Done. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:604: url: subscriptionToChange On 2017/08/09 18:14:50, Thomas Greiner wrote: > This variable seems redundant because you should be able to determine that at > runtime since it's the only language subscription that the user has installed. Done. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:1094: addSubscription({ On 2017/08/09 18:14:50, Thomas Greiner wrote: > This looks odd. The subscription is not installed so why should we add it to the > UI? Done. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:1099: // Load user subscriptions On 2017/08/15 17:10:29, Thomas Greiner wrote: > On 2017/08/14 14:00:09, saroyanm wrote: > > On 2017/08/09 18:14:49, Thomas Greiner wrote: > > > This code doesn't depend on the value of `urlPrivacy` so why do we need to > > wait > > > for it before we can load user subscriptions? > > > > There are checks involved dependant on acceptableAdsPrivacyUrl URL, before > > adding subscription it's required in this case to know the URL for privacy > > friendly Acceptable Ads list. > > Which checks in particular are you referring to? To checks in "onSubscriptionMessage", we do same for the "acceptableAdsUrl" URL. So the idea is to first get the "Acceptable Ads" and "Privacy Friendly Acceptable Ads" URLs and after load the subscription. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:1218: "[name='acceptable-ads'][value='tracking']").checked = true; On 2017/08/09 18:14:49, Thomas Greiner wrote: > I noticed that you're only changing the value of `<input name="acceptable-ads">` > when a subscription gets added but since the case exists where none of them is > enabled we should also react to when a subscription gets removed. Done. https://codereview.adblockplus.org/29478597/diff/29498701/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29478597/diff/29498701/skin/new-options.cs... skin/new-options.css:104: #dialog-body .table button[role="checkbox"] On 2017/08/15 17:10:30, Thomas Greiner wrote: > On 2017/08/14 14:00:11, saroyanm wrote: > > On 2017/08/09 18:14:50, Thomas Greiner wrote: > > > Detail: Why are those duplicated selectors for checkboxes necessary? On > first > > > glance it appears to work without it. > > > > I'll recheck, but I almost sure I did it because of specificity > > Acknowledged. Yes it is because of specificity, anyway I'll revisit the Dialog styles separately. https://codereview.adblockplus.org/29478597/diff/29498701/skin/new-options.cs... skin/new-options.css:381: .table li:first-child:nth-last-child(2) [data-single='hidden'] On 2017/08/14 14:00:10, saroyanm wrote: > On 2017/08/09 18:14:51, Thomas Greiner wrote: > > Detail: It's a nice trick to select an element if it's the only one but it's > not > > easy to understand because you'd have to know that the last element will > always > > be the `<template>` element. > > > > Therefore let's use `li:first-of-type:last-of-type` instead because it makes > > clear that we're only selecting the element if it's both the first and last > > `<li>`. > > I'll try that. Done. https://codereview.adblockplus.org/29478597/diff/29498701/skin/new-options.cs... skin/new-options.css:386: .table li:first-child:nth-last-child(2) [data-single='visible'] On 2017/08/09 18:14:50, Thomas Greiner wrote: > Suggestion: You could move that selector into the rule above using > `li:not(:first-of-type:last-of-type)` to avoid having to know whether it's > supposed to be a block or an inline element. Apparently it's not possible to have two pseudo classes in the :not
https://codereview.adblockplus.org/29478597/diff/29498701/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29478597/diff/29498701/locale/en-US/new-op... locale/en-US/new-options.json:58: "options_aa_header": { On 2017/08/16 14:17:35, saroyanm wrote: > On 2017/08/09 18:14:47, Thomas Greiner wrote: > > Detail: It'd be great if we could stay away from abbreviations because not > > everyone might be aware that "aa" here stands for "Acceptable Ads" so using > more > > descriptive words such as "acceptable" could help make the code more readable. > > I used acceptabe_ads as already used that in some other places, let me know if > you want me to shorten this. Nah, no need to shorten. It looks good the way it is. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:1099: // Load user subscriptions On 2017/08/16 14:17:37, saroyanm wrote: > On 2017/08/15 17:10:29, Thomas Greiner wrote: > > On 2017/08/14 14:00:09, saroyanm wrote: > > > On 2017/08/09 18:14:49, Thomas Greiner wrote: > > > > This code doesn't depend on the value of `urlPrivacy` so why do we need to > > > wait > > > > for it before we can load user subscriptions? > > > > > > There are checks involved dependant on acceptableAdsPrivacyUrl URL, before > > > adding subscription it's required in this case to know the URL for privacy > > > friendly Acceptable Ads list. > > > > Which checks in particular are you referring to? > > To checks in "onSubscriptionMessage", we do same for the "acceptableAdsUrl" URL. > > So the idea is to first get the "Acceptable Ads" and "Privacy Friendly > Acceptable Ads" URLs and after load the subscription. Ok, makes sense. https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... new-options.js:1219: if (subscription.url == acceptableAdsPrivacyUrl) On 2017/08/14 14:00:10, saroyanm wrote: > On 2017/08/09 18:14:48, Thomas Greiner wrote: > > Detail: This condition will never be `true` if the one above is so let's make > > this an `else if` instead to clarify that relation. > > agree. Maybe I'm missing something but I can't find the change. https://codereview.adblockplus.org/29478597/diff/29498701/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29478597/diff/29498701/skin/new-options.cs... skin/new-options.css:104: #dialog-body .table button[role="checkbox"] On 2017/08/16 14:17:39, saroyanm wrote: > On 2017/08/15 17:10:30, Thomas Greiner wrote: > > On 2017/08/14 14:00:11, saroyanm wrote: > > > On 2017/08/09 18:14:50, Thomas Greiner wrote: > > > > Detail: Why are those duplicated selectors for checkboxes necessary? On > > first > > > > glance it appears to work without it. > > > > > > I'll recheck, but I almost sure I did it because of specificity > > > > Acknowledged. > > Yes it is because of specificity, anyway I'll revisit the Dialog styles > separately. Acknowledged. https://codereview.adblockplus.org/29478597/diff/29498701/skin/new-options.cs... skin/new-options.css:386: .table li:first-child:nth-last-child(2) [data-single='visible'] On 2017/08/16 14:17:38, saroyanm wrote: > On 2017/08/09 18:14:50, Thomas Greiner wrote: > > Suggestion: You could move that selector into the rule above using > > `li:not(:first-of-type:last-of-type)` to avoid having to know whether it's > > supposed to be a block or an inline element. > > Apparently it's not possible to have two pseudo classes in the :not Interesting, I wasn't aware of that. They only accept simple selectors according to https://www.w3.org/TR/css3-selectors/#negation so you're right. FYI: The upcoming Selectors Level 4 standard appears to change that so it accepts a selector list instead. See https://www.w3.org/TR/selectors4/#negation https://codereview.adblockplus.org/29478597/diff/29517651/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29478597/diff/29517651/locale/en-US/new-op... locale/en-US/new-options.json:70: "options_acceptable_ads_tracking_description_1": { Detail: There are still a couple of strings whose ID contains "tracking" instead of "privacy". See https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newc... new-options.js:223: for (let displayElement of element.querySelectorAll(".display")) According to https://developer.mozilla.org/en-US/docs/Web/API/NodeList#Browser_compatibility `NodeList` wasn't iterable yet in Chrome 49 which is the oldest version we still support. Therefore we shouldn't use a for-of loop here. https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newc... new-options.js:383: collection = collections.security; Detail: The previous comments for this still applies. See https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newc... new-options.js:394: collection = collections.custom; So we're still putting recommended subscriptions into the "More Filters" section? See https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newc... new-options.js:396: else This part of the if-statement is redundant because that's what's happening at the end of the function anyway. https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newc... new-options.js:431: collections.custom.addItem(subscription); Again, it seems like we're still putting recommended subscriptions into the "More Filters" section. See https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newc... new-options.js:594: } Detail: Let's exit the loop as soon as we find the filter list we've been searching for. https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newc... new-options.js:648: case "switch-acceptable-ads": It's great that the code has been made more consistent but this is not what I meant with my comment at https://codereview.adblockplus.org/29478597/diff/29498701/new-options.html#ne.... Here's an example code to clarify what I was thinking of: case "switch-acceptable-ads": let values = new Set(element.value.split(",")); ext.backgroundPage.sendMessage({ type: (values.has("privacy")) ? "subscriptions.add" : "subscriptions.remove", url: acceptableAdsPrivacyUrl }); ext.backgroundPage.sendMessage({ type: (values.has("ads")) ? "subscriptions.add" : "subscriptions.remove" }); break; https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newc... new-options.js:1232: setAcceptableAds("default"); We cannot determine the current state based on which filter list got added last because the other one could still be present. Therefore I'd recommend checking which of the two filters lists are installed and enabled each time any of them gets added, removed, enabled or disabled (e.g. `if (isAcceptable(subscription.url) refreshAcceptableAds()`). My suggestion for the states: - aa:false + aa-privacy:false = all ads blocked - aa:true + aa-privacy:false = all Acceptable Ads unblocked - aa:false + aa-privacy:true = only privacy-friendly Acceptable Ads unblocked - aa:true + aa-privacy:true = all Acceptable Ads unblocked Because since the privacy-friendly list is only a subset of the regular one, having both enabled at the same time is equivalent to only having the regular one enabled. https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newc... new-options.js:1260: collections.custom.removeItem(subscription); Detail: Recommended subscriptions should not be in the "More Filters" section anyway. https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newc... new-options.js:1261: delete subscriptionsMap[subscription.url]; In `addSubscription()` we're adding Acceptable Ads subscriptions to `subscriptionsMap` so why aren't we removing them here?
Thanks for reviewing Thomas, please see my comments. https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newc... new-options.js:223: for (let displayElement of element.querySelectorAll(".display")) On 2017/08/16 17:57:09, Thomas Greiner wrote: > According to > https://developer.mozilla.org/en-US/docs/Web/API/NodeList#Browser_compatibility > `NodeList` wasn't iterable yet in Chrome 49 which is the oldest version we still > support. Therefore we shouldn't use a for-of loop here. well spotted. https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newc... new-options.js:394: collection = collections.custom; On 2017/08/16 17:57:10, Thomas Greiner wrote: > So we're still putting recommended subscriptions into the "More Filters" > section? > > See > https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... Yes, because according to the subscription.xml "Malware Domains" and "Adblock Warning Removal List" are also in the recommendation, but not recommended in the specifications. So currently we show in More Filters section the filters as specified below: * Recommended subscriptions that aren't "Protecting filters" and Adblocking filters * Subscriptions that aren't recommended and aren't "AcceptableAds" subscriptions. https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newc... new-options.js:396: else On 2017/08/16 17:57:09, Thomas Greiner wrote: > This part of the if-statement is redundant because that's what's happening at > the end of the function anyway. agree. https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newc... new-options.js:431: collections.custom.addItem(subscription); On 2017/08/16 17:57:09, Thomas Greiner wrote: > Again, it seems like we're still putting recommended subscriptions into the > "More Filters" section. > > See > https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... As, same as the comment above, the definition of the recommended in the specs is "not recommended within the Privacy & Security and Language sections". https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newc... new-options.js:648: case "switch-acceptable-ads": On 2017/08/16 17:57:10, Thomas Greiner wrote: > It's great that the code has been made more consistent but this is not what I > meant with my comment at > https://codereview.adblockplus.org/29478597/diff/29498701/new-options.html#ne.... > > Here's an example code to clarify what I was thinking of: > > case "switch-acceptable-ads": > let values = new Set(element.value.split(",")); > ext.backgroundPage.sendMessage({ > type: (values.has("privacy")) ? "subscriptions.add" : > "subscriptions.remove", > url: acceptableAdsPrivacyUrl > }); > ext.backgroundPage.sendMessage({ > type: (values.has("ads")) ? "subscriptions.add" : "subscriptions.remove" > }); > break; I think I missed the part of combining them into one message, I agree with you, but we don't have the case for "ads,privacy", also not sure if we should allow that case and having value="" is still have the issue of selecting with CSS. What if we will still use your solution, but the way it matches the case ? let value = element.value; ext.backgroundPage.sendMessage({ type: value == "privacy" ? "subscriptions.add" : "subscriptions.remove", url: acceptableAdsPrivacyUrl }); ext.backgroundPage.sendMessage({ type: value == "ads" ? "subscriptions.add" : "subscriptions.remove" }); And values in HTML will be "ads", "privacy" and "none". <input data-action="switch-acceptable-ads" type="radio" name="acceptable-ads" value="ads"> <input data-action="switch-acceptable-ads" type="radio" name="acceptable-ads" value="privacy"> <input data-action="switch-acceptable-ads" type="radio" name="acceptable-ads" value="none"> Let me know what you think about this and thanks for clarification. https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newc... new-options.js:1232: setAcceptableAds("default"); On 2017/08/16 17:57:10, Thomas Greiner wrote: > We cannot determine the current state based on which filter list got added last > because the other one could still be present. > > Therefore I'd recommend checking which of the two filters lists are installed > and enabled each time any of them gets added, removed, enabled or disabled (e.g. > `if (isAcceptable(subscription.url) refreshAcceptableAds()`). > > My suggestion for the states: > - aa:false + aa-privacy:false = all ads blocked > - aa:true + aa-privacy:false = all Acceptable Ads unblocked > - aa:false + aa-privacy:true = only privacy-friendly Acceptable Ads unblocked > - aa:true + aa-privacy:true = all Acceptable Ads unblocked > > Because since the privacy-friendly list is only a subset of the regular one, > having both enabled at the same time is equivalent to only having the regular > one enabled. I agree. I'll update accordingly. https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newc... new-options.js:1261: delete subscriptionsMap[subscription.url]; On 2017/08/16 17:57:10, Thomas Greiner wrote: > In `addSubscription()` we're adding Acceptable Ads subscriptions to > `subscriptionsMap` so why aren't we removing them here? I tried to be consistent with old implementation here I think... But I do agree that we should remove it, I'll check if that will couse any sort of unexpected behavior, but I think we should be safe here, also same should apply to some recommended subscriptions that are not "ads" and "protection" type.
https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newc... new-options.js:394: collection = collections.custom; On 2017/08/17 11:21:19, saroyanm wrote: > On 2017/08/16 17:57:10, Thomas Greiner wrote: > > So we're still putting recommended subscriptions into the "More Filters" > > section? > > > > See > > > https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... > > Yes, because according to the subscription.xml "Malware Domains" and "Adblock > Warning Removal List" are also in the recommendation, but not recommended in the > specifications. So currently we show in More Filters section the filters as > specified below: > * Recommended subscriptions that aren't "Protecting filters" and Adblocking > filters > * Subscriptions that aren't recommended and aren't "AcceptableAds" > subscriptions. If they are in subscriptions.xml but are not supposed to be recommended then let's simply not include them in subscriptions.xml and adapt https://issues.adblockplus.org/ticket/2825 accordingly. https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newc... new-options.js:648: case "switch-acceptable-ads": On 2017/08/17 11:21:18, saroyanm wrote: > On 2017/08/16 17:57:10, Thomas Greiner wrote: > > It's great that the code has been made more consistent but this is not what I > > meant with my comment at > > > https://codereview.adblockplus.org/29478597/diff/29498701/new-options.html#ne.... > > > > Here's an example code to clarify what I was thinking of: > > > > case "switch-acceptable-ads": > > let values = new Set(element.value.split(",")); > > ext.backgroundPage.sendMessage({ > > type: (values.has("privacy")) ? "subscriptions.add" : > > "subscriptions.remove", > > url: acceptableAdsPrivacyUrl > > }); > > ext.backgroundPage.sendMessage({ > > type: (values.has("ads")) ? "subscriptions.add" : "subscriptions.remove" > > }); > > break; > > I think I missed the part of combining them into one message, I agree with you, > but we don't have the case for "ads,privacy", also not sure if we should allow > that case and having value="" is still have the issue of selecting with CSS. > What if we will still use your solution, but the way it matches the case ? > > let value = element.value; > ext.backgroundPage.sendMessage({ > type: value == "privacy" ? "subscriptions.add" : > "subscriptions.remove", > url: acceptableAdsPrivacyUrl > }); > ext.backgroundPage.sendMessage({ > type: value == "ads" ? "subscriptions.add" : "subscriptions.remove" > }); > > And values in HTML will be "ads", "privacy" and "none". > <input data-action="switch-acceptable-ads" type="radio" name="acceptable-ads" > value="ads"> > <input data-action="switch-acceptable-ads" type="radio" name="acceptable-ads" > value="privacy"> > <input data-action="switch-acceptable-ads" type="radio" name="acceptable-ads" > value="none"> > > Let me know what you think about this and thanks for clarification. Your proposal is perfectly fine. :) https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newc... new-options.js:1261: delete subscriptionsMap[subscription.url]; On 2017/08/17 11:21:17, saroyanm wrote: > On 2017/08/16 17:57:10, Thomas Greiner wrote: > > In `addSubscription()` we're adding Acceptable Ads subscriptions to > > `subscriptionsMap` so why aren't we removing them here? > > I tried to be consistent with old implementation here I think... > But I do agree that we should remove it, I'll check if that will couse any sort > of unexpected behavior, but I think we should be safe here, also same should > apply to some recommended subscriptions that are not "ads" and "protection" > type. Sounds good.
https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newc... new-options.js:394: collection = collections.custom; On 2017/08/17 12:06:05, Thomas Greiner wrote: > On 2017/08/17 11:21:19, saroyanm wrote: > > On 2017/08/16 17:57:10, Thomas Greiner wrote: > > > So we're still putting recommended subscriptions into the "More Filters" > > > section? > > > > > > See > > > > > > https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... > > > > Yes, because according to the subscription.xml "Malware Domains" and "Adblock > > Warning Removal List" are also in the recommendation, but not recommended in > the > > specifications. So currently we show in More Filters section the filters as > > specified below: > > * Recommended subscriptions that aren't "Protecting filters" and Adblocking > > filters > > * Subscriptions that aren't recommended and aren't "AcceptableAds" > > subscriptions. > > If they are in subscriptions.xml but are not supposed to be recommended then > let's simply not include them in subscriptions.xml and adapt > https://issues.adblockplus.org/ticket/2825 accordingly. Is it the decision that we can make ? If so I'm fine and will change the implementation accordingly, but on other hand what about other locations where we recommend them (ex.: https://adblockplus.org/subscriptions) ? Isn't the idea to have 1 subscriptions.xml in the end shared across products/websites and etc ?
https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newc... new-options.js:394: collection = collections.custom; On 2017/08/17 12:32:08, saroyanm wrote: > Is it the decision that we can make ? > > If so I'm fine and will change the implementation accordingly, but on other hand > what about other locations where we recommend them (ex.: > https://adblockplus.org/subscriptions) ? We are the ones who added them in the first place so those are not being used anywhere (see https://issues.adblockplus.org/ticket/2825). No need to worry. :) Ensuring that introducing those new non-ads subscription types doesn't cause any negative side effects is what https://issues.adblockplus.org/ticket/2821 is for. Also note that the subscriptions.xml in the adblockplusui repository is only being used for the development environment so changes to that one don't affect anything else. The extensions bundle their own version of subscriptions.xml. > Isn't the idea to have 1 subscriptions.xml in the end shared across products/websites and etc ? No, that's what we have https://hg.adblockplus.org/subscriptionlist for. It provides a single source for generating subscriptions.xml, subscriptions2.xml and adblockplus.org/subscriptions.
https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newc... new-options.js:394: collection = collections.custom; On 2017/08/17 12:49:06, Thomas Greiner wrote: > On 2017/08/17 12:32:08, saroyanm wrote: > > Is it the decision that we can make ? > > > > If so I'm fine and will change the implementation accordingly, but on other > hand > > what about other locations where we recommend them (ex.: > > https://adblockplus.org/subscriptions) ? > > We are the ones who added them in the first place so those are not being used > anywhere (see https://issues.adblockplus.org/ticket/2825). No need to worry. :) > > Ensuring that introducing those new non-ads subscription types doesn't cause any > negative side effects is what https://issues.adblockplus.org/ticket/2821 is for. > > Also note that the subscriptions.xml in the adblockplusui repository is only > being used for the development environment so changes to that one don't affect > anything else. The extensions bundle their own version of subscriptions.xml. > > > Isn't the idea to have 1 subscriptions.xml in the end shared across > products/websites and etc ? > > No, that's what we have https://hg.adblockplus.org/subscriptionlist for. It > provides a single source for generating subscriptions.xml, subscriptions2.xml > and adblockplus.org/subscriptions. Thanks for clarifying that, in this case whole logic should be supper simple in general: Privacy and Security subscriptions = recommendation which are not ads type. More subscriptions = Not a recommendation and not Acceptable Ads subscription. Saying that I think we even can close -> https://issues.adblockplus.org/ticket/2825 because we will need to distinguish between Ads type and non Ads type recommendation. What you think ?
On 2017/08/17 14:06:18, saroyanm wrote: > Thanks for clarifying that, in this case whole logic should be supper simple in > general: > Privacy and Security subscriptions = recommendation which are not ads type. > More subscriptions = Not a recommendation and not Acceptable Ads subscription. Yep, looks good. > Saying that I think we even can close -> > https://issues.adblockplus.org/ticket/2825 because we will need to distinguish > between Ads type and non Ads type recommendation. What you think ? No, we shouldn't close that ticket because… 1. We still need to mark certain filter lists as "recommended". 2. Currently, EasyPrivacy has the type "ads" which will cause issues for us so this ticket will change its type to "privacy". 3. While it looks like we won't be using any of the new types anymore in the options page, they'll still provide value for adblockplus.org/subscriptions because the filter lists will appear in more descriptive sections such as "Privacy" rather than all of them being put under "Miscellaneous".
On 2017/08/17 14:53:11, Thomas Greiner wrote: > On 2017/08/17 14:06:18, saroyanm wrote: > > Thanks for clarifying that, in this case whole logic should be supper simple > in > > general: > > Privacy and Security subscriptions = recommendation which are not ads type. > > More subscriptions = Not a recommendation and not Acceptable Ads subscription. > > Yep, looks good. > > > Saying that I think we even can close -> > > https://issues.adblockplus.org/ticket/2825 because we will need to distinguish > > between Ads type and non Ads type recommendation. What you think ? > > No, we shouldn't close that ticket because… > > 1. We still need to mark certain filter lists as "recommended". > 2. Currently, EasyPrivacy has the type "ads" which will cause issues for us so > this ticket will change its type to "privacy". > 3. While it looks like we won't be using any of the new types anymore in the > options page, they'll still provide value for adblockplus.org/subscriptions > because the filter lists will appear in more descriptive sections such as > "Privacy" rather than all of them being put under "Miscellaneous". Thanks, I've updated the #2825 and now I have everything to prepare new patch, I'll upload the new patch later today.
New patch ready for the review. I do have one small issue, which I want to discuss regarding the tooltips of "More Subscriptions" https://codereview.adblockplus.org/29478597/diff/29517651/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29478597/diff/29517651/locale/en-US/new-op... locale/en-US/new-options.json:70: "options_acceptable_ads_tracking_description_1": { On 2017/08/16 17:57:09, Thomas Greiner wrote: > Detail: There are still a couple of strings whose ID contains "tracking" instead > of "privacy". > > See > https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... Done. https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newc... new-options.js:223: for (let displayElement of element.querySelectorAll(".display")) On 2017/08/17 11:21:17, saroyanm wrote: > On 2017/08/16 17:57:09, Thomas Greiner wrote: > > According to > > > https://developer.mozilla.org/en-US/docs/Web/API/NodeList#Browser_compatibility > > `NodeList` wasn't iterable yet in Chrome 49 which is the oldest version we > still > > support. Therefore we shouldn't use a for-of loop here. > > well spotted. Done. https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newc... new-options.js:383: collection = collections.security; On 2017/08/16 17:57:10, Thomas Greiner wrote: > Detail: The previous comments for this still applies. > > See > https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... Done. https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newc... new-options.js:394: collection = collections.custom; On 2017/08/17 12:49:06, Thomas Greiner wrote: > On 2017/08/17 12:32:08, saroyanm wrote: > > Is it the decision that we can make ? > > > > If so I'm fine and will change the implementation accordingly, but on other > hand > > what about other locations where we recommend them (ex.: > > https://adblockplus.org/subscriptions) ? > > We are the ones who added them in the first place so those are not being used > anywhere (see https://issues.adblockplus.org/ticket/2825). No need to worry. :) > > Ensuring that introducing those new non-ads subscription types doesn't cause any > negative side effects is what https://issues.adblockplus.org/ticket/2821 is for. > > Also note that the subscriptions.xml in the adblockplusui repository is only > being used for the development environment so changes to that one don't affect > anything else. The extensions bundle their own version of subscriptions.xml. > > > Isn't the idea to have 1 subscriptions.xml in the end shared across > products/websites and etc ? > > No, that's what we have https://hg.adblockplus.org/subscriptionlist for. It > provides a single source for generating subscriptions.xml, subscriptions2.xml > and adblockplus.org/subscriptions. Done. https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newc... new-options.js:396: else On 2017/08/17 11:21:18, saroyanm wrote: > On 2017/08/16 17:57:09, Thomas Greiner wrote: > > This part of the if-statement is redundant because that's what's happening at > > the end of the function anyway. > > agree. Done. https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newc... new-options.js:431: collections.custom.addItem(subscription); On 2017/08/16 17:57:09, Thomas Greiner wrote: > Again, it seems like we're still putting recommended subscriptions into the > "More Filters" section. > > See > https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc... Done. https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newc... new-options.js:594: } On 2017/08/16 17:57:10, Thomas Greiner wrote: > Detail: Let's exit the loop as soon as we find the filter list we've been > searching for. Done. https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newc... new-options.js:648: case "switch-acceptable-ads": On 2017/08/17 12:06:05, Thomas Greiner wrote: > On 2017/08/17 11:21:18, saroyanm wrote: > > On 2017/08/16 17:57:10, Thomas Greiner wrote: > > > It's great that the code has been made more consistent but this is not what > I > > > meant with my comment at > > > > > > https://codereview.adblockplus.org/29478597/diff/29498701/new-options.html#ne.... > > > > > > Here's an example code to clarify what I was thinking of: > > > > > > case "switch-acceptable-ads": > > > let values = new Set(element.value.split(",")); > > > ext.backgroundPage.sendMessage({ > > > type: (values.has("privacy")) ? "subscriptions.add" : > > > "subscriptions.remove", > > > url: acceptableAdsPrivacyUrl > > > }); > > > ext.backgroundPage.sendMessage({ > > > type: (values.has("ads")) ? "subscriptions.add" : "subscriptions.remove" > > > }); > > > break; > > > > I think I missed the part of combining them into one message, I agree with > you, > > but we don't have the case for "ads,privacy", also not sure if we should allow > > that case and having value="" is still have the issue of selecting with CSS. > > What if we will still use your solution, but the way it matches the case ? > > > > let value = element.value; > > ext.backgroundPage.sendMessage({ > > type: value == "privacy" ? "subscriptions.add" : > > "subscriptions.remove", > > url: acceptableAdsPrivacyUrl > > }); > > ext.backgroundPage.sendMessage({ > > type: value == "ads" ? "subscriptions.add" : "subscriptions.remove" > > }); > > > > And values in HTML will be "ads", "privacy" and "none". > > <input data-action="switch-acceptable-ads" type="radio" name="acceptable-ads" > > value="ads"> > > <input data-action="switch-acceptable-ads" type="radio" name="acceptable-ads" > > value="privacy"> > > <input data-action="switch-acceptable-ads" type="radio" name="acceptable-ads" > > value="none"> > > > > Let me know what you think about this and thanks for clarification. > > Your proposal is perfectly fine. :) Done. https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newc... new-options.js:1232: setAcceptableAds("default"); On 2017/08/17 11:21:18, saroyanm wrote: > On 2017/08/16 17:57:10, Thomas Greiner wrote: > > We cannot determine the current state based on which filter list got added > last > > because the other one could still be present. > > > > Therefore I'd recommend checking which of the two filters lists are installed > > and enabled each time any of them gets added, removed, enabled or disabled > (e.g. > > `if (isAcceptable(subscription.url) refreshAcceptableAds()`). > > > > My suggestion for the states: > > - aa:false + aa-privacy:false = all ads blocked > > - aa:true + aa-privacy:false = all Acceptable Ads unblocked > > - aa:false + aa-privacy:true = only privacy-friendly Acceptable Ads unblocked > > - aa:true + aa-privacy:true = all Acceptable Ads unblocked > > > > Because since the privacy-friendly list is only a subset of the regular one, > > having both enabled at the same time is equivalent to only having the regular > > one enabled. > > I agree. I'll update accordingly. Done. https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newc... new-options.js:1260: collections.custom.removeItem(subscription); On 2017/08/16 17:57:09, Thomas Greiner wrote: > Detail: Recommended subscriptions should not be in the "More Filters" section > anyway. Done. https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newc... new-options.js:1261: delete subscriptionsMap[subscription.url]; On 2017/08/17 12:06:06, Thomas Greiner wrote: > On 2017/08/17 11:21:17, saroyanm wrote: > > On 2017/08/16 17:57:10, Thomas Greiner wrote: > > > In `addSubscription()` we're adding Acceptable Ads subscriptions to > > > `subscriptionsMap` so why aren't we removing them here? > > > > I tried to be consistent with old implementation here I think... > > But I do agree that we should remove it, I'll check if that will couse any > sort > > of unexpected behavior, but I think we should be safe here, also same should > > apply to some recommended subscriptions that are not "ads" and "protection" > > type. > > Sounds good. With current implementation is more understandable,actually we shouldn't delete "knownSubscription"(subscriptionsMap) for recommended subscriptions, otherwise they will stop being "recommended" and this will lead to being added into the "more subscriptions" section and lead to unexpected behaviors. On another hand we check URL for Acceptable Ads and we can remove it from "knownSubscription" and that what I did. https://codereview.adblockplus.org/29478597/diff/29518637/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29478597/diff/29518637/locale/en-US/new-op... locale/en-US/new-options.json:133: "options_more_malware_tooltip": { While we removed all non Privacy and Security related recommendations, I'm having problem displaying the tooltip according to the types. I'm not sure what is the best way to show the tooltip, should we use subscription list URL ? see -> https://bitbucket.org/adblockplus/spec/src/faa220d44342001ec61904f30c97b1e9a1...
Unfortunately, I didn't have time yet to review your latest patch set but I quickly wanted to give you feedback on the issue you brought up. https://codereview.adblockplus.org/29478597/diff/29518637/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29478597/diff/29518637/locale/en-US/new-op... locale/en-US/new-options.json:133: "options_more_malware_tooltip": { On 2017/08/17 21:24:08, saroyanm wrote: > While we removed all non Privacy and Security related recommendations, I'm > having problem displaying the tooltip according to the types. > > I'm not sure what is the best way to show the tooltip, should we use > subscription list URL ? > > see -> > https://bitbucket.org/adblockplus/spec/src/faa220d44342001ec61904f30c97b1e9a1... Good point. Where can users even add those filter lists? Because if there's no UI in the options page that allows them to easily add one of them, there might not be a reason to add a tooltip for them.
https://codereview.adblockplus.org/29478597/diff/29518637/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29478597/diff/29518637/locale/en-US/new-op... locale/en-US/new-options.json:133: "options_more_malware_tooltip": { On 2017/08/18 16:27:14, Thomas Greiner wrote: > On 2017/08/17 21:24:08, saroyanm wrote: > > While we removed all non Privacy and Security related recommendations, I'm > > having problem displaying the tooltip according to the types. > > > > I'm not sure what is the best way to show the tooltip, should we use > > subscription list URL ? > > > > see -> > > > https://bitbucket.org/adblockplus/spec/src/faa220d44342001ec61904f30c97b1e9a1... > > Good point. Where can users even add those filter lists? Because if there's no > UI in the options page that allows them to easily add one of them, there might > not be a reason to add a tooltip for them. I agree in general, but for the Adblock Warning list we do have UI ("Extension native notification"). Anyway I think for the time being is better to remove them and we can discuss that during next meeting.
https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newc... new-options.js:1261: delete subscriptionsMap[subscription.url]; On 2017/08/17 21:24:07, saroyanm wrote: > On 2017/08/17 12:06:06, Thomas Greiner wrote: > > On 2017/08/17 11:21:17, saroyanm wrote: > > > On 2017/08/16 17:57:10, Thomas Greiner wrote: > > > > In `addSubscription()` we're adding Acceptable Ads subscriptions to > > > > `subscriptionsMap` so why aren't we removing them here? > > > > > > I tried to be consistent with old implementation here I think... > > > But I do agree that we should remove it, I'll check if that will couse any > > sort > > > of unexpected behavior, but I think we should be safe here, also same should > > > apply to some recommended subscriptions that are not "ads" and "protection" > > > type. > > > > Sounds good. > > With current implementation is more understandable,actually we shouldn't delete > "knownSubscription"(subscriptionsMap) for recommended subscriptions, otherwise > they will stop being "recommended" and this will lead to being added into the > "more subscriptions" section and lead to unexpected behaviors. On another hand > we check URL for Acceptable Ads and we can remove it from "knownSubscription" > and that what I did. I don't think not removing recommended subscriptions is a good idea because we'll mark subscriptions as disabled even though they aren't which may cause inconsistencies in how they're displayed and how they can be interacted with as well as errors when trying to change the state of a subscription (e.g. trying to enable a subscription that is not even installed). Nevertheless, as you pointed out, we need to know whether a subscription is recommended and which type of subscription it is. We don't need to keep subscriptions in `subscriptionsMap` for that though but store that information separately. https://codereview.adblockplus.org/29478597/diff/29518637/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29478597/diff/29518637/locale/en-US/new-op... locale/en-US/new-options.json:133: "options_more_malware_tooltip": { On 2017/08/18 16:44:03, saroyanm wrote: > On 2017/08/18 16:27:14, Thomas Greiner wrote: > > On 2017/08/17 21:24:08, saroyanm wrote: > > > While we removed all non Privacy and Security related recommendations, I'm > > > having problem displaying the tooltip according to the types. > > > > > > I'm not sure what is the best way to show the tooltip, should we use > > > subscription list URL ? > > > > > > see -> > > > > > > https://bitbucket.org/adblockplus/spec/src/faa220d44342001ec61904f30c97b1e9a1... > > > > Good point. Where can users even add those filter lists? Because if there's no > > UI in the options page that allows them to easily add one of them, there might > > not be a reason to add a tooltip for them. > > I agree in general, but for the Adblock Warning list we do have UI ("Extension > native notification"). > Anyway I think for the time being is better to remove them and we can discuss > that during next meeting. Acknowledged. https://codereview.adblockplus.org/29478597/diff/29518637/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29478597/diff/29518637/new-options.html#ne... new-options.html:114: <p class="i18n_options_acceptable_ads_no_privacy_description"></p> You're using the "no_privacy" description for the "privacy" option whereas above you use the "privacy" description for the "ads" option. That's why I'd generally avoid negation in IDs because it's easy to miss that you mean the opposite. So I'd suggest rusing the same values we introduced (i.e. "ads", "privacy" and "none") for string IDs as well to avoid this confusion. For that it might also be useful to replace "acceptable_ads" to "acceptableAds" to avoid confusing ID parts such as "acceptable_ads_ads". e.g. options_acceptable_ads_no_privacy_* -> options_acceptableAds_ads_* options_acceptable_ads_privacy_* options_acceptable_ads_disable_* -> options_acceptableAds_none_* https://codereview.adblockplus.org/29478597/diff/29518637/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29478597/diff/29518637/new-options.js#newc... new-options.js:1000: function setDntNotification(state) Let's move this into `setAcceptableAds()` as well to keep everything related to changing the state of Acceptable Ads in one place. https://codereview.adblockplus.org/29478597/diff/29518637/new-options.js#newc... new-options.js:1023: } No need to loop through `subscriptionsMap` or tick `[value="none"]` by default. Instead you could write it like this: let option = "none"; if (acceptableAdsUrl in subscriptionsMap) { option = "ads"; } else if (acceptableAdsPrivacyUrl in subscriptionsMap) { option = "privacy"; } document.forms["acceptable-ads"]["acceptable-ads"].value = option; https://codereview.adblockplus.org/29478597/diff/29518637/new-options.js#newc... new-options.js:1225: delete subscriptionsMap[subscription.url]; Detail: This happens whenever a non-recommended subscription gets removed so rather than writing if (...) else if (...) delete else delete you could avoid this duplication by changing the structure to if (...) else if (...) else delete
Hope it's closer to be ready, or in the best scenario ready. https://codereview.adblockplus.org/29478597/diff/29518637/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29478597/diff/29518637/locale/en-US/new-op... locale/en-US/new-options.json:133: "options_more_malware_tooltip": { On 2017/08/22 16:08:57, Thomas Greiner wrote: > On 2017/08/18 16:44:03, saroyanm wrote: > > On 2017/08/18 16:27:14, Thomas Greiner wrote: > > > On 2017/08/17 21:24:08, saroyanm wrote: > > > > While we removed all non Privacy and Security related recommendations, I'm > > > > having problem displaying the tooltip according to the types. > > > > > > > > I'm not sure what is the best way to show the tooltip, should we use > > > > subscription list URL ? > > > > > > > > see -> > > > > > > > > > > https://bitbucket.org/adblockplus/spec/src/faa220d44342001ec61904f30c97b1e9a1... > > > > > > Good point. Where can users even add those filter lists? Because if there's > no > > > UI in the options page that allows them to easily add one of them, there > might > > > not be a reason to add a tooltip for them. > > > > I agree in general, but for the Adblock Warning list we do have UI ("Extension > > native notification"). > > Anyway I think for the time being is better to remove them and we can discuss > > that during next meeting. > > Acknowledged. Done. https://codereview.adblockplus.org/29478597/diff/29518637/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29478597/diff/29518637/new-options.html#ne... new-options.html:114: <p class="i18n_options_acceptable_ads_no_privacy_description"></p> On 2017/08/22 16:08:57, Thomas Greiner wrote: > You're using the "no_privacy" description for the "privacy" option whereas above > you use the "privacy" description for the "ads" option. > > That's why I'd generally avoid negation in IDs because it's easy to miss that > you mean the opposite. So I'd suggest rusing the same values we introduced (i.e. > "ads", "privacy" and "none") for string IDs as well to avoid this confusion. For > that it might also be useful to replace "acceptable_ads" to "acceptableAds" to > avoid confusing ID parts such as "acceptable_ads_ads". > > e.g. > options_acceptable_ads_no_privacy_* -> options_acceptableAds_ads_* > options_acceptable_ads_privacy_* > options_acceptable_ads_disable_* -> options_acceptableAds_none_* Done. https://codereview.adblockplus.org/29478597/diff/29518637/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29478597/diff/29518637/new-options.js#newc... new-options.js:1000: function setDntNotification(state) On 2017/08/22 16:08:57, Thomas Greiner wrote: > Let's move this into `setAcceptableAds()` as well to keep everything related to > changing the state of Acceptable Ads in one place. Done. https://codereview.adblockplus.org/29478597/diff/29518637/new-options.js#newc... new-options.js:1023: } On 2017/08/22 16:08:58, Thomas Greiner wrote: > No need to loop through `subscriptionsMap` or tick `[value="none"]` by default. > Instead you could write it like this: > > let option = "none"; > if (acceptableAdsUrl in subscriptionsMap) > { > option = "ads"; > } > else if (acceptableAdsPrivacyUrl in subscriptionsMap) > { > option = "privacy"; > } > document.forms["acceptable-ads"]["acceptable-ads"].value = option; Done. https://codereview.adblockplus.org/29478597/diff/29518637/new-options.js#newc... new-options.js:1225: delete subscriptionsMap[subscription.url]; On 2017/08/22 16:08:58, Thomas Greiner wrote: > Detail: This happens whenever a non-recommended subscription gets removed so > rather than writing > > if (...) > else if (...) > delete > else > delete > > you could avoid this duplication by changing the structure to > > if (...) > else > if (...) > else > delete Done.
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
