Closed Bug 312599 Opened 15 years ago Closed 15 years ago
Move parsing from ns
IMAPBody Shell to ns Imap Server Response Parser, eliminate 200 lines
In the present implementation, a BODYSTRUCTURE response is parsed multiple times; first, in nsImapServerResponseParser, the full response is parsed and then written to a string. Second, this string is then passed to nsIMAPBodyShell, which parses it again. This multiple parsing should be avoided. Furthermore, it is somewhat unnatural that the parsing takes places within |nsIMAPBodyShell| and its related classes. It would make much more sense to do all IMAP parsing in |nsImapServerResponseParser|. Both of these issues can be resolved by moving the parsing code from nsIMAPBodyShell.cpp to nsImapServerResponseParser.cpp, which can be done without changing the behavior of the IMAP implementation. Additionally, the moved code becomes much easier to understand and the code shrinks by ~200 lines.
Roughly speaking, my strategy is to make the following changes. * |nsIMAPBodypart| no longer inherits from |nsIMAPGenericParser| * move method |nsIMAPBodypart::CreatePart| to |nsImapServerResponseParser::bodystructure_part()| * merge methods |nsIMAPBodypartLeaf::ParseIntoObjects()| and |nsIMAPBodypartMessage::ParseIntoObjects()| into |nsImapServerResponseParser::bodystructure_leaf()| * move method |nsIMAPBodypartMultipart::ParseIntoObjects| to |nsImapServerResponseParser::bodystructure_multipart| * eliminate the following fields/methods: nsIMAPBodypart::m_responseBuffer nsIMAPBodypart::GetNextLineForParser() nsIMAPBodypart::ContinueParse() nsIMAPBodypart::GetPartNumber() nsIMAPBodypart::m_partNumber findEndParenInBuffer()
Status: NEW → ASSIGNED
also eliminate field |nsIMAPBodypart::m_shell|
Comment on attachment 199750 [details] [diff] [review] Move parsing code to nsImapServerResponseParser, removed more '\t's can I get a -uwp8 diff for the purpose of a review, so I don't need to figure out what parts are whitespace only? thx!
patch with diff -uwp8N
Comment on attachment 199750 [details] [diff] [review] Move parsing code to nsImapServerResponseParser, removed more '\t's this looks OK - I'll try running with it before checking in.
Attachment #199750 - Flags: review?(bienvenu) → review+
fix checked in, thx!
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Verified via LXR (code inspection) and by a couple of days without seeing any regressions... ;-)
Status: RESOLVED → VERIFIED
Unfortunately I discovered a problem caused by this checkin. I filed bug 315975 (saying I wasn't going to try to find the responsible checkin...but I lied ;o) After I filed my bugreport I realized that the test case I attached is not going to show you the bug: when I save the email as a file and then reopen it using 'Open Saved Message' it displays perfectly. I suppose I see this problem because the email is sitting on an imap server? I don't know how to test this theory because forwarding the message to myself also avoids the bug. I suppose you could subscribe to the same newsletter and wait for your first email to arrive :o)
right, the message would need to be on an imap server. Hans-Andreas or I should be able to get the message onto an imap server for testing.
You need to log in before you can comment on or make changes to this bug.