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

ext/background.js

Index: ext/background.js
===================================================================
--- a/ext/background.js
+++ b/ext/background.js
@@ -312,109 +312,100 @@
   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) =>

[Sebastian Noack, 2018/06/13 01:35:33]

Nit: The parenthesis around the argument list is optional in arrow functions
with a single argument. Every where else we omit the parenthesis in this case.

[Jon Sonesen, 2018/06/13 23:05:07]

Oh yeah forgot about that, thanks

       {
-        // 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.
-        // https://bugzilla.mozilla.org/show_bug.cgi?id=1331746
-        if ("setIcon" in browser.browserAction)
+        if (change == "iconPath")

[Sebastian Noack, 2018/06/13 01:35:33]

Unrelated, but since you are changing this code anyway, we can simplify the
Firefox for Android checks by combining them with the check here:

  if (change == "iconPath && "setIcon" in browser.browserAction)
    ...

Same for the other methods below.

[Jon Sonesen, 2018/06/13 23:05:07]

Acknowledged.

         {
-          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
+          // 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.
+          // https://bugzilla.mozilla.org/show_bug.cgi?id=1331746
+          if ("setIcon" in browser.browserAction)
           {
-            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});
+            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

[Jon Sonesen, 2018/06/12 21:18:02]

Now that this is in polyfill.js I suspect this not longer works for edge, but do
not have a test VM at this exact moment.

+            {
+              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];
+              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)
+        if (change == "badgeText")
         {
-          browser.browserAction.setBadgeText({
-            tabId: this._tabId,
-            text: this._changes.badgeText
-          });
+          // There is no badge on Firefox for Android; the browser action is
+          // simply a menu item.
+          if ("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({
-            tabId: this._tabId,
-            color: this._changes.badgeColor
-          });
-        }
-      }
 
-      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)
+        if (change == "badgeColor")
         {
-          let onReplaced = (addedTabId, removedTabId) =>
+          // There is no badge on Firefox for Android; the browser action is
+          // simply a menu item.
+          if ("setBadgeBackgroundColor" in browser.browserAction)
           {
-            if (addedTabId == this._tabId)
-            {
-              browser.tabs.onReplaced.removeListener(onReplaced);
-              this._applyChanges();
-            }
-          };
-          browser.tabs.onReplaced.addListener(onReplaced);
+            return browser.browserAction.setBadgeBackgroundColor({
+              tabId: this._tabId,
+              color: this._changes.badgeColor
+            });
+          }
         }
-        else
-        {
-          this._applyChanges();
-        }
-      });
+
+        this._changes = null;

[Sebastian Noack, 2018/06/13 01:35:33]

It seems in case of failure (when we register the onReplaced handler to handle
prerendered tabs) the changes (still to be performed) will no longer be known.

[Jon Sonesen, 2018/06/13 23:05:07]

Acknowledged.

+      }));
     },
     _addChange(name, value)
     {
       if (!this._changes)
       {
         this._changes = {};
-        this._queueChanges();
       }
 
       this._changes[name] = value;
+      this._applyChanges().catch(() =>

[Sebastian Noack, 2018/06/13 01:35:33]

If this._changes is not null (and an onReplaced handler has already been
registered), we should not call _applyChanges() again but wait for the
onReplaced handler to take care of them.

[Jon Sonesen, 2018/06/13 23:06:40]

Acknowledged.

+      {
+        // 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.
+        let onReplaced = (addedTabId, removedTabId) =>
+        {
+          if (addedTabId == this._tabId)
+          {
+            browser.tabs.onReplaced.removeListener(onReplaced);
+            this._applyChanges();
+          }
+        };
+        browser.tabs.onReplaced.addListener(onReplaced);
+      });
     },
     setIcon(path)
     {
       this._addChange("iconPath", path);
     },
     setBadge(badge)
     {
       if (!badge)