Added Safari Support

background.js

Index: background.js
===================================================================
--- a/background.js
+++ b/background.js
@@ -113,33 +113,33 @@
 // Adds or removes page action icon according to options.
 function refreshIconAndContextMenu(tab)
 {
-  // The tab could have been closed by the time this function is called
-  if(!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 = excluded ? "icons/abp-19-whitelisted.png" : "icons/abp-19.png";
+  var iconFilename;
 
-  if (activeNotification)
-    startIconAnimation(tab, 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";
   else
-    chrome.pageAction.setIcon({tabId: tab.id, path: iconFilename});
+    iconFilename = "icons/abp-18-whitelisted.png";
 
-  // Only show icon for pages we can influence (http: and https:)
-  if(/^https?:/.test(tab.url))
-  {
-    chrome.pageAction.setTitle({tabId: tab.id, title: "Adblock Plus"});
-    if ("shouldShowIcon" in localStorage && localStorage["shouldShowIcon"] == "false")
-      chrome.pageAction.hide(tab.id);
-    else
-      chrome.pageAction.show(tab.id);
+  tab.pageAction.setIcon(iconFilename);
+  tab.pageAction.setTitle("Adblock Plus");
 
-    // Set context menu status according to whether current tab has whitelisted domain
-    if (excluded)
+  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.

       chrome.contextMenus.removeAll();
-    else
-      showContextMenu();
-  }
+  else
+    showContextMenu();
 }
 
 /**
@@ -367,8 +367,8 @@
 
   function notifyUser()
   {
-    chrome.tabs.create({
-      url: chrome.extension.getURL("firstRun.html")
+    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.

+      win.openTab(ext.getURL("firstRun.html"));
     });
   }
 
@@ -402,11 +402,14 @@
 // 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)
+      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});
@@ -415,171 +418,139 @@
   });
 }
 
-/**
- * Opens Options window or focuses an existing one.
- * @param {Function} callback  function to be called with the window object of
- *                             the Options window
- */
 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.

 {
-  function findOptions(selectTab)
-  {
-    var views = chrome.extension.getViews({type: "tab"});
-    for (var i = 0; i < views.length; i++)
-      if ("startSubscriptionSelection" in views[i])
-        return views[i];
+  ext.windows.getLastFocused(function(win) {
+    win.getAllTabs(function(tabs) {
+      var optionsUrl = ext.getURL("options.html");
 
-    return null;
-  }
+      for (var i = 0; i < tabs.length; i++)
+        if (tabs[i].url == optionsUrl) {
+          tabs[i].activate();
+          if (callback)
+            callback(tabs[i]);
+          return;
+        }
 
-  function selectOptionsTab()
-  {
-    chrome.windows.getAll({populate: true}, function(windows)
-    {
-      var url = chrome.extension.getURL("options.html");
-      for (var i = 0; i < windows.length; i++)
-        for (var j = 0; j < windows[i].tabs.length; j++)
-          if (windows[i].tabs[j].url == url)
-            chrome.tabs.update(windows[i].tabs[j].id, {selected: true});
-    });
-  }
-
-  var view = findOptions();
-  if (view)
-  {
-    selectOptionsTab();
-    callback(view);
-  }
-  else
-  {
-    var onLoad = function()
-    {
-      var view = findOptions();
-      if (view)
-        callback(view);
-    };
-
-    chrome.tabs.create({url: chrome.extension.getURL("options.html")}, function(tab)
-    {
-      if (tab.status == "complete")
-        onLoad();
-      else
-      {
-        var id = tab.id;
-        var listener = function(tabId, changeInfo, tab)
-        {
-          if (tabId == id && changeInfo.status == "complete")
-          {
-            chrome.tabs.onUpdated.removeListener(listener);
-            onLoad();
-          }
-        };
-        chrome.tabs.onUpdated.addListener(listener);
-      }
-    });
-  }
-}
-
-var iconAnimationTimer = null;
-var animatedIconTab = null;
-
-function stopIconAnimation()
-{
-  if (!iconAnimationTimer)
-    return;
-
-  clearTimeout(iconAnimationTimer);
-  iconAnimationTimer = null;
-  animatedIconTab = null;
-}
-
-function loadImages(imageFiles, callback)
-{
-  var images = {};
-  var imagesLoaded = 0;
-  imageFiles.forEach(function(imageFile)
-  {
-    var image = new Image();
-    image.src = imageFile;
-    image.addEventListener("load", function()
-    {
-      images[imageFile] = image;
-      if (++imagesLoaded === imageFiles.length)
-        callback(images);
+      win.openTab(optionsUrl, callback && function(tab) {
+        tab.onCompleted.addListener(callback);
+      });
     });
   });
 }
 
-function startIconAnimation(tab, iconPath)
-{
-  stopIconAnimation();
-  animatedIconTab = tab;
+function IconAnimation() {
+    this._icons = new TabMap();
+    this._animatedTabs = new TabMap();
+    this._step = 0;
+};
+IconAnimation.prototype = {
+  update: function(severity) {
+    if (severity == this._severity)
+       return;
 
-  var severitySuffix = activeNotification.severity === "critical"
-      ? "critical" : "information";
-  var notificationIconPath = "icons/notification-" + severitySuffix + ".png";
-  var iconFiles = [iconPath, notificationIconPath];
-  loadImages(iconFiles, function(images)
-  {
-    var icon = images[iconPath];
-    var notificationIcon = images[notificationIconPath];
+    if (!this._severity)
+      this._start();
 
-    var canvas = document.createElement("canvas");
-    canvas.width = icon.width;
-    canvas.height = icon.height;
-    var context = canvas.getContext("2d");
+    this._severity = severity;
+  },
+  stop: function() {
+    clearInterval(this._interval);
 
-    var currentFrame = 0;
-    var frameOpacities = [0, 0.2, 0.4, 0.6, 0.8,
-                          1, 1, 1, 1, 1,
-                          0.8, 0.6, 0.4, 0.2, 0];
+    delete this._interval;
+    delete this._severity;
 
-    function animationStep()
-    {
-      var opacity = frameOpacities[currentFrame];
-      context.clearRect(0, 0, canvas.width, canvas.height);
-      context.globalAlpha = 1;
-      context.drawImage(icon, 0, 0);
-      context.globalAlpha = opacity;
-      context.drawImage(notificationIcon, 0, 0);
-      var imageData = context.getImageData(0, 0, canvas.width, canvas.height);
-      chrome.pageAction.setIcon({tabId: tab.id, imageData: imageData});
+    this._animatedTabs.clear();
+  },
+  registerTab: function(tab, icon) {
+    this._icons.set(tab, icon);
 
-      var interval;
-      currentFrame++;
-      if (currentFrame < frameOpacities.length)
-      {
-        var duration = 3000;
-        interval = duration / frameOpacities.length;
+    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);
+        });
       }
-      else
-      {
-        currentFrame = 0;
-        interval = 10000;
-      }
-      iconAnimationTimer = setTimeout(animationStep, interval);
+    });
+  },
+  _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);
     }
-    animationStep();
-  });
-}
+
+    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()
-  {
-    var tab = animatedIconTab;
-    stopIconAnimation();
+  activeNotification.onClicked = function() {
+    iconAnimation.stop();
     activeNotification = null;
-    refreshIconAndContextMenu(tab);
   };
 
-  chrome.windows.getLastFocused({populate: true}, function(window)
-  {
-    chrome.tabs.query({active: true, windowId: window.id}, function(tabs)
-    {
-      tabs.forEach(refreshIconAndContextMenu);
-    });
-  });
+  iconAnimation.update(activeNotification.severity);
 }
 
 function showNotification(notification)
@@ -599,84 +570,71 @@
 
 /**
  * This function is a hack - we only know the tabId and document URL for a
- * message but we need to know the frame ID. Try to find it in webRequest's
+ * message but we need to know the frame ID. Try to find it in webRequest"s
  * frame data.
  */
-function getFrameId(tabId, url)
+function getFrameId(tab, url)
 {
-  if (tabId in frames)
-  {
-    for (var f in frames[tabId])
-    {
-      if (getFrameUrl(tabId, f) == url)
-        return f;
-    }
-  }
+  for (var frameId in frames.get(tab))
+    if (getFrameUrl(tab, frameId) == url)
+      return frameId;
   return -1;
 }
 
-chrome.extension.onRequest.addListener(function(request, sender, sendResponse)
-{
-  switch (request.reqtype)
+ext.onMessage.addListener(function (msg, sender, sendResponse) {
+  switch (msg.type)
   {
     case "get-settings":
       var hostDomain = null;
       var selectors = null;
 
-      var tabId = -1;
-      var frameId = -1;
-      if (sender.tab)
-      {
-        tabId = sender.tab.id;
-        frameId = getFrameId(tabId, request.frameUrl);
-      }
+      var frameId = sender.tab ? getFrameId(sender.tab, msg.frameUrl) : -1;
+      var enabled = false;
 
-      var enabled = !isFrameWhitelisted(tabId, frameId, "DOCUMENT") && !isFrameWhitelisted(tabId, frameId, "ELEMHIDE");
-      if (enabled && request.selectors)
-      {
-        var noStyleRules = false;
-        var host = extractHostFromURL(request.frameUrl);
-        hostDomain = getBaseDomain(host);
-        for (var i = 0; i < noStyleRulesHosts.length; i++)
+      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)
         {
-          var noStyleHost = noStyleRulesHosts[i];
-          if (host == noStyleHost || (host.length > noStyleHost.length &&
-                                      host.substr(host.length - noStyleHost.length - 1) == "." + noStyleHost))
+          var noStyleRules = false;
+          var host = extractHostFromURL(msg.frameUrl);
+          hostDomain = getBaseDomain(host);
+          for (var i = 0; i < noStyleRulesHosts.length; i++)
           {
-            noStyleRules = true;
+            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)
+          selectors = ElemHide.getSelectorsForDomain(host, false);
+          if (noStyleRules)
           {
-            return !/\[style[\^\$]?=/.test(s);
-          });
+            selectors = selectors.filter(function(s)
+            {
+              return !/\[style[\^\$]?=/.test(s);
+            });
+          }
         }
       }
 
       sendResponse({enabled: enabled, hostDomain: hostDomain, selectors: selectors});
       break;
     case "should-collapse":
-      var tabId = -1;
-      var frameId = -1;
-      if (sender.tab)
-      {
-        tabId = sender.tab.id;
-        frameId = getFrameId(tabId, request.documentUrl);
-      }
+      var frameId = sender.tab ? getFrameId(sender.tab, msg.documentUrl) : -1;
 
-      if (isFrameWhitelisted(tabId, frameId, "DOCUMENT"))
+      if (isFrameWhitelisted(sender.tab, frameId, "DOCUMENT"))
       {
         sendResponse(false);
         break;
       }
 
-      var requestHost = extractHostFromURL(request.url);
-      var documentHost = extractHostFromURL(request.documentUrl);
+      var requestHost = extractHostFromURL(msg.url);
+      var documentHost = extractHostFromURL(msg.documentUrl);
       var thirdParty = isThirdParty(requestHost, documentHost);
-      var filter = defaultMatcher.matchesAny(request.url, request.type, documentHost, thirdParty);
+      var filter = defaultMatcher.matchesAny(msg.url, msg.mediatype, documentHost, thirdParty);
       if (filter instanceof BlockingFilter)
       {
         var collapse = filter.collapse;
@@ -697,20 +655,19 @@
       }
       break;
     case "add-filters":
-      if (request.filters && request.filters.length)
+      if (msg.filters && msg.filters.length)
       {
-        for (var i = 0; i < request.filters.length; i++)
-          FilterStorage.addFilter(Filter.fromText(request.filters[i]));
+        for (var i = 0; i < msg.filters.length; i++)
+          FilterStorage.addFilter(Filter.fromText(msg.filters[i]));
       }
       break;
     case "add-subscription":
-      openOptions(function(view)
-      {
-        view.startSubscriptionSelection(request.title, request.url);
+      openOptions(function(tab) {
+        tab.sendMessage(msg);
       });
       break;
     case "forward":
-      chrome.tabs.sendRequest(sender.tab.id, request.request, sendResponse);
+      tab.sendMessage(msg.payload, sendResponse);
       break;
     default:
       sendResponse({});
@@ -719,34 +676,17 @@
 });
 
 // Show icon as page action for all tabs that already exist
-chrome.windows.getAll({populate: true}, function(windows)
-{
+ext.windows.getAll(function(windows) {
   for (var i = 0; i < windows.length; i++)
-    for (var j = 0; j < windows[i].tabs.length; j++)
-      refreshIconAndContextMenu(windows[i].tabs[j]);
+    windows[i].getAllTabs(function(tabs) {
+      tabs.forEach(refreshIconAndContextMenu);
+    });
 });
 
 // Update icon if a tab changes location
-chrome.tabs.onUpdated.addListener(function(tabId, changeInfo, tab)
-{
-  chrome.tabs.sendRequest(tabId, {reqtype: "clickhide-deactivate"})
-  if(changeInfo.status == "loading")
-    refreshIconAndContextMenu(tab);
-});
-
-// Refresh icon when switching tabs or windows
-chrome.tabs.onActivated.addListener(function(activeInfo)
-{
-  refreshIconAndContextMenu(animatedIconTab);
-  chrome.tabs.get(activeInfo.tabId, refreshIconAndContextMenu);
-});
-chrome.windows.onFocusChanged.addListener(function(windowId)
-{
-  refreshIconAndContextMenu(animatedIconTab);
-  chrome.tabs.query({active: true, windowId: windowId}, function(tabs)
-  {
-    tabs.forEach(refreshIconAndContextMenu);
-  });
+ext.tabs.onBeforeNavigate.addListener(function(tab) {
+  tab.sendMessage({type: "clickhide-deactivate"});
+  refreshIconAndContextMenu(tab);
 });
 
 setTimeout(function()