The Simple MAPI is implemented as a COM component is available as a dll. The xpfe/bootstrap code loads this feature using traditional LoadLibrary calls. Bundling this inside an XPCOM component facilitates better management of the feature.

Created a new xpcom dll named msgMapi.dll. This is a merger of nsMapiRegistry component and msgMapi.dll of 0.9.4 branch. To do this I have reorganized the mailnews/mapi directory and a new private branch (MAPI_TRUNK_LANDING) is created. The fix needs change in xpfe code. Created a patch for the xpfe code. For the mapi part trying to create a patch, but could not succeed yet, as the directory structure is changed. I will talk to Alec on this and try to create a patch for mapi. The new directory structure is like this : merged entries -------------- mapi\registry, mapi\build into mapi\mapihook mapi\registry\Registry.cpp & header into nsMapiRegistryUtils.cpp & header new entries ----------- mapihook\build mapihook\public nsIMapiSupport.idl mapihook\src nsMapiSupport.cpp & nsMapiSupport.h removed entries --------------- mapihook\msgMapiSupport.cpp and header file. To build MAPI with the trunk follow the instructions below : ------------------------------------------------------------ 1. Fetch the trunk 2. Rename mozilla/mailnews/mapi to any other name (for ex. mapi-old-old) 3. Fetch the new mapi code from MAPI_TRUNK_LANDING branch 4. Apply the patches put by mscott and srilatha http://bugzilla.mozilla.org/show_bug.cgi?id=102918 Index: base/prefs/resources/content/accountUtils.js http://bugzilla.mozilla.org/show_bug.cgi?id=103260 Index: mozilla/mailnews/base/prefs/resources/content/pref-mailnews.js Index: mozilla/mailnews/mapi/resources/content/pref-mailnewsOverlay.js http://bugzilla.mozilla.org/show_bug.cgi?id=102821 Index: xpfe/bootstrap/nsAppRunner.cpp

Alec and Doug ... can you please r and sr the this bug. thanks.

We need this bug to be fixed in order to check into the trunk. Added target milestone: m0.9.5.

0.9.5 has already passed, I think you mean 0.9.6?

ok, the patch in bug 102821 does not go into the trunk - you should use the standard category-based nsICmdLineHandler method to register the MAPI handler so we don't hijack LaunchApplication() As for the patch that is here, please do a diff -u - I'm going to try to read a patch without context. However, I'm going to tell you right now that there's no WAY we're adding any dependencies on MAPI (i.e. the change to REQUIRES that you added to xpfe/bootstrap/makefile.win) so you need to come up with a better solution.

Alec: Thx. Yes, I mean 0.9.6.

Comment on attachment 56177 [details] [diff] [review] mapi startup/shutdown dependency is removed from xpfe that's great! sr=alecf make sure to test this on all platforms, to make sure they don't implement StartAddonFeatures or something

Landing 094 onto trunk is deferred for the time being as the 096 freeze. Created new branch MAPI_NEW_DIR_TRUNK (for mapi only) and merged 094 mapi with this. This branch will be merged with the trunk after getting r and srs' for the fixes. No checkins will be made to MAPI_NEW_DIR_TRUNK without an r and sr for any of the problems b'cos this is going to be merged with trunk. Posting the patch against MAPI_NEW_DIR_TRUNK for this bug fix. Doug and Alec : Can you please review and super-review the same. Thx!!

Mark as blocker for simple MAPI trunk landing.

reassigning to rdayal.

Hi Alec and Doug, Can u please sr the attached patch (id=57112). This patch shows all the changes that are done to existing files in 094 branch for trunk landing. The new dir struct for trunk landing is in the new branch MAPI_NEW_DIR_TRUNK. This branch has both the old and the new dir struct which might make it a bit confusing, but below is the new dir struct (the files and sub-dirs in these dirs) that will be checked into the trunk after your reviews : mapi\resources : all files and sub-dirs. mapi\mapiDll : all files. mapi\mapihook : NO files from this dir, ONLY following sub-dirs : mapi\mapihook\src : all files. mapi\mapihook\public : all files mapi\mapihook\build : all files. thanks, - Rajiv.

Comment on attachment 57112 [details] [diff] [review] self reliant MAPI for startup/shutdown ensure that this does not negatively effect startup times.

Comment on attachment 56177 [details] [diff] [review] mapi startup/shutdown dependency is removed from xpfe r=mscott

checked into the trunk the first patch (id=56177) which cleans up xpfe/bootstrap and removes the MAPI related code from mozilla/xpfe. The trunk landing of MAPI is anyway planned to be independent of any changes to xpfe. This patch (id=56177) also resolves the bug # 111199.

Comment on attachment 57112 [details] [diff] [review] self reliant MAPI for startup/shutdown - dont' use <> in #include unless its a system library. i.e. #include "nsISupports.h" not #include <nsISupports.h> - Please match your case to the filenames. There is no such "nsComPtr.h" but there is a nsCOMPtr.h - it sure looks to me like you're calling AddObserver() inside of Observe(), that can't be right.. - you're missing an unregister procedure to match nsMapiRegistrationProc - you're #including "assert.h" - why? it is verboten to call assert()

ok, comments on the files that are now on the branch: - mapihook/public/nsIMapiRegistry.idl - why are there 3 routines (setDefaultMailClient, unsetDefaultMailClient, isDefaultMailClient) when you could just make it one single "attribute boolean isDefaultMailClient;" - in nsMapiRegistry.cpp, ShowMailIntegrationDialog and ShowMapiErrorDialog share a tremendous amount of uncommented code to retrieve strings - combine this code into a helper function, and document what you're showing... - mapihook/public - the makefiles dump the midl output into the current directory.. that needs to go into some sort of output directory - I had originally thought WIN32_<x>.OBJ, but now I'm thinking somethin else, how about _midlgen (to match _xpidlgen) - in nsMapiRegistryUtils, you have a static nsAutoString - bad! no static objects allowed. just keep the buffer around and return a char*, no need to use ns*String classes here - same with brandName - and brandName is a single-byte string - don't ever use WithConversion, it is deprecated, and probably means your code is losing data somehwere. - for that matter, you shouldn't ever return an object directly from a class - either return a reference to it (but then you have to maintain one with a lifetime longer than just the call - i.e. no stack-allocated objects) - use nsDependentString (jag will change it to the new-and-improved class in time) instead of nsAutoString if you don't need to modify the string - it will reduce string copying - in IsDefaultMailClient() and saveDefaultMailClient, keyName should be nsCAutoString, not nsCString - dont' use AssignWithConversion("Mapi32.dll") - use NS_LITERAL_STRING instead. in fact, use NS_NAMED_LITERAL_STRING(fileName, "Mapi32.dll"); - dont' name variables "buffer" unless they are truly temporary or used for some generic purpose. in your case "buffer" almost always refers to the system directory - name it that way. - there's a comment about hardcoding "Netscape 6.2 Mail" and fixing before the trunk - let's get that string from a string bundle. in msgMapiFactory: - can we call this class (nsMapiFactory) something that doesn't start with "ns" so reviewers (like myself) don't think this is a XPCOM class? how about CMapiFactory? - same thing in msgMapiImp for nsMapiImp - also, give this class a better name - what are we implementing? what does this class do? - move the nserror -> mapierror switch statement into a helper routine - cleans up the code, and you never know when we'll need it - ack, sure enough you're adding an observer in ::Observe - this doesn't make any sense, figure out where that belongs - in msgMapiMain, its "Password" not "PassWord" - why are m_ProfileMap and m_SessionMap pointers to hashtables that are allocated with new? you should just make them straight member variables and save the heap fragmentation and 4 bytes of pointer - msgMapiSupport seems like a very strange place for the NS_IMPL_NSGETMODULE - perhaps you can put it in some file in build/? that's it for mapihook, I'll get to the rest later.

i was unfamiliar with quite some code u mentioned in comments above, when i looked into it i found that there are more things we can do to clean it up. I have changed the nsRegistryUtils.cpp and nsRegistry.cpp code to organize it in such a way to avoid code duplication, issues with 'static' nsAutoStrings, string allocations and all string handling issues. Also, I have put the all the registry utilities functions in a simple utils class so that it is more object oriented and easier to understand as well as easier for reuse. Please find a patch dealing with the comments and cleanup attached below. As per comments # 19 : - done. - done. - Observe gets called by XPCOM during initialization and it is during this first call, AddObserver is called, that is why it worked till now. But will move to nsMapiSupport::InitializeMAPISupport to avoid confusion. - is this really required since the category entry for Mapi support should not be deleted till mozilla is alive. Also as per its comments it is an optional function. - done As per comments # 20 : Registry related : - the set and unset functions are implemented to execute some code and are called from javascript separately to do changes to Windows registry besides changing the attribute. Although the function names sound similiar to the attribute name they do more. - done. there is another function in Registry utils which is now reused to minimize code duplication. - not part of this patch, will do. - done in the brandName function, conversion done only when must .. for adding string value to Windows Registry. - done. yes, one should rarely return class objects. - most places are modifying the string. - done - done - done, buffer var name used only for temp data - fix will be part of the patch on bug # 109101 fixed before trunk landing MapiFactory and MapiImp related : - done. - done. - see above. - done. - done.

