Fallback to form history if there is no form autofill profile saved

VERIFIED FIXED in Firefox 56

Status

()

Toolkit
Form Manager
P3
enhancement
VERIFIED FIXED
8 months ago
22 days ago

People

(Reporter: lchang, Assigned: steveck)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 disabled, firefox55 disabled, firefox56 verified)

Details

(Whiteboard: [form autofill:M1])

MozReview Requests

()

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

Attachments

(1 attachment)

(Reporter)

Description

8 months ago
A form history list should be prompted if there is no profile saved. We may listen to profile changing event and then register/unregister the form autofill searching module accordingly.
(Assignee)

Comment 1

7 months ago
Just a quick recap for this issue:
- We might need to resume the original config "browser.autofill.enabled" flag, and there could be 4 key points that need to fall back:
- 1) Check "browser.autofill.enabled" in init and reg/unreg.
- 2) Check storage status in init and reg/unreg.
- 3) Add the config listener in init and reg/unreg.
- 4) Add storage listener in init and reg/unreg.

I think (1) and (3) is accessible right now, and (2) might be blocked by bug 1300992 since it will provide an API to get all the storage. Hi Luke, do we have a bug for storage listener? Maybe we and split (2) and (4) into another bug and start (1) and (3) right in this bug.
Flags: needinfo?(luke)
(Assignee)

Updated

7 months ago
Flags: needinfo?(luke) → needinfo?(lchang)
(Reporter)

Comment 2

7 months ago
Agree that addressing config and storage status separately in two bugs would be better. Thanks.
Flags: needinfo?(lchang)
(Assignee)

Comment 3

7 months ago
Except for the empty profile fall back, there is another case for single input that we might need to unmark the input if profiles doesn't have specific type for the focused input(For exemple, user is typing in the "company" field but there's no company data in the existing profiles). Although we could avoid it with checking all profile data before calling markAs, it's still possible that we need to clear the marked input due any profile change.

Hi Matt, do you think it's fine to add another API like "unmarkFormFill" as the fall back mechanism for marked input, and, is it fine to have both ensureRegistered/ensureUnregistered and mark/unmark for different purpose?
Flags: needinfo?(MattN+bmo)
(In reply to Steve Chung [:steveck] from comment #3)
> Hi Matt, do you think it's fine to add another API like "unmarkFormFill" as
> the fall back mechanism for marked input, and, is it fine to have both
> ensureRegistered/ensureUnregistered and mark/unmark for different purpose?

Yes I think it's fine to add an unmark API but for this bug I think it's sufficient to listen for the storage change and call ensureUnregistered if storage becomes empty. I think you can probably add the storage observer in this bug now that bug 1330567 was separated.
Flags: needinfo?(MattN+bmo)
See Also: → bug 1330567
(Reporter)

Updated

6 months ago
Assignee: nobody → schung
(Assignee)

Updated

6 months ago
Blocks: 1338485
Comment hidden (mozreview-request)
(Assignee)

Updated

6 months ago
Attachment #8836647 - Flags: feedback?(MattN+bmo)
(Assignee)

Comment 6

6 months ago
mozreview-review
Comment on attachment 8836647 [details]
Bug 1325724 - Fallback to form history if there is no form autofill profile saved,

https://reviewboard.mozilla.org/r/112030/#review113252

::: browser/extensions/formautofill/FormAutofillParent.jsm:159
(Diff revision 1)
>        case "FormAutofill:getEnabledStatus":
>          target.messageManager.sendAsyncMessage("FormAutofill:enabledStatus",
>                                                 this._enabled);
>          break;
> +      case "FormAutofill:ProfileUpdated":
> +        let currentStatus = this._getStatus();

It seems like we came move this dupe code into one function, but it'll only be used twice...

::: browser/extensions/formautofill/ProfileStorage.jsm:127
(Diff revision 1)
>      profileToSave.timesUsed = 0;
>  
>      this._store.data.profiles.push(profileToSave);
>  
>      this._store.saveSoon();
> +    Services.mm.sendAsyncMessage("FormAutofill:ProfileUpdated");

Not sure if the "ProfileUpdated" is a fine naming because I guess store is not actually changed at this moment.
Comment on attachment 8836647 [details]
Bug 1325724 - Fallback to form history if there is no form autofill profile saved,

https://reviewboard.mozilla.org/r/112030/#review113252

> Not sure if the "ProfileUpdated" is a fine naming because I guess store is not actually changed at this moment.

Will we use the same message for payment profiles in the future? How about FormAutofill:StorageChanged
Comment on attachment 8836647 [details]
Bug 1325724 - Fallback to form history if there is no form autofill profile saved,

https://reviewboard.mozilla.org/r/112030/#review113580

Please add xpcshell tests using `TestUtils.topicObserved("formautofill-storage-changed", …)`.

::: browser/extensions/formautofill/FormAutofillParent.jsm:136
(Diff revision 1)
>    _getStatus() {
> -    return Services.prefs.getBoolPref(ENABLED_PREF);
> +    if (!Services.prefs.getBoolPref(ENABLED_PREF)) {
> +      return false;
> +    }
> +
> +    return this._profileStore.getAll().length > 0 ? true: false;

Nit: this is the same as:
`return this._profileStore.getAll().length > 0;`

::: browser/extensions/formautofill/ProfileStorage.jsm:127
(Diff revision 1)
>      profileToSave.timesUsed = 0;
>  
>      this._store.data.profiles.push(profileToSave);
>  
>      this._store.saveSoon();
> +    Services.mm.sendAsyncMessage("FormAutofill:ProfileUpdated");

Since the consumers are both in the same process, an observer notification is usually used e.g. passwordmgr-storage-changed and satchel-storage-changed. See https://dxr.mozilla.org/mozilla-central/search?q=storage-changed . I think it makes it more clear that this isn't a message being sent to another process. I also think we should indicate the reason for the change like pwmgr does so we can do proper tests of the notification:
`Services.obs.notifyObserver(null, "formautofill-storage-changed", "add");`
Attachment #8836647 - Flags: feedback?(MattN+bmo) → feedback+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8836647 [details]
Bug 1325724 - Fallback to form history if there is no form autofill profile saved,

https://reviewboard.mozilla.org/r/112030/#review114096

::: browser/extensions/formautofill/ProfileStorage.jsm:127
(Diff revision 3)
>      profileToSave.timesUsed = 0;
>  
>      this._store.data.profiles.push(profileToSave);
>  
>      this._store.saveSoon();
> +    Services.obs.notifyObservers(null, "formautofill-storage-changed", "add");

Please use `TestUtils.topicObserved("formautofill-storage-changed", …)` to test that this gets fired for storage changes e.g. in test_profileStorage
Attachment #8836647 - Flags: review?(MattN+bmo) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 14

6 months ago
Thanks for the review!
Status: NEW → ASSIGNED
Keywords: checkin-needed

Comment 15

6 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d10905c44181
Fallback to form history if there is no form autofill profile saved, r=MattN
Keywords: checkin-needed

Comment 16

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d10905c44181
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Verified on 56.0a1 20170725100346:
Ubuntu 16.04, Windows 10, Mac OSX 10.12
Status: RESOLVED → VERIFIED
status-firefox54: fixed → disabled
status-firefox55: --- → disabled
status-firefox56: --- → verified
(Reporter)

Updated

22 days ago
See Also: → bug 1378668
You need to log in before you can comment on or make changes to this bug.