[AF] Implement "Maximize rewards" suggestion selection [chromium/src : main]
Haochen Feng (Gerrit)
Haochen Feng added 6 comments![]( "Open in Gerrit")
Thanks for the comments! I had mistakenly moved the commit for all the code and unit tests surrounding the `is_card_number_field_empty_` variable elsewhere, sorry about that.
[AF] Implement "Maximize Rewards" Suggestion SelectionHaochen Feng"Maximize rewards" suggestion selection
Done
This CL contains the necessary code to support the suggestion selection flow for the `MaximizeRewardsEntry` suggestion and the corresponding `AiCardRecommendationManager`. It routes suggestion selection to `OnUserDecisionToUseMaximizeRewards`, where it stops at triggering `TriggerCheckoutAmountExtractionWithAi`.Haochen FengThis CL doesn't trigger amount extraction though.
My bad, I seem to have mistakenly moved a commit from this CL to another branch when rebasing. I undid that and double-checked this new CL.
This CL contains the necessary code to support the suggestion selection flow for the `MaximizeRewardsEntry` suggestion and the corresponding `AiCardRecommendationManager`. It routes suggestion selection to `OnUserDecisionToUseMaximizeRewards`, where it stops at triggering `TriggerCheckoutAmountExtractionWithAi`.
Additionally, it handles unit tests, and maintaining a private variable `is_card_number_field_empty_`, denoting if the credit card number box is empty or not, which is updated by `NotifyOfFieldState`.Haochen FengLong lines should be wrapped to 72 columns for easier log message
viewing in terminals: https://chromium.googlesource.com/chromium/src/+/HEAD/docs/contributing.md#uploading-a-change-for-review
Done
Additionally, it handles unit tests, and maintaining a private variable `is_card_number_field_empty_`, denoting if the credit card number box is empty or not, which is updated by `NotifyOfFieldState`.Haochen FengAlso, this CL doesn't store the private variable `is_card_number_field_empty_`. Did you plan to do this in a separate CL?
ditto
OnUserDecisionToUseMaximizeCreditCardBenefits() {}Haochen FengIf you plan to implement this in a later CL leave a todo and `NOTIMPLEMENTED()` here.
ditto
Related details
- Fernando Ramirez
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. |
Fernando Ramirez (Gerrit)
Fernando Ramirez added 5 comments![]( "Open in Gerrit")
This CL contains the necessary code to support the suggestion selection flow for the `MaximizeRewardsEntry` suggestion and the corresponding `AiCardRecommendationManager`. It routes suggestion selection to `OnUserDecisionToUseMaximizeRewards`, where it stops at triggering `TriggerCheckoutAmountExtractionWithAi`.Haochen FengThis CL doesn't trigger amount extraction though.
My bad, I seem to have mistakenly moved a commit from this CL to another branch when rebasing. I undid that and double-checked this new CL.
No worries, thanks!
virtual void OnUserDecisionToUseMaximizeCreditCardBenefits();Let's add a code comment above this function to explain its purpose.
bool is_card_number_field_empty) {Can this ever be `false`?
`NotifyOfFieldState` is only called if `ShouldShowMaximizeCreditCardBenefitsSuggestion` evaluates to `true`. In order for `ShouldShowMaximizeCreditCardBenefitsSuggestion` to evaluate to `true`, the card number field should be empty:
```
// This ensures the credit card field is empty before we run AI amount
// extraction logic to avoid feeding any credit card numbers to Gemini.
if (!is_card_number_field_empty) {
return false;
}
```
Should we instead remove this parameter and rename this function to `NotifyOfCardNumberFieldEmpty` or something similar?
if (!is_card_number_field_empty_) {
browser_autofill_manager_->GetAmountExtractionManager().Reset();
weak_ptr_factory_.InvalidateWeakPtrs();
}If `is_card_number_field_empty_` can currently never be `false`, then this logic is unreachable.
ShouldShowMaximizeCreditCardBenefitsSuggestion(I left a comment here: https://chromium-review.git.corp.google.com/c/chromium/src/+/7920915/comment/9b4d075e_b45aa58d/.
Depending on what Vinny says, we may need to remove the `is_card_number_field_empty` param from this function. This way `NotifyOfFieldState` is also called when `is_card_number_field_empty` is `false`.
Related details
- Haochen Feng
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. |
Haochen Feng (Gerrit)
Haochen Feng added 4 comments![]( "Open in Gerrit")
virtual void OnUserDecisionToUseMaximizeCreditCardBenefits();Let's add a code comment above this function to explain its purpose.
done -- let me know if this works.
bool is_card_number_field_empty) {Can this ever be `false`?
`NotifyOfFieldState` is only called if `ShouldShowMaximizeCreditCardBenefitsSuggestion` evaluates to `true`. In order for `ShouldShowMaximizeCreditCardBenefitsSuggestion` to evaluate to `true`, the card number field should be empty:
```
// This ensures the credit card field is empty before we run AI amount
// extraction logic to avoid feeding any credit card numbers to Gemini.
if (!is_card_number_field_empty) {
return false;
}
```Should we instead remove this parameter and rename this function to `NotifyOfCardNumberFieldEmpty` or something similar?
I implemented the fix as we discussed, with the in-line check. Let me know if this works.
For the fix I removed the check for `is_card_number_field_empty` in `ShouldShowMaximizeCreditCardBenefitsSuggestion`, as well as the parameter, so it could successfully notify the manager.
Then, I added an in-line check in `credit_card_suggestion_generator.cc` to prevent suggestion Generation when `is_card_number_field_empty` is false, similar to BNPL
if (!is_card_number_field_empty_) {
browser_autofill_manager_->GetAmountExtractionManager().Reset();
weak_ptr_factory_.InvalidateWeakPtrs();
}If `is_card_number_field_empty_` can currently never be `false`, then this logic is unreachable.
Resolved -- we discussed in meeting and ended up removing the param and the associated check, so the logic is reachable now.
I left a comment here: https://chromium-review.git.corp.google.com/c/chromium/src/+/7920915/comment/9b4d075e_b45aa58d/.
Depending on what Vinny says, we may need to remove the `is_card_number_field_empty` param from this function. This way `NotifyOfFieldState` is also called when `is_card_number_field_empty` is `false`.
Resolved -- we discussed in meeting and ended up removing the param and the associated check.
Related details
- Fernando Ramirez
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. |
Fernando Ramirez (Gerrit)
Fernando Ramirez voted and added 5 comments![]( "Open in Gerrit")
Votes added by Fernando Ramirez
| Code-Review | +1 |
5 comments
LGTM % left a question and recommended unittest update.
virtual void OnUserDecisionToUseMaximizeCreditCardBenefits();Haochen FengLet's add a code comment above this function to explain its purpose.
done -- let me know if this works.
Thanks!
bool is_card_number_field_empty) {Haochen FengCan this ever be `false`?
`NotifyOfFieldState` is only called if `ShouldShowMaximizeCreditCardBenefitsSuggestion` evaluates to `true`. In order for `ShouldShowMaximizeCreditCardBenefitsSuggestion` to evaluate to `true`, the card number field should be empty:
```
// This ensures the credit card field is empty before we run AI amount
// extraction logic to avoid feeding any credit card numbers to Gemini.
if (!is_card_number_field_empty) {
return false;
}
```Should we instead remove this parameter and rename this function to `NotifyOfCardNumberFieldEmpty` or something similar?
I implemented the fix as we discussed, with the in-line check. Let me know if this works.
For the fix I removed the check for `is_card_number_field_empty` in `ShouldShowMaximizeCreditCardBenefitsSuggestion`, as well as the parameter, so it could successfully notify the manager.
Then, I added an in-line check in `credit_card_suggestion_generator.cc` to prevent suggestion Generation when `is_card_number_field_empty` is false, similar to BNPL
Thank you! This looks good.
weak_ptr_factory_.InvalidateWeakPtrs();What is this needed for?
// Tests that NotifyOfFieldState(false) cancels ongoing amount extraction.Is amount extraction currently running given this test setup?
Calling `NotifyOfFieldState(true)` doesn't mean that it is.
I think this should instead test that calling `NotifyOfFieldState(false)` calls `Reset`. Similar to: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/autofill/payments/android_bnpl_ui_delegate_unittest.cc;l=104-111;drc=fbb0fa2da371a4a3baf7ba38ac12796ebf20ca52
Related details
- Haochen Feng
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. |