227633 - Replace 0x0a by nsCRT::LF, 0x0d by nsCRT::CR

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect, P4)

Description

[Serge Gautherie (:sgautherie)]

User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.6a) Gecko/20031030
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.6a) Gecko/20031030

Spun off from bug 119441 comment 25:
{
instead of 0x0A and 0x0D, consider using nsCRT::CR and nsCRT::LF, to make the
code more readable.
(if you want to fix the rest of the import code, that would be great, but that
can be spun off to a new bug.)
}


Reproducible: Always

Steps to Reproduce:

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1_Bw) Fixes most '/mailnews/' '0x0?' occurences (review only)

This does 1+ minor code optimizations too.

'review only':
Since all theses files are using Tabs,
the full patch has some more Tab->Space conversions,
only to better see the if/while/... blocks (involved in this patch).

Comment 2

[Frank Wein [:mcsmurf]]

Comment on attachment 136934 [details] [diff] [review]
(Av1_Bw) Fixes most '/mailnews/' '0x0?' occurences (review only)

>-		else {
>+        else
> 			m_pBuffer[offset] = *pData;
>-		}
>+

Also i'm no expert at coding and C(++), but this looks wrong :)

Comment 3

[[e-mail]]

Comment on attachment 136934 [details] [diff] [review]
(Av1_Bw) Fixes most '/mailnews/' '0x0?' occurences (review only)

nice work! is there any more of this to come for the rest of the tree?

i'd suggest asking for rs= on this, rather than sr=... saves the superreviewer
from reading the patch.

r=dwitte

Comment 4

[Serge Gautherie (:sgautherie)]

Attachment: (Av1a-Bw) <mailnews/*> (review only)

Av1_Bw, updated to current trunk.

Comment 5

[Serge Gautherie (:sgautherie)]

Comment on attachment 154285 [details] [diff] [review]
(Av1a-Bw) <mailnews/*> (review only)

Keeping
{{
(Av1_Bw) Fixes most '/mailnews/' '0x0?' occurences (review only)	 patch 
	2003-12-06 07:58 PDT	dwitte: review+
}}

Seth:
Dan suggests giving 'rs' would be appropriate: proceed as you see fit...

Then, I'll post a full diff for checkin...

Comment 6

[Serge Gautherie (:sgautherie)]

(In reply to comment #2)
> (From update of attachment 136934 [details] [diff] [review])
> >-		else {
> >+        else
> > 			m_pBuffer[offset] = *pData;
> >-		}
> >+
> 
> Also i'm no expert at coding and C(++), but this looks wrong :)

I'm only removing the brackets (and changing indentation)...
What would be wrong exactly ?

Comment 7

[Serge Gautherie (:sgautherie)]

(In reply to comment #3)
> (From update of attachment 136934 [details] [diff] [review])
> nice work! is there any more of this to come for the rest of the tree?

Yes, there should be ... depending on how this (first) patch goes.

Comment 8

[Serge Gautherie (:sgautherie)]

Matthias: Could you try to compile, and possibly test, this patch ? Thanks.

Comment 9

[Matthias Bockelkamp]

(In reply to comment #8)
> Matthias: Could you try to compile, and possibly test, this patch ? Thanks.

It compiles and the composer part seems to work (tested with a few signatures)
but I cannot test the import parts.

Comment 10

[Serge Gautherie (:sgautherie)]

(In reply to comment #9)
> (In reply to comment #8)
> > Matthias: Could you try to compile, and possibly test, this patch ? Thanks.
> 
> It compiles and the composer part seems to work (tested with a few signatures)
> but I cannot test the import parts.

Let's be optimistic !?

Seth:
Would you have time to rs (or sr) this patch ?

Comment 11

[Serge Gautherie (:sgautherie)]

Comment on attachment 154285 [details] [diff] [review]
(Av1a-Bw) <mailnews/*> (review only)

No super-review from <[e-mail]> since "2004-07-25" :-(

DanM:
DanW suggests giving 'rs' would be appropriate: proceed as you see fit...

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 154285 [details] [diff] [review]
(Av1a-Bw) <mailnews/*> (review only)

Nice work, and sorry for the delay.  rs=[e-mail].

Comment 13

[Serge Gautherie (:sgautherie)]

Attachment: (Av1b) <mailnews/*> (full diff)

Av1a-Bw, full diff for checkin,
and diffed to the current Trunk.

Keeping
{{
(Av1a-Bw) <mailnews/*> (review only)	 patch		2004-07-25 06:32 PDT   
21.24 KB	gautheri: review+
dmose: superreview+
}}

'approval1.8b4=?':
Trivial code cleanup, no risk.

Comment 14

[Benjamin Smedberg]

Comment on attachment 191128 [details] [diff] [review]
(Av1b) <mailnews/*> (full diff)

I'm concerned that we're replacing tabs with newlines out of context here...
let's hold off and do this in 1.9, but I would really prefer that we use tabs
in context unless we're really going to fix up blocks of whitespace together.

Comment 15

[Serge Gautherie (:sgautherie)]

Attachment: (Av1c) Replace 0x0A by nsCRT::LF, 0x0D by nsCRT::CR, 9 by '\t', and 32 by ' ', in mailnews/ [Checked in: Comment 17]

Av1b, unbitrotted.
"99%" of comment 14 suggestion(s) has been done in the meantime.

Included logic rewrites are:
*nsMsgCompose.cpp: merges 2 |*wPtr = ...|.
*nsEudoraCompose::FindNextEndLine(): moves 2 |(count < len)| to front.
*nsEudoraMailbox::ExamineAttachment(): moves 1 |(cnt < len)| to front.

(Mostly) Already r+ and rs+, but as that was 6.5 years ago...

Comment 16

[Serge Gautherie (:sgautherie)]

Comment on attachment 586694 [details] [diff] [review]
(Av1c) Replace 0x0A by nsCRT::LF, 0x0D by nsCRT::CR, 9 by '\t', and 32 by ' ', in mailnews/
[Checked in: Comment 17]

Succeeded as
http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=cb3971f9652d

Comment 17

[Serge Gautherie (:sgautherie)]

Comment on attachment 586694 [details] [diff] [review]
(Av1c) Replace 0x0A by nsCRT::LF, 0x0D by nsCRT::CR, 9 by '\t', and 32 by ' ', in mailnews/
[Checked in: Comment 17]

http://hg.mozilla.org/comm-central/rev/dba9d75af64f

Comment 18

[Serge Gautherie (:sgautherie)]

Ftr, as these seem fine as is:

http://mxr.mozilla.org/comm-central/search?string=0x0a&find=%2Fmailnews%2F
"Found 24 matching lines in 6 files"
http://mxr.mozilla.org/comm-central/search?string=0x0d&find=%2Fmailnews%2F
"Found 25 matching lines in 9 files"

V.Fixed, per MXR.