Closed Bug 1300990 Opened 7 years ago Closed 7 years ago

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

Categories

(Toolkit :: Form Manager, enhancement, P3)

Product:
Toolkit
Component:
Form Manager
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: MattN, Assigned: lchang)

References

Details

(Whiteboard: [form autofill:M1])

Attachments

(1 file)

Bug 1300990 - Implement an API in the parent process to decide which values of an autofill profile would be filled in which field;
58 bytes, text/x-review-board-request
MattN
: review+
Details 
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: nobody → lchang
Status: NEW → ASSIGNED
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+
Matt, Thanks for the feedback. I'll start to write the unit tests.
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 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!
Tests are passed.
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/17dd918189c1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
As per San-Francisco meeting with :vchen, marking this bug as qe-.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.