The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 54

Status

()

Toolkit
Form Manager
P3
enhancement
RESOLVED FIXED
7 months ago
2 months ago

People

(Reporter: MattN, Assigned: steveck)

Tracking

(Blocks: 3 bugs)

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [form autofill:M1])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 1 obsolete attachment)

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.
Depends on: 1300993
Blocks: 990187
Whiteboard: [form autofill:M1] → [form autofill:M1][form autofill]
Whiteboard: [form autofill:M1][form autofill] → [form autofill:M1]

Updated

5 months ago
Assignee: nobody → lchang
(Assignee)

Updated

3 months ago
Blocks: 1325538
(Assignee)

Comment 1

3 months ago
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.
(Assignee)

Comment 2

3 months ago
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?
Flags: needinfo?(MattN+bmo)
Comment hidden (mozreview-request)
(Assignee)

Comment 4

3 months ago
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.
Attachment #8822630 - Flags: review?(MattN+bmo) → feedback?(MattN+bmo)
(Assignee)

Updated

3 months ago
Assignee: lchang → schung
Summary: Populate autofill profiles in autocomplete for relevant fields → Fill the autocomplete result with real profile by using profile storage API
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Attachment #8823200 - Flags: feedback?(MattN+bmo)
Depends on: 1328778
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.
Flags: needinfo?(MattN+bmo)
Attachment #8822630 - Flags: feedback?(MattN+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8822630 - Flags: review?(MattN+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8823200 - Attachment is obsolete: true
(Assignee)

Comment 12

2 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 15

2 months ago
mozreview-review
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 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.
Attachment #8824939 - Flags: review?(MattN+bmo)
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.
Attachment #8822630 - Flags: review?(MattN+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8822630 [details]
Bug 1300992 - Part 1: Communication between processes,

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

Thanks
Attachment #8822630 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8824939 [details]
Bug 1300992 - Part 2: Return the filtered profile result,

https://reviewboard.mozilla.org/r/103282/#review106584
Attachment #8824939 - Flags: review?(MattN+bmo) → review+
Comment hidden (mozreview-request)
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
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
Attachment #8828653 - Flags: review?(MattN+bmo) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 31

2 months ago
Thanks for all the reviews!
Keywords: checkin-needed

Comment 32

2 months ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed

Comment 33

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/633977a7059f
https://hg.mozilla.org/mozilla-central/rev/0a8ed970a0fd
https://hg.mozilla.org/mozilla-central/rev/c2c09d5010ee
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.