98566 - separating out MAPI support release

Status: VERIFIED FIXED

(MailNews Core :: Simple MAPI, defect)

Description

[Rajiv Dayal]

The MAPI support will be released as a patch for eMojo. To facilitate this the 
changes to existing code, UI and strings for localization done to provide 
MAPI support in Mozilla needs to be part of the eMojo release. And the 
actual MAPI support feature will be released as a patch with the new Dlls for 
its implementation at a later date.

This bug is to track the changes to be done in existing code (xpfe\bootstrap), 
UI and strings for the eMojo release.

Comment 1

[Srilatha Moturi]

Attachment: patch v1

Comment 2

[Srilatha Moturi]

Attachment: patch v2

Comment 3

[Srilatha Moturi]

patch v1 is the diff for UI and prefs code
patch v2 is the diff for changes in xpfe/bootstrap code

Comment 4

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

instead of doing this in nsAppRunner.cpp,

+#ifdef XP_WIN32
+  StartMapi();
+#endif

do this:  

move the code for CheckMapiSupport() into StartMapi(), and then where you are
calling CheckMapiSupport() in nsNativeAppSupportWin.cpp, you call StartMapi().

Comment 5

[Rajiv Dayal]

The StartMAPI call is made separately in nsAppRunner so that it is called after 
the profile is set/selected. If this is not done, in case if the Mozilla is 
started separately and a MAPI call is made at the same time, if profile is not 
selected before the StartMAPI call two Profile Manager dialogs would be 
displayed to the user. 

Comment 6

[Rajiv Dayal]

Hi Bill and Conrad,

Can u please review the patch v2 for changes to the nsAppRunner and 
nsNativeAppSupportWin.cpp. These are small changes done to facilitate 
loading and initializing MAPI support dll. Details of the design for MAPI 
support is in the doc attached to the MAPI support main bug # 91702.

Comment 7

[Srilatha Moturi]

Attachment: screen shot from jglick for the mailnews prefs pane

Comment 8

[Srilatha Moturi]

Attachment: comments from jennifer and robin.

Comment 9

[Srilatha Moturi]

Assigning QA to trix.
The patch v1 has the changes based on comments from Jennifer and Robin.

Comment 10

[Conrad Carlen (not reading bugmail)]

I don't think using nsNativeAppSupportWin::EnsureProfile() in this way is right.
It was a private method, designed to support -turbo mode, and be called when we
get a request to make a new window. Calling it from nsAppRunner at this point
can cause profile UI to be shown if the app was launched with the -turbo arg.
That's not supposed to happen.

Comment 11

[Krishna Mohan]

The profile manager is NOT shown if you start mozilla in turbo/server mode.  
Also, EnsureProfile is not called in nsAppRunner.  Only callback is passed to 
MAPI clients.  MAPI clients invokes mozilla for a new compose window (mail 
services).  This bug is related to 95117.

Comment 12

[Conrad Carlen (not reading bugmail)]

It calls StartMapi() which calls the initialization function, passing a proc ptr
to EnsureProfileForMapi(). I guess the initialization function calls that at
some later time, when a window is being made? Then it's OK. My mistake for
assuming it was called right away.

Comment 13

[Bill Law]

I don't fully understand the entirety of this MAPI support business.  I'm
confused by Rajiv's comment on 2001-09-06 16:37:

>The StartMAPI call is made separately in nsAppRunner so that it is called after 
>the profile is set/selected

It looks to me as if the whole point of StartMAPI is to provide a hook into
EnsureProfile to the mapi dll.  But if it is true that "it is called after the
profile is set/selected" then I'm not sure what the point of providing that hook
is (i.e., EnsureProfile doesn't do anything if a profile is already
set/selected).  So I'm very confused on this point.

But strictly from the point of view of nsAppRunner/nsINativeAppSupport, here's
the concerns I have:

1. I'd prefer not to see #ifdef <platform> code proliferate in nsAppRunner.  The
platform specific stuff is supposed to be in the platform-specific
implementation of nsINativeAppSupport.  Specifically, rather than add a call to
StartMAPI in nsAppRunner.cpp, I'd rather see that call somewhere in
nsNativeAppSupportWin.cpp.

2. If possible, I'd prefer to not have all the MAPI-specific stuff added to
nsNativeAppSupportWin.cpp.  Instead, I think it would be better to package that
code up in the implementation of some new xpcom interface and have
nsNativeAppSupportWin utilize that interface.  Basically, I'd like to keep the
code a little more modular.

It might be that organizing the code along these lines will help clear up some
of the confusion/problems with the current proposal.

Comment 14

[Rajiv Dayal]

Bill,

The StartMapi calls the InitFunction of the mapi support dll and passes the
EnsureProfile fn pointer used by MAPILogon implementation. This Init Function
however also registers the MS COM component for MAPI support. 

Thus if mozilla is started and an MAPI application makes a MAPILogon call
simultaneously, the MS COM MAPI support call from the MAPI app is blocked since
the MS COM component for it is not yet registered. Meanwhile the Profile set up
for mozilla start up thread goes thru and a profile is now set. The StartMAPI
call is then made from nsAppRunner which registers the MAPI support component.
The MS COM MAPI support call thread is then unblocked since the MS COM component
is registered now and the Ensure Profile returns the profile that has been set
during mozilla startup. And thus it does not throw up the Profile manager dialog
again.

When we tested this we amazingly found that the MSCOM call from the MAPI app
does not return immediately if the MS COM components are not registered, but it
blocks for quite some time before returning. The time is quite enough for the
mozilla start up thread to throw up the Profile dialog and user selection of the
profile. Of course the call fails if the user waits too long for profile
selection.

This solution is not perfect and depends on how long the user takes to make the
selection, however this solution is still better than throwing up two profile
manager dialogs. If the InitFunction call is made from
nsNativeAppSupportWin::Start, in the above case of MAPILogon and mozilla startup
happening simultaneously, EnsureProfile call from MAPILogon might throw up the
Profile dialog and also the EnsureProfile call from mozilla start up thread
would throw up the profile dialog.

Hope i could make the reason for calling StartMapi from nsApprunner clear. We
could not figure out a better place to put the call. If you think there is any
code in nsNativeAppSupportWin which is called after Profile selection we can put
the code there too. 

Comment 15

[Rajiv Dayal]

I think to take care of the above situation and deal with EnsureProfile call
made simultaneously and avoid the case where Profile Manager dialog is thrown up
twice a better solution is to make EnsureProfile thread safe. That is make the
function implemetation protected by a Mutex.

Bill, if we do this will it effect other any other places adversly .. maybe for
other OS ?

Krishna can u try this out. For our MAPI support too we will not have to rely on
how long user takes to select the Profile.

thanks,

- rajiv.

Comment 16

[Bill Law]

OK, thanks for the clarification.

I wasn't necessarily suggesting that StartMAPI() be called at a different point
in time.  I think it should be called from the Windows implementation of some
nsINativeAppSupport method, is all.  nsINativeAppSupport::Start maybe isn't the
right method.  We could add a new method to nsINativeAppSupport and call that at
the appropriate point in time.

But back to this business of multiple profile manager dialogs:  It seems that
the nsmapi.dll code involves another thread and will invoke EnsureProfile on
some other thread (other than the main Mozilla UI thread).  Is that right?
It seems that that is a problem in and of itself, because Mozilla itself can
open a profile manager window at any point after the main thread enters the
message loop.  This is after the call to StartMAPI (as currently proposed).

Specifically,

1. User starts Mozilla in -turbo mode.
2. No profile manager appears.
3. StartMAPI is called
4. MAPI application triggers call to EnsureProfile (and display of profile
manager window)
5. User double clicks on Mozilla program icon.
6. Second instance of Mozilla sends WM_COPYDATA to first.
7. First instance (main thread) gets request, calls EnsureProfile, and opens
profile manager window.

