95955 - Remove extraneous ConvertLineEndings() call and cleanup of nsMsgCompose.cpp

Status: VERIFIED WONTFIX

(MailNews Core :: Composition, defect)

Description

[Håkan Waara]

I did some general cleanup to nsMsgCompose.cpp and caught an extraneous call to 
the expensive function ConvertLineEndings().

Patch coming up.

Requestion review from ducarroz and bienvenu.

Comment 1

[Håkan Waara]

Attachment: proposed patch

Comment 2

[Håkan Waara]

Oh, and I also made it so we don't go through ConvertLineEndings even for empty 
strings. Stuff like that.

Comment 3

[Håkan Waara]

Attachment: Better fix.

Comment 4

[Håkan Waara]

Varada spotted an error in my patch that my last patch fixes.

Requesting review from bienvenu/varada.

Comment 5

[Håkan Waara]

Ducarroz, can I get an r=?

Comment 6

[(not reading, please use [e-mail] instead)]

awaiting r from ducarroz before I do the sr.

Comment 7

[Håkan Waara]

Ok, requesting review from J-F Ducarroz.

Comment 8

[Jean-Francois Ducarroz]

Comment on attachment 47111 [details] [diff] [review]
Better fix.

Sorry for the delay, I was on vacation.

see comments in bug report:

Comment 9

[Jean-Francois Ducarroz]

1) instead of writing 3 times

+  if (!aPrefix.IsEmpty())
+    TranslateLineEnding(...);

would be better to do the check directly inside the TranslateLineEnding function.

2)
-    (void)TagEmbeddedObjects(aEditorShell);
+    TagEmbeddedObjects(aEditorShell);

The purpose of puting (void) in the front of the function is to clearly show
that we don't care of the result. Please do not remove it.

3)
+    if (aQuoted)
+    {
     nsCOMPtr<nsISelectionController> selCon;
     editor->GetSelectionController(getter_AddRefs(selCon));
 
     if (selCon)
-      selCon->ScrollSelectionIntoView(nsISelectionController::SELECTION_NORMAL,
nsISelectionController::SELECTION_ANCHOR_REGION);
+        selCon->ScrollSelectionIntoView(nsISelectionController::SELECTION_NORMAL,
+                                       
nsISelectionController::SELECTION_ANCHOR_REGION);
+    }

Why do you don't want to scroll the view when we are not quoting? Seems wrong to me!

