Implement ProfileAutoCompleteResult for Profile Form Fill usage only

RESOLVED FIXED in Firefox 53

Status

()

Toolkit
Form Manager
P3
enhancement
RESOLVED FIXED
4 months ago
a month ago

People

(Reporter: seanlee, Assigned: seanlee)

Tracking

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [form autofill:M1])

MozReview Requests

()

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

Attachments

(1 attachment)

(Assignee)

Description

4 months ago
FormAutoCompleteResult[1] is used to provide the result for Profile AUtoComplete Popup. I would suggest to create ProfileAutoCompletePopup for handling profile case only. Here are some reasons:
1. AFAIK, FormAutoCompleteResult is for form history. An isolated design can avoid the mixed codes of FormHistory and Profile in FormAutoCompleteResult.
2. Reducing the risk of affecting FormHistory feature.
3. Put all Profile relative codes into the same directory.

[1] http://searchfox.org/mozilla-central/rev/51aa673e28802fe6ea89f4793721fc55334a6ac8/toolkit/components/satchel/nsFormAutoCompleteResult.jsm#12
[2] bug 1300989 comment 11
Comment hidden (mozreview-request)
Comment on attachment 8823925 [details]
Bug 1328778 - Implement ProfileAutoCompleteResult for Profile AutoFormFill usage only.;

https://reviewboard.mozilla.org/r/102388/#review102796

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:7
(Diff revision 1)
> +const Ci = Components.interfaces;
> +const Cr = Components.results;

Nit: For new files we normally destructure all of Cu/Cc/Ci/Cr in the same order on one line.

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:10
(Diff revision 1)
> +this.EXPORTED_SYMBOLS = [ "ProfileAutoCompleteResult" ];
> +
> +const Ci = Components.interfaces;
> +const Cr = Components.results;
> +
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");

