893547 - About SeaMonkey page claims to be on the "default" update channel for 2.20b1 and later

Status: VERIFIED FIXED

(SeaMonkey :: General, defect)

Description

[rsx11m]

That's weird, app.update.channel is set correctly but Help > About SeaMonkey or just about: states "You are currently on the *default* update channel."

This works properly in the 2.19 release, but no longer in the 2.20 beta 1.

Also visible in 2.21a2 and 2.22a1 nightly builds. On Windows, I'm using the ZIP downloads from the ftp.mozilla.org web site, in case it makes a difference that those are not the full installers (but then, 2.19 should be affected too).

Comment 1

[Tony Mechelynck [:tonymec]]

Mozilla/5.0 (X11; Linux i686 on x86_64; rv:25.0) Gecko/20100101 Firefox/25.0 SeaMonkey/2.22a1 ID:20130714003001 c-c:c44fd572c038 m-c:18467a85acf6

I see this too, on SeaMonkey 2.22a1 on Linux: app.update.channel is set to "nightly", but the about: page tells me "You are currently on the *default* update channel."

Comment 2

[rsx11m]

http://mxr.mozilla.org/comm-central/source/suite/common/about.xhtml#91 contains the respective inline JavaScript code:

> 91  // Determine and display current channel.
> 92  try {
> 93    var updateChannel = Services.prefs.getDefaultBranch("").getCharPref("app.update.channel");
> 94    document.getElementById("currentChannel").textContent = updateChannel;
> 95  } catch (ex) {}

Given that all other items up to the BuildID are filled in correctly before getting to this point, apparently either Services.prefs.getDefaultBranch or setting .textContent fails, leaving "default" as string for id="currentChannel".

Comment 3

[rsx11m]

Attachment: Easy fix

Well, apparently Services.* got lost somehow. Thus, importing it explicitly fixes the problem for me, where I'm not sure if that's the best solution.

Other observations:

 - Should the fixed text rather read "unknown" than "default" to distinguish the
   true "default" channel setting from a failed channel lookup?

 - Is the try {} ex {} clause helpful in this case? It was hiding the issue by
   shielding the Error Console from displaying an error that Services.* is not
   defined (first thing I did was uncommenting that to get the message).

Comment 4

[rsx11m]

Attachment: Alternative patch

This changes the XUL label text to be better recognizable on failure and removes the try/catch block as proposed.

Comment 5

[rsx11m]

(I realize that "unknown" is an unlocalizable string and in contrast to "default" not a real value of the channel description, but neither is "default" correct in this case; the string is only visible when a failure occurs, so this should be acceptable, or can be replace with something l10n-neutral like "???" if not.)

Comment 6

[[e-mail]]

(In reply to rsx11m from comment #3)
> Is the try {} ex {} clause helpful in this case? It was hiding the issue by
> shielding the Error Console from displaying an error that Services.* is not
> defined (first thing I did was uncommenting that to get the message).
No, it's unhelpful, the pref should always exist!

> Should the fixed text rather read "unknown" than "default" to distinguish the
> true "default" channel setting from a failed channel lookup?
Interesting point. It's almost worth leaving it blank, which looks more like an error. I'll see whether ewong agrees with me.

Comment 7

[Edmund Wong (:ewong)]

(In reply to [e-mail] from comment #6)
> (In reply to rsx11m from comment #3)
> > Is the try {} ex {} clause helpful in this case? It was hiding the issue by
> > shielding the Error Console from displaying an error that Services.* is not
> > defined (first thing I did was uncommenting that to get the message).
> No, it's unhelpful, the pref should always exist!

Right.  So from the start to this point, Services.* isn't imported?
(I am surprised about this, but I haven't tracked the Services.* importing.)

> 
> > Should the fixed text rather read "unknown" than "default" to distinguish the
> > true "default" channel setting from a failed channel lookup?
> Interesting point. It's almost worth leaving it blank, which looks more like
> an error. I'll see whether ewong agrees with me.

I'd agree with Neil.  Leaving it blank seems like a better alternative
than to having something arbitrary (which is probably just as equivalent to
'unknown').

Comment 8

[Edmund Wong (:ewong)]

Comment on attachment 775342 [details] [diff] [review]
Easy fix

I like this patch as opposed to the second one.

Comment 9

[rsx11m]

Attachment: Proposed patch (v3) [check-in comment 14, comment 19]

Ok, so this is attachment 775344 [details] [diff] [review] with the <strong> text initially kept blank to make it better recognizable as an error condition.

Comment 10

[rsx11m]

Comment on attachment 775342 [details] [diff] [review]
Easy fix

(In reply to Edmund Wong (:ewong) from comment #8)
> I like this patch as opposed to the second one.

Oops, I've missed that one, thus reinstating this patch for review (though personally I'd prefer the second version with the <strong> text blanked).

Comment 11

[Philip Chee]

> +      Components.utils.import("resource://gre/modules/Services.jsm");
Can I suggest (perhaps in a followup bug):
1) Move the Services.jsm import to the top of the script.
2) Use Services for nsIURLFormatter, nsIXULAppInfo

Comment 12

[rsx11m]

The aim for this bug is to make the about: page work again on all channels. While comment #11 sure makes sense, I think that should go into a separate bug as it's more like an improvement of the page than fixing the immediate issue.

I have filed bug 893740 for this.

Push for attachment 775427 [details] [diff] [review] on comm-central please, whenever possible.

Comment 13

[rsx11m]

Comment on attachment 775427 [details] [diff] [review]
Proposed patch (v3) [check-in comment 14, comment 19]

[Approval Request Comment]
Regression caused by (bug #): bug 735333
User impact if declined: invalid display of update channel in about: page
Testing completed (on m-c, etc.): c-c
Risk to taking this patch (and alternatives if risky): low
String changes made by this patch: removed non-localized string, no l10n impact

Comment 14

[Philip Chee]

Comment on attachment 775427 [details] [diff] [review]
Proposed patch (v3) [check-in comment 14, comment 19]

Pushed to comm-central:
https://hg.mozilla.org/comm-central/rev/d4d533c8663f

Comment 15

[rsx11m]

Thanks Phil. Did you leave the "checkin-needed" pending on purpose?

Comment 16

[Philip Chee]

> Thanks Phil. Did you leave the "checkin-needed" pending on purpose?
Yes. Perhaps I should remove it pending approval?

Comment 17

[rsx11m]

Let's remove it until the branch approvals are actually in to avoid confusion.

Comment 18

[rsx11m]

Thanks for the approvals, reinstating checkin-needed for the branches.

Comment 19

[Philip Chee]

Pushed to branches.
remote:   https://hg.mozilla.org/releases/comm-aurora/rev/dfdbe6c067b1
remote:   https://hg.mozilla.org/releases/comm-beta/rev/ef499ee2aa1d

Comment 20

[Tony Mechelynck [:tonymec]]

(In reply to Philip Chee from comment #14)
> Comment on attachment 775427 [details] [diff] [review]
> Proposed patch (v3) [check-in comment 14, comment 19]
> 
> Pushed to comm-central:
> https://hg.mozilla.org/comm-central/rev/d4d533c8663f

Mozilla/5.0 (X11; Linux i686 on x86_64; rv:25.0) Gecko/20100101 Firefox/25.0 SeaMonkey/2.22a1 ID:20130718003001 c-c:3efb0311a21c m-c:f26e4c26ce4a

This trunk build says indeed that it is on the "nightly" channel. If aurora and beta builds are OK too, this bug can be VERIFIED.

Comment 21

[rsx11m]

Windows aurora nightly build 20130717013001 shows "aurora" again, thus marking verified as well. SM 2.20b2 should come early next week.

Comment 22

[rsx11m]

The about: page on the official Windows 2.20b2 build correctly reports "You are currently on the beta update channel" thus (with my "reporter" hat on) verified fixed for that branch now too.