162523 - Java Script doesn't carry over the charset for the url containing non-ascii chars

Status: VERIFIED FIXED

(Core :: Internationalization, defect, P1)

Description

[ji]

This is another bug seperated from bug 155569. 
Java Script doesn't carry over the charset for the url containing non-ascii chars

Steps to reproduce: 
case 1:
copied over from bug 161174
1. From Netscape Mail client, send a gb2312 mail with a Chinese attachment
filename to a hotmail account. Please use doc attachment file so you can have a
download dialog on hotmail.
2. Login Hotmail from Chinese UI.
3. Download the attachment file from Hotmail account on browser.

The Chinese filename is shown as question marks on download file dialog.

Case 2.
Open attached html page, clicking on OK button, the Chinese filename is shown as
question marks on file save dialog. 

With the fix for bug 155569 (currently the fix is landed on the trunk), enter
the Chinese url directly on the URL bar, you can see the Chinese filename is
displayed correctly on file save dialog.

Comment 1

[ji]

Attachment: A gb2312 html page containing a javaScript invoking a Chinese URL.

Comment 2

[ji]

Nominating. This is a very common case that a user can easily come across.

Comment 3

[ji]

Attachment: Another gb2312 html test case w/o charset meta tag

Please set browser default charset to gb2312 and click on OK button.

Comment 4

[ji]

Jaime, Naoki already has a fix for this. I tested his personal build. It works.
Can we have an ADT keyword for this? I think this is a very important fix for
international users. Thanks.

Comment 5

[nhottanscp]

Attachment: Get a doc charset and use it for nsIURI and removed unnecessary convert/escape part.

Comment 6

[nhottanscp]

The existing code was written before nsIURI got originCharset. 
By setting originCharset to the doc charset, we do not need to apply charset
conversion and url escaping.

darin, could you review the patch?

Comment 7

[nhottanscp]

So the current problem happens because we don't set originCharset for the URI
also there is a bug in the code to escape 8 bit. The patch sets originCharset
and removed the 8 bit escape code.

Comment 8

[nhottanscp]

cc to jst, could you review the patch?

Comment 9

[Jaime Rodriguez, Jr.]

Let's move to get this landed, and verified on the trunk.

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 95152 [details] [diff] [review]
Get a doc charset and use it for nsIURI and removed unnecessary convert/escape part.

- In GetDocumentCharacterSetForURI(const nsAString& aHref, nsACString&
aCharset):
 {
-  aEscapedHref.Truncate(0);
-
   nsresult rv;
...

You should truncate aCharset at the beginning of this method, just to be sure
people don't mis-use it.

Other than that, sr=jst

Comment 11

[nhottanscp]

Attachment: Truncate aCharset.

Comment 12

[nhottanscp]

Comment on attachment 95165 [details] [diff] [review]
Truncate aCharset.

copy 'sr'

Comment 13

[Darin Fisher]

Comment on attachment 95165 [details] [diff] [review]
Truncate aCharset.

>Index: nsLocation.cpp

>+  aCharset.Assign(NS_LossyConvertUCS2toASCII(charset));

use this instead:

    CopyUCS2toASCII(charset, aCharset);

it avoids a string copy.

>+  if (IsASCII(aHref))
>     result = NS_NewURI(getter_AddRefs(newUri), aHref, nsnull, aBase);
>+  else {
>+    nsCAutoString docCharset;
>+    if (NS_SUCCEEDED(GetDocumentCharacterSetForURI(aHref, docCharset)))
>+      result = NS_NewURI(getter_AddRefs(newUri), aHref, docCharset.get(), aBase);
>+    else
>+      result = NS_NewURI(getter_AddRefs(newUri), aHref, nsnull, aBase);
>+  }

don't you still want to set the document charset even if the URL string is
ASCII?
otherwise, you won't be able to unescape the URL for display purposes, right? 
i
guess you're not wanting to query for the document charset each time because it
is expensive, right?  can we do anything to make it faster?

Comment 14

[nhottanscp]

>guess you're not wanting to query for the document charset each time because it
>is expensive, right?  can we do anything to make it faster?
I don't know other way to get the doc charset. But it may be okay to do it for
Ascii case too assuming we only deal with one link at a time.

jst, do you have any idea?

Comment 15

[Johnny Stenback (:jst)]

Performance isn't much of an issue here, this code would get called once per
setting window.location, so not a big deal at all.

Comment 16

[nhottanscp]

Attachment: Changed to CopyUCS2toASCII, and always get a doc charset.

Comment 17

[Johnny Stenback (:jst)]

Comment on attachment 95177 [details] [diff] [review]
Changed to CopyUCS2toASCII, and always get a doc charset.

sr=jst

Comment 18

[Darin Fisher]

Comment on attachment 95177 [details] [diff] [review]
Changed to CopyUCS2toASCII, and always get a doc charset.

r=darin (nice work!)

Comment 19

[nhottanscp]

Attachment: Adding a test case for escaped non ASCII.

Comment 20

[nhottanscp]

checked in to the trunk


The hotmail attachment problem (bug 161174) is still seen with a private build
with the patch. We need to reopen it if the problem still exists in tomorrow's
trunk.

Comment 21

[nhottanscp]

I reopened bug 161174 and added some info.

Comment 22

[ji]

Verified as fixed on 08/14 trunk build.
Hotmail attachment download problem is reported in bug 161174

Comment 23

[bobj]

Do we want to take these fixes (bug 161174 and bug 162523) for 1.0.1 if
there's a respin? 

We definitely need it for 1.0.x embedded clients.

Comment 24

[ji]

QA contact to Yuying for the verification on the branch build. Thanks.

Comment 25

[Frank Tang]

jaimejr, when should we land this into the m1.0 branch? how many approval we
need to got post machv?

Comment 26

[Jaime Rodriguez, Jr.]

adt1.0.1+ (on ADT's behalf) approval for checkin to the 1.0 branch, pending
Drivers' approval. pls check this in asap, the replace "Mozilla1.0.1+" with
"fixed1.0.1". [Using adt1.0.1 keywords as a proxy, since no edt1.0.1 keywords
were created for the 1.0.1 branch. This is needed on the 1.0 branch for a major
embedor]. thanks! 

Comment 27

[chris hofmann]

Comment on attachment 95177 [details] [diff] [review]
Changed to CopyUCS2toASCII, and always get a doc charset.

a=chofmann for 1.0.2

Comment 28

[Jaime Rodriguez, Jr.]

Marking as "mozilla1.0.2+" per Comment #27 From chris hofmann.

Comment 29

[Shanjian Li]

patch checked into 1.02 branch.

Comment 30

[Shanjian Li]

mark it as fixed1.0.2

Comment 31

[ji]

verified as fixed with 2002-08-28-08-1.0 build.