GetMsgTextFromStream should use more libmime

RESOLVED FIXED in mozilla1.9.1a2

Status

RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: sid0, Assigned: sid0)

Tracking

Trunk
mozilla1.9.1a2
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

10 years ago
Created attachment 331194 [details] [diff] [review]
patch v1

GetMsgTextFromStream right now uses its own header parsing -- libmime can definitely be used for this instead.

There are also several tests I wrote to test this. The in and out files are very bare, containing just the necessary headers to test. One nice side effect of this is that there are now several emails of different kinds that can be used as fodder for tests -- more headers can probably be added without severely breaking this test.

Requesting r+sr from bienvenu for main function changes, r from Standard8 for tests.

Please make sure that
1. all the test data files have CRLF line endings
2. the out files do not have a newline at the end

both are currently necessary for the test to work.
Attachment #331194 - Flags: superreview?(bienvenu)
Attachment #331194 - Flags: review?(bienvenu)
(Assignee)

Updated

10 years ago
Attachment #331194 - Flags: review?(bugzilla)
Product: Core → MailNews Core
Comment on attachment 331194 [details] [diff] [review]
patch v1

>diff --git a/mailnews/base/public/nsIMsgFolder.idl b/mailnews/base/public/nsIMsgFolder.idl

This file is missing a uuid change.


>+function run_test()


>+    var inStream = Cc["@mozilla.org/network/file-input-stream;1"].createInstance(Ci.nsIFileInputStream);

nit: wrap these lines please (I suggest putting the .createInstance on the next line, and aligning the . with the [

>+    var outFile = do_get_file(kDataRoot + test.name + ".out");
>+    var outStream = Cc["@mozilla.org/network/file-input-stream;1"].createInstance(Ci.nsIFileInputStream);
>+    outStream.init(outFile, -1, -1, Ci.nsIFileInputStream.CLOSE_ON_EOF);

This is confusing, you've called it an outStream, but its an input stream. How about resultFile and resultStream?

Also, I've just created a function loadFileToStream in mailTestUtils.js to do a very similar function. How about extending that to take a charset parameter, if the param is null, just do what it does currently, otherwise pass it through the converter input stream?
(Assignee)

Comment 2

10 years ago
Created attachment 332346 [details] [diff] [review]
update v1.1, addressing nits and extending loadFileToString
Assignee: nobody → sid1337
Attachment #331194 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #331194 - Flags: superreview?(bienvenu)
Attachment #331194 - Flags: review?(bugzilla)
Attachment #331194 - Flags: review?(bienvenu)
(Assignee)

Updated

10 years ago
Attachment #332346 - Flags: superreview?(bienvenu)
Attachment #332346 - Flags: review?(bienvenu)
(Assignee)

Updated

10 years ago
Attachment #332346 - Flags: review?(bugzilla)
Comment on attachment 332346 [details] [diff] [review]
update v1.1, addressing nits and extending loadFileToString

r=me for the unit test changes
Attachment #332346 - Flags: review?(bugzilla) → review+

Comment 4

10 years ago
sid0, very cool.  

one nit - mimehdrpar - can you put some upper case in there, like mimeHdrParam?

I haven't had a chance to test this yet, and I would like to make sure it doesn't break Spotlight, so I'll apply it now and test it.

Comment 5

10 years ago
Comment on attachment 332346 [details] [diff] [review]
update v1.1, addressing nits and extending loadFileToString

TB's mdimporter seems to be functioning w/ this patch
Attachment #332346 - Flags: superreview?(bienvenu)
Attachment #332346 - Flags: superreview+
Attachment #332346 - Flags: review?(bienvenu)
Attachment #332346 - Flags: review+
(Assignee)

Comment 6

10 years ago
Created attachment 332378 [details] [diff] [review]
patch to checkin

nit fixed, carrying forward r+sr=bienvenu for main changes, r=Standard8 for unit test changes.
Attachment #332346 - Attachment is obsolete: true
Attachment #332378 - Flags: superreview+
Attachment #332378 - Flags: review+
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
Checked in, changeset id: 57:81120c50a9b3
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla1.9.1a2
(Assignee)

Updated

10 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.