Simplify form filling code and remove some legacy cruft

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Dolske, Assigned: Dolske)

Tracking

unspecified
mozilla39
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(8 attachments, 1 obsolete attachment)

39 bytes, text/x-review-board-request
Dolske
: review+
Details
39 bytes, text/x-review-board-request
Dolske
: review+
Details
39 bytes, text/x-review-board-request
Dolske
: review+
Details
39 bytes, text/x-review-board-request
Dolske
: review+
Details
39 bytes, text/x-review-board-request
Dolske
: review+
Details
39 bytes, text/x-review-board-request
Dolske
: review+
Details
39 bytes, text/x-review-board-request
Dolske
: review+
Details
39 bytes, text/x-review-board-request
Dolske
: review+
Details
Password manager's form filling code [_fillForm() in LoginManagerContent.jsm] could be cleaned up to be significantly easier to read. The main issue is the code flow isn't as obvious as it could be due to nested conditionals, and most of that is a result of trying to have a single path (no early returns) to the _notifyFoundLogins() call towards the end.

So here's a cleanup in 6 parts:

A - Remove the return type and the E10S-unfriendly fillForm() entry point on nsILoginManager. The latter isn't needed in our own code, and I don't see any usage in the Addons MXR. Also remove the passwordmgr-found-form notification, which is similarly unneeded and unused.

B - Remove the __notifyFoundLogins() and the passwordmgr-found-logins notification. This notification was originally added as a way to allow addons fix password manager limitations, but no addons are using it and the didntFillReason tracking is needless complication. Our own tests do use this notification as a signal to check the results of a form fill, so I've replaced it with a simpler passwordmgr-processed-form notification that always fires.

C - Remove the didntFillReason tracking.

D - Add some early returns and move/unnest some code around so it's clearer that we always end up with selectedLogin set to a login at a certain point.

E - Remove didFillForm tracking.

F - More early returns and code moving/unnesting.


(Reviewboard requests coming after a Try run)
Try looks green so far: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f9a36b08b64

But ReviewBoard decided to attach the review to bug 1135322 instead of this one. :( I'm not sure how to fix that.

https://bugzilla.mozilla.org/attachment.cgi?id=8567613
MozReview Request: bz://1135322/dolske
Flags: needinfo?(MattN+bmo)
(In reply to Justin Dolske [:Dolske] from comment #1)
> But ReviewBoard decided to attach the review to bug 1135322 instead of this
> one. :( I'm not sure how to fix that.

It uses the bug number of the first commit for review which was bug 1135322. You should be able to specify the revisions to review with the `-r` argument. e.g. `hg push -r {part1rev}::{partNrev}` or supposedly --from (neither of which I've used yet). See https://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/review-requests.html#managing-review-requests
Status: NEW → ASSIGNED
/r/4211 - Bug 1135451 part a - remove unused return type, kill E10S unfriendly fillForm from nsILoginManager, kill passwordmgr-found-form notification. (largely a backout of 439365)
/r/4213 - Bug 1135451 part b - remove _notifyFoundLogins() and passwordmgr-found-logins
/r/4215 - Bug 1135451 part c - remove didntFillReason
/r/4217 - Bug 1135451 part d - Use early returns to un-nest logic and make it more obvious that selectedLogin is always set.
/r/4219 - Bug 1135451 Part e - remove didFillForm
/r/4221 - 1135451 part f - use early return instead of else blocks

Pull down these commits:

hg pull review -r a9966b18d21e4109697354db4678f3c489d55a48
Attachment #8568259 - Flags: review?(MattN+bmo)
https://reviewboard.mozilla.org/r/4211/#review3343

::: toolkit/components/passwordmgr/nsILoginManager.idl
(Diff revision 1)
> -  jsval fillForm(in nsIDOMHTMLFormElement aForm);

I was thinking recently that we would use this for the fallback UIs to trigger an autofill. I would rather wait to remove the API so we don't have to add it back.
https://reviewboard.mozilla.org/r/4213/#review3347

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
(Diff revision 1)
> +    Services.obs.notifyObservers(form, "passwordmgr-processed-form", null);

Hmm… it feels weird to do this in this method (named "loginsFound"). I think it would be better in a `finally` block at the bottom of \_fillForm.
https://reviewboard.mozilla.org/r/4221/#review3365

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
(Diff revision 1)
> +    if (isFormDisabled) {
> +      log("autocomplete=off but form can be filled; notified observers");
> +      recordAutofillResult(AUTOFILL_RESULT.AUTOCOMPLETE_OFF);

Would you mind adding a commit to rename "isFormDisabled" to something less likely to be confused with @disabled? e.g. autocompleteOffRequested
Comment on attachment 8568259 [details]
MozReview Request: bz://1135451/dolske

https://reviewboard.mozilla.org/r/4209/#review3367

r=me with the comments that preceeded this.
Attachment #8568259 - Flags: review?(MattN+bmo) → review+
Flags: needinfo?(MattN+bmo)
The code is much nicer to read now.
Comment on attachment 8568259 [details]
MozReview Request: bz://1135451/dolske

/r/4211 - Bug 1135451 - fillForm() cleanup part A: remove unused return type, kill E10S unfriendly fillForm from nsILoginManager, kill passwordmgr-found-form notification, largely a backout of bug 439365. r=MattN
/r/4213 - Bug 1135451 - fillForm() cleanup part B: remove _notifyFoundLogins() and passwordmgr-found-logins notifications. Add a new passwordmgr-processed-form for tests. r=MattN
/r/4215 - Bug 1135451 - fillForm() cleanup part C: remove didntFillReason. r=MattN
/r/4217 - Bug 1135451 - fillForm() cleanup part D: Use early returns to un-nest logic and make it more obvious that selectedLogin is always set.  r=MattN
/r/4219 - Bug 1135451 - fillForm() cleanup part E: remove didFillForm. r=MattN
/r/4221 - Bug 1135451 - fillForm() cleanup part F: use early return instead of else blocks. r=MattN
/r/4453 - Bug 1135451 - fillForm() cleanup part G: move passwordmgr-processed-form notification into a finally-block. r=MattN
/r/4455 - Bug 1135451 - fillForm() cleanup part H: rename isFormDisabled to be obvious as to purpose. r=MattN

Pull down these commits:

hg pull review -r a8c3bd91175d40fc322f6bb60f374e4eb5de14d8
Attachment #8568259 - Flags: review+ → review?(MattN+bmo)
From IRC:
- Still going to kill the XPCOM fillForm() API, if new JS callers from upcoming password manager work need something like this it should just be a JS API on LoginManagerParent/Child.
- Added finally block from comment 5, although I just did it as a new changeset to avoid hassles from whitespace changes.
- Fixed comment 8 to make the isFormDisabled purpose by renaming it to isAutocompleteOff and refactoring a bit.
Comment on attachment 8568259 [details]
MozReview Request: bz://1135451/dolske

(Not sure why mozreview felt the need to request review again)
Attachment #8568259 - Flags: review?(MattN+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/c76c6ea1c023
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla39
Attachment #8568259 - Attachment is obsolete: true
Attachment #8619550 - Flags: review+
Attachment #8619551 - Flags: review+
Attachment #8619552 - Flags: review+
Attachment #8619553 - Flags: review+
Attachment #8619554 - Flags: review+
Attachment #8619555 - Flags: review+
Attachment #8619556 - Flags: review+
Attachment #8619557 - Flags: review+
You need to log in before you can comment on or make changes to this bug.