Convert mailnews to frozen/external linkages

RESOLVED FIXED

Status

MailNews Core
Backend
RESOLVED FIXED
11 years ago
3 years ago

People

(Reporter: Scott MacGregor, Assigned: Jan Horak)

Tracking

(Blocks: 1 bug)

Dependency tree / graph
Bug Flags:
wanted-thunderbird3 -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(28 attachments, 74 obsolete attachments)

274.22 KB, patch
Details | Diff | Splinter Review
1.81 KB, patch
neil@parkwaycc.co.uk
: review+
neil@parkwaycc.co.uk
: superreview+
Details | Diff | Splinter Review
644 bytes, patch
standard8
: review+
standard8
: superreview+
Details | Diff | Splinter Review
686 bytes, patch
neil@parkwaycc.co.uk
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
86.46 KB, patch
Details | Diff | Splinter Review
7.75 KB, text/html
Details
9.97 KB, patch
Bienvenu
: review+
Details | Diff | Splinter Review
6.43 KB, patch
Bienvenu
: review+
standard8
: superreview+
Details | Diff | Splinter Review
14.71 KB, patch
Bienvenu
: review+
Details | Diff | Splinter Review
12.88 KB, patch
Details | Diff | Splinter Review
6.05 KB, patch
neil@parkwaycc.co.uk
: review+
standard8
: superreview+
Details | Diff | Splinter Review
34.98 KB, patch
neil@parkwaycc.co.uk
: review+
standard8
: superreview+
Details | Diff | Splinter Review
3.27 KB, patch
neil@parkwaycc.co.uk
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
23.32 KB, patch
Details | Diff | Splinter Review
1.54 KB, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
65.90 KB, patch
neil@parkwaycc.co.uk
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
7.45 KB, patch
neil@parkwaycc.co.uk
: review+
neil@parkwaycc.co.uk
: superreview+
Details | Diff | Splinter Review
5.03 KB, patch
neil@parkwaycc.co.uk
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
11.64 KB, patch
neil@parkwaycc.co.uk
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
61.09 KB, patch
neil@parkwaycc.co.uk
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
2.18 KB, patch
neil@parkwaycc.co.uk
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
610 bytes, patch
neil@parkwaycc.co.uk
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
656 bytes, patch
neil@parkwaycc.co.uk
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
757 bytes, patch
neil@parkwaycc.co.uk
: review+
neil@parkwaycc.co.uk
: superreview+
Details | Diff | Splinter Review
100.17 KB, text/plain
Details
44.97 KB, patch
neil@parkwaycc.co.uk
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
15.41 KB, patch
Details | Diff | Splinter Review
580 bytes, patch
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
For seamonkey and thunderbird to use libxul, we're going to have to port the mailnews module to use frozen linkages. David and I got a brain dump today from Benjamin describing what we need to do.
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?
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.
(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

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

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

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

Comment 5

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

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

Comment 6

11 years ago
Thx, Axel - that's kinda been my thinking - that mapping is basically a hash table, anyway. The harder issue is replacing the UI driven by RDF data sources
(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?
(Reporter)

Updated

11 years ago
Depends on: 379070
(Reporter)

Updated

11 years ago
Depends on: 379068
(Reporter)

Updated

10 years ago
Blocks: 306324

Updated

10 years ago
OS: Windows XP → All
Hardware: PC → All
(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

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

Replacing RDF for the template stuff is going to be more than tricky, though you may be able to hook up something into the template processing that is tailored to what mailnews needs and wants. Again, Neil would know better.
Created attachment 265944 [details] [diff] [review]
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.
(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.
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.

Updated

10 years ago
Depends on: 394502

Updated

10 years ago
Blocks: 394502
No longer depends on: 394502
Depends on: 395701

Comment 13

10 years ago
Did I understand correctly the + operator won't be available for strings? Sure sounds a bit limiting. Why?
Depends on: 406035
Depends on: 406169
Created attachment 294819 [details] [diff] [review]
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).
Attachment #294819 - Flags: superreview?(neil)
Attachment #294819 - Flags: review?(neil)
(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.
Depends on: 410177

Updated

10 years ago
Attachment #294819 - Flags: superreview?(neil)
Attachment #294819 - Flags: superreview+
Attachment #294819 - Flags: review?(neil)
Attachment #294819 - Flags: review+
Attachment #294819 - Attachment description: Finish address book migration to frozen linkage on Linux. → [checked in] Almost Finish address book migration to frozen linkage on Linux.
Depends on: 419592
nominating for wanted-thunderbird3 to keep it on the radar.
Flags: wanted-thunderbird3?
Blocks: 432162
Depends on: 435881
Product: Core → MailNews Core
No longer depends on: 379070
No longer blocks: 394502
No longer blocks: 306324

Comment 17

9 years ago
Last I heard this isn't gonna be finished for tb3. Patches wanted of course...
Marking wanted‑thunderbird3-
Flags: wanted-thunderbird3? → wanted-thunderbird3-
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.
Summary: Convert mailnews to frozen linkages → Convert mailnews to frozen/external linkages
(Assignee)

Comment 19

8 years ago
Created attachment 416064 [details] [diff] [review]
[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.
(Assignee)

Comment 20

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

Comment 22

8 years ago
According to building output the libxul is linked against libxpcom_core. I'm going to check nsStringGlue.h now. I also try to divide patch by mailnews directory modules. Thanks for feedback.
(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

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

Updated

8 years ago
Depends on: 536139
(Assignee)

Comment 25

8 years ago
Created attachment 418821 [details] [diff] [review]
address book patch
Attachment #416064 - Attachment is obsolete: true
(Assignee)

Comment 26

8 years ago
Created attachment 418822 [details] [diff] [review]
base/build patch
(Assignee)

Comment 27

8 years ago
Created attachment 418823 [details] [diff] [review]
base/search patch
(Assignee)

Comment 28

8 years ago
Created attachment 418824 [details] [diff] [review]
base/src patch
(Assignee)

Comment 29

8 years ago
Created attachment 418825 [details] [diff] [review]
base/util patch
(Assignee)

Comment 30

8 years ago
Created attachment 418827 [details] [diff] [review]
compose patch
(Assignee)

Comment 31

8 years ago
Created attachment 418828 [details] [diff] [review]
db patch
(Assignee)

Comment 32

8 years ago
Created attachment 418829 [details] [diff] [review]
extensions patch
(Assignee)

Comment 33

8 years ago
Created attachment 418830 [details] [diff] [review]
imap patch
(Assignee)

Comment 34

8 years ago
Created attachment 418831 [details] [diff] [review]
import patch
(Assignee)

Comment 35

8 years ago
Created attachment 418832 [details] [diff] [review]
local patch
(Assignee)

Comment 36

8 years ago
Created attachment 418833 [details] [diff] [review]
mapi patch
(Assignee)

Comment 37

8 years ago
Created attachment 418834 [details] [diff] [review]
mime patch
(Assignee)

Comment 38

8 years ago
Created attachment 418835 [details] [diff] [review]
news patch
(Assignee)

Comment 39

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

Comment 40

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

Updated

8 years ago
Attachment #418821 - Flags: review?(bugzilla)
(Assignee)

Updated

8 years ago
Attachment #418822 - Flags: review?(bugzilla)
(Assignee)

Updated

8 years ago
Attachment #418823 - Flags: review?(bugzilla)
(Assignee)

Updated

8 years ago
Attachment #418824 - Flags: review?(bugzilla)
(Assignee)

Updated

8 years ago
Attachment #418825 - Flags: review?(bugzilla)
(Assignee)

Updated

8 years ago
Attachment #418827 - Flags: review?(bugzilla)
(Assignee)

Updated

8 years ago
Attachment #418828 - Flags: review?(bugzilla)
(Assignee)

Updated

8 years ago
Attachment #418829 - Flags: review?(bugzilla)
(Assignee)

Updated

8 years ago
Attachment #418830 - Flags: review?(bugzilla)
(Assignee)

Updated

8 years ago
Attachment #418831 - Flags: review?(bugzilla)
(Assignee)

Updated

8 years ago
Attachment #418832 - Flags: review?(bugzilla)
(Assignee)

Updated

8 years ago
Attachment #418833 - Flags: review?(bugzilla)
(Assignee)

Updated

8 years ago
Attachment #418834 - Flags: review?(bugzilla)
(Assignee)

Updated

8 years ago
Attachment #418835 - Flags: review?(bugzilla)
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 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.
Attachment #418821 - Flags: review?(bugzilla) → review-
Attachment #418822 - Flags: superreview+
Attachment #418822 - Flags: review?(bugzilla)
Attachment #418822 - Flags: review+
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.
Attachment #418823 - Flags: review?(bugzilla) → review-
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?
(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?
(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 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 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 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 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 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 on attachment 418831 [details] [diff] [review]
import patch

This patch (unsurprisingly) looks incomplete.
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 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 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 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 ();
(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 on attachment 418830 [details] [diff] [review]
imap patch

>-  LossyCopyUTF16toASCII(pHost, m_logonHost);
>+  m_logonHost = NS_LossyConvertUTF16toASCII(pHost);
nsDependentString(pHost) also works.
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.
(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 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.
Attachment #418824 - Flags: review?(bugzilla)
Attachment #418825 - Flags: review?(bugzilla)
Attachment #418827 - Flags: review?(bugzilla)
Attachment #418828 - Flags: review?(bugzilla)
Attachment #418829 - Flags: review?(bugzilla)
Attachment #418830 - Flags: review?(bugzilla)
Attachment #418831 - Flags: review?(bugzilla)
Attachment #418832 - Flags: review?(bugzilla)
Attachment #418833 - Flags: review?(bugzilla)
Attachment #418834 - Flags: review?(bugzilla)
Attachment #418835 - Flags: review?(bugzilla)
(Assignee)

Comment 62

8 years ago
Created attachment 420041 [details] [diff] [review]
extensions patch v. 2

Thanks for comments, sending hopefully better version.
Attachment #418829 - Attachment is obsolete: true
Attachment #420041 - Flags: review?(neil)
(Assignee)

Comment 63

8 years ago
Created attachment 420042 [details] [diff] [review]
mime patch v 2

Please check Adopt() usage. It's now the right approach?
Attachment #418834 - Attachment is obsolete: true
Attachment #420042 - Flags: review?(neil)
(Assignee)

Comment 64

8 years ago
Created attachment 420307 [details] [diff] [review]
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?
Attachment #418821 - Attachment is obsolete: true
Attachment #420307 - Flags: review?(neil)
(Assignee)

Comment 65

8 years ago
Created attachment 420328 [details] [diff] [review]
local patch v2
Attachment #418832 - Attachment is obsolete: true
Attachment #420328 - Flags: review?(neil)
(Assignee)

Comment 66

8 years ago
Created attachment 420329 [details] [diff] [review]
news patch v2

Fixed version according to your comments.
Attachment #418835 - Attachment is obsolete: true
Attachment #420329 - Flags: review?(neil)

Comment 67

8 years ago
Jan,

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

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

If there's anything I can do to help, please let me know.
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, '@'))
[But with better formatting than Bugzilla default wrapping!]
(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.
(Assignee)

Comment 71

8 years ago
Created attachment 420542 [details] [diff] [review]
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.
Attachment #420307 - Attachment is obsolete: true
Attachment #420307 - Flags: review?(neil)
(Assignee)

Comment 72

8 years ago
Created attachment 420546 [details] [diff] [review]
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?
Attachment #418827 - Attachment is obsolete: true
Attachment #420546 - Flags: review?(neil)
(Assignee)

Updated

8 years ago
Attachment #420542 - Flags: review?(neil)
(Assignee)

Comment 73

8 years ago
Created attachment 420548 [details] [diff] [review]
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.
Attachment #418825 - Attachment is obsolete: true
Attachment #420548 - Flags: review?(neil)

Comment 74

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

I used nsINetUtil::UnescapeString in my version and it seems to work fine.
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
Attachment #420042 - Flags: review?(neil) → review+

Updated

8 years ago
Attachment #420542 - Flags: review?(neil) → review+
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 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 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.
(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 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

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

Comment 82

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

Comment 83

8 years ago
Created attachment 421805 [details] [diff] [review]
address book patch v4

Replaced some StringTail by Substring, added missing headers, sync patch to trunk.
Attachment #420542 - Attachment is obsolete: true
Attachment #421805 - Flags: review?(neil)
(Assignee)

Comment 84

8 years ago
Created attachment 421809 [details] [diff] [review]
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.
Attachment #420546 - Attachment is obsolete: true
Attachment #421809 - Flags: review?(neil)
Attachment #420546 - Flags: review?(neil)
(Assignee)

Comment 85

8 years ago
Created attachment 421810 [details] [diff] [review]
extensions patch v 3

Fixed MsgReplaceSubstring (require nsACString instead of char * now), sync to trunk.
Attachment #420041 - Attachment is obsolete: true
Attachment #421810 - Flags: review?(neil)
Attachment #420041 - Flags: review?(neil)
(Assignee)

Comment 86

8 years ago
Created attachment 421829 [details] [diff] [review]
imap patch v2

Replaced some StringTail by Substring, few Find->FindChar, MsgUnescape->MsgUnescapeString and getting current thread fixed.
Attachment #418830 - Attachment is obsolete: true
Attachment #421829 - Flags: review?(neil)
(Assignee)

Comment 87

8 years ago
Created attachment 421831 [details] [diff] [review]
local patch v3

AppendNative()->Append(), MoveToNative->MoveTo(), StringTail->Substring, Find->FindChar.
Attachment #420328 - Attachment is obsolete: true
Attachment #421831 - Flags: review?(neil)
Attachment #420328 - Flags: review?(neil)
(Assignee)

Comment 88

8 years ago
Created attachment 421832 [details] [diff] [review]
mapi patch

Fixed current thread retrieve, Find->FindChar.
Attachment #418833 - Attachment is obsolete: true
Attachment #421832 - Flags: review?(neil)
(Assignee)

Comment 89

8 years ago
Created attachment 421834 [details] [diff] [review]
mime patch v3

Find->FindChar, fixed Adopt issue
Attachment #420042 - Attachment is obsolete: true
Attachment #421834 - Flags: review?(neil)
(Assignee)

Updated

8 years ago
Attachment #418828 - Flags: review?(neil)
(Assignee)

Comment 90

8 years ago
Created attachment 421838 [details] [diff] [review]
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
Attachment #420329 - Attachment is obsolete: true
Attachment #421838 - Flags: review?(neil)
Attachment #420329 - Flags: review?(neil)
(Assignee)

Comment 91

8 years ago
Created attachment 421840 [details] [diff] [review]
base/util patch v3

Fixed get of current thread, renamed nsErrorAccordingToNSPR, removed MsgUnescapeURL, MsgUnescapeURL and MsgEscape.
Attachment #420548 - Attachment is obsolete: true
Attachment #421840 - Flags: review?(neil)
Attachment #420548 - Flags: review?(neil)
(Assignee)

Comment 92

8 years ago
Created attachment 421843 [details] [diff] [review]
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.
Attachment #418824 - Attachment is obsolete: true
Attachment #421843 - Flags: review?(neil)
(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 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.

Updated

8 years ago
Attachment #421834 - Flags: review?(neil) → review+

Updated

8 years ago
Attachment #421805 - Flags: review?(neil) → review+

Updated

8 years ago
Attachment #418828 - Flags: review?(neil) → review+
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?
My bad, someone already wrapped that one in an #ifdef
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 on attachment 421810 [details] [diff] [review]
extensions patch v 3

You don't seem to have read comment #68 :-(
(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());
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.
Filed bug 540322 on Last, SetCharAt etc.
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).
Attachment #421809 - Flags: review?(neil) → review-
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?
Attachment #421829 - Flags: review?(neil) → review-
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 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.]
Attachment #421831 - Flags: review?(neil) → review+
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 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 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.
Attachment #421843 - Flags: review?(neil) → review-
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));
}
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.
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.

Updated

8 years ago
Attachment #421810 - Flags: review?(neil) → review-
Comment on attachment 421810 [details] [diff] [review]
extensions patch v 3

Cancelling the remaining reviews due to the use of #ifdef

Updated

8 years ago
Attachment #421838 - Flags: review?(neil) → review-

Updated

8 years ago
Attachment #421840 - Flags: review?(neil) → review-
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.

Updated

8 years ago
Attachment #421831 - Flags: review+ → review-
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&);
In fact when using the external API I can do better for Last() by using
#define Last() EndReading()[-1]
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

8 years ago
Created attachment 423762 [details] [diff] [review]
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.
Created attachment 423807 [details]
Draft conversions

After discussion with another of the mailnews peers, I have created this consolidated list of APIs with their suggested conversions.
Created attachment 423996 [details] [diff] [review]
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.
Attachment #423996 - Flags: superreview?(dmose)
Attachment #423996 - Flags: review?(bienvenu)
Created attachment 424006 [details] [diff] [review]
Preprocessed conversions

With all the back-and-forth between APIs, some changes got lost :-(
Attachment #423996 - Attachment is obsolete: true
Attachment #424006 - Flags: superreview?(dmose)
Attachment #424006 - Flags: review?(bienvenu)
Attachment #423996 - Flags: superreview?(dmose)
Attachment #423996 - Flags: review?(bienvenu)
(Assignee)

Comment 121

8 years ago
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?
(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

8 years ago
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

8 years ago
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.
(Assignee)

Comment 125

8 years ago
(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

8 years ago
(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.
(Assignee)

Comment 127

8 years ago
(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.
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

8 years ago
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

8 years ago
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)
Ah, this is because ToInteger can take two arguments, and I hadn't noticed.
Created attachment 425645 [details] [diff] [review]
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.
Attachment #425645 - Flags: review?(bienvenu)

Comment 133

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

Updated

8 years ago
Attachment #425645 - Flags: review?(bienvenu) → review+

Comment 134

8 years ago
Comment on attachment 424006 [details] [diff] [review]
Preprocessed conversions

this is obsolete, I guess.
Attachment #424006 - Attachment is obsolete: true
Attachment #424006 - Flags: superreview?(dmose)
Attachment #424006 - Flags: review?(bienvenu)
Comment on attachment 425645 [details] [diff] [review]
ToInteger
[Checkin: Comment 135]

Pushed changeset 435954c9da59 to comm-central.
Attachment #425645 - Attachment description: ToInteger → ToInteger [Checkin: Comment 135]
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
Attachment #294819 - Attachment description: [checked in] Almost Finish address book migration to frozen linkage on Linux. → Almost Finish address book migration to frozen linkage on Linux [Checkin: Comment 136]
Created attachment 426114 [details] [diff] [review]
Fixed conversions
[Checkin: Comment 148]

The only difference between this patch and attachment 424006 [details] [diff] [review] is that ToInteger now takes two parameters.
Attachment #426114 - Flags: superreview?(bugzilla)
Attachment #426114 - Flags: review?(bienvenu)
Comment on attachment 418822 [details] [diff] [review]
base/build patch


This patch looks ready for checkin, is it not?
(Assignee)

Comment 139

8 years ago
Created attachment 426258 [details] [diff] [review]
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.
Attachment #426258 - Flags: review?
(Assignee)

Updated

8 years ago
Attachment #426258 - Flags: review? → review?(neil)
(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.
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

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

typo nit:

+ * The following definitons exist for compatibility between the internal and
Attachment #426114 - Flags: review?(bienvenu) → review+
(Assignee)

Comment 143

8 years ago
(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.
(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
(Assignee)

Comment 145

8 years ago
(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?
(In reply to comment #145)
> Ah, I've changed nsCaseInsensitiveCStringComparator into
> CaseInsensitiveCompare where possible. Should I turn it back, right?
Yes please.
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 on attachment 426114 [details] [diff] [review]
Fixed conversions
[Checkin: Comment 148]

Pushed changeset f8515b71dd94 to comm-central.

r+sr=bienvenu over IRC.
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);
(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.
(Assignee)

Comment 151

8 years ago
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 :).
(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.
(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...
(Assignee)

Comment 154

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

Updated

8 years ago
Attachment #426258 - Flags: review?(neil)
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.
Attachment #426114 - Attachment description: Fixed conversions → Fixed conversions [Checkin: Comment 148]

Comment 158

8 years ago
(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.
(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.
Created attachment 426927 [details] [diff] [review]
Remove #ifdef MOZILLA_INTERNAL_API
[Checkin: Comment 218]

Since attachment 426114 [details] [diff] [review], some of the easier #ifdef blocks can now be removed.
Attachment #426927 - Flags: superreview?(dmose)
Attachment #426927 - Flags: review?(bienvenu)
(Assignee)

Comment 161

8 years ago
Created attachment 427086 [details] [diff] [review]
address book patch v5

Removed ifdefs.
Attachment #421805 - Attachment is obsolete: true
Attachment #427086 - Flags: review?(neil)
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.
Attachment #427086 - Flags: review?(neil) → review+
(Assignee)

Comment 163

8 years ago
Created attachment 427106 [details] [diff] [review]
news patch v4

Removed ifdefs, fixed commented issues.
Attachment #421838 - Attachment is obsolete: true
Attachment #427106 - Flags: review?(neil)
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 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.
Attachment #426114 - Flags: superreview?(bugzilla) → superreview+
(Assignee)

Comment 166

8 years ago
Created attachment 427318 [details] [diff] [review]
address book patch v6

Removed nsMsgUtils.h from nsAbBoolExprToLDAPFilter.cpp and used MsgGetProxyForObject. No other changes.
Attachment #427086 - Attachment is obsolete: true
Attachment #427318 - Flags: review?(neil)
(In reply to comment #166)
> Removed nsMsgUtils.h from nsAbBoolExprToLDAPFilter.cpp and used
> MsgGetProxyForObject. No other changes.
Unfortunately nobody's written MsgGetProxyForObject yet ;-)
(Assignee)

Comment 168

8 years ago
Created attachment 427334 [details] [diff] [review]
nsMsgUtils additions

I did :). I'm sending patch now.
Attachment #426258 - Attachment is obsolete: true
(Assignee)

Comment 169

8 years ago
Created attachment 427336 [details] [diff] [review]
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) {
Attachment #427106 - Attachment is obsolete: true
Attachment #427336 - Flags: review?(neil)
Attachment #427106 - Flags: review?(neil)
(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 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

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

nice to see those #ifdef's go away!
Attachment #426927 - Flags: review?(bienvenu) → review+
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) ;-)
(Assignee)

Comment 174

8 years ago
Created attachment 427558 [details] [diff] [review]
news patch v5

Uh, sorry for +/-1 mistakes. 

>>+    PRInt32 keyEndSeparator = MsgFindCharInSet(uriStr, "?&", keySeparator);
>This needs the util patch, right?
Yes, it does.
Attachment #427336 - Attachment is obsolete: true
Attachment #427558 - Flags: review?(neil)
Attachment #427336 - Flags: review?(neil)
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.
(Assignee)

Comment 176

8 years ago
Created attachment 427736 [details] [diff] [review]
news patch v6
[Checkin: Comment 234]

New line fixed hopefully.
Attachment #427558 - Attachment is obsolete: true
Attachment #427736 - Flags: review?(neil)
Attachment #427558 - Flags: review?(neil)
(Assignee)

Comment 177

8 years ago
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.
(Assignee)

Comment 178

8 years ago
Created attachment 427761 [details] [diff] [review]
extensions patch v4

Fixed issue by comment #68 :)
Attachment #421810 - Attachment is obsolete: true
Attachment #427761 - Flags: review?(neil)
(Assignee)

Comment 179

8 years ago
Created attachment 427764 [details] [diff] [review]
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.
Attachment #421831 - Attachment is obsolete: true
Attachment #427764 - Flags: review?(neil)
(In reply to comment #179)
> do_GetAtom changed to Msg_do_GetAtom
Would you mind calling it MsgGetAtom?
(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.

Updated

8 years ago
Attachment #427736 - Flags: review?(neil) → review+
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 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.

Updated

8 years ago
Attachment #427764 - Attachment is patch: true
Attachment #427764 - Attachment mime type: application/octet-stream → text/plain
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");

Updated

8 years ago
(Assignee)

Comment 185

8 years ago
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)?
(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.
(Assignee)

Comment 187

8 years ago
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.
(Assignee)

Comment 188

8 years ago
Created attachment 428436 [details] [diff] [review]
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.
Attachment #427764 - Attachment is obsolete: true
Attachment #428436 - Flags: review?(neil)
Attachment #427764 - Flags: review?(neil)
(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.
(Assignee)

Comment 190

8 years ago
Created attachment 428437 [details] [diff] [review]
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?
Attachment #427761 - Attachment is obsolete: true
Attachment #428437 - Flags: review?(neil)
Attachment #427761 - Flags: review?(neil)
(Assignee)

Comment 191

8 years ago
(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?
/xpcom/glue/nsStringAPI.cpp
* line 310 -- nsAString::LowerCaseEqualsLiteral(const char *aASCIIString) const
(Assignee)

Comment 193

8 years ago
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.
(Assignee)

Comment 194

8 years ago
Created attachment 428444 [details] [diff] [review]
compose patch v4

Bringing back compose path without ifdefs.
Attachment #421809 - Attachment is obsolete: true
Attachment #428444 - Flags: review?(neil)
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;;
Attachment #428437 - Flags: review?(neil) → review+
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.
Attachment #428436 - Flags: review?(neil) → review+
(Assignee)

Comment 197

8 years ago
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?
(Assignee)

Comment 198

8 years ago
Sorry, ignore comment #197. I've got pretty mess in my nsMsgUtils.h now.
(Assignee)

Comment 199

8 years ago
(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
(Assignee)

Comment 200

8 years ago
Created attachment 428695 [details] [diff] [review]
compose patch v5

Fixed MsgLowerCaseEqual/LowerCaseEqual, hope I got it now :).
Attachment #428444 - Attachment is obsolete: true
Attachment #428695 - Flags: review?(neil)
Attachment #428444 - Flags: review?(neil)
(Assignee)

Comment 201

8 years ago
Created attachment 428696 [details] [diff] [review]
local patch v5

Fixed MsgLowerCaseEqual/LowerCaseEqual too.
Attachment #428436 - Attachment is obsolete: true
Attachment #428696 - Flags: review?(neil)
(Assignee)

Comment 202

8 years ago
Created attachment 428698 [details] [diff] [review]
mapi patch v2

Removed unnecessary changes, keeping kNotFound and Last(), replaced Right() by Substring.
Attachment #421832 - Attachment is obsolete: true
Attachment #428698 - Flags: review?(neil)
Attachment #421832 - Flags: review?(neil)
(Assignee)

Comment 203

8 years ago
Created attachment 428700 [details] [diff] [review]
mime patch v4
[Checkin: Comment 241]

Removed ifdefs and introducing MsgLowerCaseEqualsLiteral.
Attachment #421834 - Attachment is obsolete: true
Attachment #428700 - Flags: review?(neil)
Comment on attachment 428696 [details] [diff] [review]
local patch v5

>-      name.LowerCaseEqualsLiteral("rulesbackup.dat"))
>+      name.LowerCaseEqualsLiteral( "rulesbackup.dat"))
Oops ;-)
Attachment #428696 - Flags: review?(neil) → review+
(Assignee)

Comment 205

8 years ago
Created attachment 428701 [details] [diff] [review]
imap patch v3

A do_QueryElementAt, Last(), put back, removed ifdefs, using MsgLowerCaseEqualsLiteral and MsgReplaceChar where required, calling MsgGetAtom instead of do_GetAtom.
Attachment #421829 - Attachment is obsolete: true
Attachment #428701 - Flags: review?(neil)
(Assignee)

Comment 206

8 years ago
Created attachment 428710 [details] [diff] [review]
base/search patch v2

A do_QueryElementAt brought back, removed ifdefs, brought back NS_NewISupportsArray where possible.
Attachment #418823 - Attachment is obsolete: true
Attachment #428710 - Flags: review?(neil)
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?

Updated

8 years ago
Attachment #428700 - Flags: review?(neil) → review+
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.]
Attachment #428710 - Flags: review?(neil) → review+
(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 on attachment 426927 [details] [diff] [review]
Remove #ifdef MOZILLA_INTERNAL_API
[Checkin: Comment 218]

sr=dmose
Attachment #426927 - Flags: superreview?(dmose) → superreview+
(Assignee)

Comment 211

8 years ago
Created attachment 429100 [details] [diff] [review]
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
Attachment #421840 - Attachment is obsolete: true
Attachment #429100 - Flags: review?(neil)
(Assignee)

Comment 212

8 years ago
Created attachment 429106 [details] [diff] [review]
base/src patch v3

Bringing back to hopefully fixed base/src.
Attachment #421843 - Attachment is obsolete: true
Attachment #429106 - Flags: review?(neil)
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.
(Assignee)

Comment 214

8 years ago
Created attachment 429511 [details] [diff] [review]
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.
Attachment #429100 - Attachment is obsolete: true
Attachment #429511 - Flags: review?(neil)
Attachment #429100 - Flags: review?(neil)
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.
(Assignee)

Comment 216

8 years ago
Created attachment 429721 [details] [diff] [review]
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?
Attachment #429511 - Attachment is obsolete: true
Attachment #429721 - Flags: review?(neil)
Attachment #429511 - Flags: review?(neil)
(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 on attachment 426927 [details] [diff] [review]
Remove #ifdef MOZILLA_INTERNAL_API
[Checkin: Comment 218]

Pushed changeset 2e7ced2b75da to comm-central.
Attachment #426927 - Attachment description: Remove #ifdef MOZILLA_INTERNAL_API → Remove #ifdef MOZILLA_INTERNAL_API [Checkin: Comment 218]
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?
Attachment #429721 - Flags: review?(neil) → review+
(Assignee)

Comment 220

8 years ago
Created attachment 431840 [details] [diff] [review]
base/util path v7
[Checkin: Comment 223]

Fixed silly mistakes.
Attachment #429721 - Attachment is obsolete: true
Attachment #431840 - Flags: review?(neil)
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.]
Attachment #431840 - Flags: review?(neil) → review+

Updated

8 years ago
Attachment #431840 - Flags: superreview?(bugzilla)
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.
Attachment #431840 - Flags: superreview?(bugzilla) → superreview+
Comment on attachment 431840 [details] [diff] [review]
base/util path v7
[Checkin: Comment 223]

Pushed changeset f529e947df1b to comm-central.
Attachment #431840 - Attachment description: base/util path v7 → base/util path v7 [Checkin: Comment 223]
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.
Attachment #427318 - Flags: review?(neil) → review+
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.
Attachment #428695 - Flags: review?(neil) → review-
(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 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...
Attachment #428701 - Flags: review?(neil) → review-
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 on attachment 427318 [details] [diff] [review]
address book patch v6

Some of these patches could do with sr; randomly picking victims...
Attachment #427318 - Flags: superreview?(bugzilla)
Comment on attachment 427736 [details] [diff] [review]
news patch v6
[Checkin: Comment 234]

Some of these patches could do with sr; randomly picking victims...
Attachment #427736 - Flags: superreview?(bienvenu)
Comment on attachment 428437 [details] [diff] [review]
extensions patch v5

Some of these patches could do with sr; randomly picking victims...
Attachment #428437 - Flags: superreview?(dmose)

Updated

8 years ago
Attachment #428696 - Flags: superreview?(bienvenu)

Updated

8 years ago
Attachment #428700 - Flags: superreview?(bugzilla)

Updated

8 years ago
Attachment #428710 - Flags: superreview?(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?

Updated

8 years ago
Attachment #427736 - Flags: superreview?(bienvenu) → superreview+

Comment 233

8 years ago
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.
Attachment #428696 - Flags: superreview?(bienvenu)
Comment on attachment 427736 [details] [diff] [review]
news patch v6
[Checkin: Comment 234]

Pushed changeset 886d66940820 to comm-central.
Attachment #427736 - Attachment description: news patch v6 → news patch v6 [Checkin: Comment 234]
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!)
Attachment #428710 - Flags: superreview?(dmose) → superreview?(bienvenu)

Updated

8 years ago
Attachment #428437 - Flags: superreview?(dmose) → superreview?(bienvenu)
(Assignee)

Comment 236

8 years ago
Created attachment 437549 [details] [diff] [review]
compose patch v6

Sync to trunk, fixes of mentioned issues.
Attachment #428695 - Attachment is obsolete: true
Attachment #437549 - Flags: review?(neil)
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 on attachment 437549 [details] [diff] [review]
compose patch v6

Is it possible to simplify OnDataAvailable by using rv = AppendToMsgBody(nsDependentCString(newBuf, numWritten)); ?
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.
Attachment #427318 - Flags: superreview?(bugzilla) → superreview-
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.
Attachment #428700 - Flags: superreview?(bugzilla) → superreview+
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].
Attachment #428700 - Attachment description: mime patch v4 → mime patch v4 [Checkin: Comment 241]
(Assignee)

Comment 242

7 years ago
Created attachment 438723 [details] [diff] [review]
mapi patch v3 [Checkin: Comment 322]

Fixed -/+1.
Attachment #428698 - Attachment is obsolete: true
Attachment #438723 - Flags: review?(neil)
Attachment #428698 - Flags: review?(neil)
(Assignee)

Comment 243

7 years ago
Created attachment 438726 [details] [diff] [review]
address book patch v7
Attachment #427318 - Attachment is obsolete: true
Attachment #438726 - Flags: review?(neil)

Updated

7 years ago
Attachment #438723 - Flags: review?(neil) → review+
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.
Attachment #438726 - Flags: review?(neil) → review+
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.
Actually I am in fact still hoping for an answer to comment 238, aren't I?
(Assignee)

Comment 247

7 years ago
Created attachment 442373 [details] [diff] [review]
base/src patch v4

+ required changes to base/utils due to missing NS_NewArrayEnumerator for nsISupportsArray in frozen linkage.
Attachment #429106 - Attachment is obsolete: true
Attachment #429106 - Flags: review?(neil)
(Assignee)

Updated

7 years ago
Attachment #442373 - Flags: review?(neil)
(Assignee)

Comment 248

7 years ago
Created attachment 442374 [details] [diff] [review]
compose patch v7

The AppendToMsgBody used in OnDataAvailable to create less copy code.
Attachment #437549 - Attachment is obsolete: true
Attachment #442374 - Flags: review?(neil)
Attachment #437549 - Flags: review?(neil)

Updated

7 years ago
Blocks: 545716
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.
(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 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 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.)
(Assignee)

Comment 253

7 years ago
#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.
(Assignee)

Comment 254

7 years ago
(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
(Assignee)

Comment 255

7 years ago
> >   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).
(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 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.
So the usable Find methods are as follows:
theString.Find(otherString)
theString.Find("asciiString"[, PR_TRUE])
theCString.Find(otherCString[, CaseInsensitiveCompare])
theCString.Find("asciiString"[, CaseInsensitiveCompare])
(Assignee)

Comment 259

7 years ago
(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)?
(Assignee)

Comment 260

7 years ago
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).
(Assignee)

Comment 261

7 years ago
(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.
(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?
(Assignee)

Comment 263

7 years ago
Created attachment 444633 [details] [diff] [review]
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.
Attachment #444633 - Flags: review?(neil)
(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?
Although nsMsgFlatFolderDataSource calls ListDescendants, is that the issue?
(Assignee)

Comment 266

7 years ago
(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.
nsMsgFlatFolderDataSource has bugs, so I need to rewrite it anyway...
(Assignee)

Comment 268

7 years ago
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?
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 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!
(Assignee)

Comment 271

7 year