395701 - mailnews should use nsStringGlue and other Glue code

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Prasad Sunkari [:prasad]]

As part of migrating to the frozen linkages, the mailnews code should start using glue code.  This will allow easy testing and will make migration easy too.

It mostly involves using nsStringGlue.h instead of nsString.h and nsReadableUtils.h, using nsCRTGlue.h instead of nsCRT.h etc, and getting the code to compile both with MOZILLA_INTERNAL_API turned on, and off.

Comment 1

[Prasad Sunkari [:prasad]]

Attachment: Migrates addrbook code to glue code

This patch migrates addrbook to glue. All files except those that use hash tables compile well with and without MOZILLA_INTERNAL_API.  The rest of it can be done when the hash table migration happens.

In places where I could not find common API in both internal and frozen I used "#ifdef MOZILLA_INTERNAL_API" to separate the code.

Comment 2

[Mark Banner (:standard8)]

(In reply to comment #1)
> Created an attachment (id=280380) [details]
> Migrates addrbook code to glue code

If you were asking for review from me on this, then I've changed my email address. If not, you forgot to add someone ;-)

> In places where I could not find common API in both internal and frozen I used
> "#ifdef MOZILLA_INTERNAL_API" to separate the code.

For the nsCaseInsensitiveStringComparator() to CaseInsensitiveCompare change, I think it would be much better if you could get a macro defined for the internal API case:

#define CaseInsensitiveCompare nsCaseInsensitiveStringComparator()

I did this in xpfe/components/bookmarks quite successfully and only had to remove the macro once the conversion was done.

That way we can avoid cluttering up the code with ifdefs (even temporarily).

Don't know if we'd be able to get this into nsStringGlue.h, otherwise a mailnews header file (or two) would probably be ok.

Comment 3

[Prasad Sunkari [:prasad]]

Comment on attachment 280380 [details] [diff] [review]
Migrates addrbook code to glue code

Yes Mark, I wanted to ask you for review.  I will update the patch to make the Comparator change a macro.  Please go through it and tell me if you need any modifications in the rest of the patch.

Comment 4

[Prasad Sunkari [:prasad]]

(In reply to comment #2)
> 
> For the nsCaseInsensitiveStringComparator() to CaseInsensitiveCompare change, I
> think it would be much better if you could get a macro defined for the internal
> API case:
> 
> #define CaseInsensitiveCompare nsCaseInsensitiveStringComparator()
> 

I think it will not be this straight forward.  CaseInsensitiveCompare in frozen API can mean either nsCaseInsensitiveStringComparator or nsCaseInsensitiveCStringComparator (for nsString and nsCString respectively).  

Apart from that we could still need "#ifdef MOZILLA_INTERNAL_API" for cases where FindInReadable is used in internal linkage.  This function does not exist in frozen linkage... and the corresponding ns(C)String::Find is different in frozen and internal linkages.

This has been my understanding, correct me if I am wrong.

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 280380 [details] [diff] [review]
Migrates addrbook code to glue code

Thanks for starting work on this.

When I disable internal API on just the mailnews/addrbook/src/Makefile.in and try and build I get:

../../../dist/include/xpcom/nsProxiedService.h: In member function ‘void nsProxiedService::InitProxy(nsISupports*, const nsIID&, nsIEventTarget*, PRBool, nsresult*)’:
../../../dist/include/xpcom/nsProxiedService.h:132: error: ‘NS_GetProxyForObject’ was not declared in this scope

Were you expecting this? (btw I commented out the files you hadn't changed, its just this one seemed weird).

Index: mailnews/addrbook/src/nsAbAddressCollecter.cpp

+#include "nsIAddressBook.h"
+#include "nsServiceManagerUtils.h"
+#include "nsComponentManagerUtils.h"

You don't need to add nsIAddressBook.h

-  email.Right(domain, aEmail.Length() - (atPos + 1));
+  domain = StringTail(email, aEmail.Length() - (atPos + 1));

This should be better:

nsCString domain(SubString(email, atPos + 1));

(though please double check I've got my sums right ;-) )

     nsCString userName;
-    email.Left(userName, atPos);
+    userName = StringHead(email, atPos);

May as well do the string head at the same time as initialising the nsCString.

 nsresult nsAbAddressCollecter::SplitFullName(const nsACString &aFullName, nsACString &aFirstName, nsACString &aLastName)
 {
   nsCString lastName;
   nsCString firstName;
 
   int index = nsCString(aFullName).RFindChar(' ');
   if (index != -1) 
   {
-    nsCString(aFullName).Right(lastName, aFullName.Length() - (index + 1));
-    nsCString(aFullName).Left(firstName, index);
+    lastName = StringTail(aFullName, aFullName.Length() - (index + 1));
+    firstName = StringHead(aFullName, index);
  
     aLastName = lastName;
     aFirstName = firstName;

Seems to be you could drop lastName and firstName altogether here and just assign directly to aLastName and aFirstName.

Note that probably better to use SubString(); again.


   nsString xmlStr;
-  xmlStr.SetCapacity(4096); // to reduce allocations. should be enough for most cards
+  // No replacement in the frozen API -- [e-mail]
+  // xmlStr.SetCapacity(4096); // to reduce allocations. should be enough for most cards

How about .SetLength() ?

+                // see if the string is already present in the array
+                int i = 0, found = -1;
+                while ( i < aAttrs->Count() ) {
+#ifdef MOZILLA_INTERNAL_API
+                    if( aAttrs->CStringAt(i)->Equals(attrName, nsCaseInsensitiveCStringComparator()) ) {
+#else
+                    if( aAttrs->CStringAt(i)->Equals(attrName, CaseInsensitiveCompare) ) {
+#endif
+                        found = i;
+                        break;
+                    }
+                }

There's an ++i missing in this loop. Also it should be if (aAttrs.

+    outputLine2 = StringTail(inputAddress, inputAddress.Length() - (crlfPos + 2));

Use Substring again please.

+  email = StringTail(valueString, valueString.Length() - emailPos);

ditto

+      if ((fileName.Find(LDIF_FILE_EXTENSION, fileName.Length() - strlen(LDIF_FILE_EXTENSION), PR_TRUE) == -1) && 
+         (fileName.Find(LDIF_FILE_EXTENSION2, fileName.Length() - strlen(LDIF_FILE_EXTENSION2), PR_TRUE) == -1)) {
+

the second line should be indented by one more space.

+      if( (fileName.Find(TXT_FILE_EXTENSION, fileName.Length() - strlen(TXT_FILE_EXTENSION), PR_TRUE) == -1) && 
+          (fileName.Find(TAB_FILE_EXTENSION, fileName.Length() - strlen(TAB_FILE_EXTENSION), PR_TRUE) == -1)) {

Should be if ((

+  nsVoidArray *newList = NULL;

Should be nsnull.

Comment 6

[Prasad Sunkari [:prasad]]

Attachment: Migrates addrbook to glue code - v2

Mark, made the changes as suggested by you.  The error with nsProxiedService is expected when building with internal API.  The rest of files will compile too once nsHashtable migration gets done.

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 283538 [details] [diff] [review]
Migrates addrbook to glue code - v2

Thanks Prasad, that's better (though I do normally prefer a unified patch, but I managed this time).

Comment 8

[Prasad Sunkari [:prasad]]

Attachment: [checked in]Migrates addrbook to glue code - v2

Attaching the patch reviewed by Mark in unified format.
Scott, can you superreview it and commit the patch.

Comment 9

[Scott MacGregor]

Comment on attachment 284139 [details] [diff] [review]
[checked in]Migrates addrbook to glue code - v2

thanks for doing this prasad.

I'll try to land this over the weekend if someone doesn't beat me to it.

Comment 10

[Scott MacGregor]

Comment on attachment 284139 [details] [diff] [review]
[checked in]Migrates addrbook to glue code - v2

Prasad, I've checked this into the trunk. Thanks for working on this!

Comment 11

[Prasad Sunkari [:prasad]]

I am taking up the work with mailnews/base/search.
If things turn out not obvious, I will do it as part of another Bug.

Comment 12

[Prasad Sunkari [:prasad]]

Attachment: Migrate mailnews/base/search to glue code

This patch also has minor/small changes in compose, base and base/util

Comment 13

[Prasad Sunkari [:prasad]]

I am taking up the mailnews/base/util.
Again, if there is something difficult, I will post a new bug on it.

Comment 14

[Magnus Melin [:mkmelin]]

Just an uninformed question, but shouldn't FindInReadable be preferred over Find?

Comment 15

[Mark Banner (:standard8)]

(In reply to comment #14)
> Just an uninformed question, but shouldn't FindInReadable be preferred over
> Find?
In core code, probably yes, in code linking to the frozen api no - FindInReadable is not part of the frozen api/xpcom glue interface.

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 291832 [details] [diff] [review]
Migrate mailnews/base/search to glue code

Thanks for the patch, needs some changes though.

-  pBufInOut.ReplaceChar("\n\r",' '); // Searching does not like EOLs
+
+  // Searching does not like EOLs
+  PRInt32 match = 0;
+  PRUint32 offset = 0;
+  nsCString oldSubstr = NS_LITERAL_CSTRING("\n\r");
+  nsCString newSubstr = NS_LITERAL_CSTRING(" "); 
+  while (offset < pBufInOut.Length()) {
+    match = pBufInOut.Find(oldSubstr, offset);
+    if (match == -1)
+        break;
+
+    pBufInOut.Replace(offset + match, oldSubstr.Length(), newSubstr);
+    offset += (match + newSubstr.Length());
+  }

This version of ReplaceChar is working the characters as a set not a substring.
So you probably want to do something more like:
+  offset = val.FindChar(13);
+  while (offset != -1) {
+    val.Replace(offset, 1, ',');
+    offset = val.FindChar(13);
+  }
+  offset = val.FindChar(10);
+  while (offset != -1) {
+    val.Replace(offset, 1, ',');
+    offset = val.FindChar(10);
+  }

       nsCAutoString originalServerPath;
-      moveValue.Mid(originalServerPath, prefixLen, moveValue.Length() - prefixLen);
+      originalServerPath = Substring(moveValue, prefixLen, moveValue.Length() - prefixLen);

Please initialise originalServerPath on the one line, rather than doing it in two stages.

-        moveValue.ReplaceSubstring(".sbd/", "/");
+        moveValue.Replace(moveValue.Find(".sbd/"), 5, "/");

Although this looks correct, if .sbd/ isn't found, that will call replace with -1, so we'll actually append '/' to the end of the string - the old ReplaceSubstring didn't do that. Please also use single quotes for the char as then it doesn't have to work out the length.

         nsString filterName;
         rv = filter->GetFilterName(filterName);
         if (NS_FAILED(rv)) return rv;
 
         nsAutoString filterString(filterName);
 
         char *utf8Name = ToNewUTF8String(filterString);
         filterUri.Append(utf8Name);
-        Recycle(utf8Name);
+        nsMemory::Free(utf8Name);

Drop filterString & utf8Name and you can just do:

filterUri.Append(NS_ConvertUTF16toUTF8(filterString));

That'll save on all the unnecessary allocations going on.

       nsCString folderCharset;
       folder->GetCharset(folderCharset);
+#ifdef MOZILLA_INTERNAL_API
       AppendASCIItoUTF16(folderCharset, dstCharset);
+#else
+      dstCharset.AppendLiteral(folderCharset.get());
+#endif

This isn't right, you shouldn't be using .get and then appending it as a literal as it isn't. This would be better:

dstCharset.Append(NS_ConvertASCIItoUTF16(folderCharset));

and then you can drop the #ifdef from this section as well.

+
+#include "nsServiceManagerUtils.h"
+#include "nsComponentManagerUtils.h"

nit: can you drop the blank line before the includes you've added. I see no reason for it.

-      buf.Left(curMsgHeader, colonPos);
+      curMsgHeader = Substring(buf, 0, colonPos);

StringHead would be better used here.

-          PRInt32 nextFilterStartPos = m_unparsedFilterBuffer.RFind("name");
+          PRBool foundNextFilterStartPos = PR_FALSE;
+          PRInt32 nextFilterStartPos = 0;
+          while (!foundNextFilterStartPos) { 
+            nextFilterStartPos = m_unparsedFilterBuffer.RFindChar('n');
+            if (m_unparsedFilterBuffer.CharAt(nextFilterStartPos + 1) == 'a' &&
+                m_unparsedFilterBuffer.CharAt(nextFilterStartPos + 2) == 'm' &&
+                m_unparsedFilterBuffer.CharAt(nextFilterStartPos + 3) == 'e')
+              foundNextFilterStartPos = PR_TRUE;
+          }

This isn't going to work - if name doesn't exist, you'll get into an infinite loop, and if there's an "n" after the "name" its going to loop as well.

The current suggestion is something like:

function findName(buf) { var n = buf.Length - 4; while (n >= 0 && buf.substr(n, 4) != "name") switch (buf[n]) { case 'e': n -= 3; break; case 'm': n -= 2; break; case 'a': n--; default: n -=4; break; } return n; }

Obviously tidied up and checked through ;-)

Comment 17

[Prasad Sunkari [:prasad]]

Attachment: Updated patch for mailnews/base/search

Mark, I updated the patch as per your comments.  Sorry for the previous version of the patch - I should have done a proper review myself.

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 292753 [details] [diff] [review]
Updated patch for mailnews/base/search

+  int offset = pBufInOut.FindChar('\n');

nit: should be PRInt32, there's a couple of other instances of this as well.

-          PRInt32 nextFilterStartPos = m_unparsedFilterBuffer.RFind("name");
+          PRInt32 nextFilterStartPos = m_unparsedFilterBuffer.Length() - 4;
+          while (nextFilterStartPos >=0 && 
+                 Substring(m_unparsedFilterBuffer, nextFilterStartPos, 4) != NS_LITERAL_CSTRING("name")) {
+            switch(m_unparsedFilterBuffer[nextFilterStartPos]) {

Can you just add a short comment that we're reverse finding "name" just to make it easily understandable?

r=me with those fixed. Thanks for working on this.

Comment 19

[Prasad Sunkari [:prasad]]

Attachment: Updated patch for mailnews/base/search

Updated as per Mark's comments.  r=mark

Comment 20

[Prasad Sunkari [:prasad]]

There seems to be some problem with the way the first patch was committed to trunk.  The patch had changes to nsMsgRDFUtils.h and they are not visible on the trunk! Was that deliberate or a mistake?

I have the same change in the second patch too.

Comment 21

[Prasad Sunkari [:prasad]]

Attachment: addrbook migration to frozen linkage

This patch makes mailnews/addrbook compile with frozen linkage on Linux. This patch depends on bug 408808.

Comment 22

[Prasad Sunkari [:prasad]]

Attachment: addrbook migration to frozen linkage - v2

Updated the patch.  
The patch now uses do_GetService to get a proxy object manager.

Comment 23

[Prasad Sunkari [:prasad]]

Attachment: addrbook migration to frozen linkage - Fixed a nit

Fixed a nit.
Missed a nsComponentManagerUtils.h in one of the files.

Comment 24

[David :Bienvenu]

Comment on attachment 293244 [details] [diff] [review]
Updated patch for mailnews/base/search

why get rid of kNotFound? Is it not defined?

Comment 25

[Prasad Sunkari [:prasad]]

(In reply to comment #24)
> (From update of attachment 293244 [details] [diff] [review])
> why get rid of kNotFound? Is it not defined?
> 

Yes, it is not defined in the frozen linkage.

Comment 26

[Prasad Sunkari [:prasad]]

Attachment: addrbook migration to frozen linkage - v3

Mark, sorry for these silly mistakes.  Added a couple of missed lines.

Comment 27

[Mark Banner (:standard8)]

Comment on attachment 294048 [details] [diff] [review]
addrbook migration to frozen linkage - v3

-    tempLDAPURL.ReplaceSubstring("moz-abldapdirectory:", "ldap:");
+    PRInt32 offset = tempLDAPURL.Find("moz-abldapdirectory:");
+    while (offset != -1) {
+      tempLDAPURL.Replace(offset, 20, "ldap:");
+      offset = tempLDAPURL.Find("moz-abldapdirectory:", offset);
+    }

Although that's the correct conversion, I don't think we really need to do it.

I think using doing:

if (StringBeginsWith(tempLDAPURL, NS_LITERAL_CSTRING(kLDAPDirectoryRoot)))
  tempLDAPURL.Replace(0, 20, NS_LITERAL_CSTRING("ldap:"));

Index: mailnews/addrbook/src/nsAbLDAPReplicationData.cpp

This file needs 

+#include "nsXPCOMCIDInternal.h"

adding to it.

r=me with those fixed.

Comment 28

[Prasad Sunkari [:prasad]]

Mark, I did not really think about the first change, but yes, i will make the change you asked for.

About the second comment, nsXPCOMCIDInternal is already included in nsProxiedService.h (I will include it in nsAbLDAPreplicationData.cpp anyway because including it twice will not make any difference)

Will submit the updated patch for super-review.

Comment 29

[Prasad Sunkari [:prasad]]

Attachment: [checked in] addrbook migration to frozen linkage - updated as per mark's comments

Updated the patch as per Mark's comments.

Comment 30

[David :Bienvenu]

There's really no way to do a reverse find in the frozen linkages, other than rolling your own? If that's the case, then we need to write our own helper routine to do this, since we do RFinds all over the place.

Comment 31

[David :Bienvenu]

Comment on attachment 293244 [details] [diff] [review]
Updated patch for mailnews/base/search

sr=bienvenu, thx for the patch, Prasad.

Comment 32

[David :Bienvenu]

Comment on attachment 294228 [details] [diff] [review]
[checked in] addrbook migration to frozen linkage - updated as per mark's comments

thx for the patch, Prasad.

Comment 33

[Prasad Sunkari [:prasad]]

Mark, can you check-in the above two for me?
Thanks.

Comment 34

[Reed Loden [:reed]]

(In reply to comment #33)
> Mark, can you check-in the above two for me?

Does bug 408808 still block the check-in of at least one of the patches, as per comment #21? If so, which one (if any) can be committed now?

Comment 35

[Prasad Sunkari [:prasad]]

the later one, attachment 294228 [details] [diff] [review] depends on bug 408808.  however, the bug 408808 is blocking for frozen linkage builds (which currently don't happen).

we can wait for bug 408808 to be resolved :)

Comment 36

[Benjamin Smedberg]

If you need RFind in the glue I'm sure it's not hard to implement.

Comment 37

[Mark Banner (:standard8)]

(In reply to comment #35)
> the later one, attachment 294228 [details] [diff] [review] depends on bug 408808.  however, the bug
> 408808 is blocking for frozen linkage builds (which currently don't happen).
> 
> we can wait for bug 408808 to be resolved :)
> 
I've just checked this patch (attachment 294228 [details] [diff] [review]) in - as bug 408808 doesn't stop this patch building and working on current trunk. Additionally, checking it in means I don't bitrot it whilst we're waiting for resolution on bug 408808.

Comment 38

[Mark Banner (:standard8)]

(In reply to comment #19)
> Created an attachment (id=293244) [details]
> Updated patch for mailnews/base/search

(In reply to comment #30)
> There's really no way to do a reverse find in the frozen linkages, other than
> rolling your own? If that's the case, then we need to write our own helper
> routine to do this, since we do RFinds all over the place.

(In reply to comment #36)
> If you need RFind in the glue I'm sure it's not hard to implement.

Prasad, what do you want to do about this patch? Should we implement RFind in the glue before checking it in and hence save ourselves having to work it back again later?

Comment 39

[David :Bienvenu]

If Prasad is up for implementing RFind in glue, that would be the way to go, I think.

Comment 40

[Prasad Sunkari [:prasad]]

Okay, I will implement the RFind in glue and we will update the patch accordingly.

Comment 41

[Prasad Sunkari [:prasad]]

Attachment: Migrate mailnews/base/search to glue code - v3

Updated to use the new RFind from frozen linkage.  The old patch was reviewed by Standard8 and super-reviewed by Bienvenu.  Requesting a review from Standard8 for any comments.

Comment 42

[Prasad Sunkari [:prasad]]

Attachment: Migrate mailnews/base/search to glue code - v3 (full patch)

Fixed a nit in the previous patch.  Included a file that got missed in creating the patch.

Comment 43

[Mark Banner (:standard8)]

Comment on attachment 316230 [details] [diff] [review]
Migrate mailnews/base/search to glue code - v3 (full patch)

looks good. r=me

Comment 44

[Prasad Sunkari [:prasad]]

Comment on attachment 316230 [details] [diff] [review]
Migrate mailnews/base/search to glue code - v3 (full patch)

Neil, can you super-review it. I changed the patch a little after the previous super-review by bienvenu.

Also, do I need an approval to get this patch checked-in?  It does not change any content outside the mailnews/base folder

Comment 45

[Mark Banner (:standard8)]

(In reply to comment #44)
> (From update of attachment 316230 [details] [diff] [review])
> Also, do I need an approval to get this patch checked-in?  It does not change
> any content outside the mailnews/base folder

You don't need approval at the moment. After Tuesday, you'll need approval for the code freeze, the wiki page here http://wiki.mozilla.org/Thunderbird:Thunderbird_3.0a1 should detail more in the next day or so.

Comment 46

[[e-mail]]

Comment on attachment 316230 [details] [diff] [review]
Migrate mailnews/base/search to glue code - v3 (full patch)

>+#ifdef MOZILLA_INTERNAL_API
>   if (StringBeginsWith(line, NS_LITERAL_CSTRING("Content-Transfer-Encoding:"),
>       nsCaseInsensitiveCStringComparator()) &&
>-      line.Find("base64", PR_TRUE) != kNotFound)
>+      line.Find("base64", PR_TRUE) != -1)
This line is inside the #ifdef so technically you don't actually need to change it. (You could move it outside the #ifdef but then it might be less readable.)

>     nsCString buffer;
>     // this is big enough to hold a log entry.
>     // do this so we avoid growing and copying as we append to the log.
>-    buffer.SetCapacity(512);
>+    buffer.SetLength(512);
This won't work, because it will create a string whose length is already 512. Unless perhaps you change the first append to buffer into an assignment.

>+      nsCAutoString originalServerPath(Substring(moveValue, prefixLen, moveValue.Length() - prefixLen));
Isn't there a two-argument version of Substring that defaults to the rest of the string?

>+        PRInt32 offset = moveValue.Find(".sdb/");
.sbd

>+    nsMemory::Free(m_value.string);
Nit: NS_Free

>-    // When we move to frozen linkage, this should be:
>-    // utf16StrToMatch.Find(needle, CaseInsensitiveCompare)  >= 0);
>+#ifdef MOZILLA_INTERNAL_API
>     if (CaseInsensitiveFindInReadable(needle, utf16StrToMatch))
>+#else
>+    if (utf16StrToMatch.Find(needle, CaseInsensitiveCompare)  >= 0);
Actually I'd prefer != -1 (and I'd only use one space too!)

>+#ifdef MOZILLA_INTERNAL_API
>     if(needle.Equals(utf16StrToMatch, nsCaseInsensitiveStringComparator()))
>+#else
>+    if(needle.Equals(utf16StrToMatch, CaseInsensitiveCompare))
>+#endif
One trick which someone used before which you might consider was this:
#ifdef MOZILLA_INTERNAL_API
#define CaseInsensitiveCompare nsCaseInsensitiveStringComparator()
#endif

Comment 47

[Prasad Sunkari [:prasad]]

Attachment: Migrate mailnews/base/search to glue code - updated as per Neil's comments

Updated the patch as per the comments by Neil. r=Standard8

The only thing I did not do is:
#define CaseInsensitiveCompare nsCaseInsensitiveStringComparator()

I considered doing it in one of my earlier patches, but it wasn't really clean because there were cases where nsCaseInsensitiveComparator was used with FindInReadable.

Comment 48

[[e-mail]]

Comment on attachment 317059 [details] [diff] [review]
Migrate mailnews/base/search to glue code - updated as per Neil's comments

>     // this is big enough to hold a log entry.
>     // do this so we avoid growing and copying as we append to the log.
>-    buffer.SetCapacity(512);
So, sadly, my suggestion doesn't actually work... perhaps you should #ifdef it out and file a bug on exposing external API for it?

>         // need to remove ".sbd" from moveValue, and perhaps escape it.
>-        moveValue.ReplaceSubstring(".sbd/", "/");
>+        PRInt32 offset = moveValue.Find(".sbd/");
>+        if (offset != -1)
>+          moveValue.Replace(offset, 5, "/");
moveValue.Cut(offset, 4); perhaps?

Comment 49

[Prasad Sunkari [:prasad]]

Attachment: [checked in] Migrate mailnews/base/search to glue code - updated as per Neil's comments

Needs Checkin.
r=Standard8, sr=Neil

Comment 50

[[e-mail]]

Comment on attachment 317059 [details] [diff] [review]
Migrate mailnews/base/search to glue code - updated as per Neil's comments

>-    buffer.SetCapacity(512);
>+    buffer.SetLength(512);

>-    buffer += NS_ConvertUTF16toUTF8(filterLogDetectStr);
>+    buffer = NS_ConvertUTF16toUTF8(filterLogDetectStr);
Actually this might just work if you use
CopyUTF16toUTF8(filterLogDetectStr, buffer);
as it's possible that the copy won't shrink the buffer but just set the length.

You'd need to verify this in a debugger somehow; probably not worth the effort.

Comment 51

[Mark Banner (:standard8)]

Prasad, I'll leave you to look at comment 50 and submit a follow-up patch if necessary.

I've checked the patch in:

Checking in mailnews/base/search/public/nsMsgSearchScopeTerm.h;
/cvsroot/mozilla/mailnews/base/search/public/nsMsgSearchScopeTerm.h,v  <--  nsMsgSearchScopeTerm.h
new revision: 1.18; previous revision: 1.17
done
Checking in mailnews/base/search/src/nsMsgBodyHandler.cpp;
/cvsroot/mozilla/mailnews/base/search/src/nsMsgBodyHandler.cpp,v  <--  nsMsgBodyHandler.cpp
new revision: 1.33; previous revision: 1.32
done
Checking in mailnews/base/search/src/nsMsgFilter.cpp;
/cvsroot/mozilla/mailnews/base/search/src/nsMsgFilter.cpp,v  <--  nsMsgFilter.cpp
new revision: 1.124; previous revision: 1.123
done
Checking in mailnews/base/search/src/nsMsgFilterDataSource.cpp;
/cvsroot/mozilla/mailnews/base/search/src/nsMsgFilterDataSource.cpp,v  <--  nsMsgFilterDataSource.cpp
new revision: 1.28; previous revision: 1.27
done
Checking in mailnews/base/search/src/nsMsgFilterDelegateFactory.cpp;
/cvsroot/mozilla/mailnews/base/search/src/nsMsgFilterDelegateFactory.cpp,v  <--  nsMsgFilterDelegateFactory.cpp
new revision: 1.20; previous revision: 1.19
done
Checking in mailnews/base/search/src/nsMsgFilterList.cpp;
/cvsroot/mozilla/mailnews/base/search/src/nsMsgFilterList.cpp,v  <--  nsMsgFilterList.cpp
new revision: 1.114; previous revision: 1.113
done
Checking in mailnews/base/search/src/nsMsgImapSearch.cpp;
/cvsroot/mozilla/mailnews/base/search/src/nsMsgImapSearch.cpp,v  <--  nsMsgImapSearch.cpp
new revision: 1.52; previous revision: 1.51
done
Checking in mailnews/base/search/src/nsMsgLocalSearch.cpp;
/cvsroot/mozilla/mailnews/base/search/src/nsMsgLocalSearch.cpp,v  <--  nsMsgLocalSearch.cpp
new revision: 1.83; previous revision: 1.82
done
Checking in mailnews/base/search/src/nsMsgSearchAdapter.cpp;
/cvsroot/mozilla/mailnews/base/search/src/nsMsgSearchAdapter.cpp,v  <--  nsMsgSearchAdapter.cpp
new revision: 1.97; previous revision: 1.96
done
Checking in mailnews/base/search/src/nsMsgSearchNews.cpp;
/cvsroot/mozilla/mailnews/base/search/src/nsMsgSearchNews.cpp,v  <--  nsMsgSearchNews.cpp
new revision: 1.47; previous revision: 1.46
done
Checking in mailnews/base/search/src/nsMsgSearchSession.cpp;
/cvsroot/mozilla/mailnews/base/search/src/nsMsgSearchSession.cpp,v  <--  nsMsgSearchSession.cpp
new revision: 1.66; previous revision: 1.65
done
Checking in mailnews/base/search/src/nsMsgSearchTerm.cpp;
/cvsroot/mozilla/mailnews/base/search/src/nsMsgSearchTerm.cpp,v  <--  nsMsgSearchTerm.cpp
new revision: 1.157; previous revision: 1.156
done
Checking in mailnews/base/search/src/nsMsgSearchValue.cpp;
/cvsroot/mozilla/mailnews/base/search/src/nsMsgSearchValue.cpp,v  <--  nsMsgSearchValue.cpp
new revision: 1.29; previous revision: 1.28

Comment 52

[Prasad Sunkari [:prasad]]

Attachment: Compile mailnews/local with frozen linkage

Compiles mailnews/local with frozen linkage.
Did the basic testing (reading/deleting/moving) on Linux.

Comment 53

[Mark Banner (:standard8)]

Comment on attachment 331999 [details] [diff] [review]
Compile mailnews/local with frozen linkage

>diff --git a/mailnews/base/util/nsMsgUtils.cpp b/mailnews/base/util/nsMsgUtils.cpp

>+
>+NS_MSG_BASE nsresult
>+MsgExamineForProxy(const char *scheme, const char *host,
>+                   PRInt32 port, nsIProxyInfo **proxyInfo)

Just a note, as this is copied from nsNetUtil.h and we seem to be the only consumers of it: http://mxr.mozilla.org/comm-central/search?string=NS_ExamineForProxy then it would seem worthwhile to do a quick patch after this to replace the 2 remaining instances and remove the nsNetUtil.h version from mozilla-central.

>diff --git a/mailnews/local/src/nsLocalUtils.cpp b/mailnews/local/src/nsLocalUtils.cpp
>@@ -192,15 +192,11 @@ nsLocalURI2Path(const char* rootURI, con
>     while (*curPos && (*curPos)!='/') curPos++;
> 
>     nsCAutoString newPath("");
>-    char *unescaped = strdup(curPos);  
>+    nsCString unescapedStr;
>+
>     // Unescape folder name
>-    if (unescaped) {
>-      nsUnescape(unescaped);
>-      NS_MsgCreatePathStringFromFolderURI(unescaped, newPath);
>-      PR_Free(unescaped);
>-    }
>-    else
>-      NS_MsgCreatePathStringFromFolderURI(curPos, newPath);
>+    MsgUnescapeString(nsDependentCString(curPos), 0, unescapedStr);
>+    NS_MsgCreatePathStringFromFolderURI(unescapedStr.get(), newPath);

It looks like curPos could be null here, this will cause us to crash on the nsDependentCString, so you need to keep the check here.

>@@ -227,19 +223,26 @@ nsresult nsParseLocalMessageURI(const ch
>   PRInt32 keySeparator = uriStr.FindChar('#');
>   if(keySeparator != -1)
>   {
>-    PRInt32 keyEndSeparator = uriStr.FindCharInSet("?&", 
>-                                                   keySeparator); 
>-    nsAutoString folderPath;
>-    uriStr.Left(folderURI, keySeparator);
>+    PRInt32 keyEndSeparator;
>+    keyEndSeparator = uriStr.FindChar('?', keySeparator); 
>+    if (keyEndSeparator == -1)
>+      keyEndSeparator = uriStr.FindChar('&', keySeparator);

I think you should re-use the nsMsgUtils FindCharInSet here.

>diff --git a/mailnews/local/src/nsMailboxProtocol.cpp b/mailnews/local/src/nsMailboxProtocol.cpp
>--- a/mailnews/local/src/nsMailboxProtocol.cpp
>+++ b/mailnews/local/src/nsMailboxProtocol.cpp
>@@ -469,7 +469,7 @@ nsresult nsMailboxProtocol::LoadUrl(nsIU
> 
>         // check if this is a filter plugin requesting the message.
>         // in that case, set up a text converter
>-        convertData = (queryStr.Find("header=filter") != kNotFound || queryStr.Find("header=attach") != kNotFound);
>+        convertData = (queryStr.Find("header=filter") != -1 || queryStr.Find("header=attach") != -1);

Looks like the second part of the or could do with being on the next line.

>diff --git a/mailnews/local/src/nsMailboxService.cpp b/mailnews/local/src/nsMailboxService.cpp
>@@ -205,8 +204,7 @@ nsresult nsMailboxService::FetchMessage(
>     rv = fileUrl->GetFile(getter_AddRefs(file));
>     NS_ENSURE_SUCCESS(rv, rv);
>     file->GetFileSize(&fileSize);
>-    nsCAutoString uriString(aMessageURI);
>-    uriString.ReplaceSubstring(NS_LITERAL_CSTRING("file:"), NS_LITERAL_CSTRING("mailbox:"));
>+    uriString.Replace(0, 5, NS_LITERAL_CSTRING("mailbox:"));

I think this needs confirming that "file:" is at the start of the string.

>@@ -468,8 +466,8 @@ nsresult nsMailboxService::PrepareMessag
>     {
>       // set up the url spec and initialize the url with it.
>       nsCAutoString buf;
>-      NS_EscapeURL(folderPath.get(),-1,
>-                   esc_Directory|esc_Forced|esc_AlwaysCopy,buf);
>+      MsgEscapeURL(folderPath,
>+                   nsINetUtil::ESCAPE_URL_DIRECTORY|nsINetUtil::ESCAPE_URL_FORCED,buf);

nit: spaces please: either side of | and after the , (two places in this file)

>diff --git a/mailnews/local/src/nsPop3Service.cpp b/mailnews/local/src/nsPop3Service.cpp
>@@ -398,8 +398,10 @@ NS_IMETHODIMP nsPop3Service::NewURI(cons
>         if (NS_SUCCEEDED(rv))
>         {
>           nsCAutoString messageUri (aSpec);
>-          messageUri.ReplaceSubstring("mailbox:", "mailbox-message:");
>-          messageUri.ReplaceSubstring("?number=", "#");
>+          messageUri.Replace(0, 8, "mailbox-message:");

Again, I think you should check that it is mailbox: at the start of the string.

Comment 54

[Prasad Sunkari [:prasad]]

Attachment: mailnews/local - updated as per review comments

Updated the patch as per the review comments by Mark.

Had to expose FindCharInSet pair of function through nsMsgUtils for use in this patch.  The FindCharInSet functions are also used in other parts of mailnews.

nsMsgUtils also includes implementations for RFindCharInSet functions. These functions are not used anywhere else in mailnews and can probably be removed.

Comment 55

[Prasad Sunkari [:prasad]]

Attachment: Compile mailnews/db with frozen linkage

Compiles mailnews/db with frozen linkage

Comment 56

[Prasad Sunkari [:prasad]]

Attachment: Compile mailnews/base/src with Frozen linkage

Compiles mailnews/base/src with frozen linkage.

This patch depends on the mailnews/local patch for FindCharInSet functions in nsMsgUtils.

Comment 57

[Mark Banner (:standard8)]

Comment on attachment 336386 [details] [diff] [review]
Compile mailnews/base/src with Frozen linkage

>diff --git a/mailnews/base/src/nsMailDirProvider.h b/mailnews/base/src/nsMailDirProvider.h
>--- a/mailnews/base/src/nsMailDirProvider.h
>+++ b/mailnews/base/src/nsMailDirProvider.h
>@@ -43,7 +43,8 @@
> #include "nsIDirectoryService.h"
> #include "nsIGenericFactory.h"
> #include "nsISimpleEnumerator.h"
>-#include "nsString.h"
>+#include "nsStringAPI.h"
>+#include "nsCOMPtr.h"

This should be nsStringGlue.h, we're not quite ready for full frozen api yet ;-)


>@@ -208,12 +209,16 @@ nsresult ConvertAndSanitizeFileName(cons
>      The display name is in UTF-8 because it has been escaped from JS
>   */
>   MsgUnescapeString(nsDependentCString(displayName), 0, unescapedName);
>-  NS_ConvertUTF8toUTF16 ucs2Str(unescapedName);
>-
>+  nsString ucs2Str = NS_ConvertUTF8toUTF16(unescapedName);

I'm not sure why you're changing this, afaik the original method should work with the frozen api.

>-    uriString.ReplaceSubstring(NS_LITERAL_STRING("file:"), NS_LITERAL_STRING("mailbox:"));
>+    uriString.Replace(0, 5, NS_LITERAL_STRING("mailbox:"));

You need to add a comment here saying what you are replacing.

>       case HTML_FILE_TYPE:
>-          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) ) {
>-           fileName.AppendLiteral(HTML_FILE_EXTENSION2);
>-           localFile->SetLeafName(fileName);
>+#ifdef MOZILLA_INTERNAL_API
>+        if ( (fileName.RFind(HTML_FILE_EXTENSION, PR_TRUE, -1, sizeof(HTML_FILE_EXTENSION)-1) == -1) &&
>+             (fileName.RFind(HTML_FILE_EXTENSION2, PR_TRUE, -1, sizeof(HTML_FILE_EXTENSION2)-1) == -1) ) {
>+#else
>+        if (!StringEndsWith(fileName, NS_LITERAL_STRING(HTML_FILE_EXTENSION), CaseInsensitiveCompare) &&
>+            !StringEndsWith(fileName, NS_LITERAL_STRING(HTML_FILE_EXTENSION2), CaseInsensitiveCompare)) {

nit: please put the CaseInsensitiveCompare statements on a new line (same for the other similar changes in this switch).

>@@ -82,7 +82,13 @@ NS_IMETHODIMP nsMessengerContentHandler:
>         {
>           nsCAutoString queryPart;
>           aUrl->GetQuery(queryPart);
>-          queryPart.ReplaceSubstring("type=message/rfc822", "type=application/x-message-display");
>+
>+          PRUint32 offset = queryPart.Find("type=message/rfc822");
>+          while (offset != -1) {
>+            queryPart.Replace(offset, 19, "type=application/x-message-display"); 
>+            offset = queryPart.Find("type=message/rfc822", offset + 34);
>+          }

I think the second Find should have offset + 19.

>+#ifdef MOZILLA_INTERNAL_API
>         if (serverType.LowerCaseEqualsLiteral("none"))
>           accountNum += 2000;
>         else if (serverType.LowerCaseEqualsLiteral("nntp"))
>           accountNum += 3000;
>         else
>           accountNum += 1000;     // default is to appear at the top
>+#else
>+        if (serverType.Equals("none"), CaseInsensitiveCompare)

This is wrong - extra )

>diff --git a/mailnews/base/src/nsMsgGroupView.cpp b/mailnews/base/src/nsMsgGroupView.cpp
>--- a/mailnews/base/src/nsMsgGroupView.cpp
>+++ b/mailnews/base/src/nsMsgGroupView.cpp
>@@ -649,7 +649,9 @@ NS_IMETHODIMP nsMsgGroupView::GetCellTex
>       PRUint32 flags;
>       PRBool rcvDate = PR_FALSE;
>       msgHdr->GetFlags(&flags);
>+#ifdef MOZILLA_INTERNAL_API
>       aValue.SetCapacity(0);
>+#endif
>       switch (m_sortType)
>       {
>         case nsMsgViewSortType::byReceived:
>@@ -696,18 +698,27 @@ NS_IMETHODIMP nsMsgGroupView::GetCellTex
>           break;
>         case nsMsgViewSortType::byStatus:
>           rv = FetchStatus(m_flags[aRow], aValue);
>-          if (aValue.IsEmpty())
>-            aValue.Adopt(GetString(NS_LITERAL_STRING("messagesWithNoStatus").get()));
>+          if (aValue.IsEmpty()) {
>+            if (m_kNoStatusString.IsEmpty())
>+              m_kNoStatusString.Adopt(GetString(NS_LITERAL_STRING("messagesWithNoStatus").get()));
>+            aValue.Assign(m_kNoStatusString);
>+          }

In this change, and the other case statements in this file, you're actually changing the code and I don't think its frozen api related, did you include a different patch by mistake? If not, please explain what you are doing.

Comment 58

[Mark Banner (:standard8)]

Comment on attachment 336373 [details] [diff] [review]
mailnews/local - updated as per review comments

(In reply to comment #53)
> (From update of attachment 331999 [details] [diff] [review])
> >diff --git a/mailnews/local/src/nsMailboxService.cpp b/mailnews/local/src/nsMailboxService.cpp
> >@@ -205,8 +204,7 @@ nsresult nsMailboxService::FetchMessage(
> >     rv = fileUrl->GetFile(getter_AddRefs(file));
> >     NS_ENSURE_SUCCESS(rv, rv);
> >     file->GetFileSize(&fileSize);
> >-    nsCAutoString uriString(aMessageURI);
> >-    uriString.ReplaceSubstring(NS_LITERAL_CSTRING("file:"), NS_LITERAL_CSTRING("mailbox:"));
> >+    uriString.Replace(0, 5, NS_LITERAL_CSTRING("mailbox:"));
> 
> I think this needs confirming that "file:" is at the start of the string.

You missed addressing this.

+    if (curPos) {
+      nsCString unescapedStr;
+      MsgUnescapeString(nsDependentCString(curPos), 0, unescapedStr);
+      NS_MsgCreatePathStringFromFolderURI(unescapedStr.get(), newPath);
+    } else {
+      NS_MsgCreatePathStringFromFolderURI(curPos, newPath);
     }

You don't need the braces round the else part.

r=me with those fixed.

Comment 59

[Prasad Sunkari [:prasad]]

Attachment: [checked-in] updated to trunk

carrying r+ by standard8.

Comment 60

[David :Bienvenu]

Comment on attachment 360938 [details] [diff] [review]
[checked-in] updated to trunk

thx, Prasad.

Comment 61

[Prasad Sunkari [:prasad]]

Attachment: [checked in] mailnews/local - review comments and updated to trunk

Carrying forward r+ by standard8.
Also addresses the following issues raised in comment #58.

> > >diff --git a/mailnews/local/src/nsMailboxService.cpp b/mailnews/local/src/nsMailboxService.cpp
> > >@@ -205,8 +204,7 @@ nsresult nsMailboxService::FetchMessage(
> > >     rv = fileUrl->GetFile(getter_AddRefs(file));
> > >     NS_ENSURE_SUCCESS(rv, rv);
> > >     file->GetFileSize(&fileSize);
> > >-    nsCAutoString uriString(aMessageURI);
> > >-    uriString.ReplaceSubstring(NS_LITERAL_CSTRING("file:"), NS_LITERAL_CSTRING("mailbox:"));
> > >+    uriString.Replace(0, 5, NS_LITERAL_CSTRING("mailbox:"));
> > 
> > I think this needs confirming that "file:" is at the start of the string.

All the above code lies inside a conditional block that checks if the string starts with "file:".  So the above change will work.
 
> You missed addressing this.
> 
> +    if (curPos) {
> +      nsCString unescapedStr;
> +      MsgUnescapeString(nsDependentCString(curPos), 0, unescapedStr);
> +      NS_MsgCreatePathStringFromFolderURI(unescapedStr.get(), newPath);
> +    } else {
> +      NS_MsgCreatePathStringFromFolderURI(curPos, newPath);
>      }
> 
> You don't need the braces round the else part.
>

DONE.

Comment 62

[David :Bienvenu]

Comment on attachment 361124 [details] [diff] [review]
[checked in] mailnews/local - review comments and updated to trunk

sorry for the delay, looks good, thx, Prasad.

Comment 63

[Mark Banner (:standard8)]

Comment on attachment 361124 [details] [diff] [review]
[checked in] mailnews/local - review comments and updated to trunk

I just noticed this lying around with full reviews but not checked in, so I've fixed a little bit of bitrot and check it in on Prasad's behalf:

http://hg.mozilla.org/comm-central/rev/d1987b32099f

Comment 64

[Matthew Gertner]

Attachment: WIP for moving base/util to external linkage

Comment 65

[Matthew Gertner]

Attachment: WIP for moving import to external linkage

Comment 66

[Matthew Gertner]

Attachment: Move base/util to external linkage

Comment 67

[Matthew Gertner]

Attachment: Move import to external linkage

I should mention that while this and the previous patch both build on Windows, Mac and Linux, I haven't tested for regressions. I am planning to test the import code, but if there are unit and/or other tests that are applicable, they should definitely be used to check whether the changes break anything inadvertently.

Comment 68

[Mark Banner (:standard8)]

Comment on attachment 367240 [details] [diff] [review]
Move base/util to external linkage

Here's some quick comments.

Firstly, can you use -p in patches? It adds which function we're in to the code, which makes locating it in the files easier.

>-MOZILLA_INTERNAL_API = 1

We should find a way of making this optional/easier to turn on/off. I've raised bug 483577.

>-       nsCOMPtr<nsIMsgIncomingServer> server = do_QueryElementAt(allServers, serverIndex);
>+       nsCOMPtr<nsISupports> supports;
>+       rv = allServers->GetElementAt(serverIndex, getter_AddRefs(supports));
>+       NS_ENSURE_SUCCESS(rv, rv);
>+       nsCOMPtr<nsIMsgIncomingServer> server = do_QueryInterface(supports, &rv);

I don't like this, as reverting it will likely get missed when we convert allServers to an nsIArray.

As changing the rest of the nsISupportsArray functions to nsIArray is a big task, I would suggest it may be better to do something similar to what Neil did in bug 397277 in a separate patch. Maybe even moving that file into mailnews/. We should do a move/copy such as that in a different bug though.

>@@ -1659,7 +1671,7 @@
>            rv = server->GetOfflineSupportLevel(&offlineSupportLevel);
>            NS_ENSURE_SUCCESS(rv, rv);
>            nsCOMPtr<nsISupportsArray> allDescendents;
>-           NS_NewISupportsArray(getter_AddRefs(allDescendents));
>+           msgNewISupportsArray(getter_AddRefs(allDescendents));

This can just be changed to:

nsCOMPtr<nsISupportsArray> allDescendents =
  do_CreateInstance(NS_SUPPORTSARRAY_CONTRACTID, &rv);
NS_ENSURE_SUCCESS(rv, rv);

(note the check for success added as well).

>-#ifdef MOZILLA_INTERNAL_API
>-    if (NS_SUCCEEDED(rv) &&
>-        folderName.Equals(aName, nsCaseInsensitiveStringComparator()))
>-#else
>     if (NS_SUCCEEDED(rv) &&
>         folderName.Equals(aName, CaseInsensitiveCompare))
>-#endif

Errm, you shouldn't be removing the ifdef. Maybe you didn't fully realise - due to the inter-dependencies, until we get all of mailnews/ building with the external api, we can't have just one or two modules building with external and the rest with internal (for one it wouldn't work with --enable-static-mail).

> #ifdef MOZILLA_INTERNAL_API
>-    if (StringBeginsWith(contentType, NS_LITERAL_STRING("multipart/"), nsCaseInsensitiveStringComparator()))
>+    if (StringBeginsWith(contentType, NS_LITERAL_STRING("multipart/"), CaseInsensitiveCompare))
> #else
>     if (StringBeginsWith(contentType, NS_LITERAL_STRING("multipart/"), CaseInsensitiveCompare))
> #endif

These sorts of changes are wrong.

>@@ -137,7 +139,7 @@
>   else if (!*aCharset || !PL_strcasecmp(aCharset, "us-ascii") ||
>            !PL_strcasecmp(aCharset, "ISO-8859-1")) {
>     // Despite its name, it also works for Latin-1.
>-    CopyASCIItoUTF16(inString, outString);
>+    CopyASCIItoUTF16(nsCString(nsCString(inString)), outString);
>     return NS_OK;
>   }

inString is already an nsCString, so this change is unnecessary.

>@@ -653,7 +656,7 @@
>   nsresult rv = GetUsername(username);
>   NS_ENSURE_SUCCESS(rv, rv);
>   if (!username.IsEmpty()) {
>-    CopyASCIItoUTF16(username, retval);
>+    CopyASCIItoUTF16(nsCString(nsCString(username)), retval);
>     retval.AppendLiteral(" on ");
>   }

Same error...

> #ifdef MOZILLA_INTERNAL_API
>     PRInt32 err;
>-    PRInt32 port = portString.ToInteger(&err);
>+    PRInt32 port = portString.ToInteger((nsresult *) &err);
>     if (!err)
>       SetPort(port);
> #else
>     nsresult err;
>-    PRInt32 port = portString.ToInteger(&err);
>+    PRInt32 port = portString.ToInteger((nsresult *) &err);
>     if (NS_SUCCEEDED(err))
>       SetPort(port);
> #endif

No change needed, all the code needed is already here.

>+void nsReplaceChar(nsAString& aStr, const char* aSet, char aNewChar)
>+void nsReplaceChar(nsACString& aStr, char aOldChar, char aNewChar)
>+void nsReplaceSubstring(nsAString& s, const nsAString& sub1, const nsAString& sub2)
>+void nsReplaceSubstring(nsACString& s, char* sub1, char* sub2)

I would talk to Neil about these and what we've been doing for them. I think he may remember better than me.

>+nsCString operator+(const nsACString& str1, const nsACString& str2)
>+nsString operator+(const nsAString& str1, const nsAString& str2)

For these, we've just been replacing the code with.

xyz = foo;
xyz.Append(bar);

>+                                  
>+NS_MSG_BASE void nsReplaceChar(nsAString& aStr, const char* aSet, char aNewChar);
>+NS_MSG_BASE void nsReplaceChar(nsACString& aStr, char aOldChar, char aNewChar);
>+NS_MSG_BASE void nsReplaceSubstring(nsAString& s, const nsAString& sub1, const nsAString& sub2);
>+NS_MSG_BASE void nsReplaceSubstring(nsACString& s, char* sub1, char* sub2);

If we keep this, we should make them Msg... rather than ns...

>+// Utility functions needed for external linkage
>+NS_MSG_BASE nsIAtom* msgNewAtom(const char* aString);
>+NS_MSG_BASE nsresult msgNewISupportsArray(nsISupportsArray** _retval);

These functions (if we're keeping them) should have the first letter as capital as per the rest of the functions in this file.

Comment 69

[Mark Banner (:standard8)]

Comment on attachment 367241 [details] [diff] [review]
Move import to external linkage

This has many of the same mistakes as the mailnews/base version. I've not looked in detail at it.

Comment 70

[Matthew Gertner]

Attachment: Python script that automates some of the changes to external API

Just in case this is of interest/use to anyone else, here is the Python script that I used to automated some of the work on base/util and import. I don't claim that this is anything but the quickest and dirtiest of quick-and-dirty hacks, and for all I know the same thing could be written in one line of Perl. But it did help take some of the pain out of all the routine and mechanical API changes that need to be made. I would basically run the script on all source files, then compile and make any additional changes by hand.

Comment 71

[Mark Banner (:standard8)]

Frozen linkage isn't relevant any more. Lets just close this. We landed something, so "fixed".