Applied changes from emailed code review

lib/typoFixer.js

Index: lib/typoFixer.js
===================================================================
--- a/lib/typoFixer.js
+++ b/lib/typoFixer.js
@@ -143,8 +143,7 @@
 function correctURL(window, value)
 {
   let hasCorrection = false;
-
-  // Trim it
+  
   value = value.trim();
   if (value.length == 0)
     return null;
@@ -178,9 +177,9 @@
     return null;
 
   // Check manually entered corrections
-  if (Prefs.custom_replace[value])
+  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;
@@ -235,7 +234,7 @@
   let infobar = browser.getNotificationBox();
   let notif = infobar.getNotificationWithValue("url-fixer-infobar-askafter");
 
-  let [message, yes, no, cancel] = getAskAfterDialogTexts();
+  let [message, yes, no, cancel] = getInfobarTexts();
   message = message.replace(/\?1\?/g, prefix+domain);
 
   if (notif)
@@ -259,9 +258,12 @@
         callback:   function()
         {
           // No: Add to list of corrections (ignore)
-          // TODO: maybe find more appropriate place to store this information
-          Prefs.custom_replace[value] = value;
-          Prefs.custom_replace = JSON.parse(JSON.stringify(Prefs.custom_replace));
+          if (/^www\./.test(value))
+          {
+            value = value.substr(4);
+          }

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

Somebody reading this code now has to recognize that 4 is the length of the
string you looked for above; to verify correctness this somebody would have to
count the characters again and also check whether you made an off-by-one error
somewhere. This is bad for code maintainability.

How about value = value.replace(/^www\./, '') without any checks?

+          Prefs.whitelist[value] = value;
+          Prefs.whitelist = JSON.parse(JSON.stringify(Prefs.whitelist));

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

I understand why you wanted the whitelist to be a hash but why store the key as
value as well? Why not just assign true as the value?

 
           browser.loadURI(value);
           processFalsePositive(oldDomain, domain);
@@ -272,13 +274,13 @@
       message,
       "url-fixer-infobar-askafter",
       require("info").addonRoot + "icon64.png",
-      infobar.PRIORITY_INFO_LOW,
+      infobar.PRIORITY_INFO_HIGH,
       buttons
     );
     notif.persistence = 1;
   }
 
-  require("survey").incrementCorrectionsCounter(window);
+  require("survey").incrementCorrectionsCounter();
 
   // Consider the correction a second typed domain
   if (!isIPAddress(domain))
@@ -289,8 +291,9 @@
 
 let stringBundle = null;
 
-function getAskAfterDialogTexts()
+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 = [
@@ -300,6 +303,6 @@
     stringBundle.GetStringFromName("urlfixer.cancel")
   ];
 
-  getAskAfterDialogTexts = function() result;
-  return getAskAfterDialogTexts();
+  getInfobarTexts = function() result;
+  return getInfobarTexts();
 }