Closed Bug 400331 Opened 17 years ago Closed 3 years ago

Clean up (and document) nsNNTPProtocol

Categories

(MailNews Core :: Networking: NNTP, defect)

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: jcranmer, Unassigned)

References

()

Details

Attachments

(2 files, 3 obsolete files)

This bug is a holding bug for effort to clean up the NNTP protocol state machine, as well as document it. The URL references my current stub for documentation, including most notably a mostly-machine generated visual graph of the state machine. My current looking indicates that these states have no cases in the large switch statement, and a few are not externally referenced: NNTP_BLOCK_UNTIL_CONNECTIONS_ARE_AVAILABLE NNTP_CONNECTIONS_ARE_AVAILABLE NNTP_CONNECT NNTP_CONNECT_WAIT NNTP_PROFILE_ADD_RESPONSE NNTP_PROFILE_ADD NNTP_PROFILE_DELETE NNTP_PROFILE_DELETE_RESPONSE NNTP_PRINT_ARTICLE_HEADERS
This is the python script I am currently using to generate the state diagrams. Be forewarned: they get large quickly. This script is capable of deducing for *every* function the settings of the member variables, with the caveat that member variables must be prefixed with a `m_' and they must be assigned in the form `<variable> = <value>;' This appears to not hold true for very few variables, so it's not high on my list to fix this. It also has special scraping capabilities for the stateLabels[] and switch(m_nextState) loop; modifying this to work with other state machines in the mozilla database should hopefully be obvious enough. Feel free to modify to your heart's content.
Attached patch Unsanitized preliminary patch (obsolete) (deleted) — Splinter Review
Raw, unsanitized hg mq patch. I am nowhere near done with cleaning up, but I felt that I might put up what I have right now. I've gone through the URI parsing and fixed up the list of accepted URIs, and moved that list to where the URIs are actually parsed (LoadURI). I've also done some heavy cleanup, enforcing in most places a strict 80-column rule and changing all if (...) { to if (...) { as the latter seems to have predominating form. In many cases, where the if statement consists of only one line, I have removed the braces altogether.
Attached patch Updated version of cleanup (obsolete) (deleted) — Splinter Review
An updated version of my preliminary patch, also unsanitized. As part of my discoveries when cleaning up code: 1. Most of the #ifdef UNREADY_CODE hasn't been touched since 1999. 2. diff REALLY sucks at handling whitespace changes en masse. I might just end up doing a manual diff with a -w just to kill its annoyances... 3. The code with the extensions was based off of a draft RFC written in 1997. The actual RFC 3977 is, in many places, far away from the aforementioned draft: RFC 3977 draft RFC CAPABILITIES LIST EXTENSIONS LIST [ACTIVE] XACTIVE ( none ) LIST SEARCHES LIST HEADERS LIST SRCHFIELDS ( none ?) GET LIST NEWSGROUPS LIST PRETTYNAMES Capabilities: ( none ) SETGET NEWNEWS ? HDR ? LIST [*] LISTPNAMES, XACTIVE, ? Note that the number of servers that support the CAPABILITIES command is probably very small at this time: RFC 3977 is barely a year old, and INN, which is AFAICT the most prevalent news server implementation, doesn't support it yet, although several other portions appear to be supported and it is aiming to get it supported as soon as possible. (Just discovered): Maybe a link to http://www.eyrie.org/~eagle/nntp/drafts/ is helpful here: it is the listing of the earlier drafts before RFC 3977. Time to go hunting down differences. <sarcasm>Yay!</sarcasm>
Attachment #287036 - Attachment is obsolete: true
Attached patch Larger update (-w, for your sanity) (obsolete) (deleted) — Splinter Review
This is a -w version of my latest diffs, but it's still pretty big (Never mind the fact that a fair amount of the purpose of this bug involves -w). Some notable features in this latest round is the start of PRInt32->nsresult conversion, removal of half the warnings, the start of dismantling SendFirstNNTPCommand, trackers for potential large blocks of code removal, and some places where function blocks are shifted around for logical coherency. You can easily see some of the places where I'm noting my WIPs...
Attachment #287755 - Attachment is obsolete: true
Attached patch Current WIP, not -w (deleted) — Splinter Review
As much as a -w would be helpful, that probably changes no more than 100 lines due to the wide range of lines affected. I'm continuing on SendFirstNNTPCommand dismantling and have put more effort into reducing the size of the object itself. (Distraction on IRC at this time...) Anyways, the code is becoming easier to understand, I hope.
Attachment #309969 - Attachment is obsolete: true
Product: Core → MailNews Core
Assignee: Pidgeot18 → nobody
Status: ASSIGNED → NEW

Obsolete?

Flags: needinfo?(remotenonsense)
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: needinfo?(remotenonsense)
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: