Issue 6699 - Support the "circumvention" filter list as default subscription

lib/subscriptionInit.js

Index: lib/subscriptionInit.js
===================================================================
--- a/lib/subscriptionInit.js
+++ b/lib/subscriptionInit.js
@@ -86,57 +86,65 @@
   return true;
 }
 
 /**
  * Finds the element for the default ad blocking filter subscription based

[Manish Jethani, 2018/06/13 08:15:16]

"element" and "subscription" should be plural now?

[hub, 2018/06/13 16:33:36]

Done.

  * on the user's locale.
  *
  * @param {HTMLCollection} subscriptions
- * @return {Element}
+ * @return {Object}

[Manish Jethani, 2018/06/13 08:15:16]

We should probably return a Map here, since it is dynamically constructed. If
the keys are always going to be the same ("ads", "cv"), then it's better as an
object, but then we should document the keys like so:

  /**
   * @typedef {object} SubscriptionMap
   * @property {?Element} ads
   * @property {?Element} cv
   */

  /**
   * ...
   * @returns {SubscriptionMap}
   */

[hub, 2018/06/13 16:33:37]

Renamed it "DefaultSubscriptions"

  */
-function chooseFilterSubscription(subscriptions)
+function chooseFilterSubscriptions(subscriptions)
 {
-  let selectedItem = null;
+  let selectedItem = {};
   let selectedPrefix = null;
   let matchCount = 0;
   for (let subscription of subscriptions)
   {
-    if (!selectedItem)
-      selectedItem = subscription;
-
     let prefixes = subscription.getAttribute("prefixes");
     let prefix = prefixes && prefixes.split(",").find(
       lang => new RegExp("^" + lang + "\\b").test(Utils.appLocale)
     );
 
     let subscriptionType = subscription.getAttribute("type");
 
-    if (prefix && subscriptionType == "ads")
+    if (subscriptionType in ["ads", "cv"] && !selectedItem[subscriptionType])

[Manish Jethani, 2018/06/13 08:15:16]

(This is a change in behavior, just noting. Previously the first subscription
would always be returned even if there was no "ads" type of subscription. Now
the default return value will be an empty object.)

[hub, 2018/06/13 16:33:36]

There is a guarantee to have an "ads" subscription since the subscriptions.xml
ship with the add-on. This doesn't change.

[Manish Jethani, 2018/06/13 16:53:09]

Acknowledged.

+      selectedItem[subscriptionType] = subscription;
+
+    if (prefix)
     {
-      if (!selectedPrefix || selectedPrefix.length < prefix.length)
-      {
-        selectedItem = subscription;
-        selectedPrefix = prefix;
-        matchCount = 1;
-      }
-      else if (selectedPrefix && selectedPrefix.length == prefix.length)
+      // The "ads" subscription is the one driving the selection.

[Manish Jethani, 2018/06/13 08:15:16]

Why only "ads", shouldn't this logic apply to all types of subscriptions?

[Manish Jethani, 2018/06/13 13:34:02]

OK, I guess since the "cv" subscriptions are our own we just have one anyway? In
that case it's probably OK, let's leave it as it is.

[hub, 2018/06/13 16:33:36]

Only "ads" and "cv" are relevant here. Also "cv" will have language variants. At
least that's the plan.

[Manish Jethani, 2018/06/13 16:53:09]

Acknowledged.

+      if (subscriptionType == "ads")
       {
-        matchCount++;
+        if (!selectedPrefix || selectedPrefix.length < prefix.length)
+        {
+          selectedItem[subscriptionType] = subscription;
+          selectedPrefix = prefix;
+          matchCount = 1;
+        }
+        else if (selectedPrefix && selectedPrefix.length == prefix.length)
+        {
+          matchCount++;
 
-        // If multiple items have a matching prefix of the same length:
-        // Select one of the items randomly, probability should be the same
-        // for all items. So we replace the previous match here with
-        // probability 1/N (N being the number of matches).
-        if (Math.random() * matchCount < 1)
-        {
-          selectedItem = subscription;
-          selectedPrefix = prefix;
+          // If multiple items have a matching prefix of the same length:

[Manish Jethani, 2018/06/13 08:15:16]

(Note: This comment is a bit misleading, because the chances are not even. If
there are two subscriptions with the same prefix, the chances are 75:25 rather
than 50:50. This is because the first one gets two shots. The subscriptions that
appear after have a significantly lower likelihood of getting selected than the
ones that come before, progressively. This would be a serious error in
cryptography, probably not so much here; nevertheless the comment should be
updated. Let's do it as a separate patch.)

[Manish Jethani, 2018/06/13 09:42:29]

Duh! Sorry, please ignore the above, it's rubbish. I fell for a common fallacy,
of course the probability is independent on each selection.

[hub, 2018/06/13 16:33:37]

Acknowledged.

+          // Select one of the items randomly, probability should be the same
+          // for all items. So we replace the previous match here with
+          // probability 1/N (N being the number of matches).
+          if (Math.random() * matchCount < 1)
+          {
+            selectedItem[subscriptionType] = subscription;
+            selectedPrefix = prefix;
+          }
         }
       }
+      else if (subscriptionType == "cv")

[Manish Jethani, 2018/06/13 08:15:16]

(I notice that the XML file uses full names like "ads", "social", "privacy", and
so on. Can we call this "circumvention" or "anticircumvention" rather than "cv",
for consistency?)

[hub, 2018/06/13 16:33:36]

It is not visible to the user.
We have been using the term "cv" and it is much shorter.

[Manish Jethani, 2018/06/13 16:53:09]

We had this discussion about the label to use on Trac, for example. "cv" was
suggested, but we went with "circumvention". Even our IRC channel is called
#circumvention.

"cv" is not a popular abbreviation of circumvention (e.g. you won't find it in
any book or dictionary), it's something we invented, so somebody reading the XML
would have to wonder what "cv" means exactly.

Anyway, my two cents.

[hub, 2018/06/13 21:58:18]

Done.

+      {
+        selectedItem[subscriptionType] = subscription;
+      }
     }
   }
   return selectedItem;
 }
 
 function supportsNotificationsWithButtons()
 {
   // Microsoft Edge (as of EdgeHTML 16) doesn't have the notifications API.
@@ -201,27 +209,35 @@
   {
     return fetch("subscriptions.xml")
       .then(response => response.text())
       .then(text =>
       {
         let doc = new DOMParser().parseFromString(text, "application/xml");
         let nodes = doc.getElementsByTagName("subscription");
 
-        let node = chooseFilterSubscription(nodes);
-        if (node)
+        let subs = chooseFilterSubscriptions(nodes);

[Manish Jethani, 2018/06/13 08:15:16]

OK, so the return value of chooseFilterSubscriptions should be a Map now.

[hub, 2018/06/13 16:33:37]

An Object, not a Map.

[Manish Jethani, 2018/06/13 16:53:09]

Yeah, I mean let's make it a Map. We have to iterate over the entries, it's
almost certainly more efficient as a Map object.

[hub, 2018/06/13 21:58:18]

To iterate over two entries? In a function that might be called at most once?

[Manish Jethani, 2018/06/14 05:07:33]

OK, sure if you prefer it this way.

+        if (subs)
         {
-          let url = node.getAttribute("url");
-          if (url)
+          for (let name in subs)
           {
-            let subscription = Subscription.fromURL(url);
-            subscription.disabled = false;
-            subscription.title = node.getAttribute("title");
-            subscription.homepage = node.getAttribute("homepage");
-            subscriptions.push(subscription);
+            let node = subs[name];
+            if (!node)
+              continue;
+
+            let url = node.getAttribute("url");
+            if (url)
+            {
+              let subscription = Subscription.fromURL(url);
+              subscription.disabled = false;
+              subscription.title = node.getAttribute("title");
+              subscription.homepage = node.getAttribute("homepage");
+              subscription.type = node.getAttribute("type");

[Manish Jethani, 2018/06/13 08:15:16]

Don't we have to add a type property to the Subscription class in core now? You
can file an issue for that, if you haven't.

[hub, 2018/06/13 16:33:36]

Added that to issue #6689 (patch not yet in review).

[Manish Jethani, 2018/06/13 16:53:09]

Acknowledged.

+              subscriptions.push(subscription);
+            }
           }
         }
 
         return subscriptions;
       });
   }
 
   return subscriptions;
@@ -301,8 +317,13 @@
  * that will effectively be added.
  *
  * @param {function} callback
  */
 exports.setSubscriptionsCallback = callback =>
 {
   subscriptionsCallback = callback;
 };
+
+

[Manish Jethani, 2018/06/13 08:15:16]

Let's use "//" for the comment here to clearly distinguish it from JSDoc
comments.

  // Export for tests only.

[hub, 2018/06/13 16:33:36]

Done.

+/* Tests only
+ */
+exports.chooseFilterSubscriptions = chooseFilterSubscriptions;

[hub, 2018/06/13 03:36:24]

The only reason I'm exporting this is for the qunit tests. Maybe it should be
exported as "_chooseFilterSubscriptions" instead?