377319 - Convert mailnews to frozen/external linkages

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Scott MacGregor]

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.

Comment 1

[Mark Banner (:standard8)]

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?

Comment 2

[Prasad Sunkari [:prasad]]

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.

Comment 3

[Mark Banner (:standard8)]

(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?

Comment 4

[[e-mail]]

(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.

Comment 5

[Axel Hecht]

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.

Comment 6

[David :Bienvenu]

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

Comment 7

[Mark Banner (:standard8)]

(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?

Comment 8

[Mark Banner (:standard8)]

(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?

Comment 9

[Axel Hecht]

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.

Comment 10

[Prasad Sunkari [:prasad]]

Attachment: 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.

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

Comment 11

[Mark Banner (:standard8)]

(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.

Comment 12

[Prasad Sunkari [:prasad]]

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.

Comment 13

[Magnus Melin [:mkmelin]]

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

Comment 14

[Mark Banner (:standard8)]

Attachment: Almost Finish address book migration to frozen linkage on Linux [Checkin: Comment 136]

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).

Comment 15

[Mark Banner (:standard8)]

(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.

Comment 16

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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

Comment 17

[Magnus Melin [:mkmelin]]

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

Comment 18

[Mark Banner (:standard8)]

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.

Comment 19

[Jan Horak [:jhorak]]

Attachment: [WIP] port to frozen linkage

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.

Comment 20

[Jan Horak [:jhorak]]

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.

Comment 21

[Mark Banner (:standard8)]

(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).

Comment 22

[Jan Horak [:jhorak]]

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.

Comment 23

[[e-mail]]

(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.

Comment 24

[Martin Stránský [:stransky] (ni? me)]

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?

Comment 25

[Jan Horak [:jhorak]]

Attachment: address book patch

Comment 26

[Jan Horak [:jhorak]]

Attachment: base/build patch

Comment 27

[Jan Horak [:jhorak]]

Attachment: base/search patch

Comment 28

[Jan Horak [:jhorak]]

Attachment: base/src patch

Comment 29

[Jan Horak [:jhorak]]

Attachment: base/util patch

Comment 30

[Jan Horak [:jhorak]]

Attachment: compose patch

Comment 31

[Jan Horak [:jhorak]]

Attachment: db patch

Comment 32

[Jan Horak [:jhorak]]

Attachment: extensions patch

Comment 33

[Jan Horak [:jhorak]]

Attachment: imap patch

Comment 34

[Jan Horak [:jhorak]]

Attachment: import patch

Comment 35

[Jan Horak [:jhorak]]

Attachment: local patch

Comment 36

[Jan Horak [:jhorak]]

Attachment: mapi patch

Comment 37

[Jan Horak [:jhorak]]

Attachment: mime patch

Comment 38

[Jan Horak [:jhorak]]

Attachment: news patch

Comment 39

[Jan Horak [:jhorak]]

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.

Comment 40

[Robert Kaiser]

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.

Comment 41

[[e-mail]]

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.

Comment 42

[Mark Banner (:standard8)]

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.

Comment 43

[Mark Banner (:standard8)]

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.

Comment 44

[[e-mail]]

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?

Comment 45

[[e-mail]]

(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?

Comment 46

[Mark Banner (:standard8)]

(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?

Comment 47

[[e-mail]]

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.

Comment 48

[[e-mail]]

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).

Comment 49

[[e-mail]]

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 ? '?' : '&';]

Comment 50

[[e-mail]]

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?]

Comment 51

[[e-mail]]

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.

Comment 52

[[e-mail]]

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

This patch (unsurprisingly) looks incomplete.

Comment 53

[[e-mail]]

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());
???

Comment 54

[[e-mail]]

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.

Comment 55

[[e-mail]]

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);

Comment 56

[[e-mail]]

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 ();

Comment 57

[[e-mail]]

(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.

Comment 58

[[e-mail]]

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

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

Comment 59

[[e-mail]]

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.

Comment 60

[[e-mail]]

(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.)

Comment 61

[Mark Banner (:standard8)]

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.

Comment 62

[Jan Horak [:jhorak]]

Attachment: extensions patch v. 2

Thanks for comments, sending hopefully better version.

Comment 63

[Jan Horak [:jhorak]]

Attachment: mime patch v 2

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

Comment 64

[Jan Horak [:jhorak]]

Attachment: address book patch v2

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?

Comment 65

[Jan Horak [:jhorak]]

Attachment: local patch v2

Comment 66

[Jan Horak [:jhorak]]

Attachment: news patch v2

Fixed version according to your comments.

Comment 67

[Matthew Gertner]

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.

Comment 68

[[e-mail]]

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, '@'))

Comment 69

[[e-mail]]

[But with better formatting than Bugzilla default wrapping!]

Comment 70

[[e-mail]]

(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.

Comment 71

[Jan Horak [:jhorak]]

Attachment: address book patch v3

> >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.

Comment 72

[Jan Horak [:jhorak]]

Attachment: compose patch v2

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?

Comment 73

[Jan Horak [:jhorak]]

Attachment: base/util patch v2

(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.

Comment 74

[Matthew Gertner]

(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.

Comment 75

[[e-mail]]

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

Comment 76

[[e-mail]]

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.

Comment 77

[[e-mail]]

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.

Comment 78

[[e-mail]]

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.

Comment 79

[[e-mail]]

(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.

Comment 80

[[e-mail]]

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.]

Comment 81

[Matthew Gertner]

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?

Comment 82

[Jan Horak [:jhorak]]

(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.

Comment 83

[Jan Horak [:jhorak]]

Attachment: address book patch v4

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

Comment 84

[Jan Horak [:jhorak]]

Attachment: compose patch v3

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.

Comment 85

[Jan Horak [:jhorak]]

Attachment: extensions patch v 3

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

Comment 86

[Jan Horak [:jhorak]]

Attachment: imap patch v2

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

Comment 87

[Jan Horak [:jhorak]]

Attachment: local patch v3

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

Comment 88

[Jan Horak [:jhorak]]

Attachment: mapi patch

Fixed current thread retrieve, Find->FindChar.

Comment 89

[Jan Horak [:jhorak]]

Attachment: mime patch v3

Find->FindChar, fixed Adopt issue

Comment 90

[Jan Horak [:jhorak]]

Attachment: news patch v3

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

Comment 91

[Jan Horak [:jhorak]]

Attachment: base/util patch v3

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

Comment 92

[Jan Horak [:jhorak]]

Attachment: base/src patch v2

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.

Comment 93

[[e-mail]]

(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.

Comment 94

[[e-mail]]

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.

Comment 95

[[e-mail]]

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?

Comment 96

[[e-mail]]

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

Comment 97

[[e-mail]]

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.

Comment 98

[[e-mail]]

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

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

Comment 99

[[e-mail]]

(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());

Comment 100

[[e-mail]]

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.

Comment 101

[[e-mail]]

Filed bug 540322 on Last, SetCharAt etc.

Comment 102

[[e-mail]]

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).

Comment 103

[[e-mail]]

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?

Comment 104

[[e-mail]]

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.

Comment 105

[[e-mail]]

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.]

Comment 106

[[e-mail]]

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?

Comment 107

[[e-mail]]

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 ;-)

Comment 108

[[e-mail]]

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.

Comment 109

[[e-mail]]

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));
}

Comment 110

[[e-mail]]

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.

Comment 111

[[e-mail]]

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.

Comment 112

[[e-mail]]

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

Cancelling the remaining reviews due to the use of #ifdef

Comment 113

[[e-mail]]

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.

Comment 114

[[e-mail]]

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&);

Comment 115

[[e-mail]]

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

Comment 116

[[e-mail]]

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...

Comment 117

[Matthew Gertner]

Attachment: import WIP patch

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.

Comment 118

[[e-mail]]

Attachment: Draft conversions

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

Comment 119

[[e-mail]]

Attachment: Preprocessed conversions

This starts by implementing all the "automatic" conversions. Note that the ToInteger and CaseInsensitiveCompare conversions aren't used yet, while some of the existing utility methods are now only used with the external API.

Comment 120

[[e-mail]]

Attachment: Preprocessed conversions

