Closed
Bug 113540
Opened 23 years ago
Closed 23 years ago
mail back-end depends on wallet
Categories
(MailNews Core :: Backend, defect, P3)
Tracking
(Not tracked)
VERIFIED
FIXED
Future
People
(Reporter: waterson, Assigned: waterson)
References
Details
Attachments
(2 files, 2 obsolete files)
7.65 KB,
patch
|
law
:
review+
jag+mozilla
:
superreview+
|
Details | Diff | Splinter Review |
6.40 KB,
patch
|
Details | Diff | Splinter Review |
Maybe this isn't that big of a deal. But since I was fixing other stuff I thought I'd do this, too.
Assignee | ||
Updated•23 years ago
|
Assignee | ||
Comment 1•23 years ago
|
||
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.
Assignee | ||
Comment 2•23 years ago
|
||
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
Assignee | ||
Comment 3•23 years ago
|
||
Oops, I shouldn't have commented out the SetPassword("") call. Fix that if this patch ever lives.
Comment 4•23 years ago
|
||
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•23 years ago
|
||
Attachment #60444 -
Attachment is obsolete: true
Comment 6•23 years ago
|
||
mscott, sspitzer please review. Thanks.
Comment 7•23 years ago
|
||
Comment on attachment 65568 [details] [diff] [review] revised patch based on waterson's original one r=mscott
Attachment #65568 -
Flags: review+
Comment 8•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
Updated•23 years ago
|
Attachment #65568 -
Attachment is obsolete: true
Comment 11•23 years ago
|
||
Patch revised. mscott, sspitzer please re-review. Thanks.
Comment 12•23 years ago
|
||
Comment on attachment 66185 [details] [diff] [review] this time removing *all* wallet dependencies from mail r=law
Attachment #66185 -
Flags: review+
Comment 13•23 years ago
|
||
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+
Comment 14•23 years ago
|
||
checked in.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 15•23 years ago
|
||
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•23 years ago
|
||
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
Comment 19•22 years ago
|
||
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•22 years ago
|
||
Navin, is this still a problem, or has the issue you raised in comment 19 been resolved?
Comment 21•22 years ago
|
||
No response from navin so I assume that his concerns in comment 19 have been resolved.
Comment 22•22 years ago
|
||
yes, I got around the problem. thanks
Updated•20 years ago
|
Product: MailNews → Core
Updated•16 years ago
|
Product: Core → MailNews Core
You need to log in
before you can comment on or make changes to this bug.
Description
•