Closed Bug 1300988 Opened 8 years ago Closed 8 years ago

Implement an API in the content process to fill a form with a specific form autofill profile using @autocomplete

Categories

(Toolkit :: Form Manager, enhancement, P3)

Product:
Toolkit
Component:
Form Manager
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Iteration:
52.2 - Oct 17
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: MattN, Assigned: steveck)

References

Details

(Whiteboard: [form autofill:M1][ETA=10/7])

Attachments

(3 files, 3 obsolete files)

Bug 1300988 - Part 2: Move MockDocument to toolkit/modules and refactor unit test in passwordmgr.
58 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1300988 - Part 1: Add FormAutoFillHandler instance and collectFormFields/autofillFormFields methods.
58 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1300988 - Part 3: XPCShell test for collectFormFields/autofillFormFields.
58 bytes, text/x-review-board-request
MattN
: review+
Details 
We need an API in the content process that can be provided with form autofill profile data and a relevant form field (where autocomplete is/was attached) and is able to fill in the section of the form related to the field with the relevant data using @autocomplete.

For example, consider a <form> which takes two addresses, billing and shipping, when this API is called for a shipping street-address field, the profile should only be filled into other shipping fields, not into the billing fields.
Flags: qe-verify-
Blocks: 1300989
Depends on: 1300990
See FormAutofillContentService's autofillFormFields method.
It's still a WIP that simply copy the necessary part from FormAutofillContentService, and seems like we should add tests for collectFormFields and autofillFormFields separately.
Assignee: nobody → schung
Comment on attachment 8792750 [details]
Bug 1300988 - Part 1: Add FormAutoFillHandler instance and collectFormFields/autofillFormFields methods.

https://reviewboard.mozilla.org/r/79652/#review79334

Looks good overall, just a few things to adjust.

::: browser/extensions/formautofill/content/FormAutofillContent.js:14
(Diff revision 2)
> +"use strict";
> +
> +const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
> +
> +/**
> + * Handles requestAutocomplete for a DOM Form element.

Nit: stale comment mentioning "requestAutocomplete"

::: browser/extensions/formautofill/content/FormAutofillContent.js:16
(Diff revision 2)
> +function FormAutoFillHandler(aForm) {
> +  this.form = aForm;
> +  this.fieldDetails = [];
> +}
> +
> +FormAutoFillHandler.prototype = {

Nit: it seems like Autofill doesn't have the "f" captilized most commonly. (e.g. in Chrome UI) so I think we should keep it lowercase too

::: browser/extensions/formautofill/content/FormAutofillContent.js:33
(Diff revision 2)
> +   * used to identify the exact field when the serializable data is received
> +   * from the requestAutocomplete user interface.  There cannot be multiple

Nit: This comment is stale

::: browser/extensions/formautofill/content/FormAutofillContent.js:46
(Diff revision 2)
> +   * @returns Serializable data structure that can be sent to the user
> +   *          interface, or null if the operation failed because the constraints
> +   *          on the allowed fields were not honored.

There are some warnings and errors with linting if you run:
`./mach lint browser/extensions/formautofill/`

::: browser/extensions/formautofill/content/FormAutofillContent.js:50
(Diff revision 2)
> +   *
> +   * @returns Serializable data structure that can be sent to the user
> +   *          interface, or null if the operation failed because the constraints
> +   *          on the allowed fields were not honored.
> +   */
> +  collectFormFields: function () {

Nit: use ES6 method syntax in new code:
`collectFormFields() {`

::: browser/extensions/formautofill/content/FormAutofillContent.js:63
(Diff revision 2)
> +        continue;
> +      }
> +
> +      // Exclude elements to which no autocomplete field has been assigned.
> +      let info = element.getAutocompleteInfo();
> +      if (!info.fieldName || ["on", "off"].indexOf(info.fieldName) != -1) {

You can update this to use Array.prototype.includes now for easier reading

::: browser/extensions/formautofill/content/FormAutofillContent.js:75
(Diff revision 2)
> +      this.fieldDetails.push({
> +        section: info.section,
> +        addressType: info.addressType,
> +        contactType: info.contactType,
> +        fieldName: info.fieldName,
> +        element: element,
> +      });

As we talked about live now, I think we don't need to build the nested structure anymore since that was mostly useful for having UI to fill multiple sections at once with rAc. With autofill I think we will only ever fill one section at a time.

::: browser/extensions/formautofill/content/FormAutofillContent.js:108
(Diff revision 2)
> +        contactType: info.contactType,
> +        index: this.fieldDetails.length - 1

Nit: always put a trailing comma on object literal definitions so that blame doesn't change when a new property is added after.

::: browser/extensions/formautofill/content/FormAutofillContent.js:118
(Diff revision 2)
> +   * Processes form fields that can be autofilled, and populates them with the
> +   * data provided by RequestAutocompleteUI.

Stale comment

::: browser/extensions/formautofill/content/FormAutofillContent.js:134
(Diff revision 2)
> +   *            value: String with which the field should be updated.
> +   *            index: Index to match the input in fieldDetails
> +   *          ],
> +   *        }
> +   */
> +  autofillFormFields: function (aAutofillResult) {

Nit: we can now use ES6 method syntax:
`autofillFormFields(aAutofillResult) {`

::: browser/extensions/formautofill/content/FormAutofillContent.js:136
(Diff revision 2)
> +      // Get the field details, if it was processed by the user interface.
> +      let fieldDetail = this.fieldDetails[field.index];
> +
> +      if (!fieldDetail) {
> +        continue;
> +      }

I think we should also confirm that the 4 autocomplete tokens still match at that index and log an error at Cu.reportError and continue if they don't. It's possible that the form has changed since collecting
Attachment #8792750 - Flags: review?(MattN+bmo)
Comment on attachment 8793667 [details]
Bug 1300988 - Part 2: XPCShell test for collectFormFields/autofillFormFields.

https://reviewboard.mozilla.org/r/80376/#review79338

I'll review again since we're changing the format of collectFormFields.

::: browser/extensions/formautofill/content/FormAutofillContent.js
(Diff revision 1)
> -const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
> -

You can leave this here and use the aliases. We use these aliases in almost every file.

::: browser/extensions/formautofill/content/FormAutofillContent.js:138
(Diff revision 1)
> -      if (!fieldDetail) {
> +      // Avoid the invalid value set
> +      if (!fieldDetail || !field.value) {

Can you also move this to part 1 so part 2 is just test stuff?

::: browser/extensions/formautofill/jar.mn:5
(Diff revision 1)
>  [features/formautofill@mozilla.org] chrome.jar:
> -% resource formautofill %content/
> +% content formautofill %content/
>   content/ (content/*)

Can you move this to the previous commit?

::: browser/extensions/formautofill/test/unit/head.js:7
(Diff revision 1)
> +////////////////////////////////////////////////////////////////////////////////
> +//// Globals
> +

I personally don't find this decoration necessary so would rather not have it unless you prefer it.

::: browser/extensions/formautofill/test/unit/head.js:23
(Diff revision 1)
> +const MockDocument = {
> +  /**
> +   * Create a document for the given URL containing the given HTML with the ownerDocument of all <form>s having a mocked location.
> +   */
> +  createTestDocument(aDocumentURL, aContent = "<form>", aType = "text/html") {

Can you factor this out of the original source into toolkit/modules/MockDocument.jsm. You can put it in TESTING_JS_MODULES[1] so that it's not packaged with Firefox, only for tests.

[1] https://dxr.mozilla.org/mozilla-central/rev/f0e6cc6360213ba21fd98c887b55fce5c680df68/toolkit/modules/moz.build#12

::: browser/extensions/formautofill/test/unit/head.js:63
(Diff revision 1)
> +add_task(function* test_common_initialize()
> +{

Please put braces on the same line. I think eslint has a rule for this too.

::: browser/extensions/formautofill/test/unit/head.js:66
(Diff revision 1)
> +//// Initialization functions common to all tests
> +
> +add_task(function* test_common_initialize()
> +{
> +  Services.prefs.setBoolPref("dom.forms.autocomplete.experimental", true);
> +  Services.prefs.setBoolPref("dom.forms.requestAutocomplete", true);

You shouldn't need to change dom.forms.requestAutocomplete as that is only for requestAutocomplete and that will be removed.

::: browser/extensions/formautofill/test/unit/test_formAutoFillContent.js:7
(Diff revision 1)
> + * Test for form auto fill content helper functions.
> + */
> +
> +"use strict";
> +
> +load("FormAutofillContent.js");

Cool, I didn't even know about this function

::: browser/extensions/formautofill/test/unit/test_formAutoFillContent.js:34
(Diff revision 1)
> +               <input id="country" autocomplete="country">
> +               <input id='email' autocomplete="email">
> +               <input id="tel" autocomplete="tel"></form>`,

Nit: Your quotes are inconsistent, I guess using double-quotes for "email" would be easiest.

::: browser/extensions/formautofill/test/unit/test_formAutoFillContent.js:208
(Diff revision 1)
> +
> +for (let tc of TESTCASES) {
> +
> +  (function() {
> +    let testcase = tc;
> +    add_task(function*() {

To fix eslint's knowledge of add_task you will need to add a copy of this .eslintrc directory to the "unit" directory: https://dxr.mozilla.org/mozilla-central/source/browser/components/dirprovider/tests/unit/.eslintrc

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

Nit: fix indentation

::: browser/extensions/formautofill/test/unit/test_formAutoFillContent.js:214
(Diff revision 1)
> +      do_print("Starting testcase: " + testcase.description);
> +
> +      let doc = MockDocument.createTestDocument("http://localhost:8080/test/",
> +                                                     testcase.document);
> +      let form = doc.querySelector("form");
> +      let handler = new FormAutoFillHandler(doc.querySelector('form'));

Nit: double quotes

::: browser/extensions/formautofill/test/unit/test_formAutoFillContent.js:216
(Diff revision 1)
> +      Assert.deepEqual(handler.collectFormFields(), testcase.returnedFormat,
> +                         "Check the format of form autofill were returned correctly");
> +
> +      Assert.deepEqual(handler.fieldDetails, testcase.fieldDetails,
> +                         "Check the fieldDetails were set correctly");
> +
> +      handler.autofillFormFields(testcase.profileData);

I personally would have tested these those two methods in different files since I think we will eventually want hundreds of test cases as we address more edge cases and I think unit tests should normally be more focused.

::: browser/extensions/formautofill/test/unit/xpcshell.ini:5
(Diff revision 1)
> +[DEFAULT]
> +head = head.js
> +tail =
> +support-files = ../../content/FormAutofillContent.js
> +firefox-appdir = browser

Is this necessary after all? If not, please remove it so the tests can run on Android too.
Attachment #8793667 - Flags: review?(MattN+bmo)
Attachment #8793667 - Attachment is obsolete: true
Comment on attachment 8792750 [details]
Bug 1300988 - Part 1: Add FormAutoFillHandler instance and collectFormFields/autofillFormFields methods.

https://reviewboard.mozilla.org/r/79652/#review79356

Looks good with some small changes. Thanks.

::: browser/extensions/formautofill/content/FormAutofillContent.js:62
(Diff revisions 2 - 3)
>          continue;
>        }
>  
>        // Exclude elements to which no autocomplete field has been assigned.
>        let info = element.getAutocompleteInfo();
> -      if (!info.fieldName || ["on", "off"].indexOf(info.fieldName) != -1) {
> +      if (!info.fieldName || ["on", "off"].includes(info.fieldName.toLowerCase())) {

The "toLowerCase" shouldn't be necessary as getAutocompleteInfo should normalize for you

::: browser/extensions/formautofill/content/FormAutofillContent.js:66
(Diff revision 3)
> +      // Store the association between the field metadata and the element.
> +      if (this.fieldDetails.some(f => f.section == info.section &&
> +                                      f.addressType == info.addressType &&
> +                                      f.contactType == info.contactType &&
> +                                      f.fieldName == info.fieldName)) {
> +        // A field with the same identifier already exists.
> +        return null;
> +      }

We'll probably want to change/remove this eventually but I guess it's okay for now.

::: browser/extensions/formautofill/content/FormAutofillContent.js:83
(Diff revision 3)
> +        contactType: info.contactType,
> +        fieldName: info.fieldName,
> +      };
> +      autofillData.push(inputFormat);
> +
> +      // Clone the inputData for caching the fields and elements together

inputData vs. inputFormat?

::: browser/extensions/formautofill/content/FormAutofillContent.js:117
(Diff revision 3)
> +      let info = fieldDetail.element.getAutocompleteInfo();
> +      if (field.section == info.section &&
> +          field.addressType == info.addressType &&
> +          field.contactType == info.contactType &&
> +          field.fieldName == info.fieldName) {
> +        fieldDetail.element.value = field.value;
> +      } else {
> +        Cu.reportError("Autocomplete tokens mismatched");

To reduce the nesting for the common code path I think we should follow the "Return from errors immediately"[1] pattern (with a continue instead of return).

```
if (field.section != info.section ||
    field.addressType != info.addressType ||
    field.contactType != info.contactType ||
    field.fieldName != info.fieldName) {
  Cu.reportError("Autocomplete tokens mismatched");
  continue;    
}
```

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Return_from_errors_immediately
Attachment #8792750 - Flags: review?(MattN+bmo) → review+
(In reply to Matthew N. [:MattN] (In Taipei until Sep. 23) from comment #7)
> Comment on attachment 8793667 [details]
> Bug 1300988 - Part 2: XPCShell test for collectFormFields/autofillFormFields.

> ::: browser/extensions/formautofill/test/unit/head.js:23
> (Diff revision 1)
> > +const MockDocument = {
> > +  /**
> > +   * Create a document for the given URL containing the given HTML with the ownerDocument of all <form>s having a mocked location.
> > +   */
> > +  createTestDocument(aDocumentURL, aContent = "<form>", aType = "text/html") {
> 
> Can you factor this out of the original source into
> toolkit/modules/MockDocument.jsm. You can put it in TESTING_JS_MODULES[1] so
> that it's not packaged with Firefox, only for tests.

I tried to spilt the MockDocument into test jsm but in vain :/ I'm not sure the correct patch to load the test jsm. In the other place I saw people will apply the url resource://test-common/... to load their test modules, but that didn't work for me. Should I include it in the jar.mn? I didn't find that we need to specify the test module in jar, but maybe I'm wrong.
BTW the reason I removed the declaration of Cc/Cu/Ci... in the head.js is because they are also declared in FormAutofillContent.js, and it will return redeclaration SyntaxError while running XPCShell test. It might be better to keep declaration instead of in head.js
Whiteboard: [form autofill:M1] → [form autofill:M1][ETA=10/7]
Iteration: --- → 52.2 - Oct 17
(In reply to Steve Chung [:steveck] from comment #13)
> (In reply to Matthew N. [:MattN] (In Taipei until Sep. 23) from comment #7)
> > Comment on attachment 8793667 [details]
> > Bug 1300988 - Part 2: XPCShell test for collectFormFields/autofillFormFields.
> 
> > ::: browser/extensions/formautofill/test/unit/head.js:23
> > (Diff revision 1)
> > > +const MockDocument = {
> > > +  /**
> > > +   * Create a document for the given URL containing the given HTML with the ownerDocument of all <form>s having a mocked location.
> > > +   */
> > > +  createTestDocument(aDocumentURL, aContent = "<form>", aType = "text/html") {
> > 
> > Can you factor this out of the original source into
> > toolkit/modules/MockDocument.jsm. You can put it in TESTING_JS_MODULES[1] so
> > that it's not packaged with Firefox, only for tests.
> 
> I tried to spilt the MockDocument into test jsm but in vain :/ I'm not sure
> the correct patch to load the test jsm. In the other place I saw people will
> apply the url resource://test-common/... to load their test modules, but
> that didn't work for me. Should I include it in the jar.mn? I didn't find
> that we need to specify the test module in jar, but maybe I'm wrong.

It's resource://testing-common/… so maybe you were just missing the "ing"? You need to have the module in TESTING_JS_MODULES of moz.build too.
Comment on attachment 8796118 [details]
Bug 1300988 - Part 2: Move MockDocument to toolkit/modules and refactor unit test in passwordmgr.

https://reviewboard.mozilla.org/r/82082/#review83302

::: browser/extensions/formautofill/test/unit/.eslintrc:6
(Diff revision 1)
> +{
> +  "extends": [
> +    "../../../../../testing/xpcshell/xpcshell.eslintrc"
> +  ],
> +  "globals": {
> +    "FormAutofillHandler": true

We won't need this if we use Services.scriptloader.loadSubScript or make FormAutofillContent a JSM which I think would be better.

::: browser/extensions/formautofill/test/unit/head.js:7
(Diff revision 1)
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +Components.utils.import("resource://gre/modules/Services.jsm");

Please use Cu

::: browser/extensions/formautofill/test/unit/head.js:8
(Diff revision 1)
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "Services",
> +                                  "resource://gre/modules/Services.jsm");

Remove the lazy version since you already importing above it.

::: browser/extensions/formautofill/test/unit/test_autofillFormFields.js:7
(Diff revision 1)
> + * Test for form auto fill content helper fill all inputs function.
> + */
> +
> +"use strict";
> +
> +load("FormAutofillContent.js");

The loadSubScript/JSM route would also avoid the issue with the Cc/Ci/Cu/… consts.

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

Nit: indentation
Attachment #8796118 - Flags: review?(MattN+bmo)
Comment on attachment 8792750 [details]
Bug 1300988 - Part 1: Add FormAutoFillHandler instance and collectFormFields/autofillFormFields methods.

https://reviewboard.mozilla.org/r/79652/#review83314

::: browser/extensions/formautofill/content/FormAutofillContent.js:129
(Diff revision 5)
> +          field.fieldName != info.fieldName) {
> +        Cu.reportError("Autocomplete tokens mismatched");
> +        continue;
> +      }
> +
> +      fieldDetail.element.value = field.value;

This needs to use the setUserInput API
Attachment #8792750 - Attachment is obsolete: true
Comment on attachment 8796118 [details]
Bug 1300988 - Part 2: Move MockDocument to toolkit/modules and refactor unit test in passwordmgr.

https://reviewboard.mozilla.org/r/82082/#review83302

> We won't need this if we use Services.scriptloader.loadSubScript or make FormAutofillContent a JSM which I think would be better.

I think we still need it even it's JSM, so I moved it to test files as comment to avoid the warning

> The loadSubScript/JSM route would also avoid the issue with the Cc/Ci/Cu/… consts.

I've tried loadSubScript/JSM route but both not working...
Comment on attachment 8796118 [details]
Bug 1300988 - Part 2: Move MockDocument to toolkit/modules and refactor unit test in passwordmgr.

https://reviewboard.mozilla.org/r/82082/#review83664

::: browser/extensions/formautofill/test/unit/MockDocument.jsm:1
(Diff revision 3)
> +/**

Please make this an `hg copy` from the existing defintion and then delete the existing defintion and replace it with an import of this new file.

Also, this new file should be in a location outside the extension to so passwordmgr can still use it. I'm not sure if we have a place for general testing modules yet.

::: browser/extensions/formautofill/test/unit/test_autofillFormFields.js:10
(Diff revision 3)
> +/* global FormAutofillHandler */
> +
> +"use strict";
> +
> +Cu.import("resource://testing-common/MockDocument.jsm");
> +Cu.import("chrome://formautofill/content/FormAutofillContent.jsm");

`Cu.import(Services.io.newFileURI(do_get_file("FormAutofillContent.jsm")));` will work. It's probably worth making a helper for this in head.js though (e.g. `importAutofillModule`). To fix eslint, we should use destructuring assument like:
`let {FormAutofillContent} = importAutofillModule("FormAutofillContent.jsm");`
Attachment #8796118 - Flags: review?(MattN+bmo)
(In reply to Matthew N. [:MattN] (behind on requests) from comment #22)
> Also, this new file should be in a location outside the extension to so
> passwordmgr can still use it. I'm not sure if we have a place for general
> testing modules yet.
> 
I tried to move MockDocument to toolkit/modules/ (or maybe toolkit/modules/tests/ would be better?) and imported it with Cu.import("resource://gre/modules/MockDocument.jsm") and add MockDocument.jsm in EXTRA_JS_MODULES (in toolkit/modules/moz.build), but the error returned with invalid url error message. Is there any step I missed for toolkit module?
Comment on attachment 8796118 [details]
Bug 1300988 - Part 2: Move MockDocument to toolkit/modules and refactor unit test in passwordmgr.

https://reviewboard.mozilla.org/r/82082/#review83664

> Please make this an `hg copy` from the existing defintion and then delete the existing defintion and replace it with an import of this new file.
> 
> Also, this new file should be in a location outside the extension to so passwordmgr can still use it. I'm not sure if we have a place for general testing modules yet.

I move it to toolkit/modules/ folder as EXTRA_JS_MODULES, but it failed to load the MockDocument with ERROR NS_ERROR_FILE_NOT_FOUND: Component returned failure code. Now I placed it under TESTING_JS_MODULES because it feel more reasonable to me(but not working neither).
Comment on attachment 8800507 [details]
Bug 1300988 - Part 3: Move MockDocument to toolkit/modules and refactor unit test in passwordmgr.

https://reviewboard.mozilla.org/r/85414/#review84302

::: browser/extensions/formautofill/moz.build
(Diff revision 1)
> -TESTING_JS_MODULES += [
> -  'test/unit/MockDocument.jsm'
> -]
> -

The addition of MockDocument.jsm should be in this same commit and it should be in a commit before Form autofill code imports it e.g. part 1 or 2.

i.e. there should only be one commit that refactors MockDocument into its own JSM instead of having it in an intermediate place in this bug and then move it in a later commit in this same bug.

::: toolkit/components/passwordmgr/test/unit/head.js:99
(Diff revision 1)
>  
> -const MockDocument = {

You can just import the JSM in head.js instead of each test so make it the same as before.

::: toolkit/modules/moz.build:12
(Diff revision 1)
>  TESTING_JS_MODULES += [
> +    'MockDocument.jsm',
>      'tests/PromiseTestUtils.jsm',

Yeah, follow those others and put it in the tests/ directory
Attachment #8800507 - Flags: review?(MattN+bmo)
Comment on attachment 8800047 [details]
Bug 1300988 - Part 1: Add FormAutoFillHandler instance and collectFormFields/autofillFormFields methods.

https://reviewboard.mozilla.org/r/85054/#review84304

::: browser/extensions/formautofill/content/FormAutofillContent.js:1
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

The rename to .jsm should be happening in this commit, not the testing commit. We don't squash when we land so each commit so be doing one logical unit of change.

::: browser/extensions/formautofill/jar.mn:6
(Diff revision 1)
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +[features/formautofill@mozilla.org] chrome.jar:
> +% content formautofill %content/

Switch this back to "% resource" for JSMs
Attachment #8800047 - Flags: review?(MattN+bmo)
Comment on attachment 8796118 [details]
Bug 1300988 - Part 2: Move MockDocument to toolkit/modules and refactor unit test in passwordmgr.

https://reviewboard.mozilla.org/r/82082/#review84306

This test commit should only include the addition of the tests to test/unit plus the addition of XPCSHELL_TESTS_MANIFESTS to keep the commits logically separated.
Attachment #8796118 - Flags: review?(MattN+bmo)
Attachment #8800507 - Attachment is obsolete: true
Comment on attachment 8796118 [details]
Bug 1300988 - Part 2: Move MockDocument to toolkit/modules and refactor unit test in passwordmgr.

https://reviewboard.mozilla.org/r/82082/#review84406

::: toolkit/modules/moz.build:13
(Diff revision 7)
>  BROWSER_CHROME_MANIFESTS += ['tests/browser/browser.ini']
>  MOCHITEST_MANIFESTS += ['tests/mochitest/mochitest.ini']
>  MOCHITEST_CHROME_MANIFESTS += ['tests/chrome/chrome.ini']
>  
>  TESTING_JS_MODULES += [
> +    'tests/MockDocument.jsm',

Actually the MockDocument didn't load corectly. No matter where I set the module (TESTING_JS_MODULES or EXTRA_JS_MODULES) it would return ERROR NS_ERROR_FILE_NOT_FOUND: Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]
Comment on attachment 8796118 [details]
Bug 1300988 - Part 2: Move MockDocument to toolkit/modules and refactor unit test in passwordmgr.

https://reviewboard.mozilla.org/r/82082/#review84406

> Actually the MockDocument didn't load corectly. No matter where I set the module (TESTING_JS_MODULES or EXTRA_JS_MODULES) it would return ERROR NS_ERROR_FILE_NOT_FOUND: Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]

Ok, it might be the problem in build faster that it didn't reset the path correctly...
Comment on attachment 8800047 [details]
Bug 1300988 - Part 1: Add FormAutoFillHandler instance and collectFormFields/autofillFormFields methods.

https://reviewboard.mozilla.org/r/85054/#review85302
Attachment #8800047 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8796118 [details]
Bug 1300988 - Part 2: Move MockDocument to toolkit/modules and refactor unit test in passwordmgr.

https://reviewboard.mozilla.org/r/82082/#review85306

r=me with fixes.

::: toolkit/modules/tests/MockDocument.jsm:1
(Diff revision 8)
> +/**

This file should be an `hg copy` of toolkit/components/passwordmgr/test/unit/head.js so blame of MockDocument is preserved and then the patch will make it clear if anything changed in the implementation.

::: toolkit/modules/tests/MockDocument.jsm:2
(Diff revision 8)
> +/**
> + * Provides infrastructure for automated login components tests.

This module isn't specific to logins, it's useful for any xpcshell tests that want to mock a Document

::: toolkit/modules/tests/MockDocument.jsm:9
(Diff revision 8)
> +
> +"use strict";
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
> +
> +Cu.importGlobalProperties(['URL']);

This seems unused and is using the wrong quotes.

::: toolkit/modules/tests/MockDocument.jsm:49
(Diff revision 8)
> +    });
> +  },
> +
> +};
> +
> +this.EXPORTED_SYMBOLS = ["MockDocument"]

We normally put this at the top of the file after "use strict;"
Attachment #8796118 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8800997 [details]
Bug 1300988 - Part 3: XPCShell test for collectFormFields/autofillFormFields.

https://reviewboard.mozilla.org/r/85784/#review85310

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

Nit: indentation looks off
Attachment #8800997 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8796118 [details]
Bug 1300988 - Part 2: Move MockDocument to toolkit/modules and refactor unit test in passwordmgr.

https://reviewboard.mozilla.org/r/82082/#review85306

> This seems unused and is using the wrong quotes.

I faced a URL undefined error while testing locally, maybe it's my environment's issue. I'll remove this and test it on try build
Thanks for all the reviews! I still add "Cu.importGlobalProperties(["URL"])" back in the latest commit, otherwise it would block the unit test.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a2eba9924352
Part 1: Add FormAutoFillHandler instance and collectFormFields/autofillFormFields methods. r=MattN
https://hg.mozilla.org/integration/autoland/rev/8e5e29ff80b6
Part 2: Move MockDocument to toolkit/modules and refactor unit test in passwordmgr. r=MattN
https://hg.mozilla.org/integration/autoland/rev/8f9f2fe0ed7b
Part 3: XPCShell test for collectFormFields/autofillFormFields. r=MattN
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: