95113 - Send mail feature in simple MAPI

Status: VERIFIED FIXED

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

Description

[Tiantian Kong]

This is to track the implementation of the send mail feature in simple MAPI.

Comment 1

[Hong Kwon]

mass qa assigning all simple MAPI bugs to olgac 

Comment 2

[Rodney Velasco]

QA over to trix.

Comment 3

[Scott MacGregor]

Rajiv, can you post your current diffs here with the diffs from JF so I can see
the code you guys are calling? This might me help figure out why you aren't
getting a message body in the temp file.

Comment 4

[Rajiv Dayal]

Well the reason for not getting the message body in the temp file is that when i 
pass a unicode string for the body, it is again been converted in the mailnews 
code to unicode which of course will fail and results into an empty/null string.

JF has some fix for it and i am going to use it. However the primary problem we 
are facing is that the smtp connection is not made in case of a blind send (no 
UI), whereas in case of send with UI the connection is made and the message 
is sent even with an empty body. 

Anyway, I will put in the patch as u suggested.

Comment 5

[Scott MacGregor]

I think that's your problem. I disagree with your assertion that we aren't
making an SMTP connection. Looking at the socket logs we are clearly connecting
to the server and then are waiting for data to send. Since there is no message
content we don't have anything to send and I think the socket ends up spinning
waiting for data to send.

We'll see what happens once the body shows up. 

Comment 6

[Rajiv Dayal]

Scott, then how come the message is sent in case of send with UI even with an 
empty body ?

Anyway, let me do the testing with JF's patch and put my patch for ur perusal.

thanks.

Comment 7

[Tiantian Kong]

nsbranch+

Comment 8

[Jaime Rodriguez, Jr.]

Marking PDT per PDT.

Comment 9

[Rajiv Dayal]

Attachment: patch for the Send Mail feature of MAPI (patch v1)

Comment 10

[Rajiv Dayal]

Hi JF, can u please review the patch.
thanks, - rajiv.

Comment 11

[Jean-Francois Ducarroz]

Comment on attachment 49675 [details] [diff] [review]
patch for the Send Mail feature of MAPI (patch v1)

here are my comments:

1) to be clean and consistant with XPCOM, would be nicer than nsMAPISession::GetPassWord
and GetIdKey return a copy of the string that the caller will be in charge of freeing.

2) nsCOMPtr <nsILocalFile> pFile = do_CreateInstance (NS_LOCAL_FILE_CONTRACTID, &rv) is
inside a loop. You should be able to put it outside the loop and reuse it for every file.

3) pFile->GetURL (&pURL) return a copy of the url therefore you need to free it
(2 occurances in the code). The best way would be to declare pURL as a nsXPIDLCString,
that will take care or freeing the menory for you:

        nsXPIDLCString pURL ;
        pFile->GetURL (getter_Copies(pURL)) ;



4) nsMapiHook::ShowComposerWindow and nsMapiHook::BlindSendMail should share code instead
of duplicating it

5) Need to add a new line at the end of nsMapiHook.h, nsMapi.idl and nsMapiDefine.h

Comment 12

[Tiantian Kong]

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

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

Hi:

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

Pathes for these has been up on the bug. 

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

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

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


Rajiv, r, 		Pref, 95122					

Seth, sr, 		Pref, 95122

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

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


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

Comment 13

[Rajiv Dayal]

Please find the updated patch with the review comments attached below.

1) this change needs to be done in Login/LogoOff code which has the Session 
object, will change based on the change done there.

2) done

3) done, i thought the nsIFile will return a pointer to a URL string that is a 
member, but it does not, it creates the URL string each time GetURL is called. 
Also the GetURL returns a char * (takes in char **), i didn't know i can use 
XPIDLString there ! thanks.

