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

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement, P3)

Description

[Sean Lee [:seanlee][:weilonge]]

FormAutofill parent needs to know the selected profile is submiited and stores the last used time or other meta information into ProfileStroage.

Comment 1

[Steve Chung [:steveck]]

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.

Comment 2

[Steve Chung [:steveck]]

Attachment: Bug 1339740 - Trigger notifyUsed API during form submission.

Review commit: https://reviewboard.mozilla.org/r/150428/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150428/

Comment 3

[Luke Chang [:lchang] (inactive)]

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.

Comment 4

[Steve Chung [:steveck]]

Comment on attachment 8879091 [details]
Bug 1339740 - Trigger notifyUsed API during form submission.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150428/diff/1-2/

Comment 5

[Steve Chung [:steveck]]

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 6

[Luke Chang [:lchang] (inactive)]

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 7

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8879091 [details]
Bug 1339740 - Trigger notifyUsed API during form submission.

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

Comment 8

[Steve Chung [:steveck]]

Thanks for the review!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2f3f2c47ac3c
Trigger notifyUsed API during form submission. r=lchang

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2f3f2c47ac3c

Comment 11

[Adrian Florinescu [:aflorinescu]]

As agreed in San-Francisco, ni for Vance to clarify this bug for QA and maybe provide some steps to test this.