Closed Bug 1338420 Opened 7 years ago Closed 7 years ago

Fallback to form history if whole profiles doesn't have any data for the specific fields

Categories

(Toolkit :: Form Manager, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: steveck, Assigned: steveck)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:M1])

Attachments

(2 files, 2 obsolete files)

A form history list should be prompted if profiles doesn't have data for the targeted input field. For example, if user focus on the company field but all the profiles doesn't company data, we should fall back to history for company field.
See Also: → 1338482
Blocks: 1338485
Summary: Fallback to form history if whole profiles doesn't have any data for the specific fileds → Fallback to form history if whole profiles doesn't have any data for the specific fields
I think this topic could be split into 2 parts:
1) Do not mark the field at the _identifyAutofillFields: Since mark API is called in _identifyAutofillFields synchronously, maybe we could leverage initialProcessData that store/update the whole profiles(or even replace the original initialProcessData.autofillEnabled). 

2) Listen to the storage change in content and unmark the field when the data is removed while form is still existed: We'll need unmark API for this and bug 1338482, and dispatch the storage-changed message to content as well. Although we had the handler cache in content, it's seems not possible to iterate though all the details and unmark the fields since the cache is weakmap. I'm not sure if there's any API could query all the frames in content, or maybe we can dispatch the storage changes to frames for triggering the cache updating.

Hi Matt, do you have any thoughts about the step (2)? I don't think both of my proposals are perfect solutions ... I will start (1) first because they are unrelated.
Flags: needinfo?(MattN+bmo)
Comment on attachment 8840351 [details]
Bug 1338420 - Part 1: Cache profiles in initialProcess and check while identifying fields

Hi Matt, it's the 1st part I mentioned in the previous comment. Unless we can run _identifyAutofillFields asynchronously, I could not think of other better idea right now.
Attachment #8840351 - Flags: feedback?(MattN+bmo)
Attachment #8840351 - Flags: feedback?(MattN+bmo)
Assignee: nobody → schung
Status: NEW → ASSIGNED
Comment on attachment 8840351 [details]
Bug 1338420 - Part 1: Cache profiles in initialProcess and check while identifying fields

https://reviewboard.mozilla.org/r/114856/#review117614