4) all the code that is common in ShowComposerWindow and BlindSendMail is 
basically creation of nsCOMPtr objects. Since all these are created on stack, if 
i take out all the common code and put it in a separate function these objects 
will be out of scope when this common function returns. I too thought of 
avoiding code duplication and hence i have put as much common code possible in 
the PopulateCompFields.. functions.

5) done

Comment 14

[Rajiv Dayal]

Attachment: patch with updated code as per review comments

Comment 15

[Jean-Francois Ducarroz]

Comment on attachment 49767 [details] [diff] [review]
patch with updated code as per review comments

Still need a new line at the end of public/nsMapiDefines.h

. Appart that R=ducarroz

Comment 16

[Rajiv Dayal]

Hi David,

Can u please super review the latest patch attached (id=49767) ?

thanks,

- rajiv.

Comment 17

[Scott MacGregor]

I have some random review comments too:

1) (nit) you have some tabbing issues in MAPISendMail near the top of the method
2) (nit) more tabbing issues in your class declaration for nsMAPISendListener

3) Isn't this leaking a hidden window?
+    nsIDOMWindowInternal *hiddenWindow;
+    rv = appService->GetHiddenDOMWindow(&hiddenWindow);

4)Typically you should only name a variable with an 'a' in front of it if that
variable is an argument (the a stands for argument). People look at the code and
they see a local variable called aSendListener and they get confused thinking
it's an argument when it's really a local variable. You had a couple of thee
types of variables in nsMapiHook including:

nsCOMPtr <nsIMsgSendListener> aSendListener;
nsCOMPtr<nsIMsgCompose> aMsgCompose(do_CreateInstance(NS_MSGCOMPOSE_CONTRACTID));

5) In blind send, I don't think the following variable is used:
+    // assign to interface pointer from nsCOMPtr to facilitate typecast below
+    nsIMsgSendListener * pSendListener = sendListener ;  

6) In nsMapiHook::PopulateCompFields, I'd probably make the following
nsAutoStrings instead of nsStrings since I think the strings may be small:
+    nsString To ;
+    nsString Cc ; 
+    nsString Bcc ;

7) In nsMapiHook::PopulateCompFields, more local variables starting with the 'a'
notation: 
nsCString aAttachments ;

8) some more tabbing issues in the function name + arguments for:
nsresult nsMapiHook::PopulateCompFields
and nsMapiHook::PopulateCompFieldsWithConversion

9) In PopulateCompFieldsWithConversion, I would use nsAutoString for 
+    nsString From ;
+    nsString To ;
+    nsString Cc ; 
+    nsString Bcc ;
+    nsString Comma ;


10) In ShowComposerWindow , I'd use an auto string for the password variable. +
change the variable name for nsCOMPtr <nsIMsgSendListener> aSendListener ; to
lose the 'a' notation.

11)tabbing issues in the later half of nsMapiImp::SendMail.

