Applied changes from emailed code review

chrome/content/options.js

 /*
  * This Source Code is subject to the terms of the Mozilla Public License
  * version 2.0 (the "License"). You can obtain a copy of the License at
  * http://mozilla.org/MPL/2.0/.
  */
 
 Cu.import("resource://gre/modules/Services.jsm");
 
 let {Prefs} = require("prefs");
 Prefs.addListener(onPrefChange);
@@ skipping 29 matching lines @@
   let ruleListElement = E("custom_corrections");
   let whitelistElement = E("whitelist");
 
   // Remove existing list entries
   for (let i = ruleListElement.getRowCount() - 1; i >= 0; i--)
     ruleListElement.removeItemAt(i);
   for (let i = whitelistElement.getRowCount() - 1; i >= 0; i--)
     whitelistElement.removeItemAt(i);
 
   // Build a list of custom rules and sort it alphabetically
-  let customRules = Prefs.custom_replace;
+  let prefRulesList = Prefs.custom_replace;

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

Whitelist and custom rules are no longer mangled together, how about you also
separate the corresponding code here? Meaning: declare prefRulesList and
ruleList first, populate the latter, sort; then declare prefWhitelist and
whitelist, populate the latter, sort.

+  let prefWhitelist = Prefs.whitelist;
   let ruleList = [];
   let whitelist = [];
-  for (let searchString in customRules)
+  
+  for (let searchString in prefRulesList)
   {
-    if(searchString == customRules[searchString])
-    {
-      whitelist.push(searchString);
-    }
-    else
-    {
-      ruleList.push([searchString, customRules[searchString]]);
-    }
+    ruleList.push([searchString, prefRulesList[searchString]]);
+  }
+  for (let searchString in prefWhitelist)
+  {
+    whitelist.push(searchString);
   }
 
   ruleList.sort(function(a, b)
   {
     if (a[0] < b[0])
       return -1;
     else if (a[0] > b[0])
       return 1;
     else
       return 0;
@@ skipping 58 matching lines @@
   if (searchString.length == 0)
     return;
 
   Prefs.custom_replace[searchString] = replacement;
   Prefs.custom_replace = JSON.parse(JSON.stringify(Prefs.custom_replace));
 
   E("find").value = E("replace").value = "";
   E("find").oninput();
 }
 
-function removeRule(btn)
+function removeRule(btn, pref)

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

If you have _list as the attribute of the button then it would be consistent to
have _pref as an attribute as well instead of passing it as a parameter.

 {
   let list = E(btn.getAttribute("_list"));
   let items = list.selectedItems;
 
   for (let i = items.length - 1; i >= 0; i--)
   {
     let searchString = items[i].getAttribute("value");
-    delete Prefs.custom_replace[searchString];
+    delete Prefs[pref][searchString];
   }
-  Prefs.custom_replace = JSON.parse(JSON.stringify(Prefs.custom_replace));
+  Prefs[pref] = JSON.parse(JSON.stringify(Prefs[pref]));

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

It's too bad that we have to use that hack. This needs to be solved properly in
the prefs module...

 }