Closed Bug 206252 Opened 21 years ago Closed 21 years ago

'send page/image' assume non-ASCII filenames are always in ISO-8859-1

Categories

(MailNews Core :: Internationalization, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.6final

People

(Reporter: jshin1987, Assigned: jshin1987)

References

()

Details

(Keywords: intl)

Attachments

(4 files, 1 obsolete file)

When inline image is sent by email via 'send image' menu, 
non-ISO-8859-1 file names are treated as if they're ISO-8859-1.

The filename parameter of Content-Disposition header should
use either UTF-8 or |originCharset| of the URL.   

I thought there was a bug filed for this, but couldn't find it. 
If it's dupe, I'm sorry.
I'm changing the summary line because 'send page' (arguably more important than
'send image') is also affected.


Related to this is bug 193439. In cleaning up mime attachment code(bug 86089),
one attribute (|originCharset| of |url|) was not added to |nsIMsgAttachment|.
And, in mailNavigatorOverlay.xul[1], 'url' is passed unescaped. Instead of
adding |originCharset| attrib. to nsIMsgAttachment and changing mailnews code to
'infer' the correct filename from url and originCharset, we could just set
|name| attrib. of nsIMsgAttachment in xpfe code ( similar to what's done in
contentAreaUtils.js [2]), but it may be easier to do that on the other side. 

[1]http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/content/mailNavigatorOverlay.xul#112
[2]http://lxr.mozilla.org/seamonkey/source/xpfe/communicator/resources/content/contentAreaUtils.js#765
Assignee: smontagu → jshin
Summary: images with non-iso-8859-1 filename attached via 'send image' are always treated at ISO-8859-1 → 'send page/image' assume non-ASCII filenames are always in ISO-8859-1
test case added to the URL field.
The page is in EUC-KR and has a link to a PDF file with the filename in EUC-KR.
 Selecting 'send page' in the context menu brings up a mail composition window
with the attachment name wrong (EUC-KR misinterpreted as ISO-8859-1)
Attached patch preliminary patch (working) (obsolete) — Splinter Review
this patch works, but needs some clean-up.
basically the same, but changed the interface slightly
Attachment #136816 - Attachment is obsolete: true
Comment on attachment 136886 [details] [diff] [review]
patch (cleaned-up)

asking for r/sr.
This is kinda needed for bug 227547. (it's possible to separate two patches,
but...)
Attachment #136886 - Flags: superreview?(mscott)
Attachment #136886 - Flags: review?(bienvenu)
Comment on attachment 136886 [details] [diff] [review]
patch (cleaned-up)

this looks OK - my one question is - there are several places where you use
NS_ENSURE_SUCCESS(rv, rv) after string conversions. Is that always the right
thing to do since presumably it just aborts the attach operation? Could those
conversions fail for a reason that should cause us just to pass through the
original name?
Attachment #136886 - Flags: review?(bienvenu) → review+
Thank you for r.

I'm not sure how much sense a couple of fallbacks I added make sense. Depending
on how you look at it, the downright failure may be considered better than
using 'garbage-like' names...

I'm fine with either way. bienvenu and mscott, let me know which one you like.


BTW, I switched to ACString(from string) for 'urlCharset' to simplify
getter/setter.
Comment on attachment 136979 [details] [diff] [review]
a patch with fallback (alternative)

r=bienvenu - I prefer the fallback code if it's at all meaningful, but I leave
it up to you to decide if it's helpful.
Attachment #136979 - Flags: review+
Attachment #136886 - Flags: superreview?(mscott)
Comment on attachment 136979 [details] [diff] [review]
a patch with fallback (alternative)

Mocing sr request to this patch.

Thanks for the prompt review.
Because I'm not inclined either way, you prefer this way, and I found at least
one case where the fallback code is useful (when one has file names in a
character encoding different from the default filesystem charset), I'll go with
the fallback code.
Attachment #136979 - Flags: superreview?(mscott)
let's try to get this in before 1.6final. 

