Made browser actions per tab on Safari

safari/ext/background.js

Index: safari/ext/background.js
===================================================================
--- a/safari/ext/background.js
+++ b/safari/ext/background.js
@@ -81,6 +81,8 @@
   {
     this._tab = tab;
 
+    this.browserAction = new BrowserAction(this);
+
     this.onLoading = new LoadingTabEventTarget(tab);
     this.onBeforeNavigate = new TabEventTarget(tab, "beforeNavigate", false);
     this.onCompleted = new TabEventTarget(tab, "navigate", false);
@@ -106,23 +108,6 @@
         message, responseCallback,
         this._tab.page, this._tab
       );
-    },
-    browserAction: {
-      setIcon: function(path)
-      {
-        safari.extension.toolbarItems[0].image = safari.extension.baseURI + path;
-      },
-      setTitle: function(title)
-      {
-        safari.extension.toolbarItems[0].toolTip = title;
-      },
-      setBadge: function(badge)
-      {
-        if (!badge)
-          safari.extension.toolbarItems[0].badge = 0;
-        else if ("number" in badge)
-          safari.extension.toolbarItems[0].badge = badge.number;
-      }
     }
   };
 
@@ -193,6 +178,86 @@
     this._delete(tab._tab);
   };
 
+  ext.tabs = {
+    onLoading: new LoadingTabEventTarget(safari.application),
+    onBeforeNavigate: new TabEventTarget(safari.application, "beforeNavigate", true),
+    onCompleted: new TabEventTarget(safari.application, "navigate", true),
+    onActivated: new TabEventTarget(safari.application, "activate", true),
+    onRemoved: new TabEventTarget(safari.application, "close", true)
+  };
+
+
+  /* Browser actions */
+
+  var globalTooltipItemProperties = {}
+  var tooltipItemPropertiesPerTab = new TabMap();

[Wladimir Palant, 2014/01/15 15:24:47]

You meant toolbarItem, not tooltipItem, right?

[Sebastian Noack, 2014/01/15 19:25:47]

It is gone now.

+
+  var getToolbarItemForWindow = function(win)
+  {
+    for (var i = 0; i < safari.extension.toolbarItems.length; i++)
+    {
+      var toolbarItem = safari.extension.toolbarItems[i];
+
+      if (toolbarItem.browserWindow == win)
+        return toolbarItem;
+    }

[Wladimir Palant, 2014/01/15 15:24:47]

This should return something if no toolbar item is found - return null; at the
end. The callers also need to handle this scenario properly. We *should* not see
this scenario but I don't think the API guarantees that our toolbar item will be
in every window.

[Sebastian Noack, 2014/01/15 19:25:47]

Done. Even though I think that it is a bug when this actually happens, and I
would prefer to see an error in that case.

+  };
+
+  var BrowserAction = function(tab)
+  {
+    this._tab = tab;
+  };
+  BrowserAction.prototype = {
+    _set: function(property, value)
+    {
+      var currentWindow = this._tab._tab.browserWindow;
+      var toolbarItem = getToolbarItemForWindow(currentWindow);
+
+      if (!(property in globalTooltipItemProperties))
+        globalTooltipItemProperties[property] = toolbarItem[property];
+
+      if (this._tab._tab == currentWindow.activeTab)
+        toolbarItem[property] = value;
+
+      var props = tooltipItemPropertiesPerTab.get(this._tab);
+      if (!props)
+      {
+        props = {};
+        tooltipItemPropertiesPerTab.set(this._tab, props);
+      }
+      props[property] = value;
+    },
+    setIcon: function(path)
+    {
+      this._set("image", safari.extension.baseURI + path);
+    },
+    setTitle: function(title)
+    {
+      this._set("toolTip", title);
+    },
+    setBadge: function(badge)
+    {
+      if (!badge)
+        this._set("badge", 0);
+      else if ("number" in badge)
+        this._set("badge", badge.number);
+    }
+  };
+
+  ext.tabs.onActivated.addListener(function(tab)
+  {
+    var props = tooltipItemPropertiesPerTab.get(tab);
+    var toolbarItem = getToolbarItemForWindow(tab._tab.browserWindow);
+
+    for (var property in globalTooltipItemProperties)
+    {
+      if (props && property in props)
+        toolbarItem[property] = props[property];
+      else
+        toolbarItem[property] = globalTooltipItemProperties[property];
+    }

[Wladimir Palant, 2014/01/15 15:24:47]

What's the point of globalTooltipItemProperties? I guess the idea is to show a
proper state for new tabs where nothing loaded yet (no about:blank load
notification in Safari?). But this should really be:

    toolbarItem.badge = 0;
    if (props)
      for (property in props)
        toolbarItem[property] = props[property];

This makes sure the badge is never stuck. As to the other properties, it should
be fine to have those in a "wrong" state for new tabs (your approach doesn't
really prevent it either).

[Sebastian Noack, 2014/01/15 19:25:47]

This approach just emulates the behavior we already have in Chrome, that when
there is no state defined for the current tab, it falls back to the global
state. You might be right that ABP relies on that behavior only for the badge.
However I would like to keep the abstraction layer generic. So ABP-specific code
should be avoided here. But even more important, having different behavior for
Safari and Chrome, would quickly lead to strange bugs, when high level code
changes.

Also the code has changed a lot meanwhile. So I'm not sure whether this
discussion is actually still current, since the new version can also set the
global state via ext.browserAction, and it has to so, in order fix issues on
Safari.

+  });
+
 
   /* Windows */
 
@@ -538,14 +603,6 @@
     }
   };
 
-  ext.tabs = {
-    onLoading: new LoadingTabEventTarget(safari.application),
-    onBeforeNavigate: new TabEventTarget(safari.application, "beforeNavigate", true),
-    onCompleted: new TabEventTarget(safari.application, "navigate", true),
-    onActivated: new TabEventTarget(safari.application, "activate", true),
-    onRemoved: new TabEventTarget(safari.application, "close", true)
-  };
-
   ext.backgroundPage = {
     getWindow: function()
     {