Made browser actions per tab on Safari

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 99 matching lines @@
     // when using Safari, because icons are grayscale already and icons
     // aren't per tab in Safari.
     iconFilename = "icons/abp-16.png"
   else
   {
     var excluded = isWhitelisted(tab.url);
     iconFilename = excluded ? "icons/abp-19-whitelisted.png" : "icons/abp-19.png";
   }
 
   tab.browserAction.setIcon(iconFilename);
-  tab.browserAction.setTitle(ext.i18n.getMessage("name"));
-
   iconAnimation.registerTab(tab, iconFilename);
 
   // Set context menu status according to whether current tab has whitelisted domain
   if (excluded)
     chrome.contextMenus.removeAll();
   else
     showContextMenu();
 }
 
 /**
@@ skipping 272 matching lines @@
         // sendResponse asynchronously
         return true;
       }
       break;
     default:
       sendResponse({});
       break;
   }
 });
 
+// Set icon title/tooltip globally
+ext.browserAction.setTitle(ext.i18n.getMessage("name"));

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

If you take this route then there should also be a default image:

  ext.browserAction.setIcon("icons/abp-19.png");

Note however that for Chrome this is duplicating the defaults from manifest.json
(or metadata.chrome). Ideally, the browser action implementation would just get
the data from the metadata file. One way to implement that would be adding
corresponding values to safariInfo.js.tmpl in buildtools.

You probably won't have time to implement this right now, that's fine. We need
at least a TODO comment on that however.

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

I think it is fine as it is. the default icon is also defined in Info.plist on
Safari. The reason we have to set the title in the code, is to have it
translated.

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

The difference however is: Chrome will reset the icon automatically for a new
tab whereas Safari won't. Safari considers toolbar items to be per-window rather
than per-tab so any per-tab behavior needs to be defined explicitly.

Also, there is no reason to change title programmatically in Chrome - the title
is translated already. manifest.json specifies __MSG_name__ rather than an
explicit string. So the functionality you are working on is only required for
Safari and I see little reason to change behavior in Chrome.

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

Yes, but we backup the default (which comes from Info.plist) before setting any
per tab browser action. So it is restored, without having the default redundant
in the javascript code.
Yes, but it won't hurt to set the title programmatically on Chrome. In fact we
already did that before. The only thing I changed, is to set the title globally
instead of everytime for every tab.

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

Ok, I see - setting the image explicitly is indeed unnecessary.
No, setting the title programmatically won't hurt - as long as doing so doesn't
require adding one more class to the class hierarchy and introducing a concept
of global properties in addition to per-tab properties.

+
 // Show icon as browser action for all tabs that already exist
 ext.windows.getAll(function(windows)
 {
   for (var i = 0; i < windows.length; i++)
   {
     windows[i].getAllTabs(function(tabs)
     {
       tabs.forEach(refreshIconAndContextMenu);
     });
   }
 });
 
 // Update icon if a tab changes location
 ext.tabs.onLoading.addListener(function(tab)
 {
   tab.sendMessage({type: "clickhide-deactivate"});
   refreshIconAndContextMenu(tab);
 });
 
 setTimeout(function()
 {
   var notificationToShow = Notification.getNextToShow();
   if (notificationToShow)
     showNotification(notificationToShow);
 }, 3 * 60 * 1000);