Closed Bug 1379600 Opened 2 years ago Closed 2 years ago

[Form Autofill] Support credit card record filling and previewing in FormAutofillHandler

Categories

(Toolkit :: Form Manager, enhancement)

56 Branch
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: steveck, Assigned: selee)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:M4][ETA:8/3])

Attachments

(1 file, 1 obsolete file)

After bug 1379533 landed, we should be able to fill the address or credit card record with separated details instead of traversing all details.
Blocks: 1371118
Assignee: nobody → schung
Take the bug from Steve. :P
Assignee: schung → selee
Summary: [Form Autofill] Support credit card record filling in FormAutofillHandler.autofillFormFields → [Form Autofill] Support credit card record filling and previewing in FormAutofillHandler
Whiteboard: [form autofill:M4] → [form autofill:M4][ETA:8/3]
Depends on: 1337314
Depends on: 1388238
No longer depends on: 1337314
Comment on attachment 8890523 [details]
Bug 1379600 - Implement the feature of filling and previewing Credit Card fields.

https://reviewboard.mozilla.org/r/161666/#review173462

Overall looks good except a few comments below.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:158
(Diff revision 5)
> +    let fieldDetail = this.fieldDetails.find(
> +      detail => detail.elementWeakRef.get() == element
> +    );

nit: I prefer not to break lines.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:164
(Diff revision 5)
> +    if (FormAutofillUtils.isCreditCardField(fieldDetail.fieldName)) {
> +      return this.creditCard.fieldDetails;
> +    }

I think we don't need to check the field type twice especially when the implementation of `isAddressField()` is actually `! isCreditCardField()`.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:273
(Diff revision 5)
> +    let focusedDetail = this.fieldDetails.find(
> +      detail => detail.elementWeakRef.get() == focusedInput
> +    );

