Last Comment Bug 61831 - bloat / duplicated code: ConvertBufToPlainText() in nsMsgCompUtils.cpp and nsMessenger.cpp
: bloat / duplicated code: ConvertBufToPlainText() in nsMsgCompUtils.cpp and n...
Status: RESOLVED FIXED
[good first bug]
: footprint
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All All
: P3 enhancement (vote)
: Thunderbird 12.0
Assigned To: :aceman
:
Mentors:
Depends on:
Blocks: 155219 181558
  Show dependency treegraph
 
Reported: 2000-12-02 12:08 PST by (not reading, please use seth@sspitzer.org instead)
Modified: 2012-01-18 01:07 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
[Patch] Showing the differences between the functions... (1.88 KB, patch)
2001-02-10 19:14 PST, Håkan Waara
no flags Details | Diff | Splinter Review
patch moving the function into nsMsgUtils.cpp (9.72 KB, patch)
2011-12-03 09:53 PST, :aceman
standard8: feedback+
Details | Diff | Splinter Review
patch v2, fixing problems (14.83 KB, patch)
2012-01-03 13:06 PST, :aceman
standard8: review+
Details | Diff | Splinter Review

Description (not reading, please use seth@sspitzer.org instead) 2000-12-02 12:08:54 PST
both compose/src/nsMsgCompUtils.cpp and base/src/nsMessenger.cpp contain static
functions ConvertBufToPlainText().

this should be in one place.
Comment 1 scottputterman 2001-02-02 14:24:29 PST
reassigning to ducarroz
Comment 2 Håkan Waara 2001-02-10 19:13:36 PST
As I saw that the function in nsMsgCompUtils.cpp was longer and took one more
parameter ("PRBool formatflowed"), I just replaced the one in nsMessenger.cpp
with that one and then made a patch to show the differences.

Hope this helps.. patch coming up next
Comment 3 Håkan Waara 2001-02-10 19:14:45 PST
Created attachment 24991 [details] [diff] [review]
[Patch] Showing the differences between the functions...
Comment 4 Håkan Waara 2001-03-30 14:02:49 PST
If y'all want some cleanup, then you should take a look at my attached diff
which shows the difference between those two functions.
Comment 5 Jean-Francois Ducarroz 2001-10-05 13:10:57 PDT
We should move this function somewhere into mailnews/base/utils
Comment 6 scottputterman 2001-10-09 13:23:43 PDT
moving to 1.0
Comment 7 scottputterman 2001-11-19 21:45:02 PST
moving to future but feel free to get this in as part of any code cleanup.
Comment 8 :aceman 2011-11-06 12:43:06 PST
This seems still relevant, I could look into it.

But I need to know whether the wrapWidth = 72 in nsMessenger.cpp should stay hardcoded as is now, or it should also obey the mailnews.wraplength pref as the version in nsMsgCompUtils.cpp does.

Then I would put the shared version into mailnews/base/util/nsMsgUtils.h (cpp) as that seems to be included from both files.
Comment 9 :aceman 2011-12-03 09:53:21 PST
Created attachment 578846 [details] [diff] [review]
patch moving the function into nsMsgUtils.cpp

This is the described proposal. However I do not know whether the function should be 'static' and whether the convertedText variable should be nsAutoString or nsString.
Comment 10 :Ms2ger (⌚ UTC+1/+2) 2011-12-03 10:03:47 PST
Comment on attachment 578846 [details] [diff] [review]
patch moving the function into nsMsgUtils.cpp

>--- a/mailnews/base/util/nsMsgUtils.cpp
>+++ b/mailnews/base/util/nsMsgUtils.cpp
>+#include "nsIParser.h"
>+#include "nsParserCIID.h"
>+#include "nsIHTMLToTextSink.h"
>+#include "nsIContentSink.h"
>+#include "nsICharsetConverterManager.h"
>+#include "nsIDocumentEncoder.h"
>+
>+static NS_DEFINE_CID(kCParserCID, NS_PARSER_CID);

All this should be at the top.

>+  nsresult rv;
>+  nsCOMPtr<nsIParser> parser = do_CreateInstance(kCParserCID, &rv);
>+  if (NS_SUCCEEDED(rv) && parser)

