Issue 5539 - Implement "Acceptable Ads notification"

new-options.js

Index: new-options.js
===================================================================
--- a/new-options.js
+++ b/new-options.js
@@ -372,7 +372,7 @@
 
   function addSubscription(subscription)
   {
-    let disabled = subscription.disabled;
+    let {disabled} = subscription;

[saroyanm, 2017/09/21 15:50:40]

Note: This is part of eslint error fix. I fixed couple of errors below as well.

     let collection = null;
     if (subscription.recommended)
     {
@@ -627,7 +627,7 @@
         });
         break;
       case "switch-acceptable-ads":
-        let {value} = element;
+        let value = element.value || element.dataset.value;
         ext.backgroundPage.sendMessage({
           type: value == "privacy" ? "subscriptions.add" :
             "subscriptions.remove",
@@ -840,7 +840,6 @@
       E("whitelisting-add-button").disabled = !e.target.value;
     }, false);
 
-
     getDocLink("contribute", (link) =>
     {
       E("contribute").href = link;
@@ -1009,6 +1008,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);
@@ -1172,14 +1188,24 @@
         updateSubscription(subscription);
         break;
       case "added":
-        if (subscription.url in subscriptionsMap)
+        let {url} = subscription;
+        if (url in subscriptionsMap)
           updateSubscription(subscription);
         else
           addSubscription(subscription);
 
-        if (isAcceptableAds(subscription.url))
+        if (isAcceptableAds(url))
           setAcceptableAds();
 
+        if (url == acceptableAdsUrl || subscription.recommended == "privacy")
+        {
+          getPref("ui_warn_tracking", (showTrackingWarning) =>

[saroyanm, 2017/09/21 15:50:40]

I'm not sure if using promise is better in this case, please let me know if you
disagree, if I would use it here, it will be the only place that we are using
it.

As soon we will use this variable in multiple places I think in that case it
would make sense to use promises to make code more readable, but while we just
need it here, this code looks more compact in comparison to:

new Promise((resolve) =>
{
  getPref("ui_warn_tracking", resolve);
}).then((value) =>
{
  if (hasPrivacyConflict() && value)
    openDialog("tracking");
});

[Thomas Greiner, 2017/09/22 10:05:43]

Using a promise you could've avoided making that request to the background page
each time but this shouldn't happen too often anyway so this approach is also
totally fine with me since it keeps things simple.

It'd be great, however, if you could check for `hasPrivacyConflict()` before
retrieving the preference because we don't need to check for
`showTrackingWarning` if there's no privacy conflict.

[saroyanm, 2017/09/22 10:33:23]

I agree, done.

+          {
+            if (hasPrivacyConflict() && showTrackingWarning)
+              openDialog("tracking");
+          });
+        }
+
         collections.filterLists.addItem(subscription);
         break;
       case "removed":
@@ -1203,7 +1229,6 @@
         collections.filterLists.removeItem(subscription);
         break;
     }
-
   }
 
   function hidePref(key, value)
@@ -1329,7 +1354,8 @@
   ext.backgroundPage.sendMessage({
     type: "prefs.listen",
     filter: ["notifications_ignoredcategories", "notifications_showui",
-             "show_devtools_panel", "shouldShowBlockElementMenu"]
+             "show_devtools_panel", "shouldShowBlockElementMenu",
+             "ui_warn_tracking"]

[Thomas Greiner, 2017/09/22 10:05:43]

Detail: We're no longer reacting to changes for the "ui_warn_tracking"
preference so no need to listen to it anymore.

   });
   ext.backgroundPage.sendMessage({
     type: "subscriptions.listen",