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

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[Jorg K (CEST = GMT+2)]

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: 1393219-isempty.patch - style nit [Landed: comment #3]

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1393219-test-typo.patch [Landed: comment #7]

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b97b47c64376
Style nit: Use IsEmpty() instead of Length()==0. r=me

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: 1393219-unused-uri.patch [Landed: comment #14]

I found this browsing code the other day in the context of bug 1242030 comment #216. It's part of creating a stream of patches for triggering builds after M-C merges.

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: 1393219-whitespace-mimedrft.patch

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: 1393219-whitespace-mimedrft.cpp (v2) [Landed: comment #7]

More.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b6e47292fdff
Typo fix. rs=typo-only
https://hg.mozilla.org/comm-central/rev/5a8396d48e9e
Fix white-space issues in mimedrft.cpp. rs=white-space-only

Comment 8

[Jorg K (CEST = GMT+2)]

Comment on attachment 8905438 [details] [diff] [review]
1393219-whitespace-mimedrft.cpp (v2) [Landed: comment #7]

The patch doesn't reflect 100% what was landed since I found some more.

Comment 9

[:aceman]

Comment on attachment 8905422 [details] [diff] [review]
1393219-unused-uri.patch [Landed: comment #14]

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

Unused variable? Nice

Comment 10

[:aceman]

Comment on attachment 8905438 [details] [diff] [review]
1393219-whitespace-mimedrft.cpp (v2) [Landed: comment #7]

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

Removing ugly spaces? Good :)

::: mailnews/mime/src/mimedrft.cpp
@@ +1915,4 @@
>  
>    if (!contLoc && !newAttachment->m_realName.IsEmpty())
>      workURLSpec = ToNewCString(newAttachment->m_realName);
> +  if ((contLoc) && (!workURLSpec))

These probably do not need so many backets.

Comment 11

[Jorg K (CEST = GMT+2)]

Attachment: 1393219-mimedrft-parenthesis.patch [Landed: comment #14]

Comment 12

[:aceman]

Comment on attachment 8906016 [details] [diff] [review]
1393219-mimedrft-parenthesis.patch [Landed: comment #14]

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

Thanks :)

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: 1393219-CaseInsensitiveCompare.patch

Remove define for CaseInsensitiveCompare and its use. Why?
It's confusing like hell since this is also a comparison function in M-C:
https://dxr.mozilla.org/mozilla-central/search?q=CaseInsensitiveCompare&redirect=false

Also, there are other comparison functions which look the same:
nsCaseInsensitiveStringComparator()

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/128de925dde2
remove superfluous parenthesis in mimedrft.cpp. r=aceman
https://hg.mozilla.org/comm-central/rev/4fc52379f051
Clean-up: Remove unused variable. r=aceman

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: 1393219-findchar.patch [Landed: Comment #27]

When I wrote this, I didn't know that FindChar() could take an offset. The reviewer didn't notice it either. I've tested this to avoid an "off-by-one" error.

Only occurrence in C-C:
https://dxr.mozilla.org/comm-central/search?q=regexp%3Asubstring.*find&redirect=false

Comment 16

[:aceman]

Comment on attachment 8906153 [details] [diff] [review]
1393219-findchar.patch [Landed: Comment #27]

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

If this fixes an error (changes behaviour), then it does not look like belonging into this bug.

Comment 17

[Jorg K (CEST = GMT+2)]

No error, no behaviour change, just removing clumsiness.

Comment 18

[:aceman]

Comment on attachment 8906153 [details] [diff] [review]
1393219-findchar.patch [Landed: Comment #27]

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

Thanks, I misread your comment, thinking you fix an off-by-one error, but you just tried to avoid introducing one.

Comment 19

[:aceman]

Comment on attachment 8906135 [details] [diff] [review]
1393219-CaseInsensitiveCompare.patch

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

I'm not particularly happy about this change. The variable could be useful as it explains what the 'true' value means to the casual reader. Maybe just renaming it would solve the problem you are after (clash with name of another function) ? Similarly you could use more of the kNotFound constant instead of -1 in the patch.

Comment 20

[Jorg K (CEST = GMT+2)]

OK, how about instead of s/CaseInsensitiveCompare/true/ make "/* ignoreCase = */ true like I had here:
https://hg.mozilla.org/comm-central/rev/def874d2d27c8fa1734f885059d29362b3990032#l1.12
which was inspired by this:
https://hg.mozilla.org/mozilla-central/rev/a1bcb487ffec#l2.13

Comment 21

[Jorg K (CEST = GMT+2)]

Attachment: 1393219-CaseInsensitiveCompare.patch (v2) [Landed: Comment #26]

How about this one then? Sometimes we explain arguments at the call site, but having a define for that purpose seems misguided.

Comment 22

[:aceman]

Comment on attachment 8906266 [details] [diff] [review]
1393219-CaseInsensitiveCompare.patch (v2) [Landed: Comment #26]

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

That could work, I've seen this elsewhere. But why was renaming not good?
Also please, use the kNotFound where possible.

Comment 23

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #19)
>  Similarly you could use more of the kNotFound constant
> instead of -1 in the patch.

Personally I think we should do more of the opposite, and use -1 more. If you use a KNotFound you have to "know" that that constant exists, and that it's defined in the current file/context. That not found is -1 is common knowledge, and anybody even unfamiliar with the code (but familiar with c++ and other programming languages) would understand it and be able to use that instantly.

Comment 24

[Jorg K (CEST = GMT+2)]

Let's not get caught up in a discussion about kNotFound vs. -1 vs. >= 0.

The point is, CaseInsensitiveCompare is really bad, it confused me yesterday, see:
https://hg.mozilla.org/comm-central/rev/9e30863be03af555dc65fadac63c02138707fb8e#l3.12
https://hg.mozilla.org/comm-central/rev/def874d2d27c8fa1734f885059d29362b3990032#l1.12
since I thought it was a comparison function like nsCaseInsensitiveStringComparator().
So under bustage-fix pressure I did it the M-C way, only to realise that indeed we have a #define for this from the quirky old days that no one understands any more:

- * The internal API expects nsCaseInsensitiveC?StringComparator() and true.
- * Redefine CaseInsensitiveCompare so that Find works.
- */
-#define CaseInsensitiveCompare true
-/**

What??

Comment 25

[Magnus Melin [:mkmelin]]

Comment on attachment 8906266 [details] [diff] [review]
1393219-CaseInsensitiveCompare.patch (v2) [Landed: Comment #26]

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

LGTM, r=mkmelin

::: mailnews/base/search/src/nsMsgBodyHandler.cpp
@@ +364,5 @@
>      // MIME type. message/rfc822 is best treated as a multipart with no proper
>      // boundary; since we only use boundaries for retriggering the headers,
>      // the lack of one can safely be ignored.
> +    else if (lowerCaseLine.Find("multipart/", /* ignoreCase = */ true) != -1 ||
> +        lowerCaseLine.Find("message/", /* ignoreCase = */ true) != -1)

nit: align these two lowerCaseLines while you're toughing this

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/27e6d777270f
remove define for CaseInsensitiveCompare. r=mkmelin

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2ee641160857
Use FindChar() with offset instead of extracting Substring. r=aceman

Comment 28

[Jorg K (CEST = GMT+2)]

I'm done here. More "Code clean-up: Style nits, typos and trailing spaces" in a new bug.