Closed
Bug 412723
Opened 16 years ago
Closed 16 years ago
nsIPasswordManager is busted on trunk
Categories
(Calendar :: Sunbird Only, defect)
Calendar
Sunbird Only
Tracking
(Not tracked)
RESOLVED
FIXED
0.8
People
(Reporter: browning, Assigned: browning)
References
Details
Attachments
(2 files, 2 obsolete files)
5.48 KB,
patch
|
Fallen
:
review+
|
Details | Diff | Splinter Review |
1.74 KB,
patch
|
ssitter
:
review+
|
Details | Diff | Splinter Review |
... or rather we need to use nsILoginManager on trunk instead, since it works and the other doesn't. At any rate Sunbird/trunk can't log in to remote calendars (CalDAV for sure, I assume ICS as well) at the moment.
Assignee | ||
Comment 1•16 years ago
|
||
Attachment #297491 -
Flags: review?(philipp)
Assignee | ||
Updated•16 years ago
|
Attachment #297491 -
Flags: review?(philipp) → review-
Comment 2•16 years ago
|
||
See existing bug 387569.
Comment 3•16 years ago
|
||
Bruno, thanks for taking care. Any specific reason you didn't use the loginManager's findLogins() function? See gdata for an example. I'm duping the other bug against this one since you already attached a patch.
Updated•16 years ago
|
OS: Linux → All
Hardware: PC → All
Assignee | ||
Comment 5•16 years ago
|
||
yes, findLogins() was one reason I yanked the first version of this patch; this uses it and behaves better on branch. Tested Sb/Ltn, trunk/branch.
Attachment #297491 -
Attachment is obsolete: true
Attachment #298651 -
Flags: review?(philipp)
Comment 6•16 years ago
|
||
Comment on attachment 298651 [details] [diff] [review] patch version 2 Code looks fine, I assume you have tested this still works on branch (and of course trunk). r=philipp
Attachment #298651 -
Flags: review?(philipp) → review+
Assignee | ||
Comment 7•16 years ago
|
||
I did a one-last-test-before-checkin and found another problem that needed a new rev of the patch. Tested this one this morning 4x: Sb/Ltn, trunk/branch; working fine.
Attachment #298651 -
Attachment is obsolete: true
Assignee | ||
Updated•16 years ago
|
Attachment #299994 -
Flags: review?(philipp)
Comment 8•16 years ago
|
||
Does this affect other password-related bugs like bug 361757 and bug 365036? Do we need to retest those?
Assignee | ||
Comment 9•16 years ago
|
||
(In reply to comment #8) > Does this affect other password-related bugs like bug 361757 and bug 365036? Do > we need to retest those? > 365036 is more of a timing issue than a password one, so I don't see any relation there. I suppose it's possible that this patch might help with 361757, in the Sunbird-on-trunk case only. But I haven't tested that, and it will not have done anything for Lightning-on-trunk or either-on-branch.
Comment 10•16 years ago
|
||
Comment on attachment 299994 [details] [diff] [review] patch version 3 >+ var hasPasswordManager; >+ if ("@mozilla.org/passwordmanager;1" in Components.classes) { >+ hasPasswordManager = true; >+ } else { >+ hasPasswordManager = false; >+ } >+ >+ if (hasPasswordManager) { Any specific reason to use hasPasswordManager instead of testing for |"@mozilla.org/passwordmanager;1" in Components.classes| directly? >+ if (aPasswordRealm.passwordRealm) { >+ var passwordRealm = aPasswordRealm.passwordRealm; >+ } else { >+ var passwordRealm = aPasswordRealm; >+ } While this works fine in js since variables are scoped on a function level, I'd prefer var passwordRealm; if (aPasswordRealm.passwordRealm) { passwordRealm = aPasswordRealm.passwordRealm; } else { passwordRealm = aPasswordRealm; } Or even better: var passwordRealm = aPasswordRealm.passwordRealm || aPasswordRealm; r=philipp with above comments fixed
Attachment #299994 -
Flags: review?(philipp) → review+
Assignee | ||
Comment 11•16 years ago
|
||
patch (with reviewer's changes) checked in on trunk and MOZILLA_1_8_BRANCH ->FIXED
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Updated•16 years ago
|
Target Milestone: --- → 0.8
Comment 12•16 years ago
|
||
Error: Components.classes['@mozilla.org/login-manager;1'] is undefined Source File: chrome://passwordmgr/content/passwordManagerCommon.js Line: 24 Not yet fixed completely, at least not for win32 platform. We also need to ship the new files on Trunk. From looking at attachment 264995 [details] [diff] [review] in Bug 374723 we might be: +bin\components\loginmgr.xpt +bin\components\nsLoginInfo.js +bin\components\nsLoginManager.js +bin\components\nsLoginManagerPrompter.js +bin\components\storage-Legacy.js
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 13•16 years ago
|
||
Looks to me like this would do it, but I don't have Windows here to test it with; appreciate it if someone else could do so.
Comment 14•16 years ago
|
||
Comment on attachment 302335 [details] [diff] [review] add windows installer stuff Stefan, can you have a look at the patch?
Attachment #302335 -
Flags: review?(ssitter)
Comment 15•16 years ago
|
||
Comment on attachment 302335 [details] [diff] [review] add windows installer stuff I tested it on Windows and found that bin\components\storage-Legacy.js is also required. r=ssitter if you add bin\components\storage-Legacy.js before checkin.
Attachment #302335 -
Flags: review?(ssitter) → review+
Comment 16•16 years ago
|
||
Additional patch already checked in -> Resolving FIXED again.
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•