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

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

The function currently stays in mailnews/compose/src/nsMsgAttachmentHandler.cpp, but it is also needed for fixing bug 145293.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Proposed fix

This new function is combined the original function in nsMsgAttachmentHandler.cpp and the function mozilla/dom/workers/FileReaderSyncPrivate.cpp.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: Additional tests

Detection of UTF-16 and ShiftJis files.

Comment 3

[Mike Conley (: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?

Comment 4

[Hiroyuki Ikezoe (:hiro)]

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

Comment 5

[Mike Conley (: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.

Comment 6

[David :Bienvenu]

Comment on attachment 575390 [details] [diff] [review]
Proposed fix

thx for the patch and test

Comment 7

[Makoto Kato [:m_kato]]

http://hg.mozilla.org/comm-central/rev/aed166a284ff
http://hg.mozilla.org/comm-central/rev/60b1179ae6b9

Comment 8

[[e-mail]]

Attachment: --with-libxul-sdk fix

Comment 9

[[e-mail]]

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

Pushed changeset f8fff89bfabc to comm-central.