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

RESOLVED FIXED in Thunderbird 57.0

Status

Thunderbird
General
RESOLVED FIXED
10 months ago
9 months ago

People

(Reporter: Jorg K (GMT+2), Assigned: Jorg K (GMT+2))

Tracking

Trunk
Thunderbird 57.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 2 obsolete attachments)

(Assignee)

Description

10 months ago
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)

Comment 1

10 months ago
Created attachment 8900458 [details] [diff] [review]
1393219-isempty.patch - style nit [Landed: comment #3]
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
(Assignee)

Comment 2

10 months ago
Created attachment 8900465 [details] [diff] [review]
1393219-test-typo.patch [Landed: comment #7]

Comment 3

10 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/b97b47c64376
Style nit: Use IsEmpty() instead of Length()==0. r=me
(Assignee)

Updated

10 months ago
Attachment #8900458 - Attachment description: 1393219-isempty.patch - style nit → 1393219-isempty.patch - style nit [Landed: comment #3]
(Assignee)

Comment 4

10 months ago
Created attachment 8905422 [details] [diff] [review]
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.
Attachment #8905422 - Flags: review?(acelists)
(Assignee)

Comment 5

10 months ago
Created attachment 8905432 [details] [diff] [review]
1393219-whitespace-mimedrft.patch
(Assignee)

Updated

10 months ago
Attachment #8905432 - Attachment description: 1393219-whitespace-mimedrft.cpp → 1393219-whitespace-mimedrft.patch
Attachment #8905432 - Attachment filename: 1393219-whitespace-mimedrft.cpp → 1393219-whitespace-mimedrft.patch
Attachment #8905432 - Attachment is patch: true
(Assignee)

Comment 6

10 months ago
Created attachment 8905438 [details] [diff] [review]
1393219-whitespace-mimedrft.cpp (v2) [Landed: comment #7]

More.
Attachment #8905432 - Attachment is obsolete: true

Comment 7

10 months ago
Pushed by mozilla@jorgk.com:
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
(Assignee)

Updated

10 months ago
Attachment #8900465 - Attachment description: 1393219-test-typo.patch → 1393219-test-typo.patch [Landed: comment #7]
(Assignee)

Comment 8

10 months ago
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.
Attachment #8905438 - Attachment description: 1393219-whitespace-mimedrft.cpp (v2). → 1393219-whitespace-mimedrft.cpp (v2) [Landed: comment #7]

Comment 9

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

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

Unused variable? Nice
Attachment #8905422 - Flags: review?(acelists) → review+

Comment 10

10 months ago
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.
Attachment #8905438 - Flags: review+
(Assignee)

Comment 11

9 months ago
Created attachment 8906016 [details] [diff] [review]
1393219-mimedrft-parenthesis.patch [Landed: comment #14]
Attachment #8906016 - Flags: review?(acelists)

Comment 12

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

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

Thanks :)
Attachment #8906016 - Flags: review?(acelists) → review+
(Assignee)

Comment 13

9 months ago
Created attachment 8906135 [details] [diff] [review]
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()
Attachment #8906135 - Flags: review?(acelists)

Comment 14

9 months ago
Pushed by mozilla@jorgk.com:
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
(Assignee)

Updated

9 months ago
Attachment #8905422 - Attachment description: 1393219-unused-uri.patch → 1393219-unused-uri.patch [Landed: comment #14]
(Assignee)

Updated

9 months ago
Attachment #8906016 - Attachment description: 1393219-mimedrft-parenthesis.patch → 1393219-mimedrft-parenthesis.patch [Landed: comment #14]
(Assignee)

Comment 15

9 months ago
Created attachment 8906153 [details] [diff] [review]
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
Attachment #8906153 - Flags: review?(acelists)

Comment 16

9 months ago
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.
(Assignee)

Comment 17

9 months ago
No error, no behaviour change, just removing clumsiness.

Comment 18

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

Comment 19

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

Updated

9 months ago
Attachment #8900465 - Flags: review+

Updated

9 months ago
Attachment #8900458 - Flags: review+
(Assignee)

Comment 20

9 months ago
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
(Assignee)

Comment 21

9 months ago
Created attachment 8906266 [details] [diff] [review]
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.
Attachment #8906135 - Attachment is obsolete: true
Attachment #8906135 - Flags: feedback?(mkmelin+mozilla)
Attachment #8906266 - Flags: review?(acelists)

Comment 22

9 months ago
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

9 months ago
(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.
(Assignee)

Comment 24

9 months ago
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??
(Assignee)

Updated

9 months ago
Attachment #8906266 - Flags: review?(mkmelin+mozilla)

Comment 25

9 months ago
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
Attachment #8906266 - Flags: review?(mkmelin+mozilla) → review+

Comment 26

9 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/27e6d777270f
remove define for CaseInsensitiveCompare. r=mkmelin
(Assignee)

Updated

9 months ago
Attachment #8906266 - Attachment description: 1393219-CaseInsensitiveCompare.patch (v2) → 1393219-CaseInsensitiveCompare.patch (v2) [Landed: Comment #26]
Attachment #8906266 - Flags: review?(acelists)

Comment 27

9 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2ee641160857
Use FindChar() with offset instead of extracting Substring. r=aceman
(Assignee)

Updated

9 months ago
Attachment #8906153 - Attachment description: 1393219-findchar.patch → 1393219-findchar.patch [Landed: Comment #27]
(Assignee)

Comment 28

9 months ago
I'm done here. More "Code clean-up: Style nits, typos and trailing spaces" in a new bug.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 57.0
You need to log in before you can comment on or make changes to this bug.