Issue 6031 - Implement Acceptable Ads notification

desktop-options.js

Index: desktop-options.js
===================================================================
--- a/desktop-options.js
+++ b/desktop-options.js
@@ -28,6 +28,7 @@
   let acceptableAdsPrivacyUrl = null;
   let isCustomFiltersLoaded = false;
   let {getMessage} = browser.i18n;
+  let {setElementText} = ext.i18n;

[saroyanm, 2017/11/17 16:10:39]

Didin't we convert all ext. to browser. ?
I'm not sure if this is a correct use.

[Thomas Greiner, 2017/11/20 18:36:05]

No, we merely got rid of the abstraction layer for Web Extension APIs. This
method, however, is not part of that.

Alternatively, you could call it as `ext.i18n.setElementText()` if you don't
like doing this import.

[saroyanm, 2017/11/21 14:58:32]

I like, import here, as it's somehow consistent with other "{getMessage}"
import.

   let customFilters = [];
   let filterErrors = new Map([
     ["synchronize_invalid_url",
@@ -907,6 +908,12 @@
     {
       setLinks("enable-acceptable-ads-description", link);
     });
+    setElementText(E("tracking-warning-1"), "options_tracking_warning_1", 

[saroyanm, 2017/11/17 16:10:39]

Ideally I think we should be able to specify "translatable" arguments somehow in
the HTML data ?

Maybe <p data-i18n="options_tracking_warning_1"
data-args="common_feature_privacy_title,options_acceptableAds_ads_label"></p>

If you agree, I'll create another ticket to handle that, or can handle in this
ticket as well, depending how urgent are this changes.

[Thomas Greiner, 2017/11/20 18:36:05]

Let's wait for further cases where we'd need that. Because at this point it
wouldn't add any value since this is the only part in the code where we use it.

[saroyanm, 2017/11/21 14:58:32]

Agree.

+      [getMessage("common_feature_privacy_title"),
+      getMessage("options_acceptableAds_ads_label")]);
+    setElementText(E("tracking-warning-3"), "options_tracking_warning_3", 
+      [getMessage("options_acceptableAds_privacy_label")]);
+
     getDocLink("privacy_friendly_ads", (link) =>
     {
       E("enable-acceptable-ads-privacy-description").href = link;
@@ -1094,6 +1101,23 @@
     return url == acceptableAdsUrl || url == acceptableAdsPrivacyUrl;
   }
 
+  function hasPrivacyConflict()
+  {
+    let acceptableAdsList = subscriptionsMap[acceptableAdsUrl];
+    let privacyList = null;
+    for (let url in subscriptionsMap)
+    {
+      let subscription = subscriptionsMap[url];
+      if (subscription.recommended == "privacy")
+      {
+        privacyList = subscription;
+        break;
+      }
+    }
+    return acceptableAdsList && acceptableAdsList.disabled == false &&
+      privacyList && privacyList.disabled == false;
+  }
+
   function populateLists()
   {
     subscriptionsMap = Object.create(null);
@@ -1267,6 +1291,16 @@
         if (isAcceptableAds(url))
           setAcceptableAds();
 
+        if ((url == acceptableAdsUrl || recommended == "privacy") &&
+          hasPrivacyConflict())
+        {
+          getPref("ui_warn_tracking", (showTrackingWarning) =>
+          {
+            if (showTrackingWarning)
+              E("acceptable-ads").classList.add("show-warning");
+          });
+        }
+
         collections.filterLists.addItem(subscription);
         break;
       case "removed":
@@ -1341,6 +1375,10 @@
       case "notifications_showui":
         hidePref("notifications_ignoredcategories", !value);
         break;
+      case "ui_warn_tracking":
+        if (!value)

[saroyanm, 2017/11/17 16:45:14]

Does it make sense to check against value == false ?
In case the preferences gets corrupted, or doesn't load ?

[Thomas Greiner, 2017/11/20 18:36:05]

I'd assume not. If we cannot load the setting, we should get its default value
as defined in lib/prefs.js.

It would help in case we accidentally set the preference value to `undefined`,
`null` or `""` but then we'd need to use `value === false` since `"" == false`
and `[] == false` both evaluate to `true`.

[saroyanm, 2017/11/21 14:58:32]

I see an issue with strict equation(not sure if that might be case), when the
Browser passes non Boolean value (ex. String) ?

I think we can leave it as it is right now.

[Thomas Greiner, 2017/11/21 17:17:29]

I don't understand what you mean but I'm fine with relying on truthy and falsy
values for now since we do the same for "notifications_showui" above.

+          E("acceptable-ads").classList.remove("show-warning");

[Thomas Greiner, 2017/11/20 18:36:05]

What if the value changes to `true`? We'd probably want to reuse
`hasPrivacyConflict()` here again in that case.

While this is currently not possible via the UI, it could be useful for testing
and it could help avoid issues with this in the future.

[saroyanm, 2017/11/21 14:58:32]

Agree. Done.

+        break;
     }
 
     let checkbox = document.querySelector(
@@ -1415,7 +1453,8 @@
   browser.runtime.sendMessage({
     type: "prefs.listen",
     filter: ["notifications_ignoredcategories", "notifications_showui",
-             "show_devtools_panel", "shouldShowBlockElementMenu"]
+             "show_devtools_panel", "shouldShowBlockElementMenu",
+             "ui_warn_tracking"]
   });
   browser.runtime.sendMessage({
     type: "subscriptions.listen",