Closed Bug 742396 Opened 8 years ago Closed 8 years ago

Change stored password notification should truncate long usernames


(Toolkit :: Password Manager, defect)

Not set





(Reporter: Margaret, Assigned: pookveeraya)


(Whiteboard: [good first bug][mentor=margaret][lang=js])


(1 file, 1 obsolete file)

This is the toolkit equivalent of bug 741858. See that bug for a testcase.

For the save login dialog, we call _sanitizeUsername:

but we don't do that for the change login dialog:
Can I be assigned to this bug please? Thank you!:)
Assignee: nobody → pookveeraya
Hi, Veeraya! Thanks for taking this bug! Let us know if you have any questions about how to implement/test the fix.
Attached patch patch to bug 742396 (obsolete) — Splinter Review
Tested manually with the test provided
Attachment #612431 - Flags: review?(margaret.leibovic)
Comment on attachment 612431 [details] [diff] [review]
patch to bug 742396

Thanks for the patch!

>diff --git a/toolkit/components/passwordmgr/nsLoginManagerPrompter.js b/toolkit/components/passwordmgr/nsLoginManagerPrompter.js

>-        if (aOldLogin.username)
>+        if (aOldLogin.username){
>+            var displayUser = this._sanitizeUsername(aOldLogin.username);

We generally prefer using let over var, although var is used in the rest of this file because this is older code. I don't think it matters much, though.

I'm not technically eligible to review toolkit code, so I'm passing this over to zpao!
Attachment #612431 - Flags: review?(paul)
Attachment #612431 - Flags: review?(margaret.leibovic)
Attachment #612431 - Flags: feedback+
Comment on attachment 612431 [details] [diff] [review]
patch to bug 742396

Review of attachment 612431 [details] [diff] [review]:

Thanks for taking this on Veeraya! If you can attach a new patch with the one change below, we can get this landed!

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js
@@ +1016,5 @@
>       */
>      _showChangeLoginNotification : function (aNotifyObj, aOldLogin, aNewPassword) {
>          var notificationText;
> +        if (aOldLogin.username){
> +            var displayUser = this._sanitizeUsername(aOldLogin.username);

I'm fine with var here, so let's just go with that.

@@ +1021,4 @@
>              notificationText  = this._getLocalizedString(
>                                            "updatePasswordMsg",
> +                                          [displayUser]);
> +        } else

Please put a brace around the else block too.
Attachment #612431 - Flags: review?(paul) → review+
Attachment #612431 - Attachment is obsolete: true
Attachment #612464 - Flags: review?(paul)
Comment on attachment 612464 [details] [diff] [review]
edited patch with extra braces

Review of attachment 612464 [details] [diff] [review]:

Looks great. Thanks!
Attachment #612464 - Flags: review?(paul) → review+
Keywords: checkin-needed
Veeraya, if you're interested in porting this patch to mobile in bug 741858, I can review it for you :)
This patch landed in mozilla-central for Firefox 14.  It will be included in tomorrow's nightly build.  Thank you!
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.