Magnus Melin [:mkmelin]
	Comment 3 • 6 years ago

Comment on attachment 9050216 [details] [diff] [review]
mail-multi-emailheaderfield.patch

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

Doesn't work. If there are more than one email I get

JavaScript error: chrome://messenger/content/mailWidgets.js, line 896: TypeError: this.emailAddresses is undefined

::: mail/base/content/mailWidgets.js
@@ +648,5 @@
> + *
> + * extends {MozXULElement}
> + */
> +class MozMailMultiEmailHeaderField extends MozXULElement {
> +  connectedCallback() {

add a delayConnectedCallback() check

@@ +664,5 @@
> +    // The number of lines of addresses we will display before adding a (more) indicator to the
> +    // widget. This can be increased using the preference mailnews.headers.show_n_lines_before_more.
> +    this.maxLinesBeforeMore = 1;
> +
> +

remove the double extra empty line

@@ +712,5 @@
> +  }
> +
> +  /**
> +   * Method used to add an address to this widget.
> +   * addresses is an object with 3 properties: displayName, emailAddress and fullAddress.

please see this on how to document parameters which are objects: http://usejsdoc.org/tags-param.html
the comment is also wrong: there is no "addresses"

	Magnus Melin [:mkmelin]
	Comment 5 • 6 years ago

Comment on attachment 9051493 [details] [diff] [review]
mail-multi-emailheaderfield.patch

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

::: mail/base/content/mailWidgets.js
@@ +643,5 @@
>    customElements.define("menulist-editable", MozMenulistEditable, { extends: "menulist" });
>  });
> +
> +/**
> + * multi-emailHeaderField presents multiple emailheaderfields with a toggle.

The mail-multiemail-headerfield widgets shows multiple emails. It collapses long rows and allows toggling the full view open. This widget is typically used in the message header pane to show addresses for To, Cc, Bcc, and any other addressing type header that can contain more than one mailbox.

@@ +653,5 @@
> +    if (this.delayConnectedCallback()) {
> +        return;
> +    }
> +
> +    this.textContent = "";

while this clears out the children, I think checking hasChildNodes() is better, since then you don't need to do the unneeded other work either

@@ +655,5 @@
> +    }
> +
> +    this.textContent = "";
> +
> +    this.addChildren();

just used once, so just inline that function

@@ +716,5 @@
> +  /**
> +   * Method used to add an address to this widget.
> +   *
> +   * @param {Object} address    address is an object with 3 properties: displayName, emailAddress
> +   *                            and fullAddress

Please see my previous review comments regarding this. You didn't update this.

@@ +732,5 @@
> +
> +  /**
> +   * Private method used to set properties on an address node.
> +   */
> +  updateEmailAddressNode(emailNode, address) {

if it's private. just move it to _updateEmailAddressNode

@@ +752,5 @@
> +  /**
> +   * Private method used to create email address nodes for either our short or long view.
> +   *
> +   * @param {Boolean} all                 If false, show only a few addresses + "more".
> +   * @return {Integer}                    Number of addresses we have put into the list.

I don't think there needs to be much if any space between here

@@ +930,5 @@
> +    }
> +    let ttText = tttArray.join(", ");
> +
> +    let remainingAddresses = addresses.length - tttArray.length;
> +    // Mot all missing addresses fit in the tooltip.

Not

@@ +1012,5 @@
> +    this.clearChildNodes();
> +  }
> +}
> +
> +customElements.define("mail-multi-emailHeaderField", MozMailMultiEmailHeaderField);

let's not do any camelCasing for CE
maybe mail-multiemailheaderfield and MozMailMultiemailHeaderfield

	Magnus Melin [:mkmelin]
	Comment 8 • 6 years ago

Comment on attachment 9053930 [details] [diff] [review]
mail-multi-emailheaderfield.patch

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

Except for the comments, this looks good

::: mail/base/content/mailWidgets.js
@@ +752,5 @@
> +class MozMailMultiEmailheaderfield extends MozXULElement {
> +  constructor() {
> +    super();
> +
> +    this.mAddresses = "";

hmm

@@ +772,5 @@
> +    // This field is used to specify the maximum number of addresses in the more button tooltip
> +    // text.
> +    this.tooltipLength = 20;
> +
> +    this.mAddresses = [];

ugh!

Let's drop the m part here and use this.addresses instead

@@ +780,5 @@
> +    if (this.delayConnectedCallback()) {
> +      return;
> +    }
> +
> +    if (!this.hasChildNodes()) {

prefer an early return

@@ +814,5 @@
> +    return this.tooltipLength;
> +  }
> +
> +  /**
> +   * Method used to add an address to this widget.

Add an address to be shown in this widget.

@@ +848,5 @@
> +        UpdateEmailNodeDetails(address.emailAddress, emailNode);
> +      }
> +    } catch (ex) {
> +      dump(`UpdateEmailNodeDetails failed:  ${ex}\n`);
> +    }

we can remove this try/catch

@@ +858,5 @@
> +   * @param {Boolean} all   If false, show only a few addresses + "more".
> +   * @return {Integer}      Number of addresses we have put into the list.
> +   */
> +  _fillAddressesNode(all) {
> +    const addressesNode = this.emailAddresses;

I don't think we need this local temp var

@@ +928,5 @@
> +        // Hide the last node spanning into the additional line (n>1)
> +        // also hide it if <30px left after sliding the address (n=1)
> +        // or if the last address would be truncated without "more"
> +        if (curLine >= this.maxLinesBeforeMore &&
> +          (this.maxLinesBeforeMore > 1 ||

we usually indent these as

       if (curLine >= this.maxLinesBeforeMore &&
           (this.maxLinesBeforeMore > 1 ||

	Magnus Melin [:mkmelin]
	Comment 10 • 6 years ago

Comment on attachment 9054219 [details] [diff] [review]
mail-multi-emailheaderfield.patch

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

r=mkmelin

::: mail/base/content/mailWidgets.js
@@ +1074,5 @@
> +    // Re-render the node, this time with all the addresses.
> +    this._fillAddressesNode(true);
> +    // Compute height of 'expandedHeaderView' from 'expandedHeadersBox'.
> +    let expandedHeaderView = document.getElementById("expandedHeaderView");
> +    expandedHeaderView.setAttribute(

you don't need the variable for the view. just do
 document.getElementById("expandedHeaderView").setAttribute

	Magnus Melin [:mkmelin]
	Comment 14 • 6 years ago

Comment on attachment 9054682 [details] [diff] [review]
mail-multi-emailheaderfield.patch

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

LGTM