309335 - Unknown content type dialog(helper app dialog) shows escaped file name if it has non-ASCII char

Status: VERIFIED FIXED

(Core :: Internationalization, defect, P1)

Description

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

Unknown content type dialog shows escaped file name if it has non-ASCII char.

Comment 1

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

Attachment: screenshot

Comment 2

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

Attachment: Patch rv1.0

Maybe, this bug is happen on the os that file scheme isn't encoded by UTF-8.
I don't know this patch is correct.
result of nsIPlatformCharset::GetCharset and file scheme encoding is always
same?

Comment 3

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

This bug is important for using non-ASCII charcters users.
Please block next release.

Comment 4

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

I can reproduce on Turbo Linux.

Comment 5

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

This patch works fine on Linux too. But I cannot test on Mac.

Comment 6

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

On Mac, we use UTF-8 encoding for file scheme's URL encode. That is same as
result of nsIPlatformCharset::GetCharset(kPlatformCharsetSel_FileName). The
patch may work fine on Mac too.

Comment 7

[Mike Connor [:mconnor]]

biesi, can you review this soon if jshin doesn't get it quickly?  Looks
relatively straightforward to me...

Comment 8

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

OK, so this conflicts with bug 278161.

Why not make the file protocol handler set the origin charset to whatever the
URL is really encoded in?

Comment 9

[Boris Zbarsky [:bzbarsky]]

That would make the most sense... Does it not, right now?

Comment 10

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

Currently, nsIURI's |GetOriginCharset| returns always "UTF-8". Maybe, the
memeber is empty when it is file scheme.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Who's creating this URI?  The newURI method takes a charset param, so it sounds
like someone's just not passing it in...

Comment 12

[Boris Zbarsky [:bzbarsky]]

If nothing else, note
http://lxr.mozilla.org/seamonkey/source/netwerk/protocol/file/src/nsFileProtocolHandler.cpp#336
-- does fixing that help?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 196803 [details] [diff] [review]
Patch rv1.0

This is wrong.	It'd just wallpaper over one place where things show up wrong
instead of fixing the real issue.

Comment 14

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

Attachment: Patch rv2.0

Comment 15

[Boris Zbarsky [:bzbarsky]]

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

That seems wrong to me... If I init a file:// URI with a UTF-8 filename string,
why would we use the platform charset here, exactly?

Also, I think this should be handled by the relevant protocol handler instead
of scattering random checks for "file" all over.  What about the other
file-based schemes, for example?

Comment 16

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

Should I insert the code in nsStandardURL::SetFile?
In this case, the spec is not encoded by UTF-8 on non-UTF-8 system(Win, Linux...).

Comment 17

[Boris Zbarsky [:bzbarsky]]

Either in SetFile or in the callers of SetFile, depending on who the callers
are, etc.  At least as I understand this stuff.  You really want Darin's review
on changes to that neck of the woods, though....

Comment 18

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

The callers of SetFile cannot change the charset. Because the charset is read
only attribute on nsIURI. I think that we can only insert to
nsStandardFile::SetFile.

# I will create new patch tomorrow. Because now 07:20 AM in Japan...I will sleep.

Comment 19

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

nsStandardFile::SetFile -> nsStandardURL::SetFile

Comment 20

[Darin Fisher]

So, the origin charset value should always be taken from the charset of the
document where the file:// link was extracted.  If the user enters the file://
link manually in the URL bar, or if the file:// link comes in through the
command line then we have to guess the origin charset.  From the bug report, it
is not clear to me how the file:// URL entered the system.

Comment 21

[Darin Fisher]

One more thing though.  It is also true that file:// URLs are today always
encoded in the platform charset.  Eventually, we should move to UTF-8 for all
file:// URLs, but we shouldn't wait for that to happen.  It does seem reasonable
to me given the way nsStandardURL::SetFile works today that we might just modify
nsStandardURL::SetFile to set mOriginCharset to the platform charset since that
is  effectively what net_GetURLSpecFromFile does.

Comment 22

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

(In reply to comment #16)
> In this case, the spec is not encoded by UTF-8 on non-UTF-8 system(Win, Linux...).

(today's linux distributions usually do use UTF-8 for filenames)

Comment 23

[Darin Fisher]

right, this is mainly an issue on windows.  linux and osx mainly use UTF-8, so
it's not an issue there.

Comment 24

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

> (today's linux distributions usually do use UTF-8 for filenames)

In Japan, not so. VineLinux and TurboLinux are using EUC-JP. These distributions
are used in Japan.

Comment 25

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

> From the bug report, it
> is not clear to me how the file:// URL entered the system.

I can reproduced when the file is dropped to Firefox/Suite and the dettached
files are opened by Thunderbird.

Comment 26

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

Attachment: Patch rv3.0

I cannot fix by this patch. We may not use nsFileProtocolHandler::NewFileURI...

Comment 27

[Scott MacGregor]

Making a change like this to nsStandardURL this late in the release cycle scares
me greatly. 

masayuki, can you confirm if this is a regression over 1.0 behavior or did we
ship with this in 1.0?

Comment 28

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

This is not a regression.

Cannot we check-in the first patch to only 1.8 branch? I think that the first
patch has low risk.

Comment 29

[Boris Zbarsky [:bzbarsky]]

No, the first patch is not acceptable to me as module owner, not on trunk nor
even more so not on branches.

Comment 30

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

I think this problem is serious on thunderbird only. Because we support
detaching the attachment in the mail. This problem is reproduced only detached
file has intl name, not reproduced the file is not detached.

Comment 31

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

Do you have any idea for simple fix?
Maybe, the 'detach' function makes a little confuse the users in non-ASCII area.

This is not a regression for Tb.
But this problem is reproduced only with Tb 1.5. Because we don't have 'detach'
in Tb1.0.

Comment 32

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

Attachment: Patch rv4.0

Comment 33

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

why are both changes required?

Comment 34

[Darin Fisher]

Comment on attachment 197766 [details] [diff] [review]
Patch rv4.0

>Index: netwerk/base/src/nsStandardURL.cpp
...
>     rv = net_GetURLSpecFromFile(file, url);
>     if (NS_FAILED(rv)) return rv;
> 
>+    // We should always override the charset until bug 278161 is fixed.
>+    mOriginCharset.Truncate();
>+    nsCOMPtr <nsIPlatformCharset> platformCharset =
>+        do_GetService(NS_PLATFORMCHARSET_CONTRACTID, &rv);
>+    if (NS_SUCCEEDED(rv)) {
>+        rv = platformCharset->GetCharset(kPlatformCharsetSel_FileName,
>+                                         mOriginCharset);
>+        if (NS_FAILED(rv) || (mOriginCharset.Length() > 3 &&
>+                              IsUTFCharset(mOriginCharset.get()))) {
>+            mOriginCharset.Truncate();
>+        }
>+    }
>+
>     rv = SetSpec(url);
> 
>     // must clone |file| since its value is not guaranteed to remain constant
>     if (NS_SUCCEEDED(rv)) {

I think it would be better to call Init, so that you do not need
to duplicate the mOriginCharset normalization for UTF-8 values.

For the defaultPort and urlType parameters, just pass mDefaultPort
and mURLType.  Pass nsnull as the baseURI parameter.


>Index: netwerk/protocol/file/src/nsFileProtocolHandler.cpp
...
>@@ -264,21 +265,32 @@ nsFileProtocolHandler::NewURI(const nsAC
...
>+    nsCAutoString urlCharset(charset);
>+    if (urlCharset.IsEmpty()) {
>+        // We should set file system charset until bug 278161 is fixed.
>+        nsresult rv;
>+        nsCOMPtr <nsIPlatformCharset> platformCharset =
>+            do_GetService(NS_PLATFORMCHARSET_CONTRACTID, &rv);
>+        if (NS_SUCCEEDED(rv)) {
>+            rv = platformCharset->GetCharset(kPlatformCharsetSel_FileName,
>+                                             urlCharset);
>+            if (NS_FAILED(rv))
>+                urlCharset.Truncate();
>+        }
>+    }
> 
>     nsresult rv = url->Init(nsIStandardURL::URLTYPE_NO_AUTHORITY, -1,
>-                            *specPtr, charset, baseURI);
>+                            *specPtr, urlCharset.get(), baseURI);

I think we always want to override the origin charset here since
an unescaped file:// URL should always be in the platform charset.
So, checking if |charset| is empty seems wrong to me.

Here's a trick that you can use here to avoid copying charset
into urlCharset needlessly:

    nsCAutoString buf;
    // We should set file system charset until bug 278161 is fixed.
    nsCOMPtr <nsIPlatformCharset> platformCharset =
	    do_GetService(NS_PLATFORMCHARSET_CONTRACTID);
    if (platformCharset) {
	platformCharset->GetCharset(kPlatformCharsetSel_FileName, buf);
	if (!buf.IsEmpty())
	    charset = buf.get();
    }

    nsresult rv = url->Init(nsIStandardURL::URLTYPE_NO_AUTHORITY, -1,
			    *specPtr, charset, baseURI);


> nsFileProtocolHandler::NewFileURI(nsIFile *file, nsIURI **result)
...
>-    // XXX shouldn't we set nsIURI::originCharset ??
>-
>     rv = url->SetFile(file);
>     if (NS_FAILED(rv)) return rv;
> 
>     return CallQueryInterface(url, result);
> }

I think you should change this XXX comment to a NOTE indicating
that the origin charset is assigned the value of the platform
charset by the SetFile method.

Comment 35

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

Attachment: Patch rv4.1

Comment 36

[Benjamin Smedberg]

I'm afraid of touching the file protocol handler this late in the game... darin,
what do you think about the branch-worthiness of this not causing any regressions?

Comment 37

[Darin Fisher]

Comment on attachment 197862 [details] [diff] [review]
Patch rv4.1

r+sr=darin

Comment 38

[Scott MacGregor]

Darin, I'm nervous about changing nsStandardURL this late, even though I really
want this fix for Thunderbird. Can you comment on your confidence / risk level
for the patch? 

Masayuki, please get this landed on the trunk ASAP.

Comment 39

[Darin Fisher]

Let me encourage you to take this patch.  I think it is pretty safe.  The
changes only apply to file:// URLs, and then only to file:// URLs containing
escape sequences, which is nearly exclusive to non-english operating systems. 
Moreover, it is in very few codepaths that we actually care about the
originCharset property.

Comment 40

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

checked-in to trunk and 1.8branch.
Thank you everyone.

Comment 41

[tor]

This checkin seems to have caused the orange on the brad tinderbox.  I'm also
seeing a startup crash on my linux fc4 box, which goes away if I back out this
patch.  The crash doesn't happen if the build is started under the debugger,
which makes tracking down the cause a bit trickier.

Comment 42

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

tor:

Is the crash occured with only suite? Firefox doesn't happen?

Comment 43

[tor]

My build is seamonkey.  Haven't built firefox up on this tree yet.

Comment 44

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

Can you attach your build options?

Comment 45

[tor]

mk_add_options MOZ_CO_PROJECT=suite,browser
ac_add_options --enable-application=suite
ac_add_options --disable-tests
ac_add_options --disable-mailnews
ac_add_options --disable-ldap
ac_add_options --disable-composer
ac_add_options --disable-mathml
ac_add_options --disable-accessibility
ac_add_options --disable-installer
ac_add_options --disable-xpinstall
ac_add_options --enable-svg
ac_add_options --enable-canvas

Comment 46

[tor]

Built firefox and see the crash there as well, again fixed by backing out this
patch.  Configuration for my firefox build (optimized, no debug):

mk_add_options MOZ_CO_PROJECT=browser
ac_add_options --enable-application=browser
ac_add_options --disable-tests
ac_add_options --enable-svg
ac_add_options --enable-canvas

Comment 47

[tor]

Curiously, looking at the coredumps and valgrind output, it's dying inside
unrelated code in nsFrame.

(gdb) where
#0  0x40000402 in __kernel_vsyscall ()
#1  0x00b9c28a in raise () from /lib/libpthread.so.0
#2  0x08055e96 in nsProfileLock::FatalSignalHandler ()
#3  <signal handler called>
#4  0x409e50c9 in nsFrame::BoxReflow ()
   from /home/mozilla/trunk/opt-ff/dist/bin/components/libgklayout.so
#5  0x409e54fb in nsFrame::DoLayout ()
   from /home/mozilla/trunk/opt-ff/dist/bin/components/libgklayout.so
#6  0x40ac4c92 in nsIFrame::Layout ()
   from /home/mozilla/trunk/opt-ff/dist/bin/components/libgklayout.so
#7  0x40ac60a9 in nsBoxFrame::LayoutChildAt ()
   from /home/mozilla/trunk/opt-ff/dist/bin/components/libgklayout.so
#8  0x409f1123 in nsGfxScrollFrameInner::LayoutScrollbars ()
   from /home/mozilla/trunk/opt-ff/dist/bin/components/libgklayout.so
#9  0x409f1cf4 in nsHTMLScrollFrame::Reflow ()
   from /home/mozilla/trunk/opt-ff/dist/bin/components/libgklayout.so
#10 0x409df57b in nsContainerFrame::ReflowChild ()
   from /home/mozilla/trunk/opt-ff/dist/bin/components/libgklayout.so
#11 0x40a297af in ViewportFrame::Reflow ()
   from /home/mozilla/trunk/opt-ff/dist/bin/components/libgklayout.so
#12 0x409c2eb5 in PresShell::InitialReflow ()
   from /home/mozilla/trunk/opt-ff/dist/bin/components/libgklayout.so
#13 0x40afea17 in nsContentSink::StartLayout ()
   from /home/mozilla/trunk/opt-ff/dist/bin/components/libgklayout.so
#14 0x40bb42a0 in HTMLContentSink::StartLayout ()
   from /home/mozilla/trunk/opt-ff/dist/bin/components/libgklayout.so
#15 0x40bb7a08 in HTMLContentSink::OpenBody ()
   from /home/mozilla/trunk/opt-ff/dist/bin/components/libgklayout.so
...

Comment 48

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

Umm... I cannot reproduce with config of comment 45 on TurboLinux.

tor:

Can you reproduce after dist clean? And can you repduce on 1.8 branch?

Comment 49

[tor]

My trunk firefox build was clean, as I don't normally build that product.  A
clean build of firefox on the branch also shows the startup crash, which again
goes away if I back out this patch.

Comment 50

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

Always crashed in nsFrame::BoxReflow?

I think this patch doesn't have a crash bug. Because other tinderbox are green.
If we back out this patch, we will not show the crash. But is it correct? We(MJ)
don't want to back out this patch. Because this bug is important for our
marketing. Isn't there another way?

Comment 51

[tor]

Always seems to be in nsFrame::Reflow.  I'd suggest trying backing it out on
trunk to see if brad goes green.

There have a few other bugzilla reports today of firefox failing to start - bug
310686, bug 310688, and bug 310703.

Comment 52

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

O.K. I backed out the patch from trunk only.

Comment 53

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

backed out from 1.8branch too.

Comment 54

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

Why this patch is caused to crash on some systems?

Comment 55

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

I can reproduce on Linux after comment out nsFileProtocolHandler::NewURI's
changing. I'm checking the code.

Comment 56

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

Maybe, I think that file protocol is used by platformCharset initializing on Linux.

Comment 57

[Boris Zbarsky [:bzbarsky]]

Yeah, nsPlatformCharset::InitGetCharset in nsUNIXCharset.cpp tries to read a
properties file, which means creating a file:// URI.

Perhaps we should do this charset thing only if the filename is non-ascii?  I
think that would solve the problem...

Comment 58

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

> Perhaps we should do this charset thing only if the filename is non-ascii?

Yes, it's simple approach. But I don't link this approach. Because the codes are
required two or more cpp files. I want to use it for final approach. I'm trying
on another approach that is using global variable.

Thank you.

Comment 59

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

Mac's yesterday crash may be caused by this bug.
nsMacCharset.cpp is using file protocol too.

Comment 60

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

Attachment: Patch rv5.0

I'm testing this patch.

Comment 61

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

Comment on attachment 198230 [details] [diff] [review]
Patch rv5.0

This patch works fine on my linux environment. And on windows too.

Comment 62

[Scott MacGregor]

Comment on attachment 197862 [details] [diff] [review]
Patch rv4.1

clearing approval request since this patch had some problems and is now
obsolete.

Comment 63

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

Someone:

Could you test the patch on your Linux or Mac system?

Comment 64

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

Also on Windows too.

Comment 65

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

Comment on attachment 198230 [details] [diff] [review]
Patch rv5.0

This has same problem on Linux.

Comment 66

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

Attachment: Patch rv5.1

Comment 67

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

(In reply to comment #57)
> Perhaps we should do this charset thing only if the filename is non-ascii?  I
> think that would solve the problem...

That also requires checking if any escaped characters are present, does it not?

Comment 68

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

Yes, we need to check it.

Comment 69

[Asa Dotzler [:asa]]

If we don't have a fully reviewed patch approved and landed by midnight tonight
(PDT) this isn't going to make 1.5

Comment 70

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

Comment on attachment 198321 [details] [diff] [review]
Patch rv5.1

A Japanese tester said this patch doesn't crash on Linux when starting and
opening the file that having non-ASCII char.

Comment 71

[Darin Fisher]

Comment on attachment 198321 [details] [diff] [review]
Patch rv5.1

>Index: netwerk/base/src/nsStandardURL.cpp

>+nsCString nsStandardURL::gFileProtocolCharset;

Global (or class static) nsCString is not permitted.  It may not work
properly on some systems.


>+const char* nsStandardURL::kFileProtocolCharset = "MOZ_FPC";

What is this?  I think you need to document this.


>+        nsCAutoString fpCharset;
>+        platformCharset->GetCharset(kPlatformCharsetSel_FileName, fpCharset);
>+        gFileProtocolCharset = fpCharset;

Since gFileProtocolCharset is of type nsCString, you could have just used
it directly, like this:

	  platformCharset->GetCharset(kPlatformCharsetSel_FileName,
				      gFileProtocolCharset);


>+    else if (mOriginCharset.Equals(kFileProtocolCharset)) {
>+        if (gFileProtocolCharset.IsEmpty() &&
>+            (!IsASCII(spec) ||
>+             nsPromiseFlatCString(spec).FindChar('%', 0) != kNotFound)) {

This needs to be properly documented.  I'm also not very happy with
this approach, but I can't think of a good alternative.


>Index: netwerk/protocol/file/src/nsFileProtocolHandler.cpp

>     nsresult rv = url->Init(nsIStandardURL::URLTYPE_NO_AUTHORITY, -1,
>+                            *specPtr, nsStandardURL::kFileProtocolCharset,
>+                            baseURI);

Ah, I see... this kFileProtocolCharset thing really needs to be better
documented.


I wonder if there isn't a better way to work around the initialization
"chicken and the egg" problem.	We could perhaps solve it by pushing the
normalization of "MOZ_FPC" into GetOriginCharset.  Hmm...

Comment 72

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

(In reply to comment #71)
> We could perhaps solve it by pushing the
> normalization of "MOZ_FPC" into GetOriginCharset.  Hmm...
> 

We need mOriginCharset for BuildNormalizedSpec. Not only for GetOriginCharset.

Comment 73

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

Attachment: Patch rv5.2

Comment 74

[Darin Fisher]

Another solution to this bug would be to make the nsIPlatformCharset
implementations not depend on being able to load URLs.  They could just use the
directory service and nsIFile to load the properties files.

I know we don't have a lot of time for this release, but the current patch
(rv5.2) is really hacky.  I wish we could do something better like fix
nsIPlatformCharset.

Comment 75

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

Yes, we don't have much time. But this is very serious bug for Thunderbird
marketing in Japan. We need to fix this...

Comment 76

[Darin Fisher]

OK, nevermind hacking the platform charset implementation.  That seems beyond
the scope of this bug.  I still wish we could do something less hacky here.

Comment 77

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

Should I recreate smaller patch?
I have no idea for smaller and more safety patch...

Comment 78

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

Attachment: possible patch for the platformcharset problem

maybe something like this? this isn't finished yet, but it works on windows
already. if people are interested I can finish the patch.

Comment 79

[Darin Fisher]

biesi: Yeah, that is exactly the kind of patch I was thinking of!  it would make
it possible to use Masayuki's v4.1 patch.  Do you think you can finish that patch?

Comment 80

[Darin Fisher]

Attachment: v6 patch

OK.  This patch includes a complete implementation of nsGREResProperties.  It
also has Masayuki's v4.1 patch.

Comment 81

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 198518 [details] [diff] [review]
v6 patch

>Index: intl/uconv/src/Makefile.in
> 		nsURLProperties.cpp \

Remove that line, since you're removing those files?

And remove the references (unused?) to nsURLProperties from
nsWinUConvService.cpp

r=bzbarsky with that.

Comment 82

[Darin Fisher]

I asked dougt about nsWinUConvService, and this is what he had to say:

> [15:36] darinf19: yt?
> [15:37] darinf19: you know about nsWinUConvService.cpp, right?
> [15:37] darinf19: it seems like there are no makefile rules to build it, and 
> it looks like it wouldn't compile/link if there were.  is this something that 
> can or should be cvs removed?
> [16:12] *** "darinf19" signed off at Tue Oct 04 16:12:51 2005.
> 
> Yeah, it was Alec's attempt at making a native uconv.  I looked at using it, >
but decided to do my own for WinCE.  I do not think that the one I wrote can >
be used as a general purpose Win32 converter as mine makes the assumption 
> that the backend is always Unicode (which is only true for WinCE).
> 
> I think we can cvs remove nsWinUConvService. 

Comment 83

[Darin Fisher]

Comment on attachment 198518 [details] [diff] [review]
v6 patch

We decided to keep this patch out of 1.5 beta 2 with agreement that this would
go in for 1.5 rc1.  Please approve :)

Comment 84

[Darin Fisher]

reassigning to me so I can more easily keep track of getting this checked in.

Comment 85

[Asa Dotzler [:asa]]

Can we get this landed on the trukn and verified as working there and causing no
regressions?

Comment 86

[Darin Fisher]

(In reply to comment #85)
> Can we get this landed on the trukn and verified as working there and causing no
> regressions?

Working on that now.

Comment 87

[Darin Fisher]

fixed-on-trunk

Comment 88

[Darin Fisher]

Attachment: v6.1 patch [fixed-on-trunk]

This is the version of the patch that I landed on the trunk.

Comment 89

[Asa Dotzler [:asa]]

Masayuki, can you verify that this is fixed on the trunk and look for any
possible regressions related to this change?

Comment 90

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

This has a regression. This patch breakes nsMsgCompose::AttachmentPrettyName.
However, this regression may be fixed by bug 274264(current patch).
Bug 274264 should block rc release.

I cannot find other regression(s) on Firefox and Thunderbird.

Comment 91

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

I verified the regression is fixed by current patch of bug 274264.
Let's fix this and bug 274264!

-> v.

Comment 92

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

Bug 274264 is fixed on 1.8 branch. We don't have regression by this patch.

Drain:
Please check in to Branch!

Comment 93

[Darin Fisher]

fixed1.8

Comment 94

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

I confirmed this is fixed on 1.8 branch. Thank you!