John Marmion Reporter
	Description • 23 years ago

The 'sr' by mscott@netscapoe.com for bug 78933 resulted in the following issues:



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.

It was agreed that the following issues 4,7,11,18,21,24,29,34,35,36,38 were 
syntax related and should be logged in a separate bug. All other issues should
be addressed. This bug is a placeholder for the above 'syntax' issues.

	John Marmion Reporter
	Comment 1 • 23 years ago

Some progress has been made on this bug and i see more may be in the pipeline
with the fix for bug #83091. Here is a summary to-date of progress against the
syntax issues:

#4, #5 - Some progress
#7  None
#11 Fixed
#18 None
#21 None
#24 None
#29 Fixed
#34 None
#35 None
#36 None
#38 Fixed

	John Marmion Reporter
	Comment 2 • 22 years ago

Re-assigning this bug to my colleage sean.gao@sun.com 

Just to clarify this bug. A number of what we regarded as syntax issues were
raised against a very large patch. These issues are
#4,#5,#7,#18,#21,#24,#29,#34,#35,#36 and #38 only of the above list. This bug is
about those issues only. They are solely concerned with the following type of
statements e.g.:

if(matchFound == PR_TRUE)  

http://lxr.mozilla.org/seamonkey/source/mailnews/addrbook/src/nsAbDirectoryQuery.cpp#492

or if(xxx == PR_FALSE)

these statements are still found in the following files:

http://lxr.mozilla.org/seamonkey/source/mailnews/addrbook/src/nsAbDirectoryQuery.cpp

http://lxr.mozilla.org/seamonkey/source/mailnews/addrbook/src/nsAbDirectoryQueryProxy.cpp

http://lxr.mozilla.org/seamonkey/source/mailnews/addrbook/src/nsAbLDAPDirectoryQuery.cpp

http://lxr.mozilla.org/seamonkey/source/mailnews/addrbook/src/nsAbUtils.cpp


again many have been addressed since i last looked at this some time ago. Though
I see a few more were introduced by later patches. But I always believed these
were about style rather than substance.