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

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: General, defect)

Description

[Brian R. Bondy [:bbondy]]

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

Comment 1

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1.

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.

Comment 2

[Allison Naaktgeboren :ally]

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?

Comment 3

[Brian R. Bondy [:bbondy]]

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 4

[Matt Brubeck (:mbrubeck)]

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.  :/

Comment 5

[Brian R. Bondy [:bbondy]]

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.

Comment 6

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/projects/elm/rev/cbb24f3d4d11