Closed Bug 414432 Opened 16 years ago Closed 16 years ago

Change nsIMsgHeaderParser::makeFullAddress to use AStrings and provide a string noscript implementation

Categories

(MailNews Core :: MIME, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(1 file, 5 obsolete files)

Attached patch The fix (obsolete) — Splinter Review
I'm adding some new uses of nsIMsgHeaderParser::makeFullAddress(WString) for which I'm having to do .get() and various conversions a lot. It'd be nicer if makeFullAddress just did that for me (and perhaps later we could convert it so it didn't need to do .get()).

The attached patch:

- Changes nsIMsgHeaderParser::makeFullAddress(WString) to use A(UTF8)String
- Tidies up the function headers in nsMsgHeaderParser.cpp
- Tidies up some of the functions in nsMsgHeaderParser.cpp where we have "if ... return ... else ... return"
- Changes the associated code to use the new AString interfaces, optimising where possible.
Attachment #299827 - Flags: superreview?(neil)
Attachment #299827 - Flags: review?(neil)
Attached patch Unit test (obsolete) — Splinter Review
I've just written a few unit tests for makeFullAddress(WString) based around testing parts of RFC2822.
Attachment #299838 - Flags: review?(neil)
Comment on attachment 299827 [details] [diff] [review]
The fix

Given bug 234867 comment 4 I'm not sure this is correct... asking bienvenu for a second opinion.
Attachment #299827 - Flags: superreview?(neil) → superreview?(bienvenu)
I'm not sure either - perhaps the thing to do is find a message that triggers the assertion Jungshik mentioned, and see what happens with that message with your patch, Mark? 
Jungshik any chance you could provide some insight for us on this bug please? (see comments 2 & 3).
Depends on: 415942
Comment on attachment 299838 [details] [diff] [review]
Unit test

Reduced form of patch attached to bug  	415942. Will need to update this one when we determine the way forward.
Attachment #299838 - Attachment is obsolete: true
Attachment #299838 - Flags: review?(neil)
So, given that Jungshik doesn't seem to be around anymore, how do we progress forward with this bug?

Should we rewrite msg_make_full_address and msg_quote_phrase_or_addr (i.e. probably all of nsMsgHeaderParser) to be entirely ns*String based, and hence we'd remove the possibility for the assertion to occur?
Ah, I'd misread bug 234867 comment 6 - in that case we might as well kill the charset parameter on all of those header parser methods.
(In reply to comment #7)
> Ah, I'd misread bug 234867 comment 6 - in that case we might as well kill the
> charset parameter on all of those header parser methods.
> 

I'll probably break this into two patches then.

Looking at this one (and remembering it) we will get two functions:

AUTF8String makeFullAddress(in AUTF8String aName, in AUTF8String aAddress);
AString makeFullAddressWString(in AString aName, in AString aAddress);

makeFullAddressWString is used about 5 times, makeFullAddress is used about 7 times.

Given that makeFullAddressWString effectively calls makeFullAddress behind the scenes what's the thoughts about dropping makeFullAddressWString and just converting on call if necessary for makeFullAddress?
That makes sense particularly as the only C++ caller is due for conversion ;-)
Attached patch The fix v2 (obsolete) — Splinter Review
Revised patch, dropped makeFullAddressWString, only did changes relating to makeFullAddress.

Some of the code in nsMsgCompFields is a bit nasty, but I plan to do a follow up bug/patch that'll make it use an array-based function and hence not have to null check the pointers.
Attachment #299827 - Attachment is obsolete: true
Attachment #323041 - Flags: superreview?(bienvenu)
Attachment #323041 - Flags: review?(neil)
Attachment #299827 - Flags: superreview?(bienvenu)
Attachment #299827 - Flags: review?(neil)
Attached patch The fix v2 (obsolete) — Splinter Review
It helps if I actually think about which patch I'm attaching.
Attachment #323041 - Attachment is obsolete: true
Attachment #323042 - Flags: superreview?(bienvenu)
Attachment #323042 - Flags: review?(neil)
Attachment #323041 - Flags: superreview?(bienvenu)
Attachment #323041 - Flags: review?(neil)
Comment on attachment 323042 [details] [diff] [review]
The fix v2

I'm going to review this patch straight, although looking at it I think we should keep both makeFullAddress (noscript, using char* but losing the charset) and makeFullAddressWString (but switching to AString), and convert some of the C++ consumers (e.g. addrbook, compose) over to makeFullAddressWString.

>+            mParser->MakeFullAddress(NS_ConvertUTF16toUTF8(pDisplayNameStr),
>+                                     NS_ConvertUTF16toUTF8(pDisplayNameStr),
Seems inefficient to convert the same string twice. I'd use a temporary.

>+            pEmailStr[0] = NS_ConvertUTF8toUTF16(fullAddress);
CopyUTF8toUTF16(fullAddress, pEmailStr[0]);

>-        char * pNames = names;
>-        char * pAddresses = addresses;
>+        char* pNames = names;
>+        char* pAddresses = addresses;
Nit: hunk contains only "unrelated" whitespace changes

>+    parser->MakeFullAddress(NS_ConvertUTF16toUTF8(listName),utf8Email,
Nit: , utf8Email

>+  aResult.Adopt(msg_make_full_address(nsCString(aName), nsCString(aAddress)));
Nice job for an intern: hunt down allocator mismatches ;-)
Attachment #323042 - Flags: review?(neil) → review+
Comment on attachment 323042 [details] [diff] [review]
The fix v2

I'm investigating the alternative solution mentioned by Neil.
Attachment #323042 - Flags: superreview?(bienvenu)
Attached patch Alternate method (obsolete) — Splinter Review
Alternative method. This is based on Neil's idea of keeping the const char* version.

This does actually make the changes a lot simpler, although nsAbAutoCompleteSession turned out a little complex. I did the best on that I could, but bear in mind its going away soon :-)

I also decided to rename the scripted version to "MakeFullAddress" and the non-scripted version to "MakeFullAddressCString" (might need a slightly better name for that). I did this because I don't like the ?String extension, but with them this way round we can hide a lot of its uses and javascript users don't have to type the extra bit all the time (when it doesn't actually concern them).

So, take your pick...
Attachment #323063 - Flags: superreview?(bienvenu)
Attachment #323063 - Flags: review?(neil)
Comment on attachment 323063 [details] [diff] [review]
Alternate method

>+    nsString displayName;
>+    if (pDisplayNameStr)
>+      displayName = pDisplayNameStr;
Don't need the test, nsString displayName(pDisplayNameStr) suffices.

>+    nsString email;
Unused?

>+            mParser->MakeFullAddress(pDisplayNameStr, pDisplayNameStr,
>+                                     pEmailStr[0]);  
Hardly seems worth wrapping, espcially if you trim the trailing whitespace ;-)

>+  [noscript] string makeFullAddressCString(in string aName, in string aAddress);
I'd go for makeFullAddressString - CString is too easily confused with ACString. (Where did you think WString came from?)

>+  nsresult rv = MakeFullAddressCString(NS_ConvertUTF16toUTF8(aName).get(),
>+                                       NS_ConvertUTF16toUTF8(aAddress).get(),
>+                                       getter_Copies(utf8Str));
>   if (NS_SUCCEEDED(rv))
Hardly seems worth doing this. Or possibly call msg_make_full_address directly.
Attachment #323063 - Flags: review?(neil) → review+
Updated patch to address Neil's comments.
Attachment #323042 - Attachment is obsolete: true
Attachment #323063 - Attachment is obsolete: true
Attachment #323292 - Flags: superreview?(bienvenu)
Attachment #323292 - Flags: review+
Attachment #323063 - Flags: superreview?(bienvenu)
Comment on attachment 323292 [details] [diff] [review]
Alternate method v2

Just a couple of things I happened to notice:

>+        var address = gHeaderParser.makeFullAddress(card.displayName,
>+						    card.primaryEmail);
Some tabs crept in :-( [I didn't check the whole patch]

>+    nsString displayName(pDisplayNameStr);
>+    if (bIsMailList)
>     {
>+      if (pNotesStr && pNotesStr[0] != 0)
>+        mParser->MakeFullAddress(displayName, nsDependentString(pNotesStr),
>+                                 fullAddress);
>+      else if (pDisplayNameStr)
>+        mParser->MakeFullAddress(displayName, nsDependentString(pDisplayNameStr),
>+                                 fullAddress);
mParser->MakeFullAddress(displayName, displayName, fullAddress);
Comment on attachment 323292 [details] [diff] [review]
Alternate method v2

one nit: there are several places where we do:

+      if (pNotesStr && pNotesStr[0] != 0)

which could be instead

if (pNotesStr && *pNotesStr)
Attachment #323292 - Flags: superreview?(bienvenu) → superreview+
Checked in with comments addresses. For testers, check items relating to autocomplete/sending messages where email address formulation is taking place.
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Summary: Change nsIMsgHeaderParser::makeFullAddress(WString) to use A(UTF8)Strings → Change nsIMsgHeaderParser::makeFullAddress to use AStrings and provide a string noscript implementation
Target Milestone: --- → mozilla1.9
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: