Closed Bug 1339740 Opened 3 years ago Closed 2 years ago

Feedback the profile usage (e.g. last used time) to parent for the persistence in ProfileStorage when submitting a form

Categories

(Toolkit :: Form Manager, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: selee, Assigned: steveck, NeedInfo)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:M3])

Attachments

(1 file)

FormAutofill parent needs to know the selected profile is submiited and stores the last used time or other meta information into ProfileStroage.
Blocks: 1339745
Whiteboard: [form autofill:M1] → [form autofill:MVP]
Whiteboard: [form autofill:MVP] → [form autofill:M3]
Assignee: nobody → schung
Based on the discussion with Juwie, we'll update the last used time/ used times for following cases:
- manual fill:
    - If the submitted address is mergeable, all the merged addresses should update usage metadata
    - Other new add address should update usage metadata as well since add API didn't touch usage metadata.
- Autofill:
    - No conflict: update the selected profile's usage metadata.
    - Conflict and create new one: Update the new address' usage metadata and leave the old one unchanged.
    - Conflict and update: Update the modified address.
Attachment #8879091 - Flags: review?(MattN+bmo) → review?(lchang)
Comment on attachment 8879091 [details]
Bug 1339740 - Trigger notifyUsed API during form submission.

https://reviewboard.mozilla.org/r/150428/#review155558

::: browser/extensions/formautofill/FormAutofillParent.jsm:294
(Diff revision 1)
> -        if (!this.profileStorage.addresses.mergeToStorage(address.record)) {
> -          this.profileStorage.addresses.add(address.record);
> +        let mergedGUIDs = this.profileStorage.addresses.mergeToStorage(address.record);
> +        if (mergedGUIDs.length == 0) {
> +          let guid = this.profileStorage.addresses.add(address.record);
> +          this.profileStorage.addresses.notifyUsed(guid);
>          }
> +        mergedGUIDs.forEach(guid => this.profileStorage.addresses.notifyUsed(guid));

I prefer put it in an `else` block, but I'd rather suggest you can rename `mergedGUIDs` to something like `changedGUIDs` and do as below:

```js
if (!changedGUIDs.length) {
  let guid = this.profileStorage.addresses.add(address.record);
  changedGUIDs.push(guid);
}
```

So we can reuse this line and avoid duplicate code when there are further actions that need to be applied to both merged and added cases in the future.

::: browser/extensions/formautofill/FormAutofillParent.jsm:298
(Diff revision 1)
> -      this.profileStorage.addresses.add(address.record);
> +      let guid = this.profileStorage.addresses.add(address.record);
> +      this.profileStorage.addresses.notifyUsed(guid);

Although it's not related to this patch, I think we should try to merge profiles as well even it's the first-time use.
Attachment #8879091 - Flags: review?(lchang)
Comment on attachment 8879091 [details]
Bug 1339740 - Trigger notifyUsed API during form submission.

https://reviewboard.mozilla.org/r/150428/#review155558

> Although it's not related to this patch, I think we should try to merge profiles as well even it's the first-time use.

yeah we can address it in https://bugzilla.mozilla.org/show_bug.cgi?id=1374508
Comment on attachment 8879091 [details]
Bug 1339740 - Trigger notifyUsed API during form submission.

https://reviewboard.mozilla.org/r/150428/#review155558

> yeah we can address it in https://bugzilla.mozilla.org/show_bug.cgi?id=1374508

I'm fine with that. Remember to comment on bug 1374508 to remind us of this.
Comment on attachment 8879091 [details]
Bug 1339740 - Trigger notifyUsed API during form submission.

https://reviewboard.mozilla.org/r/150428/#review156038
Attachment #8879091 - Flags: review?(lchang) → review+
Thanks for the review!
Keywords: checkin-needed
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2f3f2c47ac3c
Trigger notifyUsed API during form submission. r=lchang
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2f3f2c47ac3c
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
As agreed in San-Francisco, ni for Vance to clarify this bug for QA and maybe provide some steps to test this.
Flags: needinfo?(vchen)
You need to log in before you can comment on or make changes to this bug.