Closed Bug 1371131 Opened 7 years ago Closed 7 years ago

[Form Autofill] Extend ProfileAutoCompleteResult to support credit cards

Categories

(Toolkit :: Form Manager, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: lchang, Assigned: ralin)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:M4][ETA:7/26])

Attachments

(4 files)

We need to extend ProfileAutoCompleteResult to return a different style and calculate the secondary label for credit cards.
Assignee: nobody → selee
Status: NEW → ASSIGNED
Discussed with @all offline, steal this bug from Sean :P
Assignee: selee → ralin
Whiteboard: [form autofill:M4] → [form autofill:M4][ETA:7/26]
Similarly to ProfileStorage.jsm, I think converting the original prototype-based constructor to class syntax can bring a better basis of extensible result. The current patch is just a WIP, I'll base on it and continue the work.
Comment on attachment 8886542 [details]
Bug 1371131 - Part 2. Refactor ProfileAutoCompleteResult to using class syntax.

https://reviewboard.mozilla.org/r/157354/#review164424

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:46
(Diff revision 3)
> -    this.searchResult = Ci.nsIAutoCompleteResult.RESULT_SUCCESS;
> +      this.searchResult = Ci.nsIAutoCompleteResult.RESULT_SUCCESS;
> -  } else {
> +    } else {
> -    this.searchResult = Ci.nsIAutoCompleteResult.RESULT_NOMATCH;
> +      this.searchResult = Ci.nsIAutoCompleteResult.RESULT_NOMATCH;
> -  }
> +    }
>  
> +    // An array of primary and secondary labels for each profiles.

nit: ...each profile.
Attachment #8886542 - Flags: review?(schung) → review+
I shouldn't have use `git mv` to rename and edit the file within a commit, I'll split Part 3 into two commits to avoid the irrational changes.
Please use `hg mv` instead of `git mv`. `git mv` will cut the relation of the old one and the new one.
thanks, rebooting hg repo... :P
Comment on attachment 8889286 [details]
Bug 1371131 - Part 4. Hide form autofill footer notes if no warning data in the results.

https://reviewboard.mozilla.org/r/160334/#review165598

::: browser/extensions/formautofill/content/formautofill.xml:273
(Diff revision 1)
> +          if (this.showWarningText) {
> -          messageManager.addMessageListener("FormAutofill:UpdateWarningMessage", this._updateWarningMsgHandler);
> +            messageManager.addMessageListener("FormAutofill:UpdateWarningMessage", this._updateWarningMsgHandler);
> +
> +            this._updateText();
> +          } else {
> +            this._itemBox.setAttribute("noWarning", "true");

not very sure, I guess I should use either @nowarning or @no-warning instead of camel naming to attribute.
Comment on attachment 8888648 [details]
Bug 1371131 - Part 1. Replace the "cc-number-masked" field name in form autofill storage with "cc-number".

https://reviewboard.mozilla.org/r/159674/#review165610
Attachment #8888648 - Flags: review?(lchang) → review+
Comment on attachment 8886542 [details]
Bug 1371131 - Part 2. Refactor ProfileAutoCompleteResult to using class syntax.

https://reviewboard.mozilla.org/r/157354/#review165612
Attachment #8886542 - Flags: review?(lchang) → review+
Comment on attachment 8887354 [details]
Bug 1371131 - Part 3. Create address and credit card result subclasses.

https://reviewboard.mozilla.org/r/158186/#review165628

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:151
(Diff revision 4)
> +
> +class AddressResult extends ProfileAutoCompleteResult {
> +  constructor(...args) {
> +    super(...args);
> +
> +    this._popupLabels.push({

You missed the comment "// Add an empty result entry for footer ...". Please also mention that "categories" and "focusedCategory" are used for "also fill" hint. Thanks.

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:250
(Diff revision 4)
> -    return JSON.stringify(this._popupLabels[index]);
> +    super(...args);
> +
> +    this._popupLabels.push({primary: "", secondary: ""});
>    }
>  
> -  /**
> +  _getSecondaryLabel(focusedFieldName, allFieldNames, profile) {

Looks like most of the code in `_getSecondaryLabel` is the same between `AddressResult` and `CreditCardResult`. Is it possible to have a simple `_adjustSecondaryLabel(currentFieldName, profile)` interface to deal with the different part?

::: browser/extensions/formautofill/test/unit/test_profileAutocompleteResult.js:219
(Diff revision 4)
>      defaultIndex: 0,
>      items: [],
>    },
>  }];
>  
> +matchingProfiles = [{

I prefer not to reuse `matchingProfiles`. How about splitting it into `matchingAddresses` and `matchingCreditCards`.
Attachment #8887354 - Flags: review?(lchang)
Comment on attachment 8889286 [details]
Bug 1371131 - Part 4. Hide form autofill footer notes if no warning data in the results.

https://reviewboard.mozilla.org/r/160334/#review165634

::: browser/extensions/formautofill/content/formautofill.xml:273
(Diff revision 1)
> +          if (this.showWarningText) {
> -          messageManager.addMessageListener("FormAutofill:UpdateWarningMessage", this._updateWarningMsgHandler);
> +            messageManager.addMessageListener("FormAutofill:UpdateWarningMessage", this._updateWarningMsgHandler);
> +
> +            this._updateText();
> +          } else {
> +            this._itemBox.setAttribute("noWarning", "true");

Not sure, either. I prefer "no-warning" though.
Attachment #8889286 - Flags: review?(lchang) → review+
Comment on attachment 8887354 [details]
Bug 1371131 - Part 3. Create address and credit card result subclasses.

https://reviewboard.mozilla.org/r/158186/#review165628

Thanks for the review :-)

> Looks like most of the code in `_getSecondaryLabel` is the same between `AddressResult` and `CreditCardResult`. Is it possible to have a simple `_adjustSecondaryLabel(currentFieldName, profile)` interface to deal with the different part?

I tried to refactor it in several ways, but at last could not find a proper approach. Since if we decided to make both GROUP_FIELDS and secondaryLabelOrder as instance's property, we will need a good point to re-assign value before invoking super() in subclass, but it's apparently not feasible. Others I can think of are in some way making code worse or more complicated, so perhaps we should leave it as-is for now. Thanks.

> I prefer not to reuse `matchingProfiles`. How about splitting it into `matchingAddresses` and `matchingCreditCards`.

discussed with you and decided not to change as object assignment shorthand make code cleaner.
Comment on attachment 8887354 [details]
Bug 1371131 - Part 3. Create address and credit card result subclasses.

https://reviewboard.mozilla.org/r/158186/#review166018

Ok. That's fine with me. Thanks.
Attachment #8887354 - Flags: review?(lchang) → review+
Comment on attachment 8889286 [details]
Bug 1371131 - Part 4. Hide form autofill footer notes if no warning data in the results.

https://reviewboard.mozilla.org/r/160334/#review166022

LGTM!

::: browser/extensions/formautofill/content/formautofill.xml:173
(Diff revision 3)
>            this._warningTextBox = document.getAnonymousElementByAttribute(
>              this, "anonid", "autofill-warning"
>            );
>  
>            let {categories: allFieldCategories, focusedCategory} = JSON.parse(this.getAttribute("ac-value"));
> +          this.showWarningText = allFieldCategories && focusedCategory;

Don't you consider to add these two lines like you did in `_adjustAcItem`?
```JS
this._allFieldCategories = value.categories;
this._focusedCategory = value.focusedCategory;

```

::: browser/extensions/formautofill/skin/shared/autocomplete-item.css:109
(Diff revision 3)
>  .autofill-footer > .autofill-option-button {
>    height: 41px;
>    background-color: #EDEDED;
>  }
> +
> +.autofill-footer[no-warning="true"] > .autofill-warning {

Just curious. Is there any reason to use `attribute` instead of `class`?
Attachment #8889286 - Flags: review?(selee) → review+
Comment on attachment 8889286 [details]
Bug 1371131 - Part 4. Hide form autofill footer notes if no warning data in the results.

https://reviewboard.mozilla.org/r/160334/#review166022

> Don't you consider to add these two lines like you did in `_adjustAcItem`?
> ```JS
> this._allFieldCategories = value.categories;
> this._focusedCategory = value.focusedCategory;
> 
> ```

Thanks for the spot, I've fixed this.

> Just curious. Is there any reason to use `attribute` instead of `class`?

I think both are fine. My idea is since we've already done something similar for layouts(1-line and 2-lines), perhaps, be consist with that would be better: changing appearance with different attribute value based on the pre-determined class sets.
Comment on attachment 8887354 [details]
Bug 1371131 - Part 3. Create address and credit card result subclasses.

https://reviewboard.mozilla.org/r/158186/#review166066

::: browser/extensions/formautofill/test/unit/test_profileAutocompleteResult.js:246
(Diff revision 5)
> +  "cc-exp-month",
> +  "cc-exp-year",
> +];
> +
> +let creditCardTestCases = [{
> +  description: "Focus on an `cc-name` field",

nit: a `cc-name` field?

::: browser/extensions/formautofill/test/unit/test_profileAutocompleteResult.js:276
(Diff revision 5)
> +      }),
> +      image: "",
> +    }],
> +  },
> +}, {
> +  description: "Focus on an `cc-number` field",

nit: a `cc-number` field?
Attachment #8887354 - Flags: review?(schung) → review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/05dde481973c
Part 1. Replace the "cc-number-masked" field name in form autofill storage with "cc-number". r=lchang
https://hg.mozilla.org/integration/autoland/rev/6efa0b5aaabe
Part 2. Refactor ProfileAutoCompleteResult to using class syntax. r=lchang,steveck
https://hg.mozilla.org/integration/autoland/rev/1f85c1bfd4bd
Part 3. Create address and credit card result subclasses. r=lchang,steveck
https://hg.mozilla.org/integration/autoland/rev/91fd8e1b78e3
Part 4. Hide form autofill footer notes if no warning data in the results. r=lchang,seanlee
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.