[Autofill] Replace CreditCard::origin(), IsVerified() with a bool [chromium/src : main]
0 views
Skip to first unread message
Christoph Schwering (Gerrit)
Jul 16, 2026, 6:04:29 PM (6 days ago) Jul 16
to Vinny Persky, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, tmartino+tran...@chromium.org, ios-r...@chromium.org, feature-me...@chromium.org, sloboda...@chromium.org, ios-revie...@chromium.org, marq+...@chromium.org, armalhotra+a...@google.com, browser-comp...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, osaul+aut...@google.com, siashah+au...@chromium.org, siyua+aut...@chromium.org, vinnypersky+...@google.com
Attention needed from Vinny Persky
Christoph Schwering added 2 comments![]( "Open in Gerrit")
File chrome/browser/autofill/android/personal_data_manager_android.cc
Line 133, Patchset 4 (Latest):
card.is_user_confirmed() ? std::string(autofill::kSettingsOrigin)
: std::string(),Christoph Schwering . unresolved
I'll look into propagating the change to Java in a followup.
File components/autofill/core/browser/payments/payments_data_cleaner.cc
Line 30, Patchset 1 (Parent):
void PaymentsDataCleaner::ClearCreditCardNonSettingsOrigins() {Christoph Schwering . unresolved
As the review agent:
By removing this cleanup, we are now relying on cards being saved to the database to clear their non-settings origin strings (lazy migration). Since CreditCardFromStatement already maps any non-settings string to is_user_confirmed = false, this is safe, but it means the DB will contain stale URL strings for a longer period than before.
I think that's partially right, but it doesn't matter:
- AFAICT, today the only origins that we pass to `CreditCard` are `""` and `"Chrome settings"`. So there's nothing to be cleared. I didn't check old code, of course.
- The database schema should be updated to remove the `"origin"` column. That'd implicitly clean up all origins.
Related details
Attention is currently required from:
- Vinny Persky
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christoph Schwering (Gerrit)
Jul 16, 2026, 6:37:09 PM (6 days ago) Jul 16
to Tommy Martino, Vinny Persky, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, tmartino+tran...@chromium.org, ios-r...@chromium.org, feature-me...@chromium.org, sloboda...@chromium.org, ios-revie...@chromium.org, marq+...@chromium.org, armalhotra+a...@google.com, browser-comp...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, osaul+aut...@google.com, siashah+au...@chromium.org, siyua+aut...@chromium.org, vinnypersky+...@google.com
Attention needed from Tommy Martino and Vinny Persky
Christoph Schwering added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 4 (Latest):
Christoph Schwering . resolved
Tommy, could you ptal at //ios?
Related details
Attention is currently required from:
- Tommy Martino
- Vinny Persky
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tommy Martino (Gerrit)
Jul 17, 2026, 10:38:02 AM (5 days ago) Jul 17
to Vinny Persky, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, tmartino+tran...@chromium.org, ios-r...@chromium.org, feature-me...@chromium.org, sloboda...@chromium.org, ios-revie...@chromium.org, marq+...@chromium.org, armalhotra+a...@google.com, browser-comp...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, osaul+aut...@google.com, siashah+au...@chromium.org, siyua+aut...@chromium.org, vinnypersky+...@google.com
Attention needed from Christoph Schwering and Vinny Persky
Tommy Martino voted and added 1 comment![]( "Open in Gerrit")
Votes added by Tommy Martino
| Code-Review | +1 |
1 comment
Patchset-level comments
Related details
Attention is currently required from:
- Christoph Schwering
- Vinny Persky
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christoph Schwering (Gerrit)
Jul 20, 2026, 7:08:31 PM (2 days ago) Jul 20
to Tommy Martino, Vinny Persky, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, tmartino+tran...@chromium.org, ios-r...@chromium.org, feature-me...@chromium.org, sloboda...@chromium.org, ios-revie...@chromium.org, marq+...@chromium.org, armalhotra+a...@google.com, browser-comp...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, osaul+aut...@google.com, siashah+au...@chromium.org, siyua+aut...@chromium.org, vinnypersky+...@google.com
Attention needed from Vinny Persky
Christoph Schwering added 1 comment![]( "Open in Gerrit")
File components/autofill/core/browser/payments/payments_data_cleaner.cc
Line 30, Patchset 1 (Parent):
void PaymentsDataCleaner::ClearCreditCardNonSettingsOrigins() {Christoph Schwering . unresolved
As the review agent:
By removing this cleanup, we are now relying on cards being saved to the database to clear their non-settings origin strings (lazy migration). Since CreditCardFromStatement already maps any non-settings string to is_user_confirmed = false, this is safe, but it means the DB will contain stale URL strings for a longer period than before.
I think that's partially right, but it doesn't matter:
- AFAICT, today the only origins that we pass to `CreditCard` are `""` and `"Chrome settings"`. So there's nothing to be cleared. I didn't check old code, of course.
- The database schema should be updated to remove the `"origin"` column. That'd implicitly clean up all origins.
Christoph Schwering
The database schema should be updated to remove the "origin" column.
s/should/will/: the schema update is in the followup https://crrev.com/c/8112597
Related details
Attention is currently required from:
- Vinny Persky
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vinny Persky (Gerrit)
Jul 21, 2026, 1:02:15 PM (yesterday) Jul 21
to Tommy Martino, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, tmartino+tran...@chromium.org, ios-r...@chromium.org, feature-me...@chromium.org, sloboda...@chromium.org, ios-revie...@chromium.org, marq+...@chromium.org, armalhotra+a...@google.com, browser-comp...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, osaul+aut...@google.com, siashah+au...@chromium.org, siyua+aut...@chromium.org, vinnypersky+...@google.com
Attention needed from Christoph Schwering
Vinny Persky added 1 comment![]( "Open in Gerrit")
File ios/chrome/browser/autofill/model/automation/automation_app_interface.mm
Line 83, Patchset 6 (Parent):
autofill::CreditCard credit_card(
base::Uuid::GenerateRandomV4().AsLowercaseString(),
"https://www.example.com/");Vinny Persky . unresolved
It seems we also set the origin here.. Should this be set as user_confirmed so that we don't change the behavior of these cards?
Related details
Attention is currently required from:
- Christoph Schwering
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christoph Schwering (Gerrit)
Jul 21, 2026, 2:28:11 PM (yesterday) Jul 21
to Tommy Martino, Vinny Persky, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, tmartino+tran...@chromium.org, ios-r...@chromium.org, feature-me...@chromium.org, sloboda...@chromium.org, ios-revie...@chromium.org, marq+...@chromium.org, armalhotra+a...@google.com, browser-comp...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, osaul+aut...@google.com, siashah+au...@chromium.org, siyua+aut...@chromium.org, vinnypersky+...@google.com
Attention needed from Vinny Persky
Christoph Schwering added 1 comment![]( "Open in Gerrit")
File ios/chrome/browser/autofill/model/automation/automation_app_interface.mm
Line 83, Patchset 6 (Parent):
autofill::CreditCard credit_card(
base::Uuid::GenerateRandomV4().AsLowercaseString(),
"https://www.example.com/");Vinny Persky . unresolved
It seems we also set the origin here.. Should this be set as user_confirmed so that we don't change the behavior of these cards?
Christoph Schwering
That'd be wrong:
- We want that `is_user_confirmed()` in the new code is equivalent to `IsVerified()` in the old code.)
- Therefore, in the new code, we need to call `set_is_user_confirmed(true)` iff in the old code, the origin is a non-empty but **in**valid URL. (I find that super confusing.)
- Since `https://www.example.com/` is a valid URL (in the sense that `GURL("https://www.example.com/").is_valid()`), we want `!is_user_confirmed()` here.
Related details
Attention is currently required from:
- Vinny Persky
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vinny Persky (Gerrit)
5:13 PM (4 hours ago) 5:13 PM
to Tommy Martino, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, tmartino+tran...@chromium.org, ios-r...@chromium.org, feature-me...@chromium.org, sloboda...@chromium.org, ios-revie...@chromium.org, marq+...@chromium.org, armalhotra+a...@google.com, browser-comp...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, osaul+aut...@google.com, siashah+au...@chromium.org, siyua+aut...@chromium.org, vinnypersky+...@google.com
Attention needed from Christoph Schwering
Vinny Persky voted and added 1 comment![]( "Open in Gerrit")
Votes added by Vinny Persky
| Code-Review | +1 |
1 comment
File ios/chrome/browser/autofill/model/automation/automation_app_interface.mm
Line 83, Patchset 6 (Parent):
autofill::CreditCard credit_card(
base::Uuid::GenerateRandomV4().AsLowercaseString(),
"https://www.example.com/");Vinny Persky . resolved
Christoph SchweringIt seems we also set the origin here.. Should this be set as user_confirmed so that we don't change the behavior of these cards?
That'd be wrong:
- We want that `is_user_confirmed()` in the new code is equivalent to `IsVerified()` in the old code.)
- Therefore, in the new code, we need to call `set_is_user_confirmed(true)` iff in the old code, the origin is a non-empty but **in**valid URL. (I find that super confusing.)
- Since `https://www.example.com/` is a valid URL (in the sense that `GURL("https://www.example.com/").is_valid()`), we want `!is_user_confirmed()` here.
Vinny Persky
I... totally missed the invalid URL piece.
That's... awful. Thanks for cleaning this up haha
Related details
Attention is currently required from:
- Christoph Schwering
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christoph Schwering (Gerrit)
5:25 PM (4 hours ago) 5:25 PM
to Vinny Persky, Tommy Martino, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, tmartino+tran...@chromium.org, ios-r...@chromium.org, feature-me...@chromium.org, sloboda...@chromium.org, ios-revie...@chromium.org, marq+...@chromium.org, armalhotra+a...@google.com, browser-comp...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, osaul+aut...@google.com, siashah+au...@chromium.org, siyua+aut...@chromium.org, vinnypersky+...@google.com
Christoph Schwering voted and added 3 comments![]( "Open in Gerrit")
Votes added by Christoph Schwering
| Commit-Queue | +2 |
3 comments
Patchset-level comments
Christoph Schwering . resolved
Thanks!
File chrome/browser/autofill/android/personal_data_manager_android.cc
Line 133, Patchset 4:
card.is_user_confirmed() ? std::string(autofill::kSettingsOrigin)
: std::string(),Christoph Schwering . resolved
I'll look into propagating the change to Java in a followup.
Christoph Schwering
Done
File components/autofill/core/browser/payments/payments_data_cleaner.cc
Line 30, Patchset 1 (Parent):
void PaymentsDataCleaner::ClearCreditCardNonSettingsOrigins() {Christoph Schwering . resolved
Christoph SchweringAs the review agent:
By removing this cleanup, we are now relying on cards being saved to the database to clear their non-settings origin strings (lazy migration). Since CreditCardFromStatement already maps any non-settings string to is_user_confirmed = false, this is safe, but it means the DB will contain stale URL strings for a longer period than before.
I think that's partially right, but it doesn't matter:
- AFAICT, today the only origins that we pass to `CreditCard` are `""` and `"Chrome settings"`. So there's nothing to be cleared. I didn't check old code, of course.
- The database schema should be updated to remove the `"origin"` column. That'd implicitly clean up all origins.
The database schema should be updated to remove the "origin" column.
s/should/will/: the schema update is in the followup https://crrev.com/c/8112597
Christoph Schwering
Acknowledged
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
7:21 PM (2 hours ago) 7:21 PM
to Vinny Persky, Tommy Martino, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, tmartino+tran...@chromium.org, ios-r...@chromium.org, feature-me...@chromium.org, sloboda...@chromium.org, ios-revie...@chromium.org, marq+...@chromium.org, armalhotra+a...@google.com, browser-comp...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, osaul+aut...@google.com, siashah+au...@chromium.org, siyua+aut...@chromium.org, vinnypersky+...@google.com
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Unreviewed changes
6 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
Commit message:
[Autofill] Replace CreditCard::origin(), IsVerified() with a bool
The semantics of CreditCard::origin() boils down to a single bool:
origin() == "Chrome settings" iff the user confirmed the card in
chrome://settings.
The documented semantics is even stranger: origin() is a non-empty
invalid URL iff the user confirmed the card in chrome://settings.
The above simplification comes from the actual usage in the code.
See crbug.com/535623365 for a more detailed argument.
This CL therefore replaces CreditCard::origin_ with a single bool.
Followup CLs will simplify UpdateFromImportedCard() and remove the
origin from the PaymentsAutofillTable.
The CL is supposed to make no behavior change.
TAG=agy
CONV=8b078322-df0d-4996-a59b-e2e316eafee0
Bug: 535623365
Change-Id: I02628afb29d82cce97f5b24c32dabde00b467431
Reviewed-by: Vinny Persky <vinny...@google.com>
Commit-Queue: Christoph Schwering <schw...@google.com>
Reviewed-by: Tommy Martino <tmar...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1666696}
Files:
- M chrome/browser/autofill/android/personal_data_manager_android.cc
- M chrome/browser/autofill/captured_sites_test_utils.cc
- M chrome/browser/extensions/api/autofill_private/autofill_private_api.cc
- M components/autofill/core/browser/data_manager/payments/payments_data_manager_unittest.cc
- M components/autofill/core/browser/data_model/payments/credit_card.cc
- M components/autofill/core/browser/data_model/payments/credit_card.h
- M components/autofill/core/browser/data_model/payments/credit_card_unittest.cc
- M components/autofill/core/browser/form_import/payments/payments_form_data_importer_unittest.cc
- M components/autofill/core/browser/payments/payments_data_cleaner.cc
- M components/autofill/core/browser/payments/payments_data_cleaner.h
- M components/autofill/core/browser/payments/payments_data_cleaner_unittest.cc
- M components/autofill/core/browser/suggestions/payments/credit_card_suggestion_generator_unittest.cc
- M components/autofill/core/browser/test_utils/autofill_test_utils.cc
- M components/autofill/core/browser/webdata/payments/payments_autofill_table.cc
- M components/autofill/core/browser/webdata/payments/payments_autofill_table_unittest.cc
- M components/autofill/core/browser/webdata/web_data_service_unittest.cc
- M ios/chrome/browser/autofill/model/automation/automation_app_interface.mm
- M ios/chrome/browser/infobars/model/overlays/browser_agent/interaction_handlers/test/mock_autofill_save_card_infobar_delegate_mobile.mm
- M ios/chrome/browser/infobars/model/overlays/default_infobar_overlay_request_factory_unittest.mm
- M ios/chrome/browser/overlays/ui_bundled/infobar_banner/save_card/save_card_infobar_banner_overlay_mediator_unittest.mm
- M ios/chrome/browser/overlays/ui_bundled/infobar_banner/save_cvc/save_cvc_infobar_banner_overlay_mediator_unittest.mm
- M ios/chrome/browser/overlays/ui_bundled/infobar_modal/save_card/save_card_infobar_modal_overlay_coordinator_unittest.mm
- M ios/chrome/browser/overlays/ui_bundled/infobar_modal/save_card/save_card_infobar_modal_overlay_mediator_unittest.mm
- M ios/chrome/browser/settings/ui_bundled/autofill/autofill_credit_card_table_view_controller_unittest.mm
Change size: L
Delta: 24 files changed, 260 insertions(+), 503 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Tommy Martino, +1 by Vinny Persky
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages