issue 6075 - Hide Acceptable Ads notification when corresponding subscription is removed

desktop-options.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present 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
@@ skipping 1083 matching lines @@
       acceptableAdsPrivacy.setAttribute("aria-disabled", true);
       acceptableAdsPrivacy.setAttribute("tabindex", -1);
     }
   }
 
   function isAcceptableAds(url)
   {
     return url == acceptableAdsUrl || url == acceptableAdsPrivacyUrl;
   }
 
-  function hasPrivacyConflict()
+  function setPrivacyConflict()
   {
     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;
+    E("acceptable-ads").classList.remove("show-warning");

[Thomas Greiner, 2017/11/23 14:46:12]

Detail: We can avoid this unnecessary call by moving it into the "else" case and
using `Element.classList.toggle()` instead of `Element.classList.add()`.

[saroyanm, 2017/11/23 16:41:38]

I agree, well spotted, done.

+    if (acceptableAdsList && acceptableAdsList.disabled == false &&
+      privacyList && privacyList.disabled == false)
+    {
+      getPref("ui_warn_tracking", (showTrackingWarning) =>
+      {
+        if (showTrackingWarning)
+          E("acceptable-ads").classList.add("show-warning");
+      });
+    }
   }
 
   function populateLists()
   {
     subscriptionsMap = Object.create(null);
     filtersMap = Object.create(null);
 
     // Empty collections and lists
     for (let property in collections)
       collections[property].clearAll();
@@ skipping 130 matching lines @@
         if (property == "title" && knownSubscription.recommended)
           knownSubscription.originalTitle = subscription.title;
         else
           knownSubscription[property] = subscription[property];
       }
       subscription = knownSubscription;
     }
     switch (action)
     {
       case "disabled":
+        setPrivacyConflict();

[Thomas Greiner, 2017/11/23 14:46:12]

Detail: Let's do this after we've updated the UI. Because in case an error
occurs in either we probably care more about the UI being up-to-date rather than
the warning being shown.

Same applies to the other places where we call `setPrivacyConflict()`.

[saroyanm, 2017/11/23 16:41:38]

DOne.

         updateSubscription(subscription);
         break;
       case "downloading":
       case "downloadStatus":
       case "homepage":
       case "lastDownload":
       case "title":
         updateSubscription(subscription);
         break;
       case "added":
         let {url, recommended} = subscription;
         // Handle custom subscription
         if (/^~user/.test(url))
         {
           loadCustomFilters(subscription.filters);
           return;
         }
         else if (url in subscriptionsMap)
           updateSubscription(subscription);
         else
           addSubscription(subscription);
 
         if (isAcceptableAds(url))
           setAcceptableAds();
 
-        if ((url == acceptableAdsUrl || recommended == "privacy") &&
-          hasPrivacyConflict())
-        {
-          getPref("ui_warn_tracking", (showTrackingWarning) =>
-          {
-            if (showTrackingWarning)
-              E("acceptable-ads").classList.add("show-warning");
-          });
-        }
-
+        setPrivacyConflict();
         collections.filterLists.addItem(subscription);
         break;
       case "removed":
         if (subscription.recommended)
         {
           subscription.disabled = true;
           onSubscriptionMessage("disabled", subscription);
         }
         else
         {
           delete subscriptionsMap[subscription.url];
           if (isAcceptableAds(subscription.url))
           {
             setAcceptableAds();
           }
           else
           {
             collections.more.removeItem(subscription);
           }
         }
+
+        setPrivacyConflict();
         collections.filterLists.removeItem(subscription);
         break;
     }
   }
 
   function hidePref(key, value)
   {
     let element = document.querySelector("[data-pref='" + key + "']");
     if (element)
       element.setAttribute("aria-hidden", value);
@@ skipping 35 matching lines @@
     switch (key)
     {
       case "notifications_ignoredcategories":
         value = value.indexOf("*") == -1;
         break;
 
       case "notifications_showui":
         hidePref("notifications_ignoredcategories", !value);
         break;
       case "ui_warn_tracking":
-        let showWarning = (value && hasPrivacyConflict());
-        E("acceptable-ads").classList.toggle("show-warning", showWarning);
+        setPrivacyConflict();
         break;
     }
 
     let checkbox = document.querySelector(
       "[data-pref='" + key + "'] button[role='checkbox']"
     );
     if (checkbox)
       checkbox.setAttribute("aria-checked", value);
   }
 
@@ skipping 67 matching lines @@
   });
   browser.runtime.sendMessage({
     type: "subscriptions.listen",
     filter: ["added", "disabled", "homepage", "lastDownload", "removed",
              "title", "downloadStatus", "downloading"]
   });
 
   window.addEventListener("DOMContentLoaded", onDOMLoaded, false);
   window.addEventListener("hashchange", onHashChange, false);
 }