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) =>
   {
-    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)
+    {
+      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 = {
+      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;
       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);

[Sebastian Noack, 2017/08/15 15:25:01]

It seems somewhat dirty to me to register the same function as success and error
callback, and then distinguish the case by checking whether an error argument is
given inside the function. Wouldn't it be cleaner to use a different functions
as success/error callbacks?

[Manish Jethani, 2017/08/16 10:23:01]

Done (but see comments).

+  })
+  .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 =>