2031552 - Sort out UID Set handling - AllocateImapUidString(), AllocateUidStringFromKeys() et al

Status: NEW

(MailNews Core :: Networking: IMAP, task)

Description

[Ben Campbell]

We've got two separate methods for building IMAP UID sets from a list of numeric UIDs.

There is:
nsImapMailFolder::AllocateUidStringFromKeys(const nsTArray<nsMsgKey>& keys, nsCString& msgIds)

Which seems pretty sane, barring the use of nsMsgKeys as UIDs (but that'll be patched up in bug 1806770).

Then there's:
AllocateImapUidString(const ImapUid* msgUids, uint32_t& msgCount, nsImapFlagAndUidState* flagState, nsCString& returnString)
Which mostly does the same thing except:

1. it doesn't sort the UIDs, so {5,4,3,2,1} won't be turned into a range.
2. If flagState is given, it'll use it to determine if there are gaps in what the IMAP server sent us, so might be able to construct ranges even if there are apparent gaps in msgUids.
3. the output list might be clipped to 950 characters (but only if flagState is given). In this case the count reference passed in is updated to reflect the number of UIDs included in the output.

1 may or may not be fixable, depending on if the callers depend on ordering.
2 is an issue because nsImapFlagAndUidState should only be accessible from the IMAP thread, so there is a thread race if the main thread tries to use it. See Bug 2031181.
3 is just rather complicated and non-obvious.

Broadly speaking, AllocateImapUidString() is used on the protocol side of the folder/protocol divide (in the IMAP thread), and AllocateUidStringFromKeys() is used on the folder side (in the main thread). But there is crossover.

It'd be good to rationalise and simplify it all, and ensure we can't do thread-unsafe things.
In particular, AllocateImapUidString() seems overly fiddly and obtuse.