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 179 matching lines @@
   let BrowserAction = function(tabId)
   {
     this._tabId = tabId;
     this._changes = null;
   };
   BrowserAction.prototype = {
     _applyChanges()
     {
       return Promise.all(Object.keys(this._changes).map(change =>
       {
-        if (change == "iconPath")

[Sebastian Noack, 2018/06/16 02:02:52]

Combine the check here as well?

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

[Jon Sonesen, 2018/07/09 21:15:25]

Acknowledged.

-        {
-          // 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)
+        // 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 (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
           {
-            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
-            {
-              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});
-            }
+            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});
           }
         }
 
         // There is no badge on Firefox for Android; the browser action is
         // simply a menu item.
         if (change == "badgeText" && "setBadgeText" in browser.browserAction)
           return 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 (change == "badgeColor" &&
-        "setBadgeBackgroundColor" in browser.browserAction)

[Sebastian Noack, 2018/06/16 02:02:52]

Nit: Please indent this line so that the condition on this line is aligned with
the condition on the line above:

  if (change == "badgeColor" &&
      "setBadgeBackgroundColor" in browser.browserAction)

[Jon Sonesen, 2018/07/09 21:15:25]

Acknowledged.

+            "setBadgeBackgroundColor" in browser.browserAction)
           return browser.browserAction.setBadgeBackgroundColor({
             tabId: this._tabId,
             color: this._changes.badgeColor
           });
       }));
     },
     _addChange(name, value)
     {
       let onReplaced = (addedTabId, removedTabId) =>

[Sebastian Noack, 2018/06/16 02:02:53]

For better code locality, I'd define this listener right before registering it,
like you did before.

[Jon Sonesen, 2018/07/09 21:15:24]

Acknowledged.

[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;
           });
         }
       };
       if (!this._changes)
         this._changes = {};
 
       this._changes[name] = value;
       if (!browser.tabs.onReplaced.hasListener(onReplaced))

[Sebastian Noack, 2018/06/16 02:02:53]

Is this check necessary? It might be better to just rely on this._changes:

  let applyChanges = false;

  if (!this._changes)
  {
    this._changes = {};
    applyChanges = true;
  }

  this._changes[name] = value;

  if (applyChanges)
  {
    this._applyChanges().then(...);
  }

[Jon Sonesen, 2018/07/09 21:15:24]

Acknowledged.

[Jon Sonesen, 2018/07/09 22:46:00]

So perhaps i did this wrong but removing the check and the onReplaced definition
and using the applyChanges boolean doesn't seem to work right. It appears to not
be a reliable indicator of whether the listener is registered.

[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(() =>

[Sebastian Noack, 2018/06/16 02:02:53]

Instead of .then(onSuccess).catch(onFailure),
you can call .then(onSuccess, onFailure).

[Jon Sonesen, 2018/07/09 21:15:25]

Yeah, I am aware of that. I thought it was a bit more readable but your
suggestion fits our callbackish coding style better.

[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);
         });
       }
     },
     setIcon(path)
     {
@@ skipping 166 matching lines @@
           return frames.get(0) || null;
         }
       };
     }
 
     return ext.onMessage._dispatch(
       message, sender, sendResponse
     ).includes(true);
   });
 }