314231 - If link target URL has non-ASCII char that is not encoded by UTF-8, the default file name is always escaped at "Save Link Target As..."

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect, P1)

Description

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

If link target URL has non-ASCII char that is not encoded by UTF-8, the default
file name is always escaped at "Save Link Target As..."

The test case is here.
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2931&action=view

You may look Japanese default file name only on the "href has encoded by
UTF-8".
On other links, you can look the escaped default file name.

# This is SeaMonkey version of bug 314222

Comment 1

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

Attachment: Patch rv1.0

This fix this bug.

aDocument is always null in initFileInfo if it's called by "Save Link Target As...".
We should use current document charset instead of aDocument's charset if
aDocument is null.

Comment 2

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

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

hm... would it be possible to instead pass an nsIURI to initFileInfo and have the caller take care of the charset?

Comment 3

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

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

r- until I get a reply to comment 2

Comment 4

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

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

Oops. Sorry for the delay.

> would it be possible to instead pass an nsIURI to initFileInfo and have
> the caller take care of the charset?

I cannot understand well.
The caller(internalSave) does not have document. Because when "Save Link Target As...", the document is not loaded because the user doesn't move to linked target. Therefore, we need current document charset by referrer.

Comment 5

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

Ah, I understan what you said.

Of course, it's possible. But current code make nsIURI in the initFileInfo.
I obeied current code.

Comment 6

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

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

hrm, ok, but then why not just pass the charset to this function, instead of the entire referrer?

Comment 7

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

Isn't my patch more readable than sending charset directly?

My params means,
if document is not null(so, the URI is loaded), we should use the doc charset.
otherwise(so, the URI is not loaded), if referrer is not null(so, it means previous document charset), we should use the charset.
otherwise, we assume that the charset is UTF-8 from general rule.

Comment 8

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

biesi:

Please continue to review.

Comment 9

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

This bug is reproduced on Web Mail Service.
This is serious i18n bug. Please hurry up to review.

Comment 10

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

from mconnor,
>  ------- Comment #5 From Mike Connor  2005-12-16 11:35 PST  [reply] -------
> 
> (From update of attachment 201154 [details] [diff] [review] [edit])
>>+ * @param aReferrer the referrer URI object (not URL string) to use, or null
>>+ *        if no referrer should be sent.
> 
> this comment is bogus, since we're simply passing the referrer to extract the
> charset, just use something like this:
> 
> * @param aReferrer An nsIURI object for the referrer document, null if there is
> no referrer

I'll change this comment also in XPFE's patch.

Comment 11

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

as this function does not use the referrer, it would imo be better to always pass a charset to it and use that. the caller can decide whether to pass the referrer's origin charset or the document's charset. that way, the caller need not make up a referrer in case it doesn't have one.

Comment 12

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

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

so I'll mark this review-.

Comment 13

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

Attachment: Patch rv2.0

Comment 14

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

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

thanks, and sorry again about the delay

Comment 15

[[e-mail]]

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

>   var sourceURL = getContentFrameURI(focusedWindow);
>+  var sourceDocument = getContentFrameDocument(focusedWindow);
> 
>   try {
>-    return makeURI(sourceURL);
>+    return makeURI(sourceURL, sourceDocument.characterSet);
Nit: you could replace sourceURL with sourceDocument.location.href thus saving you the call to getContentFrameURI

>+    var charset = aDocument ? aDocument.characterSet : null;
>+    if (!aDocument && aReferrer)
>+      charset = aReferrer.originCharset;
This is clumsy... I'd use a double ?: or an if/else if/else to select between aDocument.characterSet and aReferrer.originCharset (or if you're feeling really leet, aDocument && aDocument.characterSet || aReferrer && aReferrer.originCharset).

Comment 16

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

>>+    var charset = aDocument ? aDocument.characterSet : null;
>>+    if (!aDocument && aReferrer)
>>+      charset = aReferrer.originCharset;
> This is clumsy... I'd use a double ?: or an if/else if/else to select between
> aDocument.characterSet and aReferrer.originCharset (or if you're feeling really
> leet, aDocument && aDocument.characterSet || aReferrer &&
> aReferrer.originCharset).

Thanks,

My current patch is correctly. We should use aReferrer only if aDocument is null.
# Therefore, my first patch was wrong.

Comment 17

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

Attachment: Patch for check-in

Comment 18

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

checked-in.

Comment 19

[Benjamin Smedberg]

Can anyone identify whether this patch affects tbird at all? Tbird does use the xpfe contentAreaUtils, but I can't tell from basic examination whether these functions are being used.

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 206517 [details] [diff] [review]
Patch for check-in

a=dveditz for drivers

Comment 21

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

Thank you everyone!

Comment 22

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Cancelling blocking-seamonkey1.0 request since this is already fixed.