Bug 575767: Stop Feedback from collection data until the user has been notified. r=dtownsend GECKO20b1_20100628_RELBRANCH FIREFOX_4_0b1_BUILD2 FIREFOX_4_0b1_RELEASE
authorJono S Xia <jdicarlo@mozilla.com>
Wed, 30 Jun 2010 10:54:21 -0700
branchGECKO20b1_20100628_RELBRANCH
changeset 46447 65c30e4ee631158408970e91fd14b3924ab18497
parent 46419 69b71ac327814ff96ad8ddafbd64edef54bbc281
child 46451 133daf2cf5fa2a23c101a2ee07de86262c883ca6
push idunknown
push userunknown
push dateunknown
reviewersdtownsend
bugs575767
milestone2.0b1
Bug 575767: Stop Feedback from collection data until the user has been notified. r=dtownsend
browser/app/profile/extensions/testpilot@labs.mozilla.com/modules/setup.js
browser/app/profile/extensions/testpilot@labs.mozilla.com/modules/tasks.js
--- a/browser/app/profile/extensions/testpilot@labs.mozilla.com/modules/setup.js
+++ b/browser/app/profile/extensions/testpilot@labs.mozilla.com/modules/setup.js
@@ -57,17 +57,17 @@ const POPUP_REMINDER_INTERVAL = "extensi
 const ALWAYS_SUBMIT_DATA = "extensions.testpilot.alwaysSubmitData";
 const LOG_FILE_NAME = "TestPilotErrorLog.log";
 
 let TestPilotSetup = {
   didReminderAfterStartup: false,
   startupComplete: false,
   _shortTimer: null,
   _longTimer: null,
-  _remoteExperimentLoader: null,
+  _remoteExperimentLoader: null, // TODO make this a lazy initializer too?
   taskList: [],
   version: "",
 
   // Lazy initializers:
   __application: null,
   get _application() {
     if (this.__application == null) {
       this.__application = Cc["@mozilla.org/fuel/application;1"]
@@ -367,20 +367,26 @@ let TestPilotSetup = {
     // exists.  No excuse to ever be running two copies of the same task.
     this.taskList.push(testPilotTask);
   },
 
   _showNotification: function TPS__showNotification(task, fragile, text, title,
                                                     iconClass, showSubmit,
 						    showAlwaysSubmitCheckbox,
                                                     linkText, linkUrl,
-						    isExtensionUpdate) {
+						    isExtensionUpdate,
+                                                    onCloseCallback) {
+    /* TODO: Refactor the arguments of this function, it's getting really
+     * unweildly.... maybe pass in an object, or even make a notification an
+     * object that you create and then call .show() on. */
+
     // If there are multiple windows, show notifications in the frontmost
     // window.
-    let doc = this._getFrontBrowserWindow().document;
+    let window = this._getFrontBrowserWindow();
+    let doc = window.document;
     let popup = doc.getElementById("pilot-notification-popup");
 
     let anchor;
     if (this._isFfx4BetaVersion()) {
       /* If we're in the Ffx4Beta version, popups come down from feedback
        * button, but if we're in the standalone extension version, they
        * come up from status bar icon. */
       anchor = doc.getElementById("feedback-menu-button");
@@ -417,24 +423,24 @@ let TestPilotSetup = {
     alwaysSubmitCheckbox.setAttribute("hidden", !showAlwaysSubmitCheckbox);
     if (showSubmit) {
       if (isExtensionUpdate) {
         submitBtn.setAttribute("label",
 	  this._stringBundle.GetStringFromName(
 	    "testpilot.notification.update"));
 	submitBtn.onclick = function() {
           this._extensionUpdater.check(EXTENSION_ID);
-          self._hideNotification();
+          self._hideNotification(window, onCloseCallback);
 	};
       } else {
         submitBtn.setAttribute("label",
 	  this._stringBundle.GetStringFromName("testpilot.submit"));
         // Functionality for submit button:
         submitBtn.onclick = function() {
-          self._hideNotification();
+          self._hideNotification(window, onCloseCallback);
           if (showAlwaysSubmitCheckbox && alwaysSubmitCheckbox.checked) {
             self._prefs.setValue(ALWAYS_SUBMIT_DATA, true);
           }
           task.upload( function(success) {
             if (success) {
               self._showNotification(
 		task, true,
                 self._stringBundle.GetStringFromName(
@@ -459,46 +465,52 @@ let TestPilotSetup = {
       link.setAttribute("class", "notification-link");
       link.onclick = function(event) {
         if (event.button == 0) {
 	  if (task) {
             task.loadPage();
 	  } else {
             self._openChromeless(linkUrl);
 	  }
-          self._hideNotification();
+          self._hideNotification(window, onCloseCallback);
         }
       };
       link.setAttribute("hidden", false);
     } else {
       link.setAttribute("hidden", true);
     }
 
     closeBtn.onclick = function() {
-      self._hideNotification();
+      self._hideNotification(window, onCloseCallback);
     };
 
     // Show the popup:
     popup.hidden = false;
     popup.setAttribute("open", "true");
     popup.openPopup( anchor, "after_end");
   },
 
   _openChromeless: function TPS__openChromeless(url) {
     let window = this._getFrontBrowserWindow();
     window.TestPilotWindowUtils.openChromeless(url);
   },
 
-  _hideNotification: function TPS__hideNotification() {
-    let window = this._getFrontBrowserWindow();
+  _hideNotification: function TPS__hideNotification(window, onCloseCallback) {
+    /* Note - we take window as an argument instead of just using the frontmost
+     * window because the window order might have changed since the notification
+     * appeared and we want to be sure we close the notification in the same
+     * window as we opened it in! */
     let popup = window.document.getElementById("pilot-notification-popup");
     popup.hidden = true;
     popup.setAttribute("open", "false");
     popup.removeAttribute("tpisextensionupdate");
     popup.hidePopup();
+    if (onCloseCallback) {
+      onCloseCallback();
+    }
   },
 
   _isShowingUpdateNotification : function() {
     let window = this._getFrontBrowserWindow();
     let popup = window.document.getElementById("pilot-notification-popup");
 
     return popup.hasAttribute("tpisextensionupdate");
   },
@@ -538,36 +550,38 @@ let TestPilotSetup = {
       }
     }
 
     // If there's no finished test, next highest priority is new tests that
     // are starting...
     if (this._prefs.getValue(POPUP_SHOW_ON_NEW, false)) {
       for (i = 0; i < this.taskList.length; i++) {
         task = this.taskList[i];
-        if (task.status == TaskConstants.STATUS_STARTING ||
+        if (task.status == TaskConstants.STATUS_PENDING ||
             task.status == TaskConstants.STATUS_NEW) {
           if (task.taskType == TaskConstants.TYPE_EXPERIMENT) {
 	    this._showNotification(
-	      task, true,
+	      task, false,
 	      this._stringBundle.formatStringFromName(
 		"testpilot.notification.newTestPilotStudy.message",
 		[task.title], 1),
 	      this._stringBundle.GetStringFromName(
 		"testpilot.notification.newTestPilotStudy"),
 	      "new-study", false, false,
 	      this._stringBundle.GetStringFromName("testpilot.moreInfo"),
-	      task.defaultUrl);
-            // Having shown the notification, update task status so that this
-            // notification won't be shown again.
-            task.changeStatus(TaskConstants.STATUS_IN_PROGRESS, true);
+	      task.defaultUrl, false, function() {
+                /* on close callback (Bug 575767) -- when the "new study
+                 * starting" popup is dismissed, then the study can start. */
+                task.changeStatus(TaskConstants.STATUS_IN_PROGRESS, true);
+                TestPilotSetup.reloadRemoteExperiments();
+              });
             return;
           } else if (task.taskType == TaskConstants.TYPE_SURVEY) {
 	    this._showNotification(
-	      task, true,
+	      task, false,
 	      this._stringBundle.formatStringFromName(
 		"testpilot.notification.newTestPilotSurvey.message",
 		[task.title], 1),
               this._stringBundle.GetStringFromName(
 		"testpilot.notification.newTestPilotSurvey"),
 	      "new-study", false, false,
 	      this._stringBundle.GetStringFromName("testpilot.moreInfo"),
 	      task.defaultUrl);
--- a/browser/app/profile/extensions/testpilot@labs.mozilla.com/modules/tasks.js
+++ b/browser/app/profile/extensions/testpilot@labs.mozilla.com/modules/tasks.js
@@ -198,16 +198,17 @@ var TestPilotTask = {
 
   onWindowClosed: function TestPilotTask_onWindowClosed(window) {
   },
 
   onUrlLoad: function TestPilotTask_onUrlLoad(url) {
   },
 
   onDetailPageOpened: function TestPilotTask_onDetailPageOpened(){
+    // TODO fold this into loadPage()?
   },
 
   checkDate: function TestPilotTask_checkDate() {
   },
 
   changeStatus: function TPS_changeStatus(newStatus, suppressNotification) {
     // TODO we always suppress notifications except when new status is
     // "finished"; maybe remove that argument and only fire notification
@@ -439,24 +440,19 @@ TestPilotExperiment.prototype = {
       break;
     }
     if (!waitForData) {
       callback(content);
     }
   },
 
   experimentIsRunning: function TestPilotExperiment_isRunning() {
-    if (this._optInRequired) {
-      return (this._status == TaskConstants.STATUS_STARTING ||
-              this._status == TaskConstants.STATUS_IN_PROGRESS);
-    } else {
-      // Tests that don't require extra opt-in should start running even
-      // if you haven't seen them yet.
-      return (this._status < TaskConstants.STATUS_FINISHED);
-    }
+    // bug 575767
+    return (this._status == TaskConstants.STATUS_STARTING ||
+            this._status == TaskConstants.STATUS_IN_PROGRESS);
   },
 
   // Pass events along to handlers:
   onNewWindow: function TestPilotExperiment_onNewWindow(window) {
     this._logger.trace("Experiment.onNewWindow called.");
     if (this.experimentIsRunning()) {
       this._handlers.onNewWindow(window);
     }
@@ -594,21 +590,33 @@ TestPilotExperiment.prototype = {
         numTimesRun++;
         this._logger.trace("Test recurring... incrementing " + RECUR_TIMES_PREF_PREFIX + this._id + " to " + numTimesRun);
         Application.prefs.setValue( RECUR_TIMES_PREF_PREFIX + this._id,
                                     numTimesRun );
         this._logger.trace("Incremented it.");
       }
     }
 
-    // No-opt-in required tests skip PENDING and go straight to STARTING.
+    // If the notify-on-new-study pref is turned off, and the test doesn't
+    // require opt-in, then it can jump straight ahead to STARTING.
     if (!this._optInRequired &&
-        this._status < TaskConstants.STATUS_STARTING &&
+        !Application.prefs.getValue("extensions.testpilot.popup.showOnNewStudy",
+                                    false) &&
+        (this._status == TaskConstants.STATUS_NEW ||
+         this._status == TaskConstants.STATUS_PENDING)) {
+      this._logger.info("Skipping pending and going straight to starting.");
+      this.changeStatus(TaskConstants.STATUS_STARTING, true);
+    }
+
+    // If a study is STARTING, and we're in the right date range,
+    // then start it, and move it to IN_PROGRESS.
+    if ( this._status == TaskConstants.STATUS_STARTING &&
         currentDate >= this._startDate &&
         currentDate <= this._endDate) {
+      this._logger.info("Study now starting.");
       let uuidGenerator =
         Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator);
       let uuid = uuidGenerator.generateUUID().toString();
       // remove the brackets from the generated UUID
       if (uuid.indexOf("{") == 0) {
         uuid = uuid.substring(1, (uuid.length - 1));
       }
       // clear the data before starting.