106137 - Bundle Simple MAPI COM component inside an XPCOM component

Status: VERIFIED FIXED

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

Description

[Krishna Mohan]

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.

Comment 1

[Krishna Mohan]

Attachment: xpfe part of the patch

Comment 2

[Krishna Mohan]

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

Comment 3

[Krishna Mohan]

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

Comment 4

[Tiantian Kong]

We need this bug to be fixed in order to check into the trunk. 

Added target milestone: m0.9.5. 

Comment 5

[Alec Flett]

0.9.5 has already passed, I think you mean 0.9.6?

Comment 6

[Alec Flett]

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.

Comment 7

[Tiantian Kong]

Alec: Thx. Yes, I mean 0.9.6.

Comment 8

[Krishna Mohan]

Attachment: mapi part of the patch

Comment 9

[Krishna Mohan]

Attachment: mapi startup/shutdown dependency is removed from xpfe

Comment 10

[Alec Flett]

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

Comment 11

[Krishna Mohan]

Attachment: self reliant mapi for startup & shutdown

Comment 12

[Krishna Mohan]

Attachment: self reliant MAPI for startup/shutdown

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

Comment 13

[Tiantian Kong]

Mark as blocker for simple MAPI trunk landing.

Comment 14

[scottputterman]

reassigning to rdayal.

Comment 15

[Rajiv Dayal]

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 16

[Doug Turner (:dougt)]

Comment on attachment 57112 [details] [diff] [review]
self reliant MAPI for startup/shutdown

ensure that this does not negatively effect startup times.

Comment 17

[Scott MacGregor]

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

r=mscott

Comment 18

[Rajiv Dayal]

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 19

[Alec Flett]

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

Comment 20

[Alec Flett]

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.

Comment 21

[Rajiv Dayal]

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.

Comment 22

[Rajiv Dayal]

Attachment: patch to take care of the comments

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 23

[Alec Flett]

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.

Comment 24

[Alec Flett]

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!

Comment 25

[Rajiv Dayal]

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

Comment 26

[Rajiv Dayal]

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.

Comment 27

[Rajiv Dayal]

Attachment: patch to take care of comments

Comment 28

[Rajiv Dayal]

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.

Comment 29

[Alec Flett]

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

Comment 30

[Rajiv Dayal]

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.

Comment 31

[Rajiv Dayal]

Attachment: patch with to take care of all the comments

Comment 32

[Alec Flett]

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)

Comment 33

[Rajiv Dayal]

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

Comment 34

[Rajiv Dayal]

Attachment: latest patch with all the cleanup

Hi Scott,

Can u please take a look at the patch attached.

thanks, - Rajiv.

Comment 35

[Scott MacGregor]

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.

Comment 36

[Rajiv Dayal]

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 37

[Rajiv Dayal]

Attachment: updated patch

Comment 38

[Scott MacGregor]

Comment on attachment 62588 [details] [diff] [review]
updated patch

great. sr=mscott

Comment 39

[Rajiv Dayal]

Comment on attachment 62588 [details] [diff] [review]
updated patch

has another sr=alecf as per his comments # 32.

Comment 40

[Rajiv Dayal]

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.

Comment 41

[Rajiv Dayal]

updated patch (id=62588) checked into MAPI_NEW_DIR_TRUNK.

Comment 42

[Rajiv Dayal]

checked into the trunk.

Comment 43

[Trix Supremo]

verified on mozilla & netscape trunk 2002022703 builds