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 win = null;
 let surveyLang = null;
 let surveyUrl = "http://urlfixer.org"; //"http://adblockplus.org/usersurvey/index.php?sid=68316";
 
-let langData = {
-  en: {
-    title: "Tell us your opinion",
-    question: "We would like to ask you a few questions about URL Fixer to help us improve it. If you can spare 5 minutes please click the button below to take the survey.",
-    note: "This is a one-time message and will not appear again.",
-    accept: "Take the survey",
-    decline: "Maybe some other time"
-  },
-  de: {
-    title: "Sagen Sie uns Ihre Meinung",
-    question: "Wir w\xFCrden Ihnen gerne einige Fragen zu URL Fixer stellen, um es verbessern zu k\xF6nnen. Falls Sie gerade 5 Minuten haben, dr\xFCcken Sie bitte die Taste unten, um an der Nutzerumfrage teilzunehmen.",
-    note: "Das ist eine einmalige Nachricht, die nicht wieder erscheinen wird.",
-    accept: "An der Umfrage teilnehmen",
-    decline: "Vielleicht ein anderes Mal"
-  },
-  ru: {
-    title: decodeURIComponent("%D0%9F%D0%BE%D0%B4%D0%B5%D0%BB%D0%B8%D1%82%D0%B5%D1%81%D1%8C%20%D1%81%20%D0%BD%D0%B0%D0%BC%D0%B8%20%D1%81%D0%B2%D0%BE%D0%B8%D0%BC%20%D0%BC%D0%BD%D0%B5%D0%BD%D0%B8%D0%B5%D0%BC"),
-    question: decodeURIComponent("%D0%9C%D1%8B%20%D1%85%D0%BE%D1%82%D0%B5%D0%BB%D0%B8%20%D0%B1%D1%8B%20%D0%B7%D0%B0%D0%B4%D0%B0%D1%82%D1%8C%20%D0%B2%D0%B0%D0%BC%20%D0%BD%D0%B5%D0%BA%D0%BE%D1%82%D0%BE%D1%80%D1%8B%D0%B5%20%D0%B2%D0%BE%D0%BF%D1%80%D0%BE%D1%81%D1%8B%20%D0%BE%D0%B1%20URL%20Fixer%2C%20%D1%87%D1%82%D0%BE%D0%B1%D1%8B%20%D0%BB%D1%83%D1%87%D1%88%D0%B5%20%D0%BE%D0%BF%D1%80%D0%B5%D0%B4%D0%B5%D0%BB%D0%B8%D1%82%D1%8C%20%D0%BD%D0%B0%D0%BF%D1%80%D0%B0%D0%B2%D0%BB%D0%B5%D0%BD%D0%B8%D0%B5%20%D0%B4%D0%BB%D1%8F%20%D0%B5%D0%B3%D0%BE%20%D0%B4%D0%B0%D0%BB%D1%8C%D0%BD%D0%B5%D0%B9%D1%88%D0%B5%D0%B3%D0%BE%20%D1%80%D0%B0%D0%B7%D0%B2%D0%B8%D1%82%D0%B8%D1%8F.%20%D0%95%D1%81%D0%BB%D0%B8%20%D1%83%20%D0%B2%D0%B0%D1%81%20%D0%B5%D1%81%D1%82%D1%8C%20%D1%81%D0%B2%D0%BE%D0%B1%D0%BE%D0%B4%D0%BD%D1%8B%D0%B5%205%20%D0%BC%D0%B8%D0%BD%D1%83%D1%82%2C%20%D1%82%D0%BE%20%D0%BD%D0%B0%D0%B6%D0%BC%D0%B8%D1%82%D0%B5%2C%20%D0%BF%D0%BE%D0%B6%D0%B0%D0%BB%D1%83%D0%B9%D1%81%D1%82%D0%B0%2C%20%D0%BD%D0%B0%20%D0%BA%D0%BD%D0%BE%D0%BF%D0%BA%D1%83%2C%20%D1%87%D1%82%D0%BE%D0%B1%D1%8B%20%D0%BF%D1%80%D0%B8%D0%BD%D1%8F%D1%82%D1%8C%20%D1%83%D1%87%D0%B0%D1%81%D1%82%D0%B8%D0%B5%20%D0%B2%20%D0%BE%D0%BF%D1%80%D0%BE%D1%81%D0%B5."),
-    note: decodeURIComponent("%D0%AD%D1%82%D0%BE%20%D0%BE%D0%B4%D0%BD%D0%BE%D1%80%D0%B0%D0%B7%D0%BE%D0%B2%D0%BE%D0%B5%20%D1%81%D0%BE%D0%BE%D0%B1%D1%89%D0%B5%D0%BD%D0%B8%D0%B5%2C%20%D0%BE%D0%BD%D0%BE%20%D0%B1%D0%BE%D0%BB%D1%8C%D1%88%D0%B5%20%D0%BD%D0%B5%20%D0%B1%D1%83%D0%B4%D0%B5%D1%82%20%D0%BF%D0%BE%D0%BA%D0%B0%D0%B7%D1%8B%D0%B2%D0%B0%D1%82%D1%8C%D1%81%D1%8F."),
-    accept: decodeURIComponent("%D0%9F%D1%80%D0%B8%D0%BD%D1%8F%D1%82%D1%8C%20%D1%83%D1%87%D0%B0%D1%81%D1%82%D0%B8%D0%B5%20%D0%B2%20%D0%BE%D0%BF%D1%80%D0%BE%D1%81%D0%B5"),
-    decline: decodeURIComponent("%D0%9C%D0%BE%D0%B6%D0%B5%D1%82%20%D0%B2%20%D0%B4%D1%80%D1%83%D0%B3%D0%BE%D0%B9%20%D1%80%D0%B0%D0%B7")
+exports.incrementCorrectionsCounter = incrementCorrectionsCounter;
+function incrementCorrectionsCounter()
+{
+  // Only users with 5 URL corrections
+  if (++Prefs.corrections_count == 5)
+  {
+    initSurvey();
   }
-};
-
-exports.incrementCorrectionsCounter = incrementCorrectionsCounter;
-function incrementCorrectionsCounter(window)
-{
-  let count = typeof(Prefs.corrections_count)!="undefined" && JSON.parse(Prefs.corrections_count) || 0;
-  
-  // Only users with 5 URL corrections
-  if(++count == 5)
-  {
-    initSurvey(window);
-  }
-
-  Prefs.corrections_count = count;
 }
 
-function initSurvey(window)
+function initSurvey()
 {
-  // Don't do anything on Fennec.
-  if ("Browser" in window || "BrowserApp" in window)
-    return;
-
   // Don't ask after 2012-10-15
   if (Date.now() > 1350259200000)
     return;
 
   // Only Firefox users
-  if (Services.appinfo.ID != "{ec8030f7-c20a-464f-9b0e-13a3a9e97384}")
+  let {application} = require("info");
+  if (application != "firefox")
     return;
 
   // Only Firefox 4 and higher
   if (Services.vc.compare(Services.appinfo.platformVersion, "2.0") < 0)
     return;
 
   // Survey is only available in English/German/Russian
   if (!/^(en|de|ru)\b/.test(Services.locale.getApplicationLocale().getCategory("NSILOCALE_CTYPE")))
     return;
   surveyLang = RegExp.$1;
 
   // Delay survey question by 5 seconds
   surveyTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
   surveyTimer.initWithCallback(runSurvey, 5000, Ci.nsITimer.TYPE_ONE_SHOT);
-
-  win = window;
 }
 
 function runSurvey()
 {
-  win.openDialog("chrome://url-fixer/content/survey.xul", "survey", "top=100,left=300,chrome,dialog,dependent");
+  //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.

+    document.insertBefore(style, document.firstChild);
+
+    let panel = new window.DOMParser().parseFromString(request.responseText, "text/xml").documentElement;
+    if (!document.getElementById(panel.id))
+    {
+      document.getElementById("mainPopupSet").appendChild(panel);
+    }
+    else
+    {
+      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();
+      }
+    );
+
+    let anchor = document.getElementById("identity-box");
+    panel.openPopup(anchor, "after_start", 0, 0, false, true);
+  }, false);
+  request.send(null);
 }
 
-exports.openSurvey = openSurvey;
 function openSurvey()
 {
-  require("appIntegration").getBrowser(win).loadOneTab(surveyUrl + "&lang=" + surveyLang, {
+  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
   });
 }
-
-exports.getSurveyDialogTexts = getSurveyDialogTexts;
-function getSurveyDialogTexts()
-{
-  return langData[surveyLang];
-}