Applied changes from emailed code review

lib/typoFixer.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");
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 
 let {Prefs} = require("prefs");
 let {WindowObserver} = require("windowObserver");
 let {getSchemeCorrection, isKnownScheme, getDomainCorrection, getDomainReferral} = require("rules");
-let {processTypedDomain} = require("typedItCollector");
-let {processUserCorrection} = require("typedItCollector");
-let {processFalsePositive} = require("typedItCollector");
+let {processTypedDomain, processDomainCorrection,
+     processUserCorrection, processFalsePositive} = require("typedItCollector");
 let appIntegration = require("appIntegration");
 
 // Attach our handlers to all browser windows
 new WindowObserver(
 {
   applyToWindow: function(window)
   {
     if (!appIntegration.isKnownWindow(window))
       return;
 
@@ skipping 63 matching lines @@
   let retryButton = document.getElementById("url-fixer-retry");
   retryButton.addEventListener("click", function()
   {
     let newURL = textField.value.replace(/^\s+/, "").replace(/\s+$/, "");
     if (!newURL.length)
       return;
 
     let [prefix, newHost, suffix] = parseURL(newURL);
     let oldHost = document.defaultView.location.hostname.toLowerCase();
 
-    processTypedDomain(newHost);
     processUserCorrection(oldHost, newHost);
 
     if (newHost.indexOf("www.") == 0 && oldHost.indexOf("www.") == 0)
     {
       // Ignore www. prefix if they both start with it
       newHost = newHost.substr(4);
       oldHost = oldHost.substr(4);
     }
     if (oldHost && newHost != oldHost)
     {
@@ skipping 28 matching lines @@
   }
   catch (e)
   {
     return (e.result == Cr.NS_ERROR_HOST_IS_IP_ADDRESS);
   }
 }
 
 function correctURL(window, value)
 {
   let hasCorrection = false;
-  
+
   value = value.trim();
   if (value.length == 0)
     return null;
 
   // Replace backslashes
   value = value.replace(/\\/g, "/");
 
   // Does the URL scheme need correcting?
   if (/^([^\/]+)(\/.*)/.test(value))
   {
@@ skipping 15 matching lines @@
   if ("getShortcutOrURI" in window && window.getShortcutOrURI(value) != value)
     return null;
 
   // Spaces before the first slash or period is probably a quick search
   if (/^[^\/\.\s]+\s/.test(value))
     return null;
 
   // Check manually entered corrections
   if (Prefs.custom_replace.hasOwnProperty(value) && Prefs.custom_replace[value])
     return Prefs.custom_replace[value];
-  
+
   let [prefix, domain, suffix] = parseURL(value);
   if (!domain)
     return null;
 
   let oldDomain = domain;
   if (!isIPAddress(domain))
   {
     processTypedDomain(domain);
 
     let newDomain = getDomainCorrection(domain);
     if (newDomain != domain)
     {
+      processDomainCorrection(domain, newDomain);
       domain = newDomain;
       hasCorrection = true;
 
       let referral = getDomainReferral(domain.replace(/^www\./, ""));
       if (referral)
       {
         // We need to add a query string parameter when sending users to this domain
         let anchorIndex = suffix.indexOf("#");
         let anchor = "";
         if (anchorIndex >= 0)
@@ skipping 18 matching lines @@
 
         suffix += anchor;
       }
     }
   }
 
   if (!hasCorrection)
     return null;
 
   // Show infobar to inform and ask about correction
-  let [message, yes, no, cancel] = getInfobarTexts();

[Wladimir Palant, 2012/09/28 09:29:34]

You don't use a Cancel button - please remove the variable and the corresponding
strings.

+  let [message, yes, no] = getInfobarTexts();
   message = message.replace(/\?1\?/g, prefix+domain);
   let buttons = [
     {
       label:      yes,
       accessKey:  null,
       callback:   function()
       {
         // Yes: Do nothing
       }
     },
     {
       label:      no,
       accessKey:  null,
       callback:   function()
       {
         // No: Add to list of corrections (ignore)
-        if (/^www\./.test(value))
-        {
-          value = value.substr(4);
-        }
-        Prefs.whitelist[value] = value;

[Wladimir Palant, 2012/09/28 09:29:34]

Comments from previous review not addressed here - one of my changes replaced
the code here however.

+        let {onWhitelistEntryAdded} = require("rules");
+        let entry = oldDomain.replace(/^www\./, "");
+        Prefs.whitelist[entry] = true;
+        onWhitelistEntryAdded(entry);
         Prefs.whitelist = JSON.parse(JSON.stringify(Prefs.whitelist));
 
-        require("appIntegration").loadURI(value);
-        processFalsePositive(oldDomain, domain);
+        require("appIntegration").loadURI(window, value);
+        processFalsePositive(domain, oldDomain);
       }
     }
   ];
+  // We need to have persistence being set to 1 due to redirect which happens afterwards
   require("appIntegration").openInfobar(window, "url-fixer-infobar-askafter", message, buttons, 1);

[Wladimir Palant, 2012/09/28 09:29:34]

It isn't obvious why we pass 1 for persistence here, I had to look it up myself.
Reason is that we are about to load a different page which will change location
- so the message needs to survive one location change. Maybe add a comment
explaining that?

 
   require("survey").incrementCorrectionsCounter();
-
-  // Consider the correction a second typed domain
-  if (!isIPAddress(domain))
-    processTypedDomain(domain);
 
   return prefix + domain + suffix;
 }
 
 let stringBundle = null;
 
 function getInfobarTexts()
 {
   // Randomize URI to work around bug 719376
   if (!stringBundle)
     stringBundle = Services.strings.createBundle("chrome://url-fixer/locale/locale.properties?" + Math.random());
   let result = [
     stringBundle.GetStringFromName("urlfixer.isItCorrect"),
     stringBundle.GetStringFromName("urlfixer.yes"),
-    stringBundle.GetStringFromName("urlfixer.no"),
-    stringBundle.GetStringFromName("urlfixer.cancel")
+    stringBundle.GetStringFromName("urlfixer.no")
   ];
 
   getInfobarTexts = function() result;
   return getInfobarTexts();
 }