Closed Bug 113540 Opened 23 years ago Closed 23 years ago

mail back-end depends on wallet

Categories

(MailNews Core :: Backend, defect, P3)

x86
Linux
defect

Tracking

(Not tracked)

VERIFIED FIXED
Future

People

(Reporter: waterson, Assigned: waterson)

References

Details

Attachments

(2 files, 2 obsolete files)

Maybe this isn't that big of a deal. But since I was fixing other stuff I
thought I'd do this, too.
Blocks: 18352
Status: NEW → ASSIGNED
Priority: -- → P3
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.
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.
Target Milestone: --- → Future
Oops, I shouldn't have commented out the SetPassword("") call. Fix that if this
patch ever lives.
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.
Attachment #60444 - Attachment is obsolete: true
mscott, sspitzer please review.  Thanks.
Comment on attachment 65568 [details] [diff] [review]
revised patch based on waterson's original one

r=mscott
Attachment #65568 - Flags: review+
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.

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)
Attachment #65568 - Attachment is obsolete: true
Patch revised.  mscott, sspitzer please re-review.  Thanks.
Comment on attachment 66185 [details] [diff] [review]
this time removing *all* wallet dependencies from mail

r=law
Attachment #66185 - Flags: review+
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.
Attachment #66185 - Flags: superreview+
checked in.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
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.
but, all that aside, thanks for fixing mailnews to not be dependent on wallet.
QA Contact: esther → stephend
Anybody object if I verify this using LXR, or am I going to have to look at 
makefile-foo?
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.
Status: RESOLVED → VERIFIED
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. 
Navin, is this still a problem, or has the issue you raised in comment 19 been 
resolved?
No response from navin so I assume that his concerns in comment 19 have been
resolved.
yes, I got around the problem. thanks
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: