813322 - Add ability to control time interval for restart prompt to apply update

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Stephen A Pohl [:spohl]]

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]]

Attachment: Patch in progress

Comment 2

[Stephen A Pohl [:spohl]]

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]]

Attachment: Tests in progress

Comment 4

[Stephen A Pohl [:spohl]]

Attachment: Tests in progress

Comment 5

[Stephen A Pohl [:spohl]]

Pushed patches to try and tests seem to pass: https://tbpl.mozilla.org/?tree=Try&rev=66f714bd02f7

Comment 6

[Alex Keybl [:akeybl]]

(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 (they/them - no direct email)]

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 (they/them - no direct email)]

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 (they/them - no direct email)]

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]]

Attachment: 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]]

Attachment: 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]]

Attachment: Patch

Fixed test failure. Setting for review.

Comment 13

[Stephen A Pohl [:spohl]]

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]]

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]]

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]]

Attachment: Tests

Comment 17

[Stephen A Pohl [:spohl]]

(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]]

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]]

Attachment: Tests

Carrying over r+ for the last requested change.

Comment 20

[Robert Strong (they/them - no direct email)]

(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]]

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 (they/them - no direct email)]

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]]

If this passes try tests please add the keyword checkin-needed
Thanks!

Comment 24

[Robert Strong (they/them - no direct email)]

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 (they/them - no direct email)]

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 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/bbf1ed096bd8
https://hg.mozilla.org/mozilla-central/rev/a8233178dd72

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/bbf1ed096bd8
https://hg.mozilla.org/mozilla-central/rev/a8233178dd72

Comment 28

[Lukas Blakk [:lsblakk] use ?needinfo]

This has been on central for almost a week, can someone nominate for Aurora?

Comment 29

[Robert Strong (they/them - no direct email)]

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]]

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 (they/them - no direct email)]

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]

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/[e-mail]-115622821375