312599 - Move parsing from nsIMAPBodyShell to nsImapServerResponseParser, eliminate 200 lines

Status: VERIFIED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Hans-Andreas Engel]

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.

Comment 1

[Hans-Andreas Engel]

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()

Comment 2

[Hans-Andreas Engel]

Attachment: Move parsing code to nsImapServerResponseParser

also eliminate field |nsIMAPBodypart::m_shell|

Comment 3

[Hans-Andreas Engel]

Attachment: Move parsing code to nsImapServerResponseParser, removed more '\t's

Comment 4

[David :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!

Comment 5

[Hans-Andreas Engel]

Attachment: Move parsing code to nsImapServerResponseParser, diff -uwp8N

patch with diff -uwp8N

Comment 6

[David :Bienvenu]

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.

Comment 7

[David :Bienvenu]

fix checked in, thx!

Comment 8

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified via LXR (code inspection) and by a couple of days without seeing any regressions... ;-)

Comment 9

[walter]

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)

Comment 10

[David :Bienvenu]

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.