[IBAN local save] Add metrics for add, edit and remove IBAN. [chromium/src : main]

Qihui Zhao (Gerrit)

unread,

Jan 27, 2023, 1:05:55 PM1/27/23

to Vinny Persky, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, michaelpg+wa...@chromium.org

Attention is currently required from: Vinny Persky.

Qihui Zhao would like Vinny Persky to review this change.

View Change

[IBAN local save] Add metrics for add, edit and remove IBAN.

This CL adds metrics for:
1. Add an new IBAN with/without nickname.
2. Edit an existing IBAN with/without nickname.
3. Delete an existing IBAN.

Bug:1349109
Change-Id: Ic8b012a2e35c81212977fd81de26038ba2f34bac
---
M chrome/browser/extensions/api/autofill_private/autofill_private_api.cc
M chrome/browser/resources/settings/autofill_page/payments_section.ts
M tools/metrics/actions/actions.xml
3 files changed, 68 insertions(+), 0 deletions(-)

To view, visit change 4200848. To unsubscribe, or for help writing mail filters, visit settings.

Qihui Zhao (Gerrit)

unread,

Jan 27, 2023, 1:06:02 PM1/27/23

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, michaelpg+wa...@chromium.org, Vinny Persky, Tricium, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Vinny Persky.

Patch set 2:Commit-Queue +1

View Change

To view, visit change 4200848. To unsubscribe, or for help writing mail filters, visit settings.

Vinny Persky (Gerrit)

unread,

Jan 30, 2023, 2:11:16 PM1/30/23

to Qihui Zhao, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, michaelpg+wa...@chromium.org, Tricium, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Qihui Zhao.

Patch set 2:Code-Review +1

View Change

1 comment:

Patchset:
- Patch Set #2:
  Thanks! Going to rely on Siyu's expertise for this settings-page metrics logging.

To view, visit change 4200848. To unsubscribe, or for help writing mail filters, visit settings.

Qihui Zhao (Gerrit)

unread,

Jan 30, 2023, 2:44:18 PM1/30/23

to Siyu An, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, michaelpg+wa...@chromium.org, Vinny Persky

Attention is currently required from: Siyu An.

Qihui Zhao would like Siyu An to review this change.

View Change

[IBAN local save] Add metrics for add, edit and remove IBAN.

This CL adds metrics for:
1. Add an new IBAN with/without nickname.
2. Edit an existing IBAN with/without nickname.
3. Delete an existing IBAN.

Bug:1349109
Change-Id: Ic8b012a2e35c81212977fd81de26038ba2f34bac
---
M chrome/browser/extensions/api/autofill_private/autofill_private_api.cc
M chrome/browser/resources/settings/autofill_page/payments_section.ts
M tools/metrics/actions/actions.xml
3 files changed, 68 insertions(+), 0 deletions(-)

To view, visit change 4200848. To unsubscribe, or for help writing mail filters, visit settings.

Siyu An (Gerrit)

unread,

Jan 30, 2023, 7:22:29 PM1/30/23

to Qihui Zhao, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, michaelpg+wa...@chromium.org, Vinny Persky, Tricium, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Qihui Zhao.

View Change

1 comment:

File chrome/browser/resources/settings/autofill_page/payments_section.ts:
- Patch Set #2, Line 454: AutofillIbansDeleted
  This ideally should also be logged in autofill_private_api.cc as other actions. Right now we don't log credit card/address removal. You should be able to fix that all together in the RemoveEntry

To view, visit change 4200848. To unsubscribe, or for help writing mail filters, visit settings.

Qihui Zhao (Gerrit)

unread,

Jan 30, 2023, 8:37:39 PM1/30/23

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, michaelpg+wa...@chromium.org, Siyu An, Vinny Persky, Tricium, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Siyu An, Vinny Persky.

View Change

2 comments:

Patchset:
- Patch Set #3:
  Thanks!
File chrome/browser/resources/settings/autofill_page/payments_section.ts:

