1300992 - Fill the autocomplete result with real profile by using profile storage API

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement, P3)

Description

[Matthew N. [:MattN]]

When our autocomplete implementation's search method is called, return results containing autofill profiles.

For this initial bug, don't worry about varying which label to show for the profile. We can just always show the address, for example.

Comment 1

[Steve Chung [:steveck]]

Once we start to search the profile, the result should be returned asynchronously. It means that we should implement a search cancel method and the cancelling results should work correctly even the results are not returned yet.

Comment 2

[Steve Chung [:steveck]]

Hi Matt,
Since this bug might block bug 1300989 because of the scheme of the search result, I think it might help if we could define the interface of the search result and bug 1300989 could keep moving in parallel(Sean will help on this one).

So the idea of the result scheme is simply add a array of id for the FormAutoCompleteResult, and create getIdAt(index) method in FormAutoCompleteResult/AutoCompleteResultView for querying the id to fetch the complete profile. Do you think it's fine to add the additional id property and it is only exist for form fill system addon case?

Comment 3

[Steve Chung [:steveck]]

Attachment: Bug 1300992 - Part 1: Communication between processes,

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

Comment 4

[Steve Chung [:steveck]]

Comment on attachment 8822630 [details]
Bug 1300992 - Part 1: Communication between processes,

Hi Matt,

The first part is about the cross process communication. I think it could be independent from the storage query and filter, so it might be great if you can give any early feedback simply about the communication mechanism.

Comment 5

[Steve Chung [:steveck]]

Attachment: Bug 1300992 - Part 2: Query the focused input's field name for generating the popup result.

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

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8822630 [details]
Bug 1300992 - Part 1: Communication between processes,

https://reviewboard.mozilla.org/r/101508/#review102790

::: browser/extensions/formautofill/FormAutofillParent.jsm:127
(Diff revision 1)
> +   * Populates the field values and notifies content to fill in. Exception will
> +   * be thrown if there's no matching profile.

This comment doesn't seem to match the method name or what is happening.

::: browser/extensions/formautofill/FormAutofillParent.jsm:159
(Diff revision 1)
> +    results.forEach((result, index) => {
> +      data.values[index] = result.value;
> +      data.labels[index] = result.label;
> +      data.comments[index] = result.comment;
> +      data.guids[index] = result.guid;
> +    });
> +    target.messageManager.sendAsyncMessage("FormAutofill:SetPopupResult", data);

I think we should try to keep the nsIAutoCompleteResult details (comments vs. values) away from our code more. As we discussed in this weeks meeting I think it may be better to just return an array of profiles and have the new `FormAutoCompleteResult` replacement that Sean made handle the details of mapping profiles to label/value/comment/etc.

::: browser/extensions/formautofill/content/FormAutofillContent.js:24
(Diff revision 1)
> +const messageManager = content.document.defaultView
> +                                       .QueryInterface(Ci.nsIInterfaceRequestor)
> +                                       .getInterface(Ci.nsIWebNavigation)
> +                                       .QueryInterface(Ci.nsIDocShell)

Are you sure you need this if you're just using `sendAsyncMessage`? I thought it is a global in all frame script scopes and I honestly would have to double-check that you're getting the equivalent message manager here.

