Last Comment Bug 893547 - About SeaMonkey page claims to be on the "default" update channel for 2.20b1 and later
: About SeaMonkey page claims to be on the "default" update channel for 2.20b1 ...
Status: VERIFIED FIXED
: regression
Product: SeaMonkey
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.22
Assigned To: rsx11m
:
Mentors:
about:
Depends on:
Blocks: 735333 893740
  Show dependency treegraph
 
Reported: 2013-07-14 05:27 PDT by rsx11m
Modified: 2013-07-25 08:45 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
unaffected
verified
verified
verified


Attachments
Easy fix (1.04 KB, patch)
2013-07-14 09:04 PDT, rsx11m
ewong: feedback+
Details | Diff | Splinter Review
Alternative patch (2.36 KB, patch)
2013-07-14 09:32 PDT, rsx11m
no flags Details | Diff | Splinter Review
Proposed patch (v3) [check-in comment 14, comment 19] (2.34 KB, patch)
2013-07-14 19:50 PDT, rsx11m
neil: review+
iann_bugzilla: approval‑comm‑aurora+
iann_bugzilla: approval‑comm‑beta+
Details | Diff | Splinter Review

Description rsx11m 2013-07-14 05:27:40 PDT
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] 2013-07-14 08:01:09 PDT
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 2013-07-14 08:38:55 PDT
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 2013-07-14 09:04:07 PDT
Created attachment 775342 [details] [diff] [review]
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 2013-07-14 09:32:54 PDT
Created attachment 775344 [details] [diff] [review]
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 2013-07-14 09:53:17 PDT
(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 neil@parkwaycc.co.uk 2013-07-14 16:57:32 PDT
(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) 2013-07-14 17:37:46 PDT
(In reply to neil@parkwaycc.co.uk 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) 2013-07-14 18:05:07 PDT
Comment on attachment 775342 [details] [diff] [review]
Easy fix

I like this patch as opposed to the second one.
Comment 9 rsx11m 2013-07-14 19:50:24 PDT
Created attachment 775427 [details] [diff] [review]
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 2013-07-14 19:54:35 PDT
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 2013-07-15 04:42:06 PDT
> +      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 2013-07-15 05:22:10 PDT
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 2013-07-15 05:24:56 PDT
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 2013-07-15 06:56:32 PDT
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 2013-07-15 07:12:16 PDT
Thanks Phil. Did you leave the "checkin-needed" pending on purpose?
Comment 16 Philip Chee 2013-07-15 11:33:07 PDT
> Thanks Phil. Did you leave the "checkin-needed" pending on purpose?
Yes. Perhaps I should remove it pending approval?
Comment 17 rsx11m 2013-07-15 11:50:20 PDT
Let's remove it until the branch approvals are actually in to avoid confusion.
Comment 18 rsx11m 2013-07-15 15:38:43 PDT
Thanks for the approvals, reinstating checkin-needed for the branches.
Comment 20 Tony Mechelynck [:tonymec] 2013-07-18 08:10:28 PDT
(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 2013-07-18 08:26:15 PDT
Windows aurora nightly build 20130717013001 shows "aurora" again, thus marking verified as well. SM 2.20b2 should come early next week.
Comment 22 rsx11m 2013-07-25 08:45:34 PDT
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.

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