::: browser/extensions/formautofill/FormAutofillContent.jsm:359
(Diff revision 2)
> +  _fieldInProfile(fieldName) {
> +    return Services.cpmm.initialProcessData.profiles.some((profile) => {

This won't handle storage changes since intialProcessData is a clone from process creation time. I also don't think we should be sending all the profile data to the content process like this since it would eventually include things like (encrypted) credit card numbers. Ideally we would only give that to processes that need it. There may also be a performance impact to clone all profiles in each process.

What about the idea I discussed before of having our autocomplete search controller forwarding the search to Form History directly? Otherwise how about sending only the populated field names to the content process and update it when storage changes?
(In reply to Steve Chung [:steveck] from comment #1)
> 2) Listen to the storage change in content and unmark the field when the
> data is removed while form is still existed: We'll need unmark API for this
> and bug 1338482, and dispatch the storage-changed message to content as
> well. Although we had the handler cache in content, it's seems not possible
> to iterate though all the details and unmark the fields since the cache is
> weakmap. I'm not sure if there's any API could query all the frames in
> content, or maybe we can dispatch the storage changes to frames for
> triggering the cache updating.

I think this is an edge case we don't need to worry about and can be lower priority. The easiest solution is forwarding the call to Form History from startSearch or unmarking lazily upon focus or inside startSearch (though the unmarking would be too late in startSearch probably, possibly not too late upon focus if we use a capturing handler?)
Flags: needinfo?(MattN+bmo)
Attachment #8840351 - Flags: feedback?(MattN+bmo) → feedback-
Comment on attachment 8842788 [details]
Bug 1338420 - Part 1: Add initialProcessData autofillSavedFieldNames set and broadcast the changes to content.

Even we can(?) switch the search dynamically at startSearch, I still think we should cache the valid fields to avoid unnecessary profile querying at the beginning.
Attachment #8842788 - Flags: feedback?(MattN+bmo)
Comment on attachment 8842789 [details]
Bug 1338420 - Part 2: Apply form history search at startSearch and clean up redundant code.

I tried to create a form history search instance and called history search's startSearch, but not thing happened... :/ Is there anything I missed?
Attachment #8842789 - Flags: feedback?(MattN+bmo)
Attachment #8842789 - Flags: feedback?(MattN+bmo)
Attachment #8840351 - Attachment is obsolete: true
Comment on attachment 8842789 [details]
Bug 1338420 - Part 2: Apply form history search at startSearch and clean up redundant code.

https://reviewboard.mozilla.org/r/116572/#review118568

::: browser/extensions/formautofill/FormAutofillContent.jsm:99
(Diff revision 2)
>      let info = this._serializeInfo(FormAutofillContent.getInputDetails(focusedInput));
>  
> +    if (!Services.cpmm.initialProcessData.existedFields.has(info.fieldName)) {
> +      let formHistory = Cc["@mozilla.org/autocomplete/search;1?name=form-history"].
> +                          createInstance(Ci.nsIAutoCompleteSearch);
> +      formHistory.startSearch(...arguments);

It seems like `formHistory.startSearch` expects a different kind of `listener` argument so we may need to provide a mapping function here instead of passing it right through. See onSearchResult vs. onSearchCompletion
Comment on attachment 8842789 [details]
Bug 1338420 - Part 2: Apply form history search at startSearch and clean up redundant code.

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #12)
> Comment on attachment 8842789 [details]
> Bug 1338420 - Part 2: Apply form history search at startSearch.
> 
> https://reviewboard.mozilla.org/r/116572/#review118568
> 
> ::: browser/extensions/formautofill/FormAutofillContent.jsm:99
> (Diff revision 2)
> >      let info = this._serializeInfo(FormAutofillContent.getInputDetails(focusedInput));
> >  
> > +    if (!Services.cpmm.initialProcessData.existedFields.has(info.fieldName)) {
> > +      let formHistory = Cc["@mozilla.org/autocomplete/search;1?name=form-history"].
> > +                          createInstance(Ci.nsIAutoCompleteSearch);
> > +      formHistory.startSearch(...arguments);
> 
> It seems like `formHistory.startSearch` expects a different kind of
> `listener` argument so we may need to provide a mapping function here
> instead of passing it right through. See onSearchResult vs.
> onSearchCompletion

Sorry, I guess I was wrong. I don't see what's wrong so you'll probably need to debug the C++. This is the right idea though
Attachment #8842789 - Flags: feedback?(MattN+bmo) → feedback+
Comment on attachment 8842789 [details]
Bug 1338420 - Part 2: Apply form history search at startSearch and clean up redundant code.

I found that I need to set the AutofillProfileAutoCompleteSearch as the "this" in onSearchResult instead of the formHistory one, and it'll involve some additional work while filling the profile. So I introduced the commit about adding initial state for every detail info(which you've reviewed few days ago).
Attachment #8842789 - Flags: feedback?(MattN+bmo)
Comment on attachment 8843256 [details]
Bug 1338420 - Part 2: Add initial state for field details.

https://reviewboard.mozilla.org/r/117052/#review118628

::: browser/extensions/formautofill/FormAutofillHandler.jsm:87
(Diff revision 1)
>          section: info.section,
>          addressType: info.addressType,
>          contactType: info.contactType,
>          fieldName: info.fieldName,
> +        state: null,
> +        element, // TODO: Apply Cu.getWeakReference once we figure out the issue while marking.

It'll return an error while the mark API is call with weak reference element: NS_ERROR_XPC_BAD_CONVERT_JS: Could not convert JavaScript argument arg 0 [nsIFormFillController.markAsAutofillField]. Any idea about this error message?
(In reply to Steve Chung [:steveck] from comment #17)
> Comment on attachment 8843256 [details]
> Bug 1338420 - Part 2: Add initial state for field details.
> 
> https://reviewboard.mozilla.org/r/117052/#review118628
> 
> ::: browser/extensions/formautofill/FormAutofillHandler.jsm:87
> (Diff revision 1)
> >          section: info.section,
> >          addressType: info.addressType,
> >          contactType: info.contactType,
> >          fieldName: info.fieldName,
> > +        state: null,
> > +        element, // TODO: Apply Cu.getWeakReference once we figure out the issue while marking.
> 
> It'll return an error while the mark API is call with weak reference
> element: NS_ERROR_XPC_BAD_CONVERT_JS: Could not convert JavaScript argument
> arg 0 [nsIFormFillController.markAsAutofillField]. Any idea about this error
> message?

You need to call .get() on the weak reference to get a strong reference whenever you want to use a weak reference.
Attachment #8842788 - Flags: feedback?(lchang)
Attachment #8842789 - Flags: feedback?(lchang)
Attachment #8843256 - Flags: feedback?(lchang)
Hi Luke, since MattN might not have enough bandwidth to clean up the review queue, it would be great if you have free cycle to take a glance at there patches at first. Please let me know if you have any problem while reading the code, thanks!
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #21)
> (In reply to Steve Chung [:steveck] from comment #17)
> > Comment on attachment 8843256 [details]
> > Bug 1338420 - Part 2: Add initial state for field details.
> > 
> > https://reviewboard.mozilla.org/r/117052/#review118628
> > 
> > ::: browser/extensions/formautofill/FormAutofillHandler.jsm:87
> > (Diff revision 1)
> > >          section: info.section,
> > >          addressType: info.addressType,
> > >          contactType: info.contactType,
> > >          fieldName: info.fieldName,
> > > +        state: null,
> > > +        element, // TODO: Apply Cu.getWeakReference once we figure out the issue while marking.
> > 
> > It'll return an error while the mark API is call with weak reference
> > element: NS_ERROR_XPC_BAD_CONVERT_JS: Could not convert JavaScript argument
> > arg 0 [nsIFormFillController.markAsAutofillField]. Any idea about this error
> > message?
> 
> You need to call .get() on the weak reference to get a strong reference
> whenever you want to use a weak reference.

Ok, then it'll work. But it means that we'll need to update every element referencing in content process(and test as well). I think it'll be better to split weakRef into another bug.
Attachment #8842789 - Flags: feedback?(lchang)
Attachment #8843256 - Flags: feedback?(lchang)
Comment on attachment 8842788 [details]
Bug 1338420 - Part 1: Add initialProcessData autofillSavedFieldNames set and broadcast the changes to content.

https://reviewboard.mozilla.org/r/116570/#review119998

::: browser/extensions/formautofill/FormAutofillParent.jsm:54
(Diff revision 2)
>  this.log = null;
>  FormAutofillUtils.defineLazyLogGetter(this, this.EXPORTED_SYMBOLS[0]);
>  
>  const PROFILE_JSON_FILE_NAME = "autofill-profiles.json";
>  const ENABLED_PREF = "browser.formautofill.enabled";
> +const INTERAL_FIELDS =

`INTERNAL_FIELDS`?

::: browser/extensions/formautofill/FormAutofillParent.jsm:250
(Diff revision 2)
> +      Services.ppmm.initialProcessData.existedFields.clear();
> +    }
> +
> +    this._profileStore.getAll().forEach((profile) => {
> +      Object.keys(profile).forEach((field) =>
> +        Services.ppmm.initialProcessData.existedFields.add(field));

We might need to check `profile[field]` isn't empty before adding it to the set.
Comment on attachment 8842788 [details]
Bug 1338420 - Part 1: Add initialProcessData autofillSavedFieldNames set and broadcast the changes to content.

https://reviewboard.mozilla.org/r/116570/#review120256

Hi Steve, sorry for the delay, I have mostly naming questions except for "Are we not also planning to update this state in existing processes?"

::: browser/extensions/formautofill/FormAutofillParent.jsm:54
(Diff revision 2)
>  this.log = null;
>  FormAutofillUtils.defineLazyLogGetter(this, this.EXPORTED_SYMBOLS[0]);
>  
>  const PROFILE_JSON_FILE_NAME = "autofill-profiles.json";
>  const ENABLED_PREF = "browser.formautofill.enabled";
> +const INTERAL_FIELDS =

What about defining this as `ProfileStorage.prototype.INTERNAL_FIELDS`?

::: browser/extensions/formautofill/FormAutofillParent.jsm:241
(Diff revision 2)
>      }
>  
>      target.sendAsyncMessage("FormAutofill:Profiles", profiles);
>    },
> +
> +  _updateValidFields() {

How about `_updateKnownFieldNames` or `_updateSavedFieldNames`? I think "valid" may be confusing as people may think about whether a field is valid? Unless that's intentional and you were thinking that we would do other validation on the stored fields? If so, then I think `_updateValidProfileFieldNames` is longer but more clear.

::: browser/extensions/formautofill/FormAutofillParent.jsm:242
(Diff revision 2)
> +    if (!Services.ppmm.initialProcessData.existedFields) {
> +      Services.ppmm.initialProcessData.existedFields = new Set();

I think this should be clearer too. Remember that `Services.ppmm.initialProcessData` is shared across all of Firefox so it's too generic. Maybe we should keep the "autofill" prefix like our other property but also be more verbose: e.g. `.autofillSavedFieldNames`

::: browser/extensions/formautofill/FormAutofillParent.jsm:249
(Diff revision 2)
> +    } else {
> +      Services.ppmm.initialProcessData.existedFields.clear();
> +    }
> +
> +    this._profileStore.getAll().forEach((profile) => {
> +      Object.keys(profile).forEach((field) =>

Nit: s/field/fieldName/ to make it more clear that it's the field name not the value?

::: browser/extensions/formautofill/FormAutofillParent.jsm:250
(Diff revision 2)
> +      Services.ppmm.initialProcessData.existedFields.clear();
> +    }
> +
> +    this._profileStore.getAll().forEach((profile) => {
> +      Object.keys(profile).forEach((field) =>
> +        Services.ppmm.initialProcessData.existedFields.add(field));

Are we not also planning to update this state in existing processes?

::: browser/extensions/formautofill/FormAutofillParent.jsm:253
(Diff revision 2)
> +    this._profileStore.getAll().forEach((profile) => {
> +      Object.keys(profile).forEach((field) =>
> +        Services.ppmm.initialProcessData.existedFields.add(field));
> +    });
> +
> +    // Remove the interal guid and metadata fields.

The same typo that Luke pointed out is here too
Attachment #8842788 - Flags: review?(MattN+bmo)
Comment on attachment 8842789 [details]
Bug 1338420 - Part 2: Apply form history search at startSearch and clean up redundant code.

https://reviewboard.mozilla.org/r/116572/#review120260

::: browser/extensions/formautofill/FormAutofillContent.jsm:209
(Diff revision 5)
> +        if (FormAutofillContent.getInputDetails(focusedInput).state == "fall-back") {
> +          // Early exit if focused input will fall back to history search.
> +          break;
> +        }

I think the check `this._lastAutoCompleteResult.getStyleAt(selectedIndex) != "autofill-profile"` already handles this case, right? What else will we need the `state` for?
Attachment #8842789 - Flags: review?(MattN+bmo)
Comment on attachment 8843256 [details]
Bug 1338420 - Part 2: Add initial state for field details.

https://reviewboard.mozilla.org/r/117052/#review120258

::: browser/extensions/formautofill/FormAutofillHandler.jsm
(Diff revision 3)
> -   * @returns {Array<Object>} 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() {
> -    let autofillData = [];

Don't you need to clear `this.fieldDetails`?

::: browser/extensions/formautofill/FormAutofillHandler.jsm:63
(Diff revision 3)
> -
>      for (let element of this.form.elements) {
>        // Exclude elements to which no autocomplete field has been assigned.
>        let info = FormAutofillHeuristics.getInfo(element);
>        if (!info) {
> +        log.debug("Not collecting because no info from heuristics");

Isn't this currently quite spammy since most sites don't implement @autocomplete.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:82
(Diff revision 3)
> -      let inputFormat = {
> +      let formatWithElement = {
>          section: info.section,
>          addressType: info.addressType,
>          contactType: info.contactType,
>          fieldName: info.fieldName,
> +        state: null,

See my question in part 3 about where else we'll need this since I don't believe we need it yet.
Attachment #8843256 - Flags: review?(MattN+bmo)
Comment on attachment 8842789 [details]
Bug 1338420 - Part 2: Apply form history search at startSearch and clean up redundant code.

https://reviewboard.mozilla.org/r/116572/#review120260

> I think the check `this._lastAutoCompleteResult.getStyleAt(selectedIndex) != "autofill-profile"` already handles this case, right? What else will we need the `state` for?

It's true that we could leverage lastAutoCompleteResult here. But for the state, I think it would be nice for Bug 1338482 (although it's still not must have). I'll explain in patch 2
Comment on attachment 8843256 [details]
Bug 1338420 - Part 2: Add initial state for field details.

https://reviewboard.mozilla.org/r/117052/#review120258

> Isn't this currently quite spammy since most sites don't implement @autocomplete.

Since FormAutofillHeuristics will actually return the information based on heuristic instead of @autocomplete, I thought it would be fine for having a debug message like this. But you're right it would add lots of noises right now and I'm fine if you prefer to remove it. Maybe the debug message could be added in FormAutofillHeuristics.getInfo.

> See my question in part 3 about where else we'll need this since I don't believe we need it yet.

This idea was picked from Bug 1338482, because having a state for each field could give us more flexibility to set fallback for specific form. But since UX decide that once the form is filled automatically, all the fields will need to fall back to history search. Maybe we could only check whether the profile guid is set in Bug 1338482 because I could not think of any other cases(including input backround setting) that will need state for field currently.
Attachment #8843256 - Attachment is obsolete: true
Attachment #8843256 - Flags: feedback?(lchang)
Comment on attachment 8842788 [details]
Bug 1338420 - Part 1: Add initialProcessData autofillSavedFieldNames set and broadcast the changes to content.

https://reviewboard.mozilla.org/r/116570/#review120256

> Are we not also planning to update this state in existing processes?

I'm not quite sure what you meant here... If you meant the general state like autofillEnabled, it'll be updated while profile changed as well.
Comment on attachment 8842788 [details]
Bug 1338420 - Part 1: Add initialProcessData autofillSavedFieldNames set and broadcast the changes to content.

https://reviewboard.mozilla.org/r/116570/#review120256

> I'm not quite sure what you meant here... If you meant the general state like autofillEnabled, it'll be updated while profile changed as well.

I mean that I don't see where existing processes are being told when `autofillSavedFieldNames` changes since `Services.ppmm.initialProcessData` is cloned at process creation time. For `_enabled` we use `Services.ppmm.broadcastAsyncMessage("FormAutofill:enabledStatus", this._enabled);` but you're not broadcasting changes to `autofillSavedFieldNames` from what I can tell.
Comment on attachment 8842788 [details]
Bug 1338420 - Part 1: Add initialProcessData autofillSavedFieldNames set and broadcast the changes to content.

https://reviewboard.mozilla.org/r/116570/#review119998

> We might need to check `profile[field]` isn't empty before adding it to the set.

I think this wasn't addressed yet.
Comment on attachment 8842789 [details]
Bug 1338420 - Part 2: Apply form history search at startSearch and clean up redundant code.

https://reviewboard.mozilla.org/r/116572/#review120826

::: browser/extensions/formautofill/FormAutofillContent.jsm:96
(Diff revision 6)
>      this.log.debug("startSearch: for", searchString, "with input", formFillController.focusedInput);
>      let focusedInput = formFillController.focusedInput;
>      this.forceStop = false;
> -    let info = this._serializeInfo(FormAutofillContent.getInputDetails(focusedInput));
> +    let info = FormAutofillContent.getInputDetails(focusedInput);
> +
> +    if (!Services.cpmm.initialProcessData.autofillSavedFieldNames.has(info.fieldName)) {

As I mentioned on part 1 this needs to use the live value of `autofillSavedFieldNames` which will probably need to be broadcast to the child processes like we do with the enabled status.

::: browser/extensions/formautofill/FormAutofillHandler.jsm
(Diff revision 6)
>   * Handles profile autofill for a DOM Form element.
>   * @param {FormLike} form Form that need to be auto filled
>   */
>  function FormAutofillHandler(form) {
>    this.form = form;
> -  this.fieldDetails = [];

I think it's good to leave this here too so that the shape of the JS Object doesn't change. It also makes it more clear that fieldDetails is an array that will be populated if someone inspects a new instance without calling `collectFormFields`.
Attachment #8842789 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8842788 [details]
Bug 1338420 - Part 1: Add initialProcessData autofillSavedFieldNames set and broadcast the changes to content.

https://reviewboard.mozilla.org/r/116570/#review120814

::: browser/extensions/formautofill/FormAutofillParent.jsm:240
(Diff revision 3)
>  
>      target.sendAsyncMessage("FormAutofill:Profiles", profiles);
>    },
> +
> +  _updateKnownFieldNames() {
> +    if (!Services.ppmm.initialProcessData.autofillSavedFieldNames) {

Thanks. Please remember to update the commit message.
Attachment #8842788 - Flags: review?(MattN+bmo)
Comment on attachment 8842788 [details]
Bug 1338420 - Part 1: Add initialProcessData autofillSavedFieldNames set and broadcast the changes to content.

https://reviewboard.mozilla.org/r/116570/#review122248

::: browser/extensions/formautofill/FormAutofillContent.jsm:254
(Diff revision 5)
> +   * Field name set that represents the field has corresponding data saved.
> +   */
> +  savedFieldNames: null,

"@type {Set} of the fields with usable values in any saved profile"

::: browser/extensions/formautofill/FormAutofillContent.jsm:261
(Diff revision 5)
> -    Services.cpmm.addMessageListener("FormAutofill:enabledStatus", (result) => {
> -      if (result.data) {
> +    Services.cpmm.addMessageListener("FormAutofill:enabledStatus", this);
> +    Services.cpmm.addMessageListener("FormAutofill:knownFieldNames", this);

FWIW I would have been fine using the same message (with both pieces of data) if you wanted. Up to you.
Attachment #8842788 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8842788 [details]
Bug 1338420 - Part 1: Add initialProcessData autofillSavedFieldNames set and broadcast the changes to content.

https://reviewboard.mozilla.org/r/116570/#review122250

::: browser/extensions/formautofill/FormAutofillContent.jsm:262
(Diff revision 5)
> +
>    init() {
>      FormAutofillUtils.defineLazyLogGetter(this, "FormAutofillContent");
>  
> -    Services.cpmm.addMessageListener("FormAutofill:enabledStatus", (result) => {
> -      if (result.data) {
> +    Services.cpmm.addMessageListener("FormAutofill:enabledStatus", this);
> +    Services.cpmm.addMessageListener("FormAutofill:knownFieldNames", this);

Nit: Can we used `[Ss]avedFieldNames` everywhere (including the test file name) to be consistent?
Thanks for all the reviews!
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f7605a47342d
Part 1: Add initialProcessData autofillSavedFieldNames set and broadcast the changes to content. r=MattN
https://hg.mozilla.org/integration/autoland/rev/80a584704a04
Part 2: Apply form history search at startSearch and clean up redundant code. r=MattN
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f7605a47342d
https://hg.mozilla.org/mozilla-central/rev/80a584704a04
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Blocks: 1348757
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: