Last Comment Bug 813322 - Add ability to control time interval for restart prompt to apply update
: Add ability to control time interval for restart prompt to apply update
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Application Update (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla20
Assigned To: Stephen A Pohl [:spohl]
:
Mentors:
Depends on: 823818
Blocks: 816408 821624
  Show dependency treegraph
 
Reported: 2012-11-19 14:07 PST by Stephen A Pohl [:spohl]
Modified: 2012-12-21 15:22 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
Patch in progress (2.93 KB, patch)
2012-11-19 14:42 PST, Stephen A Pohl [:spohl]
robert.strong.bugs: feedback+
Details | Diff | Review
Tests in progress (14.83 KB, patch)
2012-11-21 11:12 PST, Stephen A Pohl [:spohl]
no flags Details | Diff | Review
Tests in progress (16.87 KB, patch)
2012-11-21 11:24 PST, Stephen A Pohl [:spohl]
robert.strong.bugs: feedback+
Details | Diff | Review
Patch (2.95 KB, patch)
2012-11-27 16:59 PST, Stephen A Pohl [:spohl]
no flags Details | Diff | Review
Tests in progress (19.59 KB, patch)
2012-11-27 17:00 PST, Stephen A Pohl [:spohl]
netzen: review+
Details | Diff | Review
Patch (2.97 KB, patch)
2012-11-28 01:43 PST, Stephen A Pohl [:spohl]
netzen: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review
Tests (20.12 KB, patch)
2012-11-28 11:13 PST, Stephen A Pohl [:spohl]
netzen: review+
Details | Diff | Review
Tests (19.93 KB, patch)
2012-11-28 11:26 PST, Stephen A Pohl [:spohl]
spohl.mozilla.bugs: review+
Details | Diff | Review

Description Stephen A Pohl [:spohl] 2012-11-19 14:07:51 PST
Currently we prompt to apply update after 24 hours of continuous use. This would allow us to override the time interval remotely.
Comment 1 Stephen A Pohl [:spohl] 2012-11-19 14:42:43 PST
Created attachment 683319 [details] [diff] [review]
Patch in progress
Comment 2 Stephen A Pohl [:spohl] 2012-11-19 15:46:58 PST
Hi Alex, this is Stephen. I just started on Platform Integration (Firefox). This is the bug you and Robert discussed.
Comment 3 Stephen A Pohl [:spohl] 2012-11-21 11:12:09 PST
Created attachment 684107 [details] [diff] [review]
Tests in progress
Comment 4 Stephen A Pohl [:spohl] 2012-11-21 11:24:39 PST
Created attachment 684113 [details] [diff] [review]
Tests in progress
Comment 5 Stephen A Pohl [:spohl] 2012-11-21 13:11:46 PST
Pushed patches to try and tests seem to pass: https://tbpl.mozilla.org/?tree=Try&rev=66f714bd02f7
Comment 6 Alex Keybl [:akeybl] 2012-11-27 12:12:21 PST
(In reply to Stephen Pohl [:spohl] from comment #2)
> Hi Alex, this is Stephen. I just started on Platform Integration (Firefox).
> This is the bug you and Robert discussed.

Thanks for putting this together Stephen! This will really help us to change the user experience around software updates for the better.
Comment 7 Robert Strong [:rstrong] (use needinfo to contact me) 2012-11-27 13:30:55 PST
Comment on attachment 684113 [details] [diff] [review]
Tests in progress

>diff -r e7f8b95d1f5e toolkit/mozapps/update/test/chrome/test_0095_restartNotification.xul
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/toolkit/mozapps/update/test/chrome/test_0095_restartNotification.xul	Wed Nov 21 11:16:23 2012 -0800
>@@ -0,0 +1,58 @@
>+<?xml version="1.0"?>
>+<!--
>+/* Any copyright is dedicated to the Public Domain.
>+ * http://creativecommons.org/publicdomain/zero/1.0/
>+ */
>+-->
>+
>+<?xml-stylesheet href="chrome://global/skin" type="text/css"?>
>+<?xml-stylesheet href="chrome://mochikit/content/tests/SimpleTest/test.css" type="text/css"?>
>+
>+<window title="Update Wizard pages: finished background"
nit: please change this
s/Update Wizard pages: finished background/Update Wizard pages: restart notification pref promptWaitTime/

>diff -r e7f8b95d1f5e toolkit/mozapps/update/test/chrome/test_0096_restartNotification_remote.xul
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/toolkit/mozapps/update/test/chrome/test_0096_restartNotification_remote.xul	Wed Nov 21 11:16:23 2012 -0800
>@@ -0,0 +1,61 @@
>+<?xml version="1.0"?>
>+<!--
>+/* Any copyright is dedicated to the Public Domain.
>+ * http://creativecommons.org/publicdomain/zero/1.0/
>+ */
>+-->
>+
>+<?xml-stylesheet href="chrome://global/skin" type="text/css"?>
>+<?xml-stylesheet href="chrome://mochikit/content/tests/SimpleTest/test.css" type="text/css"?>
>+
>+<window title="Update Wizard pages: finished background"
nit: please change this
s/Update Wizard pages: finished background/Update Wizard pages: restart notification xml promptWaitTime/

>diff -r e7f8b95d1f5e toolkit/mozapps/update/test/chrome/update.sjs
>--- a/toolkit/mozapps/update/test/chrome/update.sjs	Wed Nov 21 11:15:31 2012 -0800
>+++ b/toolkit/mozapps/update/test/chrome/update.sjs	Wed Nov 21 11:16:23 2012 -0800
>@@ -123,6 +123,7 @@
>     licenseURL = URL_HOST + URL_PATH + "missing.html";
>   var showPrompt = params.showPrompt ? "true" : null;
>   var showNever = params.showNever ? "true" : null;
>+  var promptWaitTime = params.promptWaitTime ? params.promptWaitTime : null;
>   var showSurvey = params.showSurvey ? "true" : null;
> 
>   // For testing the deprecated update xml format
>@@ -145,8 +146,8 @@
>                                       displayVersion, appVersion,
>                                       platformVersion, buildID, detailsURL,
>                                       billboardURL, licenseURL, showPrompt,
>-                                      showNever, showSurvey, version,
>-                                      extensionVersion);
>+                                      showNever, promptWaitTime, showSurvey,
>+                                      version, extensionVersion);
> 
>   aResponse.write(getRemoteUpdatesXMLString(updates));
> }
>diff -r e7f8b95d1f5e toolkit/mozapps/update/test/sharedUpdateXML.js
>--- a/toolkit/mozapps/update/test/sharedUpdateXML.js	Wed Nov 21 11:15:31 2012 -0800
>+++ b/toolkit/mozapps/update/test/sharedUpdateXML.js	Wed Nov 21 11:16:23 2012 -0800
>@@ -62,14 +62,14 @@
> function getRemoteUpdateString(aPatches, aType, aName, aDisplayVersion,
>                                aAppVersion, aPlatformVersion, aBuildID,
>                                aDetailsURL, aBillboardURL, aLicenseURL,
>-                               aShowPrompt, aShowNeverForVersion, aShowSurvey,
>-                               aVersion, aExtensionVersion, aCustom1,
>+                               aShowPrompt, aShowNeverForVersion, aPromptWaitTime,
>+                               aShowSurvey, aVersion, aExtensionVersion, aCustom1,
>                                aCustom2) {
>   return getUpdateString(aType, aName, aDisplayVersion, aAppVersion,
>                          aPlatformVersion, aBuildID, aDetailsURL,
>                          aBillboardURL, aLicenseURL, aShowPrompt,
>-                         aShowNeverForVersion, aShowSurvey, aVersion,
>-                         aExtensionVersion, aCustom1, aCustom2) + ">\n" +
>+                         aShowNeverForVersion, aPromptWaitTime, aShowSurvey,
>+                         aVersion, aExtensionVersion, aCustom1, aCustom2) + ">\n" +
>               aPatches +
>          "  </update>\n";
> }
>@@ -131,9 +131,9 @@
>                               aDetailsURL, aBillboardURL, aLicenseURL,
>                               aServiceURL, aInstallDate, aStatusText,
>                               aIsCompleteUpdate, aChannel, aForegroundDownload,
>-                              aShowPrompt, aShowNeverForVersion, aShowSurvey,
>-                              aVersion, aExtensionVersion, aPreviousAppVersion,
>-                              aCustom1, aCustom2) {
>+                              aShowPrompt, aShowNeverForVersion, aPromptWaitTime,
>+                              aShowSurvey, aVersion, aExtensionVersion,
>+                              aPreviousAppVersion, aCustom1, aCustom2) {
>   let serviceURL = aServiceURL ? aServiceURL : "http://test_service/";
>   let installDate = aInstallDate ? aInstallDate : "1238441400314";
>   let statusText = aStatusText ? aStatusText : "Install Pending";
>@@ -149,8 +149,8 @@
>   return getUpdateString(aType, aName, aDisplayVersion, aAppVersion,
>                          aPlatformVersion, aBuildID, aDetailsURL, aBillboardURL,
>                          aLicenseURL, aShowPrompt, aShowNeverForVersion,
>-                         aShowSurvey, aVersion, aExtensionVersion, aCustom1,
>-                         aCustom2) +
>+                         aPromptWaitTime, aShowSurvey, aVersion, aExtensionVersion,
>+                         aCustom1, aCustom2) +
>                    " " +
>                    previousAppVersion +
>                    "serviceURL=\"" + serviceURL + "\" " +
>@@ -228,6 +228,8 @@
>  * @param  aShowNeverForVersion (optional)
>  *         Whether to show the 'No Thanks' button in the update prompt.
>  *         If not specified it will not be present and the update service will
>+ * @param  aPromptWaitTime (optional)
>+ *         Override for the app.update.promptWaitTime preference.
>  *         default to false.
The new comments should start after " *         default to false." as follows