It seems like your idea of using a mutex would protect against this case, as
well.  But why not just use the existing mechanisms to serialize the requests? 
E.g., you could pass the message window handle instead of a pointer to a
function and have the mapi code send messages to that window.

To summarize:  I still have concerns about how this new code is packaged (I'd
prefer there not be a static linkage between nsAppRunner and
nsNativeAppSupportWin, and, I'd prefer the MAPI-specific code to go into its own
module).  In addition, I would like to understand the nature of the hook being
exposed by this code: when will EnsureProfileForMAPI be called, and in what context?

Comment 17

[Rajiv Dayal]

we are modifying the code to define a new method to the nsINativeAppSupport
interface for the MAPI specific stuff to avoid the static linkage between
nsAppRunner and nsNativeAppSupportWin, as well as making EnsureProfile thread
safe using a mutex.

EnsureProfileForMAPI will be called from MAPILogon. The MAPI app makes call to
MAPILogon for using a mail account. Mail accounts are related to profiles and
the profile needs to be set first before a mail account can be used. Hence we
need to call EnsureProfile from the mapi dll.

Comment 18

[Krishna Mohan]

Attachment: Revised code as per review comments

Comment 19

[Rajiv Dayal]

For patch v1, used for enable/disable and set/unset of mailnews pref for mapi
support :
- there is an invalid character 's' in mail3PaneWindowVertLayout.xul, 4th line
in the patch
- there are changes related to tabs in 'pref-mailnews.xul', last 10 lines in the
patch
- please put the new files into mapi dir instead of nsmapi dir, the existing
mapi dir has been cleaned up by Seth.
- please add the error message for any Windows registry error to the properties
file

Comment 20

[Srilatha Moturi]

Attachment: Patch for preferences, addresses reviewers comments

Comment 21

[Krishna Mohan]

