Closed Bug 1349490 Opened 7 years ago Closed 7 years ago

Implement the first version of heuristic algorithm.

Categories

(Toolkit :: Form Manager, enhancement, P1)

Product:
Toolkit
Component:
Form Manager
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: selee, Assigned: selee)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [form autofill:M2])

Attachments

(4 files)

Bug 1349490 - Part 1: Implement FormAutofillUtil.extractLabelStrings function.;
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1349490 - Part 2: Use a set of regexp to recognize the input autofill type.;
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1349490 - Part 3: Remove unused information in test fixtures.;
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1349490 - Part 4: Add "extensions.formautofill.heuristics.enabled" for toggling form autofill heuristics.;
59 bytes, text/x-review-board-request
MattN
: review+
Details 
The bug contains the very basic logic (most of them are regexp) to apply the prediction result for the following fields:
Email
Phone
Name
Address

The detail tweaks will be implemented in each own bug.
Whiteboard: [form autofill:M2]
Based on bug 1333351 comment 9, the latest patch includes all of the regexp what we need in M2.
Some issues mentioned in bug 1347176 should be fixed:
1. Add unit tests for multi-byte characters. Probably we should use "u" instead of "i" in each regexp.
2. Add the comment to describe the regexp is from Chromium and we will improve that in the other bugs.

For the current state of the heuristic algorithm, there are some cases we should fix in each field type bugs. These bugs are sorted by the number of failed cases in the current test:
* [Bug 1349495][parallel] Address field
* [Bug 1349494][parallel] Name field
* [Bug 1349493][parallel] Phone field
* [Bug 1349492][parallel] Email field

I take some notes here to fix these issues later:
* The most of "state" (address-leve1) field is in "select" element, but we are lack of "select" support currently.
* AFAIK, we don't support tel-ext field in M2, so its effort is less than I expected.
* There (Walmart) are some cases rely on "title", "name" attribute of an input. I will consider this case in the final patch.
* Need to finish the middle name support in FormAutofill modules and ProfileStorage to fill fields.
Assignee: nobody → selee
Status: NEW → ASSIGNED
Hey MattN,

This patch takes care most of field prediction except some cases, e.g. address-line2, select/country element, autocomplete="off", etc.

Some notes:
1. Since there is no autocomplete type for middle-name initial, I still use "additional-name" for it.
2. autocomplete="off" is still in the pattern but comment.

I will leave the following cases fixing in a separate bug:
1. address-line2 - some of them can be fixed after modifying the original regexp.
2. address-level1 (state) - most of them are select element
3. country - most of them are select element
4. telephone fields
5. credit card relative fields

for item 1, I would like to fix it in another commit of this bug.
Priority: -- → P1
Comment on attachment 8856892 [details]
Bug 1349490 - Part 1: Implement FormAutofillUtil.extractLabelStrings function.;

https://reviewboard.mozilla.org/r/128810/#review135106

I still need to look closer at some parts but I think there are enough significant issues here for now.

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:47
(Diff revision 9)
> +    "email": new RegExp(
> +      "e.?mail" +
> +      "|courriel" +                                 // fr

Please include permalinks to where the regexes are from and note any differences from the source.

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:51
(Diff revision 9)
> +    // ==== Email ====
> +    "email": new RegExp(
> +      "e.?mail" +
> +      "|courriel" +                                 // fr
> +      "|メールアドレス" +                           // ja-JP
> +      "|Электронной.?Почты" +                       // ru

Are you sure you're not going to hit the issue with JSMs and latin-1 that Luke hit?

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:55
(Diff revision 9)
> +      "|メールアドレス" +                           // ja-JP
> +      "|Электронной.?Почты" +                       // ru
> +      "|邮件|邮箱" +                                // zh-CN
> +      "|電郵地址" +                                 // zh-TW
> +      "|(?:이메일|전자.?우편|[Ee]-?mail)(.?주소)?", // ko-KR
> +      "i"

I asked in the other bug if you need the "u" modifier?

Can you verify that my other existing comments in the original bug are addressed?

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:72
(Diff revision 9)
> +      "|电话" +                                      // zh-CN
> +      "|(?:전화|핸드폰|휴대폰|휴대전화)(?:.?번호)?", // ko-KR
> +      "i"
> +    ),
> +
> +    // ==== Address Relative Fields ====

Nit: I think you mean "Address-Related Fields" but "Address Fields" is sufficient.

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:146
(Diff revision 9)
> +      // TODO: JS does not support backward matching, and we should apply
> +      // this pattern in JS rather than regexp.
> +      // "(?<!united )state|county|region|province"
> +      "state|county|region|province" +

Please file a bug for this and mark it either MVP or M2 then reference the bug number here so we don't forget.

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:220
(Diff revision 9)
> +    ),
> +  },
>  
>    getFormInfo(form) {
>      let fieldDetails = [];
> +    if (form.rootElement.autocomplete == "off") {

I think this should be just `form.autocomplete` as FormLike's have an autocomplete property.

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:227
(Diff revision 9)
> +    }
>      for (let element of form.elements) {
>        // Exclude elements to which no autocomplete field has been assigned.
> -      let info = this.getInfo(element);
> +      let info = this.getInfo(element, fieldDetails);
>        if (!info) {
> +        log.debug("No info found", info, element);

This might be a bit too spammy?

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:255
(Diff revision 9)
> -  getInfo(element) {
> -    if (!(element instanceof Ci.nsIDOMHTMLInputElement)) {
> +  getInfo(element, fieldDetails) {
> +    if (!(element instanceof Ci.nsIDOMHTMLInputElement) ||
> +        element.type == "hidden" ||
> +        element.type == "submit" ||
> +        element.type == "checkbox" ||
> +        element.type == "radio") {
>        return null;

This should ideally be its own commit/bug with tests. You forgot "button" and I think it would be safer to use an allow list instead of a block list.

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:281
(Diff revision 9)
> +      if (this.VALID_LABELS["email"].test(string)) {
> +        result.fieldName = "email";
> +        return result;
> +      }
> +      if (this.VALID_LABELS["tel"].test(string)) {

I would use dot (.) syntax instead of square brackets here as it's more common.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:37
(Diff revision 9)
> +  extractElementStrings(element) {
> +    let strings = [];
> +    function iter(ele){

This method should have a JSDoc comment describing what it's doing as it's not clear to me from a quick glance. Is it a port of existing code? If so, please include a reference.

My initial thought is "how is this different than just `element.textContent`?". The JSDoc comment should answer that question. Obviously the return type is different…

::: browser/extensions/formautofill/FormAutofillUtils.jsm:37
(Diff revision 9)
> +  extractElementStrings(element) {
> +    let strings = [];
> +    function iter(ele){

This should be in its own commit/bug with tests too.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:44
(Diff revision 9)
> +      } else {
> +        let s = ele.textContent.trim();
> +        if (s) {
> +          strings.push(s);
> +        }
> +      }

If this isn't a port of some existing code where you're trying to keep them looking similar then I would avoid the else and have this base case just do a return at the beginning like the usual m-c style which reduces nesting:

```js
if (ele.childNodes.length == 0) {
  let trimmedText = ele.textContent.trim();
  if (trimmedText) {
    strings.push(trimmedText);
  }
  return;
}

for (let i = 0; i < ele.childNodes.length; i++) {
…
```

I also think `s` should be more verbose e.g. `trimmedText`

::: browser/extensions/formautofill/ProfileStorage.jsm:80
(Diff revision 9)
>    "tel",
>    "email",
>  ];
>  
>  // TODO: Remove this once we can add profile from preference.
> -const MOCK_MODE = false;
> +const MOCK_MODE = true;

Don't forget to revert this. Now that we have management UI I actually think we should remove this mock storage code.

::: browser/extensions/formautofill/ProfileStorage.jsm:88
(Diff revision 9)
> +  "additional-name": "John",
> +  "family-name": "Monster",
>    guid: "test-guid-1",
>    organization: "Sesame Street",
>    "street-address": "123 Sesame Street.",
> +  email: "elmo@sesamest.com",

Since this is a real domain we don't own I don't think we should use it in the test case. It could turn into a malware domain that causes our code to be flagged by some security software. Crawlers may also start spamming this address when the code gets indexed. If you want the domain name to be descriptive, please use an RFC 2606 TLD for testing/examples: https://tools.ietf.org/html/rfc2606#section-2

::: browser/extensions/formautofill/ProfileStorage.jsm:90
(Diff revision 9)
>    guid: "test-guid-1",
>    organization: "Sesame Street",
>    "street-address": "123 Sesame Street.",
> +  email: "elmo@sesamest.com",
> +  "postal-code": "3345678",
>    tel: "1-345-345-3456",

This phone number should also be changed if we leave it e.g. 1-800-555-0123 from https://en.wikipedia.org/wiki/555_(telephone_number)

::: browser/extensions/formautofill/ProfileStorage.jsm:97
(Diff revision 9)
> +  "given-name": "Firefox",
> +  "family-name": "Gecko",
>    guid: "test-guid-2",
>    organization: "Mozilla",
>    "street-address": "331 E. Evelyn Avenue",
> +  email: "firefox@mozilla.com",

For similar reasons I think we should avoid this email address. Use something like firefox@mozilla.example if we end up keeping the mock data in this bug.
Attachment #8856892 - Flags: review?(MattN+bmo)
Blocks: 1358960
Comment on attachment 8856892 [details]
Bug 1349490 - Part 1: Implement FormAutofillUtil.extractLabelStrings function.;

https://reviewboard.mozilla.org/r/128810/#review135106

> Are you sure you're not going to hit the issue with JSMs and latin-1 that Luke hit?

Yes, I am going to load these regular expressions (in a seperate file) with loadSubScript of mozIJSSubScriptLoader.

> This should ideally be its own commit/bug with tests. You forgot "button" and I think it would be safer to use an allow list instead of a block list.

I tried to move getInfo function into its own commit, but it looks like all regexp should be placed in the commit. The remain codes are very small, so I still let them in the same one. If you think it's necessary, I can do that again. BTW, the xpc-shell test for getInfo is in the latest patch.
Comment on attachment 8856892 [details]
Bug 1349490 - Part 1: Implement FormAutofillUtil.extractLabelStrings function.;

https://reviewboard.mozilla.org/r/128810/#review135106

> I asked in the other bug if you need the "u" modifier?
> 
> Can you verify that my other existing comments in the original bug are addressed?

The related comment is at bug 1347176 comment 3. I added "u" in the latest patch, but I will mark this `fixed` later once I verified the final result of loading regexp with loadSubScript
Comment on attachment 8856892 [details]
Bug 1349490 - Part 1: Implement FormAutofillUtil.extractLabelStrings function.;

https://reviewboard.mozilla.org/r/128810/#review135106

> Yes, I am going to load these regular expressions (in a seperate file) with loadSubScript of mozIJSSubScriptLoader.

After using loadSubScript in UTF8, the regular expression can be parsed correctly for non-latin characters.

> The related comment is at bug 1347176 comment 3. I added "u" in the latest patch, but I will mark this `fixed` later once I verified the final result of loading regexp with loadSubScript

After verifying the latest patch, we don't need to add "u" in these regexps with loadSubScript in UTF8. However, I still use it for explicit define.
Blocks: 1360370
There are some updates since the last update:
1. Multiple line address supports now, e.g. address-line2, address-line3.

2. "u" is needed after verifying some cases, and the latest patch includes it. (see comment 19)

3. Bug 1360370 is for "select" element support.
Comment on attachment 8856892 [details]
Bug 1349490 - Part 1: Implement FormAutofillUtil.extractLabelStrings function.;

https://reviewboard.mozilla.org/r/128810/#review137816

::: browser/extensions/formautofill/FormAutofillUtils.jsm:59
(Diff revision 14)
> +      for (let i = 0; i < ele.childNodes.length; i++) {
> +        iter(ele.childNodes[i]);

This includes comment text which probably isn't desired. Please add a test case with `<!-- foo -->` and ensure that we don't use it unless that's intentional. Do we want all Nodes or do we want only Elements (if so, use .children).
Attachment #8856892 - Flags: review?(MattN+bmo)
Comment on attachment 8856892 [details]
Bug 1349490 - Part 1: Implement FormAutofillUtil.extractLabelStrings function.;

https://reviewboard.mozilla.org/r/128810/#review137816

> This includes comment text which probably isn't desired. Please add a test case with `<!-- foo -->` and ensure that we don't use it unless that's intentional. Do we want all Nodes or do we want only Elements (if so, use .children).

TEXT_NODE should be considered as well, so "childNodes" is still used only for TEXT_NODE and ELEMENT_NODE.
Comment on attachment 8856892 [details]
Bug 1349490 - Part 1: Implement FormAutofillUtil.extractLabelStrings function.;

https://reviewboard.mozilla.org/r/128810/#review137838

::: browser/extensions/formautofill/FormAutofillUtils.jsm:47
(Diff revision 15)
> +   * @param  {Object} element
> +   *         A DOM element to be extracted.
> +   * @returns {Array}
> +   *          All strings in an element.
> +   */
> +  extractElementStrings(element) {

Nit: extractLabelStrings since it's not generic enough to handle all text strings.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:48
(Diff revision 15)
> +    let strings = [];
> +    function _extractElementStrings(el) {

Ideally this would work like https://cs.chromium.org/chromium/src/components/autofill/content/renderer/form_autofill_util.cc?l=200&rcl=d4fcf211213e19d716f822cc514da06155879926 and handle non-label elements but I guess we can do that later.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:60
(Diff revision 15)
> +        let node = el.childNodes[i];
> +        if (node.nodeType != Ci.nsIDOMNode.ELEMENT_NODE &&
> +            node.nodeType != Ci.nsIDOMNode.TEXT_NODE) {
> +          continue;
> +        }

Please also exclude <script>, <noscript>, <option>, <style> etc. and link to https://cs.chromium.org/chromium/src/components/autofill/content/renderer/form_autofill_util.cc?l=216&rcl=d33a171b7c308a64dc3372fac3da2179c63b419e

We should handle not traversing other autofill form fields in a follow-up.
Attachment #8856892 - Flags: review?(MattN+bmo)
Comment on attachment 8856892 [details]
Bug 1349490 - Part 1: Implement FormAutofillUtil.extractLabelStrings function.;

https://reviewboard.mozilla.org/r/128810/#review137850

::: browser/extensions/formautofill/FormAutofillUtils.jsm:65
(Diff revision 17)
> +      for (let i = 0; i < el.childNodes.length; i++) {
> +        let node = el.childNodes[i];

Nit: use for…of

::: browser/extensions/formautofill/test/unit/test_extractLabelStrings.js:11
(Diff revision 17)
> +  {
> +    description: "A label element contains one input element.",
> +    document: `<label id="typeA"> label type A
> +                 <!-- This comment should not be extracted. -->
> +                 <input type="text">
> +                 <script>FOO</script>

Add <style> in one test case please
Attachment #8856892 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8861454 [details]
Bug 1349490 - Part 2: Use a set of regexp to recognize the input autofill type.;

https://reviewboard.mozilla.org/r/133446/#review137862

::: commit-message-e78ad:1
(Diff revision 10)
> +Bug 1349490 - Part 2: Use a set of regexp to recognize the input field type.; r?MattN

s/field/autofill/

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:48
(Diff revision 10)
> +    if (!this.RULES) {
> +      let sandbox = {};
> +      let scriptLoader = Cc["@mozilla.org/moz/jssubscript-loader;1"]
> +                           .getService(Ci.mozIJSSubScriptLoader);
> +      const HEURISTICS_REGEXP = "chrome://formautofill/content/heuristicsRegexp.js";

Since `getInfo` also uses `this.RULES`, can you use `XPCOMUtils.defineLazyGetter` at the bottom of the file (since there is no `init` function currently).

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:93
(Diff revision 10)
>      return fieldDetails;
>    },
>  
> -  getInfo(element) {
> -    if (!(element instanceof Ci.nsIDOMHTMLInputElement)) {
> +  getInfo(element, fieldDetails) {
> +    if (!(element instanceof Ci.nsIDOMHTMLInputElement) ||
> +        !["text", "email", "tel"].includes(element.type) ||

What about "number" for zip codes and credit cards numbers?

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:100
(Diff revision 10)
>        return null;
>      }
>  
>      let info = element.getAutocompleteInfo();
> -    if (!info || !info.fieldName ||
> -        !this.VALID_FIELDS.includes(info.fieldName)) {
> +    if (info && info.fieldName &&
> +        Object.keys(this.RULES).includes(info.fieldName)) {

I think there should be a helper such as `isSupportedFieldName` that handles this since it doesn't seem clear to lookup in the keys for an object of heuristic regexps.

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:104
(Diff revision 10)
> +    // "tel" type is used for ZIP code for some web site (e.g. HomeDepot,
> +    // BestBuy), so element.type is not for "tel" prediction.

It seems like this comment is out-of-place

::: browser/extensions/formautofill/content/heuristicsRegexp.js:17
(Diff revision 10)
> +
> +var HeuristicsRegExp = {
> +  // These regular expressions are from Chromium source codes [1]. Most of them
> +  // converted to JS format have the same meaning with the original ones except
> +  // the first line of "address-level1".
> +  // [1] https://cs.chromium.org/chromium/src/components/autofill/core/browser/autofill_regex_constants.cc

Please add this file to https://dxr.mozilla.org/mozilla-central/rev/1c6fa11d1fed893b00d94b6a899c307262674613/toolkit/content/license.html#2725-2728
Blocks: 1361237
Comment on attachment 8861454 [details]
Bug 1349490 - Part 2: Use a set of regexp to recognize the input autofill type.;

https://reviewboard.mozilla.org/r/133446/#review146296

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:85
(Diff revision 17)
> -  getInfo(element) {
> -    if (!(element instanceof Ci.nsIDOMHTMLInputElement)) {
> +  isSupportedFieldName(fieldName) {
> +    return fieldName && Object.keys(this.RULES).includes(fieldName);
> +  },

This needs a comment explaining what it’s doing. This also doesn’t really address the point of my comment on the last revision. Why should the result of getAutocompleteInfo have anything to do with whether or not we have a regex for a field type? Perhaps isSupportedFieldName should have its own separate list of supported field types? The way you have the code working means that if we don’t support heuristics for a type, only @autocomplete then the @autocomplete-only fields won’t work. If all autofill fields in a process don’t need heuristics then we shouldn’t even load the regexes into memory. Using FIELD_GROUPS keys would be better IMO.

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:115
(Diff revision 17)
> +    let verifyFieldName = (string) => {
> +      let result = {

I think `matchStringToFieldName` or something like that would be a bit clearer since "verify" normally has a different meaning… usually about checking that something is correct.

Please add a JSDoc comment on the method too.

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:132
(Diff revision 17)
> +          // If "address-line1" or "address-line2" exist already, the string
> +          // could be satisfied with "address-line2" or "address-line3".
> +          if ((fieldName == "address-line1" &&
> +              existingFieldNames.includes("address-line1")) ||
> +              (fieldName == "address-line2" &&
> +              existingFieldNames.includes("address-line2"))) {
> +            continue;
> +          }

This seems like something getFormInfo (the caller) should handle. Did you consider that?

Then you can move `verifyFieldName` to be a method instead of an inner function (to improve readability).

::: browser/extensions/formautofill/content/heuristicsRegexp.js:157
(Diff revision 17)
> +      "|国家" +      // zh-CN
> +      "|국가|나라",  // ko-KR
> +      "iu"
> +    ),
> +
> +    // ==== Name Relative Fields ====

s/ Relative//

::: browser/extensions/formautofill/test/fixtures/autocomplete_basic.html:22
(Diff revision 17)
> +  <form id="formB">
> +    <p><label>Organization: <input type="text" id="B_organization" name="organization" /></label></p>
> +    <p><label>Address Line 1: <input type="text" id="B_address-line1" name="address-line1" /></label></p>

Can you test the id and name cases separately (at least for some of the <input>) to ensure that both work?

::: toolkit/content/license.html:2728
(Diff revision 17)
> +        <li><span class="path">browser/extensions/formautofill/content/heuristicsRegexp.js</span></li>
>          <li><span class="path">browser/extensions/formautofill/content/nameReferences.js</span></li>
>          <li><span class="path">browser/extensions/formautofill/FormAutofillNameUtils.jsm</span></li>

This should be wrapped in an `#ifndef RELEASE_OR_BETA` block (along with the existing autofill ones) since we're not shipping these yet.

https://dxr.mozilla.org/mozilla-central/rev/37d777d872002597198405f22eefa9199e5103af/browser/extensions/moz.build#17,20
Attachment #8861454 - Flags: review?(MattN+bmo)
Comment on attachment 8868941 [details]
Bug 1349490 - Part 3: Remove unused information in test fixtures.;

https://reviewboard.mozilla.org/r/140604/#review146298

::: browser/extensions/formautofill/test/fixtures/third_party/BestBuy/Checkout_Payment.html:10
(Diff revision 1)
> -         <input type="hidden" value="pcat17071" name="id">
> -         <input type="hidden" value="page" name="type">
> -         <input type="hidden" value="Global" name="sc">
> -         <input type="hidden" value="1" name="cp">
> +         <input type="hidden" value="" name="id">
> +         <input type="hidden" value="" name="type">
> +         <input type="hidden" value="" name="sc">
> +         <input type="hidden" value="" name="cp">
>           <input type="hidden" value="" name="nrp">

I don't see the advantage in clearing out generated default values since it makes our tests further from what users will actually see.

::: browser/extensions/formautofill/test/fixtures/third_party/BestBuy/Checkout_Payment.html:37
(Diff revision 1)
> -                  <input type="text" id="fulfillment.fulfillmentGroups.0.fulfillment.address.firstName" name="firstName" maxlength="29" value="Tester">
> +                  <input type="text" id="fulfillment.fulfillmentGroups.0.fulfillment.address.firstName" name="firstName" maxlength="29" value="">
>                 </div>
>              </label>
>           </div>
>           <div>
>              <label for="fulfillment.fulfillmentGroups.0.fulfillment.address.lastName">
>                 <span>
>                    <p>Last Name</p>
>                 </span>
>                 <div>
> -                  <input type="text" id="fulfillment.fulfillmentGroups.0.fulfillment.address.lastName" name="lastName" maxlength="30" value="Mo">
> +                  <input type="text" id="fulfillment.fulfillmentGroups.0.fulfillment.address.lastName" name="lastName" maxlength="30" value="">

I agree with removing the references to Vance or test addresses since they shouldn't have been included in the first place since they don't reflect how the page would be for a new visitor
Attachment #8868941 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8861454 [details]
Bug 1349490 - Part 2: Use a set of regexp to recognize the input autofill type.;

https://reviewboard.mozilla.org/r/133446/#review146296

> This needs a comment explaining what it’s doing. This also doesn’t really address the point of my comment on the last revision. Why should the result of getAutocompleteInfo have anything to do with whether or not we have a regex for a field type? Perhaps isSupportedFieldName should have its own separate list of supported field types? The way you have the code working means that if we don’t support heuristics for a type, only @autocomplete then the @autocomplete-only fields won’t work. If all autofill fields in a process don’t need heuristics then we shouldn’t even load the regexes into memory. Using FIELD_GROUPS keys would be better IMO.

Agree with you. I remove `isSupportedFieldName` check of getAutocompleteInfo result and `isSupportedFieldName` function as well.

> I think `matchStringToFieldName` or something like that would be a bit clearer since "verify" normally has a different meaning… usually about checking that something is correct.
> 
> Please add a JSDoc comment on the method too.

I move `matchStringToFieldName` out of `getInfo`.

> This seems like something getFormInfo (the caller) should handle. Did you consider that?
> 
> Then you can move `verifyFieldName` to be a method instead of an inner function (to improve readability).

I ever considered that...
Using the way with existingFieldNames here is to give address-line[1-3] field names to these inputs early.

I think iwe can implement the post process after all getInfo in getFormInfo if needed.
However, the current process can satisfy the current requirement.

> Can you test the id and name cases separately (at least for some of the <input>) to ensure that both work?

I modified the elements in `#formB` to fulfill the following combinations:
label_text, id, name
id, name
label_text, name
label_text, id
id
name
label_text
Comment on attachment 8868941 [details]
Bug 1349490 - Part 3: Remove unused information in test fixtures.;

https://reviewboard.mozilla.org/r/140604/#review146298

> I don't see the advantage in clearing out generated default values since it makes our tests further from what users will actually see.

I revert all changes to the hidden inputs.

> I agree with removing the references to Vance or test addresses since they shouldn't have been included in the first place since they don't reflect how the page would be for a new visitor

I go through all other fixtures and remove the value modified by users.
Comment on attachment 8861454 [details]
Bug 1349490 - Part 2: Use a set of regexp to recognize the input autofill type.;

https://reviewboard.mozilla.org/r/133446/#review146296

> I ever considered that...
> Using the way with existingFieldNames here is to give address-line[1-3] field names to these inputs early.
> 
> I think iwe can implement the post process after all getInfo in getFormInfo if needed.
> However, the current process can satisfy the current requirement.

Isn't the proposed solution dependent on the DOM order of the form fields e.g. if `address-line1` appears after `address-line2` then it may be incorrect? I think that's an edge case but I still don't like the change to the API, requiring existing field names to be passed in. I guess we can fix it in a follow-up so that we can get this landed.
Comment on attachment 8861454 [details]
Bug 1349490 - Part 2: Use a set of regexp to recognize the input autofill type.;

https://reviewboard.mozilla.org/r/133446/#review148072

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:88
(Diff revision 21)
> +   * existing field names to prevent the duplicating predict
> +   * (e.g. address-line[1-3).

"to prevent duplicating predictions"
Attachment #8861454 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8872214 [details]
Bug 1349490 - Part 4: Add "extensions.formautofill.heuristics.enabled" for toggling form autofill heuristics.;

https://reviewboard.mozilla.org/r/143680/#review148074

Thanks.

We need to address the Talos issues ASAP otherwise we risk having a really hard time being able to turn this pref on in the future due to piling on more performance bottlenecks.

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:151
(Diff revision 2)
> +    if (!Services.prefs.getBoolPref("extensions.formautofill.heuristics.enabled")) {
> +      return null;
> +    }

Since this is on a hot code path we should use `XPCOMUtils.defineLazyPreferenceGetter` to get and watch the pref value

::: browser/extensions/formautofill/test/unit/head.js:123
(Diff revision 2)
> +  Services.prefs.setBoolPref("extensions.formautofill.heuristics.enabled", true);
>  
>    // Clean up after every test.
>    do_register_cleanup(function head_cleanup() {
>      Services.prefs.clearUserPref("extensions.formautofill.experimental");

You forgot to clean it up (I think it's not strictly necessary for xpcshell since each test runs in it's own scope but we should be consistent with these two prefs. Either cleanup both or don't cleanup either.
Attachment #8872214 - Flags: review?(MattN+bmo) → review+
Blocks: 1368858
Blocks: 1360370
No longer depends on: 1360370
Blocks: 1368872
Comment on attachment 8872214 [details]
Bug 1349490 - Part 4: Add "extensions.formautofill.heuristics.enabled" for toggling form autofill heuristics.;

https://reviewboard.mozilla.org/r/143680/#review148074

Bug 1368872 is just filed for fixing the talos issues.

Thank you very much to review these patches!
Comment on attachment 8861454 [details]
Bug 1349490 - Part 2: Use a set of regexp to recognize the input autofill type.;

https://reviewboard.mozilla.org/r/133446/#review146296

> Isn't the proposed solution dependent on the DOM order of the form fields e.g. if `address-line1` appears after `address-line2` then it may be incorrect? I think that's an edge case but I still don't like the change to the API, requiring existing field names to be passed in. I guess we can fix it in a follow-up so that we can get this landed.

Yes, it does depend on the order. I filed a bug 1368858 to fix this.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/862c6eb8b0e3
Part 1: Implement FormAutofillUtil.extractLabelStrings function.; r=MattN
https://hg.mozilla.org/integration/autoland/rev/3c62252be2db
Part 2: Use a set of regexp to recognize the input autofill type.; r=MattN
https://hg.mozilla.org/integration/autoland/rev/7e3416b4e5fb
Part 3: Remove unused information in test fixtures.; r=MattN
https://hg.mozilla.org/integration/autoland/rev/c76be60a7965
Part 4: Add "extensions.formautofill.heuristics.enabled" for toggling form autofill heuristics.; r=MattN
Keywords: checkin-needed
Blocks: 1370429
As per San-Francisco meeting with :vchen, marking this bug as qe-.
Flags: qe-verify-
Blocks: 1392528
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: