Issue 2689 - Logic problem in BrowserAction._addChange

chrome/ext/background.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2015 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 136 matching lines @@
   });
 
   chrome.tabs.onRemoved.addListener(forgetTab);
 
 
   /* Browser actions */
 
   var BrowserAction = function(tabId)
   {
     this._tabId = tabId;
-    this._changes = null;
+    this._changes = {};
   };
   BrowserAction.prototype = {
     _applyChanges: function()
     {
       if ("iconPath" in this._changes)
       {
         chrome.browserAction.setIcon({
           tabId: this._tabId,
           path: {
             19: this._changes.iconPath.replace("$size", "19"),
@@ skipping 11 matching lines @@
       }
 
       if ("badgeColor" in this._changes)
       {
         chrome.browserAction.setBadgeBackgroundColor({
           tabId: this._tabId,
           color: this._changes.badgeColor
         });
       }
 
-      this._changes = null;
+      this._changes = {};
     },
     _queueChanges: function()
     {
       chrome.tabs.get(this._tabId, function()
       {
         // If the tab is prerendered, chrome.tabs.get() sets
         // chrome.runtime.lastError and we have to delay our changes
         // until the currently visible tab is replaced with the
         // prerendered tab. Otherwise chrome.browserAction.set* fails.
         if (chrome.runtime.lastError)
         {
           var onReplaced = function(addedTabId, removedTabId)
           {
             if (addedTabId == this._tabId)
             {
               chrome.tabs.onReplaced.removeListener(onReplaced);
               this._applyChanges();
             }
           }.bind(this);
           chrome.tabs.onReplaced.addListener(onReplaced);
         }
         else
         {
           this._applyChanges();
         }
       }.bind(this));
     },
     _addChange: function(name, value)
     {
-      if (!this._changes)
-      {
-        this._changes = {};
-        this._queueChanges();
-      }
-
       this._changes[name] = value;
+      this._queueChanges();

[Sebastian Noack, 2015/06/23 09:46:36]

This will call chrome.tabs.get() redundantly if multiple changes are added
synchronously (or just before _applyChanges is called). Frankly, I don't
understand the problem you try to fix here.

     },
     setIcon: function(path)
     {
       this._addChange("iconPath", path);
     },
     setBadge: function(badge)
     {
       if (!badge)
       {
         this._addChange("badgeText", "");
@@ skipping 408 matching lines @@
             callback(new Page(tab));
         }
         else
         {
           ext.pages.open(optionsUrl, callback);
         }
       });
     });
   };
 })();