Closed Bug 1417834 Opened 7 years ago Closed 6 years ago

[Form Autofill] Split FormAutofillSection into FormAutofillCreditCardSection and FormAutofillAddressSection

Categories

(Toolkit :: Form Autofill, enhancement, P3)

Product:
Toolkit
Component:
Form Autofill
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: selee, Assigned: selee)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:V2])

Attachments

(2 files)

Bug 1417834 - Part 1: Seperate the fields of Address and Credit Card into the different sections.
59 bytes, text/x-review-board-request
lchang
: review+
ralin
: review+
Details
Bug 1417834 - Part 2: Implement FormAutofillCreditCardSection and FormAutofillAddressSection inhertied from FormAutofillSection.
59 bytes, text/x-review-board-request
lchang
: review+
ralin
: review+
Details 
The current methods of FormAutofillSection need to determine the field type (e.g., Credit Card or Address) first then do the related operations, and the solution would be creating FormAutofillCreditCardSection and FormAutofillAddressSection classes for removing the redundant codes.
Assignee: nobody → selee
Status: NEW → ASSIGNED
Comment on attachment 8937380 [details]
Bug 1417834 - Part 1: Seperate the fields of Address and Credit Card into the different sections.

https://reviewboard.mozilla.org/r/208062/#review215306

Looks good!
Attachment #8937380 - Flags: review?(ralin) → review+
Comment on attachment 8937381 [details]
Bug 1417834 - Part 2: Implement FormAutofillCreditCardSection and FormAutofillAddressSection inhertied from FormAutofillSection.

https://reviewboard.mozilla.org/r/208064/#review215320

Looks great! Thanks. Keep r? I'd like to review again soon later because of the huge changes.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:85
(Diff revision 4)
> -        profile.tel = profile["tel-national"];
> -      }
> -    }
>    }
>  
> -  _matchSelectOptions(profile) {
> +  matchSelectOptions(profile) {

Here and other else remove the prefixed underscore, I guess is because their role are changed from private to protected. However, it's unlikely to tell which is exposed out there to section's consumer at a glance. Not sure it's proper way, but maybe we can introduce more documentation like @private @public @protected above.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:1030
(Diff revision 4)
>        address: [],
>        creditCard: [],
>      };
>  
>      for (const section of this.sections) {
> -      const secRecords = section.createRecords();
> +      const secRecord = section.createRecords();

nit: we may want to rename createRecords to createRecord as well for at most one record would be created per section.
Comment on attachment 8937381 [details]
Bug 1417834 - Part 2: Implement FormAutofillCreditCardSection and FormAutofillAddressSection inhertied from FormAutofillSection.

https://reviewboard.mozilla.org/r/208064/#review215406
Attachment #8937381 - Flags: review?(ralin) → review+
Comment on attachment 8937381 [details]
Bug 1417834 - Part 2: Implement FormAutofillCreditCardSection and FormAutofillAddressSection inhertied from FormAutofillSection.

https://reviewboard.mozilla.org/r/208064/#review215320

> Here and other else remove the prefixed underscore, I guess is because their role are changed from private to protected. However, it's unlikely to tell which is exposed out there to section's consumer at a glance. Not sure it's proper way, but maybe we can introduce more documentation like @private @public @protected above.

Sounds good. @override is another tag worth to be added for indicating the method is overrided from the parent class. We can discuss this more in this bug or another one.

> nit: we may want to rename createRecords to createRecord as well for at most one record would be created per section.

Thanks. It's been updated in the latest patch.
Comment on attachment 8937381 [details]
Bug 1417834 - Part 2: Implement FormAutofillCreditCardSection and FormAutofillAddressSection inhertied from FormAutofillSection.

https://reviewboard.mozilla.org/r/208064/#review215600

::: browser/extensions/formautofill/FormAutofillHandler.jsm:1035
(Diff revision 5)
> -      for (const [type, record] of Object.entries(secRecords)) {
> -        records[type].push(record);
> +      if (!secRecord) {
> +        continue;
> +      }
> +      if (section instanceof FormAutofillAddressSection) {
> +        records.address.push(secRecord);
> +      } else if (section instanceof FormAutofillCreditCardSection) {

The section name enum is removed since `instanceof` is a better solution here.

I would like to add an error or warning log for `else` case here. It can remind us if there is any new or unknown section type which is not handled by `createRecords`.
Comment on attachment 8937380 [details]
Bug 1417834 - Part 1: Seperate the fields of Address and Credit Card into the different sections.

https://reviewboard.mozilla.org/r/208062/#review216532
Attachment #8937380 - Flags: review?(lchang) → review+
Comment on attachment 8937381 [details]
Bug 1417834 - Part 2: Implement FormAutofillCreditCardSection and FormAutofillAddressSection inhertied from FormAutofillSection.

https://reviewboard.mozilla.org/r/208064/#review216534

::: browser/extensions/formautofill/FormAutofillHandler.jsm:146
(Diff revision 6)
> +   * Override this method if the profile is needed to apply some transformers.
> +   *
> +   * @param {Object} profile
> +   *        A profile should be converted based on the specific requirement.
> +   */
> +  applyTransformers(profile) {}

nit: I personally prefer to put all interfaces together so we can easily understand how many interfaces there are to implement when creating a new class. What do you think?

::: browser/extensions/formautofill/FormAutofillHandler.jsm:248
(Diff revision 6)
>     *        A profile to be previewed with
>     */
>    previewFormFields(profile) {
>      log.debug("preview profile: ", profile);
>  
> -    // Always show the decrypted credit card number when Master Password is
> +    this.preparePreviewProfile(profile);

Shouldn't we `await` this function?
Comment on attachment 8937381 [details]
Bug 1417834 - Part 2: Implement FormAutofillCreditCardSection and FormAutofillAddressSection inhertied from FormAutofillSection.

https://reviewboard.mozilla.org/r/208064/#review216534

> nit: I personally prefer to put all interfaces together so we can easily understand how many interfaces there are to implement when creating a new class. What do you think?

`FormAutofillSection` are gathering all override methods together now.
For some methods (`isEnabled`, `isValidSection` and `isRecordCreatable`) which must be overrided, they will throw an TypeError if the parent's method is called.

> Shouldn't we `await` this function?

`preparePreviewProfile` method don't need to be an async function since there is no promise/await in there. So I remove `async` for `preparePreviewProfile`.
Comment on attachment 8937381 [details]
Bug 1417834 - Part 2: Implement FormAutofillCreditCardSection and FormAutofillAddressSection inhertied from FormAutofillSection.

https://reviewboard.mozilla.org/r/208064/#review216566

Thanks.
Attachment #8937381 - Flags: review?(lchang) → review+
Thank you for the review! The patch is rebased based on the newer m-c.
Keywords: checkin-needed
Since there are some patches (bug 1428292) conflicts with this one, I remove Keywords: checkin-needed.
Keywords: checkin-needed
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0c0889c13f53
Part 1: Seperate the fields of Address and Credit Card into the different sections. r=lchang,ralin
https://hg.mozilla.org/integration/autoland/rev/eedac0be8efc
Part 2: Implement FormAutofillCreditCardSection and FormAutofillAddressSection inhertied from FormAutofillSection. r=lchang,ralin
https://hg.mozilla.org/mozilla-central/rev/0c0889c13f53
https://hg.mozilla.org/mozilla-central/rev/eedac0be8efc
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: