Simplify form filling code and remove some legacy cruft

RESOLVED FIXED in Firefox 39

Status

()

Toolkit
Password Manager
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Dolske, Assigned: Dolske)

Tracking

unspecified
mozilla39
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(8 attachments, 1 obsolete attachment)

39 bytes, text/x-review-board-request
Dolske
: review+
Details | Review
39 bytes, text/x-review-board-request
Dolske
: review+
Details | Review
39 bytes, text/x-review-board-request
Dolske
: review+
Details | Review
39 bytes, text/x-review-board-request
Dolske
: review+
Details | Review
39 bytes, text/x-review-board-request
Dolske
: review+
Details | Review
39 bytes, text/x-review-board-request
Dolske
: review+
Details | Review
39 bytes, text/x-review-board-request
Dolske
: review+
Details | Review
39 bytes, text/x-review-board-request
Dolske
: review+
Details | Review
(Assignee)

Description

3 years ago
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)
(Assignee)

Comment 1

3 years ago
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
(Assignee)

Comment 3

3 years ago
Created attachment 8568259 [details]
MozReview Request: bz://1135451/dolske

/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/4217/#review3361

Ship It!
https://reviewboard.mozilla.org/r/4219/#review3363

Ship It!
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.
(Assignee)

Comment 11

3 years ago
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)
(Assignee)

Comment 12

3 years ago
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.
(Assignee)

Comment 13

3 years ago
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/integration/fx-team/rev/c76c6ea1c023
https://hg.mozilla.org/integration/fx-team/rev/099a02fe5193
https://hg.mozilla.org/integration/fx-team/rev/01a4a61f9db7
https://hg.mozilla.org/integration/fx-team/rev/7171e15ba2ff
https://hg.mozilla.org/integration/fx-team/rev/d4f4f8cf938b
https://hg.mozilla.org/integration/fx-team/rev/a71d846d255c
https://hg.mozilla.org/integration/fx-team/rev/c918b8a2b9c0
https://hg.mozilla.org/integration/fx-team/rev/a80f93233b34
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/c76c6ea1c023
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla39
https://hg.mozilla.org/mozilla-central/rev/099a02fe5193
https://hg.mozilla.org/mozilla-central/rev/01a4a61f9db7
https://hg.mozilla.org/mozilla-central/rev/7171e15ba2ff
https://hg.mozilla.org/mozilla-central/rev/d4f4f8cf938b
https://hg.mozilla.org/mozilla-central/rev/a71d846d255c
https://hg.mozilla.org/mozilla-central/rev/c918b8a2b9c0
https://hg.mozilla.org/mozilla-central/rev/a80f93233b34
(Assignee)

Comment 17

3 years ago
Comment on attachment 8568259 [details]
MozReview Request: bz://1135451/dolske
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+
(Assignee)

Comment 18

3 years ago
Created attachment 8619550 [details]
MozReview Request: Bug 1135451 - fillForm() cleanup part G: move passwordmgr-processed-form notification into a finally-block. r=MattN
(Assignee)

Comment 19

3 years ago
Created attachment 8619551 [details]
MozReview Request: Bug 1135451 - fillForm() cleanup part H: rename isFormDisabled to be obvious as to purpose. r=MattN
(Assignee)

Comment 20

3 years ago
Created attachment 8619552 [details]
MozReview Request: 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
(Assignee)

Comment 21

3 years ago
Created attachment 8619553 [details]
MozReview Request: Bug 1135451 - fillForm() cleanup part B: remove _notifyFoundLogins() and passwordmgr-found-logins notifications. Add a new passwordmgr-processed-form for tests. r=MattN
(Assignee)

Comment 22

3 years ago
Created attachment 8619554 [details]
MozReview Request: Bug 1135451 - fillForm() cleanup part C: remove didntFillReason. r=MattN
(Assignee)

Comment 23

3 years ago
Created attachment 8619555 [details]
MozReview Request: 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
(Assignee)

Comment 24

3 years ago
Created attachment 8619556 [details]
MozReview Request: Bug 1135451 - fillForm() cleanup part E: remove didFillForm. r=MattN
(Assignee)

Comment 25

3 years ago
Created attachment 8619557 [details]
MozReview Request: Bug 1135451 - fillForm() cleanup part F: use early return instead of else blocks. r=MattN
You need to log in before you can comment on or make changes to this bug.