Based on the review comments I have renamed nsMapi.dll to mzMapi.dll and the 
same is reflected in the patch dated 09/07/01 17:58 ("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.

Bill : I have made changes to nsINativeAppSupport interface and removed static 
linkage between nsNativeAppSupportWin and nsAppRunner.  Also, I can not use 
WM_COPYDATA synchronization as it opens up a window by default, which may not be 
required by MAPI clients in case of silent/blind send (send mail without any UI)

Comment 22

[Rajiv Dayal]

For the latest Patch for preferences :
- it would be a good idea to do a try and catch for prefIsLocked call to take
care of the case if the pref is not defined at all. In the catch you can
directly call the pref set API which will define the pref. This needs to be done
both in mailintegration.js and pref-mailnewsOverlay.js.
- i think it would be better if we call hook.showMailIntegrationDialog only if
the preference is not already set (to use mozilla as default mail client). 

Comment 23

[Srilatha Moturi]

Attachment: patch for preferences, added try,catch and updated license too

Comment 24

[Rajiv Dayal]

r=rdayal for preferences checkin only (latest patch from Srilata 09/10/01
14:03), see review comments above (2001-01-10 11:47, 13:54).

Comment 25

[Bill Law]

re: nsINativeAppSupport.idl

I think "MakeSureProfile" should be "ensureProfile".

re: nsAppRunner.cpp

You need to test nativeApps before calling through that pointer (it might be
null; theoretically, at least).

re: nsNativeAppSupportWin.cpp

Krishna and I talked at length about how the multi-threading works vis-a-vis the
 MAPI requests.  Based on our discussion, the mutex inside EnsureProfile is not
necessary (that function is always called on the main thread so the mutex
request never fails anyway).  He talked be through the various scenarios to
convince me all was well.  But I'd like for the super-reviewer to verify that
his assumptions are OK.

Basically, I wanted to consider the scenarios whereby a MAPI request comes
through EnsureProfile while EnsureProfile was executing a "normal" request, and
vice-versa.

Given the minor changes mentioned above, r=law.

Comment 26

[Rajiv Dayal]

there is a small thing in EnsureProfile, if u are using a mutex it needs to be
Unlocked before returning .. not locked .. i guess a typo or cut paste error

Comment 27

[Sheela Ravindran]

As per esther assigning this bug to Hong 

Comment 28

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

instead of nsmapi.properties, please call the file mapi.properties.

Comment 29

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

"%S Mail"?  or "%S Mail & Newsgroups"?  see bug #98453

Comment 30

[Srilatha Moturi]

Attachment: patch with just dtd, properties, mn and makefile.win files

Comment 31

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

Comment on attachment 48955 [details] [diff] [review]
patch with just dtd, properties, mn and makefile.win files

sr=sspitzer

Comment 32

[kmurray1115]

I guess the answer to the "& Newsgroups" part lies within whether MAPI supprts
newsgroups. Do we honor newsgroups in MAPI operations?

Comment 33

[Rajiv Dayal]

MAPI support will be only for mail and not newsgroups.

Comment 34

[jglick]

Re: "%S Mail"?  or "%S Mail & Newsgroups".  Kevin, what do you want to do with 
this? There lots of places where we use the product name, Prefs, dialog titles, 
wizards, etc. Do you want the full long name in each place?  It just seems out 
of place in some instances.

srilatha, are you making a new group box for this or adding this checkbox to the 
"General" group box?  I think it belongs grouped with the "General" items. Also 
if it is its own group box, we might run into problems with stuff getting cut 
off for users with bigger fonts.

Comment 35

[kmurray1115]

We definetly need to go case by case. The most visible ones are covered I think,
but yeah, let's discuss them as they come up. And, in this case, looks like
"Mail" is sufficient.

Comment 36

[Srilatha Moturi]

Jennifer, I am putting the checkbox in the General Settings group.

Comment 37

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

question, are you sure we want to add that call to ComposeLoad()?  that will 
bring up the intergration dialog when the compose window compose up.  I don't 
think 4.x does that.  

if we want to do it on every mail related dialog, we'll have to worry about 
addressbook.

either way, there should be a way to do this so we don't need to build 
mozilla/mailnews/mapi on linux or mac.

you've added three calls:

mailnewsOverlayInit() in pref-mailnews.js
showMailIntergrationDialog() in OnLoadMessegner()
showMailIntegrationDialog() in ComposeLoad()

for mailnewsOverlayInit(), I'd suggest doing this:

in pref-mailnewsOverlay.xul, add initFunc="mailnewsOverlayInit()" to 
the "mailnewsEnableMapi" element.  then, in Init() in pref-mailnews.js, follow 
what I did in utilityOverlay.js, toggleOfflineStatus()

basically, if you can get the initFunc attribute of "mailnewsEnableMapi" 
attribute, you eval() it. 

for showMailIntergrationDialog(), you are going to need a overlay that includes 
the mailintegration.js, since it won't be available on mac on linux.  then, you 
can use a similar trick.

mac and windows might have there own non-MAPI OS integration.  I suggest you 
keep with the "mail OS integration" theme, and avoid using MAPI in any elements 
or filenames that are not MAPI specific.

Comment 38

[Michele Carlson]

L10n approves late UI changes. Please check them in this week.
thanks

Comment 39

[Srilatha Moturi]

Seth, I have tried adding the initFunc and calling it from Init() but the 
checkbox "mailnewsEnableMapi" (this is in pref-mailnewsOverlay.xul) is not 
getting displayed unless I have the following line in pref-mailnews.xul.

+<?xul-overlay href="chrome://messenger/content/pref-mailnewsOverlay.xul"?>

Since pref-mailnewsOverlay.xul exists only on win, I will get an error on unix 
and mac "Error reading file pref-mailnewsOverlay.xul"
Any suggestions how I can get the checkbox displayed without adding the above 
line.

Comment 40

[Krishna Mohan]

Attachment: fixing the regressions and implemented the review comments on 0.9.4 branch

Comment 41

[Krishna Mohan]

There were some regressions b'cos of the check-ins made by others.  Fixed those 
regressions.  Also, implemented the review comments from Bill Law.  The patch is 
generated from a 0.9.4 branch.

Comment 42

[Rajiv Dayal]

Alec can u please sr the latest patch posted by Krishna (2001-09-13 11:27). 
The EnsureProfile function returns an error if it is doingProfileStartup, this 
is used to return an error for Mapi support too if user either starts 2 MAPI 
apps simultaneously or, mozilla and MAPI apps simultaneously.

Comment 43

[Alec Flett]

Comment on attachment 49227 [details] [diff] [review]
fixing the regressions and implemented the review comments on 0.9.4 branch

It's not clear to me what will happen when EnsureProfile is called. In e-mail you said that a dialog will pop up, but I don't see where this happens in the patch. If it happens automatically, can you document this in EnsureProfile?

Secondly, all IDL should follow interCaps convention. StartExtraFeatures should be startExtraFeatures, and so forth.

Where is MakeSureProfile even called?

Finally, "MakeSure" is just a synonym for "Ensure" - can we either come up with a better name or merge these two functions? Since EnsureProfile is a private function to nsNativeAppSupportWin, seems like we could simply merge the two.

Comment 44

[Alec Flett]

ah, I see this is a branch checkin
ok the above patch is sr=alecf for the branch if you fix the IDL interCaps stuff
- all the other changes must be fixed to land on the trunk

Comment 45

[Krishna Mohan]

I will change "StartExtraFeatures" to "startAddonFeatures".  Similarly 
"StopExtraFeatures" to "stopAddonFeatures and others also.  Is this OK?

Coming to EnsureProfile; this is not only private but static as well.  I need a 
non static member function without any static linkage to other files.  So, I 
synonymously named it and also exposed through nsINativeAppSupport interface for 
use by MAPI invocations.

Finally, when Mapi Clients invokes EnsureProfile via MakeSureProfile and 
MAPILogon, no dialog is displayed except the Profile Manager Dailog.

Also, please put all your comments, to check-in to the trunk.  B'cos the 
priorities seems have been changed.

Comment 46

[Alec Flett]

yes, the interCaps stuff is fine

EnsureProfile does not HAVE to be static - it just is that way because
HandleRequest is static - but in fact HandleRequest can get the nativeAppSupport
through the appshell service.

My suggestion is to expose ensureProfile() through IDL, and when you need the
nativeAppSupport, you get it. (i.e. at the top of HandleRequest())

Comment 47

[Rajiv Dayal]

Krishna, we still need to get this thing done (sr and checkin) and close this 
bug. So please keep posting comments here for the patches here and related to 
the xpfe/bootstrap changes for mapi support.

Comment 48

[Krishna Mohan]

Alec : I feel we should leave makesureProfile as it is.  HandleRequest and 
EnsureProfile both are members of nsNativeAppSupportWin (only).  if we make 
EnsureProfile a non-static member function, HandleRequest has to take indirect 
way (appshell->nativeapp->EnsureProfile) to call its own sibling.  Where as if 
we call it from makesureProfile it is taking help of a private function 
directly.

No matter whatever we choose, both the approaches will work. Plese let me know 
what you think about it.

Comment 49

[Alec Flett]

ok, per discussion on AIM, Krishna will check this stuff in on the
netscape-owned branch, and clean it up for the trunk
sr=alecf on the branch-only patch.

Comment 50

[Bill Law]

Per my previous comment a few days back, I've reviewed this and it is OK save 
the similar comments about capitalization in the .idl and the function name.

The patch still has UpperCase method names, right?  The patch description 
says "implemented review comments" though, so I was a little confused about 
that.

Also, I think there is no problem having the method named "ensureProfile" and 
also having a static (implementation) member function of the same name.

Anyway, r=law (again).

Comment 51

[Alec Flett]

Comment on attachment 49227 [details] [diff] [review]
fixing the regressions and implemented the review comments on 0.9.4 branch

once again marking sr=alecf since pdt doesn't seem to be reading bug comments

Comment 52

[chris hofmann]

ok, check this in on the branch if its ready.

Comment 53

[Srilatha Moturi]

Checked in the UI part of this bug. see bug # 95122.

Comment 54

[Krishna Mohan]

Attachment: New patch after fixing the bugs found in developer testing

Comment 55

[Krishna Mohan]

Fixed a bug, found after integrating and testing the feature.  Also, the 
eunsureProfile is now implemented as an interface method as suggested in the 
'sr' comments.

Comment 56

[Krishna Mohan]

There was a problem when Mozilla is started,  later there is call from MAPI 
client to invoke mozilla.

I am passing two command line parameters to mozilla, when it is invoked by a 
MAPI client (/server and /MAPIStartUP).  When mozilla is already up and running
all command line parameters are handled in nsNativeAppSupportWin::HandleRequest.  
The new MAPI command line parameters are not handled before, hence Mozilla 
brings up a browser window as a default action.  The new patch (xpfePatch2) 
handles MAPI parameters in nsNativeAppSupportWin::HandleRequest function.

Comment 57

[Alec Flett]

Comment on attachment 50117 [details] [diff] [review]
New patch after fixing the bugs found in developer testing

Ok, this all looks good, but now I'm looking at this and wondering why 
startAddonFeature/stopAddonFeatures/EnsureProfile
are even exposed through the interface - I don't see
anyone calling them...
sr=alecf for the branch, but I need an explanation for the trunk

Comment 58

[Alec Flett]

Comment on attachment 50117 [details] [diff] [review]
New patch after fixing the bugs found in developer testing

also, can you mark the obsolete patches obsolete? I'm not sure which ones are 
still valid, and which ones are just old versions of this patch

Comment 59

[Krishna Mohan]

The EnsureProfile is being called from msgMapiHook.cpp (95117) and 
startAddonFeatures is being called from nsAppRunner.cpp.  stopAddonFeatures is 
called from a exposed interface method (stop).

Comment 60

[Alec Flett]

the explanation sounds good - I hadn't seen bug 95117, and I had misread part of
this patch. sr=alecf for the trunk too.

Comment 61

[Bill Law]

Krishna,  it looks like you've added this hunk (aside from the changes 
suggested previously):

