326809 - modernize nsIMsgHeaderParser - |string| and |wstring| vs AString and AUTF8String

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect, P4)

Description

[Jungshik Shin]

While trying to figure out hwo to fix bug 254519, I realized that nsIMsgHeaderParser has a baroque interface littered with |string| and |wstring| where AString and AUTF8String are either better or should be used. Besides, a bunch of methods have |charset| parameter which is always set to 'UTF-8' by all callers. That is, it needs to be removed.

Comment 1

[Magnus Melin [:mkmelin]]

Charset params removed by bug 458692.

Comment 2

[Mark Banner (:standard8)]

Attachment: [checked in] Proposed Fix for extractHeaderAddressMailboxes

First part of resolving this. Fix extractHeaderAddressMailboxes.

I have currently changed this interface to an ACString - I've been debating about whether or not to make the interface an AUTF8String. Part of me says we've not got mailnews set up for idn in mailbox addresses yet (and the specs are quite young I believe), the other part of me says we should be heading towards it. Thoughts?

Also includes testcase, some of the testcases are based on emails from the existing test that I extracted from RFC 2822. The only difference in before and after was changing:

  do_check_eq(parser.extractHeaderAddressMailboxes(""), null);
to
  do_check_eq(parser.extractHeaderAddressMailboxes(""), "");

Next on the list will be extractHeaderAddressNames and extractHeaderAddressName which shouldn't be noscript functions.

Comment 3

[[e-mail]]

Comment on attachment 342807 [details] [diff] [review]
[checked in] Proposed Fix for extractHeaderAddressMailboxes

>+  int status = msg_parse_Header_addresses(nsCString(aLine).get(), NULL, &addrs);
Please use PromiseFlatCString

>+  result.SetLength(size);
>   s = addrs;
>+  char* out = result.BeginWriting();
Annoyingly using the external API you could probably write
char* out = aResult.BeginWriting(size);

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 342807 [details] [diff] [review]
[checked in] Proposed Fix for extractHeaderAddressMailboxes

Checked in a few days ago (17th Oct), changeset id 631:e43b70b86507

Comment 5

[Mark Banner (:standard8)]

Attachment: Fix extractHeaderAddressName and extractHeaderAddressNames

Next patch in the series. Definitely AUTF8String this time as we're dealing with names and we do support unicode/UTF8 on those, also dropped the noscript as I couldn't see any reason for it.

The nsMsgDatabase function, I decided I ought to add a null-check in (as we're using nsDependentCString) and ended up reorganising the function.

nsParseMailMessageState::InternRfc822 is just a dead function.

I'll attach a diff -w in a moment.

Comment 6

[Mark Banner (:standard8)]

Attachment: Fix extractHeaderAddressName and extractHeaderAddressNames (diff -w)

Comment 7

[David :Bienvenu]

this doesn't look right:

     nsIMsgHeaderParser *headerParser = GetHeaderParser();
     if (headerParser)
-    {
+    return NS_ERROR_FAILURE;
+

Comment 8

[David :Bienvenu]

You can use PR_Free here - you don't care about nulling out addr and name since you're returning.

+  PR_FREEIF(addr);
+  PR_FREEIF(name);
+  return NS_OK;

Otherwise, it looks ok, except that if the headerParser check above is wrong, it should hav triggered a test failure.

Comment 9

[Mark Banner (:standard8)]

Attachment: [checked in] Fix extractHeaderAddressName and extractHeaderAddressNames v2

Updated patch to fix David's comments. The incorrect if statement wasn't picked up, because that requires us to hit the bit of code that is active when we are sorting a view based on From, and afaik we haven't really got any tests near that yet.

Comment 10

[David :Bienvenu]

Comment on attachment 344171 [details] [diff] [review]
[checked in] Fix extractHeaderAddressName and extractHeaderAddressNames v2

ah, makes sense. I need to see how hard it would be to add some view tests.

Comment 11

[[e-mail]]

Comment on attachment 344171 [details] [diff] [review]
[checked in] Fix extractHeaderAddressName and extractHeaderAddressNames v2

>+  nsresult rv = RowCellColumnToConstCharPtr(row, colToken, &cSender);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  // Just in case this is null
>+  if (!cSender)
>+    return NS_ERROR_NULL_POINTER;
Nit: RowCellColumnToConstCharPtr never sets cSender to null. (It does however fail to set it completely if row is null...)

>+  int status = msg_parse_Header_addresses(PromiseFlatCString(aLine).get(),
>+                                          &name, &addr, PR_FALSE, PR_FALSE,
>+                                          PR_TRUE);
>   if (status <= 0)
>-    return 0;
>+    return NS_ERROR_FAILURE;
return status; or return NS_ERROR_OUT_OF_MEMORY; seems more appropriate.

>+  aResult = (name && *name) ? name : addr;
>+
>+  PR_Free(name);
>+  PR_Free(addr);
I think name can only be null (as distinct from empty) on failure. (PR_Free might not be null-safe, but that's OK since I don't think these are null.)

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 344171 [details] [diff] [review]
[checked in] Fix extractHeaderAddressName and extractHeaderAddressNames v2

Checked in with Neil's nits fixed: http://hg.mozilla.org/comm-central/rev/5e0ce752f207

Comment 13

[Mark Banner (:standard8)]

Attachment: [checked in] Fix removeDuplicateAddresses

bwinton was asking for this to be accessible from js for his patch for the tb3 blocker - bug 45715.

Patch does:
* Drop noscript from removeDuplicateAddresses
* Changes parameters to AUTF8String
** since names and be non-ascii, we must use UTF8 string here.
* Drops the aRemoveAliasesToMe
** this was an old option that was going to have some code imported ages ago, but never happened. Given the way we do things at the moment, if we want to do this, we should be passing in specific aliases via the 'aOtherAddrs' argument. Hence I've dropped it as its a bogus attribute that'll confuse people.
* Drops some unused size calculations in msg_remove_duplicate_addresses
* Reformats some of nsSmtpProtocol to change tabs to spaces
* Provides a unit test

Comment 14

[David :Bienvenu]

Comment on attachment 377140 [details] [diff] [review]
[checked in] Fix removeDuplicateAddresses

while you're here, this is crying out for a static cast:

           nsMsgCompFields* _compFields = (nsMsgCompFields*)compFields.get();  // XXX what is this?

While you're changing the indentation here, might as well use ! instead of == nsnull
+      if (m_addressesLeft == 0 || addrs2 == nsnull)

Comment 15

[Bryan Clark (DevTools PM) [:clarkbw]]

this looks pretty much done.  I just want to make sure it's on the right lists since it's blocking bug 45715 which is set as a TB blocker

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 377140 [details] [diff] [review]
[checked in] Fix removeDuplicateAddresses

I checked this in on the 14th: http://hg.mozilla.org/comm-central/rev/80d58da1901c

Comment 17

[Mark Banner (:standard8)]

(In reply to comment #15)
> this looks pretty much done.  I just want to make sure it's on the right lists
> since it's blocking bug 45715 which is set as a TB blocker

Sorry for not having updated the bug earlier. The vital part for bug 45715 has landed so taking off the blocking list again.

Comment 18

[Mark Banner (:standard8)]

Ok, I'm not working on this at the moment, but I think we can call this fixed to some extent. AFAIK Joshua is working on improving the backend for this anyway, so hopefully some of the rest of this will fall out in the wash anyway.