970118 - Improve nsIMsgCompose::checkAndPopulateRecipients

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Joshua Cranmer [:jcranmer]]

Whooo boy. This is a nasty little interface definition, and here's the history:

Originally, this method was getNonHtmlRecipients, apparently checking if any recipients preferred plain text. Ironically enough, the only use of the return value was to check if it was non-empty. This sole use has not been changed in the intervening 14 years, even in addons code. Along the way, the method was changed to support expanding mailing lists. It was also changed to return a more nuanced preference for mail (which was broken in Thunderbird by bug 872041, see bug 921836).

As part of the structured header revolution, I want to end the use of strings to represent addressing headers, and this is a perfect method that needs to be restructured. Addon impact is extremely minimal. Proposal:

Split the method into two methods, expandMailingLists and getHTMLAction. It's clear from MsgComposeCommands.js that there is a pretty strict definition of what needs to be done there, so that JS code (cloned between SM and TB, with TB's copy being broken) should be folded into the getHTMLAction instead of determining a preferred mail format.

Comment 1

[Joshua Cranmer [:jcranmer]]

Oh, I forgot. This method also increments the popularity index of sending messages. How could I neglect to mention this all-important feature which is neither implied by the API nor mentioned in the comments?

Comment 2

[Joshua Cranmer [:jcranmer]]

Attachment: Version 1

This patch may seem a bit complicated. Originally, I had it as three separate patches, but the division between them ended up being poorer than I wanted. I also feel like it may be worth moving the address collection from nsMsgSend to nsMsgCompose, but that's another patch for another day.

Comment 3

[[e-mail]]

Eww, so every time I cancel the HTML mail question I've inadvertently bumped the popularity index of all of the recipients?

Comment 4

[[e-mail]]

Comment on attachment 8375308 [details] [diff] [review]
Version 1

ExpandMailingLists wants to handle each field separately but DetermineHTMLAction ideally wants everything in one big array; I couldn't think of a nice way of handling that.

>               rv = existingCard->GetPropertyAsUint32(kPreferMailFormatProperty,
>+                                                     &preferFormat);
I take it you check all address books until you find a card with the format property? Why throw away the value?

>+  nsTArray<nsMsgRecipient> recipientsList[MAX_OF_RECIPIENT_ARRAY];
RecipientsArray?

>+        // Grab a ref to the directory--we're appending to the nsTArray, which
>+        // can invalidate the reference to recipient underneath us.
Nit: you don't append to the nsTArray until after you need the current entry.

>+nsMsgCompose::DetermineHTMLAction(int32_t aConvertible, int32_t *result)
Looks like if there are any newsgroups you should immediately return AskUser. Then after the loop you can write:
if (allHtml) return HTML;
if (allPlain || convertible == Plain) return PlainText;
if (pref != Unknown) return pref;
return AskUser;

>+  bool allHtml = true;
>+  bool allPlain = true;
>+
>+  for (int i = 0; i < MAX_OF_RECIPIENT_ARRAY; ++i)
Nit: would be more efficient if you stopped looping once both were false.

Comment 5

[Joshua Cranmer [:jcranmer]]

(In reply to [e-mail] from comment #4)
> >               rv = existingCard->GetPropertyAsUint32(kPreferMailFormatProperty,
> >+                                                     &preferFormat);
> I take it you check all address books until you find a card with the format
> property? Why throw away the value?

/me peers in more closely to the code.

Originally, I wanted to trash the get where it was when it started, but then I decided to avoid changing semantics too much. Considering that we're trying to get specifically two different properties that may in theory want different cards, it's probably better to ensure that PAB/CAB come first here and drop the checks, since it looks like known of our other ab backends support preferMailFormat (or popularity for that matter).

Thoughts?

> >+        // Grab a ref to the directory--we're appending to the nsTArray, which
> >+        // can invalidate the reference to recipient underneath us.
> Nit: you don't append to the nsTArray until after you need the current entry.

It's very confusing at first, but the append is in the middle of a loop. If we have a mailing list with two entries, the code crashes--that's why I added the copy.

> >+nsMsgCompose::DetermineHTMLAction(int32_t aConvertible, int32_t *result)
> Looks like if there are any newsgroups you should immediately return
> AskUser.

Heh, that was my original plan all along. I only stopped doing so on the basis that it might make adding intelligent send prefs for news harder.

Comment 6

[Joshua Cranmer [:jcranmer]]

(In reply to [e-mail] from comment #3)
> Eww, so every time I cancel the HTML mail question I've inadvertently bumped
> the popularity index of all of the recipients?

Another option is to move the popularity bump to the address collector (it already has to query the lookup anyways), but that was a yet more invasive patch I didn't want to do.

Comment 7

[[e-mail]]

(In reply to Joshua Cranmer from comment #5)
> (In reply to comment #4)
> > >               rv = existingCard->GetPropertyAsUint32(kPreferMailFormatProperty,
> > >+                                                     &preferFormat);
> > I take it you check all address books until you find a card with the format
> > property? Why throw away the value?
> 
> /me peers in more closely to the code.
> 
> Originally, I wanted to trash the get where it was when it started, but then
> I decided to avoid changing semantics too much. Considering that we're
> trying to get specifically two different properties that may in theory want
> different cards, it's probably better to ensure that PAB/CAB come first here
> and drop the checks, since it looks like known of our other ab backends
> support preferMailFormat (or popularity for that matter).

My original question was really only about the detected prefer format value, although now that you draw my attention to it, your popularity appears to be broken if it doesn't find a preferred format but does find a card in a readonly address book (I don't know whether that occurs in practice).

Comment 8

[Joshua Cranmer [:jcranmer]]

Attachment: Version 2

Comment 9

[[e-mail]]

Comment on attachment 8384204 [details] [diff] [review]
Version 2

>-          if (!recipient.mProcessed)
>+          if (!recipient.mDirectory && !recipient.mCard)
Nit: You never set the card without setting the directory, so you don't need to test for it.

>+        // Bump the popularity index for this card since we are about to send
>+        // e-mail to it.
>+        uint32_t popularityIndex = 0;
>+        if (!readOnly)
Nit: move the variable inside the if

Comment 10

[Joshua Cranmer [:jcranmer]]

https://hg.mozilla.org/comm-central/rev/642cfedbc637

Comment 11

[Magnus Melin [:mkmelin]]

Wohoo! This must have been one of the ugliest interfaces we had.