Added Safari Support

background.js

Index: background.js
===================================================================
--- a/background.js
+++ b/background.js
@@ -113,33 +113,41 @@
 // 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))
     return;

[Wladimir Palant, 2013/11/12 15:19:52]

Won't this make the icon get stuck in its previous state that likely doesn't
apply to the current tab contents?

[Sebastian Noack, 2013/11/12 16:56:15]

We can't control the pageAction on pages that aren't using either http or https
scheme. Chrome will hide/remove the icon automatically in that case. Otherwise
this would already be a bug in the existing code.

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

This isn't really true - we can show our icon on pages that use neither http nor
https. However, it seems that Chrome will always hide our icon on a new page
unless we enable it explicitly so this approach will work.

 
-  var excluded = isWhitelisted(tab.url);
-  var iconFilename = excluded ? "icons/abp-19-whitelisted.png" : "icons/abp-19.png";
+  var iconFilename = "icons/abp-";
+  if ("safari" in window)

[Wladimir Palant, 2013/11/12 15:19:52]

Why resort to browser sniffing? You should use require("info") here and wherever
else this approach is used, that's what we have it for.

[Sebastian Noack, 2013/11/12 16:56:15]

In this case I agree. But when checking for the availability of window.safari in
order to call a safari.* API, I think checking for presence of the safari API is
more accurate.

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

I disagree. Please use require("info") for any browser-specific branching. While
we aren't expecting Opera to implement any Safari-specific features any time
soon, finding browser-specific code will be very hard if a different branching
approach is used every time.

+    iconFilename += "16";
+  else
+  {
+    iconFilename += "19";
 
-  if (activeNotification)
-    startIconAnimation(tab, iconFilename);
+    // If the page is whitelisted use the grayscale version of the icon
+    // for that tab, except for Safari where all icons are grayscale
+    // already and where icons aren't per tab.
+    var excluded = isWhitelisted(tab.url);
+    if (excluded)
+      iconFilename += "-whitelisted";

[Wladimir Palant, 2013/11/12 15:19:52]

The logic here is a lot more opaque than it was originally. How about you keep
the original logic for the Chrome branch:

  iconFilename = excluded ? "icons/abp-19-whitelisted.png" : "icons/abp-19.png";

And the Safari branch can be simply:

  iconFilename = "icons/abp-16-whitelisted.png"

The comment here belongs into the Safari branch and should explain why we always
use the same icon there.

+  }
+  iconFilename += ".png";
+
+  tab.pageAction.setIcon(iconFilename);
+  tab.pageAction.setTitle("Adblock Plus");

[Wladimir Palant, 2013/11/12 15:19:52]

Ouch, this should be localized - i18n.getMessage("name")

[Sebastian Noack, 2013/11/12 16:56:15]

Yes, it should. However translation round already started and there is no string
"Adblock Plus" in messages.json, yet. Also note that it also wasn't translated
before.

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

There is - "name", imported from Firefox locales.

+
+  iconAnimation.registerTab(tab, iconFilename);
+
+  if (localStorage.shouldShowIcon == "false")
+    tab.pageAction.hide();
   else
-    chrome.pageAction.setIcon({tabId: tab.id, path: iconFilename});
+    tab.pageAction.show();
 
-  // 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);
-
+  if ("chrome" in window) // 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();
-  }
 }
 
 /**
@@ -367,8 +375,8 @@
 
   function notifyUser()
   {
-    chrome.tabs.create({
-      url: chrome.extension.getURL("firstRun.html")
+    ext.windows.getLastFocused(function(win) {

[Wladimir Palant, 2013/11/12 15:19:52]

Nit: that bracket should be on the next line.

+      win.openTab(ext.getURL("firstRun.html"));
     });
   }
 
@@ -406,7 +414,7 @@
   {
     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,151 +423,27 @@
   });
 }
 
-/**
- * Opens Options window or focuses an existing one.
- * @param {Function} callback  function to be called with the window object of
- *                             the Options window
- */

[Wladimir Palant, 2013/11/12 15:19:52]

Why remove that comment? I know that we don't use JSDoc comments consistently in
the adblockpluschrome repository but we should.

[Sebastian Noack, 2013/11/12 16:56:15]

