113540 - mail back-end depends on wallet

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect, P3)

Description

[Chris Waterson]

Maybe this isn't that big of a deal. But since I was fixing other stuff I
thought I'd do this, too.

Comment 1

[Chris Waterson]

Attachment: use observer service instead of wallet interface

So this patch overlaps with attachment 60410 [details] [diff] [review] in bug 113515. It basically uses
the XPCOM observer service to store and clear passwords instead of the wallet
service directly. Shh, don't tell mitch.

Comment 2

[Chris Waterson]

Actually, I really don't think I care enough to try to fix this now. There are
other headaches in news that --disable-mail-news ought to fix for my immediate
needs.

Comment 3

[Chris Waterson]

Oops, I shouldn't have commented out the SetPassword("") call. Fix that if this
patch ever lives.

Comment 4

[Stephen P. Morse]

Attaching revised patch based on waterson's original one.  Part of his patch was 
already checked in as part of bug 113515 so that part is not present in my 
patch.  Also my new patch has the correction that waterson indicated in comment 
#3.  Tested it out and it works fine.

Comment 5

[Stephen P. Morse]

Attachment: revised patch based on waterson's original one

Comment 6

[Stephen P. Morse]

mscott, sspitzer please review.  Thanks.

Comment 7

[Scott MacGregor]

Comment on attachment 65568 [details] [diff] [review]
revised patch based on waterson's original one

r=mscott

Comment 8

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

looks good.  thanks for fixing mailnews.

one question, and forgive my string ignorance, but why is PromiseFlatString()
needed here?  

+rv = os->NotifyObservers(uri, "login-succeeded",
PromiseFlatString(NS_ConvertUTF8toUCS2(pwd)).get());
   
can't you just do:

rv = os->NotifyObservers(uri, "login-succeeded", NS_ConvertUTF8toUCS2(pwd).get());

address that (either by finding out it is needed, or removing it because it
isn't needed) and then sr=sspitzer

also note that nsNewsFolder.cpp depends on wallet.  (ah, it looks like waterson
already knew that,  "Actually, I really don't think I care enough to try to fix
this now. There are other headaches in news that --disable-mail-news ought to
fix for my immediate needs.").

just pointing that out, because after this patch the mail back-end will still
depend on wallet.

Comment 9

[Alec Flett]

right, PromiseFlatCString() is not required because NS_ConvertUCS2toUTF8 is
already a flat string

(you can see this for yourself: NS_ConvertUTF8toUCS2 derives from nsAutoString,
which derives from nsAFlatString - use http://lxr.mozilla.org/mozilla/ to learn
more)

Comment 10

[Stephen P. Morse]

Attachment: this time removing *all* wallet dependencies from mail

Comment 11

[Stephen P. Morse]

Patch revised.  mscott, sspitzer please re-review.  Thanks.

Comment 12

[Bill Law]

Comment on attachment 66185 [details] [diff] [review]
this time removing *all* wallet dependencies from mail

r=law

Comment 13

[jag (Peter Annema)]

Comment on attachment 66185 [details] [diff] [review]
this time removing *all* wallet dependencies from mail

Patch still has an unnecessary PromiseFlatString call in it.

Fix that and get an r= (or module owner approval) on the new patch from the
mail/news team, and you have sr=jag.

Comment 14

[Stephen P. Morse]

checked in.

Comment 15

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

Attachment: patch, to clean up the mailnews code

clean up for morse's fix.

instead of doing:

nsCOMPtr <nsIObserverService> os = do_CreateInstance(xxx);
if (os) {
 a;
 b;
 c;
}

I've fixed it to be

nsCOMPtr <nsIObserverService> observerService = do_CreateInstance(xxx, &rv);
NS_ENSURE_SUCCESS(rv,rv);

a;
b;
c;

I would have suggested renaming the variable (os) and changing the style to
avoid an unnecessary if {} block, but the checkin happened before I got to give
a  module owner review.  

next time, please get a module owner review.

unfortunately, all this thrashing just pushes the relevant checkin comments
down further, and makes cvs blame less useful.

Comment 16

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

but, all that aside, thanks for fixing mailnews to not be dependent on wallet.

Comment 17

[[e-mail] (gone - use [e-mail] instead)]

Anybody object if I verify this using LXR, or am I going to have to look at 
makefile-foo?

Comment 18

[[e-mail] (gone - use [e-mail] instead)]

1.144 sspitzer%netscape.com Jan 25 18:37 supplimental fix for #113540, clean up
code. rs=mscott
1.143 morse%netscape.com Jan 25 16:47 bug 113540, remove mailnews dependency on
password manager, r=mscott,sspitzer,jag,law
1.142 morse%netscape.com Jan 25 16:42 bug 113540, remove mailnews dependency on
password manager, r=mscott,sspitzer,jag,law

Code has landed, verified FIXED.

Comment 19

[Navin Gupta]

morse, I think you may have broken mail password code by removing walletService
dependency from mailnews backend. See bugs -> bug 121926, bug 140274. 

Now who is listening to "login-failed" notification. If I search in lxr I just 
see walletService listening to "login-failed". I think walletService never 
registers "login-failed" topic with the observerService. 

Comment 20

[Stephen P. Morse]

Navin, is this still a problem, or has the issue you raised in comment 19 been 
resolved?

Comment 21

[Stephen P. Morse]

No response from navin so I assume that his concerns in comment 19 have been
resolved.

Comment 22

[Navin Gupta]

yes, I got around the problem. thanks