Added Safari Support

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 95 matching lines @@
 
   var result = defaultMatcher.matchesAny(url, type || "DOCUMENT", extractHostFromURL(parentUrl || url), false);
   return (result instanceof WhitelistFilter ? result : null);
 }
 
 var activeNotification = null;
 
 // Adds or removes page action icon according to options.
 function refreshIconAndContextMenu(tab)
 {
   if(!/^https?:/.test(tab.url))

[Felix Dahlke, 2013/10/24 16:30:34]

What if tab is null or undefined? Can that still happen?

[Sebastian Noack, 2013/10/25 09:55:03]

Did you had a look at the calling code? Maybe I missed something, but I think
refreshIconAndContextMenu() is always called with a tab.

     return;
 
-  var excluded = isWhitelisted(tab.url);
   var iconFilename;
-
-  if (window.safari || !excluded)

[Felix Dahlke, 2013/10/24 16:30:34]

Why do we always show the normal icon on Safari? Definitely worth a comment.

[Sebastian Noack, 2013/10/25 09:55:03]

First of all because of in Safari icons are global and not per page. Page
actions like in Chrome doesn't exist in Safari. Also the "whitelisted" icon is
more or less just a grayscale version of the regular icon. And because of icons
in safari are always grayscale, it would be hard to recognize it anyway. Sure, I
could add a comment about that.

-    iconFilename = "icons/abp-18.png";
+  if (require("info").platform == "safari")
+    // There is no grayscale version of the icon for whitelisted tabs
+    // when using Safari, because icons are grayscale already and icons
+    // aren't per tab in Safari.
+    iconFilename = "icons/abp-16.png"
   else
-    iconFilename = "icons/abp-18-whitelisted.png";
+  {
+    var excluded = isWhitelisted(tab.url);
+    iconFilename = excluded ? "icons/abp-19-whitelisted.png" : "icons/abp-19.png";
+  }
 
   tab.pageAction.setIcon(iconFilename);
-  tab.pageAction.setTitle("Adblock Plus");
+  tab.pageAction.setTitle(ext.i18n.getMessage("name"));
 
   iconAnimation.registerTab(tab, iconFilename);
 
   if (localStorage.shouldShowIcon == "false")
     tab.pageAction.hide();
   else
     tab.pageAction.show();
 
-  // Set context menu status according to whether current tab has whitelisted domain
-  if (excluded)
-    if (window.chrome) // TODO: Implement context menus for Safari

[Felix Dahlke, 2013/10/24 16:30:34]

The surrounding |if (excluded)| needs braces, otherwise the else below will be
for |if (window.chrome)|.

[Sebastian Noack, 2013/10/25 09:55:03]

Oh, you are right.

+  if (require("info").platform == "chromium") // TODO: Implement context menus for Safari
+    // Set context menu status according to whether current tab has whitelisted domain
+    if (excluded)
       chrome.contextMenus.removeAll();
-  else
-    showContextMenu();
+    else
+      showContextMenu();
 }
 
 /**
  * Old versions stored filter data in the localStorage object, this will import
  * it into FilterStorage properly.
  */
 function importOldData()
 {
   function addSubscription(url, title)
   {
@@ skipping 207 matching lines @@
     }
     else
       addAcceptable = false;
   }
 
   if (!addSubscription && !addAcceptable)
     return;
 
   function notifyUser()
   {
-    ext.windows.getLastFocused(function(win) {

[Felix Dahlke, 2013/10/24 16:30:34]

Opening braces always go on their own line, except for object literals, where
that won't work.

The same goes for all the code below, but it's easier if I only point this out
once I guess.

[Sebastian Noack, 2013/10/25 09:55:03]

I thought we go with Mozilla coding style?

[Felix Dahlke, 2013/10/25 14:43:08]

No, we have our own: https://adblockplus.org/en/coding-style

It's largely based on Mozilla's though. Personally not a fan of opening braces
on new lines in JS, but that's how it is.

+    ext.windows.getLastFocused(function(win)
+    {
       win.openTab(ext.getURL("firstRun.html"));
     });
   }
 
   if (addSubscription)
   {
     // Load subscriptions data
     var request = new XMLHttpRequest();
     request.open("GET", "subscriptions.xml");
     request.addEventListener("load", function()
@@ skipping 14 matching lines @@
     }, false);
     request.send(null);
   }
   else
     notifyUser();
 }
 
 // Set up context menu for user selection of elements to block
 function showContextMenu()
 {
-  if (!window.chrome) // TODO: Implement context menus for Safari
-    return;
-
   chrome.contextMenus.removeAll(function()
   {
     if(typeof localStorage["shouldShowBlockElementMenu"] == "string" && localStorage["shouldShowBlockElementMenu"] == "true")
     {
       chrome.contextMenus.create({"title": chrome.i18n.getMessage("block_element"), "contexts": ["image", "video", "audio"], "onclick": function(info, tab)
       {
         if(info.srcUrl)
             chrome.tabs.sendRequest(tab.id, {reqtype: "clickhide-new-filter", filter: info.srcUrl});
       }});
     }
   });
 }
 
+/**
+  * Opens options tab or focuses an existing one, within the last focused window.
+  * @param {Function} callback  function to be called with the
+                                Tab object of the options tab
+  */
 function openOptions(callback)

[Felix Dahlke, 2013/10/24 16:30:34]

Why remove the docs?

[Sebastian Noack, 2013/10/25 09:55:03]

The old docs are deprecated now (the callback is called with the tab, not its
window object, now), and I felt that they are unneeded, since the code below is
pretty simple and easy to understand. Should I add docs that describe the new
behavior of openOptions()?

[Felix Dahlke, 2013/11/10 01:07:00]

Yeah I guess it's fine, most functions here don't have docs.

[Wladimir Palant, 2013/11/13 07:16:27]

I would actually prefer keeping and updating the documentation. That's a JSDoc
comment and I think that we'll eventually generate the documentation for the
code here (like we do for the core code) - it really isn't about whether the
code is understandable but rather about the API we are defining here.

 {
-  ext.windows.getLastFocused(function(win) {
-    win.getAllTabs(function(tabs) {
+  ext.windows.getLastFocused(function(win)
+  {
+    win.getAllTabs(function(tabs)
+    {
       var optionsUrl = ext.getURL("options.html");
 
       for (var i = 0; i < tabs.length; i++)
-        if (tabs[i].url == optionsUrl) {
+      {
+        if (tabs[i].url == optionsUrl)
+        {
           tabs[i].activate();
           if (callback)
             callback(tabs[i]);
           return;
         }
-
-      win.openTab(optionsUrl, callback && function(tab) {
+      }
+
+      win.openTab(optionsUrl, callback && function(tab)
+      {
         tab.onCompleted.addListener(callback);
       });
     });
   });
 }
 
-function IconAnimation() {
-    this._icons = new TabMap();
-    this._animatedTabs = new TabMap();
-    this._step = 0;
-};
-IconAnimation.prototype = {
-  update: function(severity) {
-    if (severity == this._severity)
-       return;
-
-    if (!this._severity)
-      this._start();
-
-    this._severity = severity;
-  },
-  stop: function() {
-    clearInterval(this._interval);
-
-    delete this._interval;
-    delete this._severity;
-
-    this._animatedTabs.clear();
-  },
-  registerTab: function(tab, icon) {
-    this._icons.set(tab, icon);
-
-    if (this._animatedTabs.has(tab))
-      this._updateIcon(tab);
-  },
-  _start: function() {
-    this._interval = setInterval(function() {
-      this._getVisibleTabs(function(tabs) {
-        if (tabs.length == 0)
-          return;
-
-        for (var i = 0; i < tabs.length; i++)
-          this._animatedTabs.set(tabs[i], null);
-
-        var interval = setInterval(function() {
-          this._step++;
-          tabs.forEach(this._updateIcon.bind(this));

[Felix Dahlke, 2013/10/24 16:30:34]

Is the |bind(this)| actually necessary here?

[Sebastian Noack, 2013/10/25 09:55:03]

Yes, it is. _updateIcon() needs this._step and this._severity. And if I don't
bind it, "this" will be the window, and not the IconAnimation object.

-
-          if (this._step < 10)
-            return;
-
-          clearInterval(interval);
-          setTimeout(function() {
-            interval = setInterval(function() {
-              this._step--;
-              tabs.forEach(this._updateIcon.bind(this));
-
-              if (this._step > 0)
-                return;
-
-              clearInterval(interval);
-              this._animatedTabs.clear();
-            }.bind(this), 100);
-          }.bind(this), 1000);
-        }.bind(this), 100);
-      }.bind(this));
-    }.bind(this), 15000);
-  },
-  _getVisibleTabs: function(callback) {
-    ext.windows.getAll(function(windows) {
-      var tabs = [];
-      var n = windows.length;

[Felix Dahlke, 2013/10/24 16:30:34]

I'd love a more descriptive variable name here. Something like |visibleWindows|
maybe?

-
-      for (var i = 0; i < windows.length; i++) {
-        if (!windows[i].visible) {
-          if (tabs.length == --n)

[Felix Dahlke, 2013/10/24 16:30:34]

tabs.length will always be 0 here, won't it? In that case I'd prefer |if (--n ==
0)| for clarity.

Then again, it seems to me like this all this can be simplified. Comparing
tabs.length and n is, how I see it, the exit condition, so we might as well move
it after the loop. Once we do that, we don't need to count visible windows
anymore, so I guess this would do:

windows.forEach(function(window)
{
  if (window.visible)
    window.getActiveTab(tabs.push);
});
callback(tabs);

Or did I miss something?

[Sebastian Noack, 2013/10/25 09:55:03]

Yes it is. When writing the code I felt it would be more clear that way. But now
I think checking against 0 would actually more clear.
That wouldn't work. The only reason I'm using callbacks here is that they are
called asynchronously (on Chrome). So tabs would always be an empty array in
your case, as callback(tabs) is called before the active tabs were actually
retrieved.

So the callback passed to _getVisibleTabs must actually be called from within
the callback passed getActiveTab(), but only if we know that it was called for
the last active window, and tabs is fully populated.

-            callback(tabs);
-
-          continue;
-        }
-
-        windows[i].getActiveTab(function(tab) {
-          tabs.push(tab);
-
-          if (tabs.length == n)
-            callback(tabs);
-        });
-      }
-    });
-  },
-  _updateIcon: function(tab) {
-    var path = this._icons.get(tab);
-
-    if (!path)
-      return;
-
-    if (this._step > 0) {
-      var suffix = "-notification-" + this._severity;
-      
-      if (this._step < 10)
-        suffix += "-" + this._step;
-
-      path = path.replace(/(?=\..+$)/, suffix);
-    }
-
-    tab.pageAction.setIcon(path);
-  }
-};
-var iconAnimation = new IconAnimation();

[Felix Dahlke, 2013/10/24 16:30:34]

If there's only one instance of IconAnimation, I'd prefer a JavaScript module.
Might be worth it's own file actually.

-
 function prepareNotificationIconAndPopup()
 {
-  activeNotification.onClicked = function() {
+  activeNotification.onClicked = function()
+  {
     iconAnimation.stop();
     activeNotification = null;
   };
 
   iconAnimation.update(activeNotification.severity);
 }
 
 function showNotification(notification)
 {
   activeNotification = notification;
@@ skipping 15 matching lines @@
  * frame data.
  */
 function getFrameId(tab, url)
 {
   for (var frameId in frames.get(tab))
     if (getFrameUrl(tab, frameId) == url)
       return frameId;
   return -1;
 }
 
-ext.onMessage.addListener(function (msg, sender, sendResponse) {
+ext.onMessage.addListener(function (msg, sender, sendResponse)
+{
   switch (msg.type)
   {
-    case "get-settings":
-      var hostDomain = null;
+    case "get-selectors":
       var selectors = null;
-
       var frameId = sender.tab ? getFrameId(sender.tab, msg.frameUrl) : -1;
-      var enabled = false;
-
-      if (!isFrameWhitelisted(sender.tab, frameId, "DOCUMENT"))

[Felix Dahlke, 2013/10/24 16:30:34]

I prefer the old code (enabled variable) to nested ifs here.

-      if (!isFrameWhitelisted(sender.tab, frameId, "ELEMHIDE")) {
-        var enabled = true;
-
-        if (msg.selectors)
+
+      if (!isFrameWhitelisted(sender.tab, frameId, "DOCUMENT") &&
+          !isFrameWhitelisted(sender.tab, frameId, "ELEMHIDE"))
+      {
+        var noStyleRules = false;
+        var host = extractHostFromURL(msg.frameUrl);
+        for (var i = 0; i < noStyleRulesHosts.length; i++)
         {
-          var noStyleRules = false;
-          var host = extractHostFromURL(msg.frameUrl);
-          hostDomain = getBaseDomain(host);
-          for (var i = 0; i < noStyleRulesHosts.length; i++)
+          var noStyleHost = noStyleRulesHosts[i];
+          if (host == noStyleHost || (host.length > noStyleHost.length &&
+                                      host.substr(host.length - noStyleHost.length - 1) == "." + noStyleHost))
           {
-            var noStyleHost = noStyleRulesHosts[i];
-            if (host == noStyleHost || (host.length > noStyleHost.length &&
-                                        host.substr(host.length - noStyleHost.length - 1) == "." + noStyleHost))
-            {
-              noStyleRules = true;
-            }
-          }
-          selectors = ElemHide.getSelectorsForDomain(host, false);
-          if (noStyleRules)
-          {
-            selectors = selectors.filter(function(s)
-            {
-              return !/\[style[\^\$]?=/.test(s);
-            });
+            noStyleRules = true;
           }
         }
-      }
-
-      sendResponse({enabled: enabled, hostDomain: hostDomain, selectors: selectors});
+        selectors = ElemHide.getSelectorsForDomain(host, false);
+        if (noStyleRules)
+        {
+          selectors = selectors.filter(function(s)
+          {
+            return !/\[style[\^\$]?=/.test(s);
+          });
+        }
+      }
+
+      sendResponse(selectors);
       break;
     case "should-collapse":
       var frameId = sender.tab ? getFrameId(sender.tab, msg.documentUrl) : -1;
 
       if (isFrameWhitelisted(sender.tab, frameId, "DOCUMENT"))
       {
         sendResponse(false);
         break;
       }
 
@@ skipping 21 matching lines @@
       }
       break;
     case "add-filters":
       if (msg.filters && msg.filters.length)
       {
         for (var i = 0; i < msg.filters.length; i++)
           FilterStorage.addFilter(Filter.fromText(msg.filters[i]));
       }
       break;
     case "add-subscription":
-      openOptions(function(tab) {
+      openOptions(function(tab)
+      {
         tab.sendMessage(msg);
       });
       break;
     case "forward":
       tab.sendMessage(msg.payload, sendResponse);
       break;
     default:
       sendResponse({});
       break;
   }
 });
 
 // Show icon as page action for all tabs that already exist
-ext.windows.getAll(function(windows) {
+ext.windows.getAll(function(windows)
+{
   for (var i = 0; i < windows.length; i++)
-    windows[i].getAllTabs(function(tabs) {
+  {
+    windows[i].getAllTabs(function(tabs)
+    {
       tabs.forEach(refreshIconAndContextMenu);
     });
+  }
 });
 
 // Update icon if a tab changes location
-ext.tabs.onBeforeNavigate.addListener(function(tab) {
+ext.tabs.onBeforeNavigate.addListener(function(tab)
+{
   tab.sendMessage({type: "clickhide-deactivate"});
   refreshIconAndContextMenu(tab);
 });
 
 setTimeout(function()
 {
   var notificationToShow = Notification.getNextToShow();
   if (notificationToShow)
     showNotification(notificationToShow);
 }, 3 * 60 * 1000);