Issue 5326 - General tab (HTML, strings and functionality)

new-options.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
- * Copyright (C) 2006-2017 eyeo GmbH
+ * 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
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
@@ skipping 120 matching lines @@
       listItem.setAttribute("role", "section");
 
       let tooltip = listItem.querySelector("[data-tooltip]");
       if (tooltip)
       {
         let tooltipId = tooltip.getAttribute("data-tooltip");
         tooltipId = tooltipId.replace("%value%", item.recommended);
         if (getMessage(tooltipId))
         {
           tooltip.setAttribute("data-tooltip", tooltipId);
-        }
-        else
-        {
-          tooltip.parentNode.removeChild(tooltip);
         }
       }
 
       for (let control of listItem.querySelectorAll(".control"))
       {
         if (control.hasAttribute("title"))
         {
           let titleValue = getMessage(control.getAttribute("title"));
           control.setAttribute("title", titleValue);
         }
@@ skipping 176 matching lines @@
     let nextElement = focusables[index];
     if (!nextElement)
       return false;
 
     nextElement.focus();
     return true;
   }
 
   collections.protection = new Collection([
     {
-      id: "recommend-security-list-table"
+      id: "recommend-protection-list-table"
     }
   ]);
   collections.langs = new Collection([
     {
       id: "blocking-languages-table",
       emptyText: ["options_language_empty"]
     }
   ]);
   collections.allLangs = new Collection([
     {
@@ skipping 276 matching lines @@
           type: "filters.importRaw",
           text: E("custom-filters-raw").value,
           removeExisting: true
         },
         () =>
         {
           setCustomFiltersView("read");
         });
         break;
       case "switch-acceptable-ads":
-        let value = element.value;
+        let {value} = element;
         ext.backgroundPage.sendMessage({
           type: value == "privacy" ? "subscriptions.add" :
             "subscriptions.remove",
           url: acceptableAdsPrivacyUrl
         });
         ext.backgroundPage.sendMessage({
           type: value == "ads" ? "subscriptions.add" : "subscriptions.remove",
           url: acceptableAdsUrl
         });
         break;
@@ skipping 338 matching lines @@
 
   function closeDialog()
   {
     let dialog = E("dialog");
     dialog.setAttribute("aria-hidden", true);
     dialog.removeAttribute("aria-labelledby");
     document.body.removeAttribute("data-dialog");
     focusedBeforeDialog.focus();
   }
 
-  function setDntNotification(state)

[Thomas Greiner, 2017/08/22 16:08:57]

Let's move this into `setAcceptableAds()` as well to keep everything related to
changing the state of Acceptable Ads in one place.

[saroyanm, 2017/08/22 17:00:07]

Done.

-  {
-    if (state)
-      E("acceptable-ads").classList.add("show-dnt-notification");
-    else
-      E("acceptable-ads").classList.remove("show-dnt-notification");
-  }
-
   function setAcceptableAds()
   {
-    let optionsContainer = E("acceptable-ads");
-    optionsContainer.querySelector("[value='none']").checked = true;
-    for (let url in subscriptionsMap)
-    {
-      if (url == acceptableAdsPrivacyUrl)
-      {
-        optionsContainer.querySelector("[value='privacy']").checked = true;
-      }
-      if (url == acceptableAdsUrl)
-      {
-        optionsContainer.querySelector("[value='ads']").checked = true;
-        break;
-      }
-    }

[Thomas Greiner, 2017/08/22 16:08:58]

No need to loop through `subscriptionsMap` or tick `[value="none"]` by default.
Instead you could write it like this:

let option = "none";
if (acceptableAdsUrl in subscriptionsMap)
{
  option = "ads";
}
else if (acceptableAdsPrivacyUrl in subscriptionsMap)
{
  option = "privacy";
}
document.forms["acceptable-ads"]["acceptable-ads"].value = option;

[saroyanm, 2017/08/22 17:00:09]

Done.

+    let option = "none";
+    document.forms["acceptable-ads"].classList.remove("show-dnt-notification");
+    if (acceptableAdsUrl in subscriptionsMap)
+    {
+      option = "ads";
+    }
+    else if (acceptableAdsPrivacyUrl in subscriptionsMap)
+    {
+      option = "privacy";
+
+      if (!navigator.doNotTrack)
+        document.forms["acceptable-ads"].classList.add("show-dnt-notification");
+    }
+    document.forms["acceptable-ads"]["acceptable-ads"].value = option;
   }
 
   function isAcceptableAds(url)
   {
     return url == acceptableAdsUrl || url == acceptableAdsPrivacyUrl;
   }
 
   function populateLists()
   {
     subscriptionsMap = Object.create(null);
@@ skipping 160 matching lines @@
         break;
       case "added":
         if (subscription.url in subscriptionsMap)
           updateSubscription(subscription);
         else
           addSubscription(subscription);
 
         if (isAcceptableAds(subscription.url))
           setAcceptableAds();
 
-        if (subscription.url == acceptableAdsPrivacyUrl)
-        {
-          if (!navigator.doNotTrack)
-            setDntNotification(true);
-        }
-
         collections.filterLists.addItem(subscription);
         break;
       case "removed":
         if (subscription.recommended)
         {
           subscription.disabled = true;
           onSubscriptionMessage("disabled", subscription);
         }
-        else if (isAcceptableAds(subscription.url))
-        {
-          if (subscription.url == acceptableAdsPrivacyUrl)
+        else
+        {
+          delete subscriptionsMap[subscription.url];
+          if (isAcceptableAds(subscription.url))
           {
-            setDntNotification(false);
+            setAcceptableAds();
           }
-
-          delete subscriptionsMap[subscription.url];

[Thomas Greiner, 2017/08/22 16:08:58]

Detail: This happens whenever a non-recommended subscription gets removed so
rather than writing

if (...)
else if (...)
  delete
else
  delete

you could avoid this duplication by changing the structure to

if (...)
else
  if (...)
  else
  delete

[saroyanm, 2017/08/22 17:00:09]

Done.

-          setAcceptableAds();
-        }
-        else
-        {
-          collections.custom.removeItem(subscription);
-          delete subscriptionsMap[subscription.url];
+          else
+          {
+            collections.custom.removeItem(subscription);
+          }
         }
         collections.filterLists.removeItem(subscription);
         break;
     }
 
     updateShareLink();
   }
 
   function hidePref(key, value)
   {
@@ skipping 146 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);
 }