Issue 5464 - Upgrade to new asynchronous version of abp2blocklist

safari/contentBlocking.js

Index: safari/contentBlocking.js
===================================================================
--- a/safari/contentBlocking.js
+++ b/safari/contentBlocking.js
@@ -32,74 +32,111 @@
   {
     port.off("safari.legacyAPISupported", onLegacyAPISupported);
     resolve(msg.legacyAPISupported);
   }
   port.on("safari.legacyAPISupported", onLegacyAPISupported);
 });
 let contentBlockingActive = false;
 let afterContentBlockingFinished = null;
+let pendingContentBlockerUpdate = null;
 let contentBlockListDirty = true;
 let lastSetContentBlockerError;
 
 function clearBlockCounters()
 {
   ext.pages.query({}, pages =>
   {
     for (let page of pages)
       page.browserAction.setBadge();
   });
 }
 
-function setContentBlocker(callback)
+function setContentBlocker()
 {
-  // When given the same rules as last time setContentBlocker will always give
-  // null (success) to the callback, even when there was actually an error. We
-  // cache the last result therefore so that we can provide a consistent result
-  // and also to avoid wastefully regenerating an identical blocklist.
-  if (!contentBlockListDirty)
+  return new Promise((resolve, reject) =>

[Manish Jethani, 2017/08/02 10:55:11]

I made this a promise now because it feels more natural here and also because
errors bubble up naturally, whereas they may be lost with the callback.

   {
-    callback(lastSetContentBlockerError);
-    return;
-  }
+    // Reset state and either fulfill or reject this promise.
+    let completePromise = error =>
+    {
+      lastSetContentBlockerError = error;
+      contentBlockListDirty = false;
 
-  let contentBlockerList = new ContentBlockerList();
-  for (let subscription of FilterStorage.subscriptions)
-    if (!subscription.disabled)
-      for (let filter of subscription.filters)
-        contentBlockerList.addFilter(filter);
+      if (error)
+        reject(error);
+      else
+        resolve();
+    };
+
+    // When given the same rules as last time setContentBlocker will always
+    // resolve with null (success), even when there was actually an
+    // error. We cache the last result therefore so that we can provide a
+    // consistent result and also to avoid wastefully regenerating an identical
+    // blocklist.
+    if (!contentBlockListDirty)
+    {
+      completePromise(lastSetContentBlockerError);
+      return;
+    }
 
-  contentBlockListDirty = false;
-  safari.extension.setContentBlocker(
-    // There is a strange bug in setContentBlocker for Safari 9 where if both
-    // the callback parameter is provided and the rules aren't converted to a
-    // JSON string it fails. Worse still it actually performs the callback twice
-    // too, firstly with an empty string and then with an Error:
-    //   "Extension compilation failed: Failed to parse the JSON String."
-    // To mitigate this we convert the rules to JSON here and also ignore
-    // callback values of "". (Usually the callback is performed with either
-    // null for success or an Error on failure.)
-    // Bug #26322821 filed on bugreport.apple.com
-    JSON.stringify(contentBlockerList.generateRules()),
-    function(error)
+    let contentBlockerList = new ContentBlockerList();
+    for (let subscription of FilterStorage.subscriptions)
+    {

[Manish Jethani, 2017/08/02 10:55:11]

Added some extra braces as per our style guide.

+      if (!subscription.disabled)
+      {
+        for (let filter of subscription.filters)
+          contentBlockerList.addFilter(filter);
+      }
+    }
+
+    contentBlockerList.generateRules().then(rules =>
     {
-      if (error == "")
-        return;
+      safari.extension.setContentBlocker(
+        // There is a strange bug in setContentBlocker for Safari 9 where if
+        // both the callback parameter is provided and the rules aren't
+        // converted to a JSON string it fails. Worse still it actually
+        // performs the callback twice too, firstly with an empty string and
+        // then with an Error: "Extension compilation failed: Failed to parse
+        // the JSON String." To mitigate this we convert the rules to JSON here
+        // and also ignore callback values of "". (Usually the callback is
+        // performed with either null for success or an Error on failure.)
+        // Bug #26322821 filed on bugreport.apple.com
+        JSON.stringify(rules),
+        function(error)
+        {
+          if (error == "")
+            return;
 
-      lastSetContentBlockerError = error;
-      callback(error);
-    }
-  );
+          completePromise(error);
+        }
+      );
+    })
+    .catch(completePromise);
+  });
 }
 
 function updateContentBlocker(isStartup, legacyAPISupported)
 {
+  // Another update can be requested while one is still in progress (e.g. the
+  // user adds filter lists in quick succession). When this happens, save the
+  // request and execute it later.
+  if (afterContentBlockingFinished)
+  {
+    pendingContentBlockerUpdate = {

[Manish Jethani, 2017/08/02 10:55:11]

Note that we only need to save the last request. We can discard intermediate
ones. We're only interested in making sure the rule set used by Safari is really
based on the user's last selection.

See also this comment:

https://issues.adblockplus.org/ticket/5464#comment:7

+      params: Array.from(arguments),
+      // Save the current dirty state so we can set it later before calling
+      // this function again.
+      setDirty: contentBlockListDirty
+    };
+    return;
+  }
+
   afterContentBlockingFinished = new Promise(resolve =>
   {
-    setContentBlocker(error =>
+    let setContentBlockerHandler = error =>
     {
       if (error instanceof Error)
       {
         let suppressErrorMessage = false;
 
         // If the content blocking API fails the first time it's used the
         // legacy blocking API (if available) won't have been disabled.
         if (!contentBlockingActive && legacyAPISupported)
@@ -115,19 +152,42 @@
           alert(error.message);
       }
       else if (!contentBlockingActive)
       {
         contentBlockingActive = true;
         clearBlockCounters();
       }
 
+      afterContentBlockingFinished = null;

[Manish Jethani, 2017/08/02 10:55:11]

This should be set to null before fulfilling the promise just to be correct.

       resolve(contentBlockingActive);
-      afterContentBlockingFinished = null;
-    });
+    };
+
+    // There's no need for a catch handler at the end because the handler is
+    // not expected to throw any errors. If this changes, however, then the
+    // error must be handled appropriately.
+    setContentBlocker().then(setContentBlockerHandler,
+                             setContentBlockerHandler);
+  })
+  .then(active =>
+  {
+    // If there's another update pending, execute it now.
+    if (pendingContentBlockerUpdate)
+    {
+      let {params, setDirty} = pendingContentBlockerUpdate;
+      pendingContentBlockerUpdate = null;
+
+      if (setDirty)
+        contentBlockListDirty = true;
+
+      updateContentBlocker.apply(null, params);
+      return afterContentBlockingFinished;
+    }
+
+    return active;
   });
 }
 
 if (contentBlockingSupported)
 {
   Promise.all([Prefs.untilLoaded,
                FilterNotifier.once("load"),
                legacyAPISupported]).then(resolvedValues =>