Charset detection function should be moved into mailnews/base/util/

RESOLVED FIXED in Thunderbird 11.0

Status

MailNews Core
Composition
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: hiro, Assigned: hiro)

Tracking

Trunk
Thunderbird 11.0
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
The function currently stays in mailnews/compose/src/nsMsgAttachmentHandler.cpp, but it is also needed for fixing bug 145293.
(Assignee)

Comment 1

6 years ago
Created attachment 575390 [details] [diff] [review]
Proposed fix

This new function is combined the original function in nsMsgAttachmentHandler.cpp and the function mozilla/dom/workers/FileReaderSyncPrivate.cpp.
Assignee: nobody → hiikezoe
Status: NEW → ASSIGNED
Attachment #575390 - Flags: review?(dbienvenu)
(Assignee)

Comment 2

6 years ago
Created attachment 575391 [details] [diff] [review]
Additional tests

Detection of UTF-16 and ShiftJis files.
Attachment #575391 - Flags: review?(mconley)
Comment on attachment 575391 [details] [diff] [review]
Additional tests

Review of attachment 575391 [details] [diff] [review]:
-----------------------------------------------------------------

hiro:

Looks pretty good! Just two things I noticed - see below.

Thanks,

-Mike

::: mailnews/compose/test/unit/test_detectAttachmentCharset.js
@@ +79,3 @@
>  {
>    var fileData = loadFileToString(gDraftFolder.filePath);
> +  dump(fileData + "\n");

We probably don't want this dump statement.

@@ +89,5 @@
> +  function createMessage1() { createMessage("data/test-UTF-8.txt"); },
> +  function checkAttachment1() { checkAttachment("UTF-8"); },
> +
> +  function createMessage2() { createMessage("data/test-UTF-16.txt"); },
> +  function checkAttachment2() { checkAttachment("UTF-16"); },

The file "test-UTF-16.txt" doesn't appear to be included in this, or the accompanying patch.  Did you forget it?
Attachment #575391 - Flags: review?(mconley) → review-
(Assignee)

Comment 4

6 years ago
Created attachment 576076 [details] [diff] [review]
Revised test

Remove a dump pointed out by mconley.

Thanks reviewing, Mike.

> The file "test-UTF-16.txt" doesn't appear to be included in this, or the
> accompanying patch.  Did you forget it?

The file is surely including in the attachment file. Splinter can not handle the file in UTF-16 or the attachment including several encodings. Anyway, you can see it in "Details" link.
Attachment #575391 - Attachment is obsolete: true
Attachment #576076 - Flags: review?(mconley)
Comment on attachment 576076 [details] [diff] [review]
Revised test

Yep, thanks - I see test-UTF-16.txt in the diff.  Good work!  r=me.
Attachment #576076 - Flags: review?(mconley) → review+

Comment 6

6 years ago
Comment on attachment 575390 [details] [diff] [review]
Proposed fix

thx for the patch and test
Attachment #575390 - Flags: review?(dbienvenu) → review+
Keywords: checkin-needed
http://hg.mozilla.org/comm-central/rev/aed166a284ff
http://hg.mozilla.org/comm-central/rev/60b1179ae6b9
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 11.0

Comment 8

6 years ago
Created attachment 578434 [details] [diff] [review]
--with-libxul-sdk fix
Attachment #578434 - Flags: review?(dbienvenu)

Updated

6 years ago
Attachment #578434 - Flags: review?(dbienvenu) → review+

Comment 9

6 years ago
Comment on attachment 578434 [details] [diff] [review]
--with-libxul-sdk fix

Pushed changeset f8fff89bfabc to comm-central.
You need to log in before you can comment on or make changes to this bug.