207102 - Sound preview doesn't work if its path contains non-ASCII string

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P3)

Description

[Koike Kazuhiko]

Sound preview doesn't work if its path contains non-ASCII string.

(1) Open Preferens - Privacy & Security - Popup Windows.
(2) Check "Play a sound" and select a file whose path contains
non-ASCII string.
(3) Hit Preview button.

Actual result: System default sound is played.
Expected result: Selected sound is played.

Reproduced with 2003052508-trunk/WinXP.

Original report in Bugzilla-jp:
http://bugzilla.mozilla.gr.jp/show_bug.cgi?id=3178

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

In nsISound.idl, playSystemSound using string for the path.

> void playSystemSound(in string soundAlias);  

Should we use AUTF8String?

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

taking.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.0

This patch fixes this bug. But on Mac, the sound cannot be played by bug
194632.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.1

Sorry. I have a mistake.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.2

Sorry for the spam.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.3

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I tested the patch on Win2k, TurboLinux10F+GTK2, GTK, XLIB, Qt.
And a contributor tested it on MacOS X.

Comment 8

[Jungshik Shin]

Comment on attachment 193699 [details] [diff] [review]
Patch rv1.3

>Index: mailnews/base/src/nsStatusBarBiffManager.cpp

>Index: widget/src/gtk/nsSound.cpp

>+#include "nsNativeCharsetUtils.h"

>+  NS_ConvertUTF8toUTF16 utf16SoundAlias(aSoundAlias);
>+  nsCAutoString nativeSoundAlias;
>+  NS_CopyUnicodeToNative(utf16SoundAlias, nativeSoundAlias);
>+
>   // create a nsILocalFile and then a nsIFileURL from that
>   nsCOMPtr <nsILocalFile> soundFile;
>-  rv = NS_NewNativeLocalFile(nsDependentCString(aSoundAlias), PR_TRUE, 
>+  rv = NS_NewNativeLocalFile(nativeSoundAlias, PR_TRUE, 
>                              getter_AddRefs(soundFile));

You don't need NS_CopyUnicodeToNative + NS_NewNativeLocalFile. Pls, just use
NS_NewLocalFile. The same is true in other cases.

Btw, I'm not sure what you did for photon is correct. Ask timeless.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.4

timeless:

Please review for photon. I don't know photon. I'm using UTF-8 string for
|access| and |PtSpawn| in this patch. Is it correct?

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.4.1

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 193788 [details] [diff] [review]
Patch rv1.4.1

+  nsresult rv = GetSoundResourceName(nsAFlatCString(aSoundName).get(),

use PromiseFlatCString instead of nsAFlatCString (various times)

have you considered making nsISound take an AString instead of an AUTF8String?
it looks like that'd be easier for most of these implementations.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.0

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

timeless:

Please check photon code is correct or not. If you allow this code, I retry
jshin's review. thanks.

Comment 14

[timeless]

Comment on attachment 193900 [details] [diff] [review]
Patch rv2.0

+    if (aSoundAlias.Equals(NS_LITERAL_STRING("_moz_mailbeep"))) {

EqualsLiteral()

passing qnx review to [e-mail]

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.1

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 193934 [details] [diff] [review]
Patch rv2.1

Jungshik:

Please re-review it.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

On MacOS X, the sound is not played. But it's another bug. See bug 191614.
On Win2000 and Linux(GTK and GTK2), this patch works fine.

Comment 18

[Jungshik Shin]

Comment on attachment 193934 [details] [diff] [review]
Patch rv2.1


>Index: widget/src/mac/nsSound.cpp

>+#include "nsNativeCharsetUtils.h"
>
>-  nsresult rv = GetSoundResourceName(aSoundName, soundResource);
>+  nsCAutoString nativeSoundName;
>+  NS_CopyUnicodeToNative(aSoundName, nativeSoundName);
>+  nsresult rv = GetSoundResourceName(nativeSoundName.get(),
>+                                     soundResource);

On OS X, the native encoding is UTF-8 so that it's better to do this:

.... GetSoundResouceName(NS_ConvertUTF16toUTF8(aSoundName).get(),
soundResource); 

With that, r=jshin

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.2

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in.

Checking in mailnews/base/src/nsStatusBarBiffManager.cpp;
/cvsroot/mozilla/mailnews/base/src/nsStatusBarBiffManager.cpp,v  <-- 
nsStatusBarBiffManager.cpp
new revision: 1.36; previous revision: 1.35
done
Checking in widget/public/nsISound.idl;
/cvsroot/mozilla/widget/public/nsISound.idl,v  <--  nsISound.idl
new revision: 1.12; previous revision: 1.11
done
Checking in widget/src/beos/nsSound.cpp;
/cvsroot/mozilla/widget/src/beos/nsSound.cpp,v  <--  nsSound.cpp
new revision: 1.20; previous revision: 1.19
done
Checking in widget/src/gtk/nsSound.cpp;
/cvsroot/mozilla/widget/src/gtk/nsSound.cpp,v  <--  nsSound.cpp
new revision: 1.42; previous revision: 1.41
done
Checking in widget/src/gtk2/nsSound.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsSound.cpp,v  <--  nsSound.cpp
new revision: 1.13; previous revision: 1.12
done
Checking in widget/src/mac/nsSound.cpp;
/cvsroot/mozilla/widget/src/mac/nsSound.cpp,v  <--  nsSound.cpp
new revision: 1.41; previous revision: 1.40
done
Checking in widget/src/os2/nsSound.cpp;
/cvsroot/mozilla/widget/src/os2/nsSound.cpp,v  <--  nsSound.cpp
new revision: 1.28; previous revision: 1.27
done
Checking in widget/src/photon/nsSound.cpp;
/cvsroot/mozilla/widget/src/photon/nsSound.cpp,v  <--  nsSound.cpp
new revision: 1.24; previous revision: 1.23
done
Checking in widget/src/qt/nsSound.cpp;
/cvsroot/mozilla/widget/src/qt/nsSound.cpp,v  <--  nsSound.cpp
new revision: 1.2; previous revision: 1.1
done
Checking in widget/src/windows/nsSound.cpp;
/cvsroot/mozilla/widget/src/windows/nsSound.cpp,v  <--  nsSound.cpp
new revision: 1.37; previous revision: 1.36
done
Checking in widget/src/xlib/nsSound.cpp;
/cvsroot/mozilla/widget/src/xlib/nsSound.cpp,v  <--  nsSound.cpp
new revision: 1.11; previous revision: 1.10
done

Comment 21

[R.K.Aa.]

is this still fixed? I don't hear a thing.. path to wav is
file:///C:/windows/Media/Windows%20XP%20-%20Pling.wav

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Did you test with what build? This is only fixed Trunk(1.9a).

Comment 23

[R.K.Aa.]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051008
SeaMonkey/1.1a

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

And this bug didn't change the code for file://.
This only changed for native file path.

Comment 25

[massey]

Is there any reason the the uuid was changed here but not in nsWidgetsCID.h ??

-[scriptable, uuid(B148EED1-236D-11d3-B35C-00A0CC3C1CDE)]
+[scriptable, uuid(B01ADAD7-D937-4738-8508-3BD5946BF9C8)]

This seems to break @mozilla.org/sound;1

I found this while looking into async sound bug 250186

Comment 26

[Christian :Biesinger (don't email me, ping me on IRC)]

eh? nsWidgetsCID.h does not have any IIDs. this can't have broken it.

Comment 27

[Christian :Biesinger (don't email me, ping me on IRC)]

(at least not that part of the patch)

Comment 28

[massey]

from http://lxr.mozilla.org/mozilla/source/widget/public/nsWidgetsCID.h#173

172 // {B148EED2-236D-11d3-B35C-00A0CC3C1CDE}
173 #define NS_SOUND_CID \
174 { 0xb148eed2, 0x236d, 0x11d3, { 0xb3, 0x5c, 0x0, 0xa0, 0xcc, 0x3c, 0x1c, 0xde } }

But this patch defines [scriptable, uuid(B01ADAD7-D937-4738-8508-3BD5946BF9C8)]
So one or the other needs changing I think. It should be reverted(just that uuid).

Comment 29

[Christian :Biesinger (don't email me, ping me on IRC)]

CIDs and IIDs are different. I have no idea why they used to be the same here. They certainly don't have to. And the change should ABSOLUTELY NOT be reverted. (at best, the nsWidgetsCID.h entry should be changed to match)

Comment 30

[massey]

Ok I revert the change to the nsISound uuid and its is usable from js again. see comment 28

I do not undersatand why this breakage should not be reverted. Going to nsWidgetsCID.h to fix what this patch breaks seems wrong.

Comment 31

[Christian :Biesinger (don't email me, ping me on IRC)]

if an interface changes, its IID must change. that is a basic COM rule to ensure binary compatibility.

I don't understand why reverting the IID change can fix anything. it doesn't seem like a right fix.