1390757 - [Form Autofill] Enable credit card feature according to "extensions.formautofill.creditCards.enabled" pref

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement, P3)

Description

[Ray Lin[:ralin]]

FAP should respect this pref and adjust its support for credit card fields.

Comment 1

[Ray Lin[:ralin]]

Attachment: Bug 1390757 - Introduce pref extensions.formautofill.creditCards.enabled to form autofill activation process.

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

Comment 2

[Ray Lin[:ralin]]

I was trying to figure out how the initialization could differ from the original if we have to consider two pref now. Do we need to update autofillSavedFieldNames as well? If the autofillSavedFieldNames size has to account for the activeStatus, autofillSavedFieldNames should be collected from the enabled collections instead of all.
e.g. when only "extensions.formautofill.creditCards.enabled" is on with only address records in storage, we shouldn't see autofill active.

Comment 3

[Luke Chang [:lchang] (inactive)]

Yes, it makes sense. "autofillSavedFieldNames" is only updated when profile is changed. Maybe we can record the saved field names of addresses and credit cards separately so we don't need to update it when only pref is changed.

BTW, are you going to update "startSearch" in the same commit or in a new one?

Comment 4

[Ray Lin[:ralin]]

> Maybe we can record the saved field names of addresses and credit cards separately so we don't need to update it when only pref is changed.
Seems like a small change, I'll do in the same commit including "startSearch" part later. Thanks.

Comment 5

[Ray Lin[:ralin]]

Comment on attachment 8900217 [details]
Bug 1390757 - Introduce pref extensions.formautofill.creditCards.enabled to form autofill activation process.

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

Comment 6

[Ray Lin[:ralin]]

Some TODOs:
1. Don't save autofillSavedFieldNames as a complex object, instead, separate into two properties of initialProcessData like autofillSavedAddressesFieldNames and autofillSavedCreditCardsFieldNames, it could have been more clearer. (will discuss with Luke in person)
2. Add test cases in test_activeStatus.js for different pref sets.
3. Update test_savedFieldNames.js due to the change of its structure.
4. Smarter fallback to form history according to pref. (maybe an extra mochitest is required for this)

Comment 7

[Ray Lin[:ralin]]

Comment on attachment 8900217 [details]
Bug 1390757 - Introduce pref extensions.formautofill.creditCards.enabled to form autofill activation process.

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

Comment 8

[Ray Lin[:ralin]]

Comment on attachment 8900217 [details]
Bug 1390757 - Introduce pref extensions.formautofill.creditCards.enabled to form autofill activation process.

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

Comment 9

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8900217 [details]
Bug 1390757 - Introduce pref extensions.formautofill.creditCards.enabled to form autofill activation process.

https://reviewboard.mozilla.org/r/171582/#review177326

Good job! Thanks.

Comment 10

[Luke Chang [:lchang] (inactive)]

(In reply to Ray Lin[:ralin] from comment #6)
> 1. Don't save autofillSavedFieldNames as a complex object, instead, separate
> into two properties of initialProcessData like
> autofillSavedAddressesFieldNames and autofillSavedCreditCardsFieldNames, it
> could have been more clearer. (will discuss with Luke in person)

Let's file a bug for following up this improvement.

Comment 11

[Ray Lin[:ralin]]

Thanks for your rapid review :D

I'm waiting for the try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8fe1c4f3bc9356c0582af1a67c93b26ada584e5
will mark checkin-needed once I can confirm the result is good to go.

(In reply to Luke Chang [:lchang] from comment #10)
> (In reply to Ray Lin[:ralin] from comment #6)
> > 1. Don't save autofillSavedFieldNames as a complex object, instead, separate
> > into two properties of initialProcessData like
> > autofillSavedAddressesFieldNames and autofillSavedCreditCardsFieldNames, it
> > could have been more clearer. (will discuss with Luke in person)
> 
> Let's file a bug for following up this improvement.
I'll file a bug for that.

Comment 12

[Ray Lin[:ralin]]

Thanks

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8fa30f1a3f4d
Introduce pref extensions.formautofill.creditCards.enabled to form autofill activation process. r=lchang

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8fa30f1a3f4d

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added to Fx58 release notes.

Comment 16

[Matthew N. [:MattN]]

This bug added the ability to control the feature being enabled or not but doesn't control making it available to users. We're not shipping credit card autofill in 58 because we want to record some telemetry first.

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Matt, this relnote was added only to the Beta58 release notes and not the 58 "release version" notes. Please let me know if these to be taken off of the beta58 release notes as well. Thanks!