Closed Bug 312599 Opened 15 years ago Closed 15 years ago

Move parsing from nsIMAPBodyShell to nsImapServerResponseParser, eliminate 200 lines

Categories

(MailNews Core :: Networking: IMAP, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: engel, Assigned: engel)

References

Details

(Keywords: fixed1.8.1)

Attachments

(2 files, 1 obsolete file)

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|
Attachment #199707 - Flags: review?(bienvenu)
Depends on: 312601
Attachment #199707 - Attachment is obsolete: true
Attachment #199750 - Flags: review?(bienvenu)
Attachment #199707 - Flags: review?(bienvenu)
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
Blocks: 313038
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.
Blocks: 315975
Blocks: 321239
Blocks: 322863
No longer blocks: 322863
Depends on: 322863
Keywords: fixed1.8.1
Blocks: 327991
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.