12) What do you think our thread story is for simple mapi. When we we were
looking at it on my machine it looked like the mapi hooks code was always called
on the UI thread and not from another process. If that's the case then we can
get rid of all the proxy service code you have in there. It'd simplify your
patch a lot. Can we prove for sure that it is always on the UI thread? If
there's a case where it isn't on the UI thread are we vulnerable with the IsDone
callback to a race condition (we used to have monitors there but decided that
kind of solution wasn't needed)

Nice work Rajiv!

Comment 18

[David :Bienvenu]

+    NS_PRECONDITION(ppListener != nsnull, "null ptr");
+    if (! ppListener)
+        return NS_ERROR_NULL_POINTER;


can just be NS_ENSURE_ARG_POINTER(ppListener). That will put up an assertion,
like NS_PRECONDITION, and return an error. 

 nsresult rv ;
+
+    nsCOMPtr <nsIProxyObjectManager> proxyMgr = do_GetService
(NS_XPCOMPROXY_CONTRACTID, &rv) ;
+    if ( NS_FAILED(rv) || (!proxyMgr) ) return rv ;
+
+

could you initialize rv here, in case do_GetService returns success but a null
proxymgr? It shouldn't do that, but since you're checking both, you shouldn't
return an unitialzed rv.

 if (!pMapiConfig) return PR_FALSE ;  // get the singelton obj
+

but the routine returns an nsresult, so you shouldn't return a boolean.

here too: +    if (NS_FAILED(rv) || (!aSendListener) ) return PR_FALSE; 

And other places - please look at all your return values and fix them.

use nsAutoString instead of nsString for local variables - this should be fixed
throughout this patch.

please add a newline at the end of public/nsMapiDefines.h

I'll re-review after you fix the above issues. Thanks!

Comment 19

[David :Bienvenu]

I too have questions about the threading issues. How did we end up on our ui
thread from the mapi call? Did we do that on purpose, or does mapi somehow do
that? It would seem to me to be better to not be on the ui thread blocking (even
though we are pumping events), but rather, to be on a separate thread that we
didn't care so much about it blocking.

Comment 20

[Rajiv Dayal]

The mapi call comes into mozilla from an MS COM call made from mapi.dll 
which is loaded into the calling app's process space. Looks like MS COM is 
calling mozilla into the main thread, maybe by pumping events into the Windows 
Message Queue for the main window of mozilla ! Initially i too thought there 
would be a separate thread for the MS COM call and hence had the monitor 
initially. However looks like MS COM uses the standard windows message queues to 
communicate between processes which makes sense too. Thus it seems like the mapi 
call will always be made to the main thread (which i guess is also the UI thread 
for mozilla).

Hence we can either remove the proxies from the existing code which i too was 
thinking about or we can create a separate thread for handling the mapi calls 
when the MS COM MAPI support interface methods in mozilla are called.

I will try the above and test to see how it works.

Comment 21

[Scott MacGregor]

One last comment Rajiv before I forget. These new files are going to go into our
mailnews\mapi directory and not a new directory called nsmapi, right? 

Comment 22

[Rajiv Dayal]

that is correct, it will go into mapi dir and not nsmapi dir. You would see the 
nsmapi dir in the attachments here but that will go away after the integration 
with latest code of Logon / Logoff and MAPI Prefs.

Comment 23

[Rajiv Dayal]

Well, I tried creating a separate thread to do the BlindSendMail so that we need 
not block the main thread. However, since MS COM always calls in to the main 
thread, we need to block it anyway before we return the call. Also even if 
multiple calls are made all calls are queued to the main thread, maybe since MS 
COM would be putting it into the Windows message queue of Mozilla's main window. 

I am currently trying some other options for this thread thing, this will need 
for me to look into how exactly MS COM calls the main thread, etc, etc.

Meanwhile can u review the currently working implementation updated with the 
review comments.

thanks.

Comment 24

[Scott MacGregor]

Hey rajiv, I already did review the earlier patch. REad up in this bug to see my
review comments. I had a bullet list listed above from earlier this afternoon =).

Comment 25

[David :Bienvenu]

I have comments too, and I don't see a patch addressing those comments either -
am I missing something?

Comment 26

[Rajiv Dayal]

Attachment: patch with updated code as per review comments (v3)

Comment 27

[Rajiv Dayal]

Oops sorry for the confusion, i forgot to put the patch in ... it got a bit late 
yesterday night !

Please find the patch updated with the review comments from mscott and david.
- made sure local variables are not named with 'a'
- return NS_ERROR_FAILURE instead of PR_FALSE
- converted nsString and nsCString to nsAutoString and nsCAutoString 
respectively
- use NS_ENSURE_ARG_POINTER instead of NS_PRECONDITION
- removed proxied objects since they are on the same main /UI thread
- use nsCOMPtr for nsIDOMWindowInternal
- initialized rv at all places

Comment 28

[David :Bienvenu]

Instead of always adding a trailing comma and removing it at the end, here's a
cleaner way to deal with the comma stuff:

if (i > 0), prepend comma to next address. 

