Made browser actions per tab on Safari

chrome/ext/background.js

Index: chrome/ext/background.js
===================================================================
--- a/chrome/ext/background.js
+++ b/chrome/ext/background.js
@@ -191,58 +191,62 @@
   };
 
 
+  /* Browser actions */
+
+  var BrowserAction = function() {};
+  BrowserAction.prototype = {
+    _prepareParams: function(params) {},
+    _apply: function(method, params)
+    {
+      this._prepareParams(params);
+      chrome.browserAction[method](params);
+    },
+    setIcon: function(path)
+    {
+      this._apply("setIcon", {path: path});
+    },
+    setTitle: function(title)
+    {
+      this._apply("setTitle", {title: title});
+    },
+    setBadge: function(badge)
+    {
+      if (badge)
+      {
+        if ("color" in badge)
+          this._apply("setBadgeBackgroundColor", {color: badge.color});
+
+        if ("number" in badge)
+          this._apply("setBadgeText", {text: badge.number.toString()});
+      }
+      else
+        this._apply("setBadgeText", {text: ""});
+    }
+  };
+
+  var TabBrowserAction = function(tabId)
+  {
+    this._tabId = tabId;
+  };
+  TabBrowserAction.prototype = {
+    __proto__: BrowserAction.prototype,
+    _prepareParams: function(params)
+    {
+      params.tabId = this._tabId;
+    }
+  };
+
+
   /* Tabs */
 
   var sendMessage = chrome.tabs.sendMessage || chrome.tabs.sendRequest;
 
-  var BrowserAction = function(tabId)
-  {
-    this._tabId = tabId;
-  };
-  BrowserAction.prototype = {
-    setIcon: function(path)
-    {
-      chrome.browserAction.setIcon({tabId: this._tabId, path: path});
-    },
-    setTitle: function(title)
-    {
-      chrome.browserAction.setTitle({tabId: this._tabId, title: title});
-    },
-    setBadge: function(badge)
-    {
-      if (!badge)
-      {
-        chrome.browserAction.setBadgeText({
-          tabId: this._tabId,
-          text: ""
-        });
-        return;
-      }
-
-      if ("color" in badge)
-      {
-        chrome.browserAction.setBadgeBackgroundColor({
-          tabId: this._tabId,
-          color: badge.color
-        });
-      }
-
-      if ("number" in badge)
-      {
-        chrome.browserAction.setBadgeText({
-          tabId: this._tabId,
-          text: badge.number.toString()
-        });
-      }
-    }
-  };
-
   Tab = function(tab)
   {
     this._id = tab.id;
     this._url = tab.url;
 
-    this.browserAction = new BrowserAction(tab.id);
+    this.browserAction = new TabBrowserAction(tab.id);
 
     this.onBeforeNavigate = ext.tabs.onBeforeNavigate._bindToTab(this);
     this.onLoading = ext.tabs.onLoading._bindToTab(this);
@@ -534,4 +538,5 @@
   };
 
   ext.onMessage = new BackgroundMessageEventTarget();
+  ext.browserAction = new BrowserAction();

[Wladimir Palant, 2014/01/16 09:10:05]

This is unnecessary IMHO, ext.browserAction can be a dummy on Chrome -
manifest.json already takes care of defining defaults. As mentioned already, I
consider ext.browserAction to be merely an intermediate solution.

[Sebastian Noack, 2014/01/16 12:45:02]

I think it is a pretty bad idea to have different behavior for Chrome and
Safari. Also it makes the API more complete, to be able to set browser actions
per tab as well as globally. And the code required therefore is minimal for
Chrome.

[Wladimir Palant, 2014/01/16 12:55:18]

It obscures the logic even more than it already is, for a functionality that is
completely unnecessary in Chrome.

[Sebastian Noack, 2014/01/16 14:01:52]

I don't agree. It isn't that much code, I have added. And in order to make the
API consistent between Chrome and Safari, I think it is totally worth it.

And even if we wouldn't expose ext.browserAction, having the tab specific code
moved into a separate class makes the code slightly better organized, IMHO.

[Wladimir Palant, 2014/01/17 07:54:36]

Quite frankly, I disagree. IMHO the abstraction layer is already bordering to
overarchitecturing. Introducing an additional concept and extending the class
hierarchy further for no good reason isn't something I will approve.

As it is now, browser actions are *always* per-tab - we copied that concept from
Chrome and it is good that way. There is merely a default for the tabs where
nothing has been set yet. We shouldn't muddy the waters unnecessarily.

[Sebastian Noack, 2014/01/17 12:56:14]

My main concern is having different behavior for Safari and Chrome. But the only
reason we would need a global browser action API is to have the extension name
translated in the toolbar item's label and tooltip on Safari.

However considering that there is currently only one language (Arabic) where we
actually translate the extension name and I'm not sure whether that is even
intended, I think the solution isn't worth all that extra code (even when we
implement it only for Safari) and the discussion about it.

And considering that there are already many other places in Safari where we just
can't translate the extension name (e.g. under Preferences->Extensions and in
the "Customize Toolbar" menu), it might even be preferable to have the extension
name consistent and therefore don't translate it here too.

I have uploaded a new patchset, with the code for configuring browser actions
globally removed, completly (also for Safari). So you get a simpler
implementation. And I get a consistent API between Chrome and Safari.

 })();