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 96 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))
     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 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";
+  var iconFilename;
+  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 += "19";
-
-    // 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";
+    iconFilename = excluded ? "icons/abp-19-whitelisted.png" : "icons/abp-19.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.

+  tab.pageAction.setTitle(ext.i18n.getMessage("name"));
 
   iconAnimation.registerTab(tab, iconFilename);
 
   if (localStorage.shouldShowIcon == "false")
     tab.pageAction.hide();
   else
     tab.pageAction.show();
 
-  if ("chrome" in window) // TODO: Implement context menus for Safari
+  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();
 }
 
 /**
  * Old versions stored filter data in the localStorage object, this will import
  * it into FilterStorage properly.
@@ skipping 212 matching lines @@
     }
     else
       addAcceptable = false;
   }
 
   if (!addSubscription && !addAcceptable)
     return;
 
   function notifyUser()
   {
-    ext.windows.getLastFocused(function(win) {

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

Nit: that bracket should be on the next line.

+    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 27 matching lines @@
     {
       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)
 {
   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?

   {
     win.getAllTabs(function(tabs)
     {
       var optionsUrl = ext.getURL("options.html");
 
       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;
         }
-
-      win.openTab(optionsUrl, callback && function(tab) {

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

Nit: this bracket should be on the next line.

+      }
+
+      win.openTab(optionsUrl, callback && function(tab)
+      {
         tab.onCompleted.addListener(callback);
       });
     });
   });
 }
 
 function prepareNotificationIconAndPopup()
 {
   activeNotification.onClicked = function()
   {
@@ skipping 29 matching lines @@
   for (var frameId in frames.get(tab))
     if (getFrameUrl(tab, frameId) == url)
       return frameId;
   return -1;
 }
 
 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"))
-      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)
+
+      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) { 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.

+      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)
 {
   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.

+  {
     windows[i].getAllTabs(function(tabs)
     {
       tabs.forEach(refreshIconAndContextMenu);
     });
+  }
 });
 
 // Update icon if a tab changes location
 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);