Scott MacGregor Reporter
	Description • 17 years ago

For seamonkey and thunderbird to use libxul, we're going to have to port the mailnews module to use frozen linkages. David and I got a brain dump today from Benjamin describing what we need to do.

	Mark Banner (:standard8)
	Comment 1 • 17 years ago

Given Benjamin's argument that for Mozilla 2 we won't be able to build a binary mailnews (if I understand http://benjamin.smedbergs.us/blog/2006-12-22/improving-xpcom-for-mozilla-2/ correctly), and later comment in a news group about it (mozilla.dev.platform I think); is it really worth us rewriting to use frozen linkages if we're then going to have to rewrite it all in js (if that's even possible)? or if not in js, restructuring it not to use xpcom and to use a js interface between the binaries?

	Prasad Sunkari [:prasad]
	Comment 2 • 17 years ago

It might be easy to do the migration from within the seamonkey (as suggested by Scott in the newsgroups).  But, there are a few situations where changes are required in mailnews/base for the addrbook to compile correctly.

I migrated some code and could compile the addrbook code with MOZILLA_INTERNAL_API turned off.  But, that required changes in a few files outside the addrbook directory.  The files are: msgCore.h, nsMsgRDFUtils.h, nsMsgI18N.h, nsMsgUtils.h, nsProxiedService.h and nsRDFResource.h

I did a dry run of make, then removed all the "-D MOZILLA_INTERNAL_API" from the command lines and used the commands to build the addrbook code.

Benjamin suggested moving nsProxiedService.h to glue code.  I will do it and submit the patch ASAP.

	Mark Banner (:standard8)
	Comment 3 • 17 years ago

(In reply to comment #2)
> I migrated some code and could compile the addrbook code with
> MOZILLA_INTERNAL_API turned off.  But, that required changes in a few files
> outside the addrbook directory.  The files are: msgCore.h, nsMsgRDFUtils.h,
> nsMsgI18N.h, nsMsgUtils.h, nsProxiedService.h and nsRDFResource.h

For the msg files, how big would be the effect of incorporating those changes on the rest of mailnews?

mailnews/base/util is pretty much central to most of mailnews, seeing as thouse files are in msgbaseutils then I think pretty much any "standalone" part of mailnews is going to struggle without those changed.

> Benjamin suggested moving nsProxiedService.h to glue code.  I will do it and
> submit the patch ASAP.

That's good. What about nsRDFResource.h? Are we going to have to reimplement that for mailnews?

	neil@parkwaycc.co.uk
	Comment 4 • 17 years ago

(In reply to comment #3)
>That's good. What about nsRDFResource.h? Are we going to have to reimplement
>that for mailnews?
I believe we're supposed to switch from resource factories to delegates. Unfortunately the relevant bugs have been marked security-sensitive, but the effect from the chrome side is as follows (nsMsgDBFolder used as an example):

1. You can't do RDF.GetResource(folderURI).QueryInterface(Components.interfaces.nsIMsgFolder) instead you have to do RDF.GetResource(folderURI).GetDelegate("msgfolder", Components.interfaces.nsIMsgFolder)

2. You can't do msgFolder.QueryInterface(Components.interfaces.nsIRDFResource) instead you have to create a readonly attribute to return the resource.
Note: currently delegate factories are created using createInstance when getService would be more appropriate.

	Axel Hecht
	Comment 5 • 17 years ago

From a strategic "kick RDF's ass" point of view, you should neither go for implementing resources nor for delegates. Delegates are a leaking pity, and we'll most likely not whack them hard to fix their ownership problems.

The probably sanest thing going forward would be to create your own mapping between resources and mailnews objects. If you want to fix the design problems between RDF and mailnews, design them for RDF possibly going away.

	David :Bienvenu
	Comment 6 • 17 years ago

Thx, Axel - that's kinda been my thinking - that mapping is basically a hash table, anyway. The harder issue is replacing the UI driven by RDF data sources

	Mark Banner (:standard8)
	Comment 7 • 17 years ago

(In reply to comment #5)
> The probably sanest thing going forward would be to create your own mapping
> between resources and mailnews objects. If you want to fix the design problems
> between RDF and mailnews, design them for RDF possibly going away.

(In reply to comment #6)
> Thx, Axel - that's kinda been my thinking - that mapping is basically a hash
> table, anyway. The harder issue is replacing the UI driven by RDF data sources

So given that most of the address book selection items are currently driven by RDF datasources, (e.g. http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/mailnews/addrbook/resources/content/abDirTreeOverlay.xul&rev=1.47&mark=51-57#51), what would be the easiest way of implementing these lists without having to rebuild them manually in the UI each time they change? - or would that no longer be possible?

	Mark Banner (:standard8)
	Comment 8 • 17 years ago

(In reply to comment #6)
> Thx, Axel - that's kinda been my thinking - that mapping is basically a hash
> table, anyway. The harder issue is replacing the UI driven by RDF data sources
> 

One way may be to do this via mozStorage - see bug 321172 which would implement being able to create templates from mozStorage. Could we store temporary (or even permanent - if we wanted to 'drop' pref structures) tables in mozStorage and use those to drive our trees etc?

	Axel Hecht
	Comment 9 • 17 years ago

As Neil points out in bug 321172 comment 5, it's not too likely that we'll get templates on top of storage. Other template implementations, namely, the one on top of XML that is in the works currently doesn't come with live updates of the UI for data changes, too.

Replacing RDF for the template stuff is going to be more than tricky, though you may be able to hook up something into the template processing that is tailored to what mailnews needs and wants. Again, Neil would know better.

	Prasad Sunkari [:prasad]
	Comment 10 • 17 years ago

Scott, can you comment on this patch.  I am sure this would not work.

The last two files in the patch were modified only to get addrbook compiled properly with MOZILLA_INTERNAL_API turned off.

	Mark Banner (:standard8)
	Comment 11 • 17 years ago

(In reply to comment #10)
> Created an attachment (id=265944) [details]
> convert addrbook to Frozen Linking (will not work, for verification only)
> 
> Scott, can you comment on this patch.  I am sure this would not work.

I've taken a few glances (as I'm interested in this code) and on the whole it looks good. Unfortunately you'll need to update your tree as there have been quite a few changes to some of the code that you've patched.

It would be nice to change some of the apis as well (to use A(C)String instead of (w)string, but we could do that later if you don't want to do it now.

	Prasad Sunkari [:prasad]
	Comment 12 • 17 years ago

This is a old patch, did this atleast a couple of months back (before the code cleanup was initiated).  I am writing smaller patches that would do the cleanup and will also update this patch accordingly.

	Magnus Melin [:mkmelin]
	Comment 13 • 17 years ago

Did I understand correctly the + operator won't be available for strings? Sure sounds a bit limiting. Why?

	Mark Banner (:standard8)
	Comment 14 • 17 years ago

Once we get the address book patch on bug 384490 approved, the changes in the patch I'm attaching should be the only remaining code changes to get the address book to compile, link and run with libxul on linux (note at this stage, this is with a very reduced and hacked mailnews build).

	Mark Banner (:standard8)
	Comment 15 • 17 years ago

(In reply to comment #14)
> Once we get the address book patch on bug 384490 approved, the changes in the
> patch I'm attaching should be the only remaining code changes to get the
> address book to compile, link and run with libxul on linux (note at this stage,
> this is with a very reduced and hacked mailnews build).

Something must have been wrong with my build yesterday. Unfortunately, there's more changes than just attachment 294819 [details] [diff] [review] that need doing. However, the patch is still necessary, so no point in cancelling reviews or anything.

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)
	Comment 16 • 17 years ago

nominating for wanted-thunderbird3 to keep it on the radar.

	Magnus Melin [:mkmelin]
	Comment 17 • 16 years ago

Last I heard this isn't gonna be finished for tb3. Patches wanted of course...
Marking wanted‑thunderbird3-

	Mark Banner (:standard8)
	Comment 18 • 16 years ago

For those wanting to work on this and its blocking bugs, there is now a new configure option (created by bug 483577):

--enable-incomplete-external-linkage

This will switch mailnews to attempt to build with external linkage. It will not build currently.

Once you have enabled this and done as much of the build as you can, then you can compile in various directories using make in objdir/mailnews/<component>. If you use --disable-static-mail then you may be able to link once mailnews/base/util is compiling and linking (which it isn't at the moment).

Note that at the moment you'll probably also need --disable-ldap to get to the mailnews build step.

	Jan Horak [:jhorak] Assignee
	Comment 19 • 15 years ago

Working on frozen linkage patch. A lot of missing issues but at least it can be compiled with --enable-incomplete-external-linkage. Still have some linking issues with xpcom and xpcom_core. Feel free to comment non-sense code.

	Jan Horak [:jhorak] Assignee
	Comment 20 • 15 years ago

I've got couple of questing about frozen linkage:
1) How much is restricted using libxpcom_core to link with mailnews/ code?
   - There are missing symbols like:
     NS_NewAtom
     NS_RegisterStaticAtoms
     NS_NewArrayEnumerator(nsISimpleEnumerator**, nsISupportsArray*)
     NS_NewCStringInputStream
     NS_NewAdoptingUTF8StringEnumerator
     NS_NewNotificationCallbacksAggregation
     and some more
2) Is it required/planned to port nsSupportsArray into nsIMutableArray/nsIArray?
3) There is a library conflict problem with nsTArray_base::EnsureCapacity() 
   function. To check if array has not yet been initialized there is used 
   pointer compare of object header to empty header.
      if (mHdr == &sEmptyHdr) 
   However the sEmptyHdr is defined in two libraries libxpcom_core.so and 
   libxpcomglue.a (or libxpcomglue_s.a). Thunderbird is linked against 
   both of them and sometimes is used pointer from libxpcom_core to initialize 
   nsTArray header and pointer from libxpcomglue.a is used to compare with this 
   header. This leads to crash in AppendElements() because instead of 
   malloc new space the EnsureCapacity() is trying to relloc.

	Mark Banner (:standard8)
	Comment 21 • 15 years ago

(In reply to comment #20)
> I've got couple of questing about frozen linkage:

(reordered)

> 2) Is it required/planned to port nsSupportsArray into
> nsIMutableArray/nsIArray?

Yes, ns(I)SupportsArray and its close relatives are obsolete. Bug 394167 is the mailnews tracker, but also see:

https://bugzilla.mozilla.org/buglist.cgi?query_format=advanced;short_desc=nsISupportsArray;short_desc_type=allwordssubstr;resolution=---

> 1) How much is restricted using libxpcom_core to link with mailnews/ code?
>    - There are missing symbols like:
>      NS_NewAtom
>      NS_RegisterStaticAtoms
>      NS_NewArrayEnumerator(nsISimpleEnumerator**, nsISupportsArray*)
>      NS_NewCStringInputStream
>      NS_NewAdoptingUTF8StringEnumerator
>      NS_NewNotificationCallbacksAggregation
>      and some more

Where items aren't obsolete, we either need to: 1) Have gecko define a relevant symbol in the external linkage, 2) translate the function into external linkage compatible code. I thought we'd actually raised a bug for NS_NewAtom (hmm, bug 435242 and/or bug 498599).

Most of these we'll have to take on an individual basis. Neil may have some useful input on these.

> 3) There is a library conflict problem with nsTArray_base::EnsureCapacity() 
...
>    However the sEmptyHdr is defined in two libraries libxpcom_core.so and 
>    libxpcomglue.a (or libxpcomglue_s.a). Thunderbird is linked against 
>    both of them and sometimes is used pointer from libxpcom_core to initialize 
>    nsTArray header and pointer from libxpcomglue.a is used to compare with this 

If we're linking against both of those libs, then iirc this is wrong and probably due to an incomplete --enable-incomplete-external-linkage implementation.


I've not looked at your patch in detail, but for it to work with both internal & external, you'll need to use things like nsStringGlue.h instead of nsStringAPI.h and you may need ifdefs in various places (it'll also be a good idea to split it into smaller segments to make it better on reviewers).

	Jan Horak [:jhorak] Assignee
	Comment 22 • 15 years ago

According to building output the libxul is linked against libxpcom_core. I'm going to check nsStringGlue.h now. I also try to divide patch by mailnews directory modules. Thanks for feedback.

	neil@parkwaycc.co.uk
	Comment 23 • 15 years ago

(In reply to comment #20)
>    - There are missing symbols like:
>      NS_NewAtom
>      NS_RegisterStaticAtoms
>      NS_NewArrayEnumerator(nsISimpleEnumerator**, nsISupportsArray*)
>      NS_NewCStringInputStream
>      NS_NewAdoptingUTF8StringEnumerator
>      NS_NewNotificationCallbacksAggregation
I would have thought it would have been possible to make most of these symbols (or dependent symbols in the case of NS_NewNotificationCallbacksAggregation) available except for NS_NewArrayEnumerator and possibly NS_RegisterStaticAtoms which may be libxul-only.

	Martin Stránský [:stransky] (ni? me)
	Comment 24 • 15 years ago

As for the NS_Atom...functions in glue, what would be a proper name for this frozen function? I expect we don't want to have the same symbol in frozen/unfrozen linkage. Or am I wrong and it's fine?

	Jan Horak [:jhorak] Assignee
	Comment 39 • 15 years ago

I've added required MOZ_INTERNAL_API ifdefs to allow to compile with internal linkage too. I also divided the big patch into smaller portions for easier review. There are still missing functions for frozen linkage like:
 NS_RegisterStaticAtoms
 NS_NewCStringInputStream
 NS_NewAdoptingUTF8StringEnumerator
 NS_NewInterfaceRequestorAggregation
 NS_NewNotificationCallbacksAggregation
 NS_GetProxyForObject
 array.SortIgnoreCase()
Thunderbird is usable with frozen linkage, but there is the issue Martin Stransky brings in bug 536139.

	Robert Kaiser
	Comment 40 • 15 years ago

jhorak: You probably should put specific review requests up for every of the patches, e.g. directed to :standard8 who already worked on a part of this.

	neil@parkwaycc.co.uk
	Comment 41 • 15 years ago

Comment on attachment 418821 [details] [diff] [review]
address book patch

>-#include "nsCRT.h"
>+#include <string.h>
This change doesn't look right. Maybe a leftover?

>+  NS_WARNING(aURI.BeginReading());
Creative use of NS_WARNING but not relevant here, I think ;-)

>-  result.Adopt(PL_Base64Encode(NS_ConvertUTF16toUTF8(xmlStr).get(), 0, nsnull));
>+  result.Assign(PL_Base64Encode(NS_ConvertUTF16toUTF8(xmlStr).get(), 0, nsnull));
I know that the old code was wrong but the new code is wrong too; I believe you have to PR_Free the return value of PL_Base64Encode.

>-      PRInt32 index = column.Find(" ", false, 0, -1);
>+      PRInt32 index = column.Find(" ");
I wonder whether FindChar would make an even better replacement.

	Mark Banner (:standard8)
	Comment 42 • 15 years ago

Comment on attachment 418821 [details] [diff] [review]
address book patch

jhorak, thanks for doing these patches. I'm going to suggest that it would be better to split these across maybe 4-5 bugs rather than try and review them all in this one and have a complex of review comments and responses in this bug.

Could you also spread the reviewers around as well so we can share the load a bit? https://developer.mozilla.org/en/Mailnews_and_Mail_code_review_requirements has some suggestions for alternatives.

Having said that, I've noticed some obvious things, so I'll comment on them now.

>   // Get the directory factory from the URI
>   nsCOMPtr<nsIAbDirFactory> dirFactory;
>+  NS_WARNING(aURI.BeginReading());

This doesn't make any sense as to why you're doing it, and needs an explanation.

>-  result.Adopt(PL_Base64Encode(NS_ConvertUTF16toUTF8(xmlStr).get(), 0, nsnull));
>+  result.Assign(PL_Base64Encode(NS_ConvertUTF16toUTF8(xmlStr).get(), 0, nsnull));

This can't be right. PL_Base64Encode is returning a char* that the caller must free. Adopt takes ownership of the char*, Assign doesn't and hence this will leak memory. I'm not sure why this would need changing for external API anyway.

	Mark Banner (:standard8)
	Comment 43 • 15 years ago

Comment on attachment 418823 [details] [diff] [review]
base/search patch

>     nsCOMPtr<nsISupportsArray> actionList;
>-    rv = NS_NewISupportsArray(getter_AddRefs(actionList));
>+    actionList = do_CreateInstance(NS_SUPPORTSARRAY_CONTRACTID, &rv);
>     NS_ENSURE_SUCCESS(rv, rv);

The problem with doing these is that you still end up with one of the supports array functions being inaccessible to external API. So you're just changing these but you're still not going to be able to link most of it. I'm sure Neil's said this before, and I'll say it as well that it would be better to fix bug 394167 and remove nsISupportsArray usage from mailnews than just replace these and fix them again when we fix linking for external linkage.

	neil@parkwaycc.co.uk
	Comment 44 • 15 years ago

Comment on attachment 418829 [details] [diff] [review]
extensions patch

>+    if (wordLength < 40 && strchr(aWord, '.') &&  numAtSign == 1)
Nit: double space crept in here. (And perhaps the logic needs a refresh.)

>-          tokenStrings.AppendElement(UTF8ToNewUnicode(nsDependentCString(
>-              tokens[ta.mTokenIndex].mWord)));
>+          NS_ConvertUTF8toUTF16 converter(tokens[ta.mTokenIndex].mWord);
>+          tokenStrings.AppendElement(ToNewUnicode(converter));
I think UTF8ToNewUnicode(nsDependentCString(...)); could be rewritten as ToNewUnicode(NS_ConvertUTF8toUTF16(...));

>+#ifdef MOZILLA_INTERNAL_API
>                   if (!mailCC.IsEmpty() && !identEmail.IsEmpty() && mailCC.Find(identEmail, PR_TRUE) != -1)
>+#else
>+                  if (!mailCC.IsEmpty() && !identEmail.IsEmpty() && mailCC.Find(identEmail, CaseInsensitiveCompare) != -1)
>+#endif
I'd be tempted to write this ;-)
#ifdef MOZILLA_INTERNAL_API
#define CaseInsensitiveCompare PR_TRUE
#endif

>         nsDependentCString email(walk);
>-        *iEA = ToNewUnicode(email);
>+        NS_ConvertUTF8toUTF16 email_utf16 = NS_ConvertUTF8toUTF16(email);
>+        *iEA = NS_strdup(email_utf16.get());
*iEA = ToNewUnicode(NS_ConvertUTF8toUTF16(walk)); perhaps?

	neil@parkwaycc.co.uk
	Comment 45 • 15 years ago

(In reply to comment #43)
>(From update of attachment 418823 [details] [diff] [review])
>>     nsCOMPtr<nsISupportsArray> actionList;
>>-    rv = NS_NewISupportsArray(getter_AddRefs(actionList));
>>+    actionList = do_CreateInstance(NS_SUPPORTSARRAY_CONTRACTID, &rv);
>>     NS_ENSURE_SUCCESS(rv, rv);
>The problem with doing these is that you still end up with one of the supports
>array functions being inaccessible to external API.
Didn't he convert do_QueryElementAt to the ->QueryElementAt longhand?

	Mark Banner (:standard8)
	Comment 46 • 15 years ago

(In reply to comment #45)
> (In reply to comment #43)
> >(From update of attachment 418823 [details] [diff] [review])
> >>     nsCOMPtr<nsISupportsArray> actionList;
> >>-    rv = NS_NewISupportsArray(getter_AddRefs(actionList));
> >>+    actionList = do_CreateInstance(NS_SUPPORTSARRAY_CONTRACTID, &rv);
> >>     NS_ENSURE_SUCCESS(rv, rv);
> >The problem with doing these is that you still end up with one of the supports
> >array functions being inaccessible to external API.
> Didn't he convert do_QueryElementAt to the ->QueryElementAt longhand?

Yes, but I'm sure you've r- one of my patches because of that conversion or something similar. Don't we loose some type safety or something?

	neil@parkwaycc.co.uk
	Comment 47 • 15 years ago

Comment on attachment 418834 [details] [diff] [review]
mime patch

>-#include "nsStringEnumerator.h"
>+//#include "nsStringEnumerator.h"
Oops.

>-  aResult.Adopt(msg_remove_duplicate_addresses(aAddrs, aOtherAddrs));
>+  aResult.Assign(msg_remove_duplicate_addresses(aAddrs, aOtherAddrs));
Ah, I see the problem here: external API only provides Adopt on nsCString, not nsACString. You'll need to use a temporary nsCString. Bleh.

	neil@parkwaycc.co.uk
	Comment 48 • 15 years ago

Comment on attachment 418832 [details] [diff] [review]
local patch

>   //Now we have a valid directory or we have returned.
>   //Make sure the new folder name is valid
>   nsAutoString safeFolderName(folderName);
>   NS_MsgHashIfNecessary(safeFolderName);
>   nsCAutoString nativeFolderName;
>-  rv = NS_CopyUnicodeToNative(safeFolderName, nativeFolderName);
>-  if (NS_FAILED(rv) || nativeFolderName.IsEmpty()) {
>+  LossyCopyUTF16toASCII(safeFolderName, nativeFolderName);
>+  if (nativeFolderName.IsEmpty()) {
>     ThrowAlertMsg("folderCreationFailed", msgWindow);
>     // I'm returning this value so the dialog stays up
>     return NS_MSG_FOLDER_EXISTS;
>   }
> 
>   path->AppendNative(nativeFolderName);
I don't see how this will work. Hopefully we can just use Append which takes a Unicode name and thus we don't have to worry about the native conversion (which is almost always UTF8/16 anyway).

	neil@parkwaycc.co.uk
	Comment 49 • 15 years ago

Comment on attachment 418835 [details] [diff] [review]
news patch

>   PRInt32 code;
>+  nsresult res;
>+#ifdef MOZILLA_INTERNAL_API
>   PRInt32 number = key.ToInteger(&code);
>   if (code != NS_OK)
>+#else
>+  PRInt32 number = key.ToInteger(&res);
>+  if (res != NS_OK)
>+#endif
All the other cases have this right, with the code/res inside the #ifdef.

>+#ifdef MOZILLA_INTERNAL_API
>     nsCAutoString value;
>     header.Right(value, header.Length() - colon -1);
>+#else
>+    nsDependentCSubstring value = StringTail(header, header.Length() - colon -1);    
>+#endif
This should be an nsCString. Only create temporary nsDependent* objects.
StringTail works with internal API too, doesn't it?
It might be worth converting this to Substring anyway.

>-  return hdr->GetMessageId(getter_Copies(result));
>+  char *res_str;
>+  rv = hdr->GetMessageId(&res_str);
>+  result.Assign(res_str); //TODO check if res_str is Adopted by result
It needs to be, so you'll have to use a temporary again...

>       {
>-        nsCAutoString currentGroup;
>+
> 
Might as well eliminate these blank lines.

>+        nsDependentCSubstring currentGroup = StringTail(theRest, theRest.Length() - currentHost.Length() - 1);
nsCString again.

>-      aURIString.FindChar('?') == kNotFound ? aURIString += "?" : aURIString += "&";
>+      aURIString.FindChar('?') == -1 ? aURIString += "?" : aURIString += "&";
[could be written aURIString += aURIString.FindChar('?') == -1 ? '?' : '&';]

	neil@parkwaycc.co.uk
	Comment 50 • 15 years ago

Comment on attachment 418825 [details] [diff] [review]
base/util patch

>+/* From nsDebugImp.cpp: */
>+NS_COM nsresult
>+NS_ErrorAccordingToNSPR()
[Where is this used?]

> static PRBool ConvertibleToNative(const nsAutoString& str)
> {
>     nsCAutoString native;
>     nsAutoString roundTripped;
>-    NS_CopyUnicodeToNative(str, native);
>-    NS_CopyNativeToUnicode(native, roundTripped);
>+    LossyCopyUTF16toASCII(str, native);
>+    CopyASCIItoUTF16(native, roundTripped);
>     return str.Equals(roundTripped);
I'm not convinced that this change is useful.

>@@ -1636,16 +1678,259 @@ NS_MSG_BASE nsresult MsgEscapeURL(const 
> {
>   nsresult rv;
>   nsCOMPtr<nsINetUtil> nu = do_GetService(NS_NETUTIL_CONTRACTID, &rv);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   return nu->EscapeURL(aStr, aFlags, aResult);
> }
> 
>+#define UNHEX(C) \
>+    ((C >= '0' && C <= '9') ? C - '0' : \
>+     ((C >= 'A' && C <= 'F') ? C - 'A' + 10 : \
>+     ((C >= 'a' && C <= 'f') ? C - 'a' + 10 : 0)))
>+
>+
>+#define IS_OK(C) (netCharType[((unsigned int) (C))] & (flags))
>+#define HEX_ESCAPE '%'
>+#define ISHEX(c) memchr(hexChars, c, sizeof(hexChars)-1)
>+
>+NS_COM PRBool MsgUnescapeURL(const char *str, PRInt32 len, PRUint32 flags, nsACString &result)
Can we not use nsINetUtil for unescaping too?

>+NS_MSG_BASE void MsgReplaceChar(nsACString& str, const char *set, const char replacement)
No obvious callers.

>+NS_MSG_BASE void MsgReplaceChar(nsAString& str, const PRUnichar *set, const PRUnichar replacement)
I could only find one caller, so it might be worth inlining this.

>+NS_MSG_BASE void MsgReplaceChar(nsACString& str, const char needle, const char replacement)
>+{
>+  char *pos;
>+  char *c_str = str.BeginWriting();
>+  if (!c_str)
>+    return;
>+  while ((pos = strchr(c_str, needle))) {
Nit: technically nsACString isn't required to be terminated on input, although as it happens writable nsACStrings are probably terminated, but maybe this will compile with ns(C)String&, which are required to be terminated, instead?

>+NS_MSG_BASE void MsgSetCharAt(nsAString& str, const PRUnichar ch, PRUint32 pos)
Can you not write str.Replace(pos, 1, ch)?

>+  if (first_pos == last_pos)
>+    return -1;
>+  if (offset > 0 && last_pos - offset > first_pos)
It looks as if this can be simplified as follows:
if (last_pos - first_pos <= offset)
  return -1;

>+  while ((last_pos--) > first_pos) {
Maybe use (--last_pos >= first_pos) ?

>+  atomService->GetAtom(NS_ConvertUTF8toUTF16(nsCString(aString)), &atom);
Why not call GetAtomUTF8?

>+        PRInt32 i = str.Find(nsDependentString(what), i);
>+        if (i == -1)
>+          break;
>+
>+        str.Replace(i, NS_strlen(replacement), replacement);
Shouldn't this be NS_strlen(what)?

>+        i += NS_strlen(replacement);
[nsDependentString and NS_strlen are constants that could be lifted?]

	neil@parkwaycc.co.uk
	Comment 51 • 15 years ago

Comment on attachment 418827 [details] [diff] [review]
compose patch

>-      uri += (uri.FindChar('?') == kNotFound) ? "?" : "&";
>+      uri += (uri.FindChar('?') == -1) ? "?" : "&";
[This would look nicer with '?' : '&']

>   else
>-    CopyASCIItoUTF16(readBuf, sigData);
>+      CopyASCIItoUTF16(nsDependentCString(readBuf), sigData);
Nit: incorrect reindentation.

>+#include <ctype.h>
[This gets used?]

>-        msgUri.SetCharAt('?', typeIndex);
>+        msgUri.BeginWriting()[typeIndex] = '?';
bsmedberg on irc suggested that we should get some extra API into nsStringAPI.h to avoid having to rewrite some of this basic stuff.

>+#ifdef MOZILLA_INTERNAL_API
>           PRInt32 endPos = listPost.Find(">", startPos);
>+#else
>+          PRInt32 endPos = listPost.Find(NS_LITERAL_STRING(">"), startPos, CaseInsensitiveCompare);
>+#endif
Can we not use FindChar here? Also why a case-insensitive compare for ">" ?

>-      char *target_charset = ToNewCString(aCharset);
>-      PRBool formatflowed = UseFormatFlowed(target_charset);
>+      nsCAutoString target_charset;
>+      LossyCopyUTF16toASCII(aCharset, target_charset);
>+      PRBool formatflowed = UseFormatFlowed(target_charset.get());
Would NS_LossyConvertUTF16toASCII(aCharset).get() help?

>+    sigData.BeginReading(&start, &end);
Unused?

>+#ifdef MOZILLA_INTERNAL_API
>         sigData.Find("\r-- \r", PR_TRUE) < 0 &&
>         sigData.Find("\n-- \n", PR_TRUE) < 0 &&
>         sigData.Find("\n-- \r", PR_TRUE) < 0)
>+#else
>+        sigData.Find(NS_LITERAL_STRING("\r-- \r"), CaseInsensitiveCompare) < 0 &&
>+        sigData.Find(NS_LITERAL_STRING("\n-- \n"), CaseInsensitiveCompare) < 0 &&
>+        sigData.Find(NS_LITERAL_STRING("\n-- \r"), CaseInsensitiveCompare) < 0)
>+#endif
Oh dear. I think we can make these case sensitive too!

>-        const nsPromiseFlatString& tString = PromiseFlatString(selPlain);
This is wrong because selPlain is already a flat nsAutoString.
>+        const nsString& tString = selPlain;
So this just... clouds the issue ;-)

>-    uriToOpen += (uriToOpen.FindChar('?') == kNotFound) ? "?" : "&";
>+    uriToOpen += (uriToOpen.FindChar('?') == -1) ? "?" : "&";
[This would look nicer with '?' : '&']

>+#ifdef MOZILLA_INTERNAL_API
>   if (fileUrl || nsDependentCString(aMsgURI).Find("&type=application/x-message-display") >= 0)
>+#else  
>+  if (fileUrl || aMsgURI.Find("&type=application/x-message-display") >= 0)
>+#endif     
Does removing the nsDependentCString not work with internal API too? (Also there are trailing spaces on the preprocessor statements.)

> nsresult
> nsMsgBuildErrorMessageByID(PRInt32 msgID, nsString& retval, nsString* param0, nsString* param1)
Ideally this method would be completely rewritten based on FormatStringFromID. Fortunately there are only two pairs of affected strings.


>     rv = image->GetName(tName);
>     NS_ENSURE_SUCCESS(rv, rv);
>-    attachment->real_name = ToNewCString(tName); // XXX i18n
>-
>-    image->GetLongDesc(tDesc);
>-    NS_ENSURE_SUCCESS(rv, rv);
>-    attachment->description = ToNewCString(tDesc); // XXX i18n
>+    nsCAutoString tName_cstr;
>+    LossyCopyUTF16toASCII(tName, tName_cstr);
>+    attachment->real_name = ToNewCString(tName_cstr); // XXX i18n
I'd go for ToNewCString(NS_LossyConvertUTF16toASCII(tName)) [or tDesc].

>-    attachment->real_name = ToNewCString(tName);
>+    attachment->real_name = ToNewCString(NS_LossyConvertUTF16toASCII(tName));
Third time lucky!

>+    nsMemory::Free(bodyText);    //Don't need it anymore
NS_Free is less typing ;-)

>+      nsCAutoString headerName("header.");
Right.

>+  nsCAutoString headers_match = nsCAutoString("\r\n");
Wrong :-(

>       mCopyFile->GetNativePath(cPath);
>-      NS_CopyNativeToUnicode(cPath, path);
>+      CopyASCIItoUTF16(cPath, path);
This should be mCopyFile->GetPath(path);

>+                  aResult.Append(NS_LITERAL_CSTRING("]"));
Could use .AppendLiteral("]") or .Append(']')

>-    if (m_responseText.IsEmpty() || m_responseText.Last() != '\n')
>+    if (m_responseText.IsEmpty() || *(m_responseText.EndReading()-1) != '\n')
Another one that we should try to get into nsStringAPI.h

>+    searchPart = StringTail(escapedPath,
>                                                escapedPath.Length() -
>                                                startOfSearchPart);
Nit: need to fix the indentation of the continuation lines.

	neil@parkwaycc.co.uk
	Comment 52 • 15 years ago

Comment on attachment 418831 [details] [diff] [review]
import patch

This patch (unsurprisingly) looks incomplete.

	neil@parkwaycc.co.uk
	Comment 53 • 15 years ago

Comment on attachment 418830 [details] [diff] [review]
imap patch

>-    tempFolderName.Left(tokenStr,slashPos);
>-    tempFolderName.Right(remStr, tempFolderName.Length()-slashPos);
>+    tokenStr = StringHead(tempFolderName, slashPos);
>+    remStr = StringTail(tempFolderName, tempFolderName.Length()-slashPos);
Substring might be a better idea than StringTail.

>-      tmpNewName.Left(parentName, folderStart);
>+      nsDependentCSubstring parentName = StringHead(tmpNewName, folderStart);
tmpNewName gets used later.

>-    res = m_stringBundle->GetStringFromID(aMsgId, getter_Copies(aString));
>+    //res = m_stringBundle->GetStringFromID(aMsgId, getter_Copies(aString));
???

>-  nsIThread *thread = NS_GetCurrentThread();
>+  nsIThread *thread;
>+  rv = NS_GetCurrentThread(&thread);
>+  NS_ENSURE_SUCCESS(rv, rv);
This is incorrect. Threads are refcounted, and the one-arg form of NS_GetCurrentThread increments the refcount. Instead of writing this, use
nsCOMPtr<nsIThread> thread(do_GetCurrentThread());

>+    PRUnichar* result;
>     rv = m_stringBundle->FormatStringFromID(aID,
>                                 formatStrings, 1,
>-                                getter_Copies(aResult));
>+                                &result);
>+    aResult.Assign(result); // TODO, does 'aResult' own 'result' string??
It needs to, so you'll have to use nsString result and getter_Copies.
Sadly the assignment then generates a copy. External string suckage :-(

>-      index = uri.Find("/", PR_FALSE, index+2); // find '/' after scheme
>+      index = uri.Find("/", index+2);           // find '/' after scheme
This doesn't do what you think; when using the internal API it does a case insensitive find.

>-    NS_CopyUnicodeToNative(currentFolderDBNameStr, leafName);
>+    LossyCopyUTF16toASCII(currentFolderDBNameStr, leafName);
>     nsCOMPtr <nsILocalFile> msfFilePath = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID, &rv);
>     NS_ENSURE_SUCCESS(rv, rv);
>     msfFilePath->InitWithFile(currentFolderPath);
>     if (NS_SUCCEEDED(rv) && msfFilePath)
>     {
>       // leaf name is the db name w/o .msf (nsShouldIgnoreFile strips it off)
>       // so this trims the .msf off the file spec.
>       msfFilePath->SetNativeLeafName(leafName);
At a guess I think this probably needs to use msfFilePath->SetLeafName(currentFolderDBNameStr);


>+    nsDependentSubstring subLeafName = StringTail(parentName, leafName.Length() - folderStart - 1);
>+    leafName.Assign(subLeafName);
This is wrong. Just write leafName = Substring(parentName, folderStart + 1);

> NS_IMETHODIMP nsImapMailFolder::GetOnlineDelimiter(char** onlineDelimiter)
> {
>   NS_ENSURE_ARG_POINTER(onlineDelimiter);
>   char delimiter = 0;
>   GetHierarchyDelimiter(&delimiter);
>-  nsAutoString str(delimiter);
>+  nsCAutoString str;
>+  str.Assign(delimiter);
>   *onlineDelimiter = ToNewCString(str);
>   return NS_OK;
[I can't believe there isn't an easier way than all that work.]

>-      m_runningUrl->AllocateServerPath(nsPromiseFlatCString(mb->GetMailboxName()).get(), mb->GetDelimiter(), &onlineName);
>+      m_runningUrl->AllocateServerPath(mb->GetMailboxName().BeginReading(), mb->GetDelimiter(), &onlineName);
This should have been PromiseFlatCString (without the ns) in the first place.

>+    nsDependentCSubstring tempProtocolString = StringHead(protocolString, crlfIndex + 2);
>+    rv = SendData(tempProtocolString.BeginReading());
This needs to be an nsCString and a .get() since .BeginReading() doesn't guarantee null-termination.

>   nsCAutoString quotacommand;
>-  quotacommand = nsDependentCString(GetServerCommandTag())
>-               + NS_LITERAL_CSTRING(" getquotaroot \"")
>-               + escapedName
>-               + NS_LITERAL_CSTRING("\"" CRLF);
>+  quotacommand = nsDependentCString(GetServerCommandTag());
nsCAutoString quotacommand(GetServerCommandTag());

>+#include <ctype.h>
...
>+#include "ctype.h"

>-    *((char **)getter_Copies(escapedUsername)) = nsEscape(username.get(), url_XAlphas);
>+    *((char **)getter_Copies(escapedUsername)) = MsgEscape(username.get(), url_XAlphas);
Wow, we do have some awful code, don't we. This should just use Adopt.

>-    msgurl->SetUri(nsDependentCString(aImapURI).get());
>+    msgurl->SetUri(aImapURI.BeginReading());
Both wrong. Should use PromiseFlatCString.

>-      CopyUTF16toMUTF7(nsDependentString(newLeafName), utfNewName);
>+      CopyUTF16toMUTF7(nsDependentString(newLeafName.BeginReading(), newLeafName.Length()),
>+          utfNewName);
CopyUTF16toMUTF7(newLeafName, utfNewName);

>-    //nsCAutoString unescapedSpec(aSpec);
>-    // nsUnescape(unescapedSpec.BeginWriting());
>+    nsCAutoString unescapedSpec(aSpec);
>+    MsgUnescape(unescapedSpec.BeginWriting());
???

	neil@parkwaycc.co.uk
	Comment 54 • 15 years ago

Comment on attachment 418824 [details] [diff] [review]
base/src patch

>-    NS_CopyUnicodeToNative(nsDependentString(aToolTipString),
>+    LossyCopyUTF16toASCII(nsDependentString(aToolTipString),
[Obsolete unused code path.]

>-             nsIThread *thread = NS_GetCurrentThread();
>-
>+             nsIThread *thread;
>+             NS_GetCurrentThread(&thread);
nsCOMPtr<nsIThread> thread(do_GetCurrentThread());

>+      nextKeyword = StringHead(keywords, endOfKeyword);
>       nextKeyword.Insert("kw-", 0);
[This would be better written as an Assign of the kw- and an Append.]

>-        aValue.Adopt(GetString(NS_LITERAL_STRING("messageHasFlag").get()));
>+        aValue.Assign(GetString(NS_LITERAL_STRING("messageHasFlag").get()));
These do need to be adopted.

>+      if (!(type.Equals("nntp", CaseInsensitiveCompare)
>+          || type.Equals("rss", CaseInsensitiveCompare)))
|| operator belongs at the end of the previous line.

>-  buffer.SetCapacity(512);
>+  buffer.SetLength(512);
Sorry, but SetLength and SetCapacity are different things. I guess you'll just have to #ifdef the SetCapacity away.

	neil@parkwaycc.co.uk
	Comment 55 • 15 years ago

Comment on attachment 418824 [details] [diff] [review]
base/src patch

>+  nsCOMPtr<nsIMsgIdentity> identity;
>+  rv = m_identities->QueryElementAt(0, NS_GET_IID(nsIMsgIdentity), getter_AddRefs(identity));
>   identity.swap(*aDefaultIdentity);
>   return rv;
This can actually be written as
return identities->QueryElementAt(0, NS_GET_IID(nsIMsgIdentity), aDefaultIdentity);

	neil@parkwaycc.co.uk
	Comment 56 • 15 years ago

Comment on attachment 418827 [details] [diff] [review]
compose patch

>-              char * cstr = ToNewCString(undisclosedRecipients);
>+              char * cstr = ToNewCString(NS_ConvertUTF16toUTF8(undisclosedRecipients));
>               if (cstr) {
>                 PUSH_STRING("To: ");
>                 PUSH_STRING(cstr);
>                 PUSH_STRING(":;");
>                 PUSH_NEWLINE ();
>               }
>               PR_Free(cstr);
This would be better rewritten as
PUSH_STRING("To: ");
PUSH_STRING(NS_ConvertUTF16toUTF8(undisclosedRecipients).get());
PUSH_STRING(":;");
PUSH_NEWLINE ();

	neil@parkwaycc.co.uk
	Comment 57 • 15 years ago

(In reply to comment #56)
>>-              char * cstr = ToNewCString(undisclosedRecipients);
>PUSH_STRING(NS_ConvertUTF16toUTF8(undisclosedRecipients).get());
Although thinking about it this only supports ISO-8859-1 strings so maybe NS_LossyConvertUTF16toASCII would be a better choice.

	neil@parkwaycc.co.uk
	Comment 58 • 15 years ago

Comment on attachment 418830 [details] [diff] [review]
imap patch

>-  LossyCopyUTF16toASCII(pHost, m_logonHost);
>+  m_logonHost = NS_LossyConvertUTF16toASCII(pHost);
nsDependentString(pHost) also works.

	neil@parkwaycc.co.uk
	Comment 59 • 15 years ago

Comment on attachment 418830 [details] [diff] [review]
imap patch

>+      nsCAutoString printingOperationStr;
>+      if (mPrintingOperation) {
>+        printingOperationStr.Assign(NS_LITERAL_CSTRING("print"));
>+      } else {
>+        printingOperationStr.Assign(EmptyCString());
You don't have to assign an empty string because it's already empty.

	neil@parkwaycc.co.uk
	Comment 60 • 15 years ago

(In reply to comment #53)
>(From update of attachment 418830 [details] [diff] [review])
>>-      tmpNewName.Left(parentName, folderStart);
>>+      nsDependentCSubstring parentName = StringHead(tmpNewName, folderStart);
>tmpNewName gets used later.
Silly me, Left doesn't change its argument, so that's not a problem (but using nsDependentCSubstring like that is still wrong.)

	Mark Banner (:standard8)
	Comment 61 • 15 years ago

Comment on attachment 418824 [details] [diff] [review]
base/src patch

I think Neil's commented on most of these. I would therefore recommend you address them and re-request review from him as you go and we'll take it from there. Hence cancelling requests for now while you fix them up.

	Jan Horak [:jhorak] Assignee
	Comment 62 • 15 years ago

Thanks for comments, sending hopefully better version.

	Jan Horak [:jhorak] Assignee
	Comment 63 • 15 years ago

Please check Adopt() usage. It's now the right approach?

	Jan Horak [:jhorak] Assignee
	Comment 64 • 15 years ago

Thanks for comments, adding newer version.

(In reply to comment #41)
> (From update of attachment 418821 [details] [diff] [review])
> >+  NS_WARNING(aURI.BeginReading());
> Creative use of NS_WARNING but not relevant here, I think ;-)
Sorry, debug line.

> >-  result.Adopt(PL_Base64Encode(NS_ConvertUTF16toUTF8(xmlStr).get(), 0, nsnull));
> >+  result.Assign(PL_Base64Encode(NS_ConvertUTF16toUTF8(xmlStr).get(), 0, nsnull));
> I know that the old code was wrong but the new code is wrong too; I believe you
> have to PR_Free the return value of PL_Base64Encode.
Using Adopt isn't wrong I belive. IIRC the 'result' should grab memory allocated by PL_Base64Encode, shouldn't it?

	Jan Horak [:jhorak] Assignee
	Comment 66 • 15 years ago

Fixed version according to your comments.

	Matthew Gertner
	Comment 67 • 15 years ago

Jan,

Some time ago I ported parts of mailnews to use external linkage for a client project. I posted some roughish patches to bug 395701 at the time. I'd like to get this code onto a standard code base, and I was planning to finish the task of getting my code cleaned up, reviewed and committed. It's great to see that you've been working on this.

It looks like patches you've flagged for review are dependent on your changes to base/util, but that patch hasn't been flagged for review (nor have base/src and import, which I also use). Is there some reason for this? Are you still planning to make changes to these modules before asking for review?

If there's anything I can do to help, please let me know.

	neil@parkwaycc.co.uk
	Comment 68 • 15 years ago

Comment on attachment 420041 [details] [diff] [review]
extensions patch v. 2

>     nsDependentCString word (aWord, wordLength); // CHEAP, no allocation occurs here...
So, it turns out that this variable is only created in order to call CountChar, so under the external API you should avoid it. Perhaps something like this:
const char *atSign = strchr(aWord, '@');
if (wordLength < 40 && strchr(aWord, '.') && atSign && !strchr(atSign + 1, '@'))

	neil@parkwaycc.co.uk
	Comment 69 • 15 years ago

[But with better formatting than Bugzilla default wrapping!]

	neil@parkwaycc.co.uk
	Comment 70 • 15 years ago

(In reply to comment #64)
>(In reply to comment #41)
>>(From update of attachment 418821 [details] [diff] [review])
>>>-  result.Adopt(PL_Base64Encode(NS_ConvertUTF16toUTF8(xmlStr).get(), 0, nsnull));
>>>+  result.Assign(PL_Base64Encode(NS_ConvertUTF16toUTF8(xmlStr).get(), 0, nsnull));
>>I know that the old code was wrong but the new code is wrong too; I believe you
>>have to PR_Free the return value of PL_Base64Encode.
>Using Adopt isn't wrong I belive. IIRC the 'result' should grab memory
>allocated by PL_Base64Encode, shouldn't it?
My nit was that PL_Base64Encode uses PR_MALLOC but Adopt eventually uses NS_Free. We don't guarantee that this will work, although it does for now.

	Jan Horak [:jhorak] Assignee
	Comment 71 • 15 years ago

> >Using Adopt isn't wrong I belive. IIRC the 'result' should grab memory
> >allocated by PL_Base64Encode, shouldn't it?
> My nit was that PL_Base64Encode uses PR_MALLOC but Adopt eventually uses
> NS_Free. We don't guarantee that this will work, although it does for now.
Ah, I see. Sending fixed version.

	Jan Horak [:jhorak] Assignee
	Comment 72 • 15 years ago

Thanks for comments, sending newer version
>>+#ifdef MOZILLA_INTERNAL_API
>>   if (fileUrl || nsDependentCString(aMsgURI).Find("&type=application/x-message-display") >= 0)
>>+#else  
>>+  if (fileUrl || aMsgURI.Find("&type=application/x-message-display") >= 0)
>>+#endif     
> Does removing the nsDependentCString not work with internal API too? (Also
> there are trailing spaces on the preprocessor statements.)
Unfortunately no, gcc ends with error: ‘const class nsACString_internal’ has no member named ‘Find’

>> nsresult
>> nsMsgBuildErrorMessageByID(PRInt32 msgID, nsString& retval, nsString* param0, nsString* param1)
> Ideally this method would be completely rewritten based on FormatStringFromID.
> Fortunately there are only two pairs of affected strings.
Sorry, I'm not sure how to solve this. Where can I get stringID for FormatStringFromID? Is it the same as for msgID for nsMsgBuildErrorMessageByID?

	Jan Horak [:jhorak] Assignee
	Comment 73 • 15 years ago

(In reply to comment #50)
> (From update of attachment 418825 [details] [diff] [review])
> >+/* From nsDebugImp.cpp: */
> >+NS_COM nsresult
> >+NS_ErrorAccordingToNSPR()
> [Where is this used?]

Just in nsMsgFileStream class's methods, like Seek, Tell, Available, etc.

> >@@ -1636,16 +1678,259 @@ NS_MSG_BASE nsresult MsgEscapeURL(const 
> > {
> >   nsresult rv;
> >   nsCOMPtr<nsINetUtil> nu = do_GetService(NS_NETUTIL_CONTRACTID, &rv);
> >   NS_ENSURE_SUCCESS(rv, rv);
> > 
> >   return nu->EscapeURL(aStr, aFlags, aResult);
> > }
> > 
> >+#define UNHEX(C) \
> >+    ((C >= '0' && C <= '9') ? C - '0' : \
> >+     ((C >= 'A' && C <= 'F') ? C - 'A' + 10 : \
> >+     ((C >= 'a' && C <= 'f') ? C - 'a' + 10 : 0)))
> >+
> >+
> >+#define IS_OK(C) (netCharType[((unsigned int) (C))] & (flags))
> >+#define HEX_ESCAPE '%'
> >+#define ISHEX(c) memchr(hexChars, c, sizeof(hexChars)-1)
> >+
> >+NS_COM PRBool MsgUnescapeURL(const char *str, PRInt32 len, PRUint32 flags, nsACString &result)
> Can we not use nsINetUtil for unescaping too?

We can, if UnescapeString is doing the same job.

	Matthew Gertner
	Comment 74 • 15 years ago

(In reply to comment #73)
> We can, if UnescapeString is doing the same job.

I used nsINetUtil::UnescapeString in my version and it seems to work fine.

	neil@parkwaycc.co.uk
	Comment 75 • 15 years ago

Comment on attachment 420042 [details] [diff] [review]
mime patch v 2

>+  nsCString res;
>+  res.Adopt(msg_remove_duplicate_addresses(aAddrs, aOtherAddrs));
>+  aResult.Assign(res);
Sadly it's the best we can do without rewriting msg_remove_duplicate_addresses

	neil@parkwaycc.co.uk
	Comment 76 • 15 years ago

Comment on attachment 420329 [details] [diff] [review]
news patch v2

>+#ifdef MOZILLA_INTERNAL_API
>+  PRInt32 res;
>+#else
>+  nsresult res;
>+#endif
[I filed a bug on allowing us to use nsresult res throughout.]

>+  PRInt32 number = key.ToInteger(&res);
>+  if (res != NS_OK)
>     return NS_ERROR_FAILURE;
[... which would also allow us to use NS_ENSURE_SUCCESS(res, res);]

>+    nsCString value;
>+    value = StringTail(header, header.Length() - colon -1);
Can you write this as nsCString value = Substring(header, colon + 1); ?
[I think you can use Substring like this in both internal and external API.]
[Some of the other uses of Right would be better converted to Substring too.]

>+  char *res_str;
>+  rv = hdr->GetMessageId(&res_str);
>+  nsCString res;
>+  res.Adopt(res_str);
This is incorrect (potentially using res_str when GetMessageId fails), but you can write this as rv = hdr->GetMessageId(getter_Copies(res)); anyway.
[I'm looking into whether it is possible to keep getter_Copies(result).]

>+#ifdef MOZILLA_INTERNAL_API
>     mSubscribeSearchResult.SortIgnoreCase();
>-
>+#else
>+    NS_WARNING("array.SortIgnoreCase() not implemented in frozen linkage");
>+    // FIXME SORT ARRAY mSubscribeSearchResult.SortIgnoreCase();
>+#endif
Unless someone can convince me otherwise I think we should leave this as a compile error for now.

	neil@parkwaycc.co.uk
	Comment 77 • 15 years ago

Comment on attachment 420328 [details] [diff] [review]
local patch v2

>   nsCAutoString newDiskName;
>-  if (NS_FAILED(NS_CopyUnicodeToNative(safeName, newDiskName)))
>-    return NS_ERROR_FAILURE;
>-
>+  LossyCopyUTF16toASCII(safeName, newDiskName);
This will need to be changed to use the Unicode nsILocalFile API too.

	neil@parkwaycc.co.uk
	Comment 78 • 15 years ago

Comment on attachment 420548 [details] [diff] [review]
base/util patch v2

>+//#include "nsStreamUtils.h"
??

>+/* From nsDebugImp.cpp: */
nsDebugImpl

>+NS_COM nsresult
>+NS_ErrorAccordingToNSPR()
Was this just copy & paste? I'm not sure that this will compile with the internal API. As well as renaming it, you might as well make it static.

>  return SetCharValue(nameEmpty.get(),
>-   aValue ? NS_LITERAL_CSTRING("true") : EmptyCString());
>+   aValue ? NS_LITERAL_CSTRING("true") : NS_LITERAL_CSTRING(""));
[This is annoying if it's needed to compile...]

> static PRBool ConvertibleToNative(const nsAutoString& str)
I don't know what the right thing to do here is, but I doubt this is it.

>+#ifndef MOZILLA_INTERNAL_API
>+NS_COM PRBool MsgIsASCII( const nsAString& aString )
Why not rename this to IsASCII and be done with it? (Maybe we should try to get this into the external API too.) Also, it needs to be NS_MSG_BASE, not NS_COM. (NS_COM is for xpcom exports.)

>+NS_COM PRBool MsgUnescapeURL(const char *str, PRInt32 len, PRUint32 flags, nsACString &result)
>+NS_COM PRBool MsgUnescapeURL(const nsACString &str, PRUint32 flags,
>+NS_COM nsCString &
>+MsgUnescapeURL(nsCString &str)
>+NS_COM char* MsgEscape(const char * str, nsEscapeMask flags)
Callers shouldn't be using these, they should be converted to use the existing Msg(Un)EscapeString(URL) methods.

>+  nsIAtom* atom;
>+  atomService->GetAtomUTF8(aString, &atom);
>+  NS_IF_ADDREF(atom);
GetAtomUTF8 already addrefs the atom.

>+NS_MSG_BASE void MsgReplaceSubstring(nsAString &str,  const PRUnichar *what, const PRUnichar *replacement)
Not sure who the caller is, but it would be more convenient if what was a const nsAString& instead.

>+ * Note: these values are copied in nsINetUtil.idl. Any changes should be kept
>+ * in sync.
Or you could include nsINetUtil.h and access the values directly.

	neil@parkwaycc.co.uk
	Comment 79 • 15 years ago

(In reply to comment #72)
>>> nsresult
>>> nsMsgBuildErrorMessageByID(PRInt32 msgID, nsString& retval, nsString* param0, nsString* param1)
>> Ideally this method would be completely rewritten based on FormatStringFromID.
>> Fortunately there are only two pairs of affected strings.
>Sorry, I'm not sure how to solve this. Where can I get stringID for
>FormatStringFromID? Is it the same as for msgID for nsMsgBuildErrorMessageByID?
I filed bug 536739 for this.

	neil@parkwaycc.co.uk
	Comment 80 • 15 years ago

Comment on attachment 420546 [details] [diff] [review]
compose patch v2

Use of (Un)Escape needs to be fixed. Some examples (not exhaustive):

>         nsCAutoString unescapeUrl(turl);
>-        nsUnescape(unescapeUrl.BeginWriting());
>+        MsgUnescape(unescapeUrl.BeginWriting());
This should use the version that unescapes into a new string, something like
MsgUnescape(turl, flags, unescapeurl);

>-  nsUnescape (attachment->m_real_name);
>+  MsgUnescape (attachment->m_real_name);
Whereas this might have to be rewritten like this:
nsCString unescapedName;
MsgUnescape(nsDependentCString(attachment->m_real_name), flags, unescapedName);
strcpy(attachment->m_real_name, unescapedName.get());

>           char *unescapedData = ToNewCString(escapedVCard);
>           if (!unescapedData)
>               return NS_ERROR_OUT_OF_MEMORY;
>-          nsUnescape(unescapedData);
>+          MsgUnescape(unescapedData);
>           char *result = PL_Base64Encode(unescapedData, 0, nsnull);
nsCString unescapedData;
MsgUnescape(escapedVCard, flags, unescapedData);
char *result = PL_Base64Encode(unescapedData.get(), 0, nsnull);

>+      nsCAutoString target_charset(NS_LossyConvertUTF16toASCII(aCharset).get());
NS_LossyConvertUTF16toASCII target_charset(aCharset);

>+      PRBool formatflowed = UseFormatFlowed(target_charset.get());
Or PRBool formatflowed = UseFormatFlowed(NS_LossyConvertUTF16toASCII(aCharset).get());

>+    PRUint32 pos;
>+    if ( (pos = sigData.Find(metaCharset, CaseInsensitiveCompare)) != -1 ) {
>+      sigData.Cut(pos, pos + metaCharset.Length());
>+    }
Does this not work with the internal API too?

>-          recipient.mEmail.Right(domain, recipient.mEmail.Length() - atPos - 1);
>+          domain = StringTail(recipient.mEmail, recipient.mEmail.Length() - atPos - 1);
Substring might work better in this and similar cases.

>-            NS_GetCurrentThread(getter_AddRefs(thread));
>+            nsCOMPtr<nsIThread> thread(do_GetCurrentThread());
This is declaring a new thread. Did you mean:
thread = do_GetCurrentThread();

>         nsCAutoString cPath;
>         nsAutoString path;
>         mTempFile->GetNativePath(cPath);
>-        NS_CopyNativeToUnicode(cPath, path);
>+        CopyASCIItoUTF16(cPath, path);
[You only correctly fixed one of these cases.]

	Matthew Gertner
	Comment 81 • 15 years ago

Which tree are these patches based off? I just pulled the latest trunk and applied the base/util patch, and I got some merge failures. Should I be pulling a specific branch?

	Jan Horak [:jhorak] Assignee
	Comment 82 • 15 years ago

(In reply to comment #81)
> Which tree are these patches based off? I just pulled the latest trunk and
> applied the base/util patch, and I got some merge failures. Should I be pulling
> a specific branch?
It is based on:
4568:2de08c283083 (http://hg.mozilla.org/comm-central/rev/2de08c283083) from Dec 21. I will sync following patches to head.

	Jan Horak [:jhorak] Assignee
	Comment 83 • 15 years ago

Replaced some StringTail by Substring, added missing headers, sync patch to trunk.

	Jan Horak [:jhorak] Assignee
	Comment 84 • 15 years ago

MsgUnescape replaced by MsgUnescapeString, sync patch to trunk.

>>+    PRUint32 pos;
>>+    if ( (pos = sigData.Find(metaCharset, CaseInsensitiveCompare)) != -1 ) {
>>+      sigData.Cut(pos, pos + metaCharset.Length());
>>+    }
>Does this not work with the internal API too?
Unfortunately no, CaseInsensitiveCompare is undeclared in internal API.

	Jan Horak [:jhorak] Assignee
	Comment 85 • 15 years ago

Fixed MsgReplaceSubstring (require nsACString instead of char * now), sync to trunk.

	Jan Horak [:jhorak] Assignee
	Comment 86 • 15 years ago

Replaced some StringTail by Substring, few Find->FindChar, MsgUnescape->MsgUnescapeString and getting current thread fixed.

	Jan Horak [:jhorak] Assignee
	Comment 87 • 15 years ago

AppendNative()->Append(), MoveToNative->MoveTo(), StringTail->Substring, Find->FindChar.

	Jan Horak [:jhorak] Assignee
	Comment 88 • 15 years ago

Fixed current thread retrieve, Find->FindChar.

	Jan Horak [:jhorak] Assignee
	Comment 89 • 15 years ago

Find->FindChar, fixed Adopt issue

	Jan Horak [:jhorak] Assignee
	Comment 90 • 15 years ago

nsUnescape->MsgUnescapeString, StringTail->Substring, 

> Can you write this as nsCString value = Substring(header, colon + 1); ?
> [I think you can use Substring like this in both internal and external API.]
> [Some of the other uses of Right would be better converted to Substring too.]
I can't compile it with MOZILLA_INTERNAL_API:
nsNNTPProtocol.cpp: In member function ‘void nsNNTPProtocol::ParseHeaderForCancel(char*)’:
nsNNTPProtocol.cpp:2631: error: conversion from ‘const nsDependentCSubstring’ to non-scalar type ‘nsCString’ requested

	Jan Horak [:jhorak] Assignee
	Comment 91 • 15 years ago

Fixed get of current thread, renamed nsErrorAccordingToNSPR, removed MsgUnescapeURL, MsgUnescapeURL and MsgEscape.

	Jan Horak [:jhorak] Assignee
	Comment 92 • 15 years ago

Hopefully fixed all your comments. Sorry for this one-man review avalanche Neil (I'm not sure who to address besides you), feel free to reassign the review process whoever you want.

	neil@parkwaycc.co.uk
	Comment 93 • 15 years ago

(In reply to comment #90)
> nsNNTPProtocol.cpp: In member function ‘void
> nsNNTPProtocol::ParseHeaderForCancel(char*)’:
> nsNNTPProtocol.cpp:2631: error: conversion from ‘const nsDependentCSubstring’
> to non-scalar type ‘nsCString’ requested
Sigh. I keep forgetting that t = v; allows the compiler to try t = u(v); and
T t(v); allows the compiler to try T t(u(v)); but T t = v; does not.

	neil@parkwaycc.co.uk
	Comment 94 • 15 years ago

Comment on attachment 421840 [details] [diff] [review]
base/util patch v3

>-    nsCAutoString portString(StringTail(hostname, hostname.Length() - colonPos));
Side note: I strongly suspect an off-by-one error here ;-)

>-    rv = NS_GetCurrentThread(getter_AddRefs(mProviderThread));
>-    if (NS_FAILED(rv)) return rv;
>+    nsCOMPtr<nsIThread> thread(do_GetCurrentThread());
Oops, copy and paste error perhaps? You want to use the existing member.

	neil@parkwaycc.co.uk
	Comment 95 • 15 years ago

Comment on attachment 421834 [details] [diff] [review]
mime patch v3

Wait a second, doesn't nsStreamConverter.cpp need to have its use of ReplaceSubstring fixed?

	neil@parkwaycc.co.uk
	Comment 96 • 15 years ago

My bad, someone already wrapped that one in an #ifdef

	neil@parkwaycc.co.uk
	Comment 97 • 15 years ago

Comment on attachment 421840 [details] [diff] [review]
base/util patch v3

>+NS_MSG_BASE void MsgReplaceSubstring(nsAString &str,  const nsAString &what, const PRUnichar *replacement)
[Nit: double space before const nsAString]
OK, so I ended up confusing you here. You changed what from a char_type* to an abstract_string& type in both versions of MsgReplaceSubstring, but in fact what I meant was that both what and replacement should change from PRUnichar* to nsAString&, but leave the char* version of the methods as char*.
Note that Replace calls BeginReading if you pass it a const nsAString&, so you may want to call BeginReading yourself outside of the loop.

Also, would it make sense to create internal API defines to map these e.g.
#ifdef MOZILLA_INTERNAL_API
#define MsgReplaceSubstring(str, what, replacement) \
        str.ReplaceSubstring(what, replacement)
#else
NS_MSG_BASE void MsgReplaceSubstring(nsACString& str, const char *what, const char *replacement);
NS_MSG_BASE void MsgReplaceSubstring(nsAString& str, const nsAString& what, const nsAString& replacement);
#endif

Then the main code would simply call MsgReplaceSubstring without #ifdef.

	neil@parkwaycc.co.uk
	Comment 98 • 15 years ago

Comment on attachment 421810 [details] [diff] [review]
extensions patch v 3

You don't seem to have read comment #68 :-(

	neil@parkwaycc.co.uk
	Comment 99 • 15 years ago

(In reply to comment #84)
>>>+    PRUint32 pos;
>>>+    if ( (pos = sigData.Find(metaCharset, CaseInsensitiveCompare)) != -1 ) {
>>>+      sigData.Cut(pos, pos + metaCharset.Length());
>>>+    }
>>Does this not work with the internal API too?
>Unfortunately no, CaseInsensitiveCompare is undeclared in internal API.
I see, so the nearest equivalent would still involve an #ifdef i.e.
#ifdef MOZILLA_INTERNAL_API
  PRUint32 pos = sigData.Find(metaCharset, PR_TRUE);
#else
  PRUint32 pos = sigData.FInd(metaCharset, CaseInsensitiveCompare);
#endif
  if (pos != -1)
    sigData.Cut(pos, metaCharset.Length());

	neil@parkwaycc.co.uk
	Comment 100 • 15 years ago

As I've been reading these patches I have of course noticed a number of recurring constructs, particularly some where the same operation has to be specified differently for internal and external linkage. They are:
1. ToInteger. This takes a PRInt32* parameter on the 1.9.2 branch but an nsresult* parameter in the external API. It may be possible to get bug 538476 landed on the 1.9.2 branch in which case we can simply go ahead and update the whole mailnews codebase in one go to use nsresult.
2. Last, SetCharAt, AppendASCII and nsACString::LowerCaseEqualsLiteral. It may be possible to get these added to the external API, at least on trunk, and possibly even on branch, so that we don't have to change mailnews.
3. NS_NewISuppportsArray. It may be possible to use a #define or inline:
#ifndef MOZILLA_INTERNAL_API
#define NS_NewISupportsArray(result) \
        CallCreateInstance(NS_SUPPORTSARRAY_CONTRACTID, result)
/* OR */
inline nsresult NS_NewISupportsArray(nsISupportsArray **aResult)
{
  return CallCreateInstance(NS_SUPPORTSARRAY_CONTRACTID, aResult);
}
#endif
4. CaseInsensitiveCompare. It may be possible to define an object to avoid having to #ifdef the use of CaseInsensitiveCompare, maybe something like this:
#ifdef MOZILLA_INTERNAL_API
struct MsgCaseInsensitiveCompare {
  operator PRBool() {
    return PR_TRUE;
  }
  operator nsCaseInsensitiveCStringComparator() {
    return nsCaseInsensitiveCStringComparator();
  }
  operator nsCaseInsensitiveStringComparator() {
    return nsCaseInsensitiveStringComparator();
  }
} CaseInsensitiveCompare;
#endif
5. Other methods such as RFindChar, ReplaceSubstring etc. If it is not possible to add these to the external API, then one alternative is to create a replacement method but provide alternative definitions. For instance
#ifdef MOZILLA_INTERNAL_API
#define MsgRFindChar(str, needle, pos) str.RFindChar(needle, pos)
#else
#define MsgRFindChar(str, needle, pos) Substring(str, 0, pos).RFindChar(needle)
#endif
Again, the idea here is to avoid #ifdef repetitively throughout the codebase.

	neil@parkwaycc.co.uk
	Comment 101 • 15 years ago

Filed bug 540322 on Last, SetCharAt etc.

	neil@parkwaycc.co.uk
	Comment 102 • 15 years ago

Comment on attachment 421809 [details] [diff] [review]
compose patch v3

>     // Unescape the path (i.e. un-URLify it) before making a FSSpec
>     nsCAutoString filePath;
>     filePath.Adopt(nsMsgGetLocalFileFromURL(sourceURISpec.get()));
>-    nsUnescape(filePath.BeginWriting());
>+    nsCAutoString escapedFilePath;
>+    MsgUnescapeString(filePath, 0, escapedFilePath);
You're actually unescaping here. It would be better to rename the original variable escapedFilePath i.e.
nsCAutoString escapedFilePath;
escapedFilePath.Adopt(nsMsgGetLocalFileFromURL(sourceURISpec.get()));
nsCAutoString filePath;
MsgUnescapeString(escapedFilePath, 0, filePath);

>+        MsgUnescapeString(turl, nsnull, unescapeUrl);
The second parameter is an integer, so you need to pass 0, not nsnull.

>-    goto done;
Oh, I do like this change!

>+          mCiteReference.Append(MsgEscapeURL(myGetter, esc_FileBaseName | esc_Forced, buf));
These should use the nsINetUtil constants...

>+    MsgUnescapeString(utf8Scheme, esc_SkipControl | esc_AlwaysCopy, _retval);
... but note that MsgUnescapeString forces esc_AlwaysCopy.

>+          domain = Substring(recipient.mEmail, atPos+1);
Nit: spaces around + operator

>+#ifdef MOZILLA_INTERNAL_API
>               && (classValue.EqualsIgnoreCase("moz-txt", 7) ||
>                   classValue.EqualsIgnoreCase("\"moz-txt", 8)))
>+#else
>+              && (classValue.Equals(NS_LITERAL_STRING("moz-txt"), CaseInsensitiveCompare) ||
>+                  classValue.Equals(NS_LITERAL_STRING("\"moz-txt"), CaseInsensitiveCompare)))
>+#endif
The use of the third parameter of EqualsIgnoreCase actually turns this into the equivalent of StringBeginsWith.

>   NS_NAMED_LITERAL_CSTRING(mftHeaderLabel, "Mail-Followup-To: ");
>+  nsCAutoString headers_match("\r\n");
>+  headers_match.Append(mftHeaderLabel);
>+
>   if ((StringHead(customHeaders, mftHeaderLabel.Length()) == mftHeaderLabel) ||
>-      (customHeaders.Find(NS_LITERAL_CSTRING("\r\n") + mftHeaderLabel) != -1))
>+      (customHeaders.Find(headers_match) != -1))
This was really badly written. Perhaps we should write this as
if (StringBeginsWith(customHeaders, "Mail-Followup-To: ") ||
    customHeaders.Find("\r\nMail-Followup-To: ") != -1)

>+                  aResult.Append(nsDependentCString(ipAddressString));
I don't think this needs the nsDependentCString any more.

>+#ifdef MOZILLA_INTERNAL_API
>         else if (responseLine.Compare("AUTH", PR_TRUE, 4) == 0)
>+#else
>+        else if (responseLine.Compare(NS_LITERAL_CSTRING("AUTH"), CaseInsensitiveCompare) == 0)
>+#endif
Again, some of these need to use StringBeginsWith.

>-    *((char **)getter_Copies(escapedHostname)) =
Oh, it feels so good to get rid of these.

>+    nsCAutoString searchPart;
>+    searchPart = Substring(escapedPath, startOfSearchPart);
I know this won't work as nsCAutoString searchPart = but maybe you could write it as nsCAutoString searchPart(Substring(escapedPath, startOfSearchPart)); ?

>+    PRUint32 numExtraChars = escapedPath.Length() - startOfSearchPart;
This is the same as searchPart.length(), although in fact...

>      escapedPath.Cut(startOfSearchPart, numExtraChars);
... this is the same as escapedPath.SetLength(startOfSearchPart); so that you don't actually need numExtraChars at all.

>   m_toPart = aRecipientsList;
>-  if (!m_toPart.IsEmpty())
>-    nsUnescape(m_toPart.BeginWriting());
>+  if (!m_toPart.IsEmpty()) {
>+    nsCString escaped_toPart;
>+    MsgUnescapeString(m_toPart, 0, escaped_toPart);
>+    m_toPart.Assign(escaped_toPart);
>+  }
There should be a way to unescape directly from aRecipientsList into m_toPart (in which case we wouldn't test it as we would want to unescape if empty).

	neil@parkwaycc.co.uk
	Comment 103 • 15 years ago

Comment on attachment 421829 [details] [diff] [review]
imap patch v2

>+      nsCString parentName;
>+      parentName = StringHead(tmpNewName, folderStart);
>       rv = GetFolder(parentName, getter_AddRefs(parent));
Just use GetFolder(StringHead(...), ...)

>     currentFolderPath->GetNativeLeafName(folderName);
>-    NS_CopyNativeToUnicode(folderName, currentFolderNameStr);
>+    CopyASCIItoUTF16(folderName, currentFolderNameStr);
This should use GetLeafName

>+    leafName.Assign( Substring(parentName, folderStart + 1) );
Nit: I prefer leafName = Substring(...);

>   char delimiter = 0;
>   GetHierarchyDelimiter(&delimiter);
>-  nsAutoString str(delimiter);
>-  *onlineDelimiter = ToNewCString(str);
This could do with a rewrite. I was thinking along these lines:
char delimiter[2] = {0, 0};
GetHierarchyDelimiter(delimiter);
*onlineDelimiter = NS_strdup(delimiter);

>+  MsgEscapeString(nsDependentCString(mURI.get() + aFolderURL.Length()),
Substring(mURI, aFolderURL.Length())

>+      nsCAutoString urlSpec, escapedUrlSpec;
>       mailnewsUrl->GetSpec(urlSpec);
>+      MsgUnescapeString(urlSpec, 0, escapedUrlSpec);
More (un)escape variable naming confusion.

>-            oldBoxName.Right(leafName, length-(leafStart+1));
>+            leafName = Substring(oldBoxName, leafStart-1);
leafStart + 1

>-          if (FolderNeedsACLInitialized(nsPromiseFlatCString(mb->GetMailboxName()).get()))
These should use PromiseFlatCString

>-  LossyCopyUTF16toASCII(pHost, m_logonHost);
No need to change this, it exist in external API.

>-    urlString.Right(mimePart, urlString.Length() - sectionPos); 
>+    nsDependentCSubstring mimePart = Substring(urlString, sectionPos);
>     uri.Append(mimePart);
Just use uri.Append(Substring(urlString, sectionPos));

>-  rv = nsParseImapMessageURI(nsDependentCString(aMessageURI).get(), folderURI, aMsgKey, nsnull);
>+  rv = nsParseImapMessageURI(aMessageURI.BeginReading(), folderURI, aMsgKey, nsnull);
PromiseFlatCString again.

>-    msgurl->SetUri(nsDependentCString(aImapURI).get());
>+    msgurl->SetUri(PromiseFlatCString(aImapURI).BeginReading());
So near, and yet, so far...

>+      CopyUTF16toMUTF7(nsDependentString(newLeafName.BeginReading(), newLeafName.Length()),
>+          utfNewName); // FIXME wants: CopyUTF16toMUTF7(newLeafName, utfNewName);
PromiseFlatCString again?

>       rv = mailnewsUrl->GetFileName(folderName);
>-      if (!folderName.IsEmpty())
>-        NS_UnescapeURL(folderName);
>+      if (!folderName.IsEmpty()) {
>+        nsCString escapedFolderName;
>+        MsgUnescapeString(folderName, nsnull, escapedFolderName);
>+        folderName.Assign(escapedFolderName);
>+      }
Here, get the file name into the temporary escapedFolderName, and then unescape it (with 0, not nsnull) into folderName.

>   *resultingCanonicalPath = PL_strdup(resultPath + 1);
>-  nsUnescape(*resultingCanonicalPath);
>+  nsCString escapedResultingCanonicalPath;
>+  MsgUnescapeString(nsCString(*resultingCanonicalPath), 0, escapedResultingCanonicalPath);
Here you need to pass in nsDependentCString(resultPath + 1)

>-      uriStr.Right(keyStr, uriStr.Length() - (keySeparator + 1));
>+      keyStr = StringTail(uriStr, uriStr.Length() - (keySeparator + 1));
Better written as Substring(uriStr, keySeparator + 1)

>         nsCString partSubStr;
>-        uriStr.Right(partSubStr, uriStr.Length() - keyEndSeparator);
>+        partSubStr = Substring(uriStr, keyEndSeparator);
>         *part = ToNewCString(partSubStr);
ToNewCString might, or might not, work directly on a Substring?

	neil@parkwaycc.co.uk
	Comment 104 • 15 years ago

I just noticed that the only flags accepted by MsgUnescapeString are the nsINetUtil::ESCAPE_URL_ONLY_NONASCII and nsINetUtil::ESCAPE_URL_SKIP_CONTROL flags. I didn't look to see whether you actually need any of the other flags.

	neil@parkwaycc.co.uk
	Comment 105 • 15 years ago

Comment on attachment 421831 [details] [diff] [review]
local patch v3

>-  nsCOMPtr<nsIMsgFolder> folder = do_QueryElementAt(folders, 0);
>+  nsCOMPtr<nsIMsgFolder> folder;
>+  folders->QueryElementAt(0, NS_GET_IID(nsIMsgFolder), getter_AddRefs(folder));
[These changes really irritate me because they'll have to be reverted once the code switches from nsISupportsArray to nsIArray.]

	neil@parkwaycc.co.uk
	Comment 106 • 15 years ago

Comment on attachment 421832 [details] [diff] [review]
mapi patch

>   nsMsgKey msgKey = keyString.ToInteger(&irv);
This won't compile with the external string API, will it?

	neil@parkwaycc.co.uk
	Comment 107 • 15 years ago

Comment on attachment 421838 [details] [diff] [review]
news patch v3

>-  char *unescapedUserPass = ToNewCString(userPass);
>-  if (!unescapedUserPass)
>-    return NS_ERROR_OUT_OF_MEMORY;
>-  nsUnescape(unescapedUserPass);
>+  nsCString unescapedUserPass;
>+  MsgUnescapeString(userPass, 0, unescapedUserPass);
Oh, this is a sight for sore eyes!

>+          MsgUnescapeString(commandSpecificData, 0, escapedCommandSpecificData);
Bad variable naming again.

>+    MsgUnescapeString(name,
>                    esc_FileBaseName|esc_Forced|esc_AlwaysCopy, unescapedName);
MsgUnescapeString doesn't accept these flags...

>-      newsUrl.Mid(groupName, groupPos + kNewsURIGroupQueryLen,
>+      groupName = Substring(groupName, groupPos + kNewsURIGroupQueryLen,
>                   keyPos - groupPos - kNewsURIGroupQueryLen);
>-      newsUrl.Mid(keyStr, keyPos + kNewsURIKeyQueryLen,
>+      keyStr = Substring(newsUrl, keyPos + kNewsURIKeyQueryLen,
>                   newsUrl.Length() - keyPos - kNewsURIKeyQueryLen);
Nit: please fix up the indentation on the continuation lines.

>+  MsgUnescapeString(nsDependentCString(path.get() + 1), 0, unescapedPath); /* skip the leading slash */
Use Substring(path, 1)

>+
> 
Nit: one blank line is enough ;-)

	neil@parkwaycc.co.uk
	Comment 108 • 15 years ago

Comment on attachment 421843 [details] [diff] [review]
base/src patch v2

>+  else if (StringBeginsWith(uriString, NS_LITERAL_STRING("mailbox:")) &&
>+           (uriString.Find(NS_LITERAL_STRING(".eml?"), CaseInsensitiveCompare) != -1))
>+#endif
>   {
>     // if we have a mailbox:// url that points to an .eml file, we have to read
>     // the file size as well
>-    uriString.ReplaceSubstring(NS_LITERAL_STRING("mailbox:"), NS_LITERAL_STRING("file:"));
>+    uriString.Replace(uriString.Find(NS_LITERAL_STRING("mailbox:")), 8, NS_LITERAL_STRING("file:"));
So, this is good, but we already know the string begins with mailbox: so we can replace that with zero. (Same goes for the other replacements.)

>+        const nsDependentCSubstring mimePart = StringTail(urlString, urlString.Length() - sectionPos);
>         fullMessageUri.Append(mimePart);
This should be written .Append(Substring(urlString, sectionPos))

>+          queryPart.Replace(queryPart.Find("type=message/rfc822"), 19,  "type=application/x-message-display");
19,  might be better written sizeof("type=message/rfc822") - 1,

>+    LossyCopyUTF16toASCII(nsDependentString(aToolTipString),
>                            nativeToolTipString);
We should ditch the ANSI fallback code, we only support W2K+.

>-  nsCOMPtr<nsIMsgIdentity> identity( do_QueryElementAt(m_identities, 0, &rv));
>-  identity.swap(*aDefaultIdentity);
>-  return rv;
>+  return m_identities->QueryElementAt(0, NS_GET_IID(nsIMsgIdentity), (void **) aDefaultIdentity);
Heh, this one almost looks nicer using external API ;-)

>   nsCOMPtr<nsISupportsArray> accounts;
>-  NS_NewISupportsArray(getter_AddRefs(accounts));
>+  accounts = do_CreateInstance(NS_SUPPORTSARRAY_CONTRACTID);
Nit: you can write this on one line.

>+  nsCOMPtr<nsIMutableArray> nodes;
>+  nodes = do_CreateInstance(NS_ARRAY_CONTRACTID, &rv);
Ditto.

>+  forceAllParts += (forceAllParts.FindChar('?') == -1) ? "?" : "&";
Nit: does this work with '?' : '&' ?

>-        aValue.Adopt(GetString(NS_LITERAL_STRING("messageUnread").get()));
>+        aValue.Assign(GetString(NS_LITERAL_STRING("messageUnread").get()));
You forgot to fix this one.

>+#ifdef MOZILLA_INTERNAL_API
>         if (junkScoreStr.IsEmpty() || (junkScoreStr.ToInteger((PRInt32*)&rv) == nsIJunkMailPlugin::IS_HAM_SCORE))
>           ApplyCommandToIndices(nsMsgViewCommandType::junk, (nsMsgViewIndex *) &row, 1);
>         else
>           ApplyCommandToIndices(nsMsgViewCommandType::unjunk, (nsMsgViewIndex *) &row, 1);
>-
>+#else
>+        if (junkScoreStr.IsEmpty() || (junkScoreStr.ToInteger(&rv) == nsIJunkMailPlugin::IS_HAM_SCORE))
>+          ApplyCommandToIndices(nsMsgViewCommandType::junk, (nsMsgViewIndex *) &row, 1);
>+        else
>+          ApplyCommandToIndices(nsMsgViewCommandType::unjunk, (nsMsgViewIndex *) &row, 1);
>+#endif
I can't see why you didn't just #ifdef the first line here?

>+    nsCOMArray<nsIMsgFolder> allFoldersCOMArray;
Not used?

>-      return NS_NewArrayEnumerator(targets, allFolders);
>+      return NS_NewArrayEnumerator(targets, m_folders);
So, I take it this is the required change, but the point is that it then allows you to make the previous loop readable?

>-              m_kTodayString.Adopt(GetString(NS_LITERAL_STRING("today").get()));
>+              m_kTodayString.Assign(GetString(NS_LITERAL_STRING("today").get()));
These changes aren't even necessary, aren't they?

>-            aValue.Adopt(GetString(NS_LITERAL_STRING("messagesWithNoStatus").get()));
>+            aValue.Assign(GetString(NS_LITERAL_STRING("messagesWithNoStatus").get()));
But these haven't been correctly fixed, unfortunately.

>-#include "nsPresContext.h"
>+//#include "nsPresContext.h"
If we don't need this, just delete it.

	neil@parkwaycc.co.uk
	Comment 109 • 15 years ago

I'm still thinking as to the best way to avoid scattering NS_GET_IID throughout the codebase; my latest idea is this:

template<class T, class U>
nsresult CallQueryElementAt(T* aArray, PRUint32 index, U** aResult)
{
  return aArray->QueryElementAt(index, NS_GET_TEMPLATE_IID(U),
                                reinterpret_cast<void**>(aResult));
}

	neil@parkwaycc.co.uk
	Comment 110 • 15 years ago

So, to summarise:
compose, imap, base/src: patch definitely contains errors
extensions, mapi, news, base/util: patch needs clarification
db, address, mime: patch ok
local: patch technically ready, but might be bitrotted by bug 540322 or by implementing simplifications suggested in some of my comments.

	neil@parkwaycc.co.uk
	Comment 111 • 15 years ago

So, I've been talking to some of the other mailnews peers, and we've already decided that sprinkling #ifdef through the code definitely isn't the way to go; when APIs don't have a suitable replacement (e.g. using StringHead instead of Left) and can't be added to XPCOM glue (e.g. do_QueryElementAt), we need to define helper methods, preferably #ifdef to use internal APIs when available.

	neil@parkwaycc.co.uk
	Comment 112 • 15 years ago

Comment on attachment 421810 [details] [diff] [review]
extensions patch v 3

Cancelling the remaining reviews due to the use of #ifdef

	neil@parkwaycc.co.uk
	Comment 113 • 15 years ago

Comment on attachment 421840 [details] [diff] [review]
base/util patch v3

It turns out that there's an NS_IsASCII which seems to be available when using the external API.

	neil@parkwaycc.co.uk
	Comment 114 • 15 years ago

I meant NS_IsAscii, sorry.

I looked at what helper methods we could use to ease the conversion. From base/src the list looks like this so far:

When using MOZILLA_INTERNAL_API
/* Allows us to pass CaseInsensitiveCompare to Find */
#define CaseInsensitiveCompare PR_TRUE
/* Allows us to pass &rv to ToInteger */
#define ToInteger(prv) ToInteger(reinterpret_cast<PRInt32*>(prv))
/* Allows us to use Last */
#define MsgLast(str) str.Last()
/* Allows us to use IsUTF8 */
#define MsgIsUTF8 IsUTF8

When not using MOZILLA_INTERNAL_API
/* Allows us to pass nsCaseInsensitiveC?StringComparator() to Equals */
#define nsCaseInsensitiveCStringComparator() CaseInsensitiveCompare
#define nsCaseInsensitiveStringComparator() CaseInsensitiveCompare
/* Allows us to use LowerCaseEqualsLiteral */
#define LowerCaseEqualsLiteral(str) Equals(str, CaseInsensitiveCompare)
/* Allows us to use kNotFound */
#define kNotFound -1
/* Allows us to use SetCharAt */
#define SetCharAt(ch, index) Replace(index, 1, ch)
/* Replacement for Last */
#define MsgLast(str) str[str.Length() - 1]
/* Existing replacement for IsUTF8 */
NS_MSG_BASE PRBool MsgIsUTF8(const nsACString&);

	neil@parkwaycc.co.uk
	Comment 115 • 15 years ago

In fact when using the external API I can do better for Last() by using
#define Last() EndReading()[-1]

	neil@parkwaycc.co.uk
	Comment 116 • 15 years ago

Comment on attachment 421843 [details] [diff] [review]
base/src patch v2

>-  nsCOMPtr<nsISupportsArray> nodes;
>-  rv = NS_NewISupportsArray(getter_AddRefs(nodes));
>+  nsCOMPtr<nsIMutableArray> nodes;
>+  nodes = do_CreateInstance(NS_ARRAY_CONTRACTID, &rv);
>   if (NS_FAILED(rv)) return rv;
> 
>   rv = NS_NewArrayEnumerator(_retval, nodes);
Ideally nodes would be an nsCOMArray<nsIRDFResource>. Also, I don't know how the old code got away with creating the enumerator before populating the array...

	Matthew Gertner
	Comment 117 • 15 years ago

I've been working on getting import, using Jan's patch as a starting point. I've got it building on Mac, but not yet on Windows. I also haven't integrated a lot of Neil's comments on other patches so I"ll have to do that before it's ready for review.

	neil@parkwaycc.co.uk
	Comment 118 • 15 years ago

After discussion with another of the mailnews peers, I have created this consolidated list of APIs with their suggested conversions.