Closed
Bug 162523
Opened 23 years ago
Closed 23 years ago
Java Script doesn't carry over the charset for the url containing non-ascii chars
Categories
(Core :: Internationalization, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla1.0.1
People
(Reporter: ji, Assigned: nhottanscp)
References
Details
(Keywords: intl, topembed, Whiteboard: [adt2 RTM] [ETA 08/21] [Land on 1.0 branch post MachV])
Attachments
(4 files, 2 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
darin.moz
:
review+
jst
:
superreview+
chofmann
:
approval+
|
Details | Diff | Splinter Review |
(deleted),
text/html
|
Details |
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.
Nominating. This is a very common case that a user can easily come across.
Please set browser default charset to gb2312 and click on OK button.
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.
Assignee | ||
Comment 5•23 years ago
|
||
Assignee | ||
Comment 6•23 years ago
|
||
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?
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•23 years ago
|
||
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.
Assignee | ||
Comment 8•23 years ago
|
||
cc to jst, could you review the patch?
Comment 9•23 years ago
|
||
Let's move to get this landed, and verified on the trunk.
Comment 10•23 years ago
|
||
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
Attachment #95152 -
Flags: superreview+
Assignee | ||
Comment 11•23 years ago
|
||
Attachment #95152 -
Attachment is obsolete: true
Assignee | ||
Comment 12•23 years ago
|
||
Comment on attachment 95165 [details] [diff] [review]
Truncate aCharset.
copy 'sr'
Attachment #95165 -
Flags: superreview+
Comment 13•23 years ago
|
||
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?
Assignee | ||
Comment 14•23 years ago
|
||
>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•23 years ago
|
||
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.
Assignee | ||
Comment 16•23 years ago
|
||
Attachment #95165 -
Attachment is obsolete: true
Comment 17•23 years ago
|
||
Comment on attachment 95177 [details] [diff] [review]
Changed to CopyUCS2toASCII, and always get a doc charset.
sr=jst
Attachment #95177 -
Flags: superreview+
Comment 18•23 years ago
|
||
Comment on attachment 95177 [details] [diff] [review]
Changed to CopyUCS2toASCII, and always get a doc charset.
r=darin (nice work!)
Attachment #95177 -
Flags: review+
Assignee | ||
Comment 19•23 years ago
|
||
Assignee | ||
Comment 20•23 years ago
|
||
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.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 21•23 years ago
|
||
I reopened bug 161174 and added some info.
Reporter | ||
Comment 22•23 years ago
|
||
Verified as fixed on 08/14 trunk build.
Hotmail attachment download problem is reported in bug 161174
Status: RESOLVED → VERIFIED
Comment 23•23 years ago
|
||
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.
Updated•22 years ago
|
Blocks: 154896
Whiteboard: [adt2 RTM] [ETA 08/15] → [adt2 RTM] [ETA 08/21] [Land on 1.0 branch post MachV]
Reporter | ||
Comment 24•22 years ago
|
||
QA contact to Yuying for the verification on the branch build. Thanks.
QA Contact: ji → ylong
Comment 25•22 years ago
|
||
jaimejr, when should we land this into the m1.0 branch? how many approval we
need to got post machv?
Comment 26•22 years ago
|
||
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•22 years ago
|
||
Comment on attachment 95177 [details] [diff] [review]
Changed to CopyUCS2toASCII, and always get a doc charset.
a=chofmann for 1.0.2
Attachment #95177 -
Flags: approval+
Updated•22 years ago
|
Keywords: mozilla1.0.1 → mozilla1.0.1+
Comment 28•22 years ago
|
||
Marking as "mozilla1.0.2+" per Comment #27 From chris hofmann.
Keywords: mozilla1.0.1+ → mozilla1.0.2+
Comment 29•22 years ago
|
||
patch checked into 1.02 branch.
Reporter | ||
Comment 31•22 years ago
|
||
verified as fixed with 2002-08-28-08-1.0 build.
Keywords: fixed1.0.2 → verified1.0.2
You need to log in
before you can comment on or make changes to this bug.
Description
•