Added ad counting functionality

stats.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
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 (function()
 {
   var backgroundPage = chrome.extension.getBackgroundPage();
   var require = backgroundPage.require;
-  var ChromeCompat = backgroundPage.ChromeCompat;
-  var stats = require("stats");
-  var StatsObject = stats.StatsObject;
-  var Stats = stats.Stats;
+  var getStats = require("stats").getStats;
   var FilterNotifier = require("filterNotifier").FilterNotifier;
   
   var currentTabId;
-  var statsPage;
-  var statsTotal;
-  var shareBox;
-  var shareURL = "https://adblockplus.org";

[Wladimir Palant, 2013/09/18 13:22:17]

Add a slash at the end of the URL?

-  var redirectURL = "https://www.facebook.com";

[Wladimir Palant, 2013/09/18 13:22:17]

Given that this is Facebook-specific, I don't think that a global variable is
necessary - just add it to the shareLinks variable.

+  var shareURL = "https://adblockplus.org/";
   
-  function createShareLink(url, params)
+  var messageMark = {};
+  var shareLinks = {
+    facebook: ["https://www.facebook.com/dialog/feed", {
+      app_id: 475542399197328,

[Wladimir Palant, 2013/09/24 12:43:36]

Just realized that this by far exceeds the 32 bit integer range meaning that
JavaScript will store this as a floating point number internally. How about
using a string to ensure that there cannot be any precision loss? Right now this
identifier is only one digit too short to suffer from precision loss in Firefox.

+      link: shareURL,
+      redirect_uri: "https://www.facebook.com/",
+      ref: "adcounter",
+      name: messageMark,
+      actions: JSON.stringify([
+        {
+          name: i18n.getMessage("stats_share_download"),
+          link: shareURL
+        }
+      ])
+    }],
+    gplus: ["https://plus.google.com/share", {
+      url: shareURL
+    }],
+    twitter: ["https://twitter.com/intent/tweet", {
+      text: messageMark,
+      url: shareURL,
+      via: "AdblockPlus"
+    }]
+  };
+  
+  function createShareLink(network, blockedCount)
   {
+    var url = shareLinks[network][0];
+    var params = shareLinks[network][1];
+    
     var querystring = [];
     for (var key in params)
-      querystring.push(key + "=" + encodeURIComponent(params[key]));

[Wladimir Palant, 2013/09/18 13:22:17]

encodeURIComponent(key) please, don't assume that it doesn't need to be encoded.

+    {
+      var value = params[key];
+      if (value == messageMark)
+        value = i18n.getMessage("stats_share_message", blockedCount);
+      querystring.push(encodeURIComponent(key) + "=" + encodeURIComponent(value));
+    }
     return url + "?" + querystring.join("&");
   }
   
-  var shareLinks = {
-    facebook: createShareLink("https://www.facebook.com/dialog/feed", {
-      app_id: 475542399197328,
-      link: shareURL,
-      redirect_uri: redirectURL,
-      ref: "adcounter",
-      name: "_MESSAGE_",
-      actions: JSON.stringify([
-        {
-          name: chrome.i18n.getMessage("stats_share_download"),
-          link: shareURL
-        }
-      ])
-    }),
-    gplus: createShareLink("https://plus.google.com/share", {
-      url: shareURL
-    }),
-    twitter: createShareLink("https://twitter.com/intent/tweet", {
-      text: "_MESSAGE_",
-      url: shareURL,
-      via: "AdblockPlus"
-    })
-  };
-  
   function onLoad()
   {
-    statsPage = document.querySelector("#statsPage > strong");
-    statsTotal = document.querySelector("#statsTotal > strong");
+    document.getElementById("shareBox").addEventListener("click", share, false);
+    document.getElementById("share").addEventListener("click", toggleShareBox, false);
     
-    shareBox = document.getElementById("shareBox");
-    var shareBoxLink = document.getElementById("share");
-    shareBoxLink.addEventListener("click", showShareBox, false);
-    
-    var shareActions = document.querySelectorAll("#shareBox > a");
-    for (var i = 0; i < shareActions.length; i++)
-      shareActions[i].addEventListener("click", share, false);

[Wladimir Palant, 2013/09/18 13:22:17]

shareBox.addEventListener("click", share, false) will have exactly the same
effect without hardcoding the page structure - events bubble up.

-    
-    // update stats
-    statsTotal.innerText = formatNumber(Stats.total.blocked);
-    chrome.tabs.query({active: true}, function(tabs)

[Wladimir Palant, 2013/09/18 13:22:17]

You need windowId: chrome.windows.WINDOW_ID_CURRENT as well, otherwise you might
get a tab from a different window. Note that currentWindow property isn't
supported before Chrome 19.

+    // Update stats
+    chrome.tabs.query({
+      active: true,
+      windowId: chrome.windows.WINDOW_ID_CURRENT
+    }, function(tabs)
     {
       if (tabs.length > 0)
       {
         currentTabId = tabs[0].id;
         updateStats();
         
         FilterNotifier.addListener(onNotify);
         
-        document.getElementById("statsStuff").removeAttribute("hidden");
+        document.getElementById("statsContainer").removeAttribute("hidden");
       }

[Wladimir Palant, 2013/09/24 12:43:36]

How about |else window.close()| here just in case? It's not like we should ever
run into that case but if we do we won't display a broken UI.

[Thomas Greiner, 2013/09/24 14:40:58]

Closing the popup right after a user clicked on the icon to open it is not a
good idea. The user should still be able to use the other features that are
available in the popup.
Therefore I'd rather leave it like that and not show the stats container in that
case.

[Wladimir Palant, 2013/09/25 08:44:51]

You are right of course. I forgot that the stats are hidden by default.

     });
   }
   
   function onUnload()
   {
     FilterNotifier.removeListener(onNotify);
   }
   
   function onNotify(action, item)
   {
-    if (action == "document.stats")
+    if (action == "filter.hitCount")
       updateStats();
   }
   
   function updateStats()
   {
-    ChromeCompat.tabs.sendMessage(currentTabId, {reqtype: "get-stats"}, function(response)
-    {
-      var page = new StatsObject(response);
-      statsPage.innerText = formatNumber(page.blocked);
-      statsTotal.innerText = formatNumber(Stats.total.blocked);

[Wladimir Palant, 2013/09/18 13:22:17]

So that's why we have placeholders, so that we can later go into the text and
assume that the placeholder was inside the <strong> block :)

I guess that the best approach would be extending public functionality in
i18n.js, e.g. but adding a public setElementText() function:

function setElementText(element, stringName, arguments)
{
  function processString(...)
  {
    ...
  }

  while (element.lastChild)
    element.removeChild(element.lastChild);
  processString(i18n.getMessage(stringName, arguments), element);
}

loadI18nStrings() could then call this function - and you could as well,
whenever you need it.

-    });
+    var statsPage = document.getElementById("statsPage");
+    var blockedPage = getStats("blocked", currentTabId).toLocaleString();
+    i18n.setElementText(statsPage, "stats_label_page", [blockedPage]);
+    
+    var statsTotal = document.getElementById("statsTotal");
+    var blockedTotal = getStats("blocked").toLocaleString();
+    i18n.setElementText(statsTotal, "stats_label_total", [blockedTotal]);
   }
   
-  function showShareBox(ev)
+  function toggleShareBox(ev)
   {
-    if (shareBox.hasAttribute("hidden"))
-      shareBox.removeAttribute("hidden");
-    else
-      shareBox.setAttribute("hidden", true);

[Wladimir Palant, 2013/09/18 13:22:17]

How about:

var shareBox = document.getElementById("shareBox");
shareBox.hidden = !shareBox.hidden;

The name should probably be toggleShareBox() to reflect the functionality. Note
that I'm opposed to caching all elements in global variables, it's typically
unnecessary (doesn't save any code and is irrelevant performance-wise) and makes
the code harder to read.

+    var shareBox = document.getElementById("shareBox");
+    shareBox.hidden = !shareBox.hidden;
   }
   
   function share(ev)
   {
     // Easter Egg
-    var blocked = Stats.total.blocked;
+    var blocked = getStats("blocked");
     if (blocked <= 9000 || blocked >= 10000)
-      blocked = formatNumber(blocked);
+      blocked = blocked.toLocaleString();
     else
-      blocked = chrome.i18n.getMessage("stats_over", formatNumber(9000));
+      blocked = i18n.getMessage("stats_over", (9000).toLocaleString());
     
-    var url = shareLinks[ev.target.dataset.social]
-      .replace("_MESSAGE_", chrome.i18n.getMessage("stats_share_message", blocked));

[Wladimir Palant, 2013/09/18 13:22:17]

This is a rather ugly approach, and you forgot to URL-encode the message here.
How about generating the links here rather then up front?. Then
createShareLink() could insert the message during link generation:

var messageMark = {};
var shareLinks = {
  facebook: ["http://...", {
    ...
    name: messageMark,
    ...
  }]
};

function createShareLink(network, blockedCount)
{
  var url = shareLinks[network][0];
  var params = shareLinks[network][1];
  ...
  var value = params[key];
  if (value == messageMark)
    value = i18n.getMessage("stats_share_message", blockedCount);
  ...
}

-    
+    var url = createShareLink(ev.target.dataset.social, blocked);
     chrome.tabs.create({url: url});
-  }
-  
-  var locale = require("utils").Utils.appLocale;
-  function formatNumber(number)
-  {
-    return number.toLocaleString(locale);
   }

[Wladimir Palant, 2013/09/18 13:22:17]

Utils.appLocale returns the browser's locale. So this code would only make sense
if toLocaleString() uses the system locale by default and not the browser
locale. In my tests this doesn't seem to be the case. In other words,
formatNumber(number) does exactly the same as number.toLocaleString().

   
   document.addEventListener("DOMContentLoaded", onLoad, false);
   window.addEventListener("unload", onUnload, false);
 })();