- Patch Set #2, Line 454: AutofillIbansDeleted

- This ideally should also be logged in autofill_private_api.cc as other actions. […]
  Remove delete metric for now, and will do that in the following CL.

To view, visit change 4200848. To unsubscribe, or for help writing mail filters, visit settings.

Vinny Persky (Gerrit)

unread,

Jan 30, 2023, 8:41:09 PM1/30/23

to Qihui Zhao, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, michaelpg+wa...@chromium.org, Siyu An, Tricium, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Qihui Zhao, Siyu An.

Patch set 3:Code-Review +1

View Change

To view, visit change 4200848. To unsubscribe, or for help writing mail filters, visit settings.

Qihui Zhao (Gerrit)

unread,

Jan 31, 2023, 12:55:50 PM1/31/23

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, michaelpg+wa...@chromium.org, Vinny Persky, Siyu An, Tricium, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Siyu An.

View Change

1 comment:

File chrome/browser/resources/settings/autofill_page/payments_section.ts:

- Patch Set #2, Line 454: AutofillIbansDeleted

- Remove delete metric for now, and will do that in the following CL.
  Updated the code in autofill_private_api.cc itself and add back the delete metric.

To view, visit change 4200848. To unsubscribe, or for help writing mail filters, visit settings.

Siyu An (Gerrit)

unread,

Jan 31, 2023, 1:10:08 PM1/31/23

to Qihui Zhao, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, michaelpg+wa...@chromium.org, Vinny Persky, Tricium, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Qihui Zhao.

Patch set 5:Code-Review +1

View Change

To view, visit change 4200848. To unsubscribe, or for help writing mail filters, visit settings.

Qihui Zhao (Gerrit)

unread,

Jan 31, 2023, 1:28:15 PM1/31/23

to Mohamed Amir Yosef, Demetrios Papadopoulos, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, michaelpg+wa...@chromium.org, Siyu An, Vinny Persky

Attention is currently required from: Demetrios Papadopoulos, Mohamed Amir Yosef.

Qihui Zhao would like Mohamed Amir Yosef and Demetrios Papadopoulos to review this change.

View Change

[IBAN local save] Add metrics for add, edit and remove IBAN.

This CL adds metrics for:
1. Add an new IBAN with/without nickname.
2. Edit an existing IBAN with/without nickname.
3. Delete an existing IBAN.

Bug:1349109
Change-Id: Ic8b012a2e35c81212977fd81de26038ba2f34bac
---
M chrome/browser/extensions/api/autofill_private/autofill_private_api.cc

M tools/metrics/actions/actions.xml
2 files changed, 71 insertions(+), 0 deletions(-)

To view, visit change 4200848. To unsubscribe, or for help writing mail filters, visit settings.

Qihui Zhao (Gerrit)

unread,

Jan 31, 2023, 1:28:19 PM1/31/23

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, michaelpg+wa...@chromium.org, Demetrios Papadopoulos, Mohamed Amir Yosef, Siyu An, Vinny Persky, Tricium, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Demetrios Papadopoulos, Mohamed Amir Yosef.

View Change

1 comment:

Patchset:
- Patch Set #5:
  Thanks a lot!

To view, visit change 4200848. To unsubscribe, or for help writing mail filters, visit settings.

Demetrios Papadopoulos (Gerrit)

unread,

Jan 31, 2023, 8:19:49 PM1/31/23

to Qihui Zhao, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, michaelpg+wa...@chromium.org, Mohamed Amir Yosef, Siyu An, Vinny Persky, Tricium, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Mohamed Amir Yosef, Qihui Zhao.

View Change

1 comment:

Patchset:

- Patch Set #5:
  Thanks a lot!

- Could you specify which reviewer is being asked to review which files?
  Regarding chrome/browser/extensions/api/autofill_private/autofill_private_api.cc, while I appear in the OWNERs list, I think is best to be reviewed by someone on the autofill team. Happy to stamp afterwards.
  Moreover, I think moving forward it would be best to add OWNERS from the autofill team for chrome/browser/extensions/api/autofill_private/, as I don't think the current OWNERship list is very representative of who is knowledgeable about this part of the codebase.

To view, visit change 4200848. To unsubscribe, or for help writing mail filters, visit settings.

Qihui Zhao (Gerrit)

unread,

Jan 31, 2023, 8:54:10 PM1/31/23

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, michaelpg+wa...@chromium.org, Demetrios Papadopoulos, Mohamed Amir Yosef, Siyu An, Vinny Persky, Tricium, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Mohamed Amir Yosef.

View Change

1 comment:

Patchset:
- Patch Set #5:
  Add mamir@ for tools/metrics/actions/actions.xml and chrome/browser/extensions/api/autofill_private/autofill_private_api.cc
  Add dpapad@ for ownership for chrome/browser/extensions/api/autofill_private/autofill_private_api.cc
  Thanks!

To view, visit change 4200848. To unsubscribe, or for help writing mail filters, visit settings.

Demetrios Papadopoulos (Gerrit)

unread,

Feb 1, 2023, 6:50:56 PM2/1/23

to Qihui Zhao, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, michaelpg+wa...@chromium.org, Mohamed Amir Yosef, Siyu An, Vinny Persky, Tricium, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Mohamed Amir Yosef, Qihui Zhao.

View Change

1 comment:

Patchset:
- Patch Set #5:
  Add mamir@ for tools/metrics/actions/actions. […]
  Happy to stamp, after mamir's approval. Thanks.

To view, visit change 4200848. To unsubscribe, or for help writing mail filters, visit settings.

Mohamed Amir Yosef (Gerrit)

unread,

Feb 2, 2023, 12:06:33 PM2/2/23

to Qihui Zhao, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, michaelpg+wa...@chromium.org, Demetrios Papadopoulos, Siyu An, Vinny Persky, Tricium, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Qihui Zhao.

View Change

4 comments:

Patchset:
- Patch Set #5:
  Thank you!
  I have added some comments!
  Would it be possible to add test for that?
File chrome/browser/extensions/api/autofill_private/autofill_private_api.cc:
- Patch Set #5, Line 456: Ibans
  Why it's all plural? Ibans?
  Why not just Iban?
  Applies everywhere.
- Patch Set #5, Line 651: if (existing_iban->nickname() != iban.nickname()) {
  Should we distinguish between added and removed Nicknames?
File tools/metrics/actions/actions.xml:
- Patch Set #5, Line 4229: settings
  Should we be consistent and call this chrome://settings/payments too?

To view, visit change 4200848. To unsubscribe, or for help writing mail filters, visit settings.

Qihui Zhao (Gerrit)

unread,

Feb 3, 2023, 12:02:21 AM2/3/23

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, michaelpg+wa...@chromium.org, Demetrios Papadopoulos, Mohamed Amir Yosef, Siyu An, Vinny Persky, Tricium, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Mohamed Amir Yosef, Siyu An, Vinny Persky.

View Change

4 comments:

Patchset:
- Patch Set #6:
  Thanks!
File chrome/browser/extensions/api/autofill_private/autofill_private_api.cc:

- Patch Set #5, Line 456: Ibans

- Why it's all plural? Ibans? […]
  Done

- Patch Set #5, Line 651: if (existing_iban->nickname() != iban.nickname()) {
  Should we distinguish between added and removed Nicknames?

- I think here we only care the action that updates the nickname, and we are logging add IBAN with nickname or not.
File tools/metrics/actions/actions.xml:

- Patch Set #5, Line 4229: settings
  Should we be consistent and call this chrome://settings/payments too?

- Done

To view, visit change 4200848. To unsubscribe, or for help writing mail filters, visit settings.

Vinny Persky (Gerrit)

unread,

Feb 3, 2023, 1:24:39 AM2/3/23

to Qihui Zhao, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, michaelpg+wa...@chromium.org, Demetrios Papadopoulos, Mohamed Amir Yosef, Siyu An, Tricium, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Mohamed Amir Yosef, Qihui Zhao, Siyu An.

Patch set 6:Code-Review +1

View Change

1 comment:

Patchset:
- Patch Set #6:
  PS 3->6 LGTM

To view, visit change 4200848. To unsubscribe, or for help writing mail filters, visit settings.

Mohamed Amir Yosef (Gerrit)

unread,

Feb 3, 2023, 2:04:32 AM2/3/23

to Qihui Zhao, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, michaelpg+wa...@chromium.org, Vinny Persky, Demetrios Papadopoulos, Siyu An, Tricium, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Qihui Zhao, Siyu An.

Patch set 6:Code-Review +1

View Change

5 comments:

Patchset:
- Patch Set #6:
  Thank you!
  LGTM % nites
File chrome/browser/extensions/api/autofill_private/autofill_private_api.cc:
- Patch Set #6, Line 455:
```
  if (personal_data->GetIBANByGUID(parameters->guid)) {
    base::RecordAction(base::UserMetricsAction("AutofillIbanDeleted"));
  }
```
  Out of curiosity, how does this work?
  How is it safe to query the PersonalDataManager using `GetIBANByGUID()` after `RemoveByGUID()` has been invoked already?
  I would have assumed that line 455 should be before 453
- Patch Set #6, Line 646: }
  optional: early return here to avoid long `if` blocks
  and switch the below to `if` instead of `else if` too.
File chrome/browser/extensions/api/autofill_private/autofill_private_apitest.cc:
- Patch Set #6, Line 130: EXPECT_EQ(1, user_action_tester.GetActionCount("AutofillIbanAdded"));
  This isn't relevant for this test.
File chrome/test/data/extensions/api_test/autofill_private/test.js:
- Patch Set #6, Line 31: console.log("ttestttt");
  remove?

To view, visit change 4200848. To unsubscribe, or for help writing mail filters, visit settings.

Qihui Zhao (Gerrit)

unread,

Feb 3, 2023, 11:28:54 AM2/3/23

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, michaelpg+wa...@chromium.org, Mohamed Amir Yosef, Vinny Persky, Demetrios Papadopoulos, Siyu An, Tricium, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Siyu An.

View Change

5 comments:

Patchset:
- Patch Set #7:
  Thanks for the review!
File chrome/browser/extensions/api/autofill_private/autofill_private_api.cc:

Patch Set #6, Line 455:

  if (personal_data->GetIBANByGUID(parameters->guid)) {
    base::RecordAction(base::UserMetricsAction("AutofillIbanDeleted"));
  }

- Out of curiosity, how does this work? […]
  good catch! Yep, the local IBANs are updated async (https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/personal_data_manager.cc;drc=f775d850d7f5beebd9308f5d02111d6069083b28;l=508) so that's why.
  Move this ahead of remove action.

- Patch Set #6, Line 646: }

- optional: early return here to avoid long `if` blocks […]
  Done
File chrome/browser/extensions/api/autofill_private/autofill_private_apitest.cc:

- Patch Set #6, Line 130: EXPECT_EQ(1, user_action_tester.GetActionCount("AutofillIbanAdded"));
  This isn't relevant for this test.

- Done
File chrome/test/data/extensions/api_test/autofill_private/test.js:

- Patch Set #6, Line 31: console.log("ttestttt");
  remove?

- Done

To view, visit change 4200848. To unsubscribe, or for help writing mail filters, visit settings.

Siyu An (Gerrit)

unread,

Feb 3, 2023, 2:32:25 PM2/3/23

to Qihui Zhao, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, michaelpg+wa...@chromium.org, Mohamed Amir Yosef, Vinny Persky, Demetrios Papadopoulos, Tricium, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Demetrios Papadopoulos, Qihui Zhao.