Hi Alec, can u please take a look at this patch and the remaining files of the MAPI_NEW_DIR_TRUNK branch for trunk landing. thanks - Rajiv.

Comment on attachment 62149 [details] [diff] [review] patch to take care of the comments great work! Just a few things: - watch your indenting. you have a lot of stuff like if (foo) { bar(); baz(); } you're still using AssignWithConversion for the brandName - this is bad because someone might release a brand name that contains non-ascii characters (how do you say mozilla in bosnian? :)) - since you're really using it to store an ASCII string in the registry, use NS_ConvertUCS2toUTF8(brandName) where appropriate - "registryUtils" is clearly a member variable - name it that way (m_registryUtils) - kill that last the extra cast to (char *)keyName.get() - as for the readonly attribute + 2 functions, I think we need to clean this up one way or the other, the names are too similar. I personally think that you could combine them into one attribute, based on the way they are used - the getter would just go retrieve the info from the registry, and the setter would add or remove the info from the registry depending on the value passed in. even with these comments, great cleanup! I'm glad to see someone really whipping this code into shape.

ok, so the code in mapiDll looks fine, sr=alecf on that the code in resources/content can be cleaned up, if we clean up those attributes/functions: if (mapiRegistry && ("mapiPref" in parent) && (mapiRegistry.isDefaultMailClient != parent.mapiPref.isDefaultMailClient)) { if (parent.mapiPref.isDefaultMailClient) mapiRegistry.setDefaultMailClient(); else mapiRegistry.unsetDefaultMailClient(); } becomes: if (mapiRegistry && ("mapiPref" in parent) && (mapiRegistry.isDefaultMailClient != parent.mapiPref.isDefaultMailClient )) mapiRegistry.isDefaultMailClient = parent.mapiPref.isDefaultMailClient; Also, regarding the code in resources/ it all has a very generic set of function names, like mailnewsOverlayInit() - seems like we need to specify something more specific, like mapiPrefsOverlayInit(), etc. frankly, I don't like the way the pref panel works at all. The way this should really work is that the checkbox should correspond to a preference, and the mapiRegistry should be an observer on that pref changing. when the pref changes, the mapiRegistry should tweak the registry as appropriate to match the preference. That would actually eliminate most JS from this panel - not that JS is a bad thing, it just seems like this code here is redundant. Basically, you should never rely on the pref panel UI to change a "pref" for you (obviously, in this case there is no corresponding pref, but it sure seems like there should be one!) this also allows you to verify that nobody has mucked with the registry since the last time you run - when the mapiRegistry object is created, it can check the current value of the pref, and tweak the windows registry as appropriate. This keeps us from being hijacked by outlook. ok, so that summarizes all my reviews of all the MAPI code to date. I guess we just need patches to reflect the resources/ and mapihook/ comments, and we'll be ready to land!

thanks so much Alec for your review. I will take a look at the resources stuff ..

I looked into the resources and have made changes for attribute isDefaultMailClient as u suggested, i see ur point here to clean it up furhter. Also I have taken care of the other comments #23. Patch coming soon... As far as the prefs things go .. this is what the design is - the checkbox to select MAPI as default mail client is sort of associated with a pref but since it requires changing the MAPI Windows Registry we use that itself as the pref rather than having another Mozilla pref. The reason we have this checkbox and not the Mozilla pref is because we want to provide an easy way to the end-user (not the admin) to change it and thus allow him/her to make us the default mail client easily. As far as hijacking our setting is concerned we have a separate Mozilla pref (i am very sure that we designed it .. i looked at the code and i think it is implemented too) for protecting our setting. This pref allows the admin to lock the settings for the end-user. The admin can set this pref and lock it which means the checkbox will be always checked and grey-ed thus not allowing the end-user to change it or .. even if Outlook hijacks the settings in Windows Registry, on startup this pref would be seen and if it is set to true and locked the settings in Registry will be changed back to our settings and the checkbox grey-ed thus overcoming the hijacking.

The above patch (id=62213) HAS the updates for the comments already taken care in previous patch (attachment id=62149) as well as for the comments #23 & #24. Hi Alec, can u please take a look at the last patch (id=62213) in here. thanks, - Rajiv.

ok, this is exactly why the pref should work the way I described: 1) its not at all clear to anyone except for the maintainer (you) why it behaves that way 2) the code would be far cleaner and the checkbox would lock automatically if you just used that pref the way I described Now that I see you have a pref, look at it this way: 1) you could change remove all that mailnewsOverlayInit() code and simply put the pref name in a prefstring="foo" (on a side note, you also shouldn't call it "system.windows.lock_ui.default_mail_client": It is a mailnews preference, so it should start with "mailnews" and while the current implementation may be windows-only, you don't know that someone wont some day add Mac support. I suggest "mailnews.default_mail_client" or something.) 2) you wouldn't even have to expose this via the nsIMapiRegistry interface - the interface would be through prefs. 3) what happens if someone changes the pref in some other way other than through the pref dialog? For instance, if someone switches profiles, or some future mechanism like ACAP/LDAP prefs changes the pref at runtime? With the current scheme, stuff like that simply isn't supported. Basically, while I understand how it works, it is the wrong approach, sorry :( In fact, MAPI support could be added on other platforms at some point, so th

i looked into the prefs related code for MAPI .. Alec, i see what u are saying .. u are talking about the pref we already have for disabling the checkbox to make Mozilla as default mail client. I initially thought u wanted me to define another pref equivalent for Windows registry setting. Anyway, i have done the change for the prefs as per the comments and below is the patch with all the changes.

ok, had one last conversation with rajiv over AIM - patch is very very close - we were able to remove all JS from the pref panel.. when the patch with that arrives, then it has sr=alecf, pending an additional review from a mail guy (sspitzer or mscott)

ok i have tested the code with the changes for removing the JS file .. please find the updated patch below. I have also removed redundant code in mailnews/base/prefs/resources/content/accountUtils.js for the MAPI preference to allow locking / unlocking the Prefs dialog check box for making Mozilla as default mail client, since the pref is now directly associated with the checkbox in mailnews/mapi/resources/content/pref-mailnewsOverlay.xul file using "prefstring=".

Hi Scott, Can u please take a look at the patch attached. thanks, - Rajiv.

Comment on attachment 62471 [details] [diff] [review] latest patch with all the cleanup r=mscott. Thanks for spending so much time with this patch during the review Alec. Rajiv, code looks great. A couple small things I may have done a little differently: + const PRUnichar *dTitlepropertyTag = dialogTitlePropertyTag.get(); + rv = bundle->FormatStringFromName(dTitlepropertyTag, I would probably just pass in dialogTitlePropertyTag.get() directly instead of declaring a const PRUnichar * variable. There were a couple spots wwhere I saw that pattern. I also had a question about the contents.rdf we are inserting into messenger.jar. It looks like we are inserint it under the messenger name space: messenger.jar: content/messenger/pref-mailnewsOverlay.xul + content/messenger/contents.rdf We don't want to do that because it means we need to copy all of the stuff in messenger's contents.rdf and put it in the mapi directory since the mapi code is over writing the contents.rdf messenger uses. I thought I had fixed the mapi code on the trunk to create a new chrome name space (messenger-mapi) and we put a contents.rdf file into that name space in messenger.jar. This .rdf file only has the new overlay mapi wants to add and it doesn't over-write messenger's contents.rdf Is that still the case or this code over writing messenger's contents.rdf? I couldn't tell from the diff and just wanted to make sure.

You are right about the contents.rdf ... in fact the trunk has the correct one .. the initial patch (id=57112) had the files from the 094 branch (i dont know why), which was checked into the MAPI_NEW_DIR_TRUNK and thus u are seeing this file in the patch since the patch is created wrt MAPI_NEW_DIR_TRUNK. In fact the jar.mn is also not the right one. Anyway, I have updated it with the ones in the trunk which takes care of it. Also I have modified the code to avoid defining a variable just to transfer a pointer value from one function to another and thus avoid an extra pointer variable copying ! Please find below the patch with these changes. You will see changes in contents.rdf, jar.mn and makefile.win in mapi\resources\contents dir but these changes are already in the trunk. The patch is created wrt to the MAPI_NEW_DIR_TRUNK.

Comment on attachment 62588 [details] [diff] [review] updated patch great. sr=mscott

Comment on attachment 62588 [details] [diff] [review] updated patch has another sr=alecf as per his comments # 32.

Thank u very much Alec and Scott for looking into the complete MAPI code (MAPI_NEW_DIR_TRUNK and the latest updated patch) for trunk landing.

updated patch (id=62588) checked into MAPI_NEW_DIR_TRUNK.

checked into the trunk.

verified on mozilla & netscape trunk 2002022703 builds