Early returns please.
Comment 11 :aceman 2011-12-03 11:03:27 PST
+    parser->Parse(aConBuf, 0, NS_LITERAL_CSTRING("text/html"), true);
+    //
+    // Now if we get here, we need to get from ASCII text to
+    // UTF-8 format or there is a problem downstream...
+    //
+    if (NS_SUCCEEDED(rv))
+    {
+      aConBuf = convertedText;
+    }
+  }

Can you tell me why rv is checked here? It seems rv is set and checked only at the top of the function. Here we already know if it is SUCCEEDED. I can't see rv could have been changed in any point since then.
Comment 12 :aceman 2011-12-03 13:46:54 PST
Comment on attachment 578846 [details] [diff] [review]
patch moving the function into nsMsgUtils.cpp

I'll surely make a new patch with ms2ger's fixes but need answer to my question to make it even better.
Comment 13 Mark Banner (:standard8) 2012-01-03 01:23:22 PST
Comment on attachment 578846 [details] [diff] [review]
patch moving the function into nsMsgUtils.cpp

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

::: mailnews/base/util/nsMsgUtils.cpp
@@ +2378,5 @@
> +      else if (wrapWidth < 10)
> +        wrapWidth = 10;
> +    }
> +
> +    converterFlags |= nsIDocumentEncoder::OutputFormatted;

In the nsMessenger case, this will actually change the conversion routine, and I think the output will be formatted rather than just converted, so therefore I think we either need to make the flags the argument, or add another bool.

@@ +2385,5 @@
> +      converterFlags |= nsIDocumentEncoder::OutputFormatFlowed;
> +
> +    nsCOMPtr<nsIContentSink> sink;
> +
> +    sink = do_CreateInstance(NS_PLAINTEXTSINK_CONTRACTID);

nit: Can just be on the same line as the variable definition.

@@ +2401,5 @@
> +    //
> +    // Now if we get here, we need to get from ASCII text to
> +    // UTF-8 format or there is a problem downstream...
> +    //
> +    if (NS_SUCCEEDED(rv))

Yeah, this check doesn't seem useful unless we start checking the return values of the parser - I'm not sure what the failure modes could be for that, but I guess we generally do the right thing at the moment.
Comment 14 :aceman 2012-01-03 02:18:38 PST
Thanks, can you also answer comment 9?
Comment 15 Mark Banner (:standard8) 2012-01-03 02:31:54 PST
(In reply to :aceman from comment #9)
> This is the described proposal. However I do not know whether the function
> should be 'static' and whether the convertedText variable should be
> nsAutoString or nsString.

The use of static on the function basically restricts the usage of the function to the file it is defined in (there's probably a bit than that) - hence you don't want it to be static.

Making the string an nsAutoString will mean it'll get a certain amount of memory pre-allocated on the stack. This can avoid some dynamic allocations if the string length doesn't go over pre-allocated limit (or can reduce, if the string length doesn't go over it straight away). I think it is fine to use nsAutoString here - some of the cases where it is used (e.g. signatures) could easily be within that pre-allocated limit.
Comment 16 :aceman 2012-01-03 13:06:01 PST
Created attachment 585528 [details] [diff] [review]
patch v2, fixing problems
Comment 17 :aceman 2012-01-10 04:57:01 PST
Thanks. This could be considered an interface change. Does it need superreview yet?
Comment 18 Mark Banner (:standard8) 2012-01-10 05:25:39 PST
(In reply to :aceman from comment #17)
> Thanks. This could be considered an interface change. Does it need
> superreview yet?

Nope, it doesn't touch an idl so you're fine there.
Comment 19 Mark Banner (:standard8) 2012-01-17 06:48:54 PST
Checked in: http://hg.mozilla.org/comm-central/rev/9bc16b440416
Comment 20 Jonathan Protzenko [:protz] 2012-01-18 01:00:35 PST
Any chance we can make this scriptable? See what lengths I have to go to provide a simple htmlToPlainText function: https://github.com/protz/thunderbird-stdlib/blob/master/compose.js#L243

Thanks,

jonathan
Comment 21 Mark Banner (:standard8) 2012-01-18 01:06:33 PST
(In reply to Jonathan Protzenko [:protz] from comment #20)
> Any chance we can make this scriptable? See what lengths I have to go to
> provide a simple htmlToPlainText function:
> https://github.com/protz/thunderbird-stdlib/blob/master/compose.js#L243

Well I don't see why you couldn't just re-implement what this function does... however, making it scriptable is what bug 181558 is about, and I wouldn't be against that happening.

Note You need to log in before you can comment on or make changes to this bug.