4)
@@ -3860,8 +3865,6 @@
 
   if (!aSignature.IsEmpty())
   {
-    TranslateLineEnding(aSignature);
-
     editor->BeginTransaction();
     editor->EndOfDocument();
     if (m_composeHTML)

Why do you remove this line. We need to convert the signature line when the user
switch identity on the fly. That the purpose of this function. Again, seems
wrong to do that.

Comment 10

[Håkan Waara]

Thanks for the comments ducarroz. Here are some responses:

#1: agreed.

#2: Looks redundant to me; the compiler doesn't warn you and if you don't put a
variable assignment there, you obviously don't care.

#3: If you don't insert any quoted text (e.g., when replying to a long message)
then the email will start up blank except for the signature. What is there to
scroll then?

#4: That line is done twice in the same function call. First at the beginning of
the function we ConvertLineEndings() for the signature, then a little bit later
we do it again. Do you mean that we need to run it *twice* always for signatures?

Thanks

Comment 11

[Jean-Francois Ducarroz]

#2 I have to disagree. It's not redundant. That clearly show that the return
value is ignored by purpose. Imagine the case where you write code that use a
function that doesn't return a value. Later somebody change this function to
make it return a value but forget to change the caller! How can you distinc it
from the purpose case if you don't either put a comment or add (void)! Please do
not change this line of code. In general, avoid to changes that doesn't node
directly affect the execution. Thanks

#3 Wrong. We have cases, where you can open a new message already prefill with
data, like template and draft.

#4 maybe I don't have the same source code than you or I am missing something
but I don't see 2 calls to TranslateLineEnding() in the function
nsMsgCompose::SetSignature

Comment 12

[Håkan Waara]

Makes sense. I'll come up with a better patch soon. Thanks for explaining!

Comment 13

[Håkan Waara]

Attachment: better fix. incorporating ducarroz's suggestions

Comment 14

[Håkan Waara]

The new patch is basically just cleaning some stuff up, adding a nullcheck,
changing a string usage etc...

Requesting r= again from J-F Ducarroz. Thanks

Comment 15

[Jean-Francois Ducarroz]

Technically the patch is good. However I am not happy at all with the fact you change the formatting of the code.

Every programer has his/her own way to code and especially to format the code. You cannot come in somebody else 
code and just change everything just because you like it better that way. You must respect the formatting of the 
code and stick with it even when you add new lines of code. Try to stay focus to the essential.

Also, by doing that, you make the path bigger and therefore the review harder.

Therefore, please post a new patch with only the needed change. It should be something like that:

Index: src/nsMsgCompose.cpp
===================================================================
RCS file: /cvsroot/mozilla/mailnews/compose/src/nsMsgCompose.cpp,v
retrieving revision 1.287
diff -u -r1.287 nsMsgCompose.cpp
--- nsMsgCompose.cpp	2001/09/14 21:28:16	1.287
+++ nsMsgCompose.cpp	2001/09/16 00:01:11
@@ -133,6 +133,9 @@
 
 static void TranslateLineEnding(nsString& data)
 {
+  if (data.IsEmpty())
+    return;
+
   PRUnichar* rPtr;   //Read pointer
   PRUnichar* wPtr;   //Write pointer
   PRUnichar* sPtr;   //Start data pointer
@@ -2802,65 +2804,56 @@
 nsresult
 nsMsgCompose::BuildBodyMessageAndSignature()
 {
-  PRUnichar   *bod = nsnull;
-  nsresult	  rv = NS_OK;
+  nsXPIDLString body;
+  nsresult      rv = NS_OK;
 
   // 
   // This should never happen...if it does, just bail out...
   //
-  if (!m_editor)
-    return NS_ERROR_FAILURE;
+  NS_ENSURE_ARG_POINTER(m_editor);
 
   // 
   // Now, we have the body so we can just blast it into the
   // composition editor window.
   //
-  m_compFields->GetBody(&bod);
+  m_compFields->GetBody(getter_Copies(body));
 
   /* Some time we want to add a signature and sometime we wont. Let's figure that now...*/
   PRBool addSignature;
+
   switch (mType)
   {
-  	case nsIMsgCompType::New :
-  	case nsIMsgCompType::Reply :				/* should not append! but just in case */
-  	case nsIMsgCompType::ReplyAll :				/* should not append! but just in case */
-  	case nsIMsgCompType::ForwardAsAttachment :	/* should not append! but just in case */
-  	case nsIMsgCompType::ForwardInline :
-  	case nsIMsgCompType::NewsPost :
-  	case nsIMsgCompType::ReplyToGroup :
-  	case nsIMsgCompType::ReplyToSender : 
-  	case nsIMsgCompType::ReplyToSenderAndGroup :
-  		addSignature = PR_TRUE;
-  		break;
-
-  	case nsIMsgCompType::Draft :
-  	case nsIMsgCompType::Template :
-  		addSignature = PR_FALSE;
-  		break;
-  	
-  	case nsIMsgCompType::MailToUrl :
-  		addSignature = !(bod && *bod != 0);
-  		break;
-
-  	default :
-  		addSignature = PR_FALSE;
-  		break;
-  }
-
-  if (m_editor)
-  {
-    nsAutoString empty;
-    nsAutoString bodStr(bod);
-    nsAutoString tSignature;
-
-    if (addSignature)
-  	  ProcessSignature(m_identity, &tSignature);
+    case nsIMsgCompType::New :
+    case nsIMsgCompType::Reply :        /* should not append! but just in case */
+    case nsIMsgCompType::ReplyAll :       /* should not append! but just in case */
+    case nsIMsgCompType::ForwardAsAttachment :  /* should not append! but just in case */
+    case nsIMsgCompType::ForwardInline :
+    case nsIMsgCompType::NewsPost :
+    case nsIMsgCompType::ReplyToGroup :
+    case nsIMsgCompType::ReplyToSender : 
+    case nsIMsgCompType::ReplyToSenderAndGroup :
+      addSignature = PR_TRUE;
+      break;
+    
+    case nsIMsgCompType::MailToUrl :
+      addSignature = !body.IsEmpty();
+      break;
 
-    rv = ConvertAndLoadComposeWindow(m_editor, empty, bodStr, tSignature,
-                                       PR_FALSE, m_composeHTML);
+    case nsIMsgCompType::Draft :
+    case nsIMsgCompType::Template :
+    default :
+      addSignature = PR_FALSE;
+      break;
   }
+
+  nsAutoString empty, bodyStr(body), tSignature;
+
+  if (addSignature)
+    ProcessSignature(m_identity, &tSignature);
+
+  rv = ConvertAndLoadComposeWindow(m_editor, empty, bodyStr, tSignature,
+                                   PR_FALSE, m_composeHTML);
 
-  PR_FREEIF(bod);
   return rv;
 }
 

Comment 16

[Håkan Waara]

-> 0.9.6

Comment 17

[James Rome]

Something has regressed. In 2001102708, when I compose, the window shows several
extraneous letters below my tyoing on the right-hand edge. They are
unselectable. This never happened before.

Comment 18

[Håkan Waara]

That's another bug

Comment 19

[[e-mail] (gone - use [e-mail] instead)]

verified won'tfix (I guess?) hwaara, you're no longer considering fixing this?