Closed Bug 836963 Opened 12 years ago Closed 12 years ago

Work - LoginManager fills forms too late causing no u/p matching because of deault value username fields

Categories

(Firefox for Metro Graveyard :: General, defect)

x86_64
Windows 8.1
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bbondy, Assigned: bbondy)

References

Details

(Whiteboard: feature=work [completed-elm])

Attachments

(1 file)

Some login input fields are preloaded with a value like in bugzilla with "email address". This causes the old loginmanager code to only match on "email address". This patch (which I'm about to post) changes it to work like the desktop Firefox one where we fill before these values are preloaded. Similar code exists here: http://dxr.mozilla.org/mozilla-central/toolkit/components/passwordmgr/nsLoginManager.js.html#l202
Attached patch Patch v1.Splinter Review
Since you're already reviewing another login manager bug, I figured I'd send this one your way too. To explain a bit more, when you go to bugzilla.mozilla.org and are logged out, the username field contains 'email address'. When you click on that field with your mouse it clears the text and allows you to enter your own username. The old login manager code looks in the db and tries to find a username called 'email address' that was previously saved. But no one's username is really 'email address'. This new patch matches a blank string for username (accepting any usernames) and works the same as desktop Firefox because it is called before the default form value is filled.
Attachment #708838 - Flags: review?(ally)
Summary: LoginManager fills forms too late causing no u/p matching because of deault value username fields → Work - LoginManager fills forms too late causing no u/p matching because of deault value username fields
Whiteboard: feature=work
Depends on: 837295
Comment on attachment 708838 [details] [diff] [review] Patch v1. Review of attachment 708838 [details] [diff] [review]: ----------------------------------------------------------------- Like bug 836915, I feel I'm not qualified to review this one solidly. mbrubeck has graciously agreed to provide a secondary review for this one as well. - thank you for meaningful comments in your code :) - same comment from 836915 about test coverage options applies here as well. - same (optional) comment from 836915 about strs& info for qa applies. - nit-y question, but is there a particular reason you are using var or let? Some of my past reviewers have ranted at me pretty hard for *not* using let, so it seems to be a discouraged practice. - these look (on my screen anyway) like 4 space tabs. I thought we were a 2 space hard tab shop?
Attachment #708838 - Flags: review?(mbrubeck)
Attachment #708838 - Flags: review?(ally)
Attachment #708838 - Flags: feedback+
Thanks for the review. I generally use 2 space tabs unless the file is already a 4-space tab file. I usually also use let, but this code was copied from desktop so I just kept it the same. I'm doing tests for both bugs inside bug 837295 by the way.
Comment on attachment 708838 [details] [diff] [review] Patch v1. Review of attachment 708838 [details] [diff] [review]: ----------------------------------------------------------------- Ugh, we should probably just switch to using toolkit's nsLoginManager.js, so we can stop porting their bug fixes to our version. I believe the only reason our fork exists was for e10s support, which we no longer require. :/
Attachment #708838 - Flags: review?(mbrubeck) → review+
I was seriously considering that but I think the long term goal is to re-introduce e10s, so I'll push this in as is and we can reconsider that on the next bug.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: feature=work → feature=work [completed-elm]
Depends on: 838711
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: