Closed Bug 412723 Opened 12 years ago Closed 12 years ago

nsIPasswordManager is busted on trunk


(Calendar :: Sunbird Only, defect)

Not set


(Not tracked)



(Reporter: browning, Assigned: browning)




(2 files, 2 obsolete files)

... 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.
Attached patch support nsILoginManager on trunk (obsolete) — Splinter Review
Attachment #297491 - Flags: review?(philipp)
Attachment #297491 - Flags: review?(philipp) → review-
See existing bug 387569.
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.
Duplicate of this bug: 387569
OS: Linux → All
Hardware: PC → All
Blocks: 402206
Attached patch patch version 2 (obsolete) — Splinter Review
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 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).

Attachment #298651 - Flags: review?(philipp) → review+
Attached patch patch version 3Splinter Review
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
Attachment #299994 - Flags: review?(philipp)
Does this affect other password-related bugs like bug 361757 and bug 365036? Do we need to retest those?
(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 on attachment 299994 [details] [diff] [review]
patch version 3

>+        var hasPasswordManager;
>+        if (";1" in Components.classes) {
>+            hasPasswordManager = true;
>+        } else {
>+            hasPasswordManager = false;
>+        }
>+        if (hasPasswordManager) {
Any specific reason to use hasPasswordManager instead of testing for |";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+
patch (with reviewer's changes) checked in on trunk and MOZILLA_1_8_BRANCH

Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.8
Error: Components.classes[';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:
Resolution: FIXED → ---
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 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 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+
Additional patch already checked in -> Resolving FIXED again.
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.