310981 - Update Available message for nightly build should contain build id

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[shutdown]

When I use Software Update, I see the following message everyday:

| A new version of Deer Park is available:
| Deer Park 1.6a1
| It is strongly recommended that you upgrade Deer Park as soon as possible.

The version string "Deer Park 1.6a1" is almostly useless for nightly builds.
It should contain the build id of the new version if
and only if the running software is nightly build.
Or, it will cause a misunderstanding described in bug 306864
(expected the latest build but got the next build).

Comment 1

[Darin Fisher]

Confirming.  I think it'd be good to make the update information more useful for
folks on the nightly build channel.  Keep in mind that this system wasn't really
designed for the nightly updates.

Comment 2

[Nick Thomas [:nthomas] (UTC+12)]

Attachment: Modify the updateName when on the nightly channel [checked in - trunk]

This patch modifies the displayed update name in the notication dialog when using the nightly channel, which is a client only fix. This seems easier for the longterm than reconfiguring the tinderbox to send a version of "<version> nightly (<buildid>)", and making sure AUS won't choke. 

This patch relies on bug 325745, which only landed on the trunk to date, to define update.channel.

Comment 3

[Darin Fisher]

Comment on attachment 220012 [details] [diff] [review]
Modify the updateName when on the nightly channel [checked in - trunk]

Nice!  Let's add this to the 1.8 branch as well for FF 2 nightlies.

r+a=darin

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #3)
> (From update of attachment 220012 [details] [diff] [review] [edit])
> Nice!  Let's add this to the 1.8 branch as well for FF 2 nightlies.


You'll probably have to QI to the new interface before the channel attribute will be available on the branch.

BTW, we need branch approval for bug 325745 :)

Comment 5

[Nick Thomas [:nthomas] (UTC+12)]

(In reply to comment #4)
> You'll probably have to QI to the new interface before the channel attribute
> will be available on the branch.

Like this ?

  onPageShow: function() {
    var updateName = gUpdates.strings.getFormattedString("updateName",
      [gUpdates.brandName, gUpdates.update.version]);
+   gUpdates.update.QueryInterface(
+         Components.interfaces.nsIUpdate_MOZILLA_1_8_BRANCH);
+   if (gUpdates.update.channel == "nightly")
+     updateName = updateName + " nightly (" + gUpdates.update.buildID + ")";
    var updateNameElement = document.getElementById("updateName");
    updateNameElement.value = updateName;

Seems to work, but I'm a newbie at this stuff.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #5)
> Like this ?

Looks right to me!

Comment 7

[:Gavin Sharp [email: [e-mail]]]

mozilla/toolkit/mozapps/update/content/updates.js 	1.56

Comment 8

[Nick Thomas [:nthomas] (UTC+12)]

Attachment: Branch version of this fix

Comment 9

[Darin Fisher]

Comment on attachment 220257 [details] [diff] [review]
Branch version of this fix

Given how many times gUpdates.update is accessed in this function, I think it would be good to store it in a local var:

var update = gUpdates.update.QueryInterface(...);

Then use |update| elsewhere instead of continuing to refer to gUpdates.update.

Comment 10

[Nick Thomas [:nthomas] (UTC+12)]

Attachment: improved branch patch

Changes per comment #9

I'm not sure I understand why update.version is being used instead of update.extensionVersion in the second last change. There seems to be a mixture of usage throughout the update code. Is this by design, or should I file another bug?

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #10)
> Created an attachment (id=220392) [edit]

>     try {
>-      gUpdates.update.getProperty("licenseAccepted");
>+      update.getProperty("licenseAccepted");
>     }
>     catch (e) {
>       gUpdates.update.setProperty("licenseAccepted", "false");
>     }

I'd switch that last gUpdates.update -> update as well.

Comment 12

[Nick Thomas [:nthomas] (UTC+12)]

(In reply to comment #11)

Won't that only set it on the local copy ? The licenseAccepted property doesn't seem to be used later in the modified function.

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 220392 [details] [diff] [review]
improved branch patch

(In reply to comment #12)

Indeed! Nice catch :) Only one other nit:

>   onPageShow: function() {
>+    var update = gUpdates.update.QueryInterface(
>+                            Components.interfaces.nsIUpdate_MOZILLA_1_8_BRANCH);
>     var updateName = gUpdates.strings.getFormattedString("updateName", 

The indentation is a bit odd here, so I suggest one of two fixes:

1) add a const at the top of the file (with the other two constants):

     const nsIUpdate_MOZILLA_1_8_BRANCH  =
       Components.interfaces.nsIUpdate_MOZILLA_1_8_BRANCH;

   and then make this call:

     var update = gUpdates.update.QueryInterface(nsIUpdate_MOZILLA_1_8_BRANCH);

2) skip the const and make this call:

     var update = gUpdates.update.
         QueryInterface(Components.interfaces.nsIUpdate_MOZILLA_1_8_BRANCH);

Either of those keeps us under the 80 char max line length.

I think I'd probably go for #2, but both styles are used in updates.js already so it's really up to you.

Comment 14

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 220392 [details] [diff] [review]
improved branch patch

Requesting branch approval (for when it reopens for 2.0a3)

Comment 15

[:Gavin Sharp [email: [e-mail]]]

mozilla/toolkit/mozapps/update/content/updates.js 	1.45.2.9