78933 - LDAP/MAPI addressbook integration tracking bug

Status: VERIFIED FIXED

(MailNews Core :: LDAP Integration, defect, P1)

Description

[Cuchulainn]

This bug tracks the following

#78931 - Enabling MAPI addressbook access
#17879 - Enabling LDAP access
#78932 - Additional changes necessary to enable multiple datasources

Comment 1

[Keyser Sose]

Marking NEW.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: address book integration diffs, v1.3

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

So these diffs build on the current tree.  These are the changes that Martin
mailed me on Friday, plus some changes to the REQUIRES lines so that builds with
MOZ_TRACK_MODULE_DEPS work (see bug 17454 for details).

From here on out, let's consider the patches in this bug to be the canonical
ones.  So when you fix stuff and make changes, attach a new version of the patch
to this bug, if you would.

A couple of random reviewer comments:

* wrapping at 80 columns makes code easier to review and read

* if MOZ_LDAP_XPCOM is not defined, the build needs to continue to work, but
without LDAP.  I made some of the necessary changes for that in the Makefile.in
files in this version of the patch.  This still needs to be fixed on windows and
tested on unix.  xpfe/components/build and xpfe/components/autocomplete/* have
examples of this sort of conditionalization of the build.

* i'm seeing assertions trying to open history.mab (the collected addressbook)
after applying the patches.

More tommorrow...

Comment 4

[John Marmion]

Attachment: delete dbPath missing from nsImportAddressBooks.cpp plus attaching the latest versions of nsAbOutlookDirectory.cpp & .h plus a minor change to nsBooleanExpression.cpp

Comment 5

[John Marmion]

Attachment: new patch file against head (changes following bug 71247)

Comment 6

[olga charapaev]

applied the patch on WinNT

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

So the MOZ_TRACK_MODULE_DEPS changes that I included in version 1.3 of the patch
haven't found their way into the most recently posted version.  If you could
integrate them into that and post a new diff, that would be great. 
Incidentally, the bug number explaining MOZ_TRACK_MODULE_DEPS is bug 59454.

As far as the assertions go, I started having problems in my unpatched builds
too, so I think my history.mab was corrupt.  After removing it, I'm not seeing
problems in either build.

Also, when posting diffs, please use unified ("diff -u") format rather than
context ("diff -c") format.  Diffs made this way tend to be easier to read.

Comment 8

[John Marmion]

Attachment: using diff -u & re-applied MOZ_LDAP_XPCOM directive changes from first patch file. No code changes.

Comment 9

[John Marmion]

Attachment: added the MOZ_LDAP_XPCOM support for Windows. This gets added to the makefile.win in addrbook src and build directories.

Comment 10

[John Marmion]

Attachment: one line change to previous addrbook/src/makefile.win patch

Comment 11

[John Marmion]

Attachment: 2 - one line fixes following testing on Solaris/Forte 6.1 Compiler

Comment 12

[olga charapaev]

applied the latest patch on WinNT
tested for addressbook functionality regressions
haven't found any

Comment 13

[yulian chang]

reassign Olga as QA contact

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Here are some more general reviewer comments that apply to all three child bugs.
 There will probably be more as I continue to work my way through the code:

general license boilerplate stuff
---------------------------------
* In the license boilerplate, there needs to be names and email
  addresses, not just names.  Part of the reason for this is to
  provide the ability for users of the code to reach the licensors and
  copyright holders.  

* For files that you have written from scratch, Netscape is _not_ the
  initial developer (presumably Sun is).

* All files that get touched by your patches need have the folks who
  made the changes added to the Contributors list if they're not there
  already.

other general
-------------

* Use nsAutoLock with anonymous scopes instead of PR_Lock/PR_Unlock or
  LockGuard: it's easy for future maintainers to miss adding necessary
  PR_Unlock()s, as they make tweaks and lockGuard appears to be just a
  second implementation of nsAutoLock (the latter is the standard).

* New URI schemes that are being introduced need to start with "moz-"
  as per bug 69513

* Why use the (tighter binding) CID for the RDF service rather than
  the contractid?

* Instead of C-style casts, use NS_{STATIC, REINTERPRET, CONST}_CAST()
  as appropriate.

* It's not intuitively obvious what the NPeer stands for in 
  nsIBooleanExpressionNPeer.  How about just nsIBooleanExpression or 
  nsIBooleanExpr?

* any reason not to get rid of the commented-out nslog.h stuff at the
  beginning of various files, since it doesn't appear to be used?

* don't do anything in class constructors that can potentially fail: 
  since Mozilla doesn't use exceptions, there's no way for the caller
  to know that something went wrong.  Better, add an Init() method to
  such classes/interfaces and do any initialization that can fail
  there.  PR_NewLock() can fail (returning NULL); I noticed a few
  examples of this in the patch.


nits: don't need to change this stuff, but worth keeping in mind for
      future code 
-----------------
* NS_ENSURE_* hides a return, making the code a bit harder to read.  Prefer
explicit NS_FAILED() and NS_SUCCEEDED() tests.

* preferring do_GetService() over NS_WITH_SERVICE() adds to readability

Comment 15

[Paul.Sandoz]

Thanks Dan for providing useful review feedback. I will help change relevant
code with our guys here ASAP.

Comment 16

[John Marmion]

Hi Dan,
We are working on implementing yout review changes. All of them appear to be
soound suggestions. I have a couple of queries. 

On the license stuff, I understand that copyright is assigned to Netscape. I
will add the email addresses but are you sure of the other stuff. It was not an
issue with new code in Patch I.

You ask why we use the tighter binding CID rather than the RDF Service. We went
with what was already in place. But can you explain the isssues here?

On the nits, the super-review for Patch I insisted on us using
NS_ENSURE_SUCCESS(). So we changed a lot of code to comply with that.

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

License stuff: (disclaimer: I Am Not A Lawyer.  This Is Not Legal Advice.  For
more authoritative info, consider posting to .license and perhaps blizzard,
mitchell, or hecker will answer).

Copyright for code you write is _not_ assigned to Netscape.  As far as the other
stuff goes, yes, I'm fairly confident of its correctness, I had to do a fair
amount of license-related foo while working full-time for mozilla.org.  Feel
free to confirm by asking in .license if you wish, though.

http://lxr.mozilla.org/mozilla/source/xpcom/doc/xpcom-code-faq.html#ContractID%20Vs%20CLSID
talks a bit about contractid vs. classid.  Generally you only want to use
classID if you know of very specific assumptions made by the class in question
(eg specific locking semantics, implementation details used under the hood) such
that you, as a client, might fail if they're not true.  One other instance where
you might use classid is if you require absolute, bug-for-bug compatibility.

As far as NS_ENSURE_*, it's certainly better than skipping the result checking
(but you already knew that :-). And the mozilla philosophy does tend to be one
of "when in rome, do as the romans do".  So perhaps the super-reviewer had that
in mind and just wanted to keep this code consistent with the rest of the
addressbook code.  Anyway, I'm comfortable with whatever you want to do in this
regard, just thought I'd bring it up to ponder.

Comment 18

[John Marmion]

Attachment: Review changes Dan + Candice

Comment 19

[John Marmion]

I have attached a new patch file which will hopefully will address the issues
raised by Dan. I have also included changes requested by Candice who is
reviewing 78931. I did not address the 'Initial Developer' bit in the header of
new files. I did ensure that all email addressees plus the fact that the files
were Created by us. I will return to the 'Initial Developer'.

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

Regarding the changes in the most recent patch:

* Still lots of C-style casts throughout the code.

* Still plenty of uses of CIDs where contract-ids would almost
  certainly do.  If you really need to use CIDs for object creation,
  it's probably worth having comments explaining why (ie locking, class 
  |friend|liness, or other semantic issues).

* Still lots of explicit .unlock()s of nsAutoLock objects rather than 
  use of appropriate anonymous scopes.

Also came across a few other things while starting my more in-depth review on
the LDAP parts of the code:

* Can we come up with a more descriptive name for SUNLDAPDIRECTORY?
  i.e. something that makes it obvious to the code reader how this
  is different from LDAPDIRECTORY.

* nsAbDirFactoryService: since waterson's RDF code was borrowed for
  the scheme char stuff, waterson should be added to the contributor
  list in the license boilerplate.

Comment 21

[John Marmion]

Attachment: Review changes following first review response

Comment 22

[John Marmion]

We have just added a new patch attachment. We still have an outstanding issue
regarding a Lock deadlock in Proxy and non-Proxy Observers. This code is in
nsAbRDFDataSource.cpp. The relevant code is in the NotifyObserver function. We
want to contact some people on this issue but in the meantime we are keen to
progress the patch.

This patch attempts to address a number of issues raised by Dan Mosedale in his
review of the patch:

1. eliminate C-style casts.
2. replace the SUNLDAPDIRECTORY with FixedQueryLDAPDirectory.
3. ensure waterson is accredited to the contributors.
4. Release mLock if it is non-zero.
5. handle a number of possible failures as outlined in the comments of #17879

We also attempted to resolve the issue raised regarding the use of CIDs versus 
Contract IDs. We have fixed up the LDAP and MAPI code where we have introduced
the use of the global gRDFService plus use RDF constants. We have not changed
the existing code as we believe this is outside the scope of the patch and can
be best served by raising a new bug to deal with this. Our initial use of CIDs
grew from observing the code in the Address Book.

We have also continued to use explicit unlocks in a small number of cases rather
than using the appropriate annonymous scope of nsAutoLock. This we feel is
important to tighten and minimize the scope of the use of locks.

Comment 23

[Paul.Sandoz]

	He is some comments about a strange threading problem
	we are experiencing in the nsAbRDFDataSource.cpp in the
	current patch. Will mail to relevant news groups tomorrow.

	There appears to be a problem relating to an RDF datasource
	using a mixture of RDF Observers and Proxy RDF Observers.
	
	The address book directory datasource NotifyObservers may
	be called from the UI thread for local address books e.g.
	creating a new card, and from another thread when it receives
	cards from an LDAP search query. In the latter case the
	datasource will use proxy observers to the UI thread.
	
	When an LDAP card is dnd'ed to a local address book an
	assertion is produced which states a possible dead lock
	may occur because two separate locks have locked the
	thread. This assertion occurs in the NotifyObservers
	method of the datasource (note that the assertion does
	not result in the card not being added the the local
	address book). Dragging a card from a local address
	book to a local address book results in no assertion,
	even though the method performs the lock in exactly
	the same way.
	
	If the datasource is modified to always use proxy observers
	then no assertion occurs between dragging from LDAP to local.
	
	This only occurs in the current head. Milestone 0.9 works
	OK.
	
	This implies that somthing outside (XUL RDF) is now making
	associations between RDF resources and the fact that they
	were asserted via notification via proxy.

	Most confusing!

Comment 24

[Dan Mosedale (:dmosedale, :dmose)]

Reassigning bugs related to LDAP/MAPI addressbook change landing to John
Marmion, as he'll be driving this landing going forward.

Comment 25

[Dan Mosedale (:dmosedale, :dmose)]

2nd try at reassignment ("bugzilla's hard; let's go shopping").

Comment 26

[John Marmion]

Attachment: Modify 'Initial Developer..' comment for new files as requested

Comment 27

[John Marmion]

Updated the license boilerplate for all new source files submitted with this
patch. I have also updated the experimental builds at
<http://abzilla.mozdev.org> with this patch against today's head for Linux,
Windows and the Mac. Note to enable the LDAP Address Book the uri entry in the
preferences file must be set to "moz-abldapdirectory://...." as requested. 

Comment 28

[Dan Mosedale (:dmosedale, :dmose)]

OK, the patch is coming along nicely.  Here are some more general comments, both
in response to the latest changes as well as a few other random things I've noticed:

* predeclaring IIDs with NS_DEFINE_IID is deprecated; instead, use the
NS_GET_IID() macro directly in the code.

* Since there's no cross-platform convention for uniformly displaying
files which contain tabs, hard tabs are generally not allowed in the
Mozilla code base (with a few necessary exceptions, like Makefiles).
If you could convert the hard tabs that are introduced by this patch
to spaces, that would be appreciated.  Feel free to add emacs
mode-lines to any existing files that don't already have them to help
prevent the inadvertant introduction of new tabs; be sure to set the
tab-width to the existing style in use in such files when doing so.

* I notice that the tab-width specified in the modelines in many of
the new files is 2, but in a bunch is 4.  Any particular rationale for
making them different from each other?

* I completely agree with the need to make locks be held for as short
a length of time as possible.  I obviously didn't do a good job of
communicating what I meant by "anonymous scopes", however, because
using .unlock() is very rarely required to get the same amount of time
spent holding a lock.  As an example, consider, from the patch:

NS_IMETHODIMP nsAbLDAPDirectory::StopSearch ()
{
        nsresult rv;

        rv = Initiate ();
        NS_ENSURE_SUCCESS(rv, rv);

        // Enter lock
        nsAutoLock lockGuard (mLock);
        if (mPerformingQuery == PR_FALSE)
                return NS_OK;
        mPerformingQuery = PR_FALSE;
        // Exit lock
        lockGuard.unlock ();

        rv = StopQuery (mContext);

        return rv;
}

This can be equivalently, but more cleanly, written as follows:

NS_IMETHODIMP nsAbLDAPDirectory::StopSearch ()
{
	nsresult rv;

        rv = Initiate ();
        NS_ENSURE_SUCCESS(rv, rv);

	// Enter lock
        {
            nsAutoLock lockGuard (mLock);

            if (mPerformingQuery == PR_FALSE)
               return NS_OK;

            mPerformingQuery = PR_FALSE;
        }
        // Exit lock

        rv = StopQuery (mContext);

        return rv;
}

The introduction of the anonymous block scope limits the lifetime of
the nsAutoLock: the destructor will be called at the closing brace of
the anonymous block scope.

* I also agree completely that changing existing uses of CIDs is
outside the scope of this bug.  But this patch introduces a whole
bunch of new ones:

> grep NS_DEFINE_CID ~/abook-p7.txt | grep ^+ | wc -l
     17

It's not totally unreasonable to use CIDs to guarantee that objects
inside your own module are interacting with other objects inside your
own module.  But for calling out of module, contractids are definitely
preferred.  So, if we ignore the addressbook related CIDs, the
following new out of module CIDs have still been introduced:

> grep NS_DEFINE_CID ~/abook-p7.txt | grep ^+ | egrep -v 'kAb|kAddr'
+static NS_DEFINE_CID(kLocaleServiceCID, NS_LOCALESERVICE_CID); 
+static NS_DEFINE_CID(kCollationFactoryCID, NS_COLLATIONFACTORY_CID);
+static NS_DEFINE_CID(kEventQueueServiceCID,  NS_EVENTQUEUESERVICE_CID);
+static NS_DEFINE_CID(kStdURLCID, NS_STANDARDURL_CID);
+static NS_DEFINE_CID(kRDFServiceCID, NS_RDFSERVICE_CID);
+static NS_DEFINE_CID(kRDFServiceCID, NS_RDFSERVICE_CID) ;
+static NS_DEFINE_CID(kTextToSubURICID, NS_TEXTTOSUBURI_CID);
+static NS_DEFINE_CID(kStdURLCID, NS_STANDARDURL_CID);

Get rid of these, or add comments to the code about what semantic concern is
being addressed by their use, and I'll be happy.

* Creating a URI by instantiating the URL object yourself has issues,
see bug 73845 for details.  Instead, use NS_NewURI() to do the work.
This has the added bonus that it gets rid of two of the CIDs above.
:-)

* As far as the deadlocking issue, you might try setting a watchpoint on
the lock itself in the debugger as a way to see who's locking it before you.

Comment 29

[Dan Mosedale (:dmosedale, :dmose)]

Whoops; I misread the deadlock description, so ignore my comments about the
watchpoint.  Hopefully your post to .xpfe will garner some useful response.

Comment 30

[John Marmion]

I have created a new patch file against todays's head in response to the
outstanding issues in the review. I believe we have addressed all of them except
for the following: 

We eliminated all but 3 of the 17 NS_DEFINE_CIDs. I have commented on one.
I have set the tab-width to 4 on all new files. I have not addressed the tab
space issue. I need to talk to the guys here to see how we can address this.

Any issues raised on the specific LDAP files, I have also changed in the
MAPI(#78931) source files.

I have also updated the experimental builds on abzilla.mozdev.org

Comment 31

[John Marmion]

Attachment: Lastest response

Comment 32

[John Marmion]

I have created a new patch file against todays's head in response to the
outstanding issues in the review. I believe we have addressed all of them except
for the following: 

We eliminated all but 3 of the 17 NS_DEFINE_CIDs. I have commented on one.
I have set the tab-width to 4 on all new files. I have not addressed the tab
space issue. I need to talk to the guys here to see how we can address this.

Any issues raised on the specific LDAP files, I have also changed in the
MAPI(#78931) source files.

I have also updated the experimental builds on abzilla.mozdev.org

Comment 33

[John Marmion]

Attachment: Remove hard tabs from all new files & replace new NS_DEFINE_CIDs

Comment 34

[John Marmion]

Attachment: Replace a few stray c style casts.

Comment 35

[Dan Mosedale (:dmosedale, :dmose)]

I started reading through some of the LDAP code, and found that I needed to
detour a bit into the infrastructure.  A few more general comments:

* if you need a literal nsCString, don't create it with
  nsCAutoString("foo"); instead use NS_LITERAL_CSTRING("foo").  Noticed
  in nsAbLDAPDirectory.cpp.

* if you need to create an nsCString from a non-literal char *, and
  you can guaranteed that the nsCString will have a lifetime shorter
  than the lifetime of the original non-literal char *, use
  nsDependentCString(foo) instead of nsCAutoString(foo); this will
  save you a copy.  Noticed in nsAbUtils.cpp.

Comment 36

[drapeau]

Okay, I've been watching the traffic on this bug for a while, and it sounds like
good discussion.  But may I offer a suggestion here?
I think the reviews have been of extremely high quality, and the responses
quick.  But the goal is to get some code checked in, tested, and used. 
Perfection isn't really the goal of the Mozilla model; the model is "check in
early and often, fix incrementally."  I remember Brendan Eich saying that to us
Blackwood folks when we first met a bunch of Mozilla people a couple of years
ago.  That's the way I've been taught about Mozilla, anyway.  Given that, can't
this current patch be checked in now, and the latest set of reviews be done as a
follow-on to the current code?

Comment 37

[John Marmion]

Dan, simply replacing the nsCAutoString("foo") with NS_LITERAL_CSTRING("foo")
will not work in this case. Try it yourself. Neither will nsDependentCString()
work. The NS_LITERAL_CSTRING seems to be just a macro for a
nsDependentCString(). This class does not belong to same class stream. Changing
to this will involve quite a bit of re-structuring.

I see you have also some comments on #78932 which I will address there. 

Comment 38

[chuang]

I've looked on 6/8/01 diffs for bug 78932, not much on the new files. r=chuang
provided no regression found on QA's end for existing features.

Comment 39

[John Marmion]

Attachment: Brief Changes requested by review - see latest comments #78932

Comment 40

[Dan Mosedale (:dmosedale, :dmose)]

John: ah, you're right.  I discussed this a bit with jag, and I think
this is to some approximation a bug in the way nsCStringArray is
specified.  But never mind that suggestion, then.

George: I can sympathize on this front, because I've had to land big
patches in the past as well.  However, I tend to infer a different
lesson from the statement you quote: not "checkin now; review later";
but "big landings are painful; split patches into small incremental
changes in order to avoid/amortize the pain".  However, that's just
me.  It is certainly true that most of my review comments thus far
have been style & speed-related, rather than correctness-related.
And another point in favor is that I seem to be the major bottleneck here,
but I'm only really on the hook for reviewing the LDAP changes, and those
can't really break existing functionality, since that part of the patch is
an entirely new feature.  I've asked Brendan to poke his head in here and voice
an opinion, so perhaps he can give us some guidance on what he thinks the best
plan for landing this is. 

Comment 41

[John Marmion]

Attachment: #74098 nsILDAPOperation::SearchExt() fix requires patch update to one file

Comment 42

[Brendan Eich [:brendan]]

I have no useful bromides to sell.  True, it's better to check in early and
often, which in the real world implies smaller changes per checkin.  Landing a
big hairy patch is never easy.  If reviewers pick over it, the patch loses
momentum and the patch tender has to keep all balls in the air.  If reviewers
rubberstamp it at some point, the remaining problems may not come out with
testing (which does not mean all such problems are trivial or of no consequence:
Mozilla didn't get too big and too slow with any one checkin.  Our testing does
not cover everything, and anyway [bromide alert], you can't test quality into
code once the design die is cast).

At some point, which we're probably past, the big-patch-stopped-by-one-reviewer
story ends with one party getting sleepy.  Either the patch is not approved by
the reviewer and the patch tender walks away, or the reviewer approves.  In the
latter case, the reviewer might rubberstamp, but that's not always the right
thing.  A better approach is to follow
http://www.mozilla.org/hacking/reviewers.html#rules-and-tips #18, and file bugs
on remaining open issues.  Is there a way to do that here?

/be

Comment 43

[Dan Mosedale (:dmosedale, :dmose)]

Since, as mentioned, I'm really just on the hook for the 17879 (LDAP) part of
the review, and since that code doesn't yet have any UI to access it unless
people mess with their prefs.js by hand, I'm ok with this code getting checked
in as is long as 17879 stays open and we continue to work on review issues
related to that there.

Unfortunately, the Mac and Linux commercial builds that I spun with these
patches from last night and this morning had a really nasty crasher in Necko in
them that made the commercial builds pretty useless for mail.  So I've finally
gotten a reasonable Linux commercial build from an updated tree, and will have a
Mac one tommorrow morning (I hope!).  So after those get posted, I'll be able to
solicit QA help in poking at them.

Comment 44

[John Marmion]

In the absence of a UI to activate both LDAP & MAPI, the most important issue 
with thus patch, I believe, is that it does not introduce a regression. We build
and test against the Head on a daily basis on both Linux and Windows. We also
test albeit less frequently on the Mac and Solaris. We are not able to test
against the Commercial tree or against the AIM stuff. But if QA were to throw
resources at these areas then we could have some degree of confidence that we
have not introduced/exposed any problems. 

I would be more than happy if QA could give us that confidence. It would then
allow us to land this patch and leave  #17879 open until the review process is
complete. There are 4 more patches queueing up behind this one. The main one is
#83023 which adds UI support for this patch. This will allow us to test the
functionality in this patch more extensively. 

Comment 45

[Scott MacGregor]

Forgive my ignorance but I'm seeing like 15 patches posted in this bug, which
one are you requesting a super review for? Thanks.....

Comment 46

[Dan Mosedale (:dmosedale, :dmose)]

They're all iterations of the same patch.  The most recent one is the one that
counts.

Comment 47

[Scott MacGregor]

thanks Dan. Wow, this diff is pretty huge. It's going to take me a while to find
the cycles to finish going through all of this. In the future, you'll find you
get much faster turn arounds for super reviews when making requests for smaller
things. i.e. working and landing in chunks. Instead of one big change when it's
all done. 

Comment 48

[John Marmion]

Attachment: fix to nsAbLDAPDirectoryQuery.cpp 'mLock' initialisation

Comment 49

[John Marmion]

Attachment: -No Code Change But Keep the patch up-to-date against today's head

Comment 50

[John Marmion]

Attachment: No Code Change but keep patch up-to-date against today's HEAD.

Comment 51

[John Marmion]

Attachment: Update Patch against Bug Fix #88413.

Comment 52

[John Marmion]

Attachment: Small change to Fix possible conversion problem.

Comment 53

[Dan Mosedale (:dmosedale, :dmose)]

I was reapplying on my Mac, and I went to abzilla.mozilla.org and got the
mac-makefile .sea, which was from early May.  Noticed a couple of minor things
that could effect building and testing:

* The addrbook/src/MANIFEST change in that .sea may effect building on all
platforms, so it should be part of the main patch.

* There has been a newer version of msgAddrbookIDL.mcp checked in, so this new
version will need to be checked out and updated with your changes and replace
the version currently in the .sea.

Comment 54

[John Marmion]

Attachment: Mac Project Files changed by this patch

Comment 55

[John Marmion]

Attachment: Include MANIFEST file in the patch as requested

Comment 56

[John Marmion]

Attachment: Keep patch up-to-date against the head

Comment 57

[Scott MacGregor]

I should be done looking at this patch by Monday night.

Comment 58

[Scott MacGregor]

Initial review comments:
Review Comments:

2) I see a couple pieces of code which look very similar. Any chance they could
leverage a shared method for turning the hash table into 
an array of ptrs then rebuilding the directory lists? In particular, I'm talking
about nsAbBSDirectory::CreateNewDirectory
and nsAbBSDirectory::CreateDirectoryByURI which seem almost identical.

3) Why do we need to worry about thread management in
nsAbRDFDataSource::CreateProxyObserver. Just looking at diffs it's hard to tell the
threading model. What part of the code is not running on the UI thread? Looks
like we are changing a lot of existing address book code, turning
objects into proxied objects. 

Performance impact of making the RDF data source proxy all the RDF observers.
Will this hurt performance of local address books that are always
on the UI thread?

4) Instead of if (mIsQueryURI == PR_TRUE), I find it much easier to read if it's
just if (mIsQueryURI). The if conditionals
are much easier to read this way. 
 
5) Why do we need to sprinkle all of these if (mIsQueryURI == PR_TRUE) clauses
around all the methods in nsAbMDBDirectory like AddCard,
DropCard, EditMailingList, etc. etc? Are you seeing cases where AddCard is
getting called while you are asynchronously performing a search on a local
address book? It wasn't clear to me why we needed all of these check points. 

6) In NS_IMETHODIMP nsAbMDBDirectory::StartSearch(), let's use nsCOMPtr w/
do_GetService instead of the old NS_WITH_SERVICE for getting the RDF service.
You've been using the com ptr version all over the places
looks like you just missed a spot when you added code to StartSearch.

7) In nsAbMDBDirectory::StopSearch, I find
if (!mIsQueryURI)
much easier to read than 
if (mIsQueryURI == PR_FALSE)


8) In the class declaration for nsAbMDBDirectory you use several nsCAutoStrings
as member variables. It doesn't make sense to use
a auto string in a class declaration. Just use nsCString.

9) I'm not sure I like the name nsIBooleanExpression (both the file names &
class names). Shouldn't we have an Ab qualifier in the file
name to match the convention used by the rest of the files in these directories?
i.e. nsIAbBooleanExpression.idl, nsIAbBooleanConditionString, etc.

10) In nsAbDirFactoryService::GetDirFactory you wrote your own parser to extract
the scheme of the url string out. We actually have
routines in our networking library that do this for you. Get the nsIIOService
and call extractScheme on it to get the scheme from
your url string.

11) In the outlook directory code, FillPropertyValues, a lot of places where we
have:
if (aCard == nsnull), if (uvalue != nsnull && nsCRT::strlen(uvalue) != 0), if
(newValue != nsnull), etc. It's much easier to read all
of these as (if (uValue && nsCRT::strlen(uvalue)) or if (!aCard). 

12) Also in FillPropertyValues, the string management here could be cleaned up a
bit:'
nsXPIDLString value ;
retCode = aCard->GetCardValue(cPropName, getter_Copies(value)) ;

const PRUnichar *uvalue = value.get();
if (uvalue != nsnull && nsCRT::strlen(uvalue) != 0) {
  nsString convertedString (uvalue) ;       
  newValue = new nsAbDirectoryQueryPropertyValue(cPropName, convertedString) ;

why even bother copying value into a nsString just to pass it into the ctor for
nsAbDirectoryQueryPropertyValue. Seems like the 
ctro should take a const PRUnichar. Then you can pass your XPIDLString (using
.get()) directly.

13) nsAbOutlookDirectory::DoQuery has a lot of if clauses where the logic can be
simplified (if foo) instead of if (foo == nsnull)

14) Small nit. In nsAbOutlookDirectory::GetChildNodes you have a nsCString
inside of a tight for loop (entryId). 
Let's move that variable declaration outside of the for loop and make it a
nsCAutoString. I bet the entry ID is usually smaller
than the 60 or so bytes on the stack that get allocated for an auto string.

15) nsAbOutlookDirectory::CommitAddressList has a nsCOMPtr<nsISupports> variable
that's also in a tight for loop. Let's pull that
declaration outside of the for loop too.

16) I bet in nsAbOutlookDirectory::CreateCard, entryString could be declared as
a nsCAutoString. Same thing with the uri variable
in that method. And with subURI in nsAbOutlookCard::Init

17) In UnicodeToWord, make your nsString a nsAutoString.

18) nsAbLDAPDirectory::Initiate has if conditions which can be simplified. So
does nsAbLDAPDirectory::InitiateConnection

19) I'm not a big fan of declaring methods to take nsCStrings when you don't
need too. I'm referring to methods like
nsAbDirectoryQueryPropertyValue::nsAbDirectoryQueryPropertyValue
 and nsAbDirectoryQueryPropertyValue::nsAbDirectoryQueryPropertyValue. Those
should be re-written to take const char *'s. 
You shouldn't force the caller to convert their char * into a nsCString to call
your method.

20) nsAbDirectoryQueryPropertyValue::GetName should just call
mName.ToNewCString() directly instead of using nsCRT::strdup

21) nsAbDirectoryQuery::query has a if condition which can be simplified. Same
for nsAbDirectoryQuery::matchCard 

22) nsAbDirectoryQuery::matchCardCondition has some string management which can
be simplified. We make an extra copy of _value into
a nsString just so we can get at a unichar ptr for the string. Look at the use
of _value, value and uvalue. You can do all that with
just the first string _value. Same thing for _matchValue, matchValue & value.

23) nsAbDirectoryQuery::queryMatch shouldn't we push the declaration of
nsCAutoString 'n' outside of the tight for loop. Esp.
if we have lots of values we are querying over. More funny string management in
this routine inside a else clause w/ value, uvalue and 'v'.

24) if condition simplificiation in nsAbDirectoryQuery::queryFinished and in
nsAbQueryLDAPMessageListener::Cancel and in
nsAbQueryLDAPMessageListener::OnLDAPMessage and in
nsAbQueryLDAPMessageListener::OnLDAPMessageSearchEntry and in
nsAbQueryLDAPMessageListener::QueryStopped 
 and in nsAbQueryLDAPMessageListener::QueryError and in
nsAbLDAPDirectoryQuery::Initiate and in nsAbLDAPDirectoryQuery::DoQuery

25) In nsAbLDAPDirectoryQuery::DoQuery, 'scope' should be a nsCAutoString.

26) I'm not too familiar with the mozilla ldap code so I'll assume dmose gave a
close look over the LDAP interaction in the nsAbLDAPQuery class.

27)  nsAbLDAPDirectoryQuery::setLdapUrl should take a const char * instead of a
nsCString so we don't force callers
to convert up to a nsCString.

28) nsAbLDAPDirectoryQuery::getLdapReturnAttributes: should move declaration of
'n' outside the tight for loop.
 
29) MozillaLdapPropertyRelator::findMozillaPropertyFromLdap and
MozillaLdapPropertyRelator::findLdapPropertyFromMozilla
 have some if conditional simplifications that can be made.

30) nsBooleanConditionString::GetName should be re-written to just return
mName.ToNewCString. 
31) nsBooleanConditionString::SetName has a redundant conditional check on aName:

if (!aName)
   return NS_ERROR_NULL_POINTER;
if (aName)
   mName = aName;

32)nsAbBoolExprToLDAPFilter::FilterExpressions: never pass a AutoString as an
argument to a function. change this to a nsCString.
Same thing for nsAbBoolExprToLDAPFilter::FilterCondition.

33) Strange string management in nsAbBoolExprToLDAPFilter::FilterCondition with
'name', 'n' and 'ldapProperty'. Extra string copies
when you can use 'name' everywhere directly. Same thing with 'value', 'v', vUTF.

34) if conditional simplification in CStringArrayToCharPtrArray::Convert

35) if conditional simplification in nsAbQueryStringToExpression::Convert and in
nsAbQueryStringToExpression::ParseExpression
and in nsAbQueryStringToExpression::ParseExpressions

36) lot of conditional simplifications in
nsAbQueryStringToExpression::CreateBooleanConditionString
37) In the declaration for nsAbDirectoryRDFResource, don't use nsCAutoStrings as
member variables. Use nsCStrings. 
38) A lot of conditional logic simplifications can be made in the if clauses in
methods for nsMapiAddressBook.

Comment 59

[Scott MacGregor]

Those are some of my initial review comments. As Brendan mentioned in this bug
in an earlier comment, "....story ends with one party getting sleepy...". I'm
sleepy.  I didn't look at the last 3rd of the diff as closely as I should have.
I'll try to look again tomorrow. 

Also I wanted to be sure of the following:
1) The dmose has given an r= stamp for the LDAP search query code.
2) that chuang (as the address book module owner) has given a r= stamp on the
changes for local address books and the new files to the address book directory. 
3) apologies if my sr comments are bit on the rambling side. I didn't edit them
before I hit the commit button and not it's too late. 

Thanks for the good work.

Comment 60

[Paul.Sandoz]

	Thanks for making the time to review the code.
	
	John is away today so i took the responsibility
	to comment.
	I categorised the items as:
	
	
	Fix:
		2,6,8,10,12,14-17,19,20,22,23,25,27,28,30-33,37
		
	Syntactic:
		4,7,11,13,18,21,24,29,34-36,38
		
	Clarification:
		3,5,9,26
		

	We have done the fixes and are currently working
	on the patch and testing it. In general with
	respect to the string related fixes the pattern
	was to convert the XPIDL strings to the nsString
	equivalent to avail of the methods on the class.
	We have removed as much redundancy as possible
	without affecting the reliance on the class
	methods.
	(it would be great if XPIDL and nsString could
	 be merged!)
	
	Not sure I agree with the syntactic items at least
	at the moment because time is very tight, perhaps
	if this is considered important we could log a bug
	to change these?
	

Clarification comments
----------------------
>3) Why do we need to worry about thread management in
>nsAbRDFDataSource::CreateProxyObserver. Just looking at diffs it's hard to tell
the
>threading model. What part of the code is not running on the UI thread? Looks
>like we are changing a lot of existing address book code, turning
>objects into proxied objects. 
>
>Performance impact of making the RDF data source proxy all the RDF observers.
>Will this hurt performance of local address books that are always
>on the UI thread?
>
	
	The LDAP address book asynchronously returns cards.
	
	However the code is efficient: it will not use proxies if
	the thread of the notify procedure is equal to the UI thread.
	Infact proxies will not be created until they are required
	i.e. no LDAP search no proxies.

	
>5) Why do we need to sprinkle all of these if (mIsQueryURI == PR_TRUE) clauses
>around all the methods in nsAbMDBDirectory like AddCard,
>DropCard, EditMailingList, etc. etc? Are you seeing cases where AddCard is
>getting called while you are asynchronously performing a search on a local
>address book? It wasn't clear to me why we needed all of these check points. 
>

	The RDF resources can have two behaviours depending
	if the resource URI contains a query paramter. If it
	does contain a query parameter certain operations are
	not possible, like AddCard.
	
	I personally do not think that this is a good OO
	solution... but it works!
	
	Although our UI patch (i think?) will ensure that
	it is not possible to add/delete cards when a query
	view result is being displayed via desensitive buttons
	we wanted to ensure that these methods returned errors
	for other, e.g. scriptable, types of clients.	


>9) I'm not sure I like the name nsIBooleanExpression (both the file names &
>class names). Shouldn't we have an Ab qualifier in the file
>name to match the convention used by the rest of the files in these
directories?
>i.e. nsIAbBooleanExpression.idl, nsIAbBooleanConditionString, etc.
>

	Yep.
	We think that these are really more generic functionality
	that could be useful in other areas. We were not exactly
	sure where their home should be! So would prefer if there
	was no 'Ab' namespace and could be placed in another
	area some time later?


>26) I'm not too familiar with the mozilla ldap code so I'll assume dmose gave a
>close look over the LDAP interaction in the nsAbLDAPQuery class.
>
	
	X fingers :-)	


Fix comments
------------	
>10) In nsAbDirFactoryService::GetDirFactory you wrote your own parser to
extract
>the scheme of the url string out. We actually have
>routines in our networking library that do this for you. Get the nsIIOService
>and call extractScheme on it to get the scheme from
>your url string.
>

	Changed to use the nsIIOService: used the start/end
	parameters for minimal change to the existing code.
	Could be better implemented using full string class
	functionality later?

Comment 61

[Cuchulainn]

Attachment: New patch with Paul's changes to address issues raised by super-review (see Paul's comments on 24-07-01)

Comment 62

[Dan Mosedale (:dmosedale, :dmose)]

mscott: I have not yet finished the review for the LDAP query stuff.  Since it
doesn't actually get exposed to the UI in this patch (you have to manually edit
prefs.js), I agreed to review this before the UI lands.  The LDAP query stuff
review that I've already done is in bug 17879.

I did this in part on Brendan's advice; see the discussion comments in this bug
between George Drapeau, Brendan and myself from 6/13 and 6/14.

In the spirit of this, if you're happy with the rest of Paul's changes, I'd like
to support his suggestion of allowing a separate bug to be opened about these
syntactic changes so that we can get at least the basic functionality checked in
for 0.9.3.
  

Comment 63

[Dan Mosedale (:dmosedale, :dmose)]

The most recent patch attached is wrong: it appears to be three
almost-but-not-quite-identical copies of the entire patch appended together.

Comment 64

[Dan Mosedale (:dmosedale, :dmose)]

OK, after comparing the three different patches with xxdiff, it appears that
they are just subsequent iterations, and that the patch generator intended each
subsequent iteration to overwrite rather than append to the previous file.  So
here's just the final patch from the last attachment.  I've also taken the
liberty of removing 63 instances of ^M which were in the patch.

Comment 65

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Previous patch had issues; this attempts to correct.

Comment 66

[John Marmion]

Attachment: Keep patch up-to-date with Head following #86734, 73353

Comment 67

[John Marmion]

Attachment: 2 line changes for fix to a MAPI issue following SR changes

Comment 68

[Scott MacGregor]

Hey Paul, you mentioned;
"In general with respect to the string related fixes the pattern
was to convert the XPIDL strings to the nsString
equivalent to avail of the methods on the class.
We have removed as much redundancy as possible
without affecting the reliance on the class
methods. (it would be great if XPIDL and nsString could be merged!)"

Good news. Both nsXPIDLString and nsString now inherit from nsAFlatString. Most
of your nsString methods come from nsAFlatString so you shouldn't have to copy
an xpidlstring to a nsString just to call nsString methods on it anymore. You
should be able to call the same methods directly on your nsXPIDLString.

2) Okay, if we don't need to create a proxy for the data source observer then we
don't. That's cool! But why do we need this proxy code at all? I'm still fuzzy.
Just because a search is asynchronous doesn't mean it runs on another thread. Or
is it just a fact that LDAP queries get spun up on another thread and processed
on that thread? Do we want to force an LDAP search to run on another thread or
should we just run it asychronously on the UI thread (that's how most of the
search code in mailnews currently works today). I'm just trying to feel out that
we intentionally decided to spin up queries in another thread & that we feel
that's the best design approach from a performance point of view.

3) I'm still not sure about the class & file names for:
nsIBooleanExpression.idl, nsIBooleanConditionString, etc. They look pretty
specific to doing LDAP search queries to me. If we really think we are going to
use them somewhere else, then where would that some where else be? Where would
you move these files to? I feel like if we are going to put them in the address
book module, they should have the address book file prefix to be consistent with
the rest of the files in that module. What do you think?

4) Thanks for switching to use the networking service to parse the URL scheme.

Comment 69

[Paul.Sandoz]

>Both nsXPIDLString and nsString now inherit from nsAFlatString

	Ah! wonderful, i should have checked the new string stuff
	more closely. Ok, i think for the most part we can 
	remove the redundancy, not sure if there is any
	functionality in the old strings classes which is not
	in the new... soon find out!

2) To proxy or not to proxy, or where to proxy

	The LDAP search results are returned on the thread that
	is communicating with the LDAP server so at some point we
	have to proxy to the UI thread.
	Outlook searches are also asynchronous on another thread
	since it could potentially take a long time to perform the
	search (e.g. going to an exchange server) and did not want
	the UI to block.

	I did not want to proxy at the LDAP level (proxying the LDAP
	listener object) because i thought it should be up to the client
	of the address book component to decide.
	We are using the address book components in the OpenOffice
	address book database driver. It relies on async threading to
	perform SQL queries, mapped to boolean expression, on the
	underlying address books.


3) The homeless boolean

	They are actually a general way to define simple
	hierarchical boolean expressions for string matching
	and there is general code to map an expresion encoded
	in a URI to the expression model. So could be 
	potentially useful for defining simple searches on
	RDF datasources. The search itself is defined in
	the graph and represents a subset.

	But... i don't have any answers to your questions!
	which implies their home should be where they are
	used now. So we will change the name space.

Comment 70

[John Marmion]

Attachment: SR Changes

Comment 71

[John Marmion]

I have attached a new patch. Changed the nsIBoolean class and file names to
nsIAbBoolean as requested by the 'sr'. Also cleared up some redundancy in the
string handling as also requested by the 'sr'. Hopefully that covers all the
areas.

Comment 72

[Scott MacGregor]

excellent. sr=mscott

I'd also like to see the syntactical changes I suggested get cleaned up at some
point. Can you file a bug to take care of issues: 4,7,11,13,18,21,24,29,34-36,38

You also still need to get your r= from the module own of the address book (chuang)

Comment 73

[chuang]

r=chuang for address book part except the LDAP area.

Comment 74

[John Marmion]

I have created #93092 as placeholder for the syntactical issues raised in the
sr.

Comment 75

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: updated .mcp files to handle *Bool* to *AbBool* name change

Comment 76

[Dan Mosedale (:dmosedale, :dmose)]

While doing some final testing last night prior to a hoped-for checkin, I came
across an issue.  Early on in this bug, I wrote:

* New URI schemes that are being introduced need to start with "moz-"
  as per bug 69513

However, subsequent patches changed all addrbook-related schemes to start with
"moz-", not just new ones introduced in this patch.  I noticed this a while ago,
but didn't bring it up because it hadn't occurred to me that it would cause
problems.  Doh!

The problem is that some of those URL schemes are already being written out in
the user's profile (at least in abook.mab; perhaps elsewhere as well) in
shipping products.  Changing the existing schemes can gratuitously cause users
who upgrade, then at some point have a problem and need to downgrade, to have a
semi- or non-functional addressbook when running with now-current or older versions.

So I think the patch needs to be changed to match the original suggestion: use
"moz-", but only in front of brand new URI schemes (presumably this set includes
 at least the ldap and outlook related schemes).

Comment 77

[Dan Mosedale (:dmosedale, :dmose)]

I talked to dbaron about this, and he pointed out that one other possible
solution to this would be to make the current code read and understand the
changed schemes at the moment, but continue to write the old ones.  Then at some
future point we could turn on writing of the new scheme names and people would
only be hit by this problem if they were downgrading to really old software,
which would happen less frequently.

However, this strikes me as more work than just reverting to the old scheme
names for existing schemes, and it doesn't really seem terribly likely that
someone else is going to try and register (eg) abcard: with the IETF.

Comment 78

[John Marmion]

Dan, I need you to absolutely certain about this. I was also concerned about
this change because I knew it would have a knock on effect on the commercial
tree. But remember we did change the URI scheme in our first patch #69480. In
order to support multiple address books we introduced the following:

	abdirectory://
		abmdbdirectory://abook.mab
		abmdbcard://abook.mab/Card1

where abdirectory:// is the root directory.
This would then get extended to in this patch to:

		abldapdirectory://hostname:port/{dn path}
			abldapcard://hostname:port/{dn path}/{dn card}

		aboutlookdirectory://outlook
			aboutlookcard://outlook/Card1
     
The #69480 has been in the tree since April, so presumably changing the URI
scheme then has had no ill effects. I know that Candice had to change the AIM
tree to support this change and to support backward compatability. We can only 
presume that this has been successful. So why should this change break that? I
will talk to you today to clarify this. It would be good to get Candice's input
into this as she oversaw the original change into AIM.

The change to the patch will be minor if we need to reverse this. I will go
ahead and have it prepared/tested on our end.
				

Comment 79

[John Marmion]

Attachment: Remove the 'moz-' prefix from all existing URIs.

Comment 80

[John Marmion]

Just attached a patch which removes the 'moz-' prefix from the existing URIs. I
have spoken with Dan who will investigate with Candice whether we need to revert
to this.

Comment 81

[Dan Mosedale (:dmosedale, :dmose)]

Candice checked out this stuff today, and figured out a few things:

the 69480 landing did in fact cause an analogous problem, but noone has yet
reported it.

But, on the bright side, it turns out that the only place any URIs were ever
written into the profile directory is in a couple of spots in the commercial
tree; nowhere in the mozilla tree.  So we can handle that with a patch to the
commercial tree, and leave all the moz- URI schemes intact.  I hope to get the
7/30 patch checked in tommorrow (Saturday).

Comment 82

[Dan Mosedale (:dmosedale, :dmose)]

Code checked in.

Comment 83

[Jacek Piskozub]

Not functional address book on existing profiles (probably caused by this
check-in) is bug 93848.

Comment 84

[yulian chang]

Verified with 20010815 trunk build

Comment 85

[Jimisola Laursen]

I added bug #368355 9 months ago and it has not received any attention. Thought that a comment here might give it the initial attention thatit needs.

368355:
Outlook Contacts integration causes TB autocomplete freezes up to 1 minute after typing one or two characters of an email adress