Closed Bug 839741 Opened 8 years ago Closed 8 years ago

Minor refactoring of login manager's load/fill triggers


(Toolkit :: Password Manager, defect)

Not set





(Reporter: Dolske, Assigned: Dolske)



(2 files)

Attached patch Patch v.0 (WIP)Splinter Review
Bug 771331 (by way of bug 355063) seeks to make login manager driven by the addition of password fields to the page, instead of having login manger scan every page exactly once when DOMContentLoad fires.

In preparation for this, I'd like to slightly refactor nsLoginManager.js so that is triggering logic is a little more isolated. Patch attached.

Things I'm unsure of but probably won't worry about:

* In an E10S future, we'll want to completely separate the content-touching code (ie, child-process code) from the main (parent) code. This is a step in the right direction, but incomplete in that respect. EG, the code that handles input field autocomplete will eventially need to move as well.

* This refactoring is still document-oriented, whereas 771331 will be form-oriented (or even form-field oriented). So there may need to be some more refactoring, but maybe that's best for a followup.
Attachment #712069 - Flags: feedback?(mnoorenberghe+bmo)
...this should also become a JSM at some point, but seemed best to deal with that entirely separately.
This moves _fillDocument() into ContentListener. I will combine this with the previous patch for the next round of review.

A couple of the challenges for bug 771331 are apparent:

* The current code throttles number of forms processed per-page, which will become tricky when we only get per-input/form events. As a first approximation, that's ok. The current code is an optimization due to to form processing being expensive, but not every form being a login form. One of the cases that triggered this is that every comment on is a form (hundreds to thousands per page), but none of those have password fields. It seems unlikely to me that useful pages will have tons of password fields, so this should be simple to throttle (if at all) down in in 771331 (or other DOM code).

* Boy, that master password (.uiBusy) code sure does suck. But I think this is also likely to easily shift over to per-form invocation, for the same reasons (ie, the current code doesn't know there are password fields until its inspected the form, in a post-771331-world I expect forms with password fields to be rather infrequent).
Attachment #712071 - Flags: feedback?(mnoorenberghe+bmo)
Grr. Actually, I want to go in a somewhat different direction, so let's just start over with a clean new bug.
Closed: 8 years ago
Resolution: --- → INVALID
No longer blocks: 355063
Attachment #712069 - Flags: feedback?(mnoorenberghe+bmo)
Attachment #712071 - Flags: feedback?(mnoorenberghe+bmo)
You need to log in before you can comment on or make changes to this bug.