From 83a9d7cc5641e29e1c3472517b0343bdc9cd5774 Mon Sep 17 00:00:00 2001 From: Jono S Xia Date: Wed, 30 Jun 2010 10:54:21 -0700 Subject: [PATCH] Bug 575767: Stop Feedback from collection data until the user has been notified. r=dtownsend --HG-- extra : transplant_source : e%C3%0EN%E61%15%84%08%97%0E%91%FD%14%B3%92J%B1%84%97 --- .../modules/setup.js | 46 ++++++++++++------- .../modules/tasks.js | 28 +++++++---- 2 files changed, 48 insertions(+), 26 deletions(-) diff --git a/browser/app/profile/extensions/testpilot@labs.mozilla.com/modules/setup.js b/browser/app/profile/extensions/testpilot@labs.mozilla.com/modules/setup.js index 8be9462cff7..6ec33d51e4a 100644 --- a/browser/app/profile/extensions/testpilot@labs.mozilla.com/modules/setup.js +++ b/browser/app/profile/extensions/testpilot@labs.mozilla.com/modules/setup.js @@ -62,7 +62,7 @@ let TestPilotSetup = { startupComplete: false, _shortTimer: null, _longTimer: null, - _remoteExperimentLoader: null, + _remoteExperimentLoader: null, // TODO make this a lazy initializer too? taskList: [], version: "", @@ -372,10 +372,16 @@ let TestPilotSetup = { 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; @@ -422,14 +428,14 @@ let TestPilotSetup = { "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); } @@ -464,7 +470,7 @@ let TestPilotSetup = { } else { self._openChromeless(linkUrl); } - self._hideNotification(); + self._hideNotification(window, onCloseCallback); } }; link.setAttribute("hidden", false); @@ -473,7 +479,7 @@ let TestPilotSetup = { } closeBtn.onclick = function() { - self._hideNotification(); + self._hideNotification(window, onCloseCallback); }; // Show the popup: @@ -487,13 +493,19 @@ let TestPilotSetup = { 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() { @@ -543,11 +555,11 @@ let TestPilotSetup = { 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), @@ -555,14 +567,16 @@ let TestPilotSetup = { "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), diff --git a/browser/app/profile/extensions/testpilot@labs.mozilla.com/modules/tasks.js b/browser/app/profile/extensions/testpilot@labs.mozilla.com/modules/tasks.js index 021aa923508..fad488bc666 100644 --- a/browser/app/profile/extensions/testpilot@labs.mozilla.com/modules/tasks.js +++ b/browser/app/profile/extensions/testpilot@labs.mozilla.com/modules/tasks.js @@ -203,6 +203,7 @@ var TestPilotTask = { }, onDetailPageOpened: function TestPilotTask_onDetailPageOpened(){ + // TODO fold this into loadPage()? }, checkDate: function TestPilotTask_checkDate() { @@ -444,14 +445,9 @@ TestPilotExperiment.prototype = { }, 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: @@ -599,11 +595,23 @@ TestPilotExperiment.prototype = { } } - // 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();