Issue 6490 - Wrap browser.browserAction.set* with Promise

ext/background.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present 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 86 matching lines @@
       }
     },
     sendMessage(message, responseCallback)
     {
       browser.tabs.sendMessage(this.id, message, responseCallback);
     }
   };
 
   ext.getPage = id => new Page({id: parseInt(id, 10)});
 
-  function afterTabLoaded(callback)
-  {
-    return openedTab =>
-    {
-      let onUpdated = (tabId, changeInfo, tab) =>
-      {
-        if (tabId == openedTab.id && changeInfo.status == "complete")
-        {
-          browser.tabs.onUpdated.removeListener(onUpdated);
-          callback(new Page(openedTab));
-        }
-      };
-      browser.tabs.onUpdated.addListener(onUpdated);
-    };
-  }
-
   ext.pages = {
     onLoading: new ext._EventTarget(),
     onActivated: new ext._EventTarget(),
     onRemoved: new ext._EventTarget()
   };
 
   browser.tabs.onUpdated.addListener((tabId, changeInfo, tab) =>
   {
     if (changeInfo.status == "loading")
       ext.pages.onLoading._dispatch(new Page(tab));
@@ skipping 119 matching lines @@
           return;
       }
     }
 
     updatePageFrameStructure(details.frameId, details.tabId, details.url,
                              details.parentFrameId);
   },
   {types: ["main_frame", "sub_frame"], urls: ["http://*/*", "https://*/*"]},
   ["responseHeaders"]);
 
+  browser.webNavigation.onBeforeNavigate.addListener(details =>
+  {
+    // Requests can be made by about:blank frames before the frame's
+    // onCommitted event has fired, so we update the frame structure
+    // for those now.
+    if (details.url == "about:blank")
+    {
+      updatePageFrameStructure(details.frameId, details.tabId, details.url,
+                               details.parentFrameId);
+    }
+  });
+
   browser.webNavigation.onCommitted.addListener(details =>
   {
     // We have to update the frame structure for documents that weren't
     // loaded over HTTP (including documents cached by Service Workers),
     // when the navigation occurs. However, we must be careful to not
     // update the state of the same document twice, otherewise the number
     // of any ads blocked already and any recorded sitekey could get lost.
     let frame = ext.getFrame(details.tabId, details.frameId);
     if (!frame || frame.url.href != details.url)
     {
@@ skipping 26 matching lines @@
   /* Browser actions */
 
   let BrowserAction = function(tabId)
   {
     this._tabId = tabId;
     this._changes = null;
   };
   BrowserAction.prototype = {
     _applyChanges()
     {
-      if ("iconPath" in this._changes)
+      return Promise.all(Object.keys(this._changes).map(change =>
       {
         // Firefox for Android displays the browser action not as an icon but
-        // as a menu item. There is no icon, but such an option may be added in
-        // the future.
+        // as a menu item. There is no icon, but such an option may be added
+        // in the future.
         // https://bugzilla.mozilla.org/show_bug.cgi?id=1331746
-        if ("setIcon" in browser.browserAction)
+        if (change == "iconPath" && "setIcon" in browser.browserAction)
         {
           let path = {
             16: this._changes.iconPath.replace("$size", "16"),
             19: this._changes.iconPath.replace("$size", "19"),
             20: this._changes.iconPath.replace("$size", "20"),
             32: this._changes.iconPath.replace("$size", "32"),
             38: this._changes.iconPath.replace("$size", "38"),
             40: this._changes.iconPath.replace("$size", "40")
           };
           try
           {
-            browser.browserAction.setIcon({tabId: this._tabId, path}, () =>
-            {
-              browser.runtime.lastError;
-            });
+            return browser.browserAction.setIcon({tabId: this._tabId, path});
           }
           catch (e)
           {
             // Edge throws if passed icon sizes different than 19,20,38,40px.
             delete path[16];
             delete path[32];
-            browser.browserAction.setIcon({tabId: this._tabId, path}, () =>
-            {
-              browser.runtime.lastError;
-            });
+            return browser.browserAction.setIcon({tabId: this._tabId, path});
           }
         }
-      }
-
-      if ("badgeText" in this._changes)
-      {
+
         // There is no badge on Firefox for Android; the browser action is
         // simply a menu item.
-        if ("setBadgeText" in browser.browserAction)
-        {
-          browser.browserAction.setBadgeText({
+        if (change == "badgeText" && "setBadgeText" in browser.browserAction)
+          return browser.browserAction.setBadgeText({
             tabId: this._tabId,
             text: this._changes.badgeText
           });
-        }
-      }
-
-      if ("badgeColor" in this._changes)
-      {
+
         // There is no badge on Firefox for Android; the browser action is
         // simply a menu item.
-        if ("setBadgeBackgroundColor" in browser.browserAction)
-        {
-          browser.browserAction.setBadgeBackgroundColor({
+        if (change == "badgeColor" &&
+            "setBadgeBackgroundColor" in browser.browserAction)
+          return browser.browserAction.setBadgeBackgroundColor({
             tabId: this._tabId,
             color: this._changes.badgeColor
           });
+      }));
+    },
+    _addChange(name, value)
+    {
+      let onReplaced = (addedTabId, removedTabId) =>

[Jon Sonesen, 2018/07/10 23:01:07]

Since I had trouble removing the check below this definition would still need to
be here.

+      {
+        if (addedTabId == this._tabId)
+        {
+          browser.tabs.onReplaced.removeListener(onReplaced);
+          this._applyChanges().then(() =>
+          {
+            this._changes = null;
+          });
         }
-      }
-
-      this._changes = null;
-    },
-    _queueChanges()
-    {
-      browser.tabs.get(this._tabId, () =>
-      {
-        // If the tab is prerendered, browser.tabs.get() sets
-        // browser.runtime.lastError and we have to delay our changes
-        // until the currently visible tab is replaced with the
-        // prerendered tab. Otherwise browser.browserAction.set* fails.
-        if (browser.runtime.lastError)
-        {
-          let onReplaced = (addedTabId, removedTabId) =>
-          {
-            if (addedTabId == this._tabId)
-            {
-              browser.tabs.onReplaced.removeListener(onReplaced);
-              this._applyChanges();
-            }
-          };
+      };
+      if (!this._changes)
+        this._changes = {};
+
+      this._changes[name] = value;
+      if (!browser.tabs.onReplaced.hasListener(onReplaced))

[Jon Sonesen, 2018/07/10 23:01:07]

Replacing this check with the applyChanges boolean does not seem to be a
reliable indication of whether a listener has been added or not. Removing the
hasListener check causes the badge color to stay blue.

+      {
+        this._applyChanges().then(() =>
+        {
+          this._changes = null;
+        }).catch(() =>

[Jon Sonesen, 2018/07/10 23:01:08]

I still think this is more readable than the callback-ish way. But am happy to
change.

+        {
+          // If the tab is prerendered, browser.browserAction.set* fails
+          // and we have to delay our changes until the currently visible tab
+          // is replaced with the prerendered tab.
           browser.tabs.onReplaced.addListener(onReplaced);
-        }
-        else
-        {
-          this._applyChanges();
-        }
-      });
-    },
-    _addChange(name, value)
-    {
-      if (!this._changes)
-      {
-        this._changes = {};
-        this._queueChanges();
-      }
-
-      this._changes[name] = value;
+        });
+      }
     },
     setIcon(path)
     {
       this._addChange("iconPath", path);
     },
     setBadge(badge)
     {
       if (!badge)
       {
         this._addChange("badgeText", "");
@@ skipping 159 matching lines @@
           return frames.get(0) || null;
         }
       };
     }
 
     return ext.onMessage._dispatch(
       message, sender, sendResponse
     ).includes(true);
   });
 }