314222 - 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 As..."

Status: VERIFIED FIXED

(Firefox :: File Handling, defect, P1)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

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 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.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Attachment: Patch rv1.0

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

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

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

Oops. This patch doesn't work at Alt + Click.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Attachment: Patch rv1.1

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

This bug is major. Because on Web Mail Service, the attached file has non-ASCII name, this bug is occured. This bug is very serious i18n bug for marketing.
But I'm waiting biesi's review in bug 314231. We should check in same patch to both xpfe and toolkit.

Comment 5

[Mike Connor [:mconnor]]

Comment on attachment 201154 [details] [diff] [review]
Patch rv1.1

>+ * @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

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Thank you. But I wait XPFE's review too.
I'll check-in this patch and bug 314231's one in same time.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Attachment: Patch rv2.0

Updtated by biesi's review in bug 314231.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Attachment: Patch for check-in

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

checked-in.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Mozilla Japan needs to be fixed on 1.8 branch too.
This bug is very serious for Web Mail Service. See comment 4.

Comment 11

[Benjamin Smedberg]

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

I don't think that we should be changing the signature of initFileInfo() on the stable branch.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

benjamin:

But this is very serious regression from 1.0.x. See comment 4.

Comment 13

[Benjamin Smedberg]

Masayuki, that's fine, but we've made a commitment not to change APIs (even non-frozen ones) on the stable branch. We could add a *new* function signature or perhaps add the charset as an optional argument on the end of this function signature without changing the API.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Umm... Should I recreate new patch only for 1.8.0 branch that has another name function? (e.g., initFileInfo2())

# We can use current patch on 1.8 branch, right?

Comment 15

[Benjamin Smedberg]

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

Yes, the current patch is fine for 1.8.1. I'll bring up the 1.8.0.x issue in the drivers meeting and get back to you.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Thank you Benjamin. Please check bug 314231 too.

Comment 17

[Daniel Veditz [:dveditz]]

Could this be rewritten to put the charset parameter at the end? Then it could be treated optionally and not break anyone else using it.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Attachment: Patch for 1.8.0.x

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Daniel:

Please check the new patch.

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 207798 [details] [diff] [review]
Patch for 1.8.0.x

a=dveditz for drivers

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Thank you everyone!

Comment 22

[Dink McCallister]

I just pooped my pants.. please help me clean myself up. I believe that it is all mozilla/firefox's fault cause I was doing some reverse-engineering with the beta browser, to try to make it toilet/thundermug compatible, and all of a sudden I lost complete control of my bowels with the brain-band synch hookup to the laptop and the toilet.  My poop was supposed to go straight to the toilet, but instead it coated all two layers of my below-waist clothing -- plus down both of my pant legs.  Obviousely, I had to spoon all of the brown lava into my mouth with a serving spoon until I could fit my pants and shorts back on without it dripping all over the floor, ( I wouldn't dare getting kah-kah on my bearskin rug ).  I am not really complaining -- as I should have taken extra precautions (like maybe connecting my bowel oriface to the toilet/thundermug), before trying a teleport of such nature -- however I am offering my experience as information to you for the purpose of improving mozilla, firefox.  Take it easy ya'll.. 

Mary McGlachenflachensteinbruner