::: browser/extensions/formautofill/content/FormAutofillContent.js:260
(Diff revision 1)
> +    messageManager.sendAsyncMessage('FormAutofill:GetPopupResult', data);
> +    return new Promise((resolve) => {
> +      messageManager.addMessageListener('FormAutofill:SetPopupResult', function getResult(result) {
> +        messageManager.removeMessageListener('FormAutofill:SetPopupResult', getResult);

Nit: use double quotes

::: browser/extensions/formautofill/content/FormAutofillContent.js:260
(Diff revision 1)
> +    messageManager.sendAsyncMessage('FormAutofill:GetPopupResult', data);
> +    return new Promise((resolve) => {
> +      messageManager.addMessageListener('FormAutofill:SetPopupResult', function getResult(result) {

Any reason not to move the sendAsyncMessage inside the Promise? When I read this I worry about the case of missing a response message since your message listener only gets added later.

Comment 7

[Steve Chung [:steveck]]

Attachment: Bug 1300992 - Part 2: Return the filtered profile result,

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

Comment 8

[Steve Chung [:steveck]]

Comment on attachment 8822630 [details]
Bug 1300992 - Part 1: Communication between processes,

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

Comment 9

[Steve Chung [:steveck]]

Comment on attachment 8823200 [details]
Bug 1300992 - Part 2: Query the focused input's field name for generating the popup result.

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

Comment 10

[Steve Chung [:steveck]]

Comment on attachment 8822630 [details]
Bug 1300992 - Part 1: Communication between processes,

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

Comment 11

[Steve Chung [:steveck]]

Comment on attachment 8824939 [details]
Bug 1300992 - Part 2: Return the filtered profile result,

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

Comment 12

[Steve Chung [:steveck]]

Comment on attachment 8822630 [details]
Bug 1300992 - Part 1: Communication between processes,

https://reviewboard.mozilla.org/r/101508/#review102790

> Are you sure you need this if you're just using `sendAsyncMessage`? I thought it is a global in all frame script scopes and I honestly would have to double-check that you're getting the equivalent message manager here.

I tried to send the message via child process message manager but I could not receive message from parent. I'm not sure if there's any other way to get the correct one.

Comment 13

[Steve Chung [:steveck]]

Comment on attachment 8822630 [details]
Bug 1300992 - Part 1: Communication between processes,

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

Comment 14

[Steve Chung [:steveck]]

Comment on attachment 8824939 [details]
Bug 1300992 - Part 2: Return the filtered profile result,

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

Comment 15

[Steve Chung [:steveck]]

Comment on attachment 8822630 [details]
Bug 1300992 - Part 1: Communication between processes,

https://reviewboard.mozilla.org/r/101508/#review105640

::: browser/extensions/formautofill/content/FormAutofillContent.js:250
(Diff revision 4)
> -      tel: "1-345-345-3456.",
> -    }, {
> -      guid: "test-guid-2",
> -      organization: "Mozilla",
> -      streetAddress: "331 E. Evelyn Avenue",
> -      tel: "1-650-903-0800",
> +    }).then((profiles) => {
> +      if (this.forceStop) {
> +        return;
> +      }
> +
> +      let result = new ProfileAutoCompleteResult(searchString, name, profiles, {});

Based on the discussion with Sean, we will need to add another parameter like "existingField" array for the result because it'll need this additional information to know the correct secondary column. We will not display the type in secondary column if it's not existed in the form.

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 8824939 [details]
Bug 1300992 - Part 2: Return the filtered profile result,

https://reviewboard.mozilla.org/r/103282/#review106186

::: browser/extensions/formautofill/FormAutofillParent.jsm:134
(Diff revision 3)
> +   * @param  {string} data.guid
> +   *         Indicates which profile to retrieve.

Remove until we need it

::: browser/extensions/formautofill/FormAutofillParent.jsm:144
(Diff revision 3)
> -      organization: "Sesame Street",
> -      streetAddress: "123 Sesame Street.",
> -      tel: "1-345-345-3456",
> -    }, {
> -      guid: "test-guid-2",
> +    } else if (guid) {
> +      profiles = [this._profileStore.get(guid)];
> +    } else {
> +      profiles = this._profileStore.getAll();
> +    }

I think this method should stay focused on autocomplete search results (using searchString and fieldName) and we should have a separate message+method when we need to get profiles other ways.

Comment 17

[Matthew N. [:MattN]]

Comment on attachment 8822630 [details]
Bug 1300992 - Part 1: Communication between processes,

https://reviewboard.mozilla.org/r/101508/#review106180

::: browser/extensions/formautofill/FormAutofillParent.jsm:132
(Diff revision 4)
> +   * @param  {string} data.fieldName
> +   *         The input autocomplete property's fieldName.

I think this should include the whole info object so we can later make decisions based on billing vs. shipping profiles.

::: browser/extensions/formautofill/FormAutofillParent.jsm:137
(Diff revision 4)
> +   * @param  {string} data.fieldName
> +   *         The input autocomplete property's fieldName.
> +   * @param  {nsIFrameMessageManager} target
> +   *         Content's message manager.
> +   */
> +  _getPopupResult({searchString, fieldName}, target) {

How about `_autocompleteSearch` or `_fetchAutocompleteProfiles`?

::: browser/extensions/formautofill/FormAutofillParent.jsm:152
(Diff revision 4)
> +      organization: "Mozilla",
> +      streetAddress: "331 E. Evelyn Avenue",
> +      tel: "1-650-903-0800",
> +    }];
> +
> +    target.messageManager.sendAsyncMessage("FormAutofill:SetPopupResult", profiles);

FormAutofill:AutocompleteResults?

::: browser/extensions/formautofill/content/FormAutofillContent.js:27
(Diff revision 4)
> +const messageManager = content.document.defaultView
> +                                       .QueryInterface(Ci.nsIInterfaceRequestor)
> +                                       .getInterface(Ci.nsIWebNavigation)
> +                                       .QueryInterface(Ci.nsIDocShell)
> +                                       .QueryInterface(Ci.nsIInterfaceRequestor)
> +                                       .getInterface(Ci.nsIContentFrameMessageManager);

It works fine for me using the frame script globals for access to the message manager: https://pastebin.mozilla.org/8965039

I confirmed I got the SetPopupResult reply when sending the message from content.

::: browser/extensions/formautofill/content/FormAutofillContent.js:276
(Diff revision 4)
> +   * @param  {string} data.fieldName
> +   *         The input autocomplete property's fieldName.
> +   * @returns {Promise}
> +   *          Promise that resolves when profiles returned from parent process.
> +   */
> +  getPopupResult(data) {

getAutocompleteResults?

::: browser/extensions/formautofill/content/FormAutofillContent.js:283
(Diff revision 4)
> +      messageManager.addMessageListener("FormAutofill:SetPopupResult", function getResult(result) {
> +        messageManager.removeMessageListener("FormAutofill:SetPopupResult", getResult);
> +        resolve(result.data);
> +      });
> +
> +      messageManager.sendAsyncMessage("FormAutofill:GetPopupResult", data);

FormAutofill:GetAutocompleteResults?

::: browser/extensions/formautofill/content/FormAutofillContent.js:299
(Diff revision 4)
> +    return new Promise((resolve) => {
> +      resolve(FormAutofillHeuristics.getInfo(input).fieldName);
> +    });

Rename this method and return the whole info object as we may need it for billing vs. shipping.

You may even want to just inline this in the one caller.

Comment 18

[Steve Chung [:steveck]]

Comment on attachment 8822630 [details]
Bug 1300992 - Part 1: Communication between processes,

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

Comment 19

[Steve Chung [:steveck]]

Comment on attachment 8824939 [details]
Bug 1300992 - Part 2: Return the filtered profile result,

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

Comment 20

[Steve Chung [:steveck]]

Comment on attachment 8824939 [details]
Bug 1300992 - Part 2: Return the filtered profile result,

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

Comment 21

[Matthew N. [:MattN]]

Comment on attachment 8822630 [details]
Bug 1300992 - Part 1: Communication between processes,

https://reviewboard.mozilla.org/r/101508/#review106582

Thanks

Comment 22

[Matthew N. [:MattN]]

Comment on attachment 8824939 [details]
Bug 1300992 - Part 2: Return the filtered profile result,

https://reviewboard.mozilla.org/r/103282/#review106584

Comment 23

[Steve Chung [:steveck]]

Attachment: Bug 1300992 - Part 3: Cache the form inputs' details for search result,

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

Comment 24

[Matthew N. [:MattN]]

Comment on attachment 8828653 [details]
Bug 1300992 - Part 3: Cache the form inputs' details for search result,

https://reviewboard.mozilla.org/r/106002/#review107338

::: browser/extensions/formautofill/content/FormAutofillContent.js:358
(Diff revision 1)
>          this._identifyAutofillFields(doc);
>          break;
>      }
>    },
>  
> +  getInputDetail(element) {

Nit: add an "s" to the name to match getFormDetails: getInputDetails

::: browser/extensions/formautofill/content/FormAutofillContent.js:380
(Diff revision 1)
> +  _infoClone(detail) {
> +    let info = Object.assign({}, detail);
> +    delete info.element;
> +    return info;

Perhaps `_serializeInfo` would be a better name? If I understand correctly you want a serializable version of the info which means everything except the DOM element reference.

::: browser/extensions/formautofill/test/unit/xpcshell.ini:9
(Diff revision 1)
>  tail =
>  support-files =
>  
>  [test_autofillFormFields.js]
>  [test_collectFormFields.js]
> +[test_getFormInputInfo.js]

Nit: getFormInputDetails.js to match the method names better?

Comment 25

[Steve Chung [:steveck]]

Comment on attachment 8828653 [details]
Bug 1300992 - Part 3: Cache the form inputs' details for search result,

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

Comment 26

[Steve Chung [:steveck]]

Comment on attachment 8828653 [details]
Bug 1300992 - Part 3: Cache the form inputs' details for search result,

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

Comment 27

[Matthew N. [:MattN]]

Comment on attachment 8828653 [details]
Bug 1300992 - Part 3: Cache the form inputs' details for search result,

https://reviewboard.mozilla.org/r/106002/#review108144

Thanks. Looks good, no major issues found.

::: browser/extensions/formautofill/content/FormAutofillContent.js:363
(Diff revisions 2 - 3)
> +  /**
> +   * Get the input's information from cache which is created after page identified.
> +   *
> +   * @returns {Object|null}
> +   *          Return target input's information that cloned from content cache
> +   *          (or retrun null if the information is not found in the cache).

Typo: return

::: browser/extensions/formautofill/content/FormAutofillContent.js:286
(Diff revision 3)
>    getInputInfo() {
> -    let input = formFillController.getFocusedInput();
> +    // TODO: Maybe we'll need to wait for cache ready if detail is empty.
> +    return FormAutofillContent.getInputDetails(formFillController.getFocusedInput());

Nit: rename both getFieldDetails for consistency and so that the name will make sense when we support textarea/select.

::: browser/extensions/formautofill/content/FormAutofillContent.js:297
(Diff revision 3)
> +  getFormInfo() {
> +    // TODO: Maybe we'll need to wait for cache ready if details is empty.
> +    return FormAutofillContent.getFormDetails(formFillController.getFocusedInput());

Nit: Rename to getFormDetails

::: browser/extensions/formautofill/content/FormAutofillContent.js:366
(Diff revision 3)
> +    for (let i in this._formsDetails) {
> +      for (let j in this._formsDetails[i]) {

I think it would be easier to read using for…of since it labels what the data types are:
```
for (let formDetails of this._formsDetails) {
  for (let fieldDetails of formDetails) {
    …
  }
}
```

::: browser/extensions/formautofill/content/FormAutofillContent.js:386
(Diff revision 3)
> +    for (let i in this._formsDetails) {
> +      let formDetails = this._formsDetails[i];

Nit: Use for…of instead since you're not using `i` for anything other than `this._formsDetails[i]`

::: browser/extensions/formautofill/content/FormAutofillContent.js:413
(Diff revision 3)
>      for (let field of doc.getElementsByTagName("input")) {
>        let formLike = FormLikeFactory.createFromField(field);

I just realized that we should be filtering for only text-like fields using mozIsTextField until we support radio and checkbox buttons: https://dxr.mozilla.org/mozilla-central/rev/8ff550409e1d1f8b54f6f7f115545dbef857be0b/dom/interfaces/html/nsIDOMHTMLInputElement.idl#96-100

::: browser/extensions/formautofill/test/unit/test_getFormInputDetails.js:76
(Diff revision 3)
> +    let doc = MockDocument.createTestDocument(
> +      "http://localhost:8080/test/", testcase.document);

Nit: fix indentation e.g. 1st or both arguments after `(` on the same line.

::: browser/extensions/formautofill/test/unit/test_getFormInputDetails.js:83
(Diff revision 3)
> +      Assert.deepEqual(FormAutofillContent.getInputDetails(input),
> +        testcase.expectedResult[i].input,
> +        "Check if returned input information is correct.");
> +
> +      Assert.deepEqual(FormAutofillContent.getFormDetails(input),
> +        testcase.expectedResult[i].form,
> +        "Check if returned form information is correct.");

Nit: fix identation to align arguments

Comment 28

[Steve Chung [:steveck]]

Comment on attachment 8822630 [details]
Bug 1300992 - Part 1: Communication between processes,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/101508/diff/5-6/

Comment 29

[Steve Chung [:steveck]]

Comment on attachment 8824939 [details]
Bug 1300992 - Part 2: Return the filtered profile result,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/103282/diff/5-6/

Comment 30

[Steve Chung [:steveck]]

Comment on attachment 8828653 [details]
Bug 1300992 - Part 3: Cache the form inputs' details for search result,

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

Comment 31

[Steve Chung [:steveck]]

Thanks for all the reviews!

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/633977a7059f
Part 1: Communication between processes, r=MattN
https://hg.mozilla.org/integration/autoland/rev/0a8ed970a0fd
Part 2: Return the filtered profile result, r=MattN
https://hg.mozilla.org/integration/autoland/rev/c2c09d5010ee
Part 3: Cache the form inputs' details for search result, r=MattN

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/633977a7059f
https://hg.mozilla.org/mozilla-central/rev/0a8ed970a0fd
https://hg.mozilla.org/mozilla-central/rev/c2c09d5010ee

Comment 34

[Adrian Florinescu [:aflorinescu]]

As per San-Francisco meeting with :vchen, marking this bug as qe-.