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

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
Internationalization
P3
normal
RESOLVED FIXED
15 years ago
12 years ago

People

(Reporter: Koike Kazuhiko, Assigned: masayuki)

Tracking

({intl})

Trunk
mozilla1.9alpha1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Description

15 years ago
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

Updated

15 years ago
Keywords: intl
In nsISound.idl, playSystemSound using string for the path.

> void playSystemSound(in string soundAlias);  

Should we use AUTF8String?
taking.
Assignee: smontagu → masayuki
OS: Windows XP → All
Priority: -- → P3
Hardware: PC → All
Target Milestone: --- → mozilla1.9alpha
Status: NEW → ASSIGNED
Created attachment 193694 [details] [diff] [review]
Patch rv1.0

This patch fixes this bug. But on Mac, the sound cannot be played by bug
194632.
Attachment #193694 - Flags: review?(jshin1987)
Depends on: 194632
Attachment #193694 - Flags: review?(jshin1987) → review-
Created attachment 193696 [details] [diff] [review]
Patch rv1.1

Sorry. I have a mistake.
Attachment #193694 - Attachment is obsolete: true
Attachment #193696 - Flags: review?(jshin1987)
Attachment #193696 - Flags: review?(jshin1987) → review-
Created attachment 193697 [details] [diff] [review]
Patch rv1.2

Sorry for the spam.
Attachment #193696 - Attachment is obsolete: true
Attachment #193697 - Flags: review?(jshin1987)
Attachment #193697 - Flags: review?(jshin1987) → review-
Created attachment 193699 [details] [diff] [review]
Patch rv1.3
Attachment #193697 - Attachment is obsolete: true
Attachment #193699 - Flags: review?(jshin1987)
I tested the patch on Win2k, TurboLinux10F+GTK2, GTK, XLIB, Qt.
And a contributor tested it on MacOS X.

Comment 8

12 years ago
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.
Attachment #193699 - Flags: review?(jshin1987)
Created attachment 193787 [details] [diff] [review]
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?
Attachment #193699 - Attachment is obsolete: true
Attachment #193787 - Flags: review?(timeless)
Attachment #193787 - Flags: review?(timeless) → review-
Created attachment 193788 [details] [diff] [review]
Patch rv1.4.1
Attachment #193787 - Attachment is obsolete: true
Attachment #193788 - Flags: review?(timeless)
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.
Attachment #193788 - Flags: review?(timeless) → review-
Created attachment 193900 [details] [diff] [review]
Patch rv2.0
Attachment #193788 - Attachment is obsolete: true
Attachment #193900 - Flags: review?(timeless)
timeless:

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

Comment 14

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

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

EqualsLiteral()

passing qnx review to amardare@qnx.com
Attachment #193900 - Flags: review?(timeless) → review?(amardare)
Attachment #193900 - Flags: review?(amardare) → review-
Created attachment 193934 [details] [diff] [review]
Patch rv2.1
Attachment #193900 - Attachment is obsolete: true
Attachment #193934 - Flags: review?(amardare)

Updated

12 years ago
Attachment #193934 - Flags: review?(amardare) → review+
Comment on attachment 193934 [details] [diff] [review]
Patch rv2.1

Jungshik:

Please re-review it.
Attachment #193934 - Flags: review?(jshin1987)
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

12 years ago
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
Attachment #193934 - Flags: review?(jshin1987) → review+
Created attachment 195742 [details] [diff] [review]
Patch rv2.2
Attachment #195742 - Flags: superreview?(roc)
Attachment #195742 - Flags: review+
Attachment #193934 - Attachment is obsolete: true
Attachment #195742 - Flags: superreview?(roc) → superreview+
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
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 21

12 years ago
is this still fixed? I don't hear a thing.. path to wav is
file:///C:/windows/Media/Windows%20XP%20-%20Pling.wav
Did you test with what build? This is only fixed Trunk(1.9a).

Comment 23

12 years ago
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051008
SeaMonkey/1.1a
And this bug didn't change the code for file://.
This only changed for native file path.

Comment 25

12 years ago
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
eh? nsWidgetsCID.h does not have any IIDs. this can't have broken it.
(at least not that part of the patch)

Comment 28

12 years ago
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).
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

12 years ago
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.
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.
You need to log in before you can comment on or make changes to this bug.