See http://codereview.adblockplus.org/16067002/diff/1/background.js#newcode421

 function openOptions(callback)
 {
-  function findOptions(selectTab)
+  ext.windows.getLastFocused(function(win)

[Wladimir Palant, 2013/11/12 15:19:52]

Why restrict it to the current browser window? We should find it in background
windows as well and bring those up - that's what the original Chrome code did.

[Sebastian Noack, 2013/11/12 16:56:15]

You are right about that the behavior changed here. If you want I can change it
back, but I think the new behavior has a better user experience. Note that you
can't bring up background windows. You can only switch the active tab in the
window. So if the options page is already loaded in a tab in a not visible
window, and you attempt to open the options page, it seems that nothing happens.

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

No, the user experience is worse - users might unintentionally open multiple
options pages, in different browser windows. Of course the window has to be
activated when we do this - that's what Chrome is doing when opening options and
we were mimicking that functionality. In Safari you have
SafariBrowserWindow.activate() which seems to do the right thing.

[Sebastian Noack, 2013/11/13 09:33:12]

I disagree. When you click on the options link in the bubble of a given window,
you probably want to see the options in the window, where you clicked on the
options link. You don't want it to bring the window, where you opened for
example porn sites in and put into background for a reason ;), into foreground
again.
Interesting, I have experienced the opposite. When I click on "Options" in the
generic context menu of the icon, and there is already a tab in the same window
with the options in it, it will activate that tab. Otherwise it opens a new tab,
even if the options are already opened in a tab of an other window. But I
completely agree that we should mimic that behavior here.

[Felix Dahlke, 2013/11/13 12:00:16]

I'm with Wladimir here, what the browser does is what the users will expect us
to do. Chrome opens the HTML-based settings in a new tab, Firefox and Safari
open them in a new native dialog, but that's really not a big difference from
the user perspective. The important thing is that they don't mess with the page
they are currently on.
Yup, that's how it works, and IMO it makes more sense.

[Sebastian Noack, 2013/11/13 12:26:34]

What Wladimir suggested is not what the browser does.
But the new code mimics what chrome does, when it shows the options.
That's not true. We use the HTML options page for Safari, too.
So you agree with me?

   {
-    var views = chrome.extension.getViews({type: "tab"});
-    for (var i = 0; i < views.length; i++)
-      if ("startSubscriptionSelection" in views[i])
-        return views[i];
+    win.getAllTabs(function(tabs)
+    {
+      var optionsUrl = ext.getURL("options.html");
 
-    return null;
-  }
+      for (var i = 0; i < tabs.length; i++)

[Wladimir Palant, 2013/11/12 15:19:52]

Nit: please always put brackets around multi-line blocks even if syntactically
they aren't required, that makes it much easier to read the code.

+        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});
+      win.openTab(optionsUrl, callback && function(tab) {

[Wladimir Palant, 2013/11/12 15:19:52]

Nit: this bracket should be on the next line.

+        tab.onCompleted.addListener(callback);
+      });
     });
-  }
-
-  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);
-    });
-  });
-}
-
-function startIconAnimation(tab, iconPath)
-{
-  stopIconAnimation();
-  animatedIconTab = tab;
-
-  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];
-
-    var canvas = document.createElement("canvas");
-    canvas.width = icon.width;
-    canvas.height = icon.height;
-    var context = canvas.getContext("2d");
-
-    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];
-
-    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});
-
-      var interval;
-      currentFrame++;
-      if (currentFrame < frameOpacities.length)
-      {
-        var duration = 3000;
-        interval = duration / frameOpacities.length;
-      }
-      else
-      {
-        currentFrame = 0;
-        interval = 10000;
-      }
-      iconAnimationTimer = setTimeout(animationStep, interval);
-    }
-    animationStep();
   });
 }
 
@@ -567,19 +451,11 @@
 {
   activeNotification.onClicked = function()
   {
-    var tab = animatedIconTab;
-    stopIconAnimation();
+    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 +475,72 @@
 
 /**
  * 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)
+ext.onMessage.addListener(function (msg, sender, sendResponse)
 {
-  switch (request.reqtype)
+  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"))
+      if (!isFrameWhitelisted(sender.tab, frameId, "ELEMHIDE")) {

[Wladimir Palant, 2013/11/12 15:19:52]

As with other occasions, please use && here. In general, please always indent if
block body and wrap multi-line bodies in brackets.

+        var enabled = true;

[Wladimir Palant, 2013/11/12 15:19:52]

You are redeclaring the variable here.

Side-note: fortunately, our Chrome content scripts no longer care about the
"enabled" field of the response - it is being set wrong, the ELEMHIDE check only
applies to selectors. As it is now, we can rename the request into
"get-selectors".

+
+        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 +561,17 @@
       }
       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); });

[Wladimir Palant, 2013/11/12 15:19:52]

Please don't compress this into one line - use the same style as the original
code.

       break;
     case "forward":
-      chrome.tabs.sendRequest(sender.tab.id, request.request, sendResponse);
+      tab.sendMessage(msg.payload, sendResponse);
       break;
     default:
       sendResponse({});
@@ -719,34 +580,20 @@
 });
 
 // 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++)

[Wladimir Palant, 2013/11/12 15:19:52]

Nit: Please add brackets around the multi-line block body here.

-    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)
+ext.tabs.onBeforeNavigate.addListener(function(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);
-  });
+  tab.sendMessage({type: "clickhide-deactivate"});
+  refreshIconAndContextMenu(tab);
 });
 
 setTimeout(function()