Issue 5539 - Implement "Acceptable Ads notification"

new-options.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2017 eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 /* globals checkShareResource, getDocLink, i18nFormatDateTime, openSharePopup,
            setLinks, E */
 
 "use strict";
 
 {
   let subscriptionsMap = Object.create(null);
   let filtersMap = Object.create(null);
   let collections = Object.create(null);
   let acceptableAdsUrl = null;
   let acceptableAdsPrivacyUrl = null;
   let isCustomFiltersLoaded = false;
+  let isSubscriptionsLoaded = false;
   let {getMessage} = ext.i18n;
   let customFilters = [];
   let filterErrors = new Map([
     ["synchronize_invalid_url",
      "options_filterList_lastDownload_invalidURL"],
     ["synchronize_connection_error",
      "options_filterList_lastDownload_connectionError"],
     ["synchronize_invalid_data",
      "options_filterList_lastDownload_invalidData"],
     ["synchronize_checksum_mismatch",
@@ skipping 981 matching lines @@
         break;
       }
     }
   }
 
   function isAcceptableAds(url)
   {
     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);
     filtersMap = Object.create(null);
 
     // Empty collections and lists
     for (let property in collections)
       collections[property].clearAll();
 
     ext.backgroundPage.sendMessage({
@@ skipping 42 matching lines @@
 
         // Load user subscriptions
         ext.backgroundPage.sendMessage({
           type: "subscriptions.get",
           downloadable: true
         },
         (subscriptions) =>
         {
           for (let subscription of subscriptions)
             onSubscriptionMessage("added", subscription);
+          isSubscriptionsLoaded = true;
         });
       });
     });
   }
 
   function addWhitelistedDomain()
   {
     let domain = E("whitelisting-textbox");
     for (let whitelistItem of collections.whitelist.items)
     {
@@ skipping 83 matching lines @@
         updateSubscription(subscription);
         break;
       case "downloading":
       case "downloadStatus":
       case "homepage":
       case "lastDownload":
       case "title":
         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":
         if (subscription.recommended)
         {
           subscription.disabled = true;
           onSubscriptionMessage("disabled", subscription);
         }
         else if (isAcceptableAds(subscription.url))
         {
@@ skipping 168 matching lines @@
   });
   ext.backgroundPage.sendMessage({
     type: "subscriptions.listen",
     filter: ["added", "disabled", "homepage", "lastDownload", "removed",
              "title", "downloadStatus", "downloading"]
   });
 
   window.addEventListener("DOMContentLoaded", onDOMLoaded, false);
   window.addEventListener("hashchange", onHashChange, false);
 }