@@ -1347,6 +1378,10 @@
       return;
     }
 
+    rv = args->GetCmdLineValue(MAPI_STARTUP_ARG, getter_Copies(arg));
+    if (NS_SUCCEEDED(rv) && (const char*)arg)
+        return;
+
 
     // ok, no idea what the param is.
 #if MOZ_DEBUG_DDE

Is that right?  I would like to see a comment there that says something like:
+    if (NS_SUCCEEDED(rv) && (const char*)arg)
+        // MAPI startup request.  Don't open any new windows and
+        // just return.
+        return;

I think that's the gist of it, correct?

Aside from that, r=law.

Comment 62

[Krishna Mohan]

that is right.. and i will put the comments.  thanks.

Comment 63

[Jaime Rodriguez, Jr.]

Pls update Patch Status with r=. 

check it into the trunk - PDT+, after a day, if nothing goes worng tomorrow.

Comment 64

[Rajiv Dayal]

The code for this bug has been checked in in the trunk and the 0.94 branch since
a few days now. Marking this as fixed.

Comment 65

[lchiang]

Hong, is this ok to mark verified since the code is on the branch 094?  I
believe there will be a new bug to track the code checkin for the trunk.

Comment 66

[lchiang]

trix - can you verify while hong is out?

Comment 67

[Trix Supremo]

verified on 2001-10-22-18-0.9.4 & 2001-10-23-11-trunk(ui verification only)

also changing Component from composition to Simple Mapi.