Issue 5539 - Implement "Acceptable Ads notification"

new-options.js

Index: new-options.js
===================================================================
--- a/new-options.js
+++ b/new-options.js
@@ -27,6 +27,7 @@
   let acceptableAdsUrl = null;
   let acceptableAdsPrivacyUrl = null;
   let isCustomFiltersLoaded = false;
+  let isSubscriptionsLoaded = false;
   let {getMessage} = ext.i18n;
   let customFilters = [];
   let filterErrors = new Map([
@@ -1028,6 +1029,18 @@
     return url == acceptableAdsUrl || url == acceptableAdsPrivacyUrl;
   }
 
+  function isPrivacyAndAdsEnabled()

[Thomas Greiner, 2017/08/25 18:44:52]

This function is a bit difficult to understand so a few recommendations from me
would be:
- Clarify intent of function in its name
- Group similar tasks within the function
- Clarify return statement

e.g.

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 &&
    privacyList && !privacyList.disabled;
}

[saroyanm, 2017/08/27 16:25:28]

Agree, thanks.

[saroyanm, 2017/09/14 21:56:14]

Done.

+  {
+    let isAcceptableAds = subscriptionsMap[acceptableAdsUrl];
+    isAcceptableAds = isAcceptableAds && isAcceptableAds.disabled == false;
+    for (let url in subscriptionsMap)
+    {
+      let subscription = subscriptionsMap[url];
+      if (subscription.recommended == "privacy")
+        return subscription.disabled == false && isAcceptableAds;
+    }
+  }
+
   function populateLists()
   {
     subscriptionsMap = Object.create(null);
@@ -1090,6 +1103,7 @@
         {
           for (let subscription of subscriptions)
             onSubscriptionMessage("added", subscription);
+          isSubscriptionsLoaded = true;
         });
       });
     });
@@ -1193,20 +1207,27 @@
         updateSubscription(subscription);
         break;
       case "added":
-        if (subscription.url in subscriptionsMap)
+        let url = subscription.url;
+        if (url in subscriptionsMap)
           updateSubscription(subscription);
         else
           addSubscription(subscription);
 
-        if (isAcceptableAds(subscription.url))
+        if (isAcceptableAds(url))
           setAcceptableAds();
 
-        if (subscription.url == acceptableAdsPrivacyUrl)
+        if (url == acceptableAdsPrivacyUrl)
         {
           if (!navigator.doNotTrack)
             setDntNotification(true);
         }
 
+        if (url == acceptableAdsUrl || subscription.recommended == "privacy")
+        {
+          if (isSubscriptionsLoaded && isPrivacyAndAdsEnabled())

[Thomas Greiner, 2017/08/25 18:44:51]

Why is checking for `isSubscriptionsLoaded` necessary here? We're not doing it
for any of the other checks either.

[saroyanm, 2017/08/27 16:25:28]

I did it in order not to show on each load of the page as we don't have "pref"
setting yet, if I would omit this the dialog will keep showing.

[Thomas Greiner, 2017/08/29 10:20:41]

So it's a stopgap solution until we have the preference? What about introducing
the preference then if it's necessary for this feature to work?

[saroyanm, 2017/09/14 21:56:14]

Done.

+            openDialog("tracking");

[saroyanm, 2017/08/19 00:00:40]

In specs it says that "This is a one time message".

Should we use Prefs for that, to store the value ?

[Thomas Greiner, 2017/08/25 18:44:51]

Yes. Probably best to prefix the preference name with "ui" to avoid naming
collisions with existing or future preferences in adblockplus, adblockpluschrome
or other users of adblockplusui.

e.g. "ui_warn_tracking"

[saroyanm, 2017/08/27 16:25:28]

Thanks, I'll have a look into this.

[saroyanm, 2017/09/14 21:56:14]

Done.

+        }
+
         collections.filterLists.addItem(subscription);
         break;
       case "removed":