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

mass qa assigning all simple MAPI bugs to olgac

[copied from bug # 91702] ------- Additional Comments From bienvenu@netscape.com 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.

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.

QA over to trix.

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.

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 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 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.

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.

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.

Our other libraries are named 'msgimap', 'msgsearch', 'msgnews' etc. I suggest you follow the same convention. 'msgmapi', 'msgmapiproxy' perhaps?

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.

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

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.

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

Follow-up of review comments from dveditz@netscape.com 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.....!!

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.

dveditz@netscape.com, 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.

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 <tiantiankong8@netscape.com> Reply-To: tiantiankong8@netscape.com To: sspitzer@netscape.com, dveditz@netscape.com, Doug Turner <dougt@netscape.com>, law@netscape.com, alecf@netscape.com, Rajiv Dayal <rdayal@netscape.com> CC: Elaine King <elaineking@netscape.com> 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 =========================

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.

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?

Pls update the Patch Status with r/sr=. Check it into, with another bug for code clean-up. Come back on Monday for PDT+.

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.

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

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?

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

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.

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

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.

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.)

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.

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.

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.

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.

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

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.

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

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.

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

+ 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.

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...

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

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

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

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

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.

+ 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++)

if you're going to fix this: + if (hRes == S_OK) + bUnInitialize = TRUE; + else + bUnInitialize = FALSE; it should just be bUnInitialize = (bRes == S_OK);

>>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 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.

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 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).

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.

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.

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).

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

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.

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.

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

Fixed in 0.9.4 branch.

verified on 2001-10-12-05-0.9.4