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 setPrivacyConflict()
+  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;
       }
     }
-    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)
+    return acceptableAdsList && acceptableAdsList.disabled == false &&
+      privacyList && privacyList.disabled == false;
+  }
+
+  function setPrivacyConflict()
+  {
+    let acceptableAdsForm = E("acceptable-ads");
+    if (hasPrivacyConflict())
     {
       getPref("ui_warn_tracking", (showTrackingWarning) =>
       {
-        if (showTrackingWarning)
-          E("acceptable-ads").classList.add("show-warning");
+        acceptableAdsForm.classList.toggle("show-warning", showTrackingWarning);
       });
+    }
+    else
+    {
+      acceptableAdsForm.classList.remove("show-warning");
     }
   }
 
   function populateLists()
   {
     subscriptionsMap = Object.create(null);
     filtersMap = Object.create(null);
 
     // Empty collections and lists
     for (let property in collections)
@@ skipping 131 matching lines @@
         if (property == "title" && knownSubscription.recommended)
           knownSubscription.originalTitle = subscription.title;
         else
           knownSubscription[property] = subscription[property];
       }
       subscription = knownSubscription;
     }
     switch (action)
     {
       case "disabled":
+        updateSubscription(subscription);
         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();
 
+        collections.filterLists.addItem(subscription);
         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);
           }
         }
 
+        collections.filterLists.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 122 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);
 }