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

RESOLVED FIXED in Firefox 19

Status

()

Toolkit
Application Update
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: spohl, Assigned: spohl)

Tracking

Trunk
mozilla20
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox19+ fixed)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

5 years ago
Currently we prompt to apply update after 24 hours of continuous use. This would allow us to override the time interval remotely.
(Assignee)

Comment 1

5 years ago
Created attachment 683319 [details] [diff] [review]
Patch in progress
Assignee: nobody → spohl.mozilla.bugs
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 2

5 years ago
Hi Alex, this is Stephen. I just started on Platform Integration (Firefox). This is the bug you and Robert discussed.
(Assignee)

Comment 3

5 years ago
Created attachment 684107 [details] [diff] [review]
Tests in progress
Attachment #684107 - Attachment is patch: true
(Assignee)

Comment 4

5 years ago
Created attachment 684113 [details] [diff] [review]
Tests in progress
Attachment #684107 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #684113 - Flags: review?(robert.bugzilla)
(Assignee)

Updated

5 years ago
Attachment #683319 - Flags: review?(robert.bugzilla)
(Assignee)

Comment 5

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

Comment 6

5 years ago
(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 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.
Attachment #684113 - Flags: review?(robert.bugzilla) → feedback+
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!
Attachment #683319 - Flags: review?(robert.bugzilla) → feedback+
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.
(Assignee)

Comment 10

5 years ago
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.
Attachment #683319 - Attachment is obsolete: true
(Assignee)

Comment 11

5 years ago
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.
Attachment #684113 - Attachment is obsolete: true
(Assignee)

Comment 12

5 years ago
Created attachment 686033 [details] [diff] [review]
Patch

Fixed test failure. Setting for review.
Attachment #685897 - Attachment is obsolete: true
Attachment #686033 - Flags: review?(netzen)
(Assignee)

Comment 13

5 years ago
Comment on attachment 685898 [details] [diff] [review]
Tests in progress

Setting for review now that the test failure is fixed.
Attachment #685898 - Flags: review?(netzen)
Comment on attachment 686033 [details] [diff] [review]
Patch

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

Looks great, nice work.
Attachment #686033 - Flags: review?(netzen) → review+
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
Attachment #685898 - Flags: review?(netzen) → review+
(Assignee)

Comment 16

5 years ago
Created attachment 686192 [details] [diff] [review]
Tests
Attachment #685898 - Attachment is obsolete: true
Attachment #686192 - Flags: review?(netzen)
(Assignee)

Comment 17

5 years ago
(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 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.
Attachment #686192 - Flags: review?(netzen) → review+
(Assignee)

Comment 19

5 years ago
Created attachment 686203 [details] [diff] [review]
Tests

Carrying over r+ for the last requested change.
Attachment #686192 - Attachment is obsolete: true
Attachment #686203 - Flags: review+
(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
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.
If you find missing ones in the app update tests please just set them explicitly as is done for the other preferences. Thanks!
If this passes try tests please add the keyword checkin-needed
Thanks!
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.
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
Target Milestone: --- → mozilla20
Blocks: 816408
https://hg.mozilla.org/mozilla-central/rev/bbf1ed096bd8
https://hg.mozilla.org/mozilla-central/rev/a8233178dd72
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
https://hg.mozilla.org/mozilla-central/rev/bbf1ed096bd8
https://hg.mozilla.org/mozilla-central/rev/a8233178dd72

Updated

5 years ago
tracking-firefox19: --- → +
This has been on central for almost a week, can someone nominate for Aurora?
status-firefox19: --- → affected
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
Attachment #686033 - Flags: approval-mozilla-aurora?
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!!
Attachment #686033 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Pushed to mozilla-aurora
https://hg.mozilla.org/releases/mozilla-aurora/rev/7b2b3268c4d0
https://hg.mozilla.org/releases/mozilla-aurora/rev/4e6b6accd13a
status-firefox19: affected → fixed

Updated

5 years ago
Blocks: 821624

Updated

5 years ago
Depends on: 823818

Comment 32

5 years ago
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
You need to log in before you can comment on or make changes to this bug.