Patch set 8:Code-Review +1

View Change

To view, visit change 4200848. To unsubscribe, or for help writing mail filters, visit settings.

Demetrios Papadopoulos (Gerrit)

unread,

Feb 6, 2023, 2:26:06 PM2/6/23

to Qihui Zhao, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, michaelpg+wa...@chromium.org, Siyu An, Mohamed Amir Yosef, Vinny Persky, Tricium, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Qihui Zhao.

Patch set 8:Code-Review +1

View Change

1 comment:

Patchset:
- Patch Set #5:

- Happy to stamp, after mamir's approval. Thanks.

- RS LGTM.

To view, visit change 4200848. To unsubscribe, or for help writing mail filters, visit settings.

Qihui Zhao (Gerrit)

unread,

Feb 6, 2023, 3:17:00 PM2/6/23

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, michaelpg+wa...@chromium.org, Demetrios Papadopoulos, Siyu An, Mohamed Amir Yosef, Vinny Persky, Tricium, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Qihui Zhao.

Patch set 8:Commit-Queue +2

View Change

To view, visit change 4200848. To unsubscribe, or for help writing mail filters, visit settings.

Findit (Gerrit)

unread,

Feb 6, 2023, 4:05:52 PM2/6/23

to Qihui Zhao, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, michaelpg+wa...@chromium.org, Demetrios Papadopoulos, Siyu An, Mohamed Amir Yosef, Vinny Persky, Tricium, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Qihui Zhao.

Patch set 8:Code-Coverage +1

View Change

To view, visit change 4200848. To unsubscribe, or for help writing mail filters, visit settings.

Qihui Zhao (Gerrit)

unread,

Feb 6, 2023, 5:13:36 PM2/6/23

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, michaelpg+wa...@chromium.org, Findit, Demetrios Papadopoulos, Siyu An, Mohamed Amir Yosef, Vinny Persky, Tricium, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Qihui Zhao.

Patch set 8:Commit-Queue +2

View Change

To view, visit change 4200848. To unsubscribe, or for help writing mail filters, visit settings.

Chromium LUCI CQ (Gerrit)

unread,

Feb 6, 2023, 8:05:55 PM2/6/23

to Qihui Zhao, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, michaelpg+wa...@chromium.org, Findit, Demetrios Papadopoulos, Siyu An, Mohamed Amir Yosef, Vinny Persky, Tricium, Chromium Metrics Reviews, chromium...@chromium.org

Chromium LUCI CQ submitted this change.

View Change

Approvals:
  Findit: Ok
  Qihui Zhao: Commit
  Siyu An: Looks good to me
  Demetrios Papadopoulos: Looks good to me
  Mohamed Amir Yosef: Looks good to me
  Vinny Persky: Looks good to me

[IBAN local save] Add metrics for add, edit and remove IBAN.

This CL adds metrics for:
1. Add an new IBAN with/without nickname.
2. Edit an existing IBAN with/without nickname.
3. Delete an existing IBAN.

Bug: 1349109
Change-Id: Ic8b012a2e35c81212977fd81de26038ba2f34bac

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4200848
Reviewed-by: Vinny Persky <vinny...@google.com>
Reviewed-by: Siyu An <si...@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpa...@chromium.org>
Reviewed-by: Mohamed Amir Yosef <ma...@chromium.org>
Code-Coverage: Findit <findit...@appspot.gserviceaccount.com>
Commit-Queue: Qihui Zhao <qihu...@google.com>
Cr-Commit-Position: refs/heads/main@{#1101896}
---
M chrome/browser/extensions/api/autofill_private/autofill_private_api.cc
M chrome/browser/extensions/api/autofill_private/autofill_private_apitest.cc
M chrome/test/data/extensions/api_test/autofill_private/test.js
M tools/metrics/actions/actions.xml
4 files changed, 227 insertions(+), 58 deletions(-)

To view, visit change 4200848. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward