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

ASSIGNED
Assigned to

Status

enhancement
ASSIGNED
Last year
2 months ago

People

(Reporter: jorgk, Assigned: jorgk)

Tracking

({leave-open})

Trunk

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Assignee

Description

Last year
+++ 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.
Assignee

Updated

Last year
Keywords: leave-open
Assignee

Comment 1

Last year
Posted patch 1463266-mimei.patch (obsolete) — Splinter Review
Attachment #8979395 - Flags: review?(acelists)
Assignee

Comment 2

Last year
Posted patch 1463266-mimei-ignore-ws.patch (obsolete) — Splinter Review
Here a |hg qdiff -w| for easier review.

Comment 3

Last year
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 ().
Attachment #8979395 - Flags: review?(acelists) → review+

Comment 4

Last year
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0c20e0da5cb8
replace tabs with spaces in db/. rs=white-space-only

Comment 5

Last year
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/afa140bcba9f
replace tabs with spaces in mailnews/base. rs=white-space-only
Assignee

Comment 6

Last year
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

Last year
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/a2c9c8bdb42b
fix inconsistent closing brackets in nsMsgBaseCID.h. rs=white-space-only DONTBUILD

Comment 8

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

Comment 9

Last year
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/7692c0e19b39
replace tabs with spaces in mailnews/addrbook. rs=white-space-only

Comment 10

Last year
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/7148c6f9fce2
replace tabs with spaces in mailnews/extensions+mapi+mime+news. rs=white-space-only

Comment 11

Last year
Pushed by mozilla@jorgk.com:
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
Assignee

Comment 12

Last year
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

Last year
Pushed by mozilla@jorgk.com:
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

Last year
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/53797651df65
replace tabs with spaces in mailnews/db. rs=white-space-only

Comment 15

Last year
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/78f99b05889a
replace tabs with spaces in mail/. rs=white-space-only DONTBUILD

Comment 16

Last year
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/1940b43f2306
replace tabs with spaces in mailnews/compose. rs=white-space-only

Comment 17

Last year
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/a6474bd264e6
replace tabs with spaces in mailnews/imap. rs=white-space-only

Comment 18

Last year
Pushed by mozilla@jorgk.com:
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
Assignee

Comment 19

Last year
Addressed review comments.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=02eb547cbf26bc2974a8bacd82e587ce8a955263
Assignee: nobody → jorgk
Attachment #8979395 - Attachment is obsolete: true
Attachment #8979396 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8985421 - Flags: review+

Comment 20

Last year
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/01215911a27e
code clean-up in mimei.cpp. r=aceman

Comment 21

Last year
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/5ebc1943af38
replace tabs with spaces in ldap/. rs=white-space-only
afaik ldap contains an upstream library, did we really want to diverge from it substantially by converting tabs/spaces?

Comment 23

Last year
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/829b7ff18e5b
Follow-up: aligned constants in ldaprot.h. rs=white-space-only DONTBUILD
Assignee

Comment 24

Last year
(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.
Assignee

Comment 25

Last year
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
Assignee

Comment 26

Last year
(In reply to Pulsebot from comment #20)
> Pushed by mozilla@jorgk.com:
> 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 28

10 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/cfc93bec7d61
Unify spelling of 'normalized'. rs=comment-only,typo-fix

Comment 29

10 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/1e38647496ae
fix some white-space issues in nsAddrDatabase.cpp. rs=white-space-only

Comment 30

10 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/accc44126f08
fix another white-space issue in nsAddrDatabase.cpp. rs=white-space-only
Assignee

Comment 32

9 months ago
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.
Attachment #9010156 - Flags: review?(acelists)

Comment 33

9 months ago
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().
Attachment #9010156 - Flags: review?(acelists) → review+

Comment 34

9 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/ca9d4cde9481
Change nsPromiseFlat[C]String to PromiseFlat[C]String for consistency. r=aceman

Comment 35

9 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/86c4629ad6e4
fix white-space/newline issue in calendar-unifinder-todo.xul. rs=white-space-only

Comment 36

8 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/00ddcafd807a
Fix white-space issue in nsMsgBodyHandler::StripHtml(). rs=white-space-only

Comment 38

8 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/3be6410d75a4
fix yet another white-space issue in nsAddrDatabase.cpp. rs=white-space-only DONTBUILD

Comment 39

7 months ago
Cleanup in mailnews/base/content/dateFormat.js
Attachment #9030112 - Flags: review?(jorgk)
Assignee

Comment 40

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

Thanks.
Attachment #9030112 - Flags: review?(jorgk) → review+
Assignee

Updated

7 months ago
Keywords: checkin-needed

Comment 41

7 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/9e7cf2d56e15
clean up white-space in mailnews/base/content/dateFormat.js. r=jorgk
Keywords: checkin-needed

Comment 42

5 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/14af8a6f714b
remove unneeded namespace qualifier in nsMsgCompose.cpp. r=me
Assignee

Comment 43

5 months ago

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

5 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/6d5f99e38a3f
remove \n in NS_ASSERTION(). r=me

Comment 45

5 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/708086efbb2c
fix white-space issue/indentation in comment in nsMsgAttachmentHandler.cpp. rs=comment-only DONTBUILD

Comment 46

3 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/bbc858a8e20d
remove trailing spaces in ldap. rs=white-space-only

Comment 47

3 months ago
Pushed by mozilla@jorgk.com:
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

3 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/6d861eaffbe9
Fix wrong indentation in Windows8WindowFrameColor.jsm. rs=white-space-only

Comment 49

2 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/93224a560c9c
Fix some spelling mistakes. rs=comment-only,typo-fix DONTBUILD

Comment 50

2 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0532fb6e8fa3
Fix comments in mimetext.cpp. rs=comment-only DONTBUILD
You need to log in before you can comment on or make changes to this bug.