Use Cu after fixing the above comment

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:13
(Diff revision 1)
> + function ProfileAutoCompleteResult(searchString,
> +                                 searchResult,
> +                                 defaultIndex,

Nit: indentation

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:13
(Diff revision 1)
> + function ProfileAutoCompleteResult(searchString,
> +                                 searchResult,
> +                                 defaultIndex,
> +                                 errorDescription,
> +                                 fieldName,
> +                                 profiles) {

I don't think we should copy the bad API design of FormAutocompleteResult with so many positional arguments. I think we should put the common and most useful arguments first and then have optional arguments afterwards (either positional or in an optional object). Example:
```
function ProfileAutoCompleteResult(searchString, fieldName, matchingProfiles, resultCode = null)
```
or 
```
function ProfileAutoCompleteResult(searchString, fieldName, matchingProfiles, { resultCode = null })
```

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:14
(Diff revision 1)
> +
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +this.ProfileAutoCompleteResult =
> + function ProfileAutoCompleteResult(searchString,
> +                                 searchResult,

`searchResult` sounds like it's the actual results instead of just the status code.

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:21
(Diff revision 1)
> +                                 errorDescription,
> +                                 fieldName,
> +                                 profiles) {
> +  this.searchString = searchString;
> +  this._searchResult = searchResult;
> +  this._defaultIndex = defaultIndex;

Do we have a use for defaultIndex? If not, I would remove it from the constructor until we need it. We should try to make `ProfileAutoCompleteResult` as simple as possible.

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:22
(Diff revision 1)
> +                                 fieldName,
> +                                 profiles) {
> +  this.searchString = searchString;
> +  this._searchResult = searchResult;
> +  this._defaultIndex = defaultIndex;
> +  this._errorDescription = errorDescription;

I also wonder if we will ever use this for anything meaningful.

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:40
(Diff revision 1)
> +  /**
> +   * A reference to the form history nsIAutocompleteResult that we're wrapping.
> +   * We use this to forward removeEntryAt calls as needed.
> +   */
> +  _formHistResult: null,

This is unused

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:46
(Diff revision 1)
> +   * A reference to the form history nsIAutocompleteResult that we're wrapping.
> +   * We use this to forward removeEntryAt calls as needed.
> +   */
> +  _formHistResult: null,
> +
> +  entries: null,

Is this used?

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:60
(Diff revision 1)
> +   *         RESULT_FAILURE   (failure)
> +   *         RESULT_NOMATCH   (no matches found)
> +   *         RESULT_SUCCESS   (matches found)
> +   */
> +  get searchResult() {
> +    return this._searchResult;

I think it may be useful to have this default to sane values if it's not specified in the constructor: 
* RESULT_NOMATCH if profiles.length is 0
* RESULT_SUCCESS if profiles.length > 0

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:95
(Diff revision 1)
> +  /**
> +   * Retrieves a result
> +   * @param  index    the index of the result requested
> +   * @return          the result at the specified index
> +   */
> +  getValueAt: function(index) {

Nit You should use new method syntax in new code. Most of browser/ and toolkit/ we converted in the last week and it's now required with eslint.

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:97
(Diff revision 1)
> +   * @param  index    the index of the result requested
> +   * @return          the result at the specified index
> +   */
> +  getValueAt: function(index) {
> +    this._checkIndexBounds(index);
> +    return this._profiles[index].value;

Since we're going to handle filling of the field ourselves, this should always return ""

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:102
(Diff revision 1)
> +    return this._profiles[index].value;
> +  },
> +
> +  getLabelAt: function(index) {
> +    this._checkIndexBounds(index);
> +    return this._profiles[index].name;

For now have this always return the entire profile object serialized as a string and the front-end will extract the relevant field from it.

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:112
(Diff revision 1)
> +   * @param  index    the index of the comment requested
> +   * @return          the comment at the specified index
> +   */
> +  getCommentAt: function(index) {
> +    this._checkIndexBounds(index);
> +    return this._profiles[index].comment;

I'm not sure if our front-end will use the label or comment but at least one of them should return the whole serialized profile and the unused one can return nothing.

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:122
(Diff revision 1)
> +   * @param  index    the index of the style hint requested
> +   * @return          the style hint at the specified index
> +   */
> +  getStyleAt: function(index) {
> +    this._checkIndexBounds(index);
> +    return "formFill";

Nit: "formFill" is a bit generic and is also the prefix for form history preferences which may be confusing. I also think we may want different styles for address and payment profiles at some point. How about "autofill-profile"?

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:145
(Diff revision 1)
> +   * Removes a result from the resultset
> +   * @param  index    the index of the result to remove
> +   */
> +  removeValueAt: function(index, removeFromDatabase) {
> +    this._checkIndexBounds(index);
> +    this._profiles.splice(index, 1);

This is used to remove from storage so we should ask UX whether we should support deleting a profile using the Delete key on Windows/Linux or Shift+Delete on OS X (like we do for form history and logins). I don't think this should land as-is since it makes it look like we're deleting the row but we're not on the backend. I think either we have those keys do nothing or we have it also remove from storage (which could be done as a follow-up).

::: browser/extensions/formautofill/content/FormAutofillContent.js:232
(Diff revision 1)
> -    let values = ["Mary S.", "John S."];
> -    let comments = ["123 Sesame Street.", "331 E. Evelyn Avenue"];
> -    let result = new FormAutoCompleteResult(searchString,
> +    let profiles = [{
> +      name: "Mary",
> +      value: "Mary S.",
> +      comment: "123 Sesame Street."
> +    }, {
> +      name: "John",
> +      value: "John S.",
> +      comment: "331 E. Evelyn Avenue"
> +    }];
> +    let result = new ProfileAutoCompleteResult(searchString,

Profile objects from storage should be passed in so outside consumers of `ProfileAutoCompleteResult` don't need to know anything about label vs. value vs. comment IMO.
Attachment #8823925 - Flags: review?(MattN+bmo)
(Assignee)

Comment 3

4 months ago
mozreview-review-reply
Comment on attachment 8823925 [details]
Bug 1328778 - Implement ProfileAutoCompleteResult for Profile AutoFormFill usage only.;

https://reviewboard.mozilla.org/r/102388/#review102796

> Since we're going to handle filling of the field ourselves, this should always return ""

For this bug, I would like to provide a very basic AutoCompleteResult implementation only.
If we discuss the filling form implementation in bug 1300989, bug 1300993 won't be blocked. Hence, Steve and I can work on each bug parallelly.
BTW, I am going to provide the proposal for filling form on bug 1300989. 
May I know you thoughts?

> For now have this always return the entire profile object serialized as a string and the front-end will extract the relevant field from it.

the same as above

> I'm not sure if our front-end will use the label or comment but at least one of them should return the whole serialized profile and the unused one can return nothing.

the same as above
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 7

4 months ago
(In reply to Sean Lee [:seanlee][:weilonge] from comment #3)
> If we discuss the filling form implementation in bug 1300989, bug 1300993
> won't be blocked. Hence, Steve and I can work on each bug parallelly.
> BTW, I am going to provide the proposal for filling form on bug 1300989. 
Sorry for the typo. Please replace bug 1300993 with bug 1300992
Comment on attachment 8823925 [details]
Bug 1328778 - Implement ProfileAutoCompleteResult for Profile AutoFormFill usage only.;

https://reviewboard.mozilla.org/r/102388/#review103608

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:9
(Diff revision 4)
> +
> +"use strict";
> +
> +this.EXPORTED_SYMBOLS = ["ProfileAutoCompleteResult"];
> +
> +const {interfaces: Ci, utils: Cu, results: Cr} = Components;

Nit: You're missing `Cc`

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:17
(Diff revision 4)
> +
> +this.ProfileAutoCompleteResult =
> +  function ProfileAutoCompleteResult(searchString,
> +                                     fieldName,
> +                                     matchingProfiles,
> +                                     {searchResultCode = 0}) {

Nit: I think `resultCode` is less redundant than `searchResultCode`. I also think `null` would be better than `0`.

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:55
(Diff revision 4)
> +   *          RESULT_IGNORED   (invalid searchString)
> +   *          RESULT_FAILURE   (failure)
> +   *          RESULT_NOMATCH   (no matches found)
> +   *          RESULT_SUCCESS   (matches found)
> +   */
> +  get searchResult() {

Can you add some quick xpcshell unit tests in /browser/extensions/formautofill/test/unit/ that test `searchResult`, `matchCount`, `defaultIndex`, `getStyleAt`, `getImageAt`?

Don't worry about `getLabelAt`, `getValueAt`, or `getCommentAt` since they may change with bug 1300992.

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:56
(Diff revision 4)
> +    if (this._matchingProfiles.length <= 0) {
> +      return Ci.nsIAutoCompleteResult.RESULT_NOMATCH;
> +    }

Do this in the constructor and set `this._searchResultCode` instead of calculating this at access time? My point of making resultCode optional in the constructor was so that in the common cases of successful searches (with or without matches) that the argument doesn't have to be used at all. You aren't ever defaulting to `RESULT_SUCCESS` though like I had in mind.

I think the following code should be in the constructor (we don't even need the getter either and can use a member variable):
```js
if (resultCode) {
  this.searchResult = resultCode
} else {
  if (matchingProfiles.length > 0) {
    this.searchResult = Ci.nsIAutoCompleteResult.RESULT_SUCCESS;
  } else {
    this.searchResult = Ci.nsIAutoCompleteResult.RESULT_NOMATCH;
  }
}
```

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:135
(Diff revision 4)
> +  removeValueAt(index, removeFromDatabase) {
> +  },

Can you comment inside with a bug number where you ask UX what to do or add a comment with the response from UX if you already know it.

::: browser/extensions/formautofill/content/FormAutofillContent.js:247
(Diff revision 4)
> +    }];
> +    let searchResultCode = Ci.nsIAutoCompleteResult.RESULT_SUCCESS;
> +    let result = new ProfileAutoCompleteResult(searchString,
> +                                               fieldName,
> +                                               profiles,
> +                                               {searchResultCode});

Use the deafult computed value of the result code by not passing it to the constructor for the non-exceptional case.
Attachment #8823925 - Flags: review?(MattN+bmo)
Comment on attachment 8823925 [details]
Bug 1328778 - Implement ProfileAutoCompleteResult for Profile AutoFormFill usage only.;

https://reviewboard.mozilla.org/r/102388/#review102796

> For this bug, I would like to provide a very basic AutoCompleteResult implementation only.
> If we discuss the filling form implementation in bug 1300989, bug 1300993 won't be blocked. Hence, Steve and I can work on each bug parallelly.
> BTW, I am going to provide the proposal for filling form on bug 1300989. 
> May I know you thoughts?

OK, I guess that's fine for now but I'm think we will want `getValueAt` to return an empty string so we can fill ourselves without a flicker of the guid in the field.
Comment hidden (mozreview-request)
(Assignee)

Comment 11

4 months ago
mozreview-review-reply
Comment on attachment 8823925 [details]
Bug 1328778 - Implement ProfileAutoCompleteResult for Profile AutoFormFill usage only.;

https://reviewboard.mozilla.org/r/102388/#review103608

> Can you comment inside with a bug number where you ask UX what to do or add a comment with the response from UX if you already know it.

UX designer said that there is no item removing feature in the spec after having a quick discussion.
(Assignee)

Comment 12

4 months ago
Hi MattN,

The latest patch is with all resolved issues you addressed. I will move the implementation discussion of getValueAt/getLabelAt/getCommentAt to bug 1300992 and bug 1300989. Thanks.
Comment on attachment 8823925 [details]
Bug 1328778 - Implement ProfileAutoCompleteResult for Profile AutoFormFill usage only.;

https://reviewboard.mozilla.org/r/102388/#review103736

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:14
(Diff revision 5)
> +const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +this.ProfileAutoCompleteResult =
> +  function ProfileAutoCompleteResult(searchString,

Nit: Is it necesary to duplicate the function name here for constructors? I find the indentation unusual  especially when looking at line 29. 

I was going to suggest using ES Classes but since I haven't used them myself to know about downsides I didn't suggest it in my past review.

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:43
(Diff revision 5)
> +
> +  // The reason the search failed
> +  errorDescription: "",
> +
> +  // The result code of this result object.
> +  searchResult: 0,

Make this `null` too.

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:51
(Diff revision 5)
> +  get wrappedJSObject() {
> +    return this;
> +  },

Since we're not using this yet I wonder if we should delete it…

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:129
(Diff revision 5)
> +    // Leave this function empty since there is no any item removing feature in
> +    // our plan.

This could be simplied like so:

// There is no plan to support removing profiles via autocomplete.

::: browser/extensions/formautofill/content/FormAutofillContent.js:243
(Diff revision 5)
> +      guid: "test-guid-2",
> +      organization: "Mozilla",
> +      streetAddress: "331 E. Evelyn Avenue",
> +      tel: "1-650-903-0800",
> +    }];
> +    let result = new ProfileAutoCompleteResult(searchString, fieldName, profiles, {});

Nit: remove the last argument: `, {}`

::: browser/extensions/formautofill/test/unit/test_profile_autocomplete_result.js:1
(Diff revision 5)
> +XPCOMUtils.defineLazyModuleGetter(this, "ProfileAutoCompleteResult",
> +                                  "resource://formautofill/ProfileAutoCompleteResult.jsm");

There's no point in making this lazy in tests, just do Cu.import

::: browser/extensions/formautofill/test/unit/test_profile_autocomplete_result.js:16
(Diff revision 5)
> +let expectedResults = [{
> +  options: {},
> +  expectedSearchResult: Ci.nsIAutoCompleteResult.RESULT_SUCCESS,
> +  expectedDefaultIndex: 0,
> +  matchingProfiles: matchingProfiles,
> +  items: [{

I find it hard to read this test since you're defining "expectedResults" which is actually defining the inputs and expected outputs but they are lumped together without a clear distinction. I suppose the main issue is that `items` should be `expectedItems` for clarity. Otherwise I wonder if a nested object under an `expected` key without the prefixes would be easier to read.

::: browser/extensions/formautofill/test/unit/test_profile_autocomplete_result.js:44
(Diff revision 5)
> +  items: [],
> +}];
> +
> +add_task(function* test_all_patterns() {
> +  expectedResults.forEach(pattern => {
> +    let actual = new ProfileAutoCompleteResult("", "",

Can you include `searchString` in your test cases since that's important to test as part of implementing `nsIAutoCompleteResult`

::: browser/extensions/formautofill/test/unit/test_profile_autocomplete_result.js:51
(Diff revision 5)
> +                                               pattern.options);
> +    equal(actual.searchResult, pattern.expectedSearchResult);
> +    equal(actual.defaultIndex, pattern.expectedDefaultIndex);
> +    equal(actual.matchCount, pattern.items.length);
> +    pattern.items.forEach((item, index) => {
> +      // getValueAt, getLabelAt, and getCommentAt should be verified here.

Put a "TODO:" prefix so this gets highlighted in editors.

::: browser/extensions/formautofill/test/unit/xpcshell.ini:11
(Diff revision 5)
>  
>  [test_autofillFormFields.js]
>  [test_collectFormFields.js]
>  [test_populateFieldValues.js]
>  [test_profileStorage.js]
>  [test_markAsAutofillField.js]

Nit: would you mind moving `[test_markAsAutofillField.js]` to the correct alphabetical order while you're here?
Attachment #8823925 - Flags: review?(MattN+bmo) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 15

4 months ago
mozreview-review-reply
Comment on attachment 8823925 [details]
Bug 1328778 - Implement ProfileAutoCompleteResult for Profile AutoFormFill usage only.;

https://reviewboard.mozilla.org/r/102388/#review103736

> Nit: remove the last argument: `, {}`

`TypeError: can't convert undefined to object` is shown when removing this {}, so it is left in this patch.

> Can you include `searchString` in your test cases since that's important to test as part of implementing `nsIAutoCompleteResult`

`fieldName` is included as well in this patch.
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
Keywords: checkin-needed

Comment 17

4 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ff26bfb05b6c
Implement ProfileAutoCompleteResult for Profile AutoFormFill usage only.; r=MattN
Keywords: checkin-needed

Comment 18

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ff26bfb05b6c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Sean, would this fix benefit from manual testing? If yes, then what should manual QA focus on?
Flags: qe-verify?
Flags: needinfo?(selee)
(Assignee)

Comment 20

2 months ago
Hey Andrei,

This patch defines the logic of what label should be shown in the autocomplete popup, and bug 1300989 includes the logic as well.
After the UI component landed, users can use "FormAutoFill" feature to fill a form with a created profile. So QA manual test case will be checking the correctness of the primary (1st item) and secondary (2nd item) [1]. Thanks.

[1] https://mozilla.invisionapp.com/share/TM8GRODVH#/screens/185446489
Flags: needinfo?(selee)
(In reply to Sean Lee [:seanlee][:weilonge] from comment #20)
> Hey Andrei,
> 
> This patch defines the logic of what label should be shown in the
> autocomplete popup, and bug 1300989 includes the logic as well.
> After the UI component landed, users can use "FormAutoFill" feature to fill
> a form with a created profile. So QA manual test case will be checking the
> correctness of the primary (1st item) and secondary (2nd item) [1]. Thanks.
> 
> [1] https://mozilla.invisionapp.com/share/TM8GRODVH#/screens/185446489

Thank you for following up, Sean! This is very helpful. We'll follow up later on with test results.
Flags: qe-verify? → qe-verify+
How can I enable the Form Autofill in beta 53? I've set browser.formautofill.experimental=TRUE but no changes. In nightly 55 the feature is properly displayed in about:preferences#privacy
Flags: needinfo?(selee)
Hi Paul,

Sorry for any confusion. We are not going to release Form Autofill in 53 since other dependencies are still under progress. It should be able to be manually tested in aurora 55 at the earliest.
Flags: needinfo?(selee)
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.