654933 - msgHdrViewOverlay needs cleaning up

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[Jim Porter (:squib)]

The code in msgHdrViewOverlay.js is pretty messy, and really needs someone to go over it and clean it up. Since I've been doing a bunch of stuff in there, I'm volunteering myself to do this. Notable issues include:

* A wide assortment of quote/brace/indent styles
* Various differently-formatted comments
* Comments (and code) which take >80 characters of space for no good reason
* Strangely named objects (why is an object named "createNewAttachmentInfo"? It
  should just be "AttachmentInfo").

Once the changes to the attachment bar settle down, I'll start uploading patches for various parts of this.

Comment 1

[Jim Porter (:squib)]

For anyone else looking to work on this (hint hint :)), we should be using double quotes and K&R indent style: https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#Naming_and_formatting_code

For commenting functions/globals, we should probably use the Javadoc-style comments:

/**
 * Function description
 *
 * @param aFoo the foo object
 * @return the bar object
 */

Comment 2

[:aceman]

Attachment: patch 1 - indents/braces/quotes

Comment 3

[Jim Porter (:squib)]

Comment on attachment 609043 [details] [diff] [review]
patch 1 - indents/braces/quotes

Looking at the patch, I see a bunch of unchanged places that still use Allman braces (opening brace on a new line). We should probably just fix all of them.

Comment 4

[:aceman]

Well you said I didn't need to change everything. So I touched only those that need it (could be ambiguous or were ugly). But no problem :)

Comment 5

[Jim Porter (:squib)]

(In reply to :aceman from comment #4)
> Well you said I didn't need to change everything. So I touched only those
> that need it (could be ambiguous or were ugly). But no problem :)

Sorry, I meant that you didn't necessarily need to do all of the bullet points I listed in comment 0. I think if we're going to fix the brace style though, we should do it for the whole file. If you prefer, you could do that in multiple parts to reduce the potential for bitrot, but in that case, I think we should at least make sure that each function uses an internally-consistent style.

Comment 6

[:aceman]

Attachment: patch 1 - indents/braces/quotes v2

Comment 7

[Jim Porter (:squib)]

Comment on attachment 609106 [details] [diff] [review]
patch 1 - indents/braces/quotes v2

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

Ok, I did a quick pass on this. r- for now, but it mostly looks good. Unfortunately, I'll be busy for about a week, so I won't be able to review followup patches until then.

Standard8, what's our policy for braces around single-line if blocks? Mozilla's style is to *always* do it, but I don't think Thunderbird obeys that.

::: mail/base/content/msgHdrViewOverlay.js
@@ +1132,4 @@
>      address.emailAddress = addresses.value[index];
>      address.fullAddress = fullNames.value[index];
>      address.displayName = names.value[index];
> +    if (headerEntry.useToggle) {

I think Thunderbird's prevailing style is *not* to put braces around single-line if blocks (despite Mozilla's rules in general). We should probably see what Standard8 thinks about that.

My *personal* preference is to omit the braces for single-line blocks (note: single-line, not single-statement), but that's just me.

@@ +1496,3 @@
>      let server = gFolderDisplay.selectedMessage.folder.server;
>      if (server)
> +      return server.QueryInterface(Components.interfaces.nsISubscribableServer);

If we do go with always using braces for if blocks, we need braces here.

@@ +1786,5 @@
>    var canDetachSelected = CanDetachAttachments() && !allSelectedDetached &&
>                            !allSelectedDeleted;
>  
> +  openMenu.disabled   = allSelectedDeleted;
> +  saveMenu.disabled   = allSelectedDeleted;

Our convention is not to align equals signs, since it just results in more work when adding a net assignment to the list (I prefer aligning equals signs, but when in Rome...)

To avoid making this patch completely unwieldy, let's just keep spacing in assignments the way it is for now. We can address that separately.

@@ +1844,3 @@
>  
> +  let detached  = currentAttachments[0].isExternalAttachment;
> +  let deleted   = !currentAttachments[0].hasFile;

As mentioned above, we should just leave the spacing as-is.

@@ +1847,4 @@
>    let canDetach = CanDetachAttachments() && !deleted && !detached;
>  
> +  openItem.disabled   = deleted;
> +  saveItem.disabled   = deleted;

Leave the spacing as-is here, too.

@@ +1871,5 @@
>    });
>    let canDetach = CanDetachAttachments() && !allDeleted && !allDetached;
>  
> +  openAllItem.disabled   = allDeleted;
> +  saveAllItem.disabled   = allDeleted;

Leave the spacing as-is here, too.

@@ +2075,5 @@
>    var attachmentSplitter = document.getElementById("attachment-splitter");
>    var bundle = document.getElementById("bundle_messenger");
>  
>    if (expanded === undefined)
>      expanded = !attachmentToggle.checked;

Again, if we're going with always using braces for if blocks, we should use braces here.

Comment 8

[:aceman]

I bet this will bitrot nicely before we can move on here ;)
Standard8, can you answer comment 7?

Comment 9

[Magnus Melin [:mkmelin]]

(In reply to Jim Porter (:squib) from comment #7)
> Standard8, what's our policy for braces around single-line if blocks?
> Mozilla's style is to *always* do it, but I don't think Thunderbird obeys
> that.

The policy is no braces for single-line if blocks. BUT, if it's an else-if and one of the else-if blocks need braces, they should all have it.

Comment 10

[:aceman]

Attachment: [checked in] patch 1 - indents/braces/quotes v3

Ok, let's try this.

Comment 11

[Jim Porter (:squib)]

Comment on attachment 614469 [details] [diff] [review]
[checked in] patch 1 - indents/braces/quotes v3

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

Looks good to me.

Comment 12

[:aceman]

Can this part be checked in?

Comment 13

[Jim Porter (:squib)]

Sure.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/comm-central/rev/9075147bd5eb

Comment 15

[:aceman]

Attachment: patch 2 - comments/long lines

Fixed long lines and reformatted comments.

I have left some overlong lines that will go away as part of bug 738991.

Comment 16

[:aceman]

The patch will need adapting to patch in bug 746450.
But the review can still proceed.

Comment 17

[Jim Porter (:squib)]

Comment on attachment 619390 [details] [diff] [review]
patch 2 - comments/long lines

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

This is just a quick pass; I'll probably find more nits to pick when I go over this in more detail. Most of it looks good, though. Clearing out review for now, since this has bit-rotted.

::: mail/base/content/msgHdrViewOverlay.js
@@ +119,5 @@
> +    {name:"newsgroups", outputFunction:OutputNewsgroups},
> +    {name:"references", outputFunction:OutputMessageIds},
> +    {name:"followup-to", outputFunction:OutputNewsgroups},
> +    {name:"content-base"},
> +    {name:"tags"} ];

I think we should indent by 2 spaces here, since that's the usual style.

@@ +141,5 @@
> + * currentHeaderData  this is an array of header name and value pairs for the
> + *                    currently displayed message. It's purely a data object
> + *                    and has no view information. View information is
> + *                    contained in the view objects.
> + *                    For a given entry in this array you can ask for:

Drop the "currentHeaderData", and unindent these lines. We don't use that notation for any of the other globals.

@@ +152,5 @@
> + * For the currently displayed message, we store all the attachment data. When
> + * displaying a particular view, it's up to the view layer to extract this
> + * attachment data and turn it into something useful.
> + * For a given entry in the attachments list, you can ask for the following
> + * properties:

We can drop this and all the properties listed afterwards. This is all documented in the AttachmentInfo code further down. But please do say that currentAttachments is an array of AttachmentInfo objects. :)

@@ +166,5 @@
>  const nsIAbListener = Components.interfaces.nsIAbListener;
>  const nsIAbCard = Components.interfaces.nsIAbCard;
>  
> +/**
> + * createHeaderEntry: Our constructor method which creates a header Entry

We can drop the "createHeaderEntry" here.

@@ +232,5 @@
>    }
>  
>    if (prefBranch.getBoolPref("mailnews.headers.showOrganization")) {
> +    var organizationEntry = { name:"organization",
> +                              outputFunction:updateHeaderValue };

Spaces after the colons, please. Likewise with the code following this.

@@ +307,5 @@
>    let openInNewWindow = document.getElementById("otherActionsOpenInNewWindow");
>    openInTab.hidden = openInNewWindow.hidden = opensAreHidden;
>  
> +  // dispatch an event letting any listeners know that we have loaded
> +  // the message pane

While we're here, could you give this sentence case?

@@ +840,5 @@
>    }
>  }
>  
> +/**
> + * Flush out any local state being held by a header entry for a given table.

Could you add some documentation about the headerTable argument? Likewise for the functions following this one.

@@ +1186,5 @@
> + * addresses, extracts them one by one, linkifying each email address into
> + * a mailto url.
> + *
> + * @param headerEntry  Then we add the link-ified email address to the parentDiv
> + *                     passed in.

This comment doesn't seem to make sense. I think the description belongs in the paragraph above.

@@ +1436,5 @@
> + * is found in the address books, then it book will contain an nsIAbDirectory,
> + * and card will contain an nsIAbCard.
> + *
> + * @param emailAddress  If the email address is not found, both
> + *                      items will contain null.

This comment also doesn't make sense, and should go in the preceding paragraph. Also, please add a @return line, too.

@@ +1521,5 @@
>  
>  /**
>   * Takes the email address title button, extracts the email address we stored
>   * in there and opens a compose window with that address.
> + * @param addressNode  a node which has a "fullAddress" or "newsgroup" attribute

Could you add an empty line before this one?

@@ +1789,5 @@
>      let stream = Services.io.newChannelFromURI(url).open();
>  
>      let inputStream = Components.classes["@mozilla.org/binaryinputstream;1"]
> +                                .createInstance(Components.interfaces
> +                                                          .nsIBinaryInputStream);

You can leave this on one line. The general guideline I use is "keep to <=80 characters on a line, unless it's involving Components.classes or Components.interfaces. Then I format it like this line was originally.

Comment 18

[:aceman]

Attachment: patch 2 - comments/long lines v2

Fixed nits. 

>> +/**
>> + * Flush out any local state being held by a header entry for a given table.
>Could you add some documentation about the headerTable argument? Likewise for >the functions following this one.
Sorry I can't do this, I do not know what the functions do. I just reformat existing info.

Comment 19

[Jim Porter (:squib)]

Comment on attachment 623294 [details] [diff] [review]
patch 2 - comments/long lines v2

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

This looks pretty good, aside from a few places we should be using sentence case. I also think I bitrotted this in bug 696414, so r-'ing for now just to keep track of that. Once you fix the bitrot, I think we can check this in, and resolve this bug. There are probably other little things we can clean up, but I think we can address those gradually, as we patch this file for other reasons.

::: mail/base/content/msgHdrViewOverlay.js
@@ +120,5 @@
> +  {name:"newsgroups", outputFunction:OutputNewsgroups},
> +  {name:"references", outputFunction:OutputMessageIds},
> +  {name:"followup-to", outputFunction:OutputNewsgroups},
> +  {name:"content-base"},
> +  {name:"tags"} ];

Could you please put spaces after the colons in this list? Spaces between the curly braces are optional (do whichever you think is better).

@@ +614,3 @@
>      {
> +      // presentation level change....don't show vcards as external attachments
> +      // in the UI. Libmime already renders them inline.

I think I bitrotted this from bug 696414.

@@ +834,5 @@
>  }
>  
> +/**
> + * Flush out any local state being held by a header entry for a given table.
> + */

Could you add a javadoc for the headerTable param here? The following should be sufficient (it's used elsewhere in the file):

 * @param aHeaderTable Table of header entries

@@ +846,5 @@
>    }
>  }
>  
> +/**
> + * make sure that any valid header entry in the table is collapsed

Sentence case, please.

@@ +855,5 @@
>      headerEntry.enclosingRow.collapsed = true;
>  }
>  
> +/**
> + * make sure that any valid header entry in the table specified is visible

Sentence case, please.

@@ +890,5 @@
>      // how many empty headers do we need to add?
>      var numEmptyHeaders = gMinNumberOfHeaders - numVisibleHeaders;
>  
> +    // we may have already dynamically created our empty rows and we just need
> +    // to make them visible

Sentence case, please.

@@ +899,5 @@
>        }
>      }
>  
> +    // ok, now if we have any extra dummy headers we need to add, create a new
> +    // header widget for them

Sentence case...

@@ +965,5 @@
>  }
>  
> +/**
> + * default method for updating a header value into a header entry
> + */

Sentence case, and please document the parameters:

 * @param headerEntry  A single header from currentHeaderData
 * @param headerValue  The new value for headerEntry

@@ +1039,3 @@
>  function UpdateExpandedMessageHeaders() {
> +  // iterate over each header we received and see if we have a matching entry
> +  // in each header view table...

Sentence case...

Comment 20

[:aceman]

Attachment: [checked in] patch 2 - comments/long lines v3

Thanks, done.

Comment 21

[Jim Porter (:squib)]

Comment on attachment 631496 [details] [diff] [review]
[checked in] patch 2 - comments/long lines v3

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

Looks good. Sorry about the delay!

Comment 22

[:aceman]

Thanks!

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 631496 [details] [diff] [review]
[checked in] patch 2 - comments/long lines v3

https://hg.mozilla.org/comm-central/rev/4878362e857d