Applied changes from emailed code review

lib/survey.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 surveyLang = null;
-let surveyUrl = "http://urlfixer.org"; //"http://adblockplus.org/usersurvey/index.php?sid=68316";
+let surveyUrl = null; //"http://urlfixer.org/usersurvey/...";
 
 exports.incrementCorrectionsCounter = incrementCorrectionsCounter;
 function incrementCorrectionsCounter()
 {
+  // Only if survey exists
+  if (!surveyUrl)
+    return;
+  
   // Only users with 5 URL corrections
   if (++Prefs.corrections_count == 5)
   {
     initSurvey();
   }
 }
 
 function initSurvey()
 {
   // Don't ask after 2012-10-15
@@ skipping 17 matching lines @@
   // Delay survey question by 5 seconds
   surveyTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
   surveyTimer.initWithCallback(runSurvey, 5000, Ci.nsITimer.TYPE_ONE_SHOT);
 }
 
 function runSurvey()
 {
   //open panel
   let request = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].createInstance(Ci.nsIXMLHttpRequest);
   request.open("GET", "chrome://url-fixer/content/survey.xul");
-  request.setRequestHeader("Content-Type", "text/html");

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

What's that good for in a non-network request? Remove this line?

   request.addEventListener("load", function(event)
   {
     let window = Services.wm.getMostRecentWindow("navigator:browser");
     let document = window.document;
 
-    let style = document.createProcessingInstruction('xml-stylesheet', 'href="chrome://url-fixer/skin/survey.css?' + (new Date().getTime()) + '" type="text/css"');

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

Why randomize the URL here? It's a static file, not going to change.

+    let style = document.createProcessingInstruction('xml-stylesheet', 'href="chrome://url-fixer/skin/survey.css" type="text/css"');
     document.insertBefore(style, document.firstChild);
 
     let panel = new window.DOMParser().parseFromString(request.responseText, "text/xml").documentElement;
-    if (!document.getElementById(panel.id))
+    let oldPanel = document.getElementById(panel.id);
+    if (oldPanel)
+      oldPanel.parentNode.removeChild(oldPanel);
+    document.getElementById("mainPopupSet").appendChild(panel);
+    document.getElementById("url-fixer-icon").setAttribute("src", require("info").addonRoot + "icon64.png");
+    document.getElementById("url-fixer-accept-button").addEventListener("command", function()
     {
-      document.getElementById("mainPopupSet").appendChild(panel);
-    }
-    else
+      openSurvey();
+      panel.hidePopup();
+    });
+    document.getElementById("url-fixer-cancel-button").addEventListener("command", function()
     {
-      panel = document.getElementById(panel.id);

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

Our panel is already in the browser document at this point? This is a sign of
big issues - probably a panel from the previous version that wasn't removed for
some reason. We should not try to reuse it, it might not be compatible with
current code. If anything, we should remove it from the document.

-    }
-    panel.querySelector("#icon").setAttribute("src", require("info").addonRoot + "icon64.png");

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

If you use unique IDs then you can use document.getElementById() instead of
querySelector().

-    panel.querySelector("#accept-button").addEventListener("click", function()

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

Don't listen to "click" events, these are mouse-only in XUL. There is a
"command" event that deals with all kinds of input.

-      {

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

This bracket should not be indented...

-        openSurvey();
-        panel.hidePopup();
-      }
-    );
-    panel.querySelector("#cancel-button").addEventListener("click", function()
-      {

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

This bracket should not be indented...

-        panel.hidePopup();
-      }
-    );
+      panel.hidePopup();
+    });
+    panel.addEventListener("popuphidden", function()
+    {
+      panel.parentNode.removeChild(panel);
+      style.parentNode.removeChild(style);
+    });
 
     let anchor = document.getElementById("identity-box");
     panel.openPopup(anchor, "after_start", 0, 0, false, true);
   }, false);
   request.send(null);
 }
 
 function openSurvey()
 {
-  require("appIntegration").getBrowser(Services.wm.getMostRecentWindow("navigator:browser")).loadOneTab(surveyUrl + "&lang=" + surveyLang, {

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

Taking over comment from previous review: line too long and result of
getMostRecentWindow() unchecked.

-      referrerURI: Services.io.newURI("http://url.fixer/", null, null),
-      inBackground: false
-  });
+  let window = Services.wm.getMostRecentWindow("navigator:browser");
+  if (window)
+  {
+    let browser = require("appIntegration").getBrowser(window);
+    browser.loadOneTab(surveyUrl + "&lang=" + surveyLang, {
+        referrerURI: Services.io.newURI("http://url.fixer/", null, null),
+        inBackground: false
+    });
+  }
 }