1668848 - Cleanup, rectify and improve readibility and performance of recipientAddPills()

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, task)

Description

[Thomas D. (:thomas8)]

+++ This bug was initially created as a clone of Bug #1666463 +++

Cleanup, rectify and improve readibility and performance of recipientAddPills().
Some notes on coding style.

Find the fault!

  let addresses = MailServices.headerParser.makeFromDisplayAddress(
    element.value
  );

  for (let address of addresses) {
    let pill = createRecipientPill(element, address);
    parent.insertBefore(pill, element);
 
    // Be sure to add the user add recipient to our ignore list
    // when the user hits enter in an autocomplete widget...
    addRecipientsToIgnoreList(element.value);
  }

• Something is wrong here: First we break up element.value (a string of comma-separated addresses) into an array of address objects, then, for each iteration, we add the full input string again to the spellcheck ignore list. We don't know anything about the performance data of the spell thingy, so doing potentially unlimited repeated rounds with that flaw isn't right.
• Oh, element is the address input, but you could have guessed that! Not? That's exactly what leads to bugs like the above. Much easier to overlook problems with that generic element argument. Imho, a litte bit of semantic/functional connotation helps.
• addRecipientsToIgnoreList() wants a string argument (which we had), but it won't tell you unless you dig around some levels deeper in the nested internals of other dependent functions. Should try to document a bit when we re-use those oldies.
• Current function name, recipientAddPill(), and the existing function comment both wrongly suggest that this function will only ever add a single recipient pill (again contributing to the above coding flaw). Imho any function which can handle/create/return more than one thing should have a plural name, and description should be clear. It makes a big difference if we create a single pill (for which there's yet another function) or multiple pills.

Comment 1

[Thomas D. (:thomas8)]

Attachment: 1668848_recipientAddPillsCleanup.diff

• Avoid repeated runs of addRecipientsToIgnoreList() for all addresses of input.value. For n address pills, we were adding n² addresses to the spell check ignore list.
• Change function name from recipientAddPill() to recipientAddPills().
• Rename 'element' argument to 'input' (for some adjacent minor functions, too).
• Improve various function comments.

Comment 2

[Alessandro Castellani [:aleca]]

Comment on attachment 9179294 [details] [diff] [review]
1668848_recipientAddPillsCleanup.diff

Review of attachment 9179294 [details] [diff] [review]:
-----------------------------------------------------------------

This is great, thank you so much for doing it.
Just a little nit if you think it's worth doing it.

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +788,1 @@
>    for (let pill of container.querySelectorAll("mail-address-pill")) {

I guess we could avoid declaring the container variable since we don't use it to check for its existence.

Comment 3

[Alessandro Castellani [:aleca]]

Comment on attachment 9179294 [details] [diff] [review]
1668848_recipientAddPillsCleanup.diff

Review of attachment 9179294 [details] [diff] [review]:
-----------------------------------------------------------------

Small eslint error

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +743,5 @@
>      return;
>    }
>  
>    let addresses = MailServices.headerParser.makeFromDisplayAddress(
> +    input.value

eslint nit: this fits all in a single line.

Comment 4

[Thomas D. (:thomas8)]

Attachment: 1668848_recipientAddPillsCleanup.diff

(In reply to Alessandro Castellani (:aleca) from comment #2)

Just a little nit if you think it's worth doing it.

Of course. Dotted the t's and crossed the i's - or something like that ;-)
Lovely... after double scrutiny from Alex and myself!

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2fa5dbefac96
Cleanup, rectify and improve readibility and performance of recipientAddPills(). r=aleca

Comment 6

[Magnus Melin [:mkmelin]]

Do we need uplifting?

Comment 7

[Alessandro Castellani [:aleca]]

I think we should.
This is purely a "clean up" type of patch, but if we don't uplift we risk to have merging issues if future patches that touch these sections need to be uplifted.

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9180263 [details] [diff] [review]
1668848_recipientAddPillsCleanup.diff

[Approval Request Comment]
Per comment 7

Comment 9

[Wayne Mery (:wsmwk)]

Comment on attachment 9180263 [details] [diff] [review]
1668848_recipientAddPillsCleanup.diff

[Triage Comment]
Approved for beta

Comment 10

[Rob Lemley [:rjl]]

Thunderbird 82.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/9921e1929e74

Comment 11

[Rob Lemley [:rjl]]

Thunderbird 82.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/9921e1929e74

Comment 12

[Wayne Mery (:wsmwk)]

Comment on attachment 9180263 [details] [diff] [review]
1668848_recipientAddPillsCleanup.diff

[Triage Comment]
Approved for esr78

Comment 13

[Rob Lemley [:rjl]]

Appears to depend on another bug. The block above this contains

      if (!element || element.classList.contains("input-pill")) {
        return;
      }

Comment 14

[Rob Lemley [:rjl]]

Depends on bug 1665025 which looks benign enough, but there is also a change from bug 1665025 that come into play in addressInputOnBlur. The later is mostly renaming element to input it looks like, so adjusting accordingly.

Comment 15

[Rob Lemley [:rjl]]

Thunderbird 78.4.0:
https://hg.mozilla.org/releases/comm-esr78/rev/f125b0ed2aeb