Last Comment Bug 703503 - Charset detection function should be moved into mailnews/base/util/
: Charset detection function should be moved into mailnews/base/util/
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Composition (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 11.0
Assigned To: Hiroyuki Ikezoe (:hiro)
:
Mentors:
Depends on:
Blocks: 145293
  Show dependency treegraph
 
Reported: 2011-11-17 21:44 PST by Hiroyuki Ikezoe (:hiro)
Modified: 2011-12-03 23:49 PST (History)
3 users (show)
m_kato: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed fix (10.03 KB, patch)
2011-11-17 21:49 PST, Hiroyuki Ikezoe (:hiro)
mozilla: review+
Details | Diff | Review
Additional tests (4.45 KB, patch)
2011-11-17 21:51 PST, Hiroyuki Ikezoe (:hiro)
mconley: review-
Details | Diff | Review
Revised test (4.43 KB, patch)
2011-11-21 20:28 PST, Hiroyuki Ikezoe (:hiro)
mconley: review+
Details | Diff | Review
--with-libxul-sdk fix (1.95 KB, patch)
2011-12-01 16:11 PST, neil@parkwaycc.co.uk
mozilla: review+
Details | Diff | Review

Description Hiroyuki Ikezoe (:hiro) 2011-11-17 21:44:43 PST
The function currently stays in mailnews/compose/src/nsMsgAttachmentHandler.cpp, but it is also needed for fixing bug 145293.
Comment 1 Hiroyuki Ikezoe (:hiro) 2011-11-17 21:49:51 PST
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.
Comment 2 Hiroyuki Ikezoe (:hiro) 2011-11-17 21:51:16 PST
Created attachment 575391 [details] [diff] [review]
Additional tests

Detection of UTF-16 and ShiftJis files.
Comment 3 Mike Conley (:mconley) - (needinfo me!) 2011-11-21 06:54:42 PST
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) 2011-11-21 20:28:14 PST
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.
Comment 5 Mike Conley (:mconley) - (needinfo me!) 2011-11-22 06:14:17 PST
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 2011-11-22 15:46:33 PST
Comment on attachment 575390 [details] [diff] [review]
Proposed fix

thx for the patch and test
Comment 8 neil@parkwaycc.co.uk 2011-12-01 16:11:20 PST
Created attachment 578434 [details] [diff] [review]
--with-libxul-sdk fix
Comment 9 neil@parkwaycc.co.uk 2011-12-03 14:35:50 PST
Comment on attachment 578434 [details] [diff] [review]
--with-libxul-sdk fix

Pushed changeset f8fff89bfabc to comm-central.

Note You need to log in before you can comment on or make changes to this bug.