Made browser actions per tab on Safari

safari/ext/background.js

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
  * Copyright (C) 2006-2013 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 171 matching lines @@
     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 toolbarItemProperties = {};
+
+  var getToolbarItemProperty = function(name)
+  {
+    var property = toolbarItemProperties[name];
+    if (!property)
+    {
+      property = {tabs: new TabMap()};
+      toolbarItemProperties[name] = property;
+    }
+    return property;
+  };
 
   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.

+
+    return null;
   };
 
   var BrowserAction = function(tab)
   {
     this._tab = tab;
   };
   BrowserAction.prototype = {
-    _set: function(property, value)
+    _set: function(name, 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;
+      if (toolbarItem)
+      {
+        var property = getToolbarItemProperty(name);
+        property.tabs.set(this._tab, value);
+
+        if (!("global" in property))
+          property.global = toolbarItem[name];
+
+        if (this._tab._tab == currentWindow.activeTab)
+          toolbarItem[name] = value;
+      }
     },
     setIcon: function(path)
     {
       this._set("image", safari.extension.baseURI + path);
     },
     setTitle: function(title)
     {
+      this._set("label", 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];
+    if (!toolbarItem)
+      return;
+
+    for (var name in toolbarItemProperties)
+    {
+      var property = toolbarItemProperties[name];
+
+      if (property.tabs.has(tab))
+        toolbarItem[name] = property.tabs.get(tab);
       else
-        toolbarItem[property] = globalTooltipItemProperties[property];
+        toolbarItem[name] = property.global;
     }

[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 */
 
   Window = function(win)
   {
     this._win = win;
   }
   Window.prototype = {
@@ skipping 342 matching lines @@
   };
 
   ext.onMessage = new BackgroundMessageEventTarget();
 
   // TODO: Implement context menu
   ext.contextMenus = {
     create: function(title, contexts, onclick) {},
     removeAll: function(callback) {}
   };
 })();