61831 - bloat / duplicated code: ConvertBufToPlainText() in nsMsgCompUtils.cpp and nsMessenger.cpp

Status: RESOLVED FIXED

(MailNews Core :: Backend, enhancement, P3)

Description

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

both compose/src/nsMsgCompUtils.cpp and base/src/nsMessenger.cpp contain static
functions ConvertBufToPlainText().

this should be in one place.

Comment 1

[scottputterman]

reassigning to ducarroz

Comment 2

[Håkan Waara]

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]

Attachment: [Patch] Showing the differences between the functions...

Comment 4

[Håkan Waara]

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]

We should move this function somewhere into mailnews/base/utils

Comment 6

[scottputterman]

moving to 1.0

Comment 7

[scottputterman]

moving to future but feel free to get this in as part of any code cleanup.

Comment 8

[:aceman]

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]

Attachment: 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 (he/him; ⌚ UTC+1/+2)]

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]

+    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]

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)]

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]

Thanks, can you also answer comment 9?

Comment 15

[Mark Banner (:standard8)]

(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]

Attachment: patch v2, fixing problems

Comment 17

[:aceman]

Thanks. This could be considered an interface change. Does it need superreview yet?

Comment 18

[Mark Banner (:standard8)]

(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)]

Checked in: http://hg.mozilla.org/comm-central/rev/9bc16b440416

Comment 20

[Jonathan Protzenko [:protz]]

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)]

(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.