1144256 - Thunderbird preferences chat.js uses preprocessor macros but isn't marked for preprocessing

Status: RESOLVED FIXED

(Thunderbird :: Preferences, defect)

Description

[:aceman]

Error: SyntaxError: illegal character
Source File: chrome://messenger/content/preferences/chat.js
Line: 66
Source Code:
#ifdef XP_MACOSX

Seems to be caused by bug 1141932. I wonder how the patch there could ever work :)

Comment 1

[:aceman]

Attachment: patch

Looks like the are more problems there. This fixes at least the JS errors. I didn't get any sound from the button (on Linux), but that is for somebody else to fix.

Comment 2

[Patrick Cloke [:clokep]]

Comment on attachment 8578809 [details] [diff] [review]
patch

This looks OK to me, I'm unsure if you still want aleth to take a look or not.

Comment 3

[aleth [:aleth]]

(In reply to :aceman from comment #1)
> Created attachment 8578809 [details] [diff] [review]
> patch
> 
> Looks like the are more problems there. This fixes at least the JS errors. I
> didn't get any sound from the button (on Linux), but that is for somebody
> else to fix.

sshagarwal, did you test your patch on Linux?

Comment 4

[:aceman]

Attachment: patch v2

As preprocessing makes life worse for devs, this version avoids it.

Comment 5

[aleth [:aleth]]

Comment on attachment 8578836 [details] [diff] [review]
patch v2

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

Good idea, thanks!

I'd still like sshagarwal to check this actually works on Linux though, and file a followup if not.

Comment 6

[Suyash Agarwal (:sshagarwal)]

(In reply to aleth [:aleth] from comment #3)
> (In reply to :aceman from comment #1)
> > Created attachment 8578809 [details] [diff] [review]
> > patch
> > 
> > Looks like the are more problems there. This fixes at least the JS errors. I
> > didn't get any sound from the button (on Linux), but that is for somebody
> > else to fix.
> 
> sshagarwal, did you test your patch on Linux?

No, I just tested it on Mac. I don't have a Linux machine now.

Comment 7

[Suyash Agarwal (:sshagarwal)]

Comment on attachment 8578836 [details] [diff] [review]
patch v2

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

::: mail/components/preferences/chat.js
@@ +65,5 @@
>      if (!soundLocation.startsWith("file://")) {
> +      if (Services.appinfo.OS == "Darwin") // OS X
> +        sound.beep();
> +      else
> +        sound.playEventSound(Ci.nsISound.EVENT_NEW_MAIL_RECEIVED);

Do we need Ci before nsISound?
http://mxr.mozilla.org/comm-central/source/suite/common/pref/preferences.js#68
doesn't have it.

Comment 8

[Suyash Agarwal (:sshagarwal)]

Okay, maybe its needed (other places use Ci for accessing constant values) but since suite didn't use it, I skipped it.

Thanks.

Comment 9

[:aceman]

(In reply to Suyash Agarwal (:sshagarwal) from comment #7)
> Comment on attachment 8578836 [details] [diff] [review]
> patch v2
> 
> Review of attachment 8578836 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/components/preferences/chat.js
> @@ +65,5 @@
> >      if (!soundLocation.startsWith("file://")) {
> > +      if (Services.appinfo.OS == "Darwin") // OS X
> > +        sound.beep();
> > +      else
> > +        sound.playEventSound(Ci.nsISound.EVENT_NEW_MAIL_RECEIVED);
> 
> Do we need Ci before nsISound?
> http://mxr.mozilla.org/comm-central/source/suite/common/pref/preferences.
> js#68
> doesn't have it.
Because that place has nsISound defined as a const.

In my patch it is visible that several lines above my change it is used as Ci.nsISound. There is no standalone nsISound defined.

Comment 10

[Kent James (:rkent)]

Comment on attachment 8578836 [details] [diff] [review]
patch v2

[Approval Request Comment]
I assume we want this in TB 38?

http://hg.mozilla.org/comm-central/rev/f5240eb961dc

Comment 11

[aleth [:aleth]]

(In reply to Kent James (:rkent) from comment #10)
> Comment on attachment 8578836 [details] [diff] [review]
> patch v2
> 
> [Approval Request Comment]
> I assume we want this in TB 38?

If you have a Linux machine handy, could you verify it works on Linux first? (as per comment 1)

Comment 12

[:aceman]

(In reply to aleth [:aleth] from comment #11)
> (In reply to Kent James (:rkent) from comment #10)
> > Comment on attachment 8578836 [details] [diff] [review]
> > patch v2
> > 
> > [Approval Request Comment]
> > I assume we want this in TB 38?
I think the regressing bug 1141932 was only for TB39, so no backporting needed.
 
> If you have a Linux machine handy, could you verify it works on Linux first?
> (as per comment 1)
The patch as a syntax error fix does work on Linux, that is where I made it.
Whether the sound actually plays is a different matter and should be filed independently. And made to block bug 1141932.

Comment 13

[Kent James (:rkent)]

Comment on attachment 8578836 [details] [diff] [review]
patch v2

Not needed in Thunderbird 38 per comment 12