1380279 - [Form Autofill] Add the ability for "startSearch" to retrieve credit card profiles from storage

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement, P3)

Description

[Ray Lin[:ralin]]

We need to a way rather than _getAddresses to retrieve records from credit cards storage in "startSearch" in order to provide corresponding results for showing popup.

Comment 1

[Ray Lin[:ralin]]

Maybe we could consider creating a new method: _getCreditCards() or refactoring the original _getAddresses() to a more generic one like: _getRecords() in FormAutofillContent.jsm.

Comment 3

[Ray Lin[:ralin]]

Attachment: Bug 1380279 - Add an ability to retrieve multiple collection type in startSearch to support credit cards autofilling.

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

Comment 4

[Ray Lin[:ralin]]

Comment on attachment 8886115 [details]
Bug 1380279 - Add an ability to retrieve multiple collection type in startSearch to support credit cards autofilling.

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

Comment 5

[Ray Lin[:ralin]]

Add a demo page for credit card: https://luke-chang.github.io/autofill-demo/basic_cc.html

We can use this page to verify if the results are actually retrieved from "creditCards" when focus on credit card fields.

Comment 6

[Ray Lin[:ralin]]

Attachment: autofill-profiles.json

We'll need a storage with credit cards profiles as well.

Comment 7

[Ray Lin[:ralin]]

Attachment: autofill-profiles.json

update autofill-profiles.json to conform with storage schema.

Comment 8

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8886115 [details]
Bug 1380279 - Add an ability to retrieve multiple collection type in startSearch to support credit cards autofilling.

https://reviewboard.mozilla.org/r/156914/#review163464

It looks good.
I have only concern that there is no specific xpcshell-test or others to verify the behavior of retrieving credit card records. e.g. `FormAutofillContent._getRecords({collectionName: "creditCards"})`
May I know how do you verify it while implementing? Could you find a proper place to add the test? Thanks.

::: browser/extensions/formautofill/FormAutofillContent.jsm:166
(Diff revision 2)
> -      Services.cpmm.addMessageListener("FormAutofill:Addresses", function getResult(result) {
> -        Services.cpmm.removeMessageListener("FormAutofill:Addresses", getResult);
> +      Services.cpmm.addMessageListener("FormAutofill:Records", function getResult(result) {
> +        Services.cpmm.removeMessageListener("FormAutofill:Records", getResult);
>          resolve(result.data);
>        });
>  
> -      Services.cpmm.sendAsyncMessage("FormAutofill:GetAddresses", data);
> +      Services.cpmm.sendAsyncMessage("FormAutofill:GetRecords", data);

Use `JSON.stringify(data)` to prevent the warning message in console.

::: browser/extensions/formautofill/FormAutofillParent.jsm:243
(Diff revision 2)
>     * @param  {nsIFrameMessageManager} target
>     *         Content's message manager.
>     */
> -  _getAddresses({searchString, info}, target) {
> -    let addresses = [];
> +  _getRecords({collectionName, searchString, info}, target) {
> +    let records = [];
> +    let collection = this.profileStorage[collectionName];

`collection` should be checked if it's `null`.
Use an empty array as the result e.g. `target.sendAsyncMessage("FormAutofill:Records", []);`.
The warning message is necessary here to record the incorrect `collectionName` access here.

::: browser/extensions/formautofill/content/manageProfiles.js:82
(Diff revision 2)
> +   *
> +   * @param  {string} data.collectionName
>     *
>     * @returns {promise}
>     */
> -  getAddresses() {
> +  getRecords(data) {

The comment only mentioned `data.collectionName`, but `data.info` and `data.searchString` do affect the results as well.
I think there are two ways to improve it here:
1. `getRecords` only allows `collectionName`.
e.g. `getRecords(collectionName)`
2. Update the comment to describe that it will pass the data object to `"FormAutofill:GetRecords" message` as well.
```* @param {Object} the parameter for "FormAutofill:GetRecords" message.
```

Comment 9

[Ray Lin[:ralin]]

Comment on attachment 8886115 [details]
Bug 1380279 - Add an ability to retrieve multiple collection type in startSearch to support credit cards autofilling.

https://reviewboard.mozilla.org/r/156914/#review163464

Thanks for the review :D

I'll fix the patch soon. For the lacking xpcshell-test, it seems reasonable to me to crib from the original test_addressRecords.js and maybe create test_creditCardsRecords.js doing something similarly.

> Use `JSON.stringify(data)` to prevent the warning message in console.

Gonna check this out, IIRC, passing pure object won't cause warning message unless the data contains DOM object?

> `collection` should be checked if it's `null`.
> Use an empty array as the result e.g. `target.sendAsyncMessage("FormAutofill:Records", []);`.
> The warning message is necessary here to record the incorrect `collectionName` access here.

Can we just refactor the if-statement instead, like:
```javascript
  let records;
  let collection = this.profileStorage[collectionName];
  
  if (!collection) {
    records = [];
  } else if(info && info.fieldName) {
    records = collection.getByFilter({searchString, info});
  } else {
    records = collection.getAll();
  }
  target.sendAsyncMessage("FormAutofill:Records", records);
    
```
would this be conciser than having another `target.sendAsyncMessage("FormAutofill:Records", []);`?

> The comment only mentioned `data.collectionName`, but `data.info` and `data.searchString` do affect the results as well.
> I think there are two ways to improve it here:
> 1. `getRecords` only allows `collectionName`.
> e.g. `getRecords(collectionName)`
> 2. Update the comment to describe that it will pass the data object to `"FormAutofill:GetRecords" message` as well.
> ```* @param {Object} the parameter for "FormAutofill:GetRecords" message.
> ```

Good point, I'd prefer approach 2 in case we need the rest of options in the future.

Comment 10

[Ray Lin[:ralin]]

(In reply to Ray Lin[:ralin] from comment #9)

> I'll fix the patch soon. For the lacking xpcshell-test, it seems reasonable
> to me to crib from the original test_addressRecords.js and maybe create
> test_creditCardsRecords.js doing something similarly.
I found this is wrong, the test_creditCardsRecords.js is already existing and irrelevant to this patch. I'll find the place to test for _getRecords, though I didn't find any testing _getAddresses() prior to this changes.

Comment 11

[Ray Lin[:ralin]]

Comment on attachment 8886115 [details]
Bug 1380279 - Add an ability to retrieve multiple collection type in startSearch to support credit cards autofilling.

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

Comment 12

[Ray Lin[:ralin]]

A unit test for parant's _getRecords was added in this revision. It's basically verify whether Parent grab the correct collection, and make sure the getAll be called in the storage. The rest of operations in Content or Storage are already covered in test_addressRecords.js/test_creditCardRecords.js, so I don't tend to duplicate those here. Thanks.

Comment 13

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8886115 [details]
Bug 1380279 - Add an ability to retrieve multiple collection type in startSearch to support credit cards autofilling.

https://reviewboard.mozilla.org/r/156914/#review164494

LGTM! Thank you!

Comment 14

[Ray Lin[:ralin]]

Thank you Sean,

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58d48d15f9dc804fbc735fe9f88ed9be4bd32497

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Autoland can't push this because it doesn't meet the review requirements in MozReview.
http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/autoland.html#landing-commits

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 8886115 [details]
Bug 1380279 - Add an ability to retrieve multiple collection type in startSearch to support credit cards autofilling.

https://reviewboard.mozilla.org/r/156914/#review164738

re=me after Sean's review.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a15a4779cbde
Add an ability to retrieve multiple collection type in startSearch to support credit cards autofilling. r=MattN,seanlee

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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