[AF] Add kMaximizeCreditCardBenefitsEntry enum [chromium/src : main]
Haochen Feng (Gerrit)
Haochen Feng added 17 comments![]( "Open in Gerrit")
Haochen FengAlso, be sure to click "CQ Dry Run". This also helps ensure that the build doesn't fail and tests pass.
will do, is it best practice to run whenever I put out a patchset? I didn't run it this time because I thought I would be putting out another patchset fairly quickly.
[AF] kMaximizeCreditCardBenefitsEntry EnumHaochen Fengnit: First line isn't usually in title case, it's meant to be a "Summary of change (one line)": https://chromium.googlesource.com/chromium/src/+/HEAD/docs/contributing.md#uploading-a-change-for-review.
WDYT of "[AF] Add kMaximizeCreditCardBenefitsEntry enum"?
ah, I did not know that. The new title makes sense
This CL includes the Enum for the new `SuggestionType` of `kMaximizeCreditCardBenefitsEntry`, as part of the AI card recommendation intern project.Haochen Fengnit: enum
Done
- returns true for `bool IsCreditCardFooterSuggestion(...) {` so the Maximize Rewards Suggestion can be placed at the bottom of the suggestions popup.Haochen Fengnit: IsCreditCardFooterSuggestion
Done
- returns true for `bool IsCreditCardFooterSuggestion(...) {` so the Maximize Rewards Suggestion can be placed at the bottom of the suggestions popup.Haochen Fengnit: "Maximize rewards" suggestion
Done
This CL includes the Enum for the new `SuggestionType` of `kMaximizeCreditCardBenefitsEntry`, as part of the AI card recommendation intern project.
Notes:
- returns true for `bool IsCreditCardFooterSuggestion(...) {` so the Maximize Rewards Suggestion can be placed at the bottom of the suggestions popup.
- however, for `SuggestionSection GetSuggestionSection(SuggestionType type) {`, this suggestion returns `SuggestionSection::kBody;` so it can be as large as a credit card suggestion, or BNPL suggestion.Haochen FengSometimes you may not be able to auto format when clicking the "format" button in the CL description editor, but "long 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
oops, I did not realize that "format" didn't solve this.
I added a new line every time the line got to the end of the editing window, which appears to be 72 columns long. Does that work?
- however, for `SuggestionSection GetSuggestionSection(SuggestionType type) {`, this suggestion returns `SuggestionSection::kBody;` so it can be as large as a credit card suggestion, or BNPL suggestion.Haochen Fengnit: GetSuggestionSection
Done
case SuggestionType::kMaximizeCreditCardBenefitsEntry:Haochen FengIf we're treating this as a footer suggestion, should it go in the footer section above?
Adding this into the footer section seems to make the suggestion generate as the same size as the "manage payment methods..." suggestion.
I think this switch statement decides the size, while `IsCreditCardFooterSuggestion` decides the position, so I followed what kBnplEntry did, and put it in the body section here, but I put it as true for `IsCreditCardFooterSuggestion`. Does this approach make sense?
case SuggestionType::kMaximizeCreditCardBenefitsEntry:Haochen Fengsmall nit: could you add this below `kFetchingAmbientData` instead? With this being a smaller CL it should land faster so you don't have to worry about merge conflicts as much.
This makes sense. I don't think we would've run into any merge conflicts since, anyway.
case SuggestionType::kMaximizeCreditCardBenefitsEntry:Haochen Fengditto
Done
case SuggestionType::kMaximizeCreditCardBenefitsEntry:Haochen Fengditto
Done
// Maximize Credit Card Benefits suggestion.Haochen FengThis re-states the name. Can you please add more context on the suggestion
I added some context -- let me know if this works
case SuggestionType::kMaximizeCreditCardBenefitsEntry:
return "kMaximizeCreditCardBenefitsEntry";Haochen Fengditto, move below
Done
break;Haochen FengProbably best to fallthrough for now
Done -- this makes sense, because this code should not be reachable in this CL right
case SuggestionType::kMaximizeCreditCardBenefitsEntry:Haochen Fengditto
Done
case SuggestionType::kMaximizeCreditCardBenefitsEntry:Haochen Fengditto
Done
Related details
- Fernando Ramirez
- Vinny Persky
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 2 comments![]( "Open in Gerrit")
Will LGTM once Vinny's comments are resolved. Thanks!
This CL includes the Enum for the new `SuggestionType` of `kMaximizeCreditCardBenefitsEntry`, as part of the AI card recommendation intern project.
Notes:
- returns true for `bool IsCreditCardFooterSuggestion(...) {` so the Maximize Rewards Suggestion can be placed at the bottom of the suggestions popup.
- however, for `SuggestionSection GetSuggestionSection(SuggestionType type) {`, this suggestion returns `SuggestionSection::kBody;` so it can be as large as a credit card suggestion, or BNPL suggestion.Haochen FengSometimes you may not be able to auto format when clicking the "format" button in the CL description editor, but "long 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
oops, I did not realize that "format" didn't solve this.
I added a new line every time the line got to the end of the editing window, which appears to be 72 columns long. Does that work?
Yup, that's exactly what you have to do in cases like this.
Related details
- Haochen Feng
- Vinny Persky
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 1 comment![]( "Open in Gerrit")
case SuggestionType::kMaximizeCreditCardBenefitsEntry:Fernando RamirezAdding a comment here because I think there were some concerns about aligning with a chrome components/autofill reviewer.
Fernando RamirezThanks, reached out to Jihad: https://docs.google.com/document/d/18I1etc4Lx_pZy-BykN9eFXuGwktlbfMjySFOkdNWYt4/edit?resourcekey=0-wisKjgzO9fnYr2wYC3MMog&disco=AAAB9s3o9FU
FYI @vinny...@google.com, Jihad approved adding this suggestion type (see link above).
@haoc...@google.com, moving forward you can assign jihad...@google.com as a reviewer for changes that require a components/autofill OWNER (after me and Vinny LGTM).
Vinny Persky (Gerrit)
Vinny Persky voted and added 3 comments![]( "Open in Gerrit")
Votes added by Vinny Persky
| Code-Review | +1 |
3 comments
Haochen FengIf we're treating this as a footer suggestion, should it go in the footer section above?
Adding this into the footer section seems to make the suggestion generate as the same size as the "manage payment methods..." suggestion.
I think this switch statement decides the size, while `IsCreditCardFooterSuggestion` decides the position, so I followed what kBnplEntry did, and put it in the body section here, but I put it as true for `IsCreditCardFooterSuggestion`. Does this approach make sense?
Acknowledged
// Maximize Credit Card Benefits suggestion.Haochen FengThis re-states the name. Can you please add more context on the suggestion
I added some context -- let me know if this works
Done
Related details
- Haochen Feng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fernando Ramirez (Gerrit)
Fernando Ramirez voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Haochen Feng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Haochen Feng (Gerrit)
Haochen Feng added 1 comment![]( "Open in Gerrit")
Hi! This is the enum for the AI card recommendation project's suggestion type.
Related details
- Jihad Hanna
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Haochen Feng (Gerrit)
Haochen Feng added 1 comment![]( "Open in Gerrit")
Hi! This is the enum for the new suggestion type of `kMaximizeCreditCardBenefitsEntry`, for the AI card recommendation intern project:
Related details
- Alexis Hétu
- Jihad Hanna
- Mohamed Amir Yosef
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexis Hétu (Gerrit)
Alexis Hétu voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alexis Hétu
| Code-Review | +1 |
1 comment
- Haochen Feng
Jihad Hanna (Gerrit)
Jihad Hanna added 2 comments![]( "Open in Gerrit")
case SuggestionType::kMaximizeCreditCardBenefitsEntry:Is it not?
default:Could you expand this into all the remaining enums? We usually prefer avoiding `default` in switch cases
Related details
- Haochen Feng
- Mohamed Amir Yosef
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 3 comments![]( "Open in Gerrit")
case SuggestionType::kMaximizeCreditCardBenefitsEntry:Is it not?
I think this switch statement was used for aligning the height of credit card suggestions:
https://source.chromium.org/chromium/chromium/src/+/6abd248ca1b4123e8ea2bec59d507991ccf65b17
And is meant for suggestions that are directly used to complete a purchase. Maybe @fe...@google.com can give some insight?
Could you expand this into all the remaining enums? We usually prefer avoiding `default` in switch cases
- Alexis Hétu
- Fernando Ramirez
- Jihad Hanna
- Mohamed Amir Yosef
- Vinny Persky
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 1 comment![]( "Open in Gerrit")
case SuggestionType::kMaximizeCreditCardBenefitsEntry:Haochen FengIs it not?
I think this switch statement was used for aligning the height of credit card suggestions:
https://source.chromium.org/chromium/chromium/src/+/6abd248ca1b4123e8ea2bec59d507991ccf65b17
And is meant for suggestions that are directly used to complete a purchase. Maybe @fe...@google.com can give some insight?
I think this switch statement was used for aligning the height of credit card suggestions:
Yup that's what it seems like.
And is meant for suggestions that are directly used to complete a purchase.
Given that this suggestion type is nearly identical to `kBnplEntry` (same structure), I'd move `kMaximizeCreditCardBenefitsEntry` to also return `true`. Either way I don't think it would make a difference to the height based on this logic:
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/autofill/popup/popup_cell_utils.cc;l=842-862;drc=ff3daaf960144bc125b95659c757ce28f826ddc1
Related details
- Alexis Hétu
- Haochen Feng
Haochen Feng (Gerrit)
Haochen Feng added 1 comment![]( "Open in Gerrit")
case SuggestionType::kMaximizeCreditCardBenefitsEntry:Haochen FengIs it not?
Fernando RamirezI think this switch statement was used for aligning the height of credit card suggestions:
https://source.chromium.org/chromium/chromium/src/+/6abd248ca1b4123e8ea2bec59d507991ccf65b17
And is meant for suggestions that are directly used to complete a purchase. Maybe @fe...@google.com can give some insight?
I think this switch statement was used for aligning the height of credit card suggestions:
Yup that's what it seems like.
And is meant for suggestions that are directly used to complete a purchase.
Given that this suggestion type is nearly identical to `kBnplEntry` (same structure), I'd move `kMaximizeCreditCardBenefitsEntry` to also return `true`. Either way I don't think it would make a difference to the height based on this logic:
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/autofill/popup/popup_cell_utils.cc;l=842-862;drc=ff3daaf960144bc125b95659c757ce28f826ddc1
done -- this makes sense
- Alexis Hétu
- Fernando Ramirez
- Jihad Hanna
- Mohamed Amir Yosef
- Vinny Persky
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jihad Hanna (Gerrit)
Jihad Hanna voted Code-Review+1![]( "Open in Gerrit")
- Alexis Hétu
- Fernando Ramirez
- Haochen Feng
- Mohamed Amir Yosef
- Vinny Persky
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexis Hétu (Gerrit)
Alexis Hétu voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alexis Hétu
| Code-Review | +1 |
ios/ lgtm
- Fernando Ramirez
- Haochen Feng
- Mohamed Amir Yosef
- Vinny Persky
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mohamed Amir Yosef (Gerrit)
Mohamed Amir Yosef voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mohamed Amir Yosef
| Code-Review | +1 |
1 comment
- Fernando Ramirez
- Haochen Feng
- Vinny Persky
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Haochen Feng (Gerrit)
Haochen Feng voted Commit-Queue+2![]( "Open in Gerrit")
- Fernando Ramirez
- Vinny Persky
Vinny Persky (Gerrit)
Vinny Persky voted Code-Review+1![]( "Open in Gerrit")
- Fernando Ramirez
- Haochen Feng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Haochen Feng (Gerrit)
Haochen Feng voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Fernando Ramirez
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)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Unreviewed changes
12 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
[AF] Add kMaximizeCreditCardBenefitsEntry enum
This CL includes the enum for the new `SuggestionType` of
`kMaximizeCreditCardBenefitsEntry`, as part of the AI card
recommendation intern project.
Notes:
- returns true for `IsCreditCardFooterSuggestion` so the "Maximize
rewards" Suggestion can be placed at the bottom of the suggestions
popup.
- however, for `GetSuggestionSection`, this suggestion returns
`SuggestionSection::kBody;` so it can be as large as a credit card
suggestion, or BNPL suggestion.
- M chrome/browser/ui/autofill/autofill_suggestion_controller_utils.cc
- M chrome/browser/ui/views/autofill/popup/popup_cell_utils.cc
- M chrome/browser/ui/views/autofill/popup/popup_row_factory_utils.cc
- M chrome/browser/ui/views/autofill/popup/popup_view_utils.cc
- M components/autofill/core/browser/filling/filling_product.cc
- M components/autofill/core/browser/foundations/browser_autofill_manager.cc
- M components/autofill/core/browser/suggestions/payments/payments_suggestion_generator_util.cc
- M components/autofill/core/browser/suggestions/suggestion_type.cc
- M components/autofill/core/browser/suggestions/suggestion_type.h
- M components/autofill/core/browser/ui/autofill_external_delegate.cc
- M components/autofill/core/browser/ui/payments/omnibox_autofill_delegate.cc
- M components/autofill/ios/browser/autofill_agent.mm
- M components/password_manager/core/browser/password_autofill_manager.cc
- M ios/chrome/browser/autofill/form_input_accessory/ui/form_suggestion_label.mm
- M tools/metrics/histograms/metadata/autofill/enums.xml
Code-Review: +1 by Mohamed Amir Yosef, +1 by Jihad Hanna, +1 by Vinny Persky, +1 by Alexis Hétu
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |