194067 - support unicode file name on drag and drop(D&D)

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[kaleida]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.2.1) Gecko/20021130
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.2.1) Gecko/20021130

When you drag and drop onto Mozilla URLs/Internet Shortcuts (created using
Internet Explorer 6.0) that contain any foreign characters in the title, Mozilla
refuses to open them.

Reproducible: Always

Steps to Reproduce:
1. Open the following Web page:
     http://www.linux.ru
using Internet Explorer 6.0 and add this Web page to Favorites.
2. Drag and drop this new Favorite/Internet Shortcut  onto Mozilla 1.2.1
3. The following error message will be displayed:
“Alert
The file /C:/Documents and Settings/user/Favorites/Linux.?? ? ????????? ??
??????? - ?????? Linux.url cannot be found. Please check the location and try again.
OK”


Actual Results:  
The following error message will be displayed:
“Alert
The file /C:/Documents and Settings/user/Favorites/Linux.?? ? ????????? ??
??????? - ?????? Linux.url cannot be found. Please check the location and try again.
OK”



Expected Results:  
Mozilla should have opened the specified Web page, since the URL was valid. (It
does work in Internet Explorer 6.0).

Comment 1

[timeless]

ok, this happens in 2/10-08

I tested with にほん.url and בּת.url both fail.

Comment 2

[Steve Chapel]

How is this different from bug 185818?

Comment 3

[Steve Chapel]

*** Bug 185818 has been marked as a duplicate of this bug. ***

Comment 4

[marina]

changing component to Intl

Comment 5

[marina]

Yuying, do you see this problem with the latest trunk builds? Thanks.

Comment 6

[Yuying Long]

I saw this on 04-09 trunk build / WinXP-JA:
Some unicode characters titled pages (e.g. Chinese or Korean on Japanese system)
has this problem.  But no problem with Japanese or French titled pages.

Comment 7

[Yuying Long]

> Some unicode characters titled pages (e.g. Chinese or Korean on 
> Japanese system) has this problem.  But no problem with Japanese or 
> French titled pages.

Correction: the problem happens on all non-native non-ascii charaters.

Comment 8

[kaleida]

This bug still has not been fixed, even in Mozilla 1.4 for Windows.
I am using Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4) Gecko/20030624

Comment 9

[Jungshik Shin]

There's a fix for this(see bug 162361), but that has been put on hold because 
we haven't yet decided what to do with  NSPR file IO APIs (necessary to fix this) 

Comment 10

[kaleida]

This bug is still present in Mozilla 1.5.

Comment 11

[Jungshik Shin]

This should be fixed on the (firefox) trunk build thanks to my check-in of the patch for bug 162361. Anybody would be welcome to test it and verify it as fixed if it's indeed fixed.

Comment 12

[Masatoshi Kimura [:emk]]

Unfortunately we can't still open dropped Internet Shortcuts with Unicode name after bug 162361 and bug 278161 are fixed.
We will have to handle CFSTR_SHELLIDLIST per masayuki.

Comment 13

[Jungshik Shin]

(In reply to comment #12)
> Unfortunately we can't still open dropped Internet Shortcuts with Unicode name
> after bug 162361 and bug 278161 are fixed.
> We will have to handle CFSTR_SHELLIDLIST per masayuki.

You're right. I mixed up this issue with importing MS IE favorites. 

Comment 14

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

Yeah, I have a patch for supporting the Unicode on D&D.
But the patch is not completed. I'll create the patch after return to Japan.

Comment 15

[Masatoshi Kimura [:emk]]

Attachment: Patch rv 1.0

I have a much simpler idea.
We can simply replace ::DragQueryFileA with ::DragQueryFileW because we dropped support for Win9x.

Comment 16

[Ere Maijala (slow)]

Comment on attachment 217662 [details] [diff] [review]
Patch rv 1.0

I'd like to use this chance to make the code more readable using some nice string stuff we have. I know there is old code like that, but could you use nsString and its friends (beginWriting, ToNewUnicode)?

+  PRUint32 len = nsCRT::strlen(inFileName);
+  if ( (len > 4) && nsCRT::IsAscii(&inFileName[len - 4])
+                 && ((char)inFileName[len - 4] == '.')
+                 && (nsCRT::ToLower((char)inFileName[len - 3]) == 'u')
+                 && (nsCRT::ToLower((char)inFileName[len - 2]) == 'r')
+                 && (nsCRT::ToLower((char)inFileName[len - 1]) == 'l') )

This would be much nicer with StringEndsWith using nsCaseInsensitiveStringComparator.

+                    *aLen = fileNameLen * 2;

Use sizeof(PRUnichar) instead of 2. It's much more indicative on what the meaning is (and future proof).

-        nsMemory::Free ( NS_REINTERPRET_CAST(char*, data) );        
+        nsMemory::Free ( NS_REINTERPRET_CAST(char*, data) );

Data would be PRUnichar, no? But do you actually need the cast at all?

+      PRUnichar* dest = NS_REINTERPRET_CAST(PRUnichar*, pDropFile + 1);

|pDropFile + 1| should be |pDropFile + pDropFile->pFiles|. 

+      const PRUnichar* source = pFileName;
+      memcpy(dest, source, (allocLen - 1) * 2);    // copies the null character in pFileName as well

Variable "source" is unnecessary.

Comment 17

[Masatoshi Kimura [:emk]]

Attachment: Patch rv 1.1

Sorry, I tried to replace |PRUnichar*| with |nsString|, but I don't understand where to use beginWriting or ToNewUnicode.

> |pDropFile + 1| should be |pDropFile + pDropFile->pFiles|. 

Due to magical C pointer arithematic, |pDropFile + 1| will add |sizeof(DROPFILES)| to the internal pointer value. And |pDropFile->pFiles| is always set to |sizeof(DROPFILES)|.
If it's preferable to type |pDropFile->pFiles| explicitly, it should be like:
PRUnichar* dest = NS_REINTERPRET_CAST(PRUnichar*, NS_REINTERPRET_CAST(char*, pDropFile) + pDropFile->pFiles);
I think it's too lengthy.

Other nits are resolved.

Comment 18

[Ere Maijala (slow)]

(In reply to comment #17)
> Due to magical C pointer arithematic, |pDropFile + 1| will add
> |sizeof(DROPFILES)| to the internal pointer value. And |pDropFile->pFiles| is
> always set to |sizeof(DROPFILES)|.
> If it's preferable to type |pDropFile->pFiles| explicitly, it should be like:
> PRUnichar* dest = NS_REINTERPRET_CAST(PRUnichar*, NS_REINTERPRET_CAST(char*,
> pDropFile) + pDropFile->pFiles);
> I think it's too lengthy.

Right, but I prefer the lengthy way for readability and safety (what if somethign changes and pDropFiles isn't sizeof(DRPOFILES) anymore?).

Comment 19

[Masatoshi Kimura [:emk]]

Attachment: Patch rv 1.2

Comment 20

[Masatoshi Kimura [:emk]]

Ere:
Could you review the patch? I changed your point in comment #18.
Do I need also using new string API? Honestly, I have no idea how to rewrite the code.

Comment 21

[Ere Maijala (slow)]

Comment on attachment 218604 [details] [diff] [review]
Patch rv 1.2

It's fine for me.

Comment 22

[Masatoshi Kimura [:emk]]

Comment on attachment 218604 [details] [diff] [review]
Patch rv 1.2

Requesting sr.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 218604 [details] [diff] [review]
Patch rv 1.2

+      PRUnichar* dest = NS_REINTERPRET_CAST(PRUnichar*, NS_REINTERPRET_CAST(char*, pDropFile) + pDropFile->pFiles);

I agree with Masatoshi and would just go with pDropFile + 1, but this is OK.

Comment 24

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

checked-in.

Comment 25

[Jungshik Shin]

Masayuki, I think we still need a patch for the branch (that should work on Win9x/ME. Of course, on Win 9x/ME, non-system-codepage char. wouldn't be supported). Can you do that (perhaps in a separate bug filed specifically for 1.8 branch) as you indicated in comment #14? 

Comment 26

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

(In reply to comment #25)
> Masayuki, I think we still need a patch for the branch (that should work on
> Win9x/ME. Of course, on Win 9x/ME, non-system-codepage char. wouldn't be
> supported). Can you do that (perhaps in a separate bug filed specifically for
> 1.8 branch) as you indicated in comment #14? 
> 

I file a new bug that is bug 336225. I'll work on that ASAP.
# Kimura-san:
# If you want to work on that, please take it :-)

Comment 27

[Masatoshi Kimura [:emk]]

(In reply to comment #26)
> # Kimura-san:
> # If you want to work on that, please take it :-)
OK, I'll take it.

Comment 28

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

thanks.