Then you can remove all that code that removes the extraneous trailing comma's.

Does that make sense?

There are still some tabs at the very begining of the diff, as near as I can
tell. The other stuff looks better, thanks! More comments later.

Comment 29

[David :Bienvenu]

One more comment, similar to the one I had for the patch in the other bug:

+    if (NS_FAILED(rv)) return rv ;
+
+    return (S_OK) ;

this seems to mix mapi error returns and nsresult errors - looking at the mapi
errors, they look to me to be small positive numbers, and nsresult error codes
are negative numbers. Should this be fixed?

Other than my last two comments, it looks good. fixing the comma stuff that I
mentioned earlier should remove a bunch of code, which would be nice.

Comment 30

[Rajiv Dayal]

There will be only a small difference if i prepend the comma before, i will have 
to put a check there too to make sure i dont prepend to the first address. But i 
will do the change anyway.

I am in the process of integration with the other Login / Logoff and Prefs 
feature and the following will be taken care during this :
- resolving the return values for Mapi 
- using mapi dir instead of nsmapi dir
- no tabs in code. 

Once i am done with integration i will put a patch with these changes.

Comment 31

[David :Bienvenu]

yes, the small check is "if (i > 0)" - I think that's much cleaner and less code
than never checking and then removing the trailing comma.

Comment 32

[Rajiv Dayal]

Attachment: patch updated with review comments

Comment 33

[Rajiv Dayal]

Please find the updated patch for MAPI send mail. This patch is updated for :
- prepend the append for the To, From, Cc and Attachments strings
- changed dirs name from nsMapi to Mapi, filenames to use msg prefix
- sprinkled some more checks for null
- matched return values to MAPI return codes before returning

This patch does not deal with the temp files in the special case of send from 
powerpoint / excel as mentioned in bug # 100669. A different patch relative to 
the patch here will be put up in that bug for the changes to handle the 
special case there.

Comment 34

[Rajiv Dayal]

Hi David,
Can u please super review the latest patch attached (id=50483). 
thanks,
- rajiv.

Comment 35

[David :Bienvenu]

I'll look at it, but it needs a review first.

Comment 36

[David :Bienvenu]

 nsCString smtpPassword ;


should be auto - passwords will never be > 64 characters long.

+            if (pURL)
+            {
+
			if (aAttachments.Length() > 0)
+
				aAttachments.Append(",") ;
+                aAttachments.Append(pURL) ;

got some tabs in here, or indentation is wrong. Other than than that, it looks
OK. Assuming you fix those, and the reviewer doesn't turn any other problems up,
then sr=bienvenu.

Comment 37

[Rajiv Dayal]

It has already been reviewed by J.F.

---- Additional Comments From Jean-Francois Ducarroz 2001-09-18 13:07 ----
>> From update of attachment 49767 [details] [diff] [review])
>> Still need a new line at the end of public/nsMapiDefines.h

>> . Appart that R=ducarroz

Thanks David for the super review.

Comment 38

[Jean-Francois Ducarroz]

Comment on attachment 50483 [details] [diff] [review]
patch updated with review comments

Save comments than Bienvenu (password and tabs). I haven't found anything else. R=ducarroz

Comment 39

[Jaime Rodriguez, Jr.]

check it in - PDT+

Comment 40

[Rajiv Dayal]

feature done, has r and sr

Comment 41

[Daniel Veditz [:dveditz]]

This doesn't appear to be checked in so can't be "fixed" (not counting the
private branch, which *doesn't* count). Please correct me if I'm wrong.

Comment 42

[Srilatha Moturi]

checked into the MOZILLA_0_9_4_BRANCH

Comment 43

[lchiang]

Shouldn't this bug be marked fixed now?  Or, are you using this bug to track the
checkin to the trunk?

Comment 44

[Tiantian Kong]

Fixed in branch

Comment 45

[Trix Supremo]

verified on 2001-10-12-05-0.9.4