mscott, can you take a look?
Blocks: 227547
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.6final
Comment on attachment 136979 [details] [diff] [review]
a patch with fallback (alternative)

let's off-load this from mscott.
sspitzer, would you care to review?
Attachment #136979 - Flags: superreview?(mscott) → superreview?(sspitzer)
Comment on attachment 136979 [details] [diff] [review]
a patch with fallback (alternative)

I can review it.
Attachment #136979 - Flags: superreview?(sspitzer) → superreview+
Comment on attachment 136979 [details] [diff] [review]
a patch with fallback (alternative)

mscott, thanks for sr.

now asking for a1.6final.

affected platforms : all

affected users : anyone wishing to attach html documents/images with non-Latin1
names via 'context-menu' to emails 

the patch was tested on Linux and Win32. 

Risk : low (with fallback code paths). In a sense, the patch is a
'prerequisite' of fixing bug 227547 (a rather high-profile bug on Mac OS X).
Comment on attachment 136979 [details] [diff] [review]
a patch with fallback (alternative)

Sorry for spamming (I pressed the wrong button).

mscott, thanks for sr.

now asking for a1.6final.

affected platforms : all

affected users : anyone wishing to attach html documents/images with non-Latin1
names via 'context-menu' to emails 

the patch was tested on Linux and Win32. 

Risk : low (with fallback code paths). In a sense, the patch is a
'prerequisite' of fixing bug 227547 (a rather high-profile bug on Mac OS X).
Attachment #136979 - Flags: approval1.6?
Asa, can you help drive this?  Thanks,

/be
Flags: blocking1.6+
Comment on attachment 136979 [details] [diff] [review]
a patch with fallback (alternative)

a=asa (on behalf of drivers) for checkin to Mozilla 1.6
Attachment #136979 - Flags: approval1.6? → approval1.6+
fix checked into the trunk.
thanks you all.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Flags: blocking1.6+
Resolution: --- → FIXED
This patch made a core API change that to mail compose APIs. The change was
never propogated to thunderbird which is now broken. Please always check
mozilla/mail mozilla/toolkit, mozilla/browser when making API changes per the
new tinderbox rules. The partitions are all open now. Thanks! :)

Patch coming up on for mozilla/mail.
I checked in a fix for thunderbird bustage. 
I missed this (as found in bug 227547).
Oops. I didn't go down to read new comments. Sorry for spamming abd TB bustage.
BTW, I think AttachmentPrettyName had beeter be called with 'null' as the second
parameter (as in my patch) in TB because attachment.urlCharset is never set in TB. 
This patch seems to have broken Firebird's "Send Image" menu item, you've
changed the line in browser.js from sendLink to sendImage but FB doesn't use
mailNavigatorOverlay.xul so it has no access to sendImage (it just sends the URL
of the image by using a mailto generated from sendLink in browser.js).

Could you look into this as soon as possible (since I think FB 0.8 is supposed
to be released shortly).

p.s. And if I'm wrong and this problem has nothing to do with this bug I
apologise in advance.
Correction: These changes were made just after Firebird branched so this is not
a problem for FB 0.8, it only needs fixing on the trunk.
Thanks for the note. Can you try reverting my patch for browser.js and see if
that solves the problem? 
(In reply to comment #24)
> Thanks for the note. Can you try reverting my patch for browser.js and see if
> that solves the problem? 

Yes reverting back fixes the Send Image item.
this is to back out what shouldn't have been included in attachment 136886 [details] [diff] [review].
jshin, I think this fix may have caused Bug #229872 if I'm reading the comments
in that bug correctly. We are no longer showing attachments correctly in the
message compose attachment window if they have non ascii characters in them. Do
you have time to take a look?
Comment on attachment 140127 [details] [diff] [review]
one liner to back out firebird part 

bryner, can you review this ? I don't mind if you just go ahead and land it.
Attachment #140127 - Flags: review?(bryner)
Attachment #140127 - Flags: review?(bryner) → review+
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: