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)
MailNews Core
MIME
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.9
People
(Reporter: standard8, Assigned: standard8)
References
Details
Attachments
(1 file, 5 obsolete files)
21.48 KB,
patch
|
standard8
:
review+
Bienvenu
:
superreview+
|
Details | Diff | 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)
Assignee | ||
Comment 1•16 years ago
|
||
I've just written a few unit tests for makeFullAddress(WString) based around testing parts of RFC2822.
Attachment #299838 -
Flags: review?(neil)
Comment 2•16 years ago
|
||
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)
Comment 3•16 years ago
|
||
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?
Assignee | ||
Comment 4•16 years ago
|
||
Jungshik any chance you could provide some insight for us on this bug please? (see comments 2 & 3).
Assignee | ||
Comment 5•16 years ago
|
||
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)
Assignee | ||
Comment 6•16 years ago
|
||
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?
Comment 7•16 years ago
|
||
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.
Assignee | ||
Comment 8•16 years ago
|
||
(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?
Comment 9•16 years ago
|
||
That makes sense particularly as the only C++ caller is due for conversion ;-)
Assignee | ||
Comment 10•16 years ago
|
||
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)
Assignee | ||
Comment 11•16 years ago
|
||
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 12•16 years ago
|
||
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+
Assignee | ||
Comment 13•16 years ago
|
||
Comment on attachment 323042 [details] [diff] [review] The fix v2 I'm investigating the alternative solution mentioned by Neil.
Attachment #323042 -
Flags: superreview?(bienvenu)
Assignee | ||
Comment 14•16 years ago
|
||
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 15•16 years ago
|
||
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+
Assignee | ||
Comment 16•16 years ago
|
||
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 17•16 years ago
|
||
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 18•16 years ago
|
||
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+
Assignee | ||
Comment 19•16 years ago
|
||
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
Updated•16 years ago
|
Product: Core → MailNews Core
You need to log in
before you can comment on or make changes to this bug.
Description
•