1463266 - Code clean-up: Style nits, typos and trailing spaces - take 3

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[Jorg K (CEST = GMT+2)]

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

To facilitate continuous integration, I will collect some patches here that can be landed if no other patch is available to trigger a build.

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1463266-mimei.patch

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1463266-mimei-ignore-ws.patch

Here a |hg qdiff -w| for easier review.

Comment 3

[:aceman]

Comment on attachment 8979395 [details] [diff] [review]
1463266-mimei.patch

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

Thanks for the cleanup.

::: mailnews/mime/src/mimei.cpp
@@ +563,5 @@
>              // place to put the define. The others seems to be in net.h
>              // but is that really really the right place? There is also
>              // a nsMimeTypes.h but that one isn't included. Bug?
> +            char *content_type_format = content_type_row ?
> +               MimeHeaders_get_parameter(content_type_row, "format", NULL,NULL) : 0;

Missing space? Also, can we use nullptr instead of the NULLs? I think BenB wrote in the email the file is parsed as C++ already (and has .cpp extension) so maybe it would work? It already uses 'false/true' bools which I think are not in plain C (may be in some new C versions).

@@ +1048,2 @@
>    else if (!child->superclass)
> +    return false;

Isn't it possible to drop this 'else' too?

@@ +1174,1 @@
>    else if (clearsigned_counts &&

Isn't it possible to drop this 'else' too?

@@ +1184,5 @@
>   */
>  bool
>  mime_crypto_stamped_p(MimeObject *obj)
>  {
>    if (!obj) return false;

This could be wrapped.

@@ +1231,5 @@
>  
>      if (part_begin)
>      {
>        for (; (*s && *s != '?' && *s != '&'); s++)
> +        ;

What? :) Maybe making this a 'while' with the body being 's++' would be more readable.

@@ +1398,5 @@
> +      obj->parent &&
> +      obj->parent->headers &&
> +      mime_typep(obj->parent,
> +                 (MimeObjectClass *)&mimeMultipartAppleDoubleClass))
> +    result = MimeHeaders_get_name(obj->parent->headers, obj->options);

Please add {} so the body is more explicit.
It is already better than the original, without indent it was questionable whether the 'result =' even belonged to the if().

@@ +1403,5 @@
>  
>    /* Else, if this part is itself an AppleDouble, and one of its children
>     has a name, then use that (check data fork first, then resource.) */
>    if (!result &&
> +    mime_typep(obj, (MimeObjectClass *)&mimeMultipartAppleDoubleClass))

More indenting needed to align below !result ?

@@ +1413,1 @@
>      result = MimeHeaders_get_name(cont->children[1]->headers, obj->options);

Indent here?

@@ +1419,1 @@
>      result = MimeHeaders_get_name(cont->children[0]->headers, obj->options);

Indent here?

@@ +1489,1 @@
>    if (! q) return 0;

Maybe drop the space?

@@ +1493,5 @@
>      const char *end, *value, *name_end;
>      for (end = q; *end && *end != '&'; end++)
>        ;
>      for (value = q; *value != '=' && value < end; value++)
>        ;

More candidates for a 'while'.

@@ +1571,4 @@
>     busted: here's a comparison of the old and new numberings with a pair
>     of hypothetical messages (one with a single part, and one with nested
>     containers.)
>     NEW:      OLD:  OR:

Does this block also need aligning with the block below?

@@ +1584,5 @@
> +  message/rfc822        1.4       -     4
> +  multipart/mixed       1.4.1     4     -
> +  text/plain            1.4.1.1   4.1   -
> +  image/jpeg            1.4.1.2   4.2   -
> +  text/plain            1.5       5     5

Nice.

@@ +1854,5 @@
>    const char *s = strrchr(url, '?');
>    if (s && !strncmp(s, "?type=application/x-message-display", sizeof("?type=application/x-message-display") - 1))
>    {
>      const char *nextTerm = strchr(s, '&');
> +    s = nextTerm ? nextTerm : s + strlen(s) - 1;

Is this meant to be 's = nextTerm ? nextTerm : (s + strlen(s) - 1);' or '(nextTerm ? nextTerm : s) + strlen(s) - 1;' ? I'd add the explicit ().

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0c20e0da5cb8
replace tabs with spaces in db/. rs=white-space-only

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/afa140bcba9f
replace tabs with spaces in mailnews/base. rs=white-space-only

Comment 6

[Jorg K (CEST = GMT+2)]

This must be some of our ugliest code. I'm not even sure whether mailnews/base/src/nsSubscribeDataSource.cpp is still used.

For me the winner was:
https://hg.mozilla.org/comm-central/rev/afa140bcba9f#l1.238
-/* B1AA0820-D04B-11d2-8069-006008128C4E */
 #define NS_MSGSTATUSFEEDBACK_CID \
-{ 0xbd85a417, 0x5433, 0x11d3, \
-  {0x8a, 0xc5, 0x0, 0x60, 0xb0, 0xfc, 0x4, 0xd2} }
+{ 0xbd85a417, 0x5433, 0x11d3,    \
+{ 0x8a, 0xc5, 0x0, 0x60, 0xb0, 0xfc, 0x4, 0xd2 } }

So a needless comment repeating the numbers which follow, and the comment was wrong :-(

More tabs in mailnews/addrbook, mailnews/compose, mailnews/db, mailnews/extensions, mailnews/imap, mailnews/mapi, mailnews/mime and mailnews/news.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a2c9c8bdb42b
fix inconsistent closing brackets in nsMsgBaseCID.h. rs=white-space-only DONTBUILD

Comment 8

[:aceman]

The plan is to kill nsSubscribeDataSource.cpp in the subscribe rewrite bug.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7692c0e19b39
replace tabs with spaces in mailnews/addrbook. rs=white-space-only

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7148c6f9fce2
replace tabs with spaces in mailnews/extensions+mapi+mime+news. rs=white-space-only

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1c587b61bc8b
replace tabs with spaces in mailnews/addrbook+base+extensions+import+intl+local+test. rs=white-space-only

Comment 12

[Jorg K (CEST = GMT+2)]

There will be tree more patches in mailnews: compose, db, imap.
There are some tabs in mail, some in editor, and an awful lot in ldap.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4586a5565e52
replace tabs with spaces in editor/. rs=white-space-only
https://hg.mozilla.org/comm-central/rev/7b16aeb90747
replace tabs with spaces in mail/. rs=white-space-only

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/53797651df65
replace tabs with spaces in mailnews/db. rs=white-space-only

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/78f99b05889a
replace tabs with spaces in mail/. rs=white-space-only DONTBUILD

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1940b43f2306
replace tabs with spaces in mailnews/compose. rs=white-space-only

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a6474bd264e6
replace tabs with spaces in mailnews/imap. rs=white-space-only

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3d4271853049
replace tabs with spaces in mailnews/imap (follow-up). rs=white-space-only
https://hg.mozilla.org/comm-central/rev/4b27cf3e7225
replace tabs with spaces in comm-central in IDL files. rs=white-space-only

Comment 19

[Jorg K (CEST = GMT+2)]

Attachment: 1463266-mimei.patch (v2)

Addressed review comments.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=02eb547cbf26bc2974a8bacd82e587ce8a955263

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/01215911a27e
code clean-up in mimei.cpp. r=aceman

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5ebc1943af38
replace tabs with spaces in ldap/. rs=white-space-only

Comment 22

[Philipp Preißer-Kewisch [:Fallen][☁️📆]]

afaik ldap contains an upstream library, did we really want to diverge from it substantially by converting tabs/spaces?

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/829b7ff18e5b
Follow-up: aligned constants in ldaprot.h. rs=white-space-only DONTBUILD

Comment 24

[Jorg K (CEST = GMT+2)]

(In reply to Philipp Kewisch [:Fallen]  from comment #22)
> afaik ldap contains an upstream library, did we really want to diverge from
> it substantially by converting tabs/spaces?
Good question. I think the version we have is really old and if we ever did something to LDAP, we'd replace it completely. If we'd ever have to compare the current version with a new one, "-w" will take care, don't you think? Also, chances are the the new version also corrected white-space issues.

Tabs are a real pain, especially in LDAP where they could have meant 1, 2, 4 or 8 spaces, so I'm glad they're gone now.

Comment 25

[Jorg K (CEST = GMT+2)]

I needed to remove a few tabs for another patch to apply:
https://hg.mozilla.org/releases/comm-beta/rev/be58f31115b63d29b5ceef414bb2447c4c138903 (BETA_60_CONTINUATION)
replace tabs with spaces in nsSubscribableServer.cpp. rs=white-space-only a=jorgk DONTBUILD

Comment 26

[Jorg K (CEST = GMT+2)]

(In reply to Pulsebot from comment #20)
> Pushed by [e-mail]:
> https://hg.mozilla.org/comm-central/rev/01215911a27e
> code clean-up in mimei.cpp. r=aceman
Uplifted that to beta so other patches can apply:
https://hg.mozilla.org/releases/comm-beta/rev/a195e8eaa188c57c067a2d8106ba6f4e3e227f96 - BETA_60_CONTINUATION

Comment 27

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/comm-esr60/rev/8e13945b53ed
https://hg.mozilla.org/releases/comm-esr60/rev/b80983a145ca

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/cfc93bec7d61
Unify spelling of 'normalized'. rs=comment-only,typo-fix

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1e38647496ae
fix some white-space issues in nsAddrDatabase.cpp. rs=white-space-only

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/accc44126f08
fix another white-space issue in nsAddrDatabase.cpp. rs=white-space-only

Comment 31

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/comm-beta/rev/7ebd318d99dac3805458096431ef1f5bf8aa6633
https://hg.mozilla.org/releases/comm-beta/rev/505528fac6296ffb436caf0b8c711df8d8c5f621
Landed the white-space fixes so other patched apply easier.

Comment 32

[Jorg K (CEST = GMT+2)]

Attachment: 1463266-nsPromiseFlat.patch

It's not really clear what the difference is, but PromiseFlat[C]String is used in all other cases, so let's get rid of these two here.

Comment 33

[:aceman]

Comment on attachment 9010156 [details] [diff] [review]
1463266-nsPromiseFlat.patch

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

Seems to me nsPromiseFlatString is a type, so the occurences you found look like a cast.
While PromiseFlatString() may be a function that returns a safe copy of the string.

There are only 3 occurences of nsPromiseFlatString() use in m-c in https://dxr.mozilla.org/comm-central/source/dom/base/ImageEncoder.cpp, so yes, it looks proper to convert to PromiseFlatString().

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ca9d4cde9481
Change nsPromiseFlat[C]String to PromiseFlat[C]String for consistency. r=aceman

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/86c4629ad6e4
fix white-space/newline issue in calendar-unifinder-todo.xul. rs=white-space-only

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/00ddcafd807a
Fix white-space issue in nsMsgBodyHandler::StripHtml(). rs=white-space-only

Comment 37

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/comm-esr60/rev/e307f54e3a2e6dc7e9b28acf213650180ce2a4f5
https://hg.mozilla.org/releases/comm-esr60/rev/6e551bbe21e8d20071592c708b653da658670a7c
Landed the white-space fixes so other patched apply easier.

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3be6410d75a4
fix yet another white-space issue in nsAddrDatabase.cpp. rs=white-space-only DONTBUILD

Comment 39

[:aceman]

Attachment: 1463266-dateFormat.js

Cleanup in mailnews/base/content/dateFormat.js

Comment 40

[Jorg K (CEST = GMT+2)]

Comment on attachment 9030112 [details] [diff] [review]
1463266-dateFormat.js

Thanks.

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9e7cf2d56e15
clean up white-space in mailnews/base/content/dateFormat.js. r=jorgk

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/14af8a6f714b
remove unneeded namespace qualifier in nsMsgCompose.cpp. r=me

Comment 43

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/comm-esr60/rev/514b38608a948c783d0e2fc26b75c6283b2eb36a
replace tabs with spaces in comm-central in IDL files. rs=white-space-only

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6d5f99e38a3f
remove \n in NS_ASSERTION(). r=me

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/708086efbb2c
fix white-space issue/indentation in comment in nsMsgAttachmentHandler.cpp. rs=comment-only DONTBUILD

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/bbc858a8e20d
remove trailing spaces in ldap. rs=white-space-only

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0db6d6cc4f92
fix typos in mailnews/ using codespell. rs=comment-only,typo-fix
https://hg.mozilla.org/comm-central/rev/73ecaa37ed5b
fix typos in mail/ using codespell. rs=comment-only,typo-fix
https://hg.mozilla.org/comm-central/rev/599d4d27b9ff
fix typos in ldap/ using codespell. rs=comment-only,typo-fix
https://hg.mozilla.org/comm-central/rev/18cc85ecd842
fix typos in chat/, common/ and editor/ using codespell. rs=comment-only,typo-fix
https://hg.mozilla.org/comm-central/rev/a90d5a8ce30d
fix typos in db/ using codespell. rs=comment-only,typo-fix DONTBUILD

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6d861eaffbe9
Fix wrong indentation in Windows8WindowFrameColor.jsm. rs=white-space-only

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/93224a560c9c
Fix some spelling mistakes. rs=comment-only,typo-fix DONTBUILD

Comment 50

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0532fb6e8fa3
Fix comments in mimetext.cpp. rs=comment-only DONTBUILD

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7f8bd17d9565
remove trailing spaces from IDL files. rs=white-space-only DONTBUILD

Comment 52

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/comm-beta/rev/c537c44e9aa8d077f3703fba7ece37a94bf7c3b5

Comment 53

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8448cd61c29f
Fix/remove some ugly/unneeded comments in nsMsgCompose.cpp. rs=comment-only

Comment 54

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2c82043b716d
Fix some ugly comments in nsMsgAttachment*.cpp. rs=comment-only

Comment 55

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/fbf344af33cf
Fix some ugly comments in nsMsgCompUtils.cpp. rs=comment-only DONTBUILD

Comment 56

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/328093467a65
remove \n in MOZ_LOG and NS_ERROR. r=me DONTBUILD

Comment 57

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7d63ed41d20c
remove \n in MOZ_LOG and NS_ERROR. r=me DONTBUILD

Comment 58

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0d0c445e5077
remove \n in MOZ_LOG. r=me

Comment 59

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1edda72532b4
Remove space before function arguments in IDL files. rs=white-space-only DONTBUILD

Comment 60

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e4d9fac14a30
fix typos in comm-central using codespell. rs=comment-only,typo-fix

Comment 61

[Jorg K (CEST = GMT+2)]

Also pushed to the branches:
https://hg.mozilla.org/releases/comm-beta/rev/f72d9fcf281ea6f25557e6be94e63151d311fb5c
https://hg.mozilla.org/releases/comm-esr68/rev/8deffa88139ba19c7f30254eb202ddca2bd8c9fd

Comment 62

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e5ccd3ff3f18
fix typos in comm-central using codespell. rs=comment-only,typo-fix

Comment 63

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/comm-esr68/rev/78387a115b8a94f0fb743aa98dfba200fb88b401 (LDAP bits)

Comment 64

[:aceman]

Attachment: 1463266-MailMigrator.patch

Some trivial stuff to land when needed :)

Comment 65

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/fd13b1c3b79c
Fix some typos in MailMigrator.jsm. r=darktrojan