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

Status: VERIFIED FIXED

(Toolkit :: Form Manager, enhancement, P3)

Description

[Luke Chang [:lchang] (inactive)]

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.

Comment 1

[Steve Chung [:steveck]]

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.

Comment 2

[Luke Chang [:lchang] (inactive)]

Agree that addressing config and storage status separately in two bugs would be better. Thanks.

Comment 3

[Steve Chung [:steveck]]

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?

Comment 4

[Matthew N. [:MattN]]

(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.

Comment 5

[Steve Chung [:steveck]]

Attachment: Bug 1325724 - Fallback to form history if there is no form autofill profile saved,

Review commit: https://reviewboard.mozilla.org/r/112030/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112030/

Comment 6

[Steve Chung [:steveck]]

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 7

[Matthew N. [:MattN]]

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 8

[Matthew N. [:MattN]]

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");`

Comment 9

[Steve Chung [:steveck]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112030/diff/1-2/

Comment 10

[Steve Chung [:steveck]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112030/diff/2-3/

Comment 11

[Matthew N. [:MattN]]

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

Comment 12

[Steve Chung [:steveck]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112030/diff/3-4/

Comment 13

[Steve Chung [:steveck]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112030/diff/4-5/

Comment 14

[Steve Chung [:steveck]]

Thanks for the review!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d10905c44181
Fallback to form history if there is no form autofill profile saved, r=MattN

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d10905c44181

Comment 17

[Adrian Florinescu [:aflorinescu]]

Verified on 56.0a1 20170725100346:
Ubuntu 16.04, Windows 10, Mac OSX 10.12