Relocated icon and redesigned icon popup

chrome/background.js

Index: chrome/background.js
===================================================================
--- a/chrome/background.js
+++ b/chrome/background.js
@@ -196,26 +196,37 @@
 
   var sendMessage = chrome.tabs.sendMessage || chrome.tabs.sendRequest;
 
-  var PageAction = function(tabId)
+  var BrowserAction = function(tabId)
   {
     this._tabId = tabId;
   };
-  PageAction.prototype = {
+  BrowserAction.prototype = {
     setIcon: function(path)
     {
-      chrome.pageAction.setIcon({tabId: this._tabId, path: path});
+      chrome.browserAction.setIcon({tabId: this._tabId, path: path});
     },
     setTitle: function(title)
     {
-      chrome.pageAction.setTitle({tabId: this._tabId, title: title});
+      chrome.browserAction.setTitle({tabId: this._tabId, title: title});
     },
     hide: function()
     {
-      chrome.pageAction.hide(this._tabId);
+      chrome.browserAction.hide(this._tabId);
     },
     show: function()
     {
-      chrome.pageAction.show(this._tabId);
+      chrome.browserAction.show(this._tabId);
+    },
+    setBadgeBackgroundColor: function(color)
+    {
+      chrome.browserAction.setBadgeBackgroundColor({tabId: this._tabId, color: color});
+    },
+    setBadgeNumber: function(number)
+    {
+      chrome.browserAction.setBadgeText({
+        tabId: this._tabId,
+        text: (number === null) ? "" : number.toString()
+      });
     }

[Wladimir Palant, 2013/12/03 12:55:43]

This is a strange API - we require setting each parameter of the badge
separately. Also, we assume that the badge will never contain anything other
than a number and we pass null instead of a number to remove the badge. IMHO it
should be rather something like the following:

  setBadge: function(badge)
  {
    if (!badge)
      // Remove badge
    else
    {
      if (!badge.text)
        throw Error("Badge text is required);
      chrome.browserAction.setBadgeText(... badge.text ...);
      if ("backgroundColor" in badge)
        chrome.browserAction.setBadgeBackgroundColor(... badge.backgroundColor
...);
    }
  }

[Sebastian Noack, 2013/12/03 13:12:26]

Safari only support numbers for the badge. That is why we call it
setBadgeNumber() and don't support arbitrary strings here.

Also I don't see why it would be better to have a single function, that takes an
object with the badge number/text and the background color, instead of having
two functions as in Chrome's API.

[Wladimir Palant, 2013/12/03 13:45:03]

I see. I guess calling it badge.number rather than badge.text makes more sense
then.
Why should have two function calls to configure the badge? I cannot imagine us
calling the one function without the other meaning that separating them makes
little sense. Chrome API likely separates them for historical reasons, we don't
have those.

Note that in Firefox we'll be able to control foreground color as well - I doubt
that we want to add a third function call if we decide to support Firefox here.

[Sebastian Noack, 2013/12/03 15:03:33]

I still see no advantage of a single function with complex calling convention
over multiple functions with a simple calling convention. However I know plenty
of reasons, why multiple functions would be preferable, here:

1. Without looking at the implementation (or documentation) everybody knows how
to call the function, just from its name.
2. It is more flexible, because you can set the label and background color
independently. 
3. The implementation is simpler (and faster) without all the unneeded
conditions required to dispatch, all the mandatory and optional parameters.
4. It is more similar to Chrome's API, and therefore developers/contributors who
already know Chrome's API, have to struggle less with our API, if we adopt it.

[Thomas Greiner, 2013/12/04 10:44:50]

The reason is that in Chrome's API you can either make a single call to set the
background color globally for each tab or for each tab specifically.

However, I agree that it might be sufficient to only implement it
tab-dependently given that the calls are currently being made like that anyway.
So merging those functions makes sense in our case.

@Sebastian
1. Shouldn't be too big of a problem if we add a documentation above it.
2. We could make both properties optional. This would allow us to either set one
or both properties which gives us the biggest possible flexibility.
3. See (2)
4. The advantage of adhering to Chrome's API definition is marginal. We already
differ quite a bit from it anyway but that's OK.

[Sebastian Noack, 2013/12/04 11:48:38]

Or to set the background color only once, instead of every time you update the
badge number. Are there any reasons why you set the background color every time
you update the number?
This just mitigates an issue, that we won't have otherwise. So it certainly
isn't a reason to use a single function.
Yes, in that case we would achieve the same flexibility, as with multiple
functions. However it still makes the implementation more complex as it has to
be, due to the need of conditions to dispatch the parameters.
Only in cases where it would be not worth the effort or is impossible to adopt
Chrome's API on Safari. This isn't the case here.

[Thomas Greiner, 2013/12/10 10:09:29]

The only reason is that it would be more confusing to have |browserAction| in
two different places (ext.browserAction.setBadgeBackgroundColor and
Tab.browserAction.setBadgeNumber). Performance-wise it shouldn't be a big deal
anyway.

[Thomas Greiner, 2013/12/11 13:15:24]

Can we agree on using Wladimir's suggestion?

@Felix What's your opinion on this?

[Felix Dahlke, 2013/12/11 13:23:02]

I think it's nicer API-wise to have a single function that can set various
properties of the badge than to have one function per property. As long as it
doesn't touch unspecified properties, but Wladimir's implementation doesn't, so
I'm good.

[Thomas Greiner, 2013/12/13 10:37:40]

Done.

   };
 
@@ -224,7 +235,7 @@
     this._id = tab.id;
 
     this.url = tab.url;
-    this.pageAction = new PageAction(tab.id);
+    this.browserAction = new BrowserAction(tab.id);
 
     this.onLoading = ext.tabs.onLoading._bindToTab(this);
     this.onCompleted = ext.tabs.onCompleted._bindToTab(this);