Comment 273 for bug 52667

Revision history for this message
In , Mkmelin+mozilla (mkmelin+mozilla) wrote :

(From update of attachment 379240)
This part seems to be working fine now!

A few tiny nits, and you'll have to get sr for the mailnews/ bits too.

>+ <xul:menuseparator anonid="hdrReplyAllSubButtonSep" />

Drop the space before />

>+ let numAddresses = hdrParser.parseHeadersWithArray(uniqueRecipients, {}, {},
>+ {});

The 80 char limit isn't quite this strict, i think. Or move more than one of the {}s to the next line too.

>+
>+ // If I've been bcc-ed, then add 1 to the number of addresses to compensate.
>+ if (imBcced)
>+ numAddresses++
>+
>+ // By default, ReplyAll if there is more than 1 person to reply to.
>+ let showReplyAll = numAddresses > 1;
>+
>+ // And ReplyToList if there is a List-Post header.
>+ let showReplyList = currentHeaderData['list-post'];

Prefer to use " instead of ' when it's not needed to use '.
Here and later on.

>+ // But, if we're in a news folder, we should default to the Reply button.
>+ if (isNewsURI(msgHdr.folder.URI))

Should probably to the same for feeds (check msgHdr.folder.type == "rss")

>+
>+ // If it's a news message, show the ReplyAll sub-button and separator.
>+ if (isNewsURI(msgHdr.folder.URI))
>+ {
>+ replyAllSubButton.hidden = false;
>+ replyAllSubButtonSep.hidden = false;
>+ }
>+ else
>+ {
>+ // otherwise, hide them.
>+ replyAllSubButton.hidden = true;
>+ replyAllSubButtonSep.hidden = true;
>+ }
>+}

Comment placement isn't consistent here. Move the "// If it's..." inside the if clause

r=mkmelin with that