Issue 5464 - Upgrade to new asynchronous version of abp2blocklist

safari/contentBlocking.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
@@ skipping 37 matching lines @@
     for (let page of pages)
       page.browserAction.setBadge();
   });
 }
 
 function setContentBlocker()
 {
   return new Promise((resolve, reject) =>
   {
     // Reset state and either fulfill or reject this promise.
-    let completePromise = error =>
+    function completePromise(error)
     {
       lastSetContentBlockerError = error;
       contentBlockListDirty = false;
 
-      if (error)
+      if (error instanceof 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;
@@ skipping 44 matching lines @@
   {
     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 =>
-  {
-    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)
-        {
-          Prefs.safariContentBlocker = false;
-          // If content blocking failed on startup and we're switching back to
-          // the legacy API anyway we don't need to show an error message.
-          if (isStartup)
-            suppressErrorMessage = true;
-        }
-
-        if (!suppressErrorMessage)
-          alert(error.message);
-      }
-      else if (!contentBlockingActive)
-      {
-        contentBlockingActive = true;
-        clearBlockCounters();
-      }
-
-      afterContentBlockingFinished = null;
-      resolve(contentBlockingActive);
-    };
-
-    // 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).

+  afterContentBlockingFinished = setContentBlocker().then(() =>
+  {
+    if (!contentBlockingActive)
+    {
+      contentBlockingActive = true;
+      clearBlockCounters();
+    }
+  },
+  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)
+    {
+      Prefs.safariContentBlocker = false;
+      // If content blocking failed on startup and we're switching back to
+      // the legacy API anyway we don't need to show an error message.
+      if (isStartup)
+        suppressErrorMessage = true;
+    }
+
+    if (!suppressErrorMessage)
+      alert(error.message);
   })
-  .then(active =>
-  {
+  .then(() =>
+  {
+    afterContentBlockingFinished = null;
+
     // 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;
+    return contentBlockingActive;
   });
 }
 
 if (contentBlockingSupported)
 {
   Promise.all([Prefs.untilLoaded,
                FilterNotifier.once("load"),
                legacyAPISupported]).then(resolvedValues =>
   {
     let legacyAPISupported = resolvedValues[2];
@@ skipping 17 matching lines @@
     });
   });
 }
 
 port.on("safari.contentBlockingActive", (msg, sender) =>
 {
   if (!contentBlockingActive && afterContentBlockingFinished)
     return afterContentBlockingFinished;
   return contentBlockingActive;
 });