>  *         default to false.
>+ * @param  aPromptWaitTime (optional)
>+ *         Override for the app.update.promptWaitTime preference.

Note: we might want to use different promptWaitTime numbers for the tests but I haven't thought that through.

Please make the changes requested above, resubmit, and feedback=me.
Comment 8 Robert Strong [:rstrong] (use needinfo to contact me) 2012-11-27 13:47:28 PST
Comment on attachment 683319 [details] [diff] [review]
Patch in progress

>diff -r 258292c9c929 toolkit/mozapps/update/nsIUpdateService.idl
>--- a/toolkit/mozapps/update/nsIUpdateService.idl	Mon Nov 19 15:35:06 2012 +0000
>+++ b/toolkit/mozapps/update/nsIUpdateService.idl	Mon Nov 19 14:41:44 2012 -0800
>@@ -87,7 +87,7 @@
>  * that the front end and other application services can use to learn more
>  * about what is going on.
>  */
>-[scriptable, uuid(b10bbf29-5a54-4e1e-aa64-c4e4e5819a52)]
>+[scriptable, uuid(8f7185a7-056a-45a8-985c-1cb39cf7b7a8)]
> interface nsIUpdate : nsISupports
> {
>   /**
>@@ -182,6 +182,13 @@
>    * present in the app.update.surveyURL preference.
>    */
>   attribute boolean showSurvey;
>+  
>+  /**
>+   * Allows overriding the default amount of time (seconds) before prompting the
s/(seconds)/in seconds/

>diff -r 258292c9c929 toolkit/mozapps/update/nsUpdateService.js
>--- a/toolkit/mozapps/update/nsUpdateService.js	Mon Nov 19 15:35:06 2012 +0000
>+++ b/toolkit/mozapps/update/nsUpdateService.js	Mon Nov 19 14:41:44 2012 -0800
>@@ -1314,6 +1315,8 @@
>       this.showNeverForVersion = attr.value == "true";
>     else if (attr.name == "showPrompt")
>       this.showPrompt = attr.value == "true";
>+    else if (attr.name == "promptWaitTime")
Please change
>+    else if (attr.name == "promptWaitTime" && !isNaN(attr.value))

>+        this.promptWaitTime = parseInt(attr.value);
>     else if (attr.name == "showSurvey")
>       this.showSurvey = attr.value == "true";
>     else if (attr.name == "version") {
>@@ -3932,11 +3936,11 @@
>       return;
>     }
> 
>-    // Give the user x seconds to react before showing the big UI
>-    var promptWaitTime = getPref("getIntPref", PREF_APP_UPDATE_PROMPTWAITTIME, 43200);
>+    // Give the user x seconds to react before prompting as defined by
>+    // promptWaitTime
>     observer.timer = Cc["@mozilla.org/timer;1"].
>                      createInstance(Ci.nsITimer);
>-    observer.timer.initWithCallback(observer, promptWaitTime * 1000,
>+    observer.timer.initWithCallback(observer, update.promptWaitTime * 1000,
>                                     observer.timer.TYPE_ONE_SHOT);
Did we go over all of the callsites? I think so.

feedback=me with the above changes.

Please resubmit both patches and request review from :bbondy. Thanks!
Comment 9 Robert Strong [:rstrong] (use needinfo to contact me) 2012-11-27 13:51:41 PST
Should add a new test for the case where the xml contains text vs. a number and verify that the value specified by the preference is used.
Comment 10 Stephen A Pohl [:spohl] 2012-11-27 16:59:48 PST
Created attachment 685897 [details] [diff] [review]
Patch

Not setting for review yet until we've figured out why the last test (test_0097_restartNotification_remoteInvalidNumber.xul) fails.
Comment 11 Stephen A Pohl [:spohl] 2012-11-27 17:00:45 PST
Created attachment 685898 [details] [diff] [review]
Tests in progress

Not setting for review yet until we've figured out why the last test (test_0097_restartNotification_remoteInvalidNumber.xul) fails.
Comment 12 Stephen A Pohl [:spohl] 2012-11-28 01:43:01 PST
Created attachment 686033 [details] [diff] [review]
Patch

Fixed test failure. Setting for review.
Comment 13 Stephen A Pohl [:spohl] 2012-11-28 01:44:07 PST
Comment on attachment 685898 [details] [diff] [review]
Tests in progress

Setting for review now that the test failure is fixed.
Comment 14 Brian R. Bondy [:bbondy] 2012-11-28 05:25:00 PST
Comment on attachment 686033 [details] [diff] [review]
Patch

Review of attachment 686033 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, nice work.
Comment 15 Brian R. Bondy [:bbondy] 2012-11-28 05:25:11 PST
Comment on attachment 685898 [details] [diff] [review]
Tests in progress

Review of attachment 685898 [details] [diff] [review]:
-----------------------------------------------------------------

Look great.

::: toolkit/mozapps/update/test/chrome/test_0095_restartNotification.xul
@@ +34,5 @@
> +                                     Services.appinfo.platformVersion);
> +  writeUpdatesToXMLFile(getLocalUpdatesXMLString(updates), true);
> +  writeUpdatesToXMLFile(getLocalUpdatesXMLString(""), false);
> +  writeStatusFile(STATE_SUCCEEDED);
> +  

nit: I think it would be good to make sure we got the value of the previous PREF_APP_UPDATE_PRMOPTWAITTIME here for gUpdateManager.activeUpdate.promptWaitTime

::: toolkit/mozapps/update/test/chrome/test_0096_restartNotification_remote.xul
@@ +39,5 @@
> +                                     false, 1);
> +  writeUpdatesToXMLFile(getLocalUpdatesXMLString(updates), true);
> +  writeUpdatesToXMLFile(getLocalUpdatesXMLString(""), false);
> +  writeStatusFile(STATE_SUCCEEDED);
> +    

nit: I think it would also be good to check gUpdateManager.activeUpdate.promptWaitTime == 1 here
Comment 16 Stephen A Pohl [:spohl] 2012-11-28 11:13:05 PST
Created attachment 686192 [details] [diff] [review]
Tests
Comment 17 Stephen A Pohl [:spohl] 2012-11-28 11:14:38 PST
(In reply to Stephen Pohl [:spohl] from comment #16)
> Created attachment 686192 [details] [diff] [review]
> Tests

I wasn't sure if I should carry over the review+ for this patch, so I set it to review? just to be safe.
Comment 18 Brian R. Bondy [:bbondy] 2012-11-28 11:21:04 PST
Comment on attachment 686192 [details] [diff] [review]
Tests

Review of attachment 686192 [details] [diff] [review]:
-----------------------------------------------------------------

Usually you can carry forward the r+ but it's fine to also re-r+.
Please carry forward the r+ for that last requested change.

Looks good to land, thanks for the changes!

::: toolkit/mozapps/update/test/chrome/test_0095_restartNotification.xul
@@ +37,5 @@
> +  writeStatusFile(STATE_SUCCEEDED);
> +
> +  ok(Services.prefs.getIntPref(PREF_APP_UPDATE_PROMPTWAITTIME) == 0,
> +     "Checking that the " + PREF_APP_UPDATE_PROMPTWAITTIME + " preference " +
> +     "has a valid default value");

nit: Remove this, in bug 816111 I'll be removing dependencies on specific app preferences.  Since this is in toolkit a lot of apps can use this and they may have different values for preferences.
Comment 19 Stephen A Pohl [:spohl] 2012-11-28 11:26:43 PST
Created attachment 686203 [details] [diff] [review]
Tests

Carrying over r+ for the last requested change.
Comment 20 Robert Strong [:rstrong] (use needinfo to contact me) 2012-11-28 12:30:04 PST
(In reply to Brian R. Bondy [:bbondy] from comment #18)
> Comment on attachment 686192 [details] [diff] [review]
> Tests
> 
> Review of attachment 686192 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Usually you can carry forward the r+ but it's fine to also re-r+.
> Please carry forward the r+ for that last requested change.
> 
> Looks good to land, thanks for the changes!
> 
> ::: toolkit/mozapps/update/test/chrome/test_0095_restartNotification.xul
> @@ +37,5 @@
> > +  writeStatusFile(STATE_SUCCEEDED);
> > +
> > +  ok(Services.prefs.getIntPref(PREF_APP_UPDATE_PROMPTWAITTIME) == 0,
> > +     "Checking that the " + PREF_APP_UPDATE_PROMPTWAITTIME + " preference " +
> > +     "has a valid default value");
> 
> nit: Remove this, in bug 816111 I'll be removing dependencies on specific
> app preferences.  Since this is in toolkit a lot of apps can use this and
> they may have different values for preferences.
Note that the app update tests try to set all of the values used by the tests instead of relying on app provided preferences for exactly this reason and that this pref is set.
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/test/chrome/utils.js#821
Comment 21 Brian R. Bondy [:bbondy] 2012-11-28 12:32:43 PST
ah didn't see that we were setting that explicitly.  I know there are some app specific prefs that I'm cleaning up in that bug though.  I don't think this check is needed but that's fine since it's set in the test.
Comment 22 Robert Strong [:rstrong] (use needinfo to contact me) 2012-11-28 12:34:48 PST
If you find missing ones in the app update tests please just set them explicitly as is done for the other preferences. Thanks!
Comment 23 Brian R. Bondy [:bbondy] 2012-11-28 16:34:08 PST
If this passes try tests please add the keyword checkin-needed
Thanks!
Comment 24 Robert Strong [:rstrong] (use needinfo to contact me) 2012-11-28 17:23:32 PST
Try url
https://tbpl.mozilla.org/?tree=Try&rev=115622821375

Looks good to me so far and if it still looks good later on tonight I'll push it to inbound tonight as well.
Comment 25 Robert Strong [:rstrong] (use needinfo to contact me) 2012-11-28 20:58:41 PST
Try build looks great!

Pushed to mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbf1ed096bd8
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8233178dd72
Comment 28 Lukas Blakk [:lsblakk] use ?needinfo 2012-12-06 10:06:04 PST
This has been on central for almost a week, can someone nominate for Aurora?
Comment 29 Robert Strong [:rstrong] (use needinfo to contact me) 2012-12-06 10:42:18 PST
Comment on attachment 686033 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): this is the feature bug.
User impact if declined: unable to provide this feature
Testing completed (on m-c, etc.): tests have landed and it has been tested locally
Risk to taking this patch (and alternatives if risky): little since it is well tested in tree and has baked
String or UUID changes made by this patch: The uuid for nsIUpdate is changed due to adding a promptWaitTime attribute
Comment 30 Alex Keybl [:akeybl] 2012-12-07 08:53:26 PST
Comment on attachment 686033 [details] [diff] [review]
Patch

[Triage Comment]
Approving for Aurora 19. This'll allow us to change the update prompting for release users based upon the data pulled in bug 816537 in the 19 timeframe. Thanks!!
Comment 31 Robert Strong [:rstrong] (use needinfo to contact me) 2012-12-07 11:30:28 PST
Pushed to mozilla-aurora
https://hg.mozilla.org/releases/mozilla-aurora/rev/7b2b3268c4d0
https://hg.mozilla.org/releases/mozilla-aurora/rev/4e6b6accd13a
Comment 32 Mozilla RelEng Bot 2012-12-21 15:22:06 PST
Try run for 115622821375 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=115622821375
Results (out of 47 total builds):
    success: 46
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/spohl@mozilla.com-115622821375

Note You need to log in before you can comment on or make changes to this bug.