Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in Thunderbird 12.0

Status

MailNews Core
Backend
P3
enhancement
RESOLVED FIXED
17 years ago
6 years ago

People

(Reporter: (not reading, please use seth@sspitzer.org instead), Assigned: aceman)

Tracking

(Blocks: 2 bugs, {footprint})

Trunk
Thunderbird 12.0
footprint
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment, 2 obsolete attachments)

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

this should be in one place.

Comment 1

17 years ago
reassigning to ducarroz
Assignee: rhp → ducarroz

Comment 2

17 years ago
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
OS: Linux → All
Hardware: PC → All

Comment 3

17 years ago
Created attachment 24991 [details] [diff] [review]
[Patch] Showing the differences between the functions...

Comment 4

17 years ago
If y'all want some cleanup, then you should take a look at my attached diff
which shows the difference between those two functions.
QA Contact: esther → stephend
We should move this function somewhere into mailnews/base/utils
Status: NEW → ASSIGNED
Keywords: footprint
Target Milestone: --- → mozilla0.9.6

Comment 6

16 years ago
moving to 1.0
Target Milestone: mozilla0.9.6 → mozilla1.0

Comment 7

16 years ago
moving to future but feel free to get this in as part of any code cleanup.
Target Milestone: mozilla1.0 → Future
Product: MailNews → Core
Assignee: ducarroz → nobody
Severity: normal → enhancement
Status: ASSIGNED → NEW
QA Contact: stephend → backend
Product: Core → MailNews Core
Whiteboard: [good first bug]
Target Milestone: Future → ---
(Assignee)

Comment 8

6 years ago
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.
Assignee: nobody → acelists
(Assignee)

Updated

6 years ago
Blocks: 181558
(Assignee)

Comment 9

6 years ago
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.
Attachment #578846 - Flags: review?(mbanner)
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
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.
(Assignee)

Comment 11

6 years ago
+    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.
(Assignee)

Comment 12

6 years ago
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.
Attachment #578846 - Flags: review?(mbanner) → feedback?(mbanner)
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.
Attachment #578846 - Flags: feedback?(mbanner) → feedback+
(Assignee)

Comment 14

6 years ago
Thanks, can you also answer comment 9?
(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.
(Assignee)

Updated

6 years ago
Blocks: 155219
(Assignee)

Comment 16

6 years ago
Created attachment 585528 [details] [diff] [review]
patch v2, fixing problems
Attachment #24991 - Attachment is obsolete: true
Attachment #578846 - Attachment is obsolete: true
Attachment #585528 - Flags: review?(mbanner)
Attachment #585528 - Flags: review?(mbanner) → review+
(Assignee)

Comment 17

6 years ago
Thanks. This could be considered an interface change. Does it need superreview yet?
(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.
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/9bc16b440416
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 12.0
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
(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.
You need to log in before you can comment on or make changes to this bug.