1260062 - Whitespace and Indention Cleanup mailWindowOverlay.js

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: General, defect)

Description

[Frank-Rainer Grahl (:frg)]

Just trailing spaces and tabs removal
\suite\mailnews\mailWindowOverlay.js

Comment 1

[Frank-Rainer Grahl (:frg)]

Attachment: 1260062-whitespce-removal.patch

Decided to create a second bug

Comment 2

[Philip Chee]

Comment on attachment 8735309 [details] [diff] [review]
1260062-whitespce-removal.patch

> Bug 1260062 - Whitespace Cleanup
Bug 1260062 - Whitespace Cleanup in mailWindowOverlay.js

> -  switch (headerchoice) 
> +  switch (headerchoice)
>    {
> -	case 2:	
> -		id = "viewallheaders";
> -		break;
> -	case 1:	
> -	default:
> -		id = "viewnormalheaders";
> -		break;
> +    case 2:
> +        id = "viewallheaders";
> +        break;
> +    case 1:
> +    default:
> +        id = "viewnormalheaders";
> +        break;
>    }
Ouch! Since we are cleaning up please use two space indent.

> -      window.openDialog("chrome://messenger/content/FilterEditor.xul", "", 
> +      window.openDialog("chrome://messenger/content/FilterEditor.xul", "",
>                          "chrome, modal, resizable,centerscreen,dialog=yes", args);
(Note: dialog=yes is the same as dialog)
>                           "chrome,modal,resizable,centerscreen,dialog", args);

>  function MsgSynchronizeOffline()
>  {
> -    //dump("in MsgSynchronize() \n"); 
> +    //dump("in MsgSynchronize() \n");
dump the dump which has been commented out since forever.

>      window.openDialog("chrome://messenger/content/msgSynchronize.xul",
Two space indent.
> -          "", "centerscreen,chrome,modal,titlebar,resizable=yes",{msgWindow:msgWindow}); 		     
> +          "", "centerscreen,chrome,modal,titlebar,resizable=yes",{msgWindow:msgWindow});
resizable=yes is the same as resizable.

Suggestion:

function MsgSynchronizeOffline()
{
  window.openDialog("chrome://messenger/content/msgSynchronize.xul", "",
                    "centerscreen,chrome,modal,titlebar,resizable",
                    {msgWindow:msgWindow});
}

Comment 3

[Frank-Rainer Grahl (:frg)]

Attachment: 1260062-whitespace-removal-V2.patch

The new patch needs a new review. Just the one place wouldn't do so I cleaned up the indention and curly braces.

Two questions:

1.) What's the preferred style for else. Closing if curly brace on the line before or on the same line? If else with a comment I would prefer on the line before so you can clearly assign the comment with the else.

2.) If with one code line with curly braces or not. This is still inconsistent. Personally I prefer always with braces but that's just me. Sonar at work flags it without the braces.

>> Suggestion:
>> function MsgSynchronizeOffline()

Put the arguments mostly on separate lines. In some places some where already there and in others not. No preference. Let me know what you think.

Comment 4

[Philip Chee]

Comment on attachment 8737599 [details] [diff] [review]
1260062-whitespace-removal-V2.patch

> The new patch needs a new review. Just the one place wouldn't do so I
> cleaned up the indention and curly braces.

Meta: Follow the prevailing style of the file except if the code is very old and there is no consistent style. Then use the latest Mozilla house style modified by Neil and Mnyromyr.

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

History. Mnyromyr (Kasten) is the (nominal) module owner of the suite/mailnews code. His house style is:

function foobar()
{
...
}

if (foo)
{
...
}
else
{
...
}

try
{
...
}
catch (e)
{
...
}

* So yeah please undo all your changes to the cuddling {}.
* I prefer if you do a V1a patch for purely white space changes.
* I don't agree with the indentation fixup - at least in this bug.
* Some of your other changes are verging on refactoring. If you want to do a refactoring I prefer that you do a refactoring patch/bug instead of trying to sneak this in to a whitespace clean up bug.

> Two questions:
> 
> 1.) What's the preferred style for else. Closing if curly brace on the line
> before or on the same line? If else with a comment I would prefer on the
> line before so you can clearly assign the comment with the else.
> 
> 2.) If with one code line with curly braces or not. This is still
> inconsistent. Personally I prefer always with braces but that's just me.

Mnyromyr: with curly braces even with single line.
Neil: no curly braces if single line.
Mozilla: if one if/else if/else needs curly braces then all clauses must have curly braces.
If in doubt ask IanN (for mailnews code)

> >> Suggestion:
> >> function MsgSynchronizeOffline()
> 
> Put the arguments mostly on separate lines. In some places some where
> already there and in others not. No preference. Let me know what you think.

>  function MsgSynchronizeOffline()
>  {
> -    //dump("in MsgSynchronize() \n"); 
> -    window.openDialog("chrome://messenger/content/msgSynchronize.xul",
> -          "", "centerscreen,chrome,modal,titlebar,resizable=yes",{msgWindow:msgWindow}); 		     
> +  window.openDialog("chrome://messenger/content/msgSynchronize.xul",
> +                    "",
> +                    "centerscreen,chrome,modal,titlebar,resizable",
> +                    {msgWindow:msgWindow});

If all the arguments can fit on one line max 80 columns. Then all in one line.
Otherwise wrap arguments over additonal lines.
If the function has too many arguments use one argument per line to make things clearer. Use your judgement and/or let the reviewer decide.

Anything beyond whitespace cleanup should be discussed with IanN first. Plus anything beyond whitespace cleanup should go into a separate bug/patch anyway.

Sorry.

Comment 5

[Karsten Düsterloh]

Frank-Rainer,

as much as we do hate misaligned whitespace, we still usually don't do mere whitespace cleanup.

Why?

Well, <https://wiki.mozilla.org/SeaMonkey:MailNews:CodingStyle> says (among interesting stuff):

| Yes, many MailNews code is misaligned, uses tabs instead of spaces for indentation,
| has trailing whitespace and what not. If doing stuff in the vicinity, fine, do the 
| clean up. But don't touch it just for arbitrary whitespace correction, this just
| messes up the "whodunnit" history in our repository (and you don't want to look
| responsible for someone else's code, do you?).

Apart from that, noone knows all the code, sometimes you just have to ask the person who wrote it — and figuring out that person is much easier if the history just contains significant code changes. :-)


Also, as Philip already pointed out, mixing different problems in a single bug is not a good idea — the problems may need input from different persons or need to be discussed elsewhere or even may hide your fix for problem #1 behind a problem with problem #2! 
Furthermore, it's much easier to find and relate to an issue later if one bug contains just a single problem …

Comment 6

[Frank-Rainer Grahl (:frg)]

Karsten,

no problem. Just wanted to get rid of the trailing blanks and tabs initially. That stuff s*cks when you do patches for the file. Just thought I should align it because Ratty pointed out the one case and that there was one coding style for all. Actually you like me seem to be one of the few persons still alive who likes having the opening bracees in the next line directly under the if. No one seems to do this anymore so I have given up on doing this:)

Will put a v1a with the original changes probably up next weekend. maybe tomorrow if I some time.

Comment 7

[Frank-Rainer Grahl (:frg)]

Attachment: 1260062-whitespace-removal-V1a.patch

Just first patch with Rattys suggestions put in and I took the liberty to remove 2 additional dumps with comments in front.

Comment 8

[Philip Chee]

Comment on attachment 8738200 [details] [diff] [review]
1260062-whitespace-removal-V1a.patch

r=me

Comment 9

[Philip Chee]

http://hg.mozilla.org/comm-central/rev/2c5f9eb9486a