95117 - Log on feature in Simple MAPI

Status: VERIFIED FIXED

(MailNews Core :: Simple MAPI, defect, P1)

Description

[Tiantian Kong]

This is to track the implementation for the Log on feature in Simple MAPI.

Comment 1

[Hong Kwon]

mass qa assigning all simple MAPI bugs to olgac 

Comment 2

[Srilatha Moturi]

Attachment: patch v1

Comment 3

[Rajiv Dayal]

[copied from bug # 91702]
------- Additional Comments From [e-mail] 2001-08-23 18:32 -------

My first comment is - don't use tabs.  It looks like all your files have tabs in
them. You should use 2 or 4 space indentation. You can configure msdev to not
put tabs in, and you can also use it to reformat all your files to take out the
tabs (edit | advanced |format selection)

Also, you should use the relevant mozilla conventions when possible (PR_TRUE
instead of TRUE, PRBool instead of BOOL, nsCRT::strcmp instead of strcmp. etc).
Some of your methods/files probably need to use COM conventions, but any mozilla
code, even if it's windows-only, should use mozilla conventions.

Comment 4

[Rajiv Dayal]

we have tried to make sure that the mozilla side of code uses only mozilla types 
(eg:mapihook.cpp) and the only code that requires usage of MS COM uses C++/COM 
types (there is lot of code using MS COM here though). However, we will all have 
a look again to make sure we adhere to mozilla standards if we missed out.

Comment 5

[Rodney Velasco]

QA over to trix.

Comment 6

[Srilatha Moturi]

Attachment: patch v2

Comment 7

[Srilatha Moturi]

In the latest patch 
- Removed all the tabs
- changed all the data types to mozilla compliance except for Microsoft specific
code.

ccing Doug for review.

Comment 8

[David :Bienvenu]

In PRBool nsMapiHook::Initialize()


+        else
+            bResult = 0;

should that be bResult = PR_FALSE?

and 
+                isMapiService = TRUE;

should be PR_TRUE.

and the variable isMapiService;

should be named m_isMapiService, so that people reading the code can tell that
it's a member variable.

Also, all the parameters to nsMapiImp::Login

should use the mozilla convention of having var names that start with a for arg,
e.g., aPassword.

 PRUnichar *Name = nsnull, *Pass = nsnull;
+

this should use an nsXPIDLString, and have lowercase names, e.g., name, pass. If
you use nsXPIDLString, you won't need the delete [] to free the storage.

here, nsMAPIConfiguration::nsMAPIConfiguration(

+  m_ProfileMap(nsnull),
+  m_SessionMap(nsnull)

why init these to nsnull if you're just going to assign them later on in the
constructor?

+static void NS_ShutDownMapiSupport()

should use nsnull here : pTemp = NULL;

Also, we don't use the standard template libraries, which is what I assume this is:

+typedef std::map<PRUint32, nsMAPISession *> SessionMap;  // session_id and
session object
+typedef std::map<PRUnichar *, PRUint32> ProfileMap;        // username and
session_id

but I'm not going to ask you to rewrite that at this point. (we also don't use
::sn_printf - we use our own string classes).

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 47532 [details] [diff] [review]
patch v2

>diff -u -r1.22 makefile.win

We need more than the default -3 context in review patches. At least -9 for my tastes, and
some patches deserve/require much more to make much sense of. If reviewers have to dig
up the original source files to make sense of a patch it's likely to drop to the bottom of their
pile of things to do.

>-DIRS=public base db local news imap mime compose addrbook import absync
>+DIRS=public base db local news imap mime compose addrbook import absync nsmapi

Please re-name this to simply "mapi", there doesn't appear to be anything Netscape specific
here. If there were we'd not be allowed to check into the Mozilla tree and you'd have to
re-do the patch anyway.

>RCS file: /cvsroot/mozilla/mailnews/base/resources/locale/en-US/messenger.properties,v
>+
>+# MAPI Messages
>+loginText=Please enter password for 

the localization folks won't like this, it clearly implies string contatenation which won't
work right in languages with different grammar. Use a %s and PL_snprintf().


>Index: mailnews/nsmapi/makefile.win
>+# The contents of this file are subject to the Netscape Public
>+# License Version 1.1 (the "License"); you may not use this file

We're encouraged to use the *Mozilla* public license on new files. The special
rights in the NPL have expired so using it only serves to annoy mozilla-zealots.

>+# The Initial Developer of the Original Code is Netscape
>+# Communications Corporation.  Portions created by Netscape are
>+# Copyright (C) 1998 Netscape Communications Corporation. All

If this is new code you don't want to copyright it three years ago.,

These comments apply to other new files in the patch.

>Index: mailnews/nsmapi/mapi32/makefile.win
>===================================================================
>+
>+DIRS=src

There's nothing wrong with putting the source files directly in this directory. By creating
the src subdirectory you've just added yet another makefile to the build and increased the
time it takes for no particular reason. Unless you eventually plan on needing a "public"
directory parallel to this please consider eliminating this.


>Index: mailnews/nsmapi/mapi32/src/makefile.win
>+
>+DEFINES=-DUNICODE -D_UNICODE
>+LCFLAGS=	\
>+	$(DEFINES)	\
>+	$(NULL)

DEFINES means nothing in a windows build, please simplify this by putting
your defines directly into the LCFLAGS variable.

>+CPPSRCS= \

CPPSRCS means nothing in a windows build, please skip this section

>+CPP_OBJS= .\$(OBJDIR)\nsMapiDll.obj   \
>+	$(NULL)

This works, but FYI both CPP_OBJS and C_OBJS are stuffed into OBJS which
you could use instead. Using CPP_ and C_ is mostly useful when you have a mix
of file types (which you don't) and are trying to keep a windows makefile roughly
parallel to a unix makefile (which uses CPPSRCS and CSRCS). In this case of a
windows only modulefile using OBJS would be simpler.

Putting an item on the same line as the variable name mostly defeats the ease-of-
maintenance purpose of using the $(NULL). Simplify it, or go whole hog, either:

    CPP_OBJS = .\$(OBJDIR)\nsMapiDLL.obj

or

   CPP_OBJS = \
         .\$(OBJDIR)\nsMapiDLL.obj \
         $(NULL)


>+WIN_LIBS=	\
>+	kernel32.lib	\
>+	user32.lib	\
>+	gdi32.lib	\
>+	winspool.lib	\
>+	comdlg32.lib	\
>+	advapi32.lib	\
>+	shell32.lib	\
>+	ole32.lib	\
>+	oleaut32.lib	\
>+	uuid.lib	\
>+	odbc32.lib	\
>+	odbccp32.lib	\
>+	$(NULL)

Holy cow, do you really need all those to implement Logon and Logoff?

>Index: mailnews/nsmapi/mapi32/src/nsMapiDll.cpp
>===================================================================
>RCS file: nsMapiDll.cpp
>diff -N nsMapiDll.cpp
>--- /dev/null	Wed Apr 26 15:53:02 2000
>+++ nsMapiDll.cpp	Wed Aug 29 13:19:34 2001
>@@ -0,0 +1,172 @@
>+#include <windows.h>

Please add the MPL license header to this file, or if this is taken from somewhere else please
put the appropriate attribution and copyright on it.

>+ULONG FAR PASCAL MAPILogon (ULONG ulUIParam, LPTSTR lpszProfileName, \
>+                            LPTSTR lpszPassword, FLAGS flFlags, \
>+                            ULONG ulReserved, LPLHANDLE lplhSession)

>+    dwResult = ::GetModuleFileName(NULL, title, sizeof(title)/sizeof(TCHAR));

Shouldn't you use title[_MAX_PATH+1] rather than hardcoding 256 and hoping for the best?

>Index: mailnews/nsmapi/mapihook/makefile.win
>+DIRS=src

ditto comments about empty build dirs with a single subdirectory from above.

>Index: mailnews/nsmapi/mapihook/src/Registry.cpp
>===================================================================
>RCS file: Registry.cpp
>diff -N Registry.cpp
>--- /dev/null	Wed Apr 26 15:53:02 2000
>+++ Registry.cpp	Wed Aug 29 13:19:34 2001
>@@ -0,0 +1,317 @@

License? This code looks cribbed from MS samples or something... is it OK to use? I'll
go ahead and assume this code is OK.

>Index: mailnews/nsmapi/mapihook/src/Registry.h
>===================================================================
>RCS file: Registry.h
>diff -N Registry.h
>--- /dev/null	Wed Apr 26 15:53:02 2000
>+++ Registry.h	Wed Aug 29 13:19:34 2001
>@@ -0,0 +1,24 @@

License?

>Index: mailnews/nsmapi/mapihook/src/makefile.win
>+MODULE=nsMapi
>+LIBRARY_NAME=$(MODULE)
>+MAKE_OBJ_TYPE = DLL
>+DLL = $(MODULE).dll

DLL is automatically set based on LIBRARY_NAME, you shouldn't need to
do this explicitly. You *do* need to set either MODULE_NAME or EXPORT_LIBRARY
before this will happen, depending on whether you're a "component" or not.

>+DEFINES=-DUNICODE -D_UNICODE
>+LCFLAGS=	\
>+	$(DEFINES)	\
>+	$(NULL)
>+
>+CPPSRCS= \

See makefile comments in the other directory...

>+LLIBS=                               \
>+ $(DIST)\lib\xpcom.lib               \
>+ $(DIST)\lib\nsldap32v40.lib         \
>+ $(DIST)\lib\js3250.lib              \
>+ $(DIST)\lib\util.lib                \
>+ $(DIST)\lib\gkgfx.lib            \
>+ $(DIST)\lib\expat.lib               \
>+ $(DIST)\lib\png.lib                 \
>+ $(DIST)\lib\mng.lib                 \
>+ $(DIST)\lib\mpfilelocprovider_s.lib \
>+ $(LIBNSPR)                          \
>+ $(NULL)

It really seems unlikely that you need all of these, or even most of them.

>+WIN_LIBS=	\
>+	kernel32.lib	\
>+	user32.lib	\
>+	gdi32.lib	\
>+	winspool.lib	\
>+	comdlg32.lib	\
>+	advapi32.lib	\
>+	shell32.lib	\
>+	ole32.lib	\
>+	oleaut32.lib	\
>+	uuid.lib	\
>+	odbc32.lib	\
>+	odbccp32.lib	\
>+	$(NULL)

ditto.

>+
>+include <$(DEPTH)\config\rules.mak>
>+
>+LIBRARY=.\$(OBJDIR)\nsMapi.lib

LIBRARY is automatically set based on LIBRARY_NAME

>+install:: $(LIBRARY)
>+	$(MAKE_INSTALL) $(LIBRARY) $(DIST)\lib
>+install:: $(DLL)
>+	$(MAKE_INSTALL) $(MODULE).$(DLL_SUFFIX) $(DIST)\bin

This, too, is automatically done if you've set up your makefile correctly. If you're
a component, set MODULE_NAME (and you shouldn't be exporting the import .lib),
otherwise set EXPORT_LIBRARY=1 and everything happens magically.

see rules.mak for details.

>\ No newline at end of file

Please fix these.

>Index: mailnews/nsmapi/mapihook/src/nsMapiFactory.cpp
>===================================================================

License? I'll stop commenting on this.

>+nsMapiFactory::~nsMapiFactory()
>+{
>+    PR_Unlock(m_Lock);
>+    PR_DestroyLock(m_Lock);
>+}

How could this be locked, since everywhere else you have lock/unlock in pairs? Oh, in the
release further down... see comments below.

>+STDMETHODIMP_(ULONG) nsMapiFactory::AddRef()
>+{
>+    PR_Lock(m_Lock);
>+    temp = ++m_cRef;
>+    PR_Unlock(m_Lock);

Use PR_AtomicIncrement() instead of locking -- faster, less overhead.

>+STDMETHODIMP_(ULONG) nsMapiFactory::Release() 
>+{

ditto for PR_AtomicDecrement, and by avoiding the lock you don't need the
naked PR_Unlock() in the destructor and the early return.

    PRInt32 tmp = PR_AtomicDecrment(m_cRef);
     if (tmp == 0)
         delete this;
     return tmp;

If this is the only reason you use a lock then you can get rid of the lock itself
as well as the explicit ctor and dtor since they do nothing else.

>Index: mailnews/nsmapi/mapihook/src/nsMapiHook.cpp
>+#define NS_PROMPTSERVICE_CID \
>+ {0xa2112d6a, 0x0e28, 0x421f, {0xb4, 0x6a, 0x25, 0xc0, 0xb3, 0x8, 0xcb, 0xd0}}

The fact that you had to do that should be a red flag. Something's wrong, and this
attempt to get around it is fragile in the face of people changing the promptservice.

>+static NS_DEFINE_CID(kStringBundleServiceCID, NS_STRINGBUNDLESERVICE_CID);
>+static NS_DEFINE_CID(kPromptServiceCID, NS_PROMPTSERVICE_CID);

You should be using CONTRACTID's instead of CID's when possible.

>+    PRBool bResult = PR_FALSE;
>+    bResult = CheckProfile();

Why not do that on one line?

Reading farther, why do it at all? you don't seem to use the result.

>+    nsCOMPtr<nsIAppShellService> appShell = \
>+    do_GetService( "@mozilla.org/appshell/appShellService;1", &rv);

The do_GetService should be indented a lot more. It's slightly more efficient
to assign this in the constructor, but as the resulting line is somewhat uglier
I don't mind the assignment. But do indent, preferably double your normal indent
so this doesn't get confused with program control flow.

>+void nsMapiHook::CleanUp()
    .....
>+              native->SetIsServerMode( PR_FALSE );
>+              // This closes app if there are no top-level windows.

This shuts off -turbo mode, doesn't it? That seems like an evil thing to do
if you weren't the one who put it in that mode.

>+    NS_WITH_PROXIED_SERVICE(nsIPromptService, dlgService, kPromptServiceCID, 
>+                          NS_UI_THREAD_EVENTQ, &rv);

A more approved way to get the service is shown at
http://lxr.mozilla.org/mozilla/source/editor/base/nsEditorShell.cpp#1850

But then you'd have to get the proxy yourself.

Dougt: NS_WITH_SERVICE() was deprecated long ago, you should follow suit
and create an equivalent to do_GetService(), like do_GetProxiedService(). Then
you can use overloading to make both CID's and contractID's work. Alternately
you could add another constructor to nsProxiedService.

>+        nsIDOMWindowInternal *hiddenWindow;
>+        rv = appShell->GetHiddenDOMWindow(&hiddenWindow);
>+        if (NS_FAILED(rv)) return PR_FALSE;

I think using the hidden window is strongly frowned upon. You'll have to talk to some
of the GUI folks (like danm?), but for one thing it can strand modal dialogs behind
an open window, which then won't respond so you can move it out of the way (particularly
on mac). I believe it'll work OK if you pass nsnull instead of the hiddenWindow, the
"windowwatcher" will take its best guess.

I've got to go home, I'll pick up the review at this point later...

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 47532 [details] [diff] [review]
patch v2

>Index: mailnews/nsmapi/mapihook/src/nsMapiHook.cpp
>===================================================================
>+PRBool nsMapiHook::VerifyUserName(PRUnichar * pUsername, char **pIdKey)

As bienvenu commented,  our coding standards for Mozilla components (to the minimal
extent we have them) are to preface argument names with an "a", and to eschew MS
hungarian notation. So aUsername and aIdKey here, and similarly elsewhere.

How exposed is this interface? If it's internal to your module you should still
probably assert the arguments aren't null to protect against future maintenance
problems. If it's public to other modules you should be more defensive and check
in optimized builds as well.

>+    for (PRUint32 i = 0; i < numIndentities; i++)
>+    {
>+        // convert supports->Identity
>+        nsCOMPtr<nsISupports> thisSupports;
>+        rv = identities->GetElementAt(i, getter_AddRefs(thisSupports));
>+        if (NS_FAILED(rv)) continue;
>+        nsCOMPtr<nsIMsgIdentity> thisIdentity = \
>+                                   do_QueryInterface(thisSupports, &rv);
>+        if (NS_SUCCEEDED(rv) && thisIdentity)
>+        {
>+            nsXPIDLCString email;
>+            rv = thisIdentity->GetEmail(getter_Copies(email));
>+            if (NS_FAILED(rv)) return PR_FALSE;

do you want to return here, or do you want to continue as you do above?

>+            PRInt32 index = email.FindChar('@');
>+            if (index < 0)

You should compare to kNotFound instead of < 0.

maybe not, I just noticed that is only defined in the "obsolete" string headers and that
the actual nsAString code returns a raw -1. I wonder why the change, the const
seemed more readable to me...

>+            nsXPIDLCString cEmail;
>+            cEmail.Adopt(ToNewCString(Substring(email, 0, index)));
>+            NS_ConvertASCIItoUCS2 p_emailName (cEmail.get());

This makes no sense to me -- why the round trip from UCS2 to ASCII and back, lopping off bytes
with abandon as you go? If you're giong to convert use the correct charset conversion, but I
don't really see what you're accomplishing since you round trip here.

The main purpose of nsXPIDL[C]String is to capture return arguments from XPCOM methods.
You're not doing that here so that's a sign you should probably be using one of the more standard
string classes.

>+            nsDependentString cUsername(pUsername);
>+
>+            if (p_emailName == cUsername)

I guess operator== is defined, but it sure looks like you're comparing pointers to someone
who doesn't know that.

Wouldn't some variation on the following avoid this whole mess?

     if (Substring(email,0,index).Equals(pUsername))   // change to aUsername

>+                rv = thisIdentity->GetKey(pIdKey);
>+
>+                if (NS_FAILED(rv))
>+                    return PR_FALSE;
>+
>+                return PR_TRUE;

what happens if pIdKey (shd be aIdKey) is null?

Since you're not using the actual rv value this whole chunk could be simplified to

                 return NS_SUCCEEDED(thisIdentity->GetKey(aIdKey));

>Index: mailnews/nsmapi/mapihook/src/nsMapiHook.h

License?

>Index: mailnews/nsmapi/mapihook/src/nsMapiImp.cpp

>+/*void DisplayDialog(char *message)
>+{
>+    TCHAR junk[256];
>+    MultiByteToWideChar(CP_ACP, 0, message, -1, junk, MAX_NAME_LEN);
>+    MessageBox (0, junk, junk, 0);
>+}
>+
>+void DisplayDialogInt(LONG value)
>+{
>+    char buf[10];
>+    sprintf (buf, "%d", value);
>+    DisplayDialog(buf);
>+}*/

There's not much point in checking in already dead code, please remove

>+nsMapiImp::nsMapiImp()
>+: m_cRef(1)
>+{
>+    m_Lock = PR_NewLock();

See earlier comments about possibly using PR_Atomic..crement functions instead of locks

>+STDMETHODIMP nsMapiImp::Login(unsigned long ulUIArg, LOGIN_PW_TYPE bsLogin, LOGIN_PW_TYPE bsPassWord,
>+                unsigned long ulFlags, unsigned long *ulSessionId, LOGIN_PW_TYPE bsTitle)
>+{

More comments would really help the reviewers and future maintainers.

>+    nsString nsProfileName ;
>+    nsString nsPassCode ;

You're not actually taking advantage of any of our string class features here, and the use you do make
of these variables is forcing extra allocations and string copies. It'd be better to use raw string pointers
than this strange mixture.

>Index: mailnews/nsmapi/mapihook/src/nsMapiMain.cpp
>+PRLock *nsMAPIConfiguration::m_Lock = PR_NewLock();

This seems pretty odd. If you can destroy it in the destructor, this should be
in a constructor. But it's declared static... I'm confused here.

I'm going to skip the rest of these mapihook files, hopefully will have the energy
to look them over when the next iteration is reviewed. Watch for a lot of the same
issues I've already mentioned many times above.

>Index: mailnews/nsmapi/public/makefile.win

directories named "public" in the tree are basically reserved for exported headers
and the like, so people poking around in the tree can figure out what parts of a module
are safe to use and which are private to the module. This directory in contrast seems
to build a .dll.   Should it be renamed "build"? (see mozilla/xpcom/build and similar).

ditto earlier makefile.win comments...

>Index: xpfe/bootstrap/makefile.win
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/bootstrap/makefile.win,v
>retrieving revision 1.45
>diff -u -r1.45 makefile.win
>--- makefile.win	2001/08/21 00:55:14	1.45
>+++ makefile.win	2001/08/29 20:20:51
>@@ -119,6 +119,7 @@
>  $(DIST)\lib\png.lib                 \
>  $(DIST)\lib\mng.lib                 \
>  $(DIST)\lib\mpfilelocprovider_s.lib \
>+ $(DIST)\lib\NsMapi.lib				\

***** WARNING! WARNING! ****

You have just made the browser depend on mailnews both at build time and runtime,
neither of which will be accepted by the super-reviewers.  The tree can be built with mailnews
disabled (thus no exported headers, no libraries to link to), and even if build time dependencies
were allowed the browser can be installed without the mail component.

What are you trying to accomplish? I'm sure there's another way.

If you are adding things that have to be loaded at startup (think twice, each added item
slows our startup and we're desperately trying to speed it up) try using the AppStartupNotifier
mechanism. (See nsIAppStartupNotifier.h) You register in a category and the service loads
you at startup. Completely dynamic.

>Index: xpfe/bootstrap/nsAppRunner.cpp
>-  appShell->SetQuitOnLastWindowClosing(PR_TRUE);	
>+  appShell->SetQuitOnLastWindowClosing(PR_TRUE);

What's the change here?

>+  // Register MAPI Support
>+  NS_InitializeMapiSupport();
>+

You want a Startup notifier -- this won't fly. But even a startup notifier raises the startup
time issue so you'd better start practising your explanations for why this is absolutely
necessary and there's no other way to do it. How expensive is InitMSCom ?

>Index: xpfe/bootstrap/nsNativeAppSupportWin.cpp

Maybe some of this does belong here, in which case this is where the interfaces should
live, exported so mailnews can use them. Talk to Bill Law about how he wants this
structured, he was the owner for NativeAppSupportWin last I heard.

Comment 11

[(not reading, please use [e-mail] instead)]

rajiv tells me that not all of these libs are needed.

kkhandrika, can you confirm, and if so, remove any unneeded libs?

+
kernel32.lib
\
+
user32.lib
\
+
gdi32.lib
\
+
winspool.lib
\
+
comdlg32.lib
\
+
advapi32.lib
\
+
shell32.lib
\
+
ole32.lib
\
+
oleaut32.lib
\
+
uuid.lib
\
+
odbc32.lib
\
+
odbccp32.lib
\

also, instead of mailnews/nsmapi, add all your new code mailnews/mapi.

I'll go move all the existing mapi code in mozilla/mailnews/mapi to
mozilla/mailnews/mapi/old, for future reference.

Comment 12

[Krishna Mohan]

Seth : Only ole32.lib, rpcrt4.lib are needed (atleast now)

Renaming mailnews/nsmapi to mailnews/mapi is fine.  There are three dlls coming 
out of that directory.

1. Mapi32.dll      - used by client applications.
2. nsMapiProxy.dll - Proxy/Stub between Mozilla and MAPI client Apps.
3. nsMapi.dll      - MAPI implementation in mozilla.

So, what should I name the last two dlls'?  (mzMapiProxy.dll and mzMapi.dll).
Dan -- Do you have any suggestions for the dll names

This has to be decided now as I am doing a LoadLibrary in the patch of 98566.

Comment 13

[Håkan Waara]

Our other libraries are named 'msgimap', 'msgsearch', 'msgnews' etc.

I suggest you follow the same convention.

'msgmapi', 'msgmapiproxy' perhaps?

Comment 14

[Krishna Mohan]

Based on the review comments I have renamed nsMapi.dll to mzMapi.dll and the 
same is reflected in the patch dated 09/07/01 17:58 of 98566 ("Revised code as 
per review comments").  I just wanted to bring this to everybodys' attention.  
B'cos the name of the dll can not be changed once the code is checked in (at 
least for this release).  So, if anybody have comments on this please come back 
immediately.

Comment 15

[Håkan Waara]

krishna, can you please name them with the common, already-used, 'msg' prefix as
I requested in my previous comment?

Thanks

Comment 16

[Krishna Mohan]

I can rename it to 'msg*'.  But the dll is going to be in /bin directory not in 
/bin/components.  As you told there are a few msg*.dlls in /bin/components but 
only one msgbsutl.dll in /bin directory.  Also, there are a few moz*.dlls in 
/bin.  Based on this please let me know your final comment.

Comment 17

[Håkan Waara]

Having it be msg* makes more sense (and manages to describe where it's being
used too) than ns/moz imho.

Comment 18

[Krishna Mohan]

Follow-up of review comments from [e-mail] on 2001-09-04 20:24

I have made changes to makefiles and also to xpfe\bootstrap code.  I got r and 
sr (branch) for xpfe\bootstrap code (98566).  I will be posting the new patch by 
the end of the day (probably).  Meanwhile I just want to open another thread to 
answer SOME of the comments, so that I can reach the dead line.

Please corelate the answers with the comments on above mentioned dates.  Thanks 
!!

Use of PR_AtomicIncrement()/PR_AtomicDecrement():
-------------------------------------------------
PR_Lock(m_Lock) is used in AddRef as well in other functions (Release).  In the 
review comments it is suggested to use the following construct

     PRInt32 tmp = PR_AtomicDecrment(m_cRef);
     if (tmp == 0)
         delete this;
     return tmp;

What happens in case some other thread calls PR_AtomicIncrement() in between the 
if statement and PR_AtomicDecrement(m_cRef).  The component is gone even if 
there is new reference just came in.  So, the comparison and destruction also 
has to be protected.  Since the lock is not yet released before making the call 
to destructor i am unlocking in the destructor.  If the destructor is called in 
some other way, then unlocking the mutex just before destroying the mutex that 
too in the DESTRUCTOR does't really matter.  So is the following code

nsMapiFactory::~nsMapiFactory()
{
    PR_Unlock(m_Lock);
    PR_DestroyLock(m_Lock);
}

Finally is the AddReft :   As I can not use PR_AtomicDecrement(); explained 
above; I can not use PR_AtomicIncrement b'cos the pair does't match.


------------------------------------------------------------------------
>+void nsMapiHook::CleanUp()
    .....
>+              native->SetIsServerMode( PR_FALSE );
>+              // This closes app if there are no top-level windows.

>>This shuts off -turbo mode, doesn't it? That seems like an evil thing to do
>>if you weren't the one who put it in that mode.

When the patch submitted only Logon is ready not the Logoff (at least the 
cleanup)
Now everything is taken care.  The new code will not release the turbo mode
if this is not the one put it in turbo mode.
---------------------------------------------------------------------------
Remaining things after lunch.....!!

Comment 19

[Krishna Mohan]

Answers to the review comments continued....

Use of HiddenWindow :
---------------------

>+        nsIDOMWindowInternal *hiddenWindow;
>+        rv = appShell->GetHiddenDOMWindow(&hiddenWindow);
>+        if (NS_FAILED(rv)) return PR_FALSE;

Initially I passed "nsnull" to PromptService->PromptUserNameAndPassword 
function.  But there was problem with the Window Watcher (I think), that is it 
not able to reparent the prompt dialog.  As a result the Prompt dialog comes up 
and disappears automatically after selecting the profile.  So, as a solution I 
am passing hiddenWndow as the parent.  Also, the prompt service dialog is 
'modal' always.  So, there is no problem wheather the parent is model or not.

Convertion from UNICODE to ASCII and back to UNICODE :
------------------------------------------------------
>+            nsXPIDLCString cEmail;
>+            cEmail.Adopt(ToNewCString(Substring(email, 0, index)));
>+            NS_ConvertASCIItoUCS2 p_emailName (cEmail.get());

We are currently looking for the alternatives.  Once we succeed we will change 
it.


Use of nsString classes :
-------------------------
>+    nsString nsProfileName ;
>+    nsString nsPassCode ;

As mentioned; I am not taking full advantage of string classes in this function 
; but using to get string references.  I think other MAPI functions ( not part 
of this patch) are taking advantage of Ns-String classes.  Then it will be 
consistent through out.

Comment 20

[Srilatha Moturi]

Attachment: patch v3

Comment 21

[Tiantian Kong]

[e-mail], dougt: I mailed you at noon today. No reply yet. Please
reply, when can you finish your r/sr? Because of the urgency, we had to request
a 24 hour turnaround. 

Elaine is helping me by calling each r and sr since noon today. If you cannot
give your r/sr comments within 24 hour, please come to the PDT to explain why at
12 noon on 9/18, pit of bldg 21. 

Comment 22

[Tiantian Kong]

Added PDT status, per PDT meeting today. Once we got all r and sr, will change
it into PDT+. 

All r ans sr, please try your best to give comments within 24 hours. If no
reply, then our assumption will be within 24 hours. If you cannot do it within
24 hours, please attend the PDT meeting at 12 noon on 9/18 at the pit of bldg 21
to explain why. Many thanks.
=======================
Subject: Urgent: your review and super review comments.
Date: Mon, 17 Sep 2001 13:18:36 -0700
From: Tiantian Kong <[e-mail]>
Reply-To: [e-mail]
To: [e-mail], [e-mail],	Doug Turner <[e-mail]>, [e-mail],	[e-mail],
Rajiv Dayal <[e-mail]>
CC: Elaine King <[e-mail]>

Hi:

You got this mail because you are the r or sr of features in simple MAPI.

Pathes for these has been up on the bug. 

On behalf on PDT, I'm requesting that you provide immediate feeback. If you
cannot be back to us today, please reply by 
email, and cc Elaine King, as to when you'll be able to get back to us.

This is an urgent request, please drop everything else that you are doing. 

Thx. Elaine will be calling each of you as well. 


Rajiv, r, 		Pref, 95122					

Seth, sr, 		Pref, 95122

Dan Veditz,	r	log on, log off, 95117/95121	

Doug Turner, sr, 	log on, log off, 95117/95121	
				
Bill Law,	r	xpfe/bootstrap for log on, off	
				
Alec, sr, 		xpfe/bootstrap for log on, off 


Tiantian
=========================

Comment 23

[Doug Turner (:dougt)]

I found many of the same problems in patch v3 as were discussed above on this
bug.  This is ALOT of code for someone to walk through at one sitting.  If you
like for a few of the superreviews/reviewer to get together and walk though this
code, that would probably be more effective.  Anyways, here are my notes wrt
patch v3.


Nits
======================================
There are a few files without newline at the end of the file.  Please fix that.


msgMapiFactory
======================================
You are not checking to see the result of PR_NewLock() in the nsMapiFactory
constructor.

In the destructor, I do not see any need to do a PR_Unlock().  Also, you should
check for null before calling PR_DestroyLock()

It is better to use a PR_Atomic(Increment|Decrement) instead of the lock you are
holding in AddRef and Release.  In fact, if you use these function, you can
remove the mLock totally.

(The three above comments may hold true for other class which is in this diff. 
Please fix all occurances.)

Shouldn't m_cRef be unsigned?

 
nsMapiHook
======================================

in Initialize(), there is no neex to have an else part of the if statement.  It
is a non-sequitur.  Just return and be done with it.  Same with the second check
for result when calling GetNativeAppSupport

NS_ASSERTION evaluate to NOTHING in optimized bits.  Use of them to prevent a
crash cause by dereferencing a null will not work.  Please protect against this
in CleanUp()

In CleanUp, it is probably better to just check for a valid
|nsINativeAppSupport|, then a result code.

In DisplayLoginDialog, and for that matter anywhere, be consistent when checking
results.  For example, the first call to do_GetService, you only check the
return result, but the second call you check both the result and the service
pointer.  (collary: I don't mind that you check both, but mix and match)

In DisplayLoginDialog, you should probably put that string URL constant in a
header file or closer to the top of the file.

Why are you using '\' in DisplayLoginDialog??  C allows  w h i t s p a c e.

I do not think that you should be using the nsIDOMWindowInternal.  Isn't there a
better way.  DanM, please save us.

Get rid of the PRUnichar* allocation in DisplayLoginDialog.  Use nsXPIDLStrings'
instead.  

In VerifyUserName, I can not believe that this is correct:

+            nsXPIDLCString cEmail;
+            cEmail.Adopt(ToNewCString(Substring(email, 0, index)));
+            NS_ConvertASCIItoUCS2 p_emailName (cEmail.get());


What type is NS_ConvertASCIItoUCS2?!  Please as Jag about this.

In msgMapiHook.h, you really don't need the |#include "nsXPIDLString.h"|.


nsMapiImp
======================================
Please rename these variables: nsString nsProfileName ; nsString nsPassCode ;,
as they do not follow are coding standards.

The check for (aFlags & MAPI_LOGON_UI) has a non-sequiter.  If you check for the
negation, you can avoid an else.

nsMapiImp::Login, I am totally confused as to why you are deleting memory
different than the way it was allocated:

+        delete [] Name;
+        delete [] Pass;

Furthermore, delete [] does not check for null usually.  (delete() does)

nit: A switch statement is a bit of an overkill.

In nsMapiImp::Logoff, you do not need that 'else'


msgMapiSupport

======================================
Could you sprinkle some check's for NULL?  If nsMapiSupport::GetNsMapiSupport
ever fails, crash.

Comment 24

[Alec Flett]

Talked with kirshna about this, I have asked him for the trunk not to use the
standard template library when this is landed on the trunk. 
He explained that this is windows-only, but there are many reasons we should not
be using templates

1) rule #1 of the C++ portability rules - don't use templates. It is a long term
goal to be able to support gcc on windows (using the Microsoft Platform SDK)
which means portability across compilers (and standard libraries) is still an issue.
2) embeddors (which include windows) need to have as few build and runtime
dependencies as possible - we already have nsHashtable to do the hashing work
which you're trying to do with std::map. And no, this might not seem like
something embeddors might want but why unnecessarily burden them with rewriting
your MAPI code when there is a perfectly good hashtable implementation available
to you?

Comment 25

[Jaime Rodriguez, Jr.]

Pls update the Patch Status with r/sr=.

Check it into, with another bug for code clean-up. 

Come back on Monday for PDT+.

Comment 26

[Srilatha Moturi]

Attachment: patch v4

Comment 27

[Alec Flett]

If you need to mark this FIXED for the pdt, this STILL does not land on the
trunk... file another bug for landing this on the trunk.

Comment 28

[Doug Turner (:dougt)]

I still do not see an r='s on this bug.  I agree with Alec, replace std::map
with our nsHashtable.

Comment 29

[Chris Waterson]

To expand on alecf's second point, do we know how use of STL might affect the
libraries that we need to link against (and therefore, possibly ship with)? In
other words, might we need to redistribute another Microsoft runtime DLL; e.g.,
to support Win95 platforms?

Comment 30

[Brendan Eich [:brendan]]

MS's licensing schemes are becoming increasingly hostile to open source
licenses, another reason to avoid depending too much on their toolchain and
libraries.

Is there a problem with nsHashtable that prevents it being used here?  There
could be, or perhaps STL just seemed better.  I'm interested in hearing about
any difficulties (memory issues) in using nsHashtable.

/be

Comment 31

[Tiantian Kong]

Dan is working on the r stamp. 

Dan: any update??? 

Alec suggested that we can check in the branch and trunk now, then back out the 
changes on trunk after a couple of days testing, check in again once we 
replaced the STL. 

Comment 32

[Jaime Rodriguez, Jr.]

DougT will help you witha a patch for the open issue. After you get and
implement Doug's patch, come to the PDT.

Comment 33

[Chad Austin]

fwiw, sometimes using STL in VC++ brings in a dependency on msvcp60.dll.  Most 
of the STL is inline, but bits and pieces of certain classes are implemented in 
that DLL.

I don't think any Windows ships with that DLL.

Comment 34

[Mike Shaver (:shaver emeritus)]

Why does it need to land on the trunk for testing?  We distribute non-trunk
builds for testing all the time, in the latest-* directories.  I don't
understand why we can't have the MAPI test crew test custom builds for a couple
of days, instead of bouncing this patch off of the trunk.

(Or, indeed, instead of fixing it according to reviewers' comments.)

Comment 35

[Krishna Mohan]

I wanted a mapping between an integer and a struct pointer.  I did not find a 
hash key of type integer.  Currently supported hash keys are

enum nsHashKeyType {
         UnknownKey,
         SupportsKey,
         VoidKey,
         IDKey,
         CStringKey,
         StringKey,
         OpaqueKey
};

To use nsHashtable I need a tailor-made nsHashkey, for which right now there is 
no time.  But I will file a bug and fix for the next release.

Coming to Chris Waterson's comment I will have to check whether we need to ship 
any other DLLs to support on 95.  At least I am not using any libs/dlls to 
compile/link.

Comment 36

[Daniel Veditz [:dveditz]]

before you check this into the trunk, please update the license headers to
comply with http://www.mozilla.org/MPL/license-policy.html (see also
http://mozilla.org/MPL/boilerplate-1.1/ ). What is the plan for landing on the
trunk? If it's soon then relicensing the files after landing on the branch
probably isn't much of a concern, otherwise I'd like it fixed correctly now
before anyone other than you touches the files.

Several files have tabs, please fix that. Quick search reveals
mapi/build/makefile.win, mapi/build/MapiProxy.def, mapi/mapi32/mapi32.def,
mapi/mapihook/makefile.win and mapi/mapihook/msgMapiHook.cpp -- I may have
missed some. Please change your editor settings so this isn't an ongoing problem.
Why does msgMapi.idl have so few methods? Do you have multiple interfaces in
order to implement the whole range of simple MAPI? If so why does Logon and
Logoff get what looks like a generic interface name? Or are you showing
different incomplete versions of this inteface in different bugs? This all comes
down to:
  Am I reviewing what you're planning to check in?

msgMapiDefs.h needs a license block

nit: "PassWord" seems odd, "Password" is usually thought of as one word

You need to add REQUIRES lines to the makefiles in order to not break when
MOZ_TRACK_MODULES_DEPS is defined. This will eventually be turned on by default
but there are a few people already working with this turned on testing it out.
This change could wait until you land on the trunk, but will be required then.

Please add a newline to the end of mapi/mapihook/msgMapiFactory.h

I'm still concerned about your use of the nativeAppSupport to turn the server
mode on and off (especially since you've obviously made changes to those
routines which don't appear in this patch). Have you talked to anyone about what
effect this will have on -turbo? It looks to me that if the user starts up in
non-turbo mode, then turns it on in the prefs, you will turn it back off when
you execute your ::Cleanup routine.  CC'ing ccarlen for his input, no r= without
his approval of this use (or some other -turbo-knowledgable guy).

Speaking of missing from the patch, some of the files I commented on before are
now missing. Did you make those changes? Where did they go?

nsMapiHook::VerifyUserName still converts from "ascii" to UCS2, which I'm sure
is going to fail in non-Latin-1 account names, maybe even European names. Since
you obviously don't intend to fix this for this release please file a bug on it
and reference that bug in this one.

nsMapiImp::Login it looks like you're passing the address of the internal
structure of an nsString as a non-const arg, and that it might get written to.
Don't know if that's really what's going on, but it looks bad on the surface and
is a misuse of nsString. The fact that you're having to cast to (PRUnichar *) is
a bad, bad sign. There is automatic conversion to the safe (const PRUnichar *)

When you delete Name and Pass you're sure they will never be NULL?

nsMapiHook::DisplayLoginDialog() is defined and used as a boolean, but it
returns nsresult values for many failures which will be accepted as TRUE (since
non-zero). That's one way you can get to trying to delete a null Name and Pass.

nsMapiHook::VerifyUserName should be defined with const PRUnichar * aUsername
since you never change it. You can then avoid ugly casts in nsMapiImp::Login.
Use const in the RegisterSession() function as well.

The template stuff doesn't bother me so much for the branch, but it will have to
be eliminated before checking into the trunk. I also echo waterson's concern
about what MS libraries it drags in. You can check this using a number of
dependency tracking tools (please check at run-time, not just statically) and
then compare against one of the Win95 machines in the QA lab.So there's at least
two .dll's in this patch, how many in the other MAPI patches? What bug contains
the updates to the install scripts and package lists? What does the additional
.DLL's do to startup time and bloat?

I don't see the changes to mailnews/makefile.win anymore, but I'd recommend
making mapi an optional part of the build until all the quirks are worked out:
  !ifdef BUILD_MAPI
  DIRS = $(DIRS) mapi
  !endif

and then make sure tinderbox and the release team know to set that one on. Or if
you're really confident in your code (and willing to face the higher
super-review hurdle right now) you could reverse the test !ifndef DISABLE_MAPI

That way if it does happen to cause problems to other users of the branch it can
easily be turned off at build time.

Comment 37

[Doug Turner (:dougt)]

You do not need to have a special key, if you dont mind casting.  I can not
compile it against the trunk from a day ago because of this error:


Microsoft (R) Program Maintenance Utility   Version 6.00.8168.0
Copyright (C) Microsoft Corp 1988-1998. All rights reserved.

+++ make: exporting headers
msgMapiHook.cpp
c:\builds\trunk\mozilla\mailnews\crap\mapihook\msgMapiHook.cpp(89) : error
C2039: 'EnsureProfile' : is not a member of 'nsDerivedSafe<class
nsINativeAppSupport>'
NMAKE : fatal error U1077: 'cl' : return code '0x2'
Stop.
NMAKE : fatal error U1077: 'c:\PROGRA~1\MICROS~3\VC98\BIN\nmake.exe' : return
code '0x2'
Stop.


I will attach a diff which will remove the stl cruft and use a **much** fast
hash table implmention.  You are going to have to test it well is all I could do
was ensure it compiled (see above for the reason I couldn't test)

Also, I am not sure exactly how you wanted to handled unregistering the session.
 I am basically deleting the entry from the session hash, but I do not think
that is enough.

Need to get some dinner.  if you have questions, call me on my cell.  I am
listed in pb.  

Comment 38

[Alec Flett]

Use nsOpaqueKey, its that simple.

PRInt32 val=444;
nsOpaqueKey key(&val, sizeof(val), nsOpaqueKey::OWN_CLONE);
hashtable->Put(key, ...);

I believe that ownership model will do the right thing. A little research will
tell if you if I'm right or not.

Since you've obviously done some work to investigate this, I suggest you just
take the 45 minutes or so that it will take to switch to nsHashtable.

Comment 39

[Doug Turner (:dougt)]

Attachment: diff between my msgMapiMain.(cpp|h) and the one in patch v.4

Comment 40

[Doug Turner (:dougt)]

crap. it is backward.  switch + to -, and - to +.  

Comment 41

[Alec Flett]

regarding your patch, doug - you don't have to new() the hashtable, you can just
make m_sessionMap and m_profileMap as actual nsHashtable - I think that also
makes the memory less fragmented because they are all allocated in the same
space with the owning class.

Comment 42

[David :Bienvenu]

Or you could use PLDHash, which works fine with 32 bit keys. 

Comment 43

[Doug Turner (:dougt)]

There are a million ways to do this.  I was just trying to follow as closely as
possible the old stl usage.  I do agree with alec, though, that droping the new
nsHashtable is better...

I sent kkhandrika the two files I was working on.  Let wait for another patch.

Comment 44

[Brendan Eich [:brendan]]

Use nsVoidKey and NS_INT32_TO_PTR (and NS_PTR_TO_INT32 to go back), to avoid
malloc'ing a four-byte buffer for nsOpaqueKey.

But for absolute least malloc overhead, use pldhash.h (it's in xpcom/ds).

/be

Comment 45

[Conrad Carlen (not reading bugmail)]

+              native->SetIsServerMode( PR_FALSE );
+              // This closes app if there are no top-level windows.
+              appShell->UnregisterTopLevelWindow( 0 );

Passing 0 to UnregisterTopLevelWindow is no longer OK. It was at some point but
now, it's not. The code which used this trick in nsNativeAppSupportWin.cpp has
been changed to not do that.

As far as turning on and off server mode, it shouldn't do too much harm. It will
cause a complete exit since it gets turned off in nsMapiHook::CleanUp. The thing
which really determines turbo mode is the Run key in the registry and using
SetServerMode won't affect that. So, the next time the user launches the app, if
they had the turbo mode pref set, it will be back in effect.

+        nsIDOMWindowInternal *hiddenWindow;
+        rv = appShell->GetHiddenDOMWindow(&hiddenWindow);
+        if (NS_FAILED(rv) || !hiddenWindow) return PR_FALSE;

You don't need to get the hidden window in order to use nsIPromptService. If you
don't have an obvious parent for a dialog, pass null for the parent param to
nsIPromptService routines and it will do the right thing. The fewer refs to the
hidden window the better.

As far as the nsHashTable vs. STL map, the hash table is better because the way
the map was being used. Iteration is not the strong suit of a map. find() and
operator [] are good, but for staight iteration, it's not very good.

Comment 46

[Krishna Mohan]

doug - I am almost changing the whole thing.  B'cos  there are 
statmenets like "nsVoidKey sessionKey(aSessionID)".  Here sessionKey is not a 
pointer which we can readily pass to 'Put' or 'Get'.  So, I have to allocate by 
'new'; then check for new failure; if failed remove all previously allocated 
stuff; un-lock; return...  Going on...

Comment 47

[Brendan Eich [:brendan]]

krishna, you don't ever new a nsHashKey subclass in order to Get or Put -- you
declare it with implicit storage class auto, and pass its address (use unary-&).
 If an entry needs to be made by Put, the key will be Cloned.

/be

Comment 48

[Krishna Mohan]

If nsHashkey is cloned that would be wonderful.  Let me test it.

Comment 49

[Doug Turner (:dougt)]

Attachment: the real deal - diff between my files and v4

Comment 50

[Doug Turner (:dougt)]

Ingore patch 50805.  I was on crack yet again today. 

Comment 51

[Krishna Mohan]

Doug - Great Work.  The new files got compiled just like that.  I will look into 
the remaining things.  A BIG THANKS.....

Comment 52

[Doug Turner (:dougt)]

just for clarification.  The first patch that I attached, I diff'ed between my
working version which was not complete and patch v4.  The new patch is a diff
between what I completed and patch v4.  I sent kkhandrika the raw files. 

Brendan, checkout the HashKey impl that I added.  It probably should get moved
to xpcom.  If you dig it, sr the bug cited.

Comment 53

[timeless]

+    if (hRes == S_OK)
+        bUnInitialize = TRUE;
+    else
+        bUnInitialize = FALSE;

please consider:
bUnInitialize = (hRes == S_OK) ? TRUE : FALSE;

+            if (!MultiByteToWideChar(CP_ACP, 0, pUserName, -1, ProfileName, \
+                                                            MAX_NAME_LEN))
does the \ add anything? and is there any reason to make the next line indent 
so far over? normally we line things up to ...Char(<-here

+        hr = pNsMapi->Login (aUIParam, aProfileName, aPassword, \
+                                                aFlags, &nSessionId);
in general you seem to use (and i like) |Fun(param, ..., param)| however you 
also use Fun( param, ..., param ) and others.

in most other places you use Function( [no ws] please be consistent.

+    (*aSession) = (LHANDLE) nSessionId;
what does () for *aSession do?

+        delete(Name);
+        delete(Pass);
+    return (S_OK);
return doesn't need ()s and delete?

+PRInt16 nsMAPIConfiguration::RegisterSession
+                return -1;
would you add a comment explaining what -1 means?

+        for (sessionIterator = (*m_SessionMap).begin(); \
+                sessionIterator != (*m_SessionMap).end(); \
+                        sessionIterator++)
why not:
+        for (sessionIterator = (*m_SessionMap).begin();
+             sessionIterator != (*m_SessionMap).end();
+             sessionIterator++)

Comment 54

[David :Bienvenu]

if you're going to fix this:

+    if (hRes == S_OK)
+        bUnInitialize = TRUE;
+    else
+        bUnInitialize = FALSE;


it should just be bUnInitialize = (bRes == S_OK);

Comment 55

[Krishna Mohan]

>>Do you have multiple interfaces in order to implement the whole range of 
simple MAPI? If so why does Logon and Logoff get what looks like a generic 
interface name? Or are you showing different incomplete versions of this 
inteface in different bugs? This all comes down to:  Am I reviewing what
you're planning to check in?

I have discussed with dveditz and clarifed that the files reviewed are checked 
in as it is.

>>I'm still concerned about your use of the nativeAppSupport to turn the server
mode on and off (especially since you've obviously made changes to those
routines which don't appear in this patch). Have you talked to anyone about
what effect this will have on -turbo? It looks to me that if the user starts up 
in non-turbo mode, then turns it on in the prefs, you will turn it back off
when you execute your ::Cleanup routine.  CC'ing ccarlen for his input, no r= 
without his approval of this use (or some other -turbo-knowledgable guy)

I have discussed with 'Conrad Carlen' and explained the behaviour of turbo mode 
in various cases. Finally we thought that may be Bill Law can look into it just 
to make sure that everything works correctly as he is the reviewer for #101364 
and #98566.  I spoke to Bill Law and he is looking into the code.  There is a 
conflict between #101364  and #98566.  I will post a comment on #101364, which 
Conrad Carlen said he will look into.

>>Speaking of missing from the patch, some of the files I commented on before 
are now missing. Did you make those changes? Where did they go?

Some files (Registyr.cpp, Registry.h) are taken off from Logon/Logoff and been 
put into preferences code (#95122).  So, logon/logoff is not dealing registry 
stuff anymore 

>>When you delete Name and Pass you're sure they will never be NULL?

delete () checks for NULL before deleting the memory (comment above - Doug T 
2001-09-17 22:19)

>>nsMapiHook::VerifyUserName still converts from "ascii" to UCS2, which I'm sure
is going to fail in non-Latin-1 account names, maybe even European names. Since
you obviously don't intend to fix this for this release please file a bug on it
and reference that bug in this one.

nsIMsgIdentity->GetEmail gets a reference to 'ASCII' string; b'cos email is 
declared as string attribute. So, the conversion from ASCII to UCS is required.

Also trying to get danm for clarification on hiddenWindow usage with 
nsIPromtService.

Comment 56

[Srilatha Moturi]

Attachment: patch v5, not using STL

Comment 57

[Alec Flett]

Comment on attachment 50959 [details] [diff] [review]
patch v5, not using STL

ok, I'm having a bunch of issues about the overall implementation here
- RegisterComponents, not "RegsiterComponents"
- why is nsIMapi a MSCOM interface? I don't see the implementation 
(nsMapiImpl) implementing any other interfaces other than private 
ones.. so how does the MAPI code know to call across to 
this interface?
it kind of looks like we're somehow calling across to 
the mozilla process, but I can't figure out exactly where this is. 
You need to put TONS of comments in here.
- it looks like we're creating two different dlls, one
as a stub and one as an XPCOM component. why don't we just put
 them both in the same DLL?
- You're defining CLSID_nsMapiImp about 5 times, instead of putting the definition 
in a header file. Traditionally, you put a #define with the {..} value, 
and then define the local constant using the macro.
- Why are you #defining all of these MAPI_E_FAILURE/etc
 macros?  aren't these defined by a microsoft header file somewhere?
- Why are you using PR_Atomic* operations? are you expecting
to be called from multiple threads?

Many of these questions may have perfectly valid answers, 
but without any comments its hard for me to just guess at
what you were intending.

Comment 58

[Dan M]

I have a smaller observation about hiddenWindow usage. (Thanks for asking!) The 
general rule is: never use hiddenWindow; it's not your friend. In the above patch 
(2001-09-26 14:31) it isn't necessary and can be trivially removed. The 
nsIPromptService interface requires that all implementations be able to handle a 
null parent (at time of writing, see http://lxr.mozilla.org/seamonkey/source/
embedding/components/windowwatcher/public/nsIPromptService.idl#44). A null parent 
is actually preferable. If you use hiddenWindow for the parent of a dialog, it's 
guaranteed to behave badly, being modal to an invisible window and all.

That said, I have to encourage you to try to find a proper parent window for your 
Prompt. If this is one of those cases where there truly is no parent window 
available, then use a null parent with my blessing. But giving the prompt service 
an explicit parent is always much better, if at all possible.

One thing more. If you expect this code to be able to run in an embedded context, 
you shouldn't use PromptService directly, since that's a Mozilla-the-app-only 
service. The best, most general interface for getting a prompt interface is to 
ask the nsIWindowWatcher service for a Prompter (and again you can give it a null 
parent). See for example http://lxr.mozilla.org/seamonkey/source/extensions/
wallet/src/wallet.cpp#4016. If you're running in a Mozilla-the-app context, then 
the nsIPrompt implementation you'll be given will have nsIPromptService behind 
it.

Life would be better if this were all documented, of course.

Comment 59

[Daniel Veditz [:dveditz]]

Comment on attachment 50959 [details] [diff] [review]
patch v5, not using STL

>> nsMapiHook::VerifyUserName still converts from "ascii" to UCS2, which I'm
>> sure is going to fail in non-Latin-1 account names, maybe even European
>> names. Since you obviously don't intend to fix this for this release
>> please file a bug on it and reference that bug in this one.
>
> nsIMsgIdentity->GetEmail gets a reference to 'ASCII' string; b'cos email
> is declared as string attribute. So, the conversion from ASCII to UCS
> is required.

I agree *some* conversion is necessary, but unless you see some pretty
explicit comments somewhere don't ever mistake "string" for ASCII. In
our code wstring is pretty reliably UCS2, but string is sometimes ASCII,
sometimes UTF8, sometimes the native OS charset (could be multibyte),
and sometimes a different user or webpage-specified charset (could
also be multibyte).

I checked rfc2822 and it said ASCII. That's a much better reason than
basing it on a "string" attribute in our interfaces.

I don't see any diffs for mailnews/makefile.win -- how does this
stuff get built?

I still don't see any install package changes. You can have the nicest
MAPI implementation in the world and it won't mean anything if no one
ever gets a copy. http://www.mozilla.org/build/making-additions.html

Update the bug with your branch tag once you've checked all your
files in because its hard to see how its going to work from patches
scattered in several bugs. Then we can try to build it and diff
it in our own tree and I'm sure it'll start making a lot more sense.
It's helpful to create a static tag for your branch point as well as
the branch itself (we usually use blahblah_BASE/blahblah_BRANCH pairs).

>Index: mailnews/mapi/makefile.win
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/mapi/makefile.win,v
>retrieving revision 1.2.166.1
>diff -u -r1.2.166.1 makefile.win

More context would help in review diffs. Try "cvs diff -uN -9"
at a minimum.

Let's get this landed on a branch and start working out the kinks there
(http://komodo.mozilla.org/planning/branches.cgi).

Comment 60

[John Bandhauer]

dveditz:
Note that while we may *misuse* 'string' in some places in our code, it 
unambiguously means ASCII.

See the table in the xpidl typelib spec at:
http://mozilla.org/scriptable/typelib_file.html#SimpleTypeDescriptor
'string' maps to type 16 with the pointer bit set to true.

I think it is wrong to claim that the burden of documention is on the interfaces 
that use this type correctly. Rather, anyone who is passing UTF8 through a param 
declared as 'string' had better be making it *very* clear that they are breaking 
the rules and they better have a good story explaining why.

Comment 61

[Alec Flett]

a followup on my comments from yesterday:
- krishna explained a bit of how this thing works, and agreed to document WAY
more before it gets checked in
- in discussion with krishna, I discovered that much of msgMapiDefs.h had been
blatantly copied from microsoft's on MAPI*.H header files, and was then
re-license with our license boilerplate. We agreed that it would not be checked
in. (Unfortunately, it was checked in last night, but fortunately mozilla.org
has yanked the files for legal reasons..that should be obvious)
- krishna, in the .idl file, you need to explain that the file is MICROSOFT
MIDL, and you are using it for inter-process communication. After some thought,
we need to file a bug about using a more standard means of IPC that can be
shared across the project.

Comment 62

[timeless]

there is an IPC thing for the entire project, dougt should be able to remind us 
what/where it is -- bug 68702.

while ms's compilers don't mind delete 0 iirc ibm's do. And I have a few other 
compilers which i would like to eventually use (watcom11c prebeta1 was just 
announced, plus i'd like to try digital mars and borland, not to mention 
someone might want to use gcc).

Comment 63

[Daniel Veditz [:dveditz]]

Which one is the "land MAPI_SUPP_BRANCH" bug? I can't find it.

Comment 64

[Mike Shaver (:shaver emeritus)]

I don't think we've ever run into a compiler that had problems with |delete f|
where |f| is null (mkaply says that the IBM compiler is fine, for example). 
Perhaps more importantly, I don't think we've ever run into a C++ _runtime_ that
had a problem with it.

That might be why there's nothing on the C++ portability guidelines about it. 
I'd be stunned if we were to discover such a platform in the future.  Bracketing
your |delete| expressions with pointer checks is just noise, IMO.

Comment 65

[David :Bienvenu]

Mozilla will not run if built by a compiler that doesn't handle delete of null.
It's that simple, so I agree with Shaver that it's just noise.

Comment 66

[Jaime Rodriguez, Jr.]

This was checked into the 094 branch, right? If so, pls replace [ETA ?] with 
[FIX on 094 Branch]

Comment 67

[Tiantian Kong]

Fixed in 0.9.4 branch.

Comment 68

[Trix Supremo]

verified on 2001-10-12-05-0.9.4