Closed Bug 893547 Opened 6 years ago Closed 6 years ago

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

Categories

(SeaMonkey :: General, defect)

defect
Not set

Tracking

(seamonkey2.19 unaffected, seamonkey2.20 verified, seamonkey2.21 verified, seamonkey2.22 verified)

VERIFIED FIXED
seamonkey2.22
Tracking Status
seamonkey2.19 --- unaffected
seamonkey2.20 --- verified
seamonkey2.21 --- verified
seamonkey2.22 --- verified

People

(Reporter: rsx11m.pub, Assigned: rsx11m.pub)

References

()

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

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).
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."
OS: Windows 7 → All
Hardware: x86_64 → All
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".
Attached patch Easy fix (obsolete) — Splinter Review
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).
Attachment #775342 - Flags: review?(neil)
Attached patch Alternative patch (obsolete) — Splinter Review
This changes the XUL label text to be better recognizable on failure and removes the try/catch block as proposed.
Attachment #775344 - Flags: review?(neil)
(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.)
(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.
Flags: needinfo?(ewong)
(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').
Flags: needinfo?(ewong)
Comment on attachment 775342 [details] [diff] [review]
Easy fix

I like this patch as opposed to the second one.
Attachment #775342 - Flags: feedback+
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.
Assignee: nobody → rsx11m.pub
Attachment #775342 - Attachment is obsolete: true
Attachment #775344 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #775342 - Flags: review?(neil)
Attachment #775344 - Flags: review?(neil)
Attachment #775427 - Flags: review?(neil)
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).
Attachment #775342 - Attachment is obsolete: false
Attachment #775342 - Flags: review?(neil)
Attachment #775427 - Flags: review?(neil) → review+
Attachment #775342 - Flags: review?(neil)
> +      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
Blocks: 893740
Attachment #775342 - Attachment is obsolete: true
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.
Keywords: checkin-needed
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
Attachment #775427 - Flags: approval-comm-beta?
Attachment #775427 - Flags: approval-comm-aurora?
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
Attachment #775427 - Attachment description: Proposed patch (v3) → Proposed patch (v3) [check-in comment 14]
Whiteboard: [leave open for comm-aurora and comm-beta]
Target Milestone: --- → seamonkey2.22
Thanks Phil. Did you leave the "checkin-needed" pending on purpose?
> Thanks Phil. Did you leave the "checkin-needed" pending on purpose?
Yes. Perhaps I should remove it pending approval?
Let's remove it until the branch approvals are actually in to avoid confusion.
Keywords: checkin-needed
Attachment #775427 - Flags: approval-comm-beta?
Attachment #775427 - Flags: approval-comm-beta+
Attachment #775427 - Flags: approval-comm-aurora?
Attachment #775427 - Flags: approval-comm-aurora+
Thanks for the approvals, reinstating checkin-needed for the branches.
Keywords: checkin-needed
Whiteboard: [leave open for comm-aurora and comm-beta] → [push for comm-aurora and comm-beta]
Pushed to branches.
remote:   https://hg.mozilla.org/releases/comm-aurora/rev/dfdbe6c067b1
remote:   https://hg.mozilla.org/releases/comm-beta/rev/ef499ee2aa1d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [push for comm-aurora and comm-beta]
Attachment #775427 - Attachment description: Proposed patch (v3) [check-in comment 14] → Proposed patch (v3) [check-in comment 14, comment 19]
(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.
Windows aurora nightly build 20130717013001 shows "aurora" again, thus marking verified as well. SM 2.20b2 should come early next week.
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.