With all the back-and-forth between APIs, some changes got lost :-(

Comment 121

[Jan Horak [:jhorak]]

Okay, how should I proceed now? There's been a lot of work on these patches. Should I drop it and use your guide and do it from the scratch?

Comment 122

[[e-mail]]

(In reply to comment #121)
> Okay, how should I proceed now? There's been a lot of work on these patches.
And it's much appreciated, because without that I had no idea what sort of problems we are facing.

> Should I drop it and use your guide and do it from the scratch?
As per comment 111, those parts of your patches that use #ifdef (except in nsMsgUtils) need to be dropped. Other parts of your patches are OK, but some of your work may have been obsoleted by my attachment 424006 [details] [diff] [review].

And don't forget that the guide isn't final; feel free to argue your case.

Comment 123

[Matthew Gertner]

Jan, how is your schedule looking to work on this? I'm working on it too (getting patches to build on Windows right now) and I'd like to avoid duplicating work. Shall we divide up the modules and work together on adapting them to Neil's guide? If so, I'll post my changes first (lot of build problems on Windows that I've had to fix) so we can work off a common base.

Comment 124

[David :Bienvenu]

Comment on attachment 424006 [details] [diff] [review]
Preprocessed conversions

I'm getting a bunch of xpcshell test failures and crashes running with this patch. I need to try backing it out and see if I see the same failures and crashes.

Comment 125

[Jan Horak [:jhorak]]

(In reply to comment #123)
> Jan, how is your schedule looking to work on this? I'm working on it too
> (getting patches to build on Windows right now) and I'd like to avoid
> duplicating work. Shall we divide up the modules and work together on adapting
> them to Neil's guide? If so, I'll post my changes first (lot of build problems
> on Windows that I've had to fix) so we can work off a common base.

Cool, I can't test patches for import because I don't have Windows installed. I'll post patches without #ifdefs today (i hope). Then you can suggest which modules you'd like to work on (at least import would be nice :) ).

Comment 126

[Matthew Gertner]

(In reply to comment #125)
> Cool, I can't test patches for import because I don't have Windows installed.
> I'll post patches without #ifdefs today (i hope). Then you can suggest which
> modules you'd like to work on (at least import would be nice :) ).

Okay, I'll try to get import and dependencies building on Windows today. Looking forward to your new patches.

Comment 127

[Jan Horak [:jhorak]]

(In reply to comment #118)
> Created an attachment (id=423807) [details]
> # (Lossy)Copy(ASCII|UTF8|16)to(ASCII|UTF8|16)(p, s)
>    * Rewrite to include nsDependent[C]String(p)
Do the nsDependent[C]String convert from utf8 to utf16 and other way round? nsDependentCString does not accept PRUnichar* and nsDependentString does not accept char*.

> # nsDependent[C]String(a)
>    * Rewrite to use PromiseFlat[C]String(a)
>    * Do not confuse with nsDependent[C]String(p)
There are many of nsDependent[C]String (in approx 334 lines). It is required to replace all occurences?

> # PromiseFlat[C]String(s)
>    * Rewrite to s!
I'm afraid it is not possible. PromiseFLat[C]String(s) is used when 's' is nsA[C]String. This class does not have
get() method to get (null terminated?) PRUnichar*/[char*]. To get pointer to data the PromiseFlat[C]String(s).get() method is used.

> # b ? NS_LITERAL_CSTRING("str") : EmptyCString()
>    * Rewrite to b ? NS_LITERAL_CSTRING("str") : NS_LITERAL_CSTRING("")
Shows some compilation error:
nsImapOfflineSync.cpp: In member function ‘void nsImapOfflineSync::ProcessKeywordOperation(nsIMsgOfflineImapOperation*)’:
nsImapOfflineSync.cpp:359: error: no match for ternary ‘operator?:’ in ‘(((nsImapOfflineSync*)this)->nsImapOfflineSync::mCurrentPlaybackOpType == 1024) ? keywords : (const nsDependentCString_external&)((const nsDependentCString_external*)(& nsDependentCString_external(((const char*)""), 0u)))’

> # ISASCII, IS_SPACE, toupper
>    * Rewrite to NS_IsAscii, isspace, NS_ToUpper
An isspace requires inclusion of ctype.h. It is ok to add it where required or should we include some other header file?

> # getter_Copies(a)
>    * Rewrite to getter_Copies(s); a.Assign(s)
A getter_Copies is frequently used (~700 occurences). It can be time consuming to check all of them. Is it required?

> # a8.LowerCaseEqualsLiteral("str")
>    * Rewrite to a8.Equals(str, nsCaseInsensitiveStringComparator())
>    * Or we could #define MsgLowerCaseEqualsLiteral
The s.LowerCaseEqualsLiteral is in frozen linkage declared in nsStringAPI.h and it is used in about 82 cases in mailnews sourcecode. We may keep it as it is.

Thanks for clarification.

Comment 128

[[e-mail]]

As usual, my comments were way too cryptic. Let me explain them!

(In reply to comment #127)
> (In reply to comment #118)
> > Created an attachment (id=423807)
> > # (Lossy)Copy(ASCII|UTF8|16)to(ASCII|UTF8|16)(p, s)
> >    * Rewrite to include nsDependent[C]String(p)
> Do the nsDependent[C]String convert from utf8 to utf16 and other way round?
> nsDependentCString does not accept PRUnichar* and nsDependentString does not
> accept char*.
As you already know (your patches already correctly did this) they just convert the PRUnichar*/char* to nsAString/nsACString so that they can be passed to the relevant Copy method.

> > # nsDependent[C]String(a)
> >    * Rewrite to use PromiseFlat[C]String(a)
> >    * Do not confuse with nsDependent[C]String(p)
> There are many of nsDependent[C]String (in approx 334 lines). It is required to
> replace all occurences?
Oh no, this refers to the use of nsDependent[C]String on a ns(A/C)String type, rather than on a PRUnichar*/char* type.

> > # PromiseFlat[C]String(s)
> >    * Rewrite to s!
> PromiseFlat[C]String(s) is used when 's' is nsA[C]String.
This rewrite refers to a couple of cases where 's' is ns[C]String.

> > # b ? NS_LITERAL_CSTRING("str") : EmptyCString()
> >    * Rewrite to b ? NS_LITERAL_CSTRING("str") : NS_LITERAL_CSTRING("")
> Shows some compilation error:
> nsImapOfflineSync.cpp: In member function ‘void
> nsImapOfflineSync::ProcessKeywordOperation(nsIMsgOfflineImapOperation*)’:
> nsImapOfflineSync.cpp:359: error: no match for ternary ‘operator?:’ in
> ‘(((nsImapOfflineSync*)this)->nsImapOfflineSync::mCurrentPlaybackOpType ==
> 1024) ? keywords : (const nsDependentCString_external&)((const
> nsDependentCString_external*)(& nsDependentCString_external(((const char*)""),
> 0u)))’
But "keywords" isn't an NS_LITERAL_CSTRING.

> > # ISASCII, IS_SPACE, toupper
> >    * Rewrite to NS_IsAscii, isspace, NS_ToUpper
> An isspace requires inclusion of ctype.h. It is ok to add it where required or
> should we include some other header file?
Please assume it is for now. It sounds OK to me at least.

> > # getter_Copies(a)
> >    * Rewrite to getter_Copies(s); a.Assign(s)
> A getter_Copies is frequently used (~700 occurences). It can be time consuming
> to check all of them. Is it required?
Usually the name of the variable is a dead giveaway e.g. getter_Copies(aResult) needs to be checked, but getter_Copies(temp) probably doesn't. (Again, it's another nsA[C]String vs. ns[C]String issue.)

> > # a8.LowerCaseEqualsLiteral("str")
> >    * Rewrite to a8.Equals(str, nsCaseInsensitiveStringComparator())
> >    * Or we could #define MsgLowerCaseEqualsLiteral
> The s.LowerCaseEqualsLiteral is in frozen linkage declared in nsStringAPI.h and
> it is used in about 82 cases in mailnews sourcecode. We may keep it as it is.
But only for PRUnichar* strings, not for char* strings, no?

Comment 129

[David :Bienvenu]

Comment on attachment 424006 [details] [diff] [review]
Preprocessed conversions

This is making one xpcshell test fail, mailnews/compose/test/test_nsMsgCompose3.js

The test tests converting a hex value to decimal, among other things (that's what I suspect is broken). I'll try to quickly figure out what's broken.

Comment 130

[David :Bienvenu]

Comment on attachment 424006 [details] [diff] [review]
Preprocessed conversions

If I remove the #define of ToInteger, that test works again.

This is the patch that introduced the code that seems to have the issue:

https://bugzilla.mozilla.org/attachment.cgi?id=424979

in particular, the nsMsgCompose.cpp part of the patch. I'm hoping it will be obvious to you what the issue is (either in that patch, or your patch, I'm not sure which is to blame, so I'm not minusing the review request for now)

Comment 131

[[e-mail]]

Ah, this is because ToInteger can take two arguments, and I hadn't noticed.

Comment 132

[[e-mail]]

Attachment: ToInteger [Checkin: Comment 135]

OK, so the easiest workaround seems to be to specify the radix on all the ToInteger calls, then I can change the #define to have two parameters.

Comment 133

[David :Bienvenu]

do you want to attach a patch that changes the #define as well, or start with the last patch first?

Comment 134

[David :Bienvenu]

Comment on attachment 424006 [details] [diff] [review]
Preprocessed conversions

this is obsolete, I guess.

Comment 135

[[e-mail]]

Comment on attachment 425645 [details] [diff] [review]
ToInteger
[Checkin: Comment 135]

Pushed changeset 435954c9da59 to comm-central.

Comment 136

[Serge Gautherie (:sgautherie)]

Comment on attachment 294819 [details] [diff] [review]
Almost Finish address book migration to frozen linkage on Linux
[Checkin: Comment 136]


http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&sortby=Date&hours=2&date=explicit&mindate=2007-12-31+03%3A57&maxdate=2007-12-31+03%3A57

Comment 137

[[e-mail]]

Attachment: Fixed conversions [Checkin: Comment 148]

The only difference between this patch and attachment 424006 [details] [diff] [review] is that ToInteger now takes two parameters.

Comment 138

[Serge Gautherie (:sgautherie)]

Comment on attachment 418822 [details] [diff] [review]
base/build patch


This patch looks ready for checkin, is it not?

Comment 139

[Jan Horak [:jhorak]]

Attachment: nsMsgUtils addition and 'fix'

Sending nsMsgUtils.cpp/.h files which compiles fine with frozen linkages and internal API.

I've got a couple of comments to attachement #426114:
MsgReplaceChar: nsString.ReplaceChar(nsString/PRUnichar*, PRUnichar) is not defined for internal API.

NS_NewISupportsArray: overloaded function, it has one or two parameters (nsresult can be omitted). Macro can't do this job.

'#define CaseInsensitiveCompare PR_TRUE' unfortunately does not work in some cases. The previous suggested solution is working well (neil's struct MsgCaseInsensitiveCompare construction, etc.)


I added NS_NewMsgInterfaceRequestorAggregation, NS_NewMsgNotificationCallbacksAggregation, NS_NewMsgInterfaceRequestorAggregation.

Please consider my changes.

Comment 140

[[e-mail]]

(In reply to comment #139)
> I've got a couple of comments to attachement #426114:
> MsgReplaceChar: nsString.ReplaceChar(nsString/PRUnichar*, PRUnichar) is not
> defined for internal API.
Out of interest, where do you need to use this?

> NS_NewISupportsArray: overloaded function, it has one or two parameters
> (nsresult can be omitted). Macro can't do this job.
I only see one definition...

> '#define CaseInsensitiveCompare PR_TRUE' unfortunately does not work in some
> cases. The previous suggested solution is working well (neil's struct
> MsgCaseInsensitiveCompare construction, etc.)
Actually my struct doesn't work. But where are you finding that #define CaseInsensitiveCompare PR_TRUE does not work?

> I added NS_NewMsgInterfaceRequestorAggregation,
> NS_NewMsgNotificationCallbacksAggregation,
> NS_NewMsgInterfaceRequestorAggregation.
Offhand I'd say they should be called
MsgNewInterfaceRequestorAggregation,
MsgNewNotificationCallbacksAggregation,
MsgNewInterfaceRequestorAggregation.

Comment 141

[[e-mail]]

Hmm, the third one was in fact (NS_)MsgNewAtom, right?

Also, the #ifdef MOZILLA_INTERNAL_API block needs some extra #define statements
#define MsgNewAtom
        NS_NewAtom
#define MsgNewInterfaceRequestorAggregation
        NS_NewInterfaceRequestorAggregation
#define MsgNewNotificationCallbacksAggregation
        NS_NewNotificationCallbacksAggregation

[Eventually the callers need to be changed too, of course.]

Comment 142

[David :Bienvenu]

Comment on attachment 426114 [details] [diff] [review]
Fixed conversions
[Checkin: Comment 148]

typo nit:

+ * The following definitons exist for compatibility between the internal and

Comment 143

[Jan Horak [:jhorak]]

(In reply to comment #140)
> (In reply to comment #139)
> > I've got a couple of comments to attachement #426114:
> > MsgReplaceChar: nsString.ReplaceChar(nsString/PRUnichar*, PRUnichar) is not
> > defined for internal API.
> Out of interest, where do you need to use this?
nsMessenger.cpp:212: MsgReplaceChar(aResult, NS_LITERAL_STRING(FILE_PATH_SEPARATOR FILE_ILLEGAL_CHARACTERS).get(), '-');
nsMsgTagService.cpp:288: MsgReplaceChar(transformedTag, NS_LITERAL_STRING(" ()/{%*<>\\\"").get(), '_');
nsMsgCompose.cpp:2048: MsgReplaceChar(sanitizedSubj, NS_LITERAL_STRING(".").get(), '_');
There may be a better approach.
 
> > NS_NewISupportsArray: overloaded function, it has one or two parameters
> > (nsresult can be omitted). Macro can't do this job.
> I only see one definition...
Oh, my mistake. Compilation failed by unresolved reference due to missing #include "nsMsgUtils.h". Also there is no sign of overloaded NS_NewISupportsArray function.

> > '#define CaseInsensitiveCompare PR_TRUE' unfortunately does not work in some
> > cases. The previous suggested solution is working well (neil's struct
> > MsgCaseInsensitiveCompare construction, etc.)
> Actually my struct doesn't work. But where are you finding that #define
> CaseInsensitiveCompare PR_TRUE does not work?
Martin Stransky managed to make it work just fine (implementation in my patch).
When using internal API the problems shows for example in:

nsMessenger.cpp: In member function ‘nsresult nsMessenger::SaveAttachment(nsIFile*, const nsACString_internal&, const nsACString_internal&, const nsACString_internal&, void*, nsIUrlListener*)’:
nsMessenger.cpp:781: error: no matching function for call to ‘nsACString_internal::Equals(const char [25], int) const’
../../../mozilla/dist/include/nsTSubstring.h:265: note: candidates are: PRBool nsACString_internal::Equals(const nsACString_internal&) const
../../../mozilla/dist/include/nsTSubstring.h:266: note:                 PRBool nsACString_internal::Equals(const nsACString_internal&, const nsCStringComparator&) const
../../../mozilla/dist/include/nsTSubstring.h:268: note:                 PRBool nsACString_internal::Equals(const char*) const
../../../mozilla/dist/include/nsTSubstring.h:269: note:                 PRBool nsACString_internal::Equals(const char*, const nsCStringComparator&) const

nsMsgDBFolder.cpp: In member function 'virtual nsresult nsMsgDBFolder::GetChildNamed(const nsAString_internal&, nsIMsgFolder**)':
nsMsgDBFolder.cpp:3374: error: no matching function for call to 'nsString::Equals(const nsAString_internal&, int)'
../../../mozilla/dist/include/nsTSubstring.h:265: note: candidates are: PRBool nsAString_internal::Equals(const nsAString_internal&) const
../../../mozilla/dist/include/nsTSubstring.h:266: note:                 PRBool nsAString_internal::Equals(const nsAString_internal&, const nsStringComparator&) const
../../../mozilla/dist/include/nsTSubstring.h:268: note:                 PRBool nsAString_internal::Equals(const PRUnichar*) const
../../../mozilla/dist/include/nsTSubstring.h:269: note:                 PRBool nsAString_internal::Equals(const PRUnichar*, const nsStringComparator&) const

etc.
str.Equals() don't accept PR_BOOL as second parameter but just compare function.

> > I added NS_NewMsgInterfaceRequestorAggregation,
> > NS_NewMsgNotificationCallbacksAggregation,
> > NS_NewMsgInterfaceRequestorAggregation.
> Offhand I'd say they should be called
> MsgNewInterfaceRequestorAggregation,
> MsgNewNotificationCallbacksAggregation,
> MsgNewInterfaceRequestorAggregation.

Okay.

Comment 144

[[e-mail]]

(In reply to comment #143)
> nsMessenger.cpp:212: MsgReplaceChar(aResult,
> NS_LITERAL_STRING(FILE_PATH_SEPARATOR FILE_ILLEGAL_CHARACTERS).get(), '-');
> nsMsgTagService.cpp:288: MsgReplaceChar(transformedTag, NS_LITERAL_STRING("
> ()/{%*<>\\\"").get(), '_');
> nsMsgCompose.cpp:2048: MsgReplaceChar(sanitizedSubj,
> NS_LITERAL_STRING(".").get(), '_');
Ah yes, thanks for reminding me, I had almost forgotten about them.

> Compilation failed by unresolved reference due to missing
> #include "nsMsgUtils.h".
Yes, that's an easy one to overlook :-(

> When using internal API the problems shows for example in:
> str.Equals() don't accept PR_BOOL as second parameter but just compare
> function.
This is handled by these defines:
#define nsCaseInsensitiveCStringComparator() CaseInsensitiveCompare
#define nsCaseInsensitiveStringComparator() CaseInsensitiveCompare

Comment 145

[Jan Horak [:jhorak]]

(In reply to comment #144)
> (In reply to comment #143)
> > When using internal API the problems shows for example in:
> > str.Equals() don't accept PR_BOOL as second parameter but just compare
> > function.
> This is handled by these defines:
> #define nsCaseInsensitiveCStringComparator() CaseInsensitiveCompare
> #define nsCaseInsensitiveStringComparator() CaseInsensitiveCompare
Ah, I've changed nsCaseInsensitiveCStringComparator  into CaseInsensitiveCompare where possible. Should I turn it back, right?

Comment 146

[[e-mail]]

(In reply to comment #145)
> Ah, I've changed nsCaseInsensitiveCStringComparator into
> CaseInsensitiveCompare where possible. Should I turn it back, right?
Yes please.

Comment 147

[[e-mail]]

Comment on attachment 426258 [details] [diff] [review]
nsMsgUtils addition and 'fix' 

>-    
>+
These whitespace changes make me unsure as to where the real changes are.

>+NS_MSG_BASE void MsgReplaceChar(nsString& str, const PRUnichar *set, const PRUnichar replacement)
I think we should be able to put these in the #ifndef MOZILLA_INTERNAL_API section, and use #define MsgReplaceChar(string, needle, replacement) \
string.ReplaceChar(needle, replacement) when we have the internal API.
To do this we have to make set a const char* but I think that should work.
We also need a version with an nsString str and const char needle for import (Windows/Mac specific code, so you probably wouldn't have tried to compile.)

>+  char *c_str = str.BeginWriting();
>+  if (!c_str)
>+    return;
c_str will never be null.

>+    c_str = pos+1;
Nit: wrap operators in spaces i.e. pos + 1

>+  nsIAtom* atom;
Nit: initialise this to nsnull.

>+    if (strLength == 0)
>+      return;
It hardly seems worth testing this, as you're checking again straight away.

>+    PRUint32 i = 0;
>+
>+    while (i < strLength)
>+      {
>+        PRInt32 i = str.Find(what, i);
Redeclaration of i? (I know, problems with signed and unsigned comparisons.)
Also, strange indentation of { which reminds me that mailnews policy is always to put {s on their own line, which I might have overlooked before.

>+NS_NewMsgNotificationCallbacksAggregation(nsIInterfaceRequestor  *callbacks,
>+                                         nsILoadGroup           *loadGroup,
>+                                         nsIInterfaceRequestor **result)
[This doesn't line up, but you were going to rename it anyway, right?]

Comment 148

[[e-mail]]

Comment on attachment 426114 [details] [diff] [review]
Fixed conversions
[Checkin: Comment 148]

Pushed changeset f8515b71dd94 to comm-central.

r+sr=bienvenu over IRC.

Comment 149

[[e-mail]]

Comment on attachment 426258 [details] [diff] [review]
nsMsgUtils addition and 'fix' 

>+  if (!atomService)
>+    return nsnull;
>+
>+  nsIAtom* atom;
>+  atomService->GetAtomUTF8(aString, &atom);
>+  return atom;
And with the nsIAtom* atom = nsnull; you could then write this as
if (atomService)
  atomService->GetAtomUTF8(aString, &atom);

Comment 150

[[e-mail]]

(In reply to comment #141)
> Hmm, the third one was in fact (NS_)MsgNewAtom, right?
> 
> Also, the #ifdef MOZILLA_INTERNAL_API block needs some extra #define statements
> #define MsgNewAtom
>         NS_NewAtom
I tried this and it doesn't work - do_GetAtom calls NS_NewAtom, which would require us to #include nsMsgUtils before nsIAtom, which isn't feasible. So, at least in this case, I don't see an alternative to calling it NS_NewAtom.

Comment 151

[Jan Horak [:jhorak]]

I'm thinking about:
-      nsCOMPtr<nsIMsgDBHdr> message = do_QueryElementAt(aMessages, i, &rv);
+      nsCOMPtr<nsIMsgDBHdr> message;
+      rv = aMessages->QueryElementAt(i, NS_GET_IID(nsIMsgDBHdr), getter_AddRefs(message));

I understand that do_QueryElementAt should not be changed into aMessages->QueryElementAt because nsI[Mutable]Array works with do_QueryElementAt just fine. However I'm asking myself how to handle it. If I don't change it I can't be able to test if frozen linkage compiles. If I change it you won't commit my patches to trunk and working on two version is not much comfortable for me.

I would like to process more systematically, patch by patch, not to pollute you by review request for more than five patches. I'm going to open depend bug for each module. This bug has a lot of comments and idea brought by IIRC Mark to divide this into more bugs wasn't bad at all :).

Comment 152

[[e-mail]]

(In reply to comment #151)
> I understand that do_QueryElementAt should not be changed into
> aMessages->QueryElementAt because nsI[Mutable]Array works with
> do_QueryElementAt just fine. However I'm asking myself how to handle it. If I
> don't change it I can't be able to test if frozen linkage compiles. If I change
> it you won't commit my patches to trunk and working on two version is not much
> comfortable for me.
It turns out that we can create an overload for our use of do_QueryElementAt on nsISupportsArray because the internal API actually defines it for nsICollection (and nsIArray, but that doesn't concern us here.)

> I would like to process more systematically, patch by patch, not to pollute you
> by review request for more than five patches. I'm going to open depend bug for
> each module.
Actually I found it helpful to compare all the modules at once.

Comment 153

[[e-mail]]

(In reply to comment #150)
> (In reply to comment #141)
> > Hmm, the third one was in fact (NS_)MsgNewAtom, right?
> > 
> > Also, the #ifdef MOZILLA_INTERNAL_API block needs some extra #define statements
> > #define MsgNewAtom
> >         NS_NewAtom
> I tried this and it doesn't work - do_GetAtom calls NS_NewAtom, which would
> require us to #include nsMsgUtils before nsIAtom, which isn't feasible. So, at
> least in this case, I don't see an alternative to calling it NS_NewAtom.
Well, I guess we could stop using do_GetAtom, or reimplement that too...

Comment 154

[Jan Horak [:jhorak]]

(In reply to comment #153)
> (In reply to comment #150)
> > (In reply to comment #141)
> > > Hmm, the third one was in fact (NS_)MsgNewAtom, right?
> > > 
> > > Also, the #ifdef MOZILLA_INTERNAL_API block needs some extra #define statements
> > > #define MsgNewAtom
> > >         NS_NewAtom
> > I tried this and it doesn't work - do_GetAtom calls NS_NewAtom, which would
> > require us to #include nsMsgUtils before nsIAtom, which isn't feasible. So, at
> > least in this case, I don't see an alternative to calling it NS_NewAtom.
> Well, I guess we could stop using do_GetAtom, or reimplement that too...
Why not just define NS_NewAtom for frozen linkage in nsMsgUtils.h?

Comment 155

[[e-mail]]

I'm worried that NS_NewAtom is defined as NS_COM linkage, and I'm not sure that would be correct for a version defined in nsMsgUtils.

Comment 156

[[e-mail]]

Comment on attachment 426258 [details] [diff] [review]
nsMsgUtils addition and 'fix' 

>+NS_MSG_BASE nsresult
>+NS_NewMsgInterfaceRequestorAggregation(nsIInterfaceRequestor *aFirst,
>+                                       nsIInterfaceRequestor *aSecond,
>+                                       nsIInterfaceRequestor **aResult);
...
>+inline nsresult
>+NS_NewMsgNotificationCallbacksAggregation(nsIInterfaceRequestor  *callbacks,
>+                                         nsILoadGroup           *loadGroup,
>+                                         nsIInterfaceRequestor **result)
Might be an idea to stick with the aArg-style naming for consistency ;-)

Comment 157

[[e-mail]]

Comment on attachment 426258 [details] [diff] [review]
nsMsgUtils addition and 'fix' 

OK, so it's partly my fault for bitrotting this by checking in attachment 426114 [details] [diff] [review], but I've reviewed it as best as I can.

Comment 158

[Matthew Gertner]

(In reply to comment #155)
> I'm worried that NS_NewAtom is defined as NS_COM linkage, and I'm not sure that
> would be correct for a version defined in nsMsgUtils.

FWIW I did run into exactly this issue on Windows. I ended up changing all the NS_NewAtom an do_GetAtom calls to MsgNewAtom. There aren't *that* many so it's not a huge big deal to rewrite them. There may be a better way I didn't think of, of course.

Comment 159

[[e-mail]]

(In reply to comment #158)
> I ended up changing all the NS_NewAtom an do_GetAtom calls to MsgNewAtom.
> There aren't *that* many so it's not a huge big deal to rewrite them.
> There may be a better way I didn't think of, of course.
Since we're going to have to change the code anyway we could bit the bullet and rewrite do_GetAtom as dont_AddRef(NS_NewAtom(...)) or we could perhaps hide that behind a macro (possibly called MsgGetAtom?) so that people don't accidentally write nsCOMPtr<nsIAtom> theAtom = MsgNewAtom(...); by mistake in future.

Comment 160

[[e-mail]]

Attachment: Remove #ifdef MOZILLA_INTERNAL_API [Checkin: Comment 218]

Since attachment 426114 [details] [diff] [review], some of the easier #ifdef blocks can now be removed.

Comment 161

[Jan Horak [:jhorak]]

Attachment: address book patch v5

Removed ifdefs.

Comment 162

[[e-mail]]

Comment on attachment 427086 [details] [diff] [review]
address book patch v5

>diff -r 70952bffd07c mailnews/addrbook/src/nsAbBoolExprToLDAPFilter.cpp
>--- a/mailnews/addrbook/src/nsAbBoolExprToLDAPFilter.cpp	Fri Feb 12 11:37:58 2010 +0530
>+++ b/mailnews/addrbook/src/nsAbBoolExprToLDAPFilter.cpp	Tue Feb 16 10:54:53 2010 +0100
>@@ -42,6 +42,7 @@
> #include "nsStringGlue.h"
> #include "nsIArray.h"
> #include "nsArrayUtils.h"
>+#include "nsMsgUtils.h"
Do you actually need this?

>-    retCode = NS_GetProxyForObject(NS_PROXY_TO_MAIN_THREAD,
>+    retCode = nsIProxyObjectManager::GetProxyForObject(NS_PROXY_TO_MAIN_THREAD,
My mistake here; NS_GetProxyForObject actually belongs in the "implement MsgXXX version" category. Marking r=me without these two changes in case you want to implement MsgGetProxyForObject in a separate patch.

Comment 163

[Jan Horak [:jhorak]]

Attachment: news patch v4

Removed ifdefs, fixed commented issues.

Comment 164

[[e-mail]]

Comment on attachment 427106 [details] [diff] [review]
news patch v4

>diff -r 70952bffd07c mailnews/news/src/nsNNTPNewsgroupList.cpp
>--- a/mailnews/news/src/nsNNTPNewsgroupList.cpp	Fri Feb 12 11:37:58 2010 +0530
>+++ b/mailnews/news/src/nsNNTPNewsgroupList.cpp	Tue Feb 16 15:48:08 2010 +0100
>@@ -52,7 +52,6 @@
> #include "nsIDBFolderInfo.h"
> #include "nsINewsDatabase.h"
> #include "nsIMsgStatusFeedback.h"
>-#include "nsCOMPtr.h"
Why this change?

>-         slashpos != kNotFound)
>+         slashpos != -1)
Nit: we don't need to change kNotFound (occurs several times in the diff)

>   if (m_newsAction == nsINntpUrl::ActionSearch) {
>-    nsUnescape(group);
>+    nsCString unescapedGroup;
>+    MsgUnescapeString(nsDependentCString(group), 0, unescapedGroup);
>+    NS_Free(group);
>+    group = ToNewCString(unescapedGroup);
>   }
>   else if (strchr(group, '@') || strstr(group,"%40")) {
>-    message_id = nsUnescape(group);
>+    nsCString unescapedGroup;
>+    MsgUnescapeString(nsDependentCString(group), 0, unescapedGroup);
>+    NS_Free(group);
>+    message_id = ToNewCString(unescapedGroup);
>     group = 0;
>   }
>   else if (!*group) {
This code is confusing - I'd suggest rewriting the function, but I can't figure out what it does :-(

>+    nsCString value;
>+    value = Substring(header, colon + 1);
[You could construct value(Substring(...)); instead if you prefer.]

>-        if (header.Find("From",PR_TRUE) == 0) {
>+        if (header.Find("From", CaseInsensitiveCompare) == 0) {
Space after comma :-)

> 
>-  NS_Free(escapedMessageID);
> 
Nit: kill one of the blank lines too please.

>+#ifdef MOZILLA_INTERNAL_API
>     rv = AddNewsgroup(nsDependentCSubstring(line, s), nsDependentCString(setStr), getter_AddRefs(child));
>+#else
>+    rv = AddNewsgroup(nsDependentCSubstring(line, s - line), nsDependentCString(setStr), getter_AddRefs(child));
>+#endif
Fix this using Substring instead of nsDependentCSubstring (did I accidentally omit that from my decoder ring?)

>+  nsCString res;
>+  rv = hdr->GetMessageId(getter_Copies(res));
>+  result.Assign(res);
Name this id instead of res perhaps?

>+    PRInt32 keyEndSeparator = FindCharInSet(uriStr, "?&", keySeparator);
I think we ought to rename this to MsgFindCharInSet to avoid confusion.

>+      keyStr = Substring(uriStr, keySeparator+1, keyEndSeparator-(keySeparator+1));
Nit: spaces around +

>+  if (groupName.Find(mSearchValue, CaseInsensitiveCompare) != 1)
Nit: could use kNotFound

>-    char *escapedMessageID = nsEscape(messageID.get(), url_Path);
>-    if (!escapedMessageID)
>-      return NS_ERROR_OUT_OF_MEMORY;
>+    nsCString escapedMessageID;
>+    MsgEscapeString(messageID, nsINetUtil::ESCAPE_ALL, escapedMessageID);
Shouldn't this be ESCAPE_URL_PATH?

>-        nsCAutoString currentGroup;
Nit: wasn't necessary to move this, you didn't even change it to a constructor.

>         // theRest is "host/group"
>-        theRest.Left(currentHost, slashpos);
>+        currentHost = StringHead(theRest, slashpos);
> 
>         // from "host/group", put "group" into currentGroup;
>-        theRest.Right(currentGroup, theRest.Length() - currentHost.Length() - 1);
>+        nsCString currentGroup;
>+        currentGroup = Substring(theRest, currentHost.Length() + 1);
[Is it me or is currentHost.Length() the same as slashpos?]

>-      aURIString.FindChar('?') == kNotFound ? aURIString += "?" : aURIString += "&";
>+      aURIString += aURIString.FindChar('?') == -1 ? '?' : '&';
[For some reason the old code actually compiles!]

Comment 165

[Mark Banner (:standard8)]

Comment on attachment 426114 [details] [diff] [review]
Fixed conversions
[Checkin: Comment 148]

Well you already seem to have got review over irc for this, but sr=Standard8 anyway.

Comment 166

[Jan Horak [:jhorak]]

Attachment: address book patch v6

Removed nsMsgUtils.h from nsAbBoolExprToLDAPFilter.cpp and used MsgGetProxyForObject. No other changes.

Comment 167

[[e-mail]]

(In reply to comment #166)
> Removed nsMsgUtils.h from nsAbBoolExprToLDAPFilter.cpp and used
> MsgGetProxyForObject. No other changes.
Unfortunately nobody's written MsgGetProxyForObject yet ;-)

Comment 168

[Jan Horak [:jhorak]]

Attachment: nsMsgUtils additions

I did :). I'm sending patch now.

Comment 169

[Jan Horak [:jhorak]]

Attachment: news patch v5

Thanks for quick comment.
>>   if (m_newsAction == nsINntpUrl::ActionSearch) {
>>-    nsUnescape(group);
>>+    nsCString unescapedGroup;
>>+    MsgUnescapeString(nsDependentCString(group), 0, unescapedGroup);
>>+    NS_Free(group);
>>+    group = ToNewCString(unescapedGroup);
>>   }
>>   else if (strchr(group, '@') || strstr(group,"%40")) {
>>-    message_id = nsUnescape(group);
>>+    nsCString unescapedGroup;
>>+    MsgUnescapeString(nsDependentCString(group), 0, unescapedGroup);
>>+    NS_Free(group);
>>+    message_id = ToNewCString(unescapedGroup);
>>     group = 0;
>>   }
>>   else if (!*group) {
>This code is confusing - I'd suggest rewriting the function, but I can't figure
>out what it does :-(

Not sure of my change either.

>>-        if (header.Find("From",PR_TRUE) == 0) {
>>+        if (header.Find("From", CaseInsensitiveCompare) == 0) {
>Space after comma :-)
Do I understand correctly you prefer this? 
+        if (header.Find("From",CaseInsensitiveCompare) == 0) {

Comment 170

[[e-mail]]

(In reply to comment #169)
> Do I understand correctly you prefer this? 
> +        if (header.Find("From",CaseInsensitiveCompare) == 0) {
No, the :-) meant that I liked the change.

Comment 171

[[e-mail]]

Comment on attachment 427336 [details] [diff] [review]
news patch v5

>-    PRInt32 keyEndSeparator = uriStr.FindCharInSet("?&",
>-                                                   keySeparator);
> 
>-    uriStr.Left(folderURI, keySeparator);
>-        folderURI.Cut(4, 8);    // cut out the -message part of news-message:
Nit: blank line crept in here.

>+    PRInt32 keyEndSeparator = MsgFindCharInSet(uriStr, "?&", keySeparator);
This needs the util patch, right?

>+      keyStr = Substring(uriStr, keySeparator+1, keyEndSeparator-(keySeparator+1));
While you're here, can you add spaces around these - and + operators?

-        uriStr.Right(keyStr, uriStr.Length() - (keySeparator + 1));
+      keyStr = Substring(uriStr, keySeparator - 1);
I must have missed this last time, but I think this should be +, not -

>-      newsUrl.Mid(groupName, groupPos + kNewsURIGroupQueryLen,
>-                  keyPos - groupPos - kNewsURIGroupQueryLen);
>-      newsUrl.Mid(keyStr, keyPos + kNewsURIKeyQueryLen,
>-                  newsUrl.Length() - keyPos - kNewsURIKeyQueryLen);
>-
>+      groupName = Substring(groupName, groupPos + kNewsURIGroupQueryLen,
>+                            keyPos - groupPos - kNewsURIGroupQueryLen);
Did you mean Substring(newsUrl, ...)? [Yes, I missed this too...]

>+      keyStr = Substring(newsUrl, keyPos + kNewsURIKeyQueryLen,
>+                         newsUrl.Length() - keyPos - kNewsURIKeyQueryLen);
[Hmm, this is equivalent to the rest of the string, isn't it?)

>-        nsCAutoString currentGroup;
>+        nsCString currentGroup;
Nit: don't need to change this

Comment 172

[David :Bienvenu]

Comment on attachment 426927 [details] [diff] [review]
Remove #ifdef MOZILLA_INTERNAL_API
[Checkin: Comment 218]

nice to see those #ifdef's go away!

Comment 173

[[e-mail]]

Comment on attachment 427334 [details] [diff] [review]
nsMsgUtils additions

>-    nsCAutoString native;
>-    nsAutoString roundTripped;
>-    NS_CopyUnicodeToNative(str, native);
>-    NS_CopyNativeToUnicode(native, roundTripped);
>-    return str.Equals(roundTripped);
>+    return str.Equals(NS_ConvertASCIItoUTF16(NS_LossyConvertUTF16toASCII(str)));
This isn't the right fix. Can we leave this until we find the right fix?

>+NS_MSG_BASE nsIAtom* NS_NewAtom(const char* aString)
I don't think we can do this. It probably doesn't matter on Linux, but on Windows there's a difference between NS_COM and NS_MSG_BASE :-(

>+  if (!atomService)
>+    return nsnull;
>+
>+  nsIAtom* atom = nsnull;
>+  atomService->GetAtomUTF8(aString, &atom);
[Or possibly use if (atomService) atomService->GetAtomUTF8(aString, &atom);]

>+NS_MSG_BASE PRInt32 MsgFindCharInSet(const nsCString &aString,
>+                                     const char* aChars, PRUint32 aOffset = 0);
>+NS_MSG_BASE PRInt32 MsgFindCharInSet(const nsString &aString,
>+                                     const char* aChars, PRUint32 aOffset = 0);
Do you think it's worth putting this in the non-internal-api block, and define it in terms of FindCharInSet when using the internal API?

> #define MsgEscapeHTML2(buffer, len) \
>         nsEscapeHTML2(buffer, len)
>+#define MsgReplaceSubstring(str, what, replacement) \
>+        str.ReplaceSubstring(what, replacement)
Nit: use (str). to get the right precedence.

>+// Allows us to use IsUTF8
>+#define MsgIsUTF8 IsUTF8
>+#define MsgNewInterfaceRequestorAggregation NS_NewInterfaceRequestorAggregation
>+#define MsgNewNotificationCallbacksAggregation NS_NewNotificationCallbacksAggregation
> 
>+#define MsgGetProxyForObject NS_GetProxyForObject
Please write these all on two lines for consistency. Thanks.
Also, do you think it's worth listing the parameters e.g.
#define MsgIsUTF8(str) \
        IsUTF8(str)

>+class NS_COM MsgQueryElementAt : public nsCOMPtr_helper
NS_MSG_BASE

>+      MsgQueryElementAt( nsISupportsArray* aCollection, PRUint32 aIndex, nsresult* aErrorPtr )
>+          : mCollection(aCollection),
Nit: please rename these variables Array, not Collection. Thanks.

>+      //virtual nsresult NS_FASTCALL operator()( const nsIID& aIID, void** ) const;
I think I'd prefer this version (i.e. put the code in the .cpp) ;-)

Comment 174

[Jan Horak [:jhorak]]

Attachment: news patch v5

Uh, sorry for +/-1 mistakes. 

>>+    PRInt32 keyEndSeparator = MsgFindCharInSet(uriStr, "?&", keySeparator);
>This needs the util patch, right?
Yes, it does.

Comment 175

[[e-mail]]

Comment on attachment 427558 [details] [diff] [review]
news patch v5

>   {
>-    PRInt32 keyEndSeparator = uriStr.FindCharInSet("?&",
>-                                                   keySeparator);
> 
Blank line is still there. I think you deleted a different one by mistake.

Comment 176

[Jan Horak [:jhorak]]

Attachment: news patch v6 [Checkin: Comment 234]

New line fixed hopefully.

Comment 177

[Jan Horak [:jhorak]]

About (Msg)FindCharInSet: I'd rather keep it as it is in my patch. FindCharInSet has three parameters, the last is offset and it is optional, so we can't use:
#define MsgFindCharInSet(string, chars)
        (string).FindCharInSet(chars)
we can use:
#define MsgFindCharInSet(string, chars, offset)
        (string).FindCharInSet(chars, offset)
and add zero offset where required. I'm not sure what is better.

Comment 178

[Jan Horak [:jhorak]]

Attachment: extensions patch v4

Fixed issue by comment #68 :)

Comment 179

[Jan Horak [:jhorak]]

Attachment: local patch v3

Removed ifdefs, do_GetAtom changed to Msg_do_GetAtom (will be in nsMsgUtils patch). Removed *Native calls, revert CaseInsensitiveCompare back to nsCaseInsensitiveCStringComparator(), PR_TRUE => CaseInsensitiveCompare where required.

Comment 180

[[e-mail]]

(In reply to comment #179)
> do_GetAtom changed to Msg_do_GetAtom
Would you mind calling it MsgGetAtom?

Comment 181

[[e-mail]]

(In reply to comment #177)
> FindCharInSet has three parameters, the last is offset and it is optional, so
> we can't use:
> #define MsgFindCharInSet(string, chars)
>         (string).FindCharInSet(chars)
> we can use:
> #define MsgFindCharInSet(string, chars, offset)
>         (string).FindCharInSet(chars, offset)
> and add zero offset where required. I'm not sure what is better.
bienvenu suggested adding an #ifdef to the body of MsgFindCharInSet to call FindCharInSet when using the internal API.

Comment 182

[[e-mail]]

Comment on attachment 427736 [details] [diff] [review]
news patch v6
[Checkin: Comment 234]

>-    AppendUTF16toUTF8(groupName, uri);
>+    uri.Append(NS_ConvertUTF16toUTF8(groupName));
Oh, one last nit: I #defined this earlier, so you don't need to change it.

Comment 183

[[e-mail]]

Comment on attachment 427761 [details] [diff] [review]
extensions patch v4

>     nsDependentCString word (aWord, wordLength); // CHEAP, no allocation occurs here...
[For a moment I thought we didn't use this, but I realise I was wrong...]

>-    text.ReplaceSubstring(NS_LITERAL_STRING("<iframe"),
>-                          NS_LITERAL_STRING("<div"));
>-    text.ReplaceSubstring(NS_LITERAL_STRING("/iframe>"),
>-                          NS_LITERAL_STRING("/div>"));
>+    MsgReplaceSubstring(text, NS_LITERAL_STRING("<iframe"), NS_LITERAL_STRING("<div"));
>+    MsgReplaceSubstring(text, NS_LITERAL_STRING("/iframe>"), NS_LITERAL_STRING("/div>"));
Nit: please keep these on two lines, rather than creating lines of over 80 characters.

>-          tokenStrings.AppendElement(UTF8ToNewUnicode(nsDependentCString(
>-              tokens[ta.mTokenIndex].mWord)));
>+          tokenStrings.AppendElement(ToNewUnicode(NS_ConvertUTF8toUTF16(tokens[ta.mTokenIndex].mWord)));
Again, the second line can be left alone.

>-    NS_NewISupportsArray(getter_AddRefs(mViewSearchTerms));
>+    mViewSearchTerms = do_CreateInstance(NS_SUPPORTSARRAY_CONTRACTID);

>-    nsCOMPtr<nsISupportsArray> mailViewList;
>-    NS_NewISupportsArray(getter_AddRefs(mailViewList));
>+    nsCOMPtr<nsISupportsArray> mailViewList = do_CreateInstance(NS_SUPPORTSARRAY_CONTRACTID);
Nit: I added a #define for these too, so you don't need to change them.

>+                if (!mailTo.IsEmpty() && !identEmail.IsEmpty() && mailTo.Find(identEmail, CaseInsensitiveCompare) != -1)

>+                  if (!mailCC.IsEmpty() && !identEmail.IsEmpty() && mailCC.Find(identEmail, CaseInsensitiveCompare) != -1)
[kNotFound perhaps?]

>diff -r 70952bffd07c mailnews/extensions/smime/src/nsSMimeJSHelper.cpp
>--- a/mailnews/extensions/smime/src/nsSMimeJSHelper.cpp	Fri Feb 12 11:37:58 2010 +0530
>+++ b/mailnews/extensions/smime/src/nsSMimeJSHelper.cpp	Fri Feb 19 15:31:18 2010 +0100
>@@ -39,13 +39,13 @@
> #include "nsSMimeJSHelper.h"
> #include "nsCOMPtr.h"
> #include "nsMemory.h"
>-#include "nsReadableUtils.h"
>-#include "nsLiteralString.h"
>-#include "nsString.h"
>+#include "nsStringGlue.h"
> #include "nsIMsgHeaderParser.h"
> #include "nsIX509CertDB.h"
> #include "nsIX509CertValidity.h"
> #include "nsIServiceManager.h"
>+#include "nsServiceManagerUtils.h"
>+#include "nsCRTGlue.h"
This file didn't compile, I think you added the wrong include somewhere.

Comment 184

[[e-mail]]

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

>-  nsCAutoString newNameDirStr = newDiskName;  //save of dir name before appending .msf
>-  rv = oldPathFile->MoveToNative(nsnull, newDiskName);
>-  if (NS_FAILED(rv))
>+  nsAutoString newNameDirStr = safeName;  //save of dir name before appending .msf
>+  rv = oldPathFile->MoveTo(nsnull, safeName);
>+  if (NS_SUCCEEDED(rv))
>+  {
>+    safeName.Append(NS_LITERAL_STRING(SUMMARY_SUFFIX));
>+    oldSummaryFile->MoveTo(nsnull, safeName);
>+  }
>+  else
>   {
>     ThrowAlertMsg("folderRenameFailed", msgWindow);
>     return rv;
>   }
> 
>-  newDiskName += SUMMARY_SUFFIX;
>-  oldSummaryFile->MoveToNative(nsnull, newDiskName);
You seem to have moved the above two lines for some reason, which looks wrong.

>-  if (count > 0)
>+  if (NS_SUCCEEDED(rv) && count > 0)
I can't see why this was changed.

>-    newNameDirStr += ".sbd";
>+    newNameDirStr.Append(NS_LITERAL_STRING(".sbd"));
Use AppendLiteral perhaps?

>-    (protocolType.LowerCaseEqualsLiteral("imap") ||
>-     protocolType.LowerCaseEqualsLiteral("news")));
Use MsgLowerCaseEqualsLiteral(protocolType, "imap") etc.

>+

>-
A couple of miscellaneous line changes crept in to the patch.

>-    PRInt32 keyEndSeparator = FindCharInSet(uriStr, "?&", keySeparator); 
>+    PRInt32 keyEndSeparator = MsgFindCharInSet(uriStr, "?&", keySeparator);
[Ideally this would land as part of the rename in nsMsgUtils]

>-        nsCOMPtr <nsIAtom> deferAtom = getter_AddRefs(NS_NewAtom("isDeferred"));
>-        nsCOMPtr <nsIAtom> canFileAtom = getter_AddRefs(NS_NewAtom("CanFileMessages"));
>+        nsCOMPtr <nsIAtom> deferAtom = getter_AddRefs(MsgNewAtom("isDeferred"));
>+        nsCOMPtr <nsIAtom> canFileAtom = getter_AddRefs(MsgNewAtom("CanFileMessages"));
The nicer way to write this is
nsCOMPtr<nsIAtom> deferAtom = MsgGetAtom("isDeferred");

Comment 185

[Jan Horak [:jhorak]]

I don't see reason why to revert 
>-    NS_NewISupportsArray(getter_AddRefs(mViewSearchTerms));
>+    mViewSearchTerms = do_CreateInstance(NS_SUPPORTSARRAY_CONTRACTID);
It's in frozen linkage guide, keeping it brings nsMsgUtils.h pollution and I have to even add includes to makefiles due to it.

>>-    (protocolType.LowerCaseEqualsLiteral("imap") ||
>>-     protocolType.LowerCaseEqualsLiteral("news")));
>Use MsgLowerCaseEqualsLiteral(protocolType, "imap") etc.
Same here. Do we make code for frozen linkage or for internal api? Why can't I use :
protocolType.Equals("imap", CaseInsensitiveCompare) 
just what frozen linkage requires (and what's in guide)?
Working on these patches is really time consuming. Reverting such changes is a lot of monkey work, not just simple replace. I've been reverting a lot of things. Can't we just focus on real problems not to this nit changes (for example 80 characters per line which is not followed in code anyway)?

Comment 186

[[e-mail]]

(In reply to comment #185)
> Do we make code for frozen linkage or for internal api?
Well the one point that was clear from my discussions with other developers was that we wanted to avoid scattering #ifdef throughout the code. And you have to consider that all current development is against the internal api, in one of three supported build configurations, to which we're adding one or two more. I also would prefer to lobby to get additional methods added to the external api, allowing us to remove these Msg* workarounds; this would be easier if we used the special names, rather than limiting ourselves to the bare common api. But it's good that you argue your case, since you're the one actually doing all the hard work, and you will find that convincing arguments will change my mind. So in this case, you explain that the NS_NewISupportsArray issue is a problem, and I don't mind if you don't change it, but I'm as yet unconvinced about literals.

Comment 187

[Jan Horak [:jhorak]]

I've started with replacing by MsgLowerCaseEqualsLiteral and I've come to this solution for frozen linkage:
inline PRBool MsgEquals(const nsAString &str, const char *literal)
{
  return str.Equals(NS_ConvertUTF8toUTF16(literal), CaseInsensitiveCompare);
}

inline PRBool MsgEquals(const nsAString &str, const nsAString &literal)
{
  return str.Equals(literal, CaseInsensitiveCompare);
}

inline PRBool MsgEquals(const nsACString &str, const char *literal)
{
  return str.Equals(literal, CaseInsensitiveCompare);
}

/// Equivalent of LowerCaseEqualsLiteral(literal)
#define MsgLowerCaseEqualsLiteral(str, literal) \
        MsgEquals(str, literal)

The problem is with literal parameter. The nsString.Equals() in frozen linkage does not accept char[], only PRUnichar[]. Using NS_LITERAL_STRING() is on the other hand not accepted in internal API.

Comment 188

[Jan Horak [:jhorak]]

Attachment: local patch v4

Tried to fix mentioned problems.
>[Ideally this would land as part of the rename in nsMsgUtils]
Not sure if I understand correctly. Do you want to add nsMsgUtils changes to this patch? My nsMsgUtils is quite changed from version in trunk. I would like to keep it off this patch.

Comment 189

[[e-mail]]

(In reply to comment #187)
> inline PRBool MsgEquals(const nsAString &str, const char *literal)
> {
>   return str.Equals(NS_ConvertUTF8toUTF16(literal), CaseInsensitiveCompare);
> }
Have I confused you again? Sorry. LowerCaseEqualsLiteral already works here.

Comment 190

[Jan Horak [:jhorak]]

Attachment: extensions patch v5

>diff -r 70952bffd07c mailnews/extensions/smime/src/nsSMimeJSHelper.cpp
>--- a/mailnews/extensions/smime/src/nsSMimeJSHelper.cpp	Fri Feb 12 11:37:58 2010 +0530
>+++ b/mailnews/extensions/smime/src/nsSMimeJSHelper.cpp	Fri Feb 19 15:31:18 2010 +0100
>This file didn't compile, I think you added the wrong include somewhere.
Really? I can compile with both frozen and internal api. Could you send me the error message, please?

Comment 191

[Jan Horak [:jhorak]]

(In reply to comment #189)
> (In reply to comment #187)
> > inline PRBool MsgEquals(const nsAString &str, const char *literal)
> > {
> >   return str.Equals(NS_ConvertUTF8toUTF16(literal), CaseInsensitiveCompare);
> > }
> Have I confused you again? Sorry. LowerCaseEqualsLiteral already works here.
Um, I'm talking about frozen linkage. There's no LowerCaseEqualsLiteral method in ns(C)String, is it?

Comment 192

[[e-mail]]

/xpcom/glue/nsStringAPI.cpp
* line 310 -- nsAString::LowerCaseEqualsLiteral(const char *aASCIIString) const

Comment 193

[Jan Horak [:jhorak]]

Ah, sorry. I was confused by:
nsMsgI18N.cpp:273: error: no matching function for call to 'nsAutoString::LowerCaseEqualsLiteral(const nsString&)'
There's no LowerCaseEqualsLiteral(const nsString&) in frozen linkage, that is the problem.

Comment 194

[Jan Horak [:jhorak]]

Attachment: compose patch v4

Bringing back compose path without ifdefs.

Comment 195

[[e-mail]]

Comment on attachment 428437 [details] [diff] [review]
extensions patch v5

I fixed my compile error, thanks for asking.

>+    nsCOMPtr<nsISupportsArray> mailViewList = do_CreateInstance(
>+        NS_SUPPORTSARRAY_CONTRACTID);;
Nit;;

Comment 196

[[e-mail]]

Comment on attachment 428436 [details] [diff] [review]
local patch v4

>-  if (name.LowerCaseEqualsLiteral("msgfilterrules.dat") ||
I think you may have made these 12 changes, and forgotten to change them back when you realised that LowerCaseEqualsLiteral exists for nsAString ;-)

>diff -r 70952bffd07c mailnews/local/src/nsNoIncomingServer.cpp
>--- a/mailnews/local/src/nsNoIncomingServer.cpp	Fri Feb 12 11:37:58 2010 +0530
>+++ b/mailnews/local/src/nsNoIncomingServer.cpp	Tue Feb 23 15:13:06 2010 +0100
>@@ -49,6 +49,7 @@
> #include "nsIMsgAccountManager.h"
> #include "nsIPop3IncomingServer.h"
> #include "nsServiceManagerUtils.h"
>+#include "nsMsgUtils.h"
This seems to be unnecessary.

>diff -r 70952bffd07c mailnews/local/src/nsPop3Service.cpp
>--- a/mailnews/local/src/nsPop3Service.cpp	Fri Feb 12 11:37:58 2010 +0530
>+++ b/mailnews/local/src/nsPop3Service.cpp	Tue Feb 23 15:13:06 2010 +0100
>@@ -64,7 +64,6 @@
> #include "nsInt64.h"
> #include "nsIPrompt.h"
> #include "nsLocalStrings.h"
>-#include "nsINetUtil.h"
Also unnecessary? r=me assuming we can agree on whether they are or not.

Comment 197

[Jan Horak [:jhorak]]

About nsStringAPI.cpp, there is no nsCString::LowerCaseEqualsLiteral in frozen linkage:
nsMessenger.cpp:781: error: ‘const class nsACString’ has no member named ‘LowerCaseEqualsLiteral’
That's why I had to add code in comment #187. How to solve this?

Comment 198

[Jan Horak [:jhorak]]

Sorry, ignore comment #197. I've got pretty mess in my nsMsgUtils.h now.

Comment 199

[Jan Horak [:jhorak]]

(In reply to comment #196)
> (From update of attachment 428436 [details] [diff] [review])
> >-  if (name.LowerCaseEqualsLiteral("msgfilterrules.dat") ||
> I think you may have made these 12 changes, and forgotten to change them back
> when you realised that LowerCaseEqualsLiteral exists for nsAString ;-)
> 
> >diff -r 70952bffd07c mailnews/local/src/nsNoIncomingServer.cpp
> >--- a/mailnews/local/src/nsNoIncomingServer.cpp	Fri Feb 12 11:37:58 2010 +0530
> >+++ b/mailnews/local/src/nsNoIncomingServer.cpp	Tue Feb 23 15:13:06 2010 +0100
> >@@ -49,6 +49,7 @@
> > #include "nsIMsgAccountManager.h"
> > #include "nsIPop3IncomingServer.h"
> > #include "nsServiceManagerUtils.h"
> >+#include "nsMsgUtils.h"
> This seems to be unnecessary.
Under frozen linkage, there's undefined reference to nsQueryElementAt:
nsNoIncomingServer.cpp:(.text._ZN16nsQueryElementAtC2EP13nsICollectionjPj[_ZN16nsQueryElementAtC5EP13nsICollectionjPj]+0x22): undefined reference to `vtable for nsQueryElementAt'
This is solved by macro in nsMsgUtils.h


> >diff -r 70952bffd07c mailnews/local/src/nsPop3Service.cpp
> >--- a/mailnews/local/src/nsPop3Service.cpp	Fri Feb 12 11:37:58 2010 +0530
> >+++ b/mailnews/local/src/nsPop3Service.cpp	Tue Feb 23 15:13:06 2010 +0100
> >@@ -64,7 +64,6 @@
> > #include "nsInt64.h"
> > #include "nsIPrompt.h"
> > #include "nsLocalStrings.h"
> >-#include "nsINetUtil.h"
> Also unnecessary? r=me assuming we can agree on whether they are or not.
Agree

Comment 200

[Jan Horak [:jhorak]]

Attachment: compose patch v5

Fixed MsgLowerCaseEqual/LowerCaseEqual, hope I got it now :).

Comment 201

[Jan Horak [:jhorak]]

Attachment: local patch v5

Fixed MsgLowerCaseEqual/LowerCaseEqual too.

Comment 202

[Jan Horak [:jhorak]]

Attachment: mapi patch v2

Removed unnecessary changes, keeping kNotFound and Last(), replaced Right() by Substring.

Comment 203

[Jan Horak [:jhorak]]

Attachment: mime patch v4 [Checkin: Comment 241]

Removed ifdefs and introducing MsgLowerCaseEqualsLiteral.

Comment 204

[[e-mail]]

Comment on attachment 428696 [details] [diff] [review]
local patch v5

>-      name.LowerCaseEqualsLiteral("rulesbackup.dat"))
>+      name.LowerCaseEqualsLiteral( "rulesbackup.dat"))
Oops ;-)

Comment 205

[Jan Horak [:jhorak]]

Attachment: imap patch v3

A do_QueryElementAt, Last(), put back, removed ifdefs, using MsgLowerCaseEqualsLiteral and MsgReplaceChar where required, calling MsgGetAtom instead of do_GetAtom.

Comment 206

[Jan Horak [:jhorak]]

Attachment: base/search patch v2

A do_QueryElementAt brought back, removed ifdefs, brought back NS_NewISupportsArray where possible.

Comment 207

[[e-mail]]

Comment on attachment 428698 [details] [diff] [review]
mapi patch v2

>-                  wholeFileName.Right(leafName, wholeFileName.Length() - lastSlash - 1);
>+                  leafName.Assign(Substring(wholeFileName, lastSlash - 1));
lastSlash + 1, no?

Comment 208

[[e-mail]]

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

>-  // I don't know how we're going to report this error if we failed to create the isupports array...
>-#ifdef DEBUG
>-  nsresult rv =
>-#endif
>-    NS_NewISupportsArray(getter_AddRefs(m_filters));
>+  nsresult rv = NS_NewISupportsArray(getter_AddRefs(m_filters));
>   NS_ASSERTION(NS_SUCCEEDED(rv), "Fixme bug 180312: NS_NewISupportsArray() failed");
Please remove this change, as it introduces an unused variable warning in non-debug builds.

>-  m_arbitraryHeaders.SetLength(0);
>+  m_arbitraryHeaders.Truncate();
[It hardly seems worth doing this, given that we just constructed it...]

>     if (utf16StrToMatch.Find(needle, CaseInsensitiveCompare) != -1);
>-#endif
>       result = PR_TRUE;
>     break;
>   case nsMsgSearchOp::DoesntContain:
>-#ifdef MOZILLA_INTERNAL_API
>-    if (!CaseInsensitiveFindInReadable(needle, utf16StrToMatch))
>-#else
>     if (utf16StrToMatch.Find(needle, CaseInsensitiveCompare) == -1);
Not your fault, but these lines end with a ; and they shouldn't. r=me with this fixed. [And we could change -1 to kNotFound too, I guess.]

Comment 209

[[e-mail]]

(In reply to comment #188)
>(In replay to comment #184)
>>-    PRInt32 keyEndSeparator = FindCharInSet(uriStr, "?&", keySeparator); 
>>+    PRInt32 keyEndSeparator = MsgFindCharInSet(uriStr, "?&", keySeparator);
>>[Ideally this would land as part of the rename in nsMsgUtils]
>Not sure if I understand correctly. Do you want to add nsMsgUtils changes to
>this patch? My nsMsgUtils is quite changed from version in trunk. I would like
>to keep it off this patch.
The other way around actually; I'd prefer if this one change was added to the nsMsgUtils changes, in case we need to check the patches in separately.

Comment 210

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 426927 [details] [diff] [review]
Remove #ifdef MOZILLA_INTERNAL_API
[Checkin: Comment 218]

sr=dmose

Comment 211

[Jan Horak [:jhorak]]

Attachment: base/util path v4

- NS_RegisterStaticAtoms still not solved in frozen linkage
- put back NS_NewISupportsArray and do_QueryElementAt where possible
- using MsgGetAtom and MsgNewAtom
- removed ifdefs
- used MsgLowerCaseEqualsLiteral instead of LowerCaseEqualsLiteral
- FindCharInSet renamed to MsgFindCharInSet
- removed RFindCharInSet (not used?)
- ConvertibleToNative still unsolved
- removed IsASCII
- added MsgInterfaceRequestorAgg, MsgNewInterfaceRequestorAggregation, MsgQueryElementAt, do_QueryElementAt helper functions and classes for frozen linkage

Comment 212

[Jan Horak [:jhorak]]

Attachment: base/src patch v3

Bringing back to hopefully fixed base/src.

Comment 213

[[e-mail]]

Comment on attachment 429100 [details] [diff] [review]
base/util path v4

It'll be good to get this finished, it will help me review the other patches.
I only found two actual coding problems, but as noticed I want to get clarification on some of the other issues too.

>+#ifdef MOZILLA_INTERNAL_API //FIXME NS_RegisterStaticAtoms
>     NS_RegisterStaticAtoms(folder_atoms, NS_ARRAY_LENGTH(folder_atoms));
>+#else
>+    NS_WARNING("NS_RegisterStaticAtoms not implemented in frozen linkage");
>+    //FIXME NS_RegisterStaticAtoms(folder_atoms, NS_ARRAY_LENGTH(folder_atoms));
>+#endif
[I need to find out how we want to deal with this e.g. we may want to check in the rest of the patch for now and fix this separately.]

>-    m_rootFolder->NotifyBoolPropertyChanged(NS_NewAtom("isSecure"),
>+    m_rootFolder->NotifyBoolPropertyChanged(MsgNewAtom("isSecure"),
>                                             isSecureOld, isSecureNew);
[This is actually a leak. Fortunately each atom only leaks once!]

>-    rv = NS_GetCurrentThread(getter_AddRefs(mProviderThread));
>-    if (NS_FAILED(rv)) return rv;
>+    nsCOMPtr<nsIThread> thread(do_GetCurrentThread());
Wrong thread, I think - you need to change mProviderThread.

>-  return NS_CopyUnicodeToNative(path, aPathCString);
[I need to find out how we want to deal with this too.]

>-                               (void **)getter_AddRefs(term));
>+                              (void **)getter_AddRefs(term));
[A whitespace-only change that isn't strictly necessary.]

>+#ifndef MOZILLA_INTERNAL_API
We already have a big (well, 160 line) block of external API helpers, maybe just tack them on to the end of that block instead?

>+    pos = 0;
[Could write the PRInt32 pos = 0; here instead of declaring it earlier.]

>+    while ((pos = str.FindChar(*set, pos)) != -1) {
>+      c_str[pos] = replacement;
Should we increment pos here, to avoid testing it again?

>+  char *c_str = str.BeginWriting();
>+  if (!c_str)
>+    return;
This can never be true.

>+  while ((pos = strchr(c_str, needle))) {
Worth reusing c_str perhaps?

>+  if (!atomService)
>+    return nsnull;
Don't need to test it here, it gets checked later anyway.

>+    PRUint32 replacementLength = replacement.Length();
>+    const PRUnichar* replacement_str = replacement.BeginReading();
Could use
const PRUnichar* replacement_str;
PRUint32 replacementLength = replacement.BeginReading(&replacement_str);

>+    while (i < strLength)
I was going to say that this will always be true (except in the case of a zero-length string, in which case the Find will always fail anyway) but then I realised that it was an error; strLength doesn't update as you make the replacements, so in theory you could overlook a replacement if the string gets too long. I also found it confusing that the break in the middle of the loop is the real condition. Perhaps it could all be replaced with this:
while ((i = str.Find(what, i)) != kNotFound)

>+    PRUint32 strLength = str.Length();
A similar argument applies to the other version of MsgReplaceSubstring.

>+    while (i < strLength)
>+      {
[Is this even the correct indentation style?]

>-                                   PRUint32 maxBufferOffset);
>+                                        PRUint32 maxBufferOffset);
[Not strictly necessary.]

> #define MsgLowerCaseEqualsLiteral(str, l) \
>-        str.LowerCaseEqualsLiteral(l)
>+        (str).LowerCaseEqualsLiteral(l)
> #define MsgRFindChar(str, ch, len) \
>-        str.RFindChar(ch, len)
>+        (str).RFindChar(ch, len)
> #define MsgCompressWhitespace(str) \
>-        str.CompressWhitespace()
>+        (str).CompressWhitespace()
Thanks for fixing these.

>+// Allows us to use IsUTF8
>+#define MsgIsUTF8(str) \
>+        IsUTF8(str)
[Not a terribly useful comment.]

>-
> #else
> 
[I'd prefer to keep the blank line before the #else in nsMsgUtils.h]

> /// Equivalent of nsEscapeHTML2(aBuffer, aLen)
> NS_MSG_BASE PRUnichar *MsgEscapeHTML2(const PRUnichar *aBuffer, PRInt32 aLen);
> 
>+// Existing replacement for IsUTF8
>+NS_MSG_BASE PRBool MsgIsUTF8(const nsACString& aString);
>+
>+NS_MSG_BASE nsIAtom* MsgNewAtom(const char* aString);
You might want to add a one-line /// comments for MsgNewAtom and other methods too (compare e.g. MsgEscapeHTML2).

>+
>+
[Only one blank line needed.]

>+NS_MSG_BASE void MsgReplaceChar(nsString& str, const char *set, const PRUnichar replacement);
>+NS_MSG_BASE void MsgReplaceChar(nsCString& str, const char needle, const char replacement);
>+
>+inline already_AddRefed<nsIAtom> MsgGetAtom(const char* aUTF8String)
>+{
>+  return MsgNewAtom(aUTF8String);
>+}
Any chance we can reorder these? MsgReplaceChar looks as if it belongs near MsgReplaceSubstring and MsgGetAtom near MsgNewAtom. Thanks.

Comment 214

[Jan Horak [:jhorak]]

Attachment: base/util path v5

>It'll be good to get this finished, it will help me review the other patches.
>I only found two actual coding problems, but as noticed I want to get
>clarification on some of the other issues too.
Couldn't agree more. Lets focus on base/(utils). Thanks for comments, hope I fixed all. I'm still unsure about NS_RegisterStaticAtoms, MsgNewAtom("isSecure") leak and NS_CopyUnicodeToNative.

Comment 215

[[e-mail]]

Comment on attachment 429511 [details] [diff] [review]
base/util path v5

This is very close to being right, just needs a few tweaks.

>+#ifdef MOZILLA_INTERNAL_API //FIXME NS_RegisterStaticAtoms
>     NS_RegisterStaticAtoms(folder_atoms, NS_ARRAY_LENGTH(folder_atoms));
>+#else
>+    NS_WARNING("NS_RegisterStaticAtoms not implemented in frozen linkage");
>+    //FIXME NS_RegisterStaticAtoms(folder_atoms, NS_ARRAY_LENGTH(folder_atoms));
>+#endif
So I asked and this change is OK if you use NS_ERROR, or you could alternatively omit this part of the patch for now.

>-            nsCOMPtr <nsIAtom> aboutToCompactAtom = do_GetAtom("AboutToCompact");
>+            nsCOMPtr <nsIAtom> aboutToCompactAtom = MsgGetAtom("AboutToCompact");
>             NotifyFolderEvent(aboutToCompactAtom);
[Example of a correct atom usage pattern for comparison with below.]

>-    m_rootFolder->NotifyBoolPropertyChanged(NS_NewAtom("isSecure"),
>+    m_rootFolder->NotifyBoolPropertyChanged(MsgNewAtom("isSecure"),
>                                             isSecureOld, isSecureNew);
[You asked about this leak which isn't your fault. The issue here is that the atom that MsgNewAtom creates never gets destroyed. It's not a major issue because subsequent calls return the same atom, rather than leaking a new one each time. An example of a correct coding pattern can be found above.]

>-
>-    rv = NS_GetCurrentThread(getter_AddRefs(mProviderThread));
>-    if (NS_FAILED(rv)) return rv;
>+    
>+    mProviderThread = do_GetCurrentThread();
I noticed some spaces crept on to the blank line.

> PRInt32
>-FindCharInSet(const nsCString &aString,
>-              const char* aChars, PRUint32 aOffset) 
>+MsgFindCharInSet(const nsCString &aString,
>+                 const char* aChars, PRUint32 aOffset) 
[The space at the end of this line isn't your fault.]

>-  PRInt32 illegalCharacterIndex = FindCharInSet(str, 
>-                                                FILE_PATH_SEPARATOR 
>-                                                FILE_ILLEGAL_CHARACTERS 
>-                                                ILLEGAL_FOLDER_CHARS);
>+  PRInt32 illegalCharacterIndex = MsgFindCharInSet(str,
>+                                                   FILE_PATH_SEPARATOR 
>+                                                   FILE_ILLEGAL_CHARACTERS 
>+                                                   ILLEGAL_FOLDER_CHARS, 0);
[Neither are the trailing spaces on these lines your fault.]

>-  PRInt32 illegalCharacterIndex = FindCharInSet(name,
>-                                                FILE_PATH_SEPARATOR 
>-                                                FILE_ILLEGAL_CHARACTERS 
>-                                                ILLEGAL_FOLDER_CHARS);
>+  PRInt32 illegalCharacterIndex = MsgFindCharInSet(name,
>+                                                   FILE_PATH_SEPARATOR 
>+                                                   FILE_ILLEGAL_CHARACTERS 
>+                                                   ILLEGAL_FOLDER_CHARS, 0);
[Nor are the trailing spaces on these lines your fault.]

>-  return NS_CopyUnicodeToNative(path, aPathCString);
>+  LossyCopyUTF16toASCII(path, aPathCString);
>+  return NS_OK;
This change isn't a good idea, since we might forget about it. I'd rather you either removed this change, or alternatively you could use NS_ERROR as above.

>+NS_MSG_BASE void MsgReplaceChar(nsString& str, const char *set, const PRUnichar replacement)
>+{
>+  PRUnichar *c_str = str.BeginWriting();
>+  if (!set)
>+    return;
[Do we actually need to null-check here? If so, it could be moved first.]

>+    while ((i = str.Find(what, i)) != kNotFound)
>+    {
>+      i = str.Find(what, i);
>+      if (i == -1)
>+        break;
Almost, but not quite, what I mean - you don't need the inner test any more.
Also I noticed that you thoughtfully reindented the second version of MsgReplaceSubstring, and I like that so much I want it here too please!

>+  if (strLength == 0)
>+    return;
Nonexistent variable, so remove this?

>+  while ((i = str.Find(what, i)) != kNotFound)
>+  {
>+    i = str.Find(what, i);
>+    if (i == -1)
>+      break;
[As above.]

>+    str.Replace(i, whatLength, replacement);
I notice we could provide the replacement length since we know what it is.

>+/* Overload function for nsISupportsArray. The do_QueryElementAt accepts 
>+   nsICollection* aCollection only which belongs only to internal API. */
I think this should be in a doxygen-style multiline comment (somewhat like the one for MsgNewNotificationCallbacksAggregation). Also I think your word order could be improved slightly like this: The do_QueryElementAt which belongs to internal API only accepts nsICollection* aCollection.

Comment 216

[Jan Horak [:jhorak]]

Attachment: base/util path v6

Thanks for comments, sorry for wrong changes in MsgReplaceSubstring, I've been too hasty.
>>-  return NS_CopyUnicodeToNative(path, aPathCString);
>>+  LossyCopyUTF16toASCII(path, aPathCString);
>>+  return NS_OK;
>This change isn't a good idea, since we might forget about it. I'd rather you
>either removed this change, or alternatively you could use NS_ERROR as above.
I don't get what's wrong in this change. Internal API implements NS_CopyUnicodeToNative like:
NS_COM nsresult
NS_CopyUnicodeToNative(const nsAString  &input, nsACString &output)
{
    LossyCopyUTF16toASCII(input, output);
    return NS_OK;
}
Or problem is with *Native at all?

Comment 217

[[e-mail]]

(In reply to comment #216)
> I don't get what's wrong in this change. Internal API implements
> NS_CopyUnicodeToNative like:
> NS_COM nsresult
> NS_CopyUnicodeToNative(const nsAString  &input, nsACString &output)
> {
>     LossyCopyUTF16toASCII(input, output);
>     return NS_OK;
> }
Only in the #else of an #if defined(XP_BEOS) || defined(XP_MACOSX) #elif defined(XP_UNIX) #elif defined(XP_WIN) #elif defined(XP_OS2) block.

Comment 218

[[e-mail]]

Comment on attachment 426927 [details] [diff] [review]
Remove #ifdef MOZILLA_INTERNAL_API
[Checkin: Comment 218]

Pushed changeset 2e7ced2b75da to comm-central.

Comment 219

[[e-mail]]

Comment on attachment 429721 [details] [diff] [review]
base/util path v6

I made the mistake of replying to your comment without reading the patch.

>+#ifdef MOZILLA_INTERNAL_API
>   return NS_CopyUnicodeToNative(path, aPathCString);
>+#else
>+  NS_ERROR("NS_CopyUnicodeToNative not implemented in frozen linkage.");
>+#endif
>+  LossyCopyUTF16toASCII(path, aPathCString);
>+  return NS_OK;
I think the #endif is in the wrong place!

>+NS_MSG_BASE void MsgReplaceSubstring(nsACString &str, const char *what, const char *replacement)
>+{
>+  PRUint32 replacementLength = strlen(replacement);
>+  PRUint32 whatLength = strlen(what);
>+  PRUint32 i = 0;
This was PRInt32 in the previous patch, was there a reason you changed it?

Comment 220

[Jan Horak [:jhorak]]

Attachment: base/util path v7 [Checkin: Comment 223]

Fixed silly mistakes.

Comment 221

[[e-mail]]

Comment on attachment 431840 [details] [diff] [review]
base/util path v7
[Checkin: Comment 223]

And a whitespace fix too :-) [Pity there's still one left. Oh well, maybe your superreviewer will find something else.]

Comment 222

[Mark Banner (:standard8)]

Comment on attachment 431840 [details] [diff] [review]
base/util path v7
[Checkin: Comment 223]

There was a little bit of bitrot which I fixed locally, I also had to change FindCharInSet to MsgFindCharInSet in nsLocalUtils.cpp to get it to build, I suspect that was added or changed recently.

sr=Standard8 with that fixed.

Comment 223

[[e-mail]]

Comment on attachment 431840 [details] [diff] [review]
base/util path v7
[Checkin: Comment 223]

Pushed changeset f529e947df1b to comm-central.

Comment 224

[[e-mail]]

Comment on attachment 427318 [details] [diff] [review]
address book patch v6

>diff -r 70952bffd07c mailnews/addrbook/src/nsAbOutlookDirectory.cpp
>--- a/mailnews/addrbook/src/nsAbOutlookDirectory.cpp	Fri Feb 12 11:37:58 2010 +0530
>+++ b/mailnews/addrbook/src/nsAbOutlookDirectory.cpp	Wed Feb 17 14:59:03 2010 +0100
>@@ -971,17 +971,17 @@ NS_IMETHODIMP nsAbOutlookDirectory::Star
>     NS_ENSURE_SUCCESS(retCode, retCode) ;
>     nsCOMPtr<nsIAbDirSearchListener> proxyListener;
> 
>-    retCode = NS_GetProxyForObject(NS_PROXY_TO_MAIN_THREAD,
>-                     NS_GET_IID(nsIAbDirSearchListener),
>-                     static_cast<nsIAbDirSearchListener *>(this),
>-                     NS_PROXY_SYNC | NS_PROXY_ALWAYS,
>-                     getter_AddRefs(proxyListener));
>+    retCode = MsgGetProxyForObject(NS_PROXY_TO_MAIN_THREAD,
>+                                   NS_GET_IID(nsIAbDirSearchListener),
>+                                   static_cast<nsIAbDirSearchListener *>(this),
>+                                   NS_PROXY_SYNC | NS_PROXY_ALWAYS,
>+                                   getter_AddRefs(proxyListener));
>     NS_ENSURE_SUCCESS(retCode, retCode);
> 
>     return DoQuery(this, arguments, proxyListener, -1, 0, &mSearchContext);
> }
> 
This hunk is malformed, and the file needs to include nsMsgUtils.h
r=me with that fixed.

Comment 225

[[e-mail]]

Comment on attachment 428695 [details] [diff] [review]
compose patch v5

I couldn't review the whole patch due to bitrot, but fortunately I found some other issues anyway ;-) I also didn't review AppendToMsgBody, sorry.

>@@ -1031,10 +1031,10 @@ nsMsgAttachmentHandler::LoadDataFromFile
>   if (charsetConversion)
>   {
>     if (NS_FAILED(ConvertToUnicode(m_charset, nsDependentCString(readBuf), sigData)))
>-      CopyASCIItoUTF16(readBuf, sigData);
>+      CopyASCIItoUTF16(nsDependentCString(readBuf), sigData);
>   }
>   else
>-    CopyASCIItoUTF16(readBuf, sigData);
>+    CopyASCIItoUTF16(nsDependentCString(readBuf), sigData);
So, that's three calls to nsDependentCString(readBuf) in quick succession ;-)
(Mind you, this isn't exactly my idea of how to read a file...)

>+        nsCAutoString unescapedUrl(turl);
>+        MsgUnescapeString(turl, 0, unescapedUrl);
MsgUnescapeString already copies, so you can eliminate the explicit copy.

>-    goto done;
Nice work!

>-      if (charset.Equals("x-windows-949",
>+      if (charset.Equals("x-windows-949", 
Nit: trailing space crept in here.

>+                    if (recipientsEmailAddresses.Find(curIdentityEmail2) != -1 ||
>+                        ccListEmailAddresses.Find(curIdentityEmail2) != -1)
Nit: Could use kNotFound

>+                sanitizedSubj.Append(NS_LITERAL_STRING(".eml"));
Nit: could use AppendLiteral

>-          AppendASCIItoUTF16(NS_EscapeURL(myGetter, esc_FileBaseName | esc_Forced, buf),
>-                             mCiteReference);
>+          mCiteReference.Append(MsgEscapeURL(myGetter,
>+                                             nsINetUtil::ESCAPE_URL_FILE_BASENAME | nsINetUtil::ESCAPE_URL_FORCED,
>+                                             buf));
NS_EscapeURL returns its result. Unfortunately MsgEscapeURL actually returns a success code!

>+    PRUint32 pos = sigData.Find(metaCharset, CaseInsensitiveCompare);
Doesn't find return PRInt32?

>+    if (pos != -1)
[Could use kNotFound again.]

>-        sigData.Find("\r-- \r", PR_TRUE) < 0 &&
>-        sigData.Find("\n-- \n", PR_TRUE) < 0 &&
>-        sigData.Find("\n-- \r", PR_TRUE) < 0)
Strange, I thought this was available in the external API.

>-          // when we move to frozen linkage this should be:
>-          // if (plaintextDomains.Find(domain, CaseInsensitiveCompare) >= 0)
[I wonder whether != kNotFound would be better.]

>-              //element.LowerCaseEqualsLiteral("blockquote") || // see below
>+              //MsgLowerCaseEqualsLiteral(element, "blockquote") || // see below
Unnecessary ;-)

>-          if (attributeValue.Find("moz-signature", PR_TRUE) != kNotFound)
>+          if (attributeValue.Find(NS_LITERAL_STRING("moz-signature"), CaseInsensitiveCompare) != kNotFound)
Isn't this unnecessary too?

>+      if (MsgFindCharInSet(selPlain, charsOnlyIf.get()) < 0)
Unfortunately the third parameter isn't optional.

>+
>           }
Nit: unnecessary blank line.

>+#ifdef MOZILLA_INTERNAL_API
>+  // Internal API nsACString does not have Find method.
>   if (fileUrl || nsDependentCString(aMsgURI).Find("&type=application/x-message-display") >= 0)
>+#else
>+  if (fileUrl || aMsgURI.Find("&type=application/x-message-display") >= 0)
>+#endif
I think the best way to work around this is to use PromiseFlatCString. (nsDependentCString was always the wrong thing to use anyway.)

>+                         (MsgLowerCaseEqualsLiteral(fileExt, "rtf") || MsgLowerCaseEqualsLiteral(fileExt, "vcs")))
Nit: would look nicer if wrapped onto two lines.

Comment 226

[[e-mail]]

(In reply to comment #225)
>(From update of attachment 428695 [details] [diff] [review])
>>+      if (MsgFindCharInSet(selPlain, charsOnlyIf.get()) < 0)
>Unfortunately the third parameter isn't optional.
Yes it is; I was thinking of a different function. Sorry about that.

Comment 227

[[e-mail]]

Comment on attachment 428701 [details] [diff] [review]
imap patch v3

>-  AppendUTF8toUTF16(message, fullMessage);
>+  fullMessage.Append(NS_ConvertUTF8toUTF16(message));
[This happens several times throughout this patch, but I have since added some macros to nsMsgUtils.h to do this automatically]

>+  if (!NS_IsAscii(PromiseFlatCString(aName).get())) {
[Can also write !NS_IsAscii(aName.BeginReading(), aName.length())]

>+      PRInt32 index = uri.FindChar('//');           // find scheme
Oops. '//' isn't a char. (Well, not a normal one.)

>+      if (uri.Find(namespacePrefix, index+1) != index+1
Find is weird. I'm not even sure that this works with the external API.

>+        && !(MsgLowerCaseEqualsLiteral(Substring(uri, index + 1, uri.Length() - index - 1), "inbox")))
[Don't need to specify the length of the substring, since we just want the rest of the string.]

>+class AdoptUTF8StringEnumerator : public nsIUTF8StringEnumerator
>+{
>+public:
>+  AdoptUTF8StringEnumerator(const nsTArray<nsCString>* array, PRBool ownsArray) :
>+    mIndex(0), mStrings(array), mOwnsArray(ownsArray)
>+  {}
>+  ~AdoptUTF8StringEnumerator()
>+  {
>+    if (mOwnsArray) {
>+      delete const_cast<nsTArray<nsCString>*>(mStrings);
Given that we only use this once, we can assume ownsArray is always true.
[There may be other optimisations but this one was most obvious.]

>+  NS_WARNING("NS_NewAdoptingUTF8StringEnumerator not implemented in frozen linkage");
[Unnecessary.]

>-      rv = NS_GetProxyForObject(m_sinkEventTarget,
>-                                NS_GET_IID(nsIStreamListener),
>-                                aRealStreamListener,
>-                                NS_PROXY_SYNC | NS_PROXY_ALWAYS,
>-                                getter_AddRefs(m_channelListener));
>+      nsCOMPtr<nsIProxyObjectManager> proxyObjMgr = do_GetService(NS_XPCOMPROXY_CONTRACTID, &rv);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+      rv = proxyObjMgr->GetProxyForObject(m_sinkEventTarget,
Didn't you add MsgGetProxyForObject to nsMsgUtils.h?

>-    nsCAutoString tempProtocolString;
>+    nsCString tempProtocolString;
[Unnecessary.]

>+        PRInt32 mpodFetchPos = uriStr.Find("fetchCompleteMessage=true", keyEndSeparator);
Again, this worries me.

>-                        (mPrintingOperation) ? NS_LITERAL_CSTRING("print") : EmptyCString(), aURL);
Couldn't you fix this simply by replacing the EmptyCString() with NS_LITERAL_CSTRING("")?

>+    MsgEscapeString(nsDependentCString(aSearchUri), nsINetUtil::ESCAPE_XPALPHAS, urlSpec);
Doesn't MsgEscapeString delete the existing contents of urlSpec?

>+      CopyUTF16toMUTF7(PromiseFlatString(newLeafName), utfNewName);
>+      // FIXME wants: CopyUTF16toMUTF7(newLeafName, utfNewName);
In case you are interested, the issue apperas to be that nsMsgI18NConvert(To|From)Unicode calls .get() instead of .BeginReading() which would allow its callers to use abstract strings.

>+        folderName.Assign(folderName);
???

>-  onlineDir = (char *)(!aString.IsEmpty() ? ToNewCString(aString) : nsnull);
Whoa, what on earth was this code doing? It looks like a really bad lossy unicode to ascii conversion.

>     nsAutoString uri;
>-    CopyASCIItoUTF16(uriStr, uri);
...
>-    *name = ToNewCString(fullName);
>+    *name = ToNewCString(NS_LossyConvertUTF16toASCII(fullName));
[I wonder what the point of converting to UTF16 was in the first place...]

>+      PRInt32 partPos = uriStr.Find("part=", keyEndSeparator);
This still makes me suspicious...

Comment 228

[[e-mail]]

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

>+#ifdef MOZILLA_INTERNAL_API
>   if (mMsgWindow && (StringBeginsWith(aUri, NS_LITERAL_CSTRING("file:")) ||
>                      nsDependentCString(aUri).Find("type=application/x-message-display") >= 0))
>+#else
>+  if (mMsgWindow && (StringBeginsWith(aUri, NS_LITERAL_CSTRING("file:")) ||
>+                     aUri.Find("type=application/x-message-display") >= 0))
>+#endif
I think PromiseFlatCString is the way to go here.

>-  nsCOMPtr<nsISupportsArray> mFoldersWithNewMail;  // keep track of all the root folders with pending new mail
>+  nsCOMPtr<nsIMutableArray> mFoldersWithNewMail;  // keep track of all the root folders with pending new mail
Why this change? (The change to the constructor doesn't match. If you need to change it, I think an nsCOMArray would be better.)

>+  rv = NS_NewArrayEnumerator(_retval, nodes);
>+  if (NS_FAILED(rv)) return rv;
> 
>   return NS_OK;
Could just return NS_NewArrayEnumerator(_retval, nodes); [Admittedly the previous code was just weird to create the enumerator before populating it.]

>+                                                   nsCOMArray<nsIRDFResource> *aNodeArray)
[I'd prefer to pass this by reference, but the callback makes this hard...]

>+    mAccountArcsOut = do_CreateInstance(NS_SUPPORTSARRAY_CONTRACTID, &rv);
Wrong contract, no?

>+        nsString tmp_str;
...
+            nsString retVal;
Nit: inconsistent temporary adopting string name.

>-              return NS_OK; // don't whitelist
>-#else
>-            if (identityDomain.Equals(domain, CaseInsensitiveCompare))
>               return NS_OK;
Nit: shouldn't delete the comment.

Comment 229

[[e-mail]]

Comment on attachment 427318 [details] [diff] [review]
address book patch v6

Some of these patches could do with sr; randomly picking victims...

Comment 230

[[e-mail]]

Comment on attachment 427736 [details] [diff] [review]
news patch v6
[Checkin: Comment 234]

Some of these patches could do with sr; randomly picking victims...

Comment 231

[[e-mail]]

Comment on attachment 428437 [details] [diff] [review]
extensions patch v5

Some of these patches could do with sr; randomly picking victims...

Comment 232

[Dan Mosedale (:dmosedale, :dmose)]

Just from skimming these patches, they don't look crazy high risk, and you (Neil) are generally a very thorough reviewer.  Would you be comfortable just stamping them r+sr=Neil?

Comment 233

[David :Bienvenu]

Comment on attachment 428696 [details] [diff] [review]
local patch v5

this change looks inadvertent:

-      name.LowerCaseEqualsLiteral("rulesbackup.dat"))
+      name.LowerCaseEqualsLiteral( "rulesbackup.dat"))

the patch no longer applies; could you get me a non-bitrotted version? thx! Clearing sr request for now; please re-request with a newer patch.

Comment 234

[[e-mail]]

Comment on attachment 427736 [details] [diff] [review]
news patch v6
[Checkin: Comment 234]

Pushed changeset 886d66940820 to comm-central.

Comment 235

[Dan Mosedale (:dmosedale, :dmose)]

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

I'm already too far behind on reviews, and I know bienvenu has some of this in his head from last week, so I'm giving to him... (Sorry, David!)

Comment 236

[Jan Horak [:jhorak]]

Attachment: compose patch v6

Sync to trunk, fixes of mentioned issues.

Comment 237

[[e-mail]]

Comment on attachment 437549 [details] [diff] [review]
compose patch v6

>+          MsgEscapeURL(myGetter, 
Nit: trailing space

>+                         (MsgLowerCaseEqualsLiteral(fileExt, "rtf")
>+                          || MsgLowerCaseEqualsLiteral(fileExt, "vcs")))
I know the code isn't consistent but current style is to put the operator (i.e. ||) at the end of the previous line instead.

Comment 238

[[e-mail]]

Comment on attachment 437549 [details] [diff] [review]
compose patch v6

Is it possible to simplify OnDataAvailable by using rv = AppendToMsgBody(nsDependentCString(newBuf, numWritten)); ?

Comment 239

[Mark Banner (:standard8)]

Comment on attachment 427318 [details] [diff] [review]
address book patch v6

This bitrotted with the checkin of attachment 426927 [details] [diff] [review]. Please can you refresh the patch.

Comment 240

[Mark Banner (:standard8)]

Comment on attachment 428700 [details] [diff] [review]
mime patch v4
[Checkin: Comment 241]

This is also bitrotted in just one file (mimemoz2.cpp), but as just one change is easier to handle, I just ignored it. sr=Standard8.

Comment 241

[[e-mail]]

Comment on attachment 428700 [details] [diff] [review]
mime patch v4
[Checkin: Comment 241]

Pushed changeset 071ad171572e to comm-central.

The bitrot was solely due to having been previously pushed as part of attachment 426927 [details] [diff] [review].

Comment 242

[Jan Horak [:jhorak]]

Attachment: mapi patch v3 [Checkin: Comment 322]

Fixed -/+1.

Comment 243

[Jan Horak [:jhorak]]

Attachment: address book patch v7

Comment 244

[[e-mail]]

Comment on attachment 438726 [details] [diff] [review]
address book patch v7

>diff -r e81538bfd96e mailnews/addrbook/src/nsAbOutlookDirectory.cpp
Still missing nsMsgUtils.h include. r=me with that fixed.

Comment 245

[[e-mail]]

Comment on attachment 437549 [details] [diff] [review]
compose patch v6

Sorry for taking so long over this, I was very busy with work.

>   inputFile->Read(readBuf, readSize, &bytesRead);
>   inputFile->Close();
> 
>+  nsDependentCString cstringReadBuf(readBuf);
[Might be worth passing bytesRead in too?]

>-          AppendASCIItoUTF16(NS_EscapeURL(myGetter, esc_FileBaseName | esc_Forced, buf),
>-                             mCiteReference);
>+          MsgEscapeURL(myGetter, 
>+                       nsINetUtil::ESCAPE_URL_FILE_BASENAME | nsINetUtil::ESCAPE_URL_FORCED,
>+                       buf);
>+          mCiteReference.Append(NS_ConvertUTF8toUTF16(buf));
Not sure whether it makes any difference, but the old code converted ASCII, not UTF8.

Comment 246

[[e-mail]]

Actually I am in fact still hoping for an answer to comment 238, aren't I?

Comment 247

[Jan Horak [:jhorak]]

Attachment: base/src patch v4

+ required changes to base/utils due to missing NS_NewArrayEnumerator for nsISupportsArray in frozen linkage.

Comment 248

[Jan Horak [:jhorak]]

Attachment: compose patch v7

The AppendToMsgBody used in OnDataAvailable to create less copy code.

Comment 249

[[e-mail]]

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

>diff -r e81538bfd96e mailnews/base/src/nsMessenger.cpp
>--- a/mailnews/base/src/nsMessenger.cpp	Sat Dec 19 23:02:56 2009 -0800
>+++ b/mailnews/base/src/nsMessenger.cpp	Thu Apr 29 14:12:59 2010 +0200
>@@ -91,7 +90,6 @@
> 
> // mail
> #include "nsIMsgMailNewsUrl.h"
>-#include "nsMsgUtils.h"
> #include "nsMsgBaseCID.h"
> #include "nsIMsgAccountManager.h"
> #include "nsIMsgMailSession.h"
>@@ -157,6 +155,7 @@ static NS_DEFINE_CID(kMsgSendLaterCID, N
> #include "nsICharsetConverterManager.h"
> #include "nsIContentSink.h"
> #include "nsIHTMLToTextSink.h"
>+#include "nsMsgUtils.h"
Is this move necessary? (Also the move in nsMsgContentPolicy.cpp)

>   if (mMsgWindow && (StringBeginsWith(aUri, NS_LITERAL_CSTRING("file:")) ||
>-                     nsDependentCString(aUri).Find("type=application/x-message-display") >= 0))
>+      PromiseFlatCString(aUri).Find("type=application/x-message-display") >= 0))
Nit: the PromisFlatCString should line up as the nsDependentCString did with the StringBeginsWith since they are both inside the inner ( || ) condition. Alternatively you could write this as
if (mMsgWindow &&
    (StringBeginsWith(...) ||
     PromiseFlatCString(aURI).Find(...) >= 0))

>   NS_NewISupportsArray(getter_AddRefs(mFoldersWithNewMail));
I thought you were going to change this to a mutable array? I wasn't sure because you changed the .h file to a mutable array but the .cpp file to NS_SUPPORTS_ARRAY_CONTRACTID. Sorry if there was any confusion.

Comment 250

[[e-mail]]

(In reply to comment #247)
> + required changes to base/utils due to missing NS_NewArrayEnumerator for
> nsISupportsArray in frozen linkage.
Where are we trying to enumerate an nsISupportsArray?

Comment 251

[[e-mail]]

Comment on attachment 442374 [details] [diff] [review]
compose patch v7

Patch looks good, will test it out shortly.

>+  if (inStr.Length() > 0)
Nit: !inStr.IsEmpty()

>   if (NS_SUCCEEDED(rv)) {
>     CopyUTF16toUTF8(retUrl, _retval);
>+    MsgUnescapeString(utf8Scheme, nsINetUtil::ESCAPE_URL_SKIP_CONTROL, _retval);
>   } else {
This was a bad merge with bug 161156, which actually remove the call to NS_Escape, so you no longer need replace it with MsgUnescapeString ;-)
(and I just noticed that utf8Scheme is now unused, so that can go too).

Comment 252

[[e-mail]]

Comment on attachment 442374 [details] [diff] [review]
compose patch v7

>+          MsgUnescapeString(group,
>+                            nsINetUtil::ESCAPE_URL_FILE_BASENAME | nsINetUtil::ESCAPE_URL_FORCED,
>                          unescapedName);
Nit: unescapedName should line up with nsINetUtil

>+                         (MsgLowerCaseEqualsLiteral(fileExt, "rtf") || 
Nit: trailing space

>+        if (responseLine.Equals(NS_LITERAL_CSTRING("STARTTLS"), nsCaseInsensitiveCStringComparator()))
I didn't think Equals needs an NS_LITERAL_CSTRING, does it?

>+            if (responseLine.Find(NS_LITERAL_CSTRING("GSSAPI"), 5, CaseInsensitiveCompare) >= 0)
For some reason this worries me now. I wonder whether we can get away without using an offset. (And probably without using NS_LITERAL_CSTRING too.)

Comment 253

[Jan Horak [:jhorak]]

#define Compare(str1, str2, comp) \
        (str1).Compare(str2, comp)(In reply to comment #249)
> (From update of attachment 442373 [details] [diff] [review])
> >diff -r e81538bfd96e mailnews/base/src/nsMessenger.cpp
> >--- a/mailnews/base/src/nsMessenger.cpp	Sat Dec 19 23:02:56 2009 -0800
> >+++ b/mailnews/base/src/nsMessenger.cpp	Thu Apr 29 14:12:59 2010 +0200
> >@@ -91,7 +90,6 @@
> > 
> > // mail
> > #include "nsIMsgMailNewsUrl.h"
> >-#include "nsMsgUtils.h"
> > #include "nsMsgBaseCID.h"
> > #include "nsIMsgAccountManager.h"
> > #include "nsIMsgMailSession.h"
> >@@ -157,6 +155,7 @@ static NS_DEFINE_CID(kMsgSendLaterCID, N
> > #include "nsICharsetConverterManager.h"
> > #include "nsIContentSink.h"
> > #include "nsIHTMLToTextSink.h"
> >+#include "nsMsgUtils.h"
> Is this move necessary? (Also the move in nsMsgContentPolicy.cpp)
There is a problem with Compare macro when compiling with frozen linkages:
#define Compare(str1, str2, comp) \
        (str1).Compare(str2, comp)
nsTPriorityQueue.h is using Compare as template parameter. The compilation will fail with:
In file included from ../../../mozilla/dist/include/nsSMILTimeContainer.h:43,
                 from ../../../mozilla/dist/include/nsSMILAnimationController.h:48,
                 from ../../../mozilla/dist/include/nsIDocument.h:62,
                 from ../../../mozilla/dist/include/nsIContent.h:45,
                 from ../../../mozilla/dist/include/nsILinkHandler.h:41,
                 from nsMessenger.cpp:140:
../../../mozilla/dist/include/nsTPriorityQueue.h:60:41: error: macro "Compare" requires 3 arguments, but only 1 given
gmake[5]: *** [nsMessenger.o] Error 1

It's not nice, but it's working.

Comment 254

[Jan Horak [:jhorak]]

(In reply to comment #250)
> (In reply to comment #247)
> > + required changes to base/utils due to missing NS_NewArrayEnumerator for
> > nsISupportsArray in frozen linkage.
> Where are we trying to enumerate an nsISupportsArray?
http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgFolderDataSource.cpp#2260

Comment 255

[Jan Horak [:jhorak]]

> >   NS_NewISupportsArray(getter_AddRefs(mFoldersWithNewMail));
> I thought you were going to change this to a mutable array? I wasn't sure
> because you changed the .h file to a mutable array but the .cpp file to
> NS_SUPPORTS_ARRAY_CONTRACTID. Sorry if there was any confusion.
Um...it looks like it is not required. Maybe I was try to rid of nsISupportsArray (but that's another bug).

Comment 256

[[e-mail]]

(In reply to comment #253)
> There is a problem with Compare macro when compiling with frozen linkages:
> nsTPriorityQueue.h is using Compare as template parameter.
Fair enough.

(In reply to comment #254)
> > Where are we trying to enumerate an nsISupportsArray?
> http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgFolderDataSource.cpp#2260
Now I know why I was confused - your original base/src patch rewrote that not to need to enumerate an nsISupportsArray, but somehow it disappeared in v3 :-(

(In reply to comment #255)
> > >   NS_NewISupportsArray(getter_AddRefs(mFoldersWithNewMail));
> Um...it looks like it is not required. Maybe I was try to rid of
> nsISupportsArray (but that's another bug).
OK, if it still works then yes we can leave that to another bug.

Comment 257

[[e-mail]]

Comment on attachment 442374 [details] [diff] [review]
compose patch v7

>+            if (responseLine.Find(NS_LITERAL_CSTRING("GSSAPI"), 5, CaseInsensitiveCompare) >= 0)
So, nsStringAPI provides the following Find methods:
nsAString::Find(const nsAString&[, ComparatorFunc])
nsAString::Find(const nsAString&, PRUint32[, ComparatorFunc])
nsAString::Find(const char *[, PRBool])
nsAString::Find(const char *, PRUint32[, PRBool])
[Doesn't that mean that Find(const char*, 0) is ambiguous?]
nsACString::Find(const nsACString&[, ComparatorFunc])
nsACString::Find(const nsACString&, PRUint32[, ComparatorFunc])
nsACString::Find(const char *[, ComparatorFunc])
nsACString::Find(const char *, PRUint32[, ComparatorFunc])
But nsTString.h provides the following Find methods:
nsString::Find(const nsString&[, PRInt32, PRInt32])
nsString::Find(const PRUnichar *[, PRInt32, PRInt32])
nsString::Find(const nsCString&[, PRBool, PRInt32, PRInt32])
nsString::Find(const char *[, PRBool, PRInt32, PRInt32])
nsCString::Find(const nsCString&[, PRBool, PRInt32, PRInt32])
nsCString::Find(const char *[, PRBool, PRInt32, PRInt32])
The internal methods provide an optional count argument that the external methods do not, but this is not a problem as we do not use it. However the case-sensitivity and offset arguments are reversed from the external methods.

The FindInReadable methods are no help as they do not provide an offset.

Comment 258

[[e-mail]]

So the usable Find methods are as follows:
theString.Find(otherString)
theString.Find("asciiString"[, PR_TRUE])
theCString.Find(otherCString[, CaseInsensitiveCompare])
theCString.Find("asciiString"[, CaseInsensitiveCompare])

Comment 259

[Jan Horak [:jhorak]]

(In reply to comment #258)
> So the usable Find methods are as follows:
> theString.Find(otherString)
> theString.Find("asciiString"[, PR_TRUE])
> theCString.Find(otherCString[, CaseInsensitiveCompare])
> theCString.Find("asciiString"[, CaseInsensitiveCompare])
Okay, it's been suspicious for me too. So how to solve Find with offset? By Substring or write something own like overloaded MsgFindOffset(nsString &str, PRInt32 offset, PRBool case_insensitive)?

Comment 260

[Jan Horak [:jhorak]]

As you mention could there be also confusion between
theString.Find("asciiString", PR_TRUE) 
and 
nsAString::Find(const char *, PRUint32[, PRBool]) 
while PR_TRUE is just defined as 1 doesn't it mean offset 1 in frozen linkage? We may replace 
theString.Find("asciiString", PR_TRUE) 
by 
theString.Find("asciiString", CaseInsensitiveCompare) 
without any problem (there are ~ 10-15 occurrences of this issue).

Comment 261

[Jan Horak [:jhorak]]

(In reply to comment #256)
> > > Where are we trying to enumerate an nsISupportsArray?
> > http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgFolderDataSource.cpp#2260
> Now I know why I was confused - your original base/src patch rewrote that not
> to need to enumerate an nsISupportsArray, but somehow it disappeared in v3 :-(

Sorry for confusion. Can we keep MsgNewEnumerator and leave conversion of nsISupportsArray into Mutable array for another bug (#394167)? My changes seems to disappear by accident when syncing to trunk version.

Comment 262

[[e-mail]]

(In reply to comment #259)
> (In reply to comment #258)
> > So the usable Find methods are as follows:
> > theString.Find(otherString)
> > theString.Find("asciiString"[, PR_TRUE])
> > theCString.Find(otherCString[, CaseInsensitiveCompare])
> > theCString.Find("asciiString"[, CaseInsensitiveCompare])
> Okay, it's been suspicious for me too. So how to solve Find with offset? By
> Substring or write something own like overloaded MsgFindOffset(nsString &str,
> PRInt32 offset, PRBool case_insensitive)?
I asked bienvenu and he thinks that in some cases it might still work without an offset.

(In reply to comment #260)
> We may replace 
> theString.Find("asciiString", PR_TRUE) 
> by 
> theString.Find("asciiString", CaseInsensitiveCompare) 
> without any problem (there are ~ 10-15 occurrences of this issue).
I don't think that's true, is it? The signature of nsAString::Find is
Find(const char *aStr, PRBool aIgnoreCase = PR_FALSE)
Find(const char *aStr, PRUint32 aOffset, PRBool aIgnoreCase = PR_FALSE)
It doesn't use CaseInsensitiveCompare.
Fortunately PRBool is signed, so that when you have two arguments the correct version of Find is selected based on whether you pass an unsigned offset or a (signed) PR_TRUE.

(In reply to comment #261)
> Can we keep MsgNewEnumerator and leave conversion of
> nsISupportsArray into Mutable array for another bug (#394167)?
I haven't asked anyone else yet but I wouldn't be keen to define MsgNewEnumerator temporarily just for this one caller; perhaps the easiest answer is to split out the array conversion for this one case into a separate patch, rather than combining it with either this or bug 394167?

Comment 263

[Jan Horak [:jhorak]]

Attachment: Conversion of nsISupportsArray to nsIMutable array in ListDescendents method

Please have a look. It would be nice if this patch will make it into trunk earlier that I don't have to deal with wrong diffs in other patches.

Comment 264

[[e-mail]]

(In reply to comment #263)
> Created an attachment (id=444633)
> Conversion of nsISupportsArray to nsIMutable array in ListDescendents method
> 
> Please have a look. It would be nice if this patch will make it into trunk
> earlier that I don't have to deal with wrong diffs in other patches.

Now I'm confused. In comment 247 you said NS_NewArrayEnumerator was missing. But none of this patch seems to have anything to do with that?

Comment 265

[[e-mail]]

Although nsMsgFlatFolderDataSource calls ListDescendants, is that the issue?

Comment 266

[Jan Horak [:jhorak]]

(In reply to comment #264)
> (In reply to comment #263)
> > Created an attachment (id=444633) [details]
> > Conversion of nsISupportsArray to nsIMutable array in ListDescendents method
> > 
> > Please have a look. It would be nice if this patch will make it into trunk
> > earlier that I don't have to deal with wrong diffs in other patches.
> 
> Now I'm confused. In comment 247 you said NS_NewArrayEnumerator was missing.
> But none of this patch seems to have anything to do with that?
Function NS_NewArrayEnumerator for nsISupportsArray parameter is only in internal API. You told me to avoid MsgNewArrayEnumerator (no problem with that). To make it working with frozen linkage I changed nsISupportsArray to nsIMutableArray and did necessary changes.

Comment 267

[[e-mail]]

nsMsgFlatFolderDataSource has bugs, so I need to rewrite it anyway...

Comment 268

[Jan Horak [:jhorak]]

Um, what's the status now? It's a bit difficult to mix two collision patches (Conversion of nsISupportsArray to nsIMutable array in ListDescendents method) with the rest. Any chance to checkin Conversion of nsISupportsArray to nsIMutable array in ListDescendents method patch?

Comment 269

[[e-mail]]

The status on base/src patch is that I don't want to change the ListDescendents method just yet, because the case where it's a problem is in code that may get removed anyway, so maybe we should just land the unrelated changes first.

I believe the compose patch needs a resolution on the Find issue.

Comment 270

[[e-mail]]

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

This review does not include:
nsMsgAccountManagerDS changes
nsMsgFolderDataSource changes
nsMsgUtils changes
These need to be addressed separately.

>   if (mMsgWindow && (StringBeginsWith(aUri, NS_LITERAL_CSTRING("file:")) ||
>-                     nsDependentCString(aUri).Find("type=application/x-message-display") >= 0))
>+      PromiseFlatCString(aUri).Find("type=application/x-message-display") >= 0))
[This nit from a previous comment is still outstanding.]

>-    if ((fileName.RFind(HTML_FILE_EXTENSION, PR_TRUE, -1,
>-                        sizeof(HTML_FILE_EXTENSION) - 1) != kNotFound) ||
>-        (fileName.RFind(HTML_FILE_EXTENSION2, PR_TRUE, -1,
>-                        sizeof(HTML_FILE_EXTENSION2) - 1) != kNotFound))
>+    if ((fileName.RFind(HTML_FILE_EXTENSION, PR_TRUE) != kNotFound) ||
>+        (fileName.RFind(HTML_FILE_EXTENSION2, PR_TRUE) != kNotFound))
>       *aSaveAsFileType = HTML_FILE_TYPE;
>-    else if (fileName.RFind(TEXT_FILE_EXTENSION, PR_TRUE, -1,
>-                            sizeof(TEXT_FILE_EXTENSION)-1) != kNotFound)
>+    else if (fileName.RFind(TEXT_FILE_EXTENSION, PR_TRUE) != kNotFound)
>       *aSaveAsFileType = TEXT_FILE_TYPE;
The third parameter is the point at which to start finding (defaulting to the difference in string lengths) and the fourth parameter is the number of substrings to compare (defaulting to starting at the point and going back to the beginning, so point+1). Omitting the fourth parameter thus means that the whole string will be searched for an extension, which makes no sense. It looks as if someone has misunderstood the point of RFind and is trying to use it when they need to use StringEndsWith. So I think this should be replaced with StringEndsWith rather than RFind.

>diff -r e81538bfd96e mailnews/base/src/nsMessengerOSXIntegration.mm
Does this need s/do_GetAtom/MsgGetAtom/ ?

>+#include "nsArrayUtils.h"
Part of the nsIArray conversion?

>-  static nsCOMPtr<nsISupportsArray> mAccountArcsOut;
>-  static nsCOMPtr<nsISupportsArray> mAccountRootArcsOut;
>+  static nsCOMPtr<nsIMutableArray> mAccountArcsOut;
>+  static nsCOMPtr<nsIMutableArray> mAccountRootArcsOut;
[Groan. Whose idea was these static nsCOMPtr variables?]

>diff -r e81538bfd96e mailnews/base/src/nsMsgRDFUtils.cpp
These changes don't seem to be necessary?

The rest of the patch seems to be fine, thanks!

Comment 271

[Jan Horak [:jhorak]]

Attachment: compose patch v8

- Removed offset from Finds
- fixed indentation
- used !IsEmpty instead of Length != 0
- removed wrongly added MsgUnescapeString

Comment 272

[Jan Horak [:jhorak]]

Attachment: imap patch v4

- removed AppendUTF8toUTF16 unrequired changes
- Fixed FindChar('//')
- removed mOwnsArray check before delete
- using MsgGetProxyForObject
- instead of FetchMessage parameter rewriting, used NS_LITERAL_CSTRING("")
- fixed replacing content of urlSpec

Comment 273

[David :Bienvenu]

Comment on attachment 428437 [details] [diff] [review]
extensions patch v5

sr=me, if you wrap these extra long lines:

-                if (!mailTo.IsEmpty() && !identEmail.IsEmpty() && mailTo.Find(identEmail, PR_TRUE) != -1)
+                if (!mailTo.IsEmpty() && !identEmail.IsEmpty() && mailTo.Find(identEmail, CaseInsensitiveCompare) != kNotFound)
                 {
                   m_identity = ident;
                   break;
@@ -984,7 +984,7 @@ nsresult nsMsgMdnGenerator::InitAndProce
                   if (NS_FAILED(rv))
                     continue;
                   ident->GetEmail(identEmail);
-                  if (!mailCC.IsEmpty() && !identEmail.IsEmpty() && mailCC.Find(identEmail, PR_TRUE) != -1)
+                  if (!mailCC.IsEmpty() && !identEmail.IsEmpty() && mailCC.Find(identEmail, CaseInsensitiveCompare) != kNotFound)

Comment 274

[[e-mail]]

Comment on attachment 447084 [details] [diff] [review]
imap patch v4

>+      index = uri.FindChar('/', index+2);       // find '/' after scheme
Nit: since you're changing this anyway, might be nice to write
index + 2

>+  const nsTArray<nsCString>* mStrings;
Nit: I don't think this needs to be const
(this would then avoid the const cast in the destructor)

>+  PRBool                     mOwnsArray;
Nit: Unused (well, write once read never!)

>+      nsCOMPtr<nsIProxyObjectManager> proxyObjMgr = do_GetService(NS_XPCOMPROXY_CONTRACTID, &rv);
>+      NS_ENSURE_SUCCESS(rv, rv);
Unused.

>+  onlineDir = NS_LossyConvertUTF16toASCII(aString);
Nit: Could use LossyCopyUTF16toASCII

>+      if (uri.Find(namespacePrefix, index+1) != index+1
>+        PRInt32 mpodFetchPos = uriStr.Find("fetchCompleteMessage=true", keyEndSeparator);
>+      PRInt32 partPos = uriStr.Find("part=", keyEndSeparator);
Sorry I forgot to check these before (well, I looked at Find before, but not in the context of this part of the patch). While they now work with the external API, these still call nsTString_CharT::Find( const char* aString, PRBool aIgnoreCase, PRInt32 aOffset, PRInt32 aCount) const when used with the internal API, but now of course the parameters are wrong.

Comment 275

[[e-mail]]

I asked bienvenu and he seemed to agree that we're going to need more macros to hide the vagaries of the offset parameter of the Find API...

Comment 276

[Jan Horak [:jhorak]]

Ok, what about:

external API:
#define MsgFind(str, what, offset, case_sensitive) \
  (str).Find(what, offset, case_sensitive)

internal API:
#define MsgFind(str, what, offset, case_sensitive) \
  (str).Find(what, case_sensitive, offset)

while there won't be MsgFind(str, what, case_sensitive), all code will have to be called including offset and case sensitivity flag. Um... a bit clumsy but it'll work.

Comment 277

[[e-mail]]

(In reply to comment #276)
> all code will have to be called including offset and case sensitivity flag.
> Um... a bit clumsy but it'll work.
Sounds good to me; the existing code that wants to include an offset already has to pass a case sensitivity flag anyway, so this isn't really any clumsier.

Comment 278

[David :Bienvenu]

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

since we have kNotFound, we could use it instead of all the -1's here, but either way is OK. thx for the patch.

Comment 279

[Jan Horak [:jhorak]]

Attachment: MsgFind patch

Unfortunately there is problem with frozen linkage:
method str.Find(const nsA(C)String &what, PRUint32 offset, PRBool case_sensitive) is not defined (case_sensitive bool is missing, only compare function is available).
Attaching patch as proposed solution.

Comment 280

[[e-mail]]

Comment on attachment 448485 [details] [diff] [review]
MsgFind patch

>-#define FIND_KEYWORD(keywords,keyword,offset) ((keywords).Find((keyword), PR_FALSE, (offset)))
>+#define FIND_KEYWORD(keywords,keyword,offset) ((keywords).Find((keyword), (offset)))
This change looks wrong; presumably what should actually happen here is that FIND_KEYWORD(keywords, keyword, offset) should become MsgFind(keywords, keyword, PR_FALSE, offset) [but you don't have to make this change here].

>+#define MsgNewArrayEnumerator(result, array) \
>+        NS_NewArrayEnumerator(result, array)
Part of another patch, I think?

>+#define MsgFind(str, what, offset, case_sensitive) \
>+        (str).Find(what, offset, case_sensitive)
As I recall, when using the internal API the case_sensitive is the second parameter to Find, which why this headache exists in the first place!

If you move case_sensitive before offset in MsgFind as well then it might make the switch from Find to MsgFind easier to read since that's the parameter order in the existing code.

>+  if (case_sensitive)
>+    return str.Find(what, offset, CaseInsensitiveCompare);
>+  else 
>+    return str.Find(what, offset);
Mozilla code style suggests not using else after return.

Comment 281

[Jan Horak [:jhorak]]

Attachment: MsgFind patch v2 [Checkin: Comment 308]

Ouch, sorry for hasty and confusing patch :(. Adding different one.

Comment 282

[[e-mail]]

Comment on attachment 448498 [details] [diff] [review]
MsgFind patch v2 [Checkin: Comment 308]

This looks good to me.

While looking for something to test this patch on, I noticed that nsAbManager seems to call Find with the arguments in external API order...

Comment 283

[[e-mail]]

(In reply to comment #279)
> Unfortunately there is problem with frozen linkage:
> method str.Find(const nsA(C)String &what, PRUint32 offset, PRBool
> case_sensitive) is not defined (case_sensitive bool is missing, only compare
> function is available).
Yes, it's annoying that the nsAString.Find(const char*) version takes a PRBool while the other versions use a compare function... it's also annoying that the case insensitive function is a global, but the default compare function is a member of the string class, so you can't easily simplify it that way either.

Comment 284

[Jan Horak [:jhorak]]

(In reply to comment #282)
> (From update of attachment 448498 [details] [diff] [review])
> This looks good to me.
> 
> While looking for something to test this patch on, I noticed that nsAbManager
> seems to call Find with the arguments in external API order...
Yes, I'm tracking all the occurrences of wrong Find usage. I'm going to add fixed patches.

Comment 285

[Jan Horak [:jhorak]]

Attachment: address book patch v8

Replacting some Find methods with MsgFind.

Comment 286

[Jan Horak [:jhorak]]

Attachment: imap patch v5

- Some Find replaced by MsgFind 
- Fixed nits
- Fixed AdoptUTF8StringEnumerator

Comment 287

[Jan Horak [:jhorak]]

Attachment: base/util MsgFind replace

Replaced Find by MsgFind in base/util directory.

Comment 288

[[e-mail]]

(In reply to comment #258)
> So the usable Find methods are as follows:
> theString.Find(otherString)
> theString.Find("asciiString"[, PR_TRUE])
> theCString.Find(otherCString[, CaseInsensitiveCompare])
> theCString.Find("asciiString"[, CaseInsensitiveCompare])
Sorry, but I think I misread this. You can actually use
theString.Find(otherString[, offset])
since the internal API's Find is always case sensitive.

Comment 289

[[e-mail]]

Comment on attachment 448498 [details] [diff] [review]
MsgFind patch v2 [Checkin: Comment 308]

>+inline PRInt32 MsgFind(nsAString &str, const nsAString &what, PRBool case_sensitive, PRUint32 offset)
So in fact we don't need this one.

Comment 290

[Jan Horak [:jhorak]]

Attachment: base/src patch v5

- removed confusing MsgNewArrayEnumerator
- fixed indentation
- unable to translate this code with frozen linkage because NS_NewArrayEnumerator not yet ported (due to missing NS_NewArrayEnumerator for nsISupportsArray), this going to be addressed separately by different bug (#394167 ?).

Comment 291

[[e-mail]]

Comment on attachment 448725 [details] [diff] [review]
address book patch v8

>-                    match = newValue.Find(oldSubstr, offset);
>+                    match = MsgFind(newValue, oldSubstr, PR_FALSE, offset);
So this one change turns out be wrong.

Also note that comment 244 still applies.

Comment 292

[[e-mail]]

Comment on attachment 448728 [details] [diff] [review]
base/util MsgFind replace

So I think this is wrong too.

Comment 293

[[e-mail]]

Comment on attachment 448727 [details] [diff] [review]
imap patch v5

Just noticed this, it appears to be a bug:
>-    char *search_cmd = nsEscape((char *)aSearchUri, url_XAlphas);
>+    MsgEscapeString(nsDependentCString(aSearchUri), nsINetUtil::ESCAPE_XPALPHAS, escapedSearchUri);
XPALPHAS != XALPHAS. r=me with this fixed.

These are all nits:
>diff -r e81538bfd96e mailnews/imap/src/nsIMAPBodyShell.cpp
>--- a/mailnews/imap/src/nsIMAPBodyShell.cpp	Sat Dec 19 23:02:56 2009 -0800
>+++ b/mailnews/imap/src/nsIMAPBodyShell.cpp	Wed Jun 02 13:53:35 2010 +0200
>@@ -44,6 +44,8 @@
> #include "nsIPrefBranch.h"
> #include "nsIPrefService.h"
> #include "nsITransport.h"
>+#include "nsServiceManagerUtils.h"
>+
> 
[unnecessary extra blank line]

>diff -r e81538bfd96e mailnews/imap/src/nsImapFlagAndUidState.cpp
>--- a/mailnews/imap/src/nsImapFlagAndUidState.cpp	Sat Dec 19 23:02:56 2009 -0800
>+++ b/mailnews/imap/src/nsImapFlagAndUidState.cpp	Wed Jun 02 13:53:35 2010 +0200
>@@ -43,6 +43,7 @@
> #include "prcmon.h"
> #include "nspr.h"
> #include "nsAutoLock.h"
>+#include "nsAlgorithm.h"
[Some of these includes look odd on their own!]

>-  AppendASCIItoUTF16(hostName, constructedPrettyName);
>+  constructedPrettyName.Append(NS_ConvertASCIItoUTF16(hostName));
[an example of an obsolete change]

>-      if (uri.Find(namespacePrefix, PR_FALSE, index+1) != index+1
>-        && !Substring(uri, index + 1, uri.Length() - index - 1).LowerCaseEqualsLiteral("inbox"))
>+      if (MsgFind(uri, namespacePrefix, PR_FALSE, index+1) != index+1
>+        && !(MsgLowerCaseEqualsLiteral(Substring(uri, index + 1), "inbox")))
[Current style would be like this:
if (MsgFind(uri, namespacePrefix, PR_FALSE, index + 1) != index + 1 &&
    !MsgLowerCaseEqualsLiteral(Substring(uri, index + 1), "inbox"))
]

>-        onlineCName.ReplaceChar('/',  m_hierarchyDelimiter);
>+        MsgReplaceChar(onlineCName, '/',  m_hierarchyDelimiter);
[doubled space]

>-  if (keywords.Find("NonJunk", PR_TRUE /* ignore case */) != -1 || keywords.Find("NotJunk", PR_TRUE /* ignore case */) != -1)
>+  if (keywords.Find("NonJunk", CaseInsensitiveCompare /* ignore case */) != -1 || keywords.Find("NotJunk", CaseInsensitiveCompare /* ignore case */) != -1)
These comments (and those later) look unnecessary now! Also, this line is very long now, it would look better if it was wrapped after the ||.

Comment 294

[[e-mail]]

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

This covers comment 249 but doesn't appear to cover comment 270?

Comment 295

[Jan Horak [:jhorak]]

Attachment: address book patch v9

- Fixed MsgFind usage
- Added nsMsgUtils.h include

Comment 296

[Jan Horak [:jhorak]]

Attachment: imap patch v6

- fixed XPALPHAS
- removed redundant comments
>-  AppendASCIItoUTF16(hostName, constructedPrettyName);
>+  constructedPrettyName.Append(NS_ConvertASCIItoUTF16(hostName));
[an example of an obsolete change]
Not sure if I understand ^^

Comment 297

[Jan Horak [:jhorak]]

Attachment: base/src patch v6

- RFind replaced with StringEndsWith
- s/do_GetAtom/MsgGetAtom for s/do_GetAtom/MsgGetAtom
- indentation
About:
>>+  static nsCOMPtr<nsIMutableArray> mAccountArcsOut;
>>+  static nsCOMPtr<nsIMutableArray> mAccountRootArcsOut;
>[Groan. Whose idea was these static nsCOMPtr variables?]
Suggesting
static nsIMutableArray mAccountArcsOut; ?

Comment 298

[[e-mail]]

Comment on attachment 449019 [details] [diff] [review]
imap patch v6

>+  if (keywords.Find("NonJunk", CaseInsensitiveCompare) != -1 || 
>+                    keywords.Find("NotJunk", CaseInsensitiveCompare) != -1)
[Nit: indentation wrong; keywords should line up with keywords]

Comment 299

[[e-mail]]

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

>diff -r e81538bfd96e mailnews/base/src/nsMessengerOSXIntegration.mm
>--- a/mailnews/base/src/nsMessengerOSXIntegration.mm	Sat Dec 19 23:02:56 2009 -0800
>+++ b/mailnews/base/src/nsMessengerOSXIntegration.mm	Thu Jun 03 17:15:27 2010 +0200
>@@ -43,6 +43,7 @@
> #endif
> 
> #include "nscore.h"
>+#include "nsMsgUtils.h"
While I spotted this one last time I forgot to check nsMessengerWinIntegration.cpp which also needs it. r=me with that fixed.

>-  nsCOMPtr<nsISupportsArray> arcs;
>+  nsCOMPtr<nsIMutableArray> arcs;
[Note to self: an nsCOMArray might work even better still.]

A couple of tiny nits:

>-
>+  
Oops, you accidentally added two spaces to this line. (There were also five other lines with trailing spaces, but they may have previously existed.)

>+  *target = isServer ||  MsgLowerCaseEqualsLiteral(serverType, "none") || MsgLowerCaseEqualsLiteral(serverType, "pop3") ?
>+            kTrueLiteral : kFalseLiteral;
> 
>-  *target = isServer || serverType.LowerCaseEqualsLiteral("none") || serverType.LowerCaseEqualsLiteral("pop3") ?
>-            kTrueLiteral : kFalseLiteral;
This looked odd to me until I realised that the blank line had moved.

Comment 300

[Jan Horak [:jhorak]]

Attachment: base/src patch v7 [Checkin: Comment 323]

- Added missing include
- Removed additional space

Comment 301

[Jan Horak [:jhorak]]

Attachment: imap patch v7

- Indentation fixed

Comment 302

[Jan Horak [:jhorak]]

Attachment: base/search patch v3

- fixed indentation

Comment 303

[Jan Horak [:jhorak]]

Attachment: extensions patch v6 [Checkin: Comment 307]

- Fixed long line

Comment 304

[[e-mail]]

Comment on attachment 449255 [details] [diff] [review]
base/src patch v7 [Checkin: Comment 323]

[Note to self: only checked for new include. Need to verify whitespace nits before checking in.]

Comment 305

[[e-mail]]

Comment on attachment 449258 [details] [diff] [review]
extensions patch v6 [Checkin: Comment 307]

[Technically you didn't need to rerequest review.]

Comment 306

[[e-mail]]

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

Transferring bienvenu's sr from v2.

Pushed changeset f9f3136382f2 to comm-central.

Comment 307

[[e-mail]]

Comment on attachment 449258 [details] [diff] [review]
extensions patch v6 [Checkin: Comment 307]

Transferring bienvenu's sr from v5.

Pushed changeset 72844f24e851 to comm-central.

Comment 308

[[e-mail]]

Comment on attachment 448498 [details] [diff] [review]
MsgFind patch v2 [Checkin: Comment 308]

Pushed changeset c9e7647fe524 to comm-central.

I renamed case_sensitive to ignore_case since it made more sense that way. I also added a couple of comments.

Comment 309

[[e-mail]]

Comment on attachment 444633 [details] [diff] [review]
Conversion of nsISupportsArray to nsIMutable array in ListDescendents method

I don't think that this patch is relevant to this particular bug any more.

Comment 310

[[e-mail]]

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

>-#ifdef MOZILLA_INTERNAL_API
>-    if (CaseInsensitiveFindInReadable(needle, utf16StrToMatch))
>-#else
>-    if (utf16StrToMatch.Find(needle, CaseInsensitiveCompare) != -1);
>-#endif
>+    if (utf16StrToMatch.Find(needle, CaseInsensitiveCompare) != kNotFound)
>       result = PR_TRUE;
>     break;
>   case nsMsgSearchOp::DoesntContain:
>-#ifdef MOZILLA_INTERNAL_API
>-    if (!CaseInsensitiveFindInReadable(needle, utf16StrToMatch))
>-#else
>-    if (utf16StrToMatch.Find(needle, CaseInsensitiveCompare) == -1);
>-#endif
>+    if (utf16StrToMatch.Find(needle, CaseInsensitiveCompare) == kNotFound)
I had to back the patch out because this change is wrong - unfortunately nsString::Find is always case sensitive, and we were ending up matching from an offset of 1 by mistake. Oops.

Comment 311

[Jan Horak [:jhorak]]

(In reply to comment #310)
> (From update of attachment 449257 [details] [diff] [review])
> >-#ifdef MOZILLA_INTERNAL_API
> >-    if (CaseInsensitiveFindInReadable(needle, utf16StrToMatch))
> >-#else
> >-    if (utf16StrToMatch.Find(needle, CaseInsensitiveCompare) != -1);
> >-#endif
> >+    if (utf16StrToMatch.Find(needle, CaseInsensitiveCompare) != kNotFound)
> >       result = PR_TRUE;
> >     break;
> >   case nsMsgSearchOp::DoesntContain:
> >-#ifdef MOZILLA_INTERNAL_API
> >-    if (!CaseInsensitiveFindInReadable(needle, utf16StrToMatch))
> >-#else
> >-    if (utf16StrToMatch.Find(needle, CaseInsensitiveCompare) == -1);
> >-#endif
> >+    if (utf16StrToMatch.Find(needle, CaseInsensitiveCompare) == kNotFound)
> I had to back the patch out because this change is wrong - unfortunately
> nsString::Find is always case sensitive, and we were ending up matching from an
> offset of 1 by mistake. Oops.
Um...this seems to be relevant for all nsString::Find((PRUni)char*/nsString, CaseInsensitiveCompare), right? In this case there are more mistakes :( (totally ~ 7 occurrences in addrbook, base/search, base/src, compose, news).
Proposed macros:
INTERNAL
#define MsgFindUTF16(str, needle, comparefunc) \
        (str).Find(needle)
FROZEN
#define MsgFindUTF16(str, needle, comparefunc) \
        (str).Find(needle, comparefunc)

Comment 312

[[e-mail]]

(In reply to comment #311)
> Um...this seems to be relevant for all nsString::Find((PRUni)char*/nsString,
> CaseInsensitiveCompare), right? In this case there are more mistakes :(
> (totally ~ 7 occurrences in addrbook, base/search, base/src, compose, news).
In your patches, you mean? base/search was the only one that "landed" though?

> Proposed macros:
> INTERNAL
> #define MsgFindUTF16(str, needle, comparefunc) \
>         (str).Find(needle)
> FROZEN
> #define MsgFindUTF16(str, needle, comparefunc) \
>         (str).Find(needle, comparefunc)
How about:
INTERNAL
#include "nsUnicharUtils.h"
FROZEN
#define CaseInsensitiveFindInReadable(what, str) \

Comment 313

[Jan Horak [:jhorak]]

Attachment: base/search patch v4

- Fixed problems with nsString::Find(nsString, CaseInsensitiveCompare)

Comment 314

[Jan Horak [:jhorak]]

Attachment: base/util path v8

- Added CaseInsensitiveFindInReadable for frozen API

Comment 315

[Jan Horak [:jhorak]]

Attachment: compose patch v9

- Fixed problems with nsString::Find(nsString, CaseInsensitiveCompare)

Comment 316

[Jan Horak [:jhorak]]

Attachment: address book patch v10 [Checkin: Comment 332]

- Fixed problems with nsString::Find(nsString, CaseInsensitiveCompare)

Comment 317

[Jan Horak [:jhorak]]

Attachment: imap patch v8 [Checkin: Comment 324]

Sorry for nonsence change:
@@ -847,7 +845,7 @@ nsresult nsImapProtocol::SetupWithUrl(ns
           connectionType = "starttls";
 
         nsCOMPtr<nsIProxyInfo> proxyInfo;
-        rv = MsgExamineForProxy("imap", m_realHostName.get(), port, getter_AddRefs(proxyInfo));
+        rv = MsgExamineForProxy("imap", hostName.get(), port, getter_AddRefs(proxyInfo));
         if (NS_FAILED(rv))
           proxyInfo = nsnull;

Comment 318

[Jan Horak [:jhorak]]

Attachment: news patch v7 [Checkin: Comment 333]

- Fixed some MsgGetAtom
- Fixed indentation
- groupName.Find(mSearchValue, CaseInsensitiveCompare) -> CaseInsensitiveFindInReadable(mSearchValue, groupName)

Comment 319

[[e-mail]]

Comment on attachment 450106 [details] [diff] [review]
base/util path v8

>-  while ((i = str.Find(what, i)) != kNotFound)
>+  while ((i = str.Find(what, i, PR_FALSE)) != kNotFound)
This is unnecessary. It only compiles because PR_FALSE is zero, and therefore the compiler casts it to the null comparator function.

> #include "nsEscape.h"
>+#include "nsUnicharUtils.h"
I'm not actually sure we need to include this here, since (unlike nsEscape.h) nsUnicharUtils.h is external API-safe, so the existing includes will suffice.

>+#define MsgFind(str, what, case_sensitive, offset) \
>+        (str).Find(what, case_sensitive, offset)
[Already landed]

>+#define CaseInsensitiveFindInReadable(what, str) \
>+        (str).Find(what, CaseInsensitiveCompare)
I forgot to point out that CaseInsensitiveFindInReadable returns a PRBool (PR_FALSE on failure) but Find returns PRInt32 (kNotFound on failure), so the Find result needs to be compared to kNotFound.

Comment 320

[[e-mail]]

Comment on attachment 450107 [details] [diff] [review]
compose patch v9

>+    PRInt32 pos = sigData.Find(metaCharset.BeginReading(), PR_TRUE);
[Nit: this should probably be .get()]

Comment 321

[[e-mail]]

Comment on attachment 450110 [details] [diff] [review]
news patch v7 [Checkin: Comment 333]

>diff -r c350165913d0 mailnews/news/src/nsNewsDownloader.cpp
>--- a/mailnews/news/src/nsNewsDownloader.cpp	Wed Jun 09 09:55:45 2010 +0100
>+++ b/mailnews/news/src/nsNewsDownloader.cpp	Wed Jun 09 16:00:22 2010 +0200
>@@ -504,7 +504,7 @@ nsresult nsMsgDownloadAllNewsgroups::Adv
>   if (!m_currentServer)
>      rv = AdvanceToNextServer(done);
>   else
>-    rv = m_serverEnumerator->Next();
>+     rv = m_serverEnumerator->Next();  
[Some sort of typo? I don't think you meant to do it!]

Comment 322

[[e-mail]]

Comment on attachment 438723 [details] [diff] [review]
mapi patch v3 [Checkin: Comment 322]

Pushed changeset 12981f9325ed to comm-central.

I botched the checkin comment though, I wrote mime instead of mapi :-(

Comment 323

[[e-mail]]

Comment on attachment 449255 [details] [diff] [review]
base/src patch v7 [Checkin: Comment 323]

Pushed changeset 49782a242a72 to comm-central.

Comment 324

[[e-mail]]

Comment on attachment 450109 [details] [diff] [review]
imap patch v8 [Checkin: Comment 324]

Pushed changeset c2c24182c931 to comm-central.

Comment 325

[Jan Horak [:jhorak]]

Attachment: base/util path v9 [Checkin: Comment 333]

> CaseInsensitiveFindInReadable returns PRBool
Ah, sorry, I missed that.

Comment 326

[Jan Horak [:jhorak]]

(In reply to comment #321)
> (From update of attachment 450110 [details] [diff] [review])
> >diff -r c350165913d0 mailnews/news/src/nsNewsDownloader.cpp
> >--- a/mailnews/news/src/nsNewsDownloader.cpp	Wed Jun 09 09:55:45 2010 +0100
> >+++ b/mailnews/news/src/nsNewsDownloader.cpp	Wed Jun 09 16:00:22 2010 +0200
> >@@ -504,7 +504,7 @@ nsresult nsMsgDownloadAllNewsgroups::Adv
> >   if (!m_currentServer)
> >      rv = AdvanceToNextServer(done);
> >   else
> >-    rv = m_serverEnumerator->Next();
> >+     rv = m_serverEnumerator->Next();  
> [Some sort of typo? I don't think you meant to do it!]
No typo, just fixing indentation with rv = AdvanceToNextServer(done);

Comment 327

[Jan Horak [:jhorak]]

Attachment: base/src fix for wrong Find

Found one another wrong Find usage.

Comment 328

[[e-mail]]

Comment on attachment 450340 [details] [diff] [review]
base/src fix for wrong Find

>-           (uriString.Find(NS_LITERAL_STRING(".eml?"), CaseInsensitiveCompare) != -1))
>+           (CaseInsensitiveFindInReadable(uriString, NS_LITERAL_STRING(".eml?"))))
"what" argument comes first, no?

Comment 329

[Jan Horak [:jhorak]]

Attachment: base/src fix for wrong Find v2 [Checkin: Comment 333]

>>-           (uriString.Find(NS_LITERAL_STRING(".eml?"), CaseInsensitiveCompare) != -1))
>>+           (CaseInsensitiveFindInReadable(uriString, NS_LITERAL_STRING(".eml?"))))
>"what" argument comes first, no?
Um, yes, you're right. Sorry.

Comment 330

[[e-mail]]

Comment on attachment 450349 [details] [diff] [review]
base/src fix for wrong Find v2 [Checkin: Comment 333]

>   else if (StringBeginsWith(uriString, NS_LITERAL_STRING("mailbox:")) &&
>-           (uriString.Find(NS_LITERAL_STRING(".eml?"), CaseInsensitiveCompare) != -1))
>+           (CaseInsensitiveFindInReadable(NS_LITERAL_STRING(".eml?"), uriString)))
[Nit: we probably don't need one set of ()s.]

Comment 331

[[e-mail]]

Comment on attachment 450336 [details] [diff] [review]
base/util path v9 [Checkin: Comment 333]

> /**
>+ * CaseInsensitiveFindInReadable is not defined in frozen API, 
>+ * redirect calls to ns(C)String::Find 
[I think nsCString::Find works, it's nsString::Find that's weird.
 There are actually three versions of CaseInsensitiveFindInReadable
 but we're only interested in the (nsString, nsString) one,
 as the other two don't have equivalents in frozen API anyway.
 In fact I would be quite happy to lose the comment and move the
 define to, say, just after the one for Compare.]

Comment 332

[[e-mail]]

Comment on attachment 450108 [details] [diff] [review]
address book patch v10 [Checkin: Comment 332]

Pushed changeset 86fd73da95aa to comm-central.

Comment 333

[[e-mail]]

Pushed changeset 606f1ecc69d9 to comm-central, covering attachments 450336, 450349 and 450110 as I couldn't be bothered to commit them separately.

Comment 334

[[e-mail]]

Comment on attachment 450108 [details] [diff] [review]
address book patch v10 [Checkin: Comment 332]

>         case nsIAbBooleanConditionTypes::Contains:
>+            *matchFound = CaseInsensitiveFindInReadable(matchValue, value);
>             break;
>         case nsIAbBooleanConditionTypes::DoesNotContain:
>+            *matchFound = CaseInsensitiveFindInReadable(matchValue, value);
>             break;
Whoops!

Comment 335

[Jan Horak [:jhorak]]

Attachment: address book patch fix [Checkin: Comment 341]

Ouch! Sorry for such invisible bugs! :(

Comment 336

[[e-mail]]

(In reply to comment #335)
> Ouch! Sorry for such invisible bugs! :(
If you thought that was bad, MsgFindCharInSet has at least three bugs, but nobody noticed until your imap patch happened to trigger one of them :-( See bug 572065 for the gory details.

Comment 337

[David :Bienvenu]

Attachment: log of failures when running xpcshell tests with compose-frozen patch

I see a bunch of xcpshell test failures in the compose directory when running with the compose-frozen patch. I tried several times w and w/o the patch, and the failures were consistent w/ the patch, and success was repeatable w/o the patch.

Comment 338

[David :Bienvenu]

Comment on attachment 450107 [details] [diff] [review]
compose patch v9

minusing because of test failures.

Comment 339

[[e-mail]]

Attachment: compose patch v9a [Checkin: Comment 342]

There was a silly typo where we were comparing a PRBool >= 0.

Comment 340

[David :Bienvenu]

Comment on attachment 452264 [details] [diff] [review]
compose patch v9a [Checkin: Comment 342]

much better, thanks.

Comment 341

[[e-mail]]

Comment on attachment 451583 [details] [diff] [review]
address book patch fix [Checkin: Comment 341]

Pushed changeset 4e969d819c3a to comm-central.

Comment 342

[[e-mail]]

Comment on attachment 452264 [details] [diff] [review]
compose patch v9a [Checkin: Comment 342]

Pushed changeset cd4402569153 to comm-central.

Comment 343

[Jan Horak [:jhorak]]

Attachment: local patch v6

Unbitrotting patch, please have a look.

Comment 344

[[e-mail]]

Comment on attachment 452703 [details] [diff] [review]
local patch v6

Looks good, and also passes all tests locally [pun] so sr should be quick ;-)

>diff -r 59be27734452 mailnews/local/src/nsLocalMailFolder.cpp
>-    NS_NewISupportsArray(getter_AddRefs(allDescendents));
>+    allDescendents = do_CreateInstance(NS_SUPPORTSARRAY_CONTRACTID, &rv);
Nit: nsMsgUtils.h has a macro for this now. Also you don't use rv.

>-      nsCOMPtr<nsISupports> supports = dont_AddRef(allDescendents->ElementAt(i));
>+      nsCOMPtr<nsISupports> supports = do_QueryElementAt(allDescendents, i);
>       nsCOMPtr<nsIMsgFolder> folder = do_QueryInterface(supports, &rv);
[Not sure why you changed this, although given that we have do_QueryElementAt
 we might as well query into folder directly i.e.
 nsCOMPtr<nsIMsgFolder> folder = do_QueryElementAt(allDescendents, i);
 although you would have to s/supports/folder/ later on in the method.]

>diff -r 59be27734452 mailnews/local/src/nsRssIncomingServer.cpp
>+#include "nsArrayUtils.h"
>+#include "nsIMutableArray.h"
Part of another patch?

>-  NS_NewISupportsArray(getter_AddRefs(allDescendents));
>+  allDescendents = do_CreateInstance(NS_SUPPORTSARRAY_CONTRACTID, &rv);
[2x, as nsLocalMailFolder above.]

>-    supports = getter_AddRefs(allDescendents->ElementAt(index));
>+    supports = do_QueryElementAt(allDescendents, index);
>     rssFolder = do_QueryInterface(supports, &rv);
[2x, as nsLocalMailFolder above, these lines could be replaced with
 rssfolder = do_QueryElementAt(allDescendents, index, &rv);
 and the supports variable could then be completely removed.]

Comment 345

[Robert Kaiser]

I don't want to disturb that work flow of you guys, but this bug is becoming somewhat cumbersome if wanting to try how far we get oneself. Could you mark all patches that have been superseeded as obsolete? It's hard right now to see what has landed, and what is still needed to do or needs work - the third category, i.e. what's under review, is findable well enough though. :)

Comment 346

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

It looks like attachment 450105 [details] [diff] [review], attachment 418828 [details] [diff] [review] and attachment 418831 [details] [diff] [review] still need to be checked in. (Sorry for the spam.)

Comment 347

[David :Bienvenu]

Comment on attachment 452703 [details] [diff] [review]
local patch v6

while you're here, can you fix this comment? It should just say:

// save dir name before appending

instead of 
+  nsAutoString newNameDirStr = safeName;  //save of dir name before appending .msf

Comment 348

[Jan Horak [:jhorak]]

Attachment: local patch v7

Fixed local patch. Not asking for review as long as it has positive review.

Comment 349

[Ludovic Hirlimann [:Usul]]

(In reply to comment #348)
> Created attachment 459775 [details] [diff] [review]
> local patch v7
> 
> Fixed local patch. Not asking for review as long as it has positive review.

checking needed maybe ?

Comment 350

[[e-mail]]

Comment on attachment 459775 [details] [diff] [review]
local patch v7

Pushed changeset 96a50f5a6b2d to comm-central.

Comment 351

[[e-mail]]

Comment on attachment 459775 [details] [diff] [review]
local patch v7

>-      nsCOMPtr<nsISupports> supports = dont_AddRef(allDescendents->ElementAt(i));
>-      nsCOMPtr<nsIMsgFolder> folder = do_QueryInterface(supports, &rv);
>+      nsCOMPtr<nsIMsgFolder> folder = do_QueryElementAt(allDescendents, i);
>       NS_ENSURE_SUCCESS(rv, rv);
Only after checking this in do I notice that the &rv seems to be missing :-(

Comment 352

[Mark Banner (:standard8)]

When adding check-in needed please specify which patches (in the whiteboard) are to be checked in - the status of patches on this bug makes it very difficult to work out.

Comment 353

[Jan Horak [:jhorak]]

Attachment: fixed local patch, &rv put back

Comment 354

[[e-mail]]

OK, so having pushed changesets 63f1c7a41b6d and 1852960eed7c to comm-central, I really think that this it's about time we resolve this bug.

Comment 355

[Jan Horak [:jhorak]]

There is still some work to do on import patch. I don't have Windows and Mac machine available so I can't contribute much. It would be nice to cooperate with some Win and Mac developer (Matthew Gertner?).

Comment 356

[[e-mail]]

(In reply to comment #138)
> (From update of attachment 418822 [details] [diff] [review])
> This patch looks ready for checkin, is it not?
That was far too long ago, I must have overlooked it :-(

Comment 357

[[e-mail]]

Oh, and in case you were wondering, I pushed changeset 8ab94cb1a3c2

Comment 358

[Jens Hatlak (:InvisibleSmiley)]

This probably caused bug 588006.