Applied changes from emailed code review

lib/typedItCollector.js

 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 Cu.import("resource://gre/modules/Services.jsm");
 
 let {Prefs} = require("prefs");
 let {WindowObserver} = require("windowObserver");
 
+let DOMAIN_TYPED = 1;
+let DOMAIN_TYPO = 2;
+let DOMAIN_CORRECTION = 3;
+let DOMAIN_FALSE_POSITIVE = 4;
+
 let domains = null;
-let userCorrections = null;
-let falsePositives = null;
 let timer = null;
 
 // Initialize and make sure to react to pref changes
 if (Prefs.domainOptIn)
   startCollection();
 Prefs.addListener(function(name)
 {
   if (name != "domainOptIn")
     return;
   if (Prefs.domainOptIn)
@@ skipping 34 matching lines @@
   if (Prefs.counter < 5)
     Prefs.counter++;
   else if (!Prefs.domainOptInAsk && !Prefs.domainOptIn)
     window.openDialog("chrome://url-fixer/content/typedItOptIn.xul", "typedItOptIn", "chrome,dialog,centerscreen,titlebar");
 }
 
 exports.processTypedDomain = processTypedDomain;
 function processTypedDomain(domain)
 {
   if (domains && !privateBrowsingEnabled())
-    domains.push(domain);
+    domains[domain] = DOMAIN_TYPED;
+}
+
+exports.processDomainCorrection = processDomainCorrection;
+function processDomainCorrection(domainFrom, domainTo)
+{
+  if (domains && !privateBrowsingEnabled())
+  {
+    domains[domainFrom] = DOMAIN_TYPO;
+    domains[domainTo] = DOMAIN_CORRECTION;
+  }
 }
 
 exports.processFalsePositive = processFalsePositive;
 function processFalsePositive(domainFrom, domainTo)
 {
-  if (falsePositives && !privateBrowsingEnabled())
-  {
-    falsePositives.push([domainFrom, domainTo]);
+  if (domains && !privateBrowsingEnabled())
+  {
+    domains[domainFrom] = DOMAIN_FALSE_POSITIVE;
+    domains[domainTo] = DOMAIN_TYPED;
   }
 }
 
 exports.processUserCorrection = processUserCorrection;
 function processUserCorrection(domainFrom, domainTo)
 {
-  if (userCorrections && !privateBrowsingEnabled())
-  {
-    userCorrections.push([domainFrom, domainTo]);
+  if (domains && !privateBrowsingEnabled())
+  {
+    domains[domainFrom] = DOMAIN_TYPO;
+    domains[domainTo] = DOMAIN_CORRECTION;
   }
 }
 
 exports.openDisclosurePage = openDisclosurePage;
 function openDisclosurePage()
 {
   let window = Services.wm.getMostRecentWindow("navigator:browser");
   if (!window)
     return;
 
   let url = "http://urlfixer.org/data/";
   if ("Browser" in window && typeof window.Browser.addTab != 'undefined')
     window.Browser.addTab(url, true);
   else if ("gBrowser" in window)
     window.gBrowser.loadOneTab(url, {inBackground: false});
 }
 
 function startCollection()
 {
-  if (domains || falsePositives || userCorrections)
+  if (domains)
     return;
 
   onShutdown.add(stopCollection);
 
-  domains = [];
-  falsePositives = [];
-  userCorrections = [];
-
-  // Send data every 15 minutes
+  domains = {};
+
+  // Send data every 60 minutes
   timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
-  timer.initWithCallback(sendAnonymousData, 1000 * 60 * 15, Ci.nsITimer.TYPE_REPEATING_SLACK);
+  timer.initWithCallback(sendAnonymousData, 1000 * 60 * 60, Ci.nsITimer.TYPE_REPEATING_SLACK);
 }
 
 function stopCollection()
 {
-  if (!domains || !falsePositives || !userCorrections)
+  if (!domains)
     return;
 
   onShutdown.remove(stopCollection);
   domains = null;
-  falsePositives = null;
-  userCorrections = null;
 
   try
   {
     timer.cancel();
   }
   catch (e)
   {
     Cu.reportError(e);
   }
   timer = null;
 }
 
 function privateBrowsingEnabled()
 {
   if (!("service" in privateBrowsingEnabled))
     privateBrowsingEnabled.service = Cc["@mozilla.org/privatebrowsing;1"].getService(Ci.nsIPrivateBrowsingService);
 
   return privateBrowsingEnabled.service.privateBrowsingEnabled;
 }
 
 function sendAnonymousData()
 {
-  if (!Prefs.domainOptIn || (domains.length == 0 && falsePositives.length == 0 && userCorrections.length == 0) || privateBrowsingEnabled())
-    return;
-  
-  let postData = {};
+  if (!Prefs.domainOptIn || privateBrowsingEnabled())
+    return;
+
+  let postData = JSON.stringify(domains);
+  if (postData == JSON.stringify({}))
+    return;
+
   let savedDomains = domains;
-  let savedFalsePositives = falsePositives;
-  let savedUserCorrections = userCorrections;
-
-  if(domains.length > 0)
-  {
-    postData.domains = domains;

[Wladimir Palant, 2012/09/21 14:40:25]

Now we are using JSON but not its advantages - the server is still receiving
three lists of domains that it has to compare. As mentioned in previous review,
it would be better to store and send structured data: each entry should
correspond to one string the user typed into the URL bar. That entry should
contain what the user type in, if we autocorrected it - what we corrected it
into and whether the user marked it as a false positive.

-    domains = [];
-  }
-  if(falsePositives.length > 0)
-  {
-    postData.falsePositives = falsePositives;
-    falsePositives = [];
-  }
-  if(userCorrections.length > 0)
-  {
-    postData.userCorrections = userCorrections;
-    userCorrections = [];
-  }
-  
+  domains = {};
+
   let request = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].createInstance(Ci.nsIXMLHttpRequest);
   request.open("POST", "http://typed.it/submitData");
-  request.setRequestHeader("Content-Type", "application/x-www-form-urlencoded");
+  request.setRequestHeader("Content-Type", "application/json");
   request.addEventListener("load", function(event)
   {
     if (event.target.status != 200)
-    {
       domains = domains.concat(savedDomains);
-      falsePositives = falsePositives.concat(savedFalsePositives);
-      userCorrections = userCorrections.concat(savedUserCorrections);
-    }
   }, false);
-  request.send("data=" + JSON.stringify(postData));

[Wladimir Palant, 2012/09/21 14:40:25]

There is little point to send JSON data in pseudo-urlencoded format.
Content-Type header should be application/json and request body just the plain
JSON data.

Side-note: Please verify that Unicode data is correctly encoded as UTF-8 here (I
think it is).

+  request.send(postData);
 }
 
 function initWebUI(event)
 {
   if (Prefs.domainOptIn)
     return;
 
   let container = event.target;
   let source = container.ownerDocument.defaultView.location.hostname;
   if (!/(^|\.)urlfixer\.org$/.test(source))
@@ skipping 12 matching lines @@
 
     Prefs.domainOptInAsk = true;
     Prefs.domainOptIn = true;
     button.style.display = "none";
     message.style.display = "";
   }, false);
 
   message.style.display = "none";
   container.style.display = "";
 }