Implement an API in the parent process to decide which values of an autofill profile would be filled in which field

RESOLVED FIXED in Firefox 52

Status

()

Toolkit
Form Manager
P3
enhancement
RESOLVED FIXED
8 months ago
6 months ago

People

(Reporter: MattN, Assigned: lchang)

Tracking

(Blocks: 1 bug)

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [form autofill:M1])

MozReview Requests

()

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

Attachments

(1 attachment)

Abstract the mapping logic used to decide how to fill from an autofill profile so we can also use it later to highlight the relevant fields when highlighting an entry in autocomplete.

See bug 1300988 for more details about how to handle not filling a full form.
Blocks: 1300996
No longer blocks: 990192
See FormAutofillContentService's collectFormFields which builds the data structure which could be sent to the parent process and onAccept[1].

[1] https://dxr.mozilla.org/mozilla-central/rev/f0f15b7c6aa77a0c5750918aa0a1cb3dc82185bc/toolkit/components/formautofill/content/requestAutocomplete.js#38
Summary: Implement an API in the content process to decide which values of an autofill profile would be filled in which field → Implement an API in the parent process to decide which values of an autofill profile would be filled in which field
(Assignee)

Updated

7 months ago
Assignee: nobody → lchang
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Comment 3

6 months ago
Comment on attachment 8802824 [details]
Bug 1300990 - Implement an API in the parent process to decide which values of an autofill profile would be filled in which field;

Hi Matt,

I'm not sure if Message Manager should be taken into account in this bug. However, I assume we can use "FormAutofill:requestAutocomplete" and "FormAutofill:fillForm" for delivering the data. The call flow I imagine would be:

1. Client listens to DOMAutocomplete event, builds the fields' structure and sends it to parent via "requestAutocomplete" message.
2. Parent fills in the fields object and sends it back via "fillForm" message.
3. Client fills in the real fields then.

It would be great if you can take a look and correct me if something mistaken. Note that "ProfileStorage.jsm" was implemented in bug 1016733 and hasn't landed yet.

Thanks.
Attachment #8802824 - Flags: feedback?(MattN+bmo)
Comment on attachment 8802824 [details]
Bug 1300990 - Implement an API in the parent process to decide which values of an autofill profile would be filled in which field;

https://reviewboard.mozilla.org/r/87126/#review89950

Looks good. f+

::: browser/extensions/formautofill/content/FormAutofillParent.jsm:39
(Diff revision 1)
> +XPCOMUtils.defineLazyModuleGetter(this, "OS",
> +                                  "resource://gre/modules/osfile.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "ProfileStorage",
> +                                  "resource://formautofill/ProfileStorage.jsm");
> +
> +var PROFILE_JSON_FILE_NAME = "formautofill-profiles.json";

Let's not mention "form" in the name since they will also be used for web payments.

::: browser/extensions/formautofill/content/FormAutofillParent.jsm:53
(Diff revision 1)
> +    this._profileStore = new ProfileStorage(storePath);
> +    this._profileStore.initialize();
> +
> +    let mm = Cc["@mozilla.org/globalmessagemanager;1"]
> +               .getService(Ci.nsIMessageListenerManager);
> +    mm.addMessageListener("FormAutofill:requestAutocomplete", this);

I wonder if "requestAutocomplete" will be confused with the form.requestAutocomplete API. How about FormAutofill:PopulateFieldValues?

::: browser/extensions/formautofill/content/FormAutofillParent.jsm:67
(Diff revision 1)
> +  },
> +
> +  _requestAutocomplete({guid, fields}, target) {
> +    let profile = this._profileStore.get(guid);
> +    if (!profile) {
> +      // Should we send failed message back?

Hmm… not sure if that will be useful yet but we should at least log a message or throw an error. I would suggest throwing an error for now `throw new Error("…");`
Comment on attachment 8802824 [details]
Bug 1300990 - Implement an API in the parent process to decide which values of an autofill profile would be filled in which field;

I think comment 3 makes sense
Attachment #8802824 - Flags: feedback?(MattN+bmo) → feedback+
Comment hidden (mozreview-request)
(Assignee)

Comment 7

6 months ago
Matt, Thanks for the feedback. I'll start to write the unit tests.
Comment hidden (mozreview-request)
Comment on attachment 8802824 [details]
Bug 1300990 - Implement an API in the parent process to decide which values of an autofill profile would be filled in which field;

https://reviewboard.mozilla.org/r/87126/#review91358

::: browser/extensions/formautofill/test/unit/test_populateFieldValues.js:2
(Diff revision 3)
> +/*
> + * Test for form auto fill content helper collectFormFields functions.

Is this comment stale?
Attachment #8802824 - Flags: review?(MattN+bmo) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 11

6 months ago
mozreview-review-reply
Comment on attachment 8802824 [details]
Bug 1300990 - Implement an API in the parent process to decide which values of an autofill profile would be filled in which field;

https://reviewboard.mozilla.org/r/87126/#review91358

Matt, Thanks a lot.

> Is this comment stale?

Uh... I forgot!
Comment hidden (mozreview-request)
(Assignee)

Comment 13

6 months ago
Tests are passed.
Keywords: checkin-needed

Comment 14

6 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/17dd918189c1
Implement an API in the parent process to decide which values of an autofill profile would be filled in which field; r=MattN
Keywords: checkin-needed

Comment 15

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/17dd918189c1
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.