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
  *
  * 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 {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"],
@@ skipping 38 matching lines @@
   {
     let access = (item.url || item.text).replace(/'/g, "\\'");
     return function(container)
     {
       return container.querySelector("[data-access='" + access + "']");
     };
   };
 
   Collection.prototype._getItemTitle = function(item, i)
   {
-    if (item.url == acceptableAdsUrl)
-      return getMessage("options_acceptableAds_description");
     if (this.details[i].useOriginalTitle && item.originalTitle)
       return item.originalTitle;
     return item.title || item.url || item.text;
   };
 
   Collection.prototype._sortItems = function()
   {
     this.items.sort((a, b) =>
     {
       // Make sure that Acceptable Ads is always last, since it cannot be
       // disabled, but only be removed. That way it's grouped together with
       // the "Own filter list" which cannot be disabled either at the bottom
       // of the filter lists in the Advanced tab.
-      if (a.url == acceptableAdsUrl)
+      if (isAcceptableAds(a.url))
         return 1;
-      if (b.url == acceptableAdsUrl)
+      if (isAcceptableAds(b.url))
         return -1;
 
       // Make sure that newly added entries always appear on top in descending
       // chronological order
       let aTimestamp = a[timestampUI] || 0;
       let bTimestamp = b[timestampUI] || 0;
       if (aTimestamp || bTimestamp)
         return bTimestamp - aTimestamp;
 
       let aTitle = this._getItemTitle(a, 0).toLowerCase();
@@ skipping 13 matching lines @@
     {
       let detail = this.details[j];
       let table = E(detail.id);
       let template = table.querySelector("template");
       let listItem = document.createElement("li");
       listItem.appendChild(document.importNode(template.content, true));
       listItem.setAttribute("aria-label", this._getItemTitle(item, j));
       listItem.setAttribute("data-access", item.url || item.text);
       listItem.setAttribute("role", "section");
 
-      let label = listItem.querySelector(".display");
-      if (item.recommended && label.hasAttribute("data-tooltip"))
+      let tooltip = listItem.querySelector("[data-tooltip]");
+      if (tooltip)
       {
-        let tooltipId = label.getAttribute("data-tooltip");
+        let tooltipId = tooltip.getAttribute("data-tooltip");
         tooltipId = tooltipId.replace("%value%", item.recommended);
-        label.setAttribute("data-tooltip", tooltipId);
+        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 54 matching lines @@
     this._sortItems();
     let access = (item.url || item.text).replace(/'/g, "\\'");
     for (let i = 0; i < this.details.length; i++)
     {
       let table = E(this.details[i].id);
       let element = table.querySelector("[data-access='" + access + "']");
       if (!element)
         continue;
 
       let title = this._getItemTitle(item, i);
-      element.querySelector(".display").textContent = title;
+      for (let displayElement of element.querySelectorAll(".display"))

[Thomas Greiner, 2017/08/16 17:57:09]

According to
https://developer.mozilla.org/en-US/docs/Web/API/NodeList#Browser_compatibility
`NodeList` wasn't iterable yet in Chrome 49 which is the oldest version we still
support. Therefore we shouldn't use a for-of loop here.

[saroyanm, 2017/08/17 11:21:17]

well spotted.

[saroyanm, 2017/08/17 21:24:07]

Done.

+        displayElement.textContent = title;
+
       element.setAttribute("aria-label", title);
       if (this.details[i].searchable)
         element.setAttribute("data-search", title.toLowerCase());
       let control = element.querySelector(".control[role='checkbox']");
       if (control)
       {
         control.setAttribute("aria-checked", item.disabled == false);
-        if (item.url == acceptableAdsUrl && this == collections.filterLists)
+        if (isAcceptableAds(item.url) && this == collections.filterLists)
           control.disabled = true;
       }
 
       let lastUpdateElement = element.querySelector(".last-update");
       if (lastUpdateElement)
       {
         let message = element.querySelector(".message");
         if (item.isDownloading)
         {
           let text = getMessage("options_filterList_lastDownload_inProgress");
@@ skipping 87 matching lines @@
     index += (index == focusables.length - 1) ? -1 : 1;
 
     let nextElement = focusables[index];
     if (!nextElement)
       return false;
 
     nextElement.focus();
     return true;
   }
 
-  collections.popular = new Collection([
+  collections.security = new Collection([
     {
-      id: "recommend-list-table"
+      id: "recommend-security-list-table"
     }
   ]);
   collections.langs = new Collection([
     {
       id: "blocking-languages-table",
-      emptyText: ["options_dialog_language_added_empty"]
-    },
-    {
-      id: "blocking-languages-dialog-table",
-      emptyText: ["options_dialog_language_added_empty"]
+      emptyText: ["options_language_empty"]
     }
   ]);
   collections.allLangs = new Collection([
     {
-      id: "all-lang-table",
-      emptyText: ["options_dialog_language_other_empty"],
-      searchable: true
-    }
-  ]);
-  collections.acceptableAds = new Collection([
-    {
-      id: "acceptableads-table"
+      id: "all-lang-table-add",
+      emptyText: ["options_dialog_language_other_empty"]
     }
   ]);
   collections.custom = new Collection([
     {
       id: "custom-list-table"
     }
   ]);
   collections.whitelist = new Collection([
     {
       id: "whitelisting-table",
       emptyText: ["options_whitelist_empty_1", "options_whitelist_empty_2"]
     }
   ]);
   collections.filterLists = new Collection([
     {
       id: "all-filter-lists-table",
       useOriginalTitle: true
     }
   ]);
 
-  function toggleShowLanguage(subscription)
+  function addSubscription(subscription)
   {
-    if (subscription.recommended == "ads")
+    let collection = null;
+    if (subscription.recommended)
     {
-      if (subscription.disabled)
+      if (isProtection(subscription.recommended))
       {
+        collection = collections.security;

[Thomas Greiner, 2017/08/16 17:57:10]

Detail: The previous comments for this still applies.

See
https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc...

[saroyanm, 2017/08/17 21:24:07]

Done.

+      }
+      else if (subscription.recommended == "ads")
+      {
+        if (subscription.disabled == false)
+          collection = collections.langs;
+
         collections.allLangs.addItem(subscription);
-        collections.langs.removeItem(subscription);
+      }
+      else if (subscription.disabled == false)
+      {
+        collection = collections.custom;

[Thomas Greiner, 2017/08/16 17:57:10]

So we're still putting recommended subscriptions into the "More Filters"
section?

See
https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc...

[saroyanm, 2017/08/17 11:21:19]

Yes, because according to the subscription.xml "Malware Domains" and "Adblock
Warning Removal List" are also in the recommendation, but not recommended in the
specifications. So currently we show in More Filters section the filters as
specified below:
* Recommended subscriptions that aren't "Protecting filters" and Adblocking
filters
* Subscriptions that aren't recommended and aren't "AcceptableAds"
subscriptions.

[Thomas Greiner, 2017/08/17 12:06:05]

If they are in subscriptions.xml but are not supposed to be recommended then
let's simply not include them in subscriptions.xml and adapt
https://issues.adblockplus.org/ticket/2825 accordingly.

[saroyanm, 2017/08/17 12:32:08]

Is it the decision that we can make ? 

If so I'm fine and will change the implementation accordingly, but on other hand
what about other locations where we recommend them (ex.:
https://adblockplus.org/subscriptions) ?  Isn't the idea to have 1
subscriptions.xml in the end shared across products/websites and etc ?

[Thomas Greiner, 2017/08/17 12:49:06]

We are the ones who added them in the first place so those are not being used
anywhere (see https://issues.adblockplus.org/ticket/2825). No need to worry. :)

Ensuring that introducing those new non-ads subscription types doesn't cause any
negative side effects is what https://issues.adblockplus.org/ticket/2821 is for.

Also note that the subscriptions.xml in the adblockplusui repository is only
being used for the development environment so changes to that one don't affect
anything else. The extensions bundle their own version of subscriptions.xml.
No, that's what we have https://hg.adblockplus.org/subscriptionlist for. It
provides a single source for generating subscriptions.xml, subscriptions2.xml
and adblockplus.org/subscriptions.

[saroyanm, 2017/08/17 14:06:18]

Thanks for clarifying that, in this case whole logic should be supper simple in
general:
Privacy and Security subscriptions = recommendation which are not ads type.
More subscriptions = Not a recommendation and not Acceptable Ads subscription.

Saying that I think we even can close ->
https://issues.adblockplus.org/ticket/2825 because we will need to distinguish
between Ads type and non Ads type recommendation. What you think ?

[saroyanm, 2017/08/17 21:24:05]

Done.

       }
       else

[Thomas Greiner, 2017/08/16 17:57:09]

This part of the if-statement is redundant because that's what's happening at
the end of the function anyway.

[saroyanm, 2017/08/17 11:21:18]

agree.

[saroyanm, 2017/08/17 21:24:05]

Done.

       {
-        collections.allLangs.removeItem(subscription);
-        collections.langs.addItem(subscription);
+        subscriptionsMap[subscription.url] = subscription;
+        return;
       }
     }
-  }
+    else if (!isAcceptableAds(subscription.url))
+    {
+      collection = collections.custom;
+    }
 
-  function addSubscription(subscription)
-  {
-    let collection;
-    if (subscription.recommended)
-    {
-      if (subscription.recommended != "ads")
-        collection = collections.popular;
-      else if (subscription.disabled == false)
-        collection = collections.langs;
-      else
-        collection = collections.allLangs;
-    }
-    else if (subscription.url == acceptableAdsUrl)
-      collection = collections.acceptableAds;
-    else
-      collection = collections.custom;
+    if (collection)
+      collection.addItem(subscription);
 
-    collection.addItem(subscription);
     subscriptionsMap[subscription.url] = subscription;
-    toggleShowLanguage(subscription);
     updateTooltips();
   }
 
   function updateSubscription(subscription)
   {
     for (let name in collections)
       collections[name].updateItem(subscription);
 
-    toggleShowLanguage(subscription);
+    if (subscription.recommended == "ads")
+    {
+      if (subscription.disabled)
+        collections.langs.removeItem(subscription);
+      else
+        collections.langs.addItem(subscription);
+    }
+    else if (!isProtection(subscription.recommended) &&
+      !isAcceptableAds(subscription.url))
+    {
+      if (subscription.disabled == false)
+      {
+        collections.custom.addItem(subscription);

[Thomas Greiner, 2017/08/16 17:57:09]

Again, it seems like we're still putting recommended subscriptions into the
"More Filters" section.

See
https://codereview.adblockplus.org/29478597/diff/29498701/new-options.js#newc...

[saroyanm, 2017/08/17 11:21:17]

As, same as the comment above, the definition of the recommended in the specs is
"not recommended within the Privacy & Security and Language sections".

[saroyanm, 2017/08/17 21:24:05]

Done.

+        updateTooltips();
+      }
+      else
+      {
+        collections.custom.removeItem(subscription);
+      }
+    }
   }
 
   function updateFilter(filter)
   {
     let match = filter.text.match(whitelistedDomainRegexp);
     if (match && !filtersMap[filter.text])
     {
       filter.title = match[1];
       collections.whitelist.addItem(filter);
     }
@@ skipping 120 matching lines @@
         let dialog = E("dialog-content-predefined");
         let title = dialog.querySelector("h3").textContent;
         let url = dialog.querySelector(".url").textContent;
         addEnableSubscription(url, title);
         closeDialog();
         break;
       }
       case "cancel-custom-filters":
         setCustomFiltersView("read");
         break;
+      case "change-language-subscription":
+        for (let key in subscriptionsMap)
+        {
+          let subscription = subscriptionsMap[key];
+          let subscriptionType = subscription.recommended;
+          if (subscriptionType == "ads" && subscription.disabled == false)
+          {
+            ext.backgroundPage.sendMessage({
+              type: "subscriptions.remove",
+              url: subscription.url
+            });
+            ext.backgroundPage.sendMessage({
+              type: "subscriptions.add",
+              url: findParentData(element, "access", false)
+            });
+          }

[Thomas Greiner, 2017/08/16 17:57:10]

Detail: Let's exit the loop as soon as we find the filter list we've been
searching for.

[saroyanm, 2017/08/17 21:24:06]

Done.

+        }
+        break;
       case "close-dialog":
         closeDialog();
         break;
       case "edit-custom-filters":
         setCustomFiltersView("write");
         break;
       case "import-subscription": {
         let url = E("blockingList-textbox").value;
         addEnableSubscription(url);
         closeDialog();
@@ skipping 31 matching lines @@
         sendMessageHandleErrors({
           type: "filters.importRaw",
           text: E("custom-filters-raw").value,
           removeExisting: true
         },
         () =>
         {
           setCustomFiltersView("read");
         });
         break;
+      case "switch-acceptable-ads":

[Thomas Greiner, 2017/08/16 17:57:10]

It's great that the code has been made more consistent but this is not what I
meant with my comment at
https://codereview.adblockplus.org/29478597/diff/29498701/new-options.html#ne....

Here's an example code to clarify what I was thinking of:

case "switch-acceptable-ads":
  let values = new Set(element.value.split(","));
  ext.backgroundPage.sendMessage({
    type: (values.has("privacy")) ? "subscriptions.add" :
"subscriptions.remove",
    url: acceptableAdsPrivacyUrl
  });
  ext.backgroundPage.sendMessage({
    type: (values.has("ads")) ? "subscriptions.add" : "subscriptions.remove"
  });
  break;

[saroyanm, 2017/08/17 11:21:18]

I think I missed the part of combining them into one message, I agree with you,
but we don't have the case for "ads,privacy", also not sure if we should allow
that case and having value="" is still have the issue of selecting with CSS.
What if we will still use your solution, but the way it matches the case ?

let value = element.value;
ext.backgroundPage.sendMessage({
  type: value == "privacy" ? "subscriptions.add" :
  "subscriptions.remove",
  url: acceptableAdsPrivacyUrl
});
ext.backgroundPage.sendMessage({
  type: value == "ads" ? "subscriptions.add" : "subscriptions.remove"
});

And values in HTML will be "ads", "privacy" and "none".
<input data-action="switch-acceptable-ads" type="radio" name="acceptable-ads"
value="ads">
<input data-action="switch-acceptable-ads" type="radio" name="acceptable-ads"
value="privacy">
<input data-action="switch-acceptable-ads" type="radio" name="acceptable-ads"
value="none">

Let me know what you think about this and thanks for clarification.

[Thomas Greiner, 2017/08/17 12:06:05]

Your proposal is perfectly fine. :)

[saroyanm, 2017/08/17 21:24:04]

Done.

+        ext.backgroundPage.sendMessage({
+          type: "subscriptions.remove",
+          url: acceptableAdsPrivacyUrl
+        });
+        ext.backgroundPage.sendMessage({
+          type: "subscriptions.add",
+          url: acceptableAdsUrl
+        });
+        break;
+      case "switch-privacy-acceptable-ads":
+        ext.backgroundPage.sendMessage({
+          type: "subscriptions.remove",
+          url: acceptableAdsUrl
+        });
+        ext.backgroundPage.sendMessage({
+          type: "subscriptions.add",
+          url: acceptableAdsPrivacyUrl
+        });
+        break;
+      case "switch-no-acceptable-ads":
+        ext.backgroundPage.sendMessage({
+          type: "subscriptions.remove",
+          url: acceptableAdsPrivacyUrl
+        });
+        ext.backgroundPage.sendMessage({
+          type: "subscriptions.remove",
+          url: acceptableAdsUrl
+        });
+        break;
       case "switch-tab":
         let tabId = findParentData(element, "tab", false);
         switchTab(tabId);
         break;
       case "toggle-disable-subscription":
         ext.backgroundPage.sendMessage({
           type: "subscriptions.toggle",
           keepInstalled: true,
           url: findParentData(element, "access", false)
         });
@@ skipping 156 matching lines @@
       let tabId = tabIds.slice(0, i + 1).join("-");
       tabContent = selectTabItem(tabId, tabContent, true);
       if (!tabContent)
         break;
     }
   }
 
   function onDOMLoaded()
   {
     populateLists();
-    function onFindLanguageKeyUp()
-    {
-      let searchStyle = E("search-style");
-      if (!this.value)
-        searchStyle.innerHTML = "";
-      else
-      {
-        searchStyle.innerHTML = "#all-lang-table li:not([data-search*=\"" +
-          this.value.toLowerCase() + "\"]) { display: none; }";
-      }
-    }
 
     // Initialize navigation sidebar
     ext.backgroundPage.sendMessage({
       type: "app.get",
       what: "addonVersion"
     },
     (addonVersion) =>
     {
       E("abp-version").textContent = addonVersion;
     });
     getDocLink("releases", (link) =>
     {
       E("link-version").setAttribute("href", link);
     });
 
     updateShareLink();
     updateTooltips();
 
     // Initialize interactive UI elements
     document.body.addEventListener("click", onClick, false);
     document.body.addEventListener("keyup", onKeyUp, false);
-    let placeholderValue = getMessage("options_dialog_language_find");
-    E("find-language").setAttribute("placeholder", placeholderValue);
-    E("find-language").addEventListener("keyup", onFindLanguageKeyUp, false);
     let exampleValue = getMessage("options_whitelist_placeholder_example",
       ["www.example.com"]);
     E("whitelisting-textbox").setAttribute("placeholder", exampleValue);
     E("whitelisting-textbox").addEventListener("keyup", (e) =>
     {
       E("whitelisting-add-button").disabled = !e.target.value;
     }, false);
 
+    getDocLink("acceptable_ads_criteria", (link) =>
+    {
+      setLinks("enable-aa-description", link);
+    });
+
     // Advanced tab
     let customize = document.querySelectorAll("#customize li[data-pref]");
     customize = Array.prototype.map.call(customize, (checkbox) =>
     {
       return checkbox.getAttribute("data-pref");
     });
     for (let key of customize)
     {
       getPref(key, (value) =>
       {
@@ skipping 118 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)
+  {
+    if (state)
+      E("acceptable-ads").classList.add("show-dnt-notification");
+    else
+      E("acceptable-ads").classList.remove("show-dnt-notification");
+  }
+
+  function setAcceptableAds(option)
+  {
+    let optionsContainer = E("acceptable-ads");
+    switch (option)
+    {
+      case "default":
+        optionsContainer.querySelector("[value='ads']").checked = true;
+        break;
+      case "privacy":
+        optionsContainer.querySelector("[value='ads,privacy']").checked = true;
+        break;
+      case "none":
+        optionsContainer.querySelector("[value='none']").checked = true;
+        break;
+    }
+  }
+
+  function isAcceptableAds(url)
+  {
+    return url == acceptableAdsUrl || url == acceptableAdsPrivacyUrl;
+  }
+
+  function isProtection(type)
+  {
+    return type == "privacy" || type == "social";
+  }
+
   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 25 matching lines @@
       key: "subscriptions_exceptionsurl"
     },
     (url) =>
     {
       acceptableAdsUrl = url;
       addSubscription({
         url: acceptableAdsUrl,
         disabled: true
       });
 
-      // Load user subscriptions
       ext.backgroundPage.sendMessage({
-        type: "subscriptions.get",
-        downloadable: true
+        type: "prefs.get",
+        key: "subscriptions_exceptionsurl_privacy"
       },
-      (subscriptions) =>
+      (urlPrivacy) =>
       {
-        for (let subscription of subscriptions)
-          onSubscriptionMessage("added", subscription);
+        acceptableAdsPrivacyUrl = urlPrivacy;
+
+        // Load user subscriptions
+        ext.backgroundPage.sendMessage({
+          type: "subscriptions.get",
+          downloadable: true
+        },
+        (subscriptions) =>
+        {
+          for (let subscription of subscriptions)
+            onSubscriptionMessage("added", subscription);
+        });
       });
     });
   }
 
   function addWhitelistedDomain()
   {
     let domain = E("whitelisting-textbox");
     for (let whitelistItem of collections.whitelist.items)
     {
       if (whitelistItem.title == domain.value)
@@ skipping 87 matching lines @@
       case "lastDownload":
       case "title":
         updateSubscription(subscription);
         break;
       case "added":
         if (subscription.url in subscriptionsMap)
           updateSubscription(subscription);
         else
           addSubscription(subscription);
 
+        if (subscription.url == acceptableAdsUrl)
+          setAcceptableAds("default");

[Thomas Greiner, 2017/08/16 17:57:10]

We cannot determine the current state based on which filter list got added last
because the other one could still be present.

Therefore I'd recommend checking which of the two filters lists are installed
and enabled each time any of them gets added, removed, enabled or disabled (e.g.
`if (isAcceptable(subscription.url) refreshAcceptableAds()`).

My suggestion for the states:
- aa:false + aa-privacy:false = all ads blocked
- aa:true + aa-privacy:false = all Acceptable Ads unblocked
- aa:false + aa-privacy:true = only privacy-friendly Acceptable Ads unblocked
- aa:true + aa-privacy:true = all Acceptable Ads unblocked

Because since the privacy-friendly list is only a subset of the  regular one,
having both enabled at the same time is equivalent to only having the regular
one enabled.

[saroyanm, 2017/08/17 11:21:18]

I agree. I'll update accordingly.

[saroyanm, 2017/08/17 21:24:04]

Done.

+
+        if (subscription.url == acceptableAdsPrivacyUrl)
+        {
+          setAcceptableAds("privacy");
+          if (!navigator.doNotTrack)
+            setDntNotification(true);
+        }
+
         collections.filterLists.addItem(subscription);
         break;
       case "removed":
-        if (subscription.url == acceptableAdsUrl || subscription.recommended)
+        if (subscription.recommended == "ads" ||
+            isProtection(subscription.recommended))
         {
           subscription.disabled = true;
           onSubscriptionMessage("disabled", subscription);
         }
+        if (isAcceptableAds(subscription.url))
+        {
+          setAcceptableAds("none");
+          if (subscription.url == acceptableAdsPrivacyUrl)
+          {
+            setDntNotification(false);
+          }
+        }
         else
         {
           collections.custom.removeItem(subscription);

[Thomas Greiner, 2017/08/16 17:57:09]

Detail: Recommended subscriptions should not be in the "More Filters" section
anyway.

[saroyanm, 2017/08/17 21:24:06]

Done.

           delete subscriptionsMap[subscription.url];

[Thomas Greiner, 2017/08/16 17:57:10]

In `addSubscription()` we're adding Acceptable Ads subscriptions to
`subscriptionsMap` so why aren't we removing them here?

[saroyanm, 2017/08/17 11:21:17]

I tried to be consistent with old implementation here I think... 
But I do agree that we should remove it, I'll check if that will couse any sort
of unexpected behavior, but I think we should be safe here, also same should
apply to some recommended subscriptions that are not "ads" and "protection"
type.

[Thomas Greiner, 2017/08/17 12:06:06]

Sounds good.

[saroyanm, 2017/08/17 21:24:07]

With current implementation is more understandable,actually we shouldn't delete
"knownSubscription"(subscriptionsMap) for recommended subscriptions, otherwise
they will stop being "recommended" and this will lead to being added into the
"more subscriptions" section and lead to unexpected behaviors. On another hand
we check URL for Acceptable Ads and we can remove it from "knownSubscription"
and that what I did. 

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

I don't think not removing recommended subscriptions is a good idea because
we'll mark subscriptions as disabled even though they aren't which may cause
inconsistencies in how they're displayed and how they can be interacted with as
well as errors when trying to change the state of a subscription (e.g. trying to
enable a subscription that is not even installed).

Nevertheless, as you pointed out, we need to know whether a subscription is
recommended and which type of subscription it is. We don't need to keep
subscriptions in `subscriptionsMap` for that though but store that information
separately.

         }
         collections.filterLists.removeItem(subscription);
         break;
     }
 
     updateShareLink();
   }
 
   function hidePref(key, value)
   {
@@ skipping 70 matching lines @@
       {
         // Hide the share tab if a script on the share page would be blocked
         E("tab-share").hidden = isAnyBlocked;
       }
     }
 
     for (let sharedResource of shareResources)
       checkShareResource(sharedResource, onResult);
   }
 
-  function getMessages(id)
-  {
-    let messages = [];
-    for (let i = 1; true; i++)
-    {
-      let message = ext.i18n.getMessage(id + "_" + i);
-      if (!message)
-        break;
-
-      messages.push(message);
-    }
-    return messages;
-  }
-
   function updateTooltips()
   {
     let anchors = document.querySelectorAll(":not(.tooltip) > [data-tooltip]");
     for (let anchor of anchors)
     {
       let id = anchor.getAttribute("data-tooltip");
 
       let wrapper = document.createElement("div");
       wrapper.className = "tooltip";
       anchor.parentNode.replaceChild(wrapper, anchor);
       wrapper.appendChild(anchor);
 
-      let topTexts = getMessages(id);
-      let bottomTexts = getMessages(id + "_notes");
-
-      // We have to use native tooltips to avoid issues when attaching a tooltip
-      // to an element in a scrollable list or otherwise it might get cut off
-      if (anchor.hasAttribute("data-tooltip-native"))
-      {
-        let title = topTexts.concat(bottomTexts).join("\n\n");
-        anchor.setAttribute("title", title);
-        continue;
-      }
-
       let tooltip = document.createElement("div");
       tooltip.setAttribute("role", "tooltip");
 
-      let flip = anchor.getAttribute("data-tooltip-flip");
-      if (flip)
-        tooltip.className = "flip-" + flip;
-
-      let imageSource = anchor.getAttribute("data-tooltip-image");
-      if (imageSource)
-      {
-        let image = document.createElement("img");
-        image.src = imageSource;
-        image.alt = "";
-        tooltip.appendChild(image);
-      }
-
-      for (let topText of topTexts)
-      {
-        let paragraph = document.createElement("p");
-        paragraph.innerHTML = topText;
-        tooltip.appendChild(paragraph);
-      }
-      if (bottomTexts.length > 0)
-      {
-        let notes = document.createElement("div");
-        notes.className = "notes";
-        for (let bottomText of bottomTexts)
-        {
-          let paragraph = document.createElement("p");
-          paragraph.innerHTML = bottomText;
-          notes.appendChild(paragraph);
-        }
-        tooltip.appendChild(notes);
-      }
+      let paragraph = document.createElement("p");
+      paragraph.textContent = getMessage(id);
+      tooltip.appendChild(paragraph);
 
       wrapper.appendChild(tooltip);
     }
   }
 
   ext.onMessage.addListener((message) =>
   {
     switch (message.type)
     {
       case "app.respond":
@@ skipping 38 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);
 }