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 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 prefRulesList)
   {
     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;
   });
   whitelist.sort();
 
   // Add the rules to the list
-  if(ruleList.length > 0)
+  if (ruleList.length > 0)
   {
     for (let i = 0; i < ruleList.length; i++)
     {
       let [searchString, replacement] = ruleList[i];
 
       let option = document.createElement('listitem');
       option.setAttribute("value", searchString);
 
       let cell1 = document.createElement('listcell');
       cell1.setAttribute("label", searchString);
       option.appendChild(cell1);
 
       let cell2 = document.createElement('listcell');
       cell2.setAttribute("label", replacement);
       option.appendChild(cell2);
 
       ruleListElement.appendChild(option);
     }
   }
   else
   {
     let option = document.createElement("listitem");
     option.setAttribute("class", "auto-entry");
     option.setAttribute("label", ruleListElement.getAttribute("_emptyLabel"));
 
     ruleListElement.appendChild(option);
   }
 
-  if(whitelist.length > 0)
+  if (whitelist.length > 0)
   {
     for (let i = 0; i < whitelist.length; i++)
     {
       let option = document.createElement("listitem");
       option.setAttribute("value", whitelist[i]);
       option.setAttribute("label", whitelist[i])
 
       whitelistElement.appendChild(option);
     }
   }
@@ skipping 14 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, 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;
 
+  let {onWhitelistEntryRemoved} = require("rules");
+
   for (let i = items.length - 1; i >= 0; i--)
   {
     let searchString = items[i].getAttribute("value");
     delete Prefs[pref][searchString];
+
+    if (pref == "whitelist")
+      onWhitelistEntryRemoved(searchString);
   }
   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...

 }