nit: I prefer not to break lines.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:283
(Diff revision 5)
> +      if (FormAutofillUtils.isCreditCardField(focusedDetail.fieldName)) {
> +        // When Master Password is enabled by users, the decryption process
> +        // should prompt Master Password dialog to get the decrypted credit
> +        // card number. Otherwise, the number can be decrypted with the default
> +        // password.
> +        profile["cc-number"] = await MasterPassword.decrypt(profile["cc-number-encrypted"], MasterPassword.isEnabled());

You can always pass `true` as the second parameter since it will self check.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:285
(Diff revision 5)
> +        if (focusedInput instanceof Ci.nsIDOMHTMLInputElement && !focusedInput.value && value) {
> +          focusedInput.setUserInput(value);
> +          this.changeFieldState(focusedDetail, "AUTO_FILLED");
> +        }

Could you comment on this why we don't fill in `focusedInput` when it's an address field?

::: browser/extensions/formautofill/FormAutofillHandler.jsm:287
(Diff revision 5)
> +        // password.
> +        profile["cc-number"] = await MasterPassword.decrypt(profile["cc-number-encrypted"], MasterPassword.isEnabled());
> +        let value = profile[focusedDetail.fieldName];
> +        if (focusedInput instanceof Ci.nsIDOMHTMLInputElement && !focusedInput.value && value) {
> +          focusedInput.setUserInput(value);
> +          this.changeFieldState(focusedDetail, "AUTO_FILLED");

Can this be removed? (since it will be handled below)

::: browser/extensions/formautofill/FormAutofillHandler.jsm:295
(Diff revision 5)
> +    } catch (e) {
> +      if (e.result == Cr.NS_ERROR_ABORT) {
> +        log.warn("User canceled master password entry");
> +        return;
> +      }
> +      throw e;
> +    }

Looks like this `try...catch` only applies to `MasterPassword.decrypt`. Could you just put it around `decrypt`?
Attachment #8890523 - Flags: review?(lchang)
Duplicate of this bug: 1381781
Comment on attachment 8890523 [details]
Bug 1379600 - Implement the feature of filling and previewing Credit Card fields.

https://reviewboard.mozilla.org/r/161666/#review173462

> nit: I prefer not to break lines.

I would perfer the break-lines because the one line version is too long.

> I think we don't need to check the field type twice especially when the implementation of `isAddressField()` is actually `! isCreditCardField()`.

For the usage of `isCreditCardField()` and `isAddressField()`, I don't think we can assume that non-address is credit card or either way. If `isCreditCardField()` and `isAddressField()` are changed for a new type, we have to go through all the caller of `isCreditCardField()` and `isAddressField()`. So I would perfer to check `isCreditCardField()` again even it's not an address one.
An exception is needed when the field name is non-credit card and non-address.

> nit: I prefer not to break lines.

As above.

> Can this be removed? (since it will be handled below)

Yes! nice catch.
Comment on attachment 8890523 [details]
Bug 1379600 - Implement the feature of filling and previewing Credit Card fields.

https://reviewboard.mozilla.org/r/161666/#review173854

::: browser/extensions/formautofill/FormAutofillHandler.jsm:401
(Diff revision 6)
> -  previewFormFields(profile) {
> +  async previewFormFields(profile, focusedInput) {
>      log.debug("preview profile in autofillFormFields:", profile);
>  
> -    for (let fieldDetail of this.address.fieldDetails) {
> +    // Always show the decrypted credit card number when Master Password is
> +    // disabled.
> +    if (!MasterPassword.isEnabled()) {

nit: Maybe we only need to decrypt if `profile["cc-number-encrypted"]` exsited

::: browser/extensions/formautofill/FormAutofillHandler.jsm:443
(Diff revision 6)
>     * Clear preview text and background highlight of all fields.
>     */
>    clearPreviewedFormFields() {
>      log.debug("clear previewed fields in:", this.form);
>  
> -    for (let fieldDetail of this.address.fieldDetails) {
> +    for (let fieldDetail of this.fieldDetails) {

Do you think it would worth to get the specific details(address or creditcard) for reducing the unnecessary field clearing? Another concern is maybe we might remove the this.fieldDetails in the future and we'll still need to update this part eventually.
Comment on attachment 8890523 [details]
Bug 1379600 - Implement the feature of filling and previewing Credit Card fields.

https://reviewboard.mozilla.org/r/161666/#review175714

::: browser/extensions/formautofill/FormAutofillHandler.jsm:286
(Diff revision 7)
> +      // When Master Password is enabled by users, the decryption process
> +      // should prompt Master Password dialog to get the decrypted credit
> +      // card number. Otherwise, the number can be decrypted with the default
> +      // password.
> +      try {
> +        profile["cc-number"] = await MasterPassword.decrypt(profile["cc-number-encrypted"], true);

As steve mentioned, we should check if `cc-number-encrypted` exists before decrypting it.

::: browser/extensions/formautofill/FormAutofillHandler.jsm
(Diff revision 7)
> -      // Unlike using setUserInput directly, FormFillController dispatches an
> -      // asynchronous "DOMAutoComplete" event with an "input" event follows right
> -      // after. So, we need to suppress the first "input" event fired off from
> -      // focused input to make sure the latter change handler won't be affected
> -      // by auto filling.

Since the flow of address part isn't changed, we should still need this workaround.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:405
(Diff revision 7)
>      log.debug("preview profile in autofillFormFields:", profile);
>  
> -    for (let fieldDetail of this.address.fieldDetails) {
> +    // Always show the decrypted credit card number when Master Password is
> +    // disabled.
> +    if (!MasterPassword.isEnabled()) {
> +      profile["cc-number"] = await MasterPassword.decrypt(profile["cc-number-encrypted"]);

Ditto.
Attachment #8890523 - Flags: review?(lchang)
Comment on attachment 8890523 [details]
Bug 1379600 - Implement the feature of filling and previewing Credit Card fields.

https://reviewboard.mozilla.org/r/161666/#review175714

> As steve mentioned, we should check if `cc-number-encrypted` exists before decrypting it.

Is there any case without profile["cc-number-encrypted"]?

> Since the flow of address part isn't changed, we should still need this workaround.

The latest patch can handle the event correctly without this suppression workaround.
Comment on attachment 8890523 [details]
Bug 1379600 - Implement the feature of filling and previewing Credit Card fields.

https://reviewboard.mozilla.org/r/161666/#review176132

Got it. Thanks.
Attachment #8890523 - Flags: review?(lchang) → review+
Comment on attachment 8890523 [details]
Bug 1379600 - Implement the feature of filling and previewing Credit Card fields.

https://reviewboard.mozilla.org/r/161666/#review175714

> Is there any case without profile["cc-number-encrypted"]?

User might be able to add a credit card profile without number via prefereces at the begining, but right now we'll have the length limitation while saving the profile, which means that every saved credit card must have number. So it's really minor now since it's unlikely happened except the test case.
Comment on attachment 8890523 [details]
Bug 1379600 - Implement the feature of filling and previewing Credit Card fields.

https://reviewboard.mozilla.org/r/161666/#review176148

LGTM, thnaks!
Attachment #8890523 - Flags: review?(schung) → review+
Attachment #8899716 - Attachment is obsolete: true
Attachment #8899716 - Flags: review?(schung)
Attachment #8899716 - Flags: review?(lchang)
Blocks: 1371145
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c58f1fe85895
Implement the feature of filling and previewing Credit Card fields. r=lchang,steveck
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c58f1fe85895
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Blocks: 1393745
You need to log in before you can comment on or make changes to this bug.