Remove flags for save card bottom sheet on iOS. [chromium/src : main]
Slobodan Pejic (Gerrit)
Slobodan Pejic added 1 comment![]( "Open in Gerrit")
"name": "autofill-local-save-card-bottomsheet",Also remove the local save metadata.
Related details
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. |
Slobodan Pejic (Gerrit)
Slobodan Pejic voted and added 1 comment![]( "Open in Gerrit")
Votes added by Slobodan Pejic
| Commit-Queue | +1 |
1 comment
Also remove the local save metadata.
Done
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
Reviewer source(s):
smcg...@chromium.org is from context(googleclient/chrome/chromium_gwsq/chrome/browser/autofill/payments/reviews.gwsq)
Related details
- Stephen McGruer
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen McGruer (Gerrit)
Stephen McGruer added 6 comments![]( "Open in Gerrit")
TEST_P(AutofillSaveCardUiInfoTestForUploadSave,Is whatever this test was testing still covered elsewhere in the case where the bottomsheet is not being used (e.g., num-strikes > 0), or is this test entirely defunct because the code it tests is also removed?
base::test::ScopedFeatureList feature_list(Although not a big deal here, usually features should be set as early as possible for good practice. Maybe move this to the first line in the test?
IOS_Infobar_SendSaveCvcSignalIfCvcEmpty_WithStrikes) {Is this test any different than IOS_Infobar_SendSaveCvcSignalIfCvcEmpty_WhenShowingInfobar ?
#if BUILDFLAG(IS_IOS)Empty buildflag block ,remove?
id<GREYMatcher> UploadBannerLabelsMatcher(bool is_bottomsheet_enabled = true) {This appears to no longer be called with is_bottomsheet_enabled=false, is that true? If so, remove that parameter and update this code.
: IDS_AUTOFILL_SAVE_CARD_PROMPT_TITLE_TO_CLOUD_V3);Once this usage is removed (see above comment), I think this message ID can be removed entirely (I think there are no other uses in the codebase).
Related details
- Slobodan Pejic
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. |
Slobodan Pejic (Gerrit)
Slobodan Pejic added 7 comments![]( "Open in Gerrit")
FYI: The change needed rebasing, and meanwhile wallet branding changes landed - https://chromium-review.googlesource.com/c/chromium/src/+/7474129.
TEST_P(AutofillSaveCardUiInfoTestForUploadSave,Is whatever this test was testing still covered elsewhere in the case where the bottomsheet is not being used (e.g., num-strikes > 0), or is this test entirely defunct because the code it tests is also removed?
From the implementation the title and explanation texts are not distinguished between infobar (strikes > 0 or requiring fix flow) and bottom sheet (strikes == 0 and not requiring fix flow), so we no longer need this "infobar" test. E.g. the V3 messages are no longer used on iOS (still used on Android), and I updated the grd file to reflect that now.
IIRC, there was some overlap in updating the messages and the creation of the bottom sheet, so the messages were updated as part of the bottom sheet creation.
Although not a big deal here, usually features should be set as early as possible for good practice. Maybe move this to the first line in the test?
That's a good point. I've hoisted the scoped feature initialization. (And then [removed the test](https://chromium-review.googlesource.com/c/chromium/src/+/7410687/comment/70c39119_57235a9d/).)
IOS_Infobar_SendSaveCvcSignalIfCvcEmpty_WithStrikes) {Is this test any different than IOS_Infobar_SendSaveCvcSignalIfCvcEmpty_WhenShowingInfobar ?
Actually, yes these are the same test now, so I removed the test above.
#if BUILDFLAG(IS_IOS)Slobodan PejicEmpty buildflag block ,remove?
Done
id<GREYMatcher> UploadBannerLabelsMatcher(bool is_bottomsheet_enabled = true) {This appears to no longer be called with is_bottomsheet_enabled=false, is that true? If so, remove that parameter and update this code.
Done
: IDS_AUTOFILL_SAVE_CARD_PROMPT_TITLE_TO_CLOUD_V3);Once this usage is removed (see above comment), I think this message ID can be removed entirely (I think there are no other uses in the codebase).
It is still used on Android, apparently, so I've put it behind `<if expr="is_android">...`. Similarly for `..._EXPLANATION_V3`.
Related details
- Stephen McGruer
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Slobodan Pejic (Gerrit)
Slobodan Pejic added 1 comment![]( "Open in Gerrit")
: IDS_AUTOFILL_SAVE_CARD_PROMPT_TITLE_TO_CLOUD_V3);Slobodan PejicOnce this usage is removed (see above comment), I think this message ID can be removed entirely (I think there are no other uses in the codebase).
It is still used on Android, apparently, so I've put it behind `<if expr="is_android">...`. Similarly for `..._EXPLANATION_V3`.
From our off-thread discussion. Actually android did not use the _V3 strings, so removed them instead.
Related details
- Stephen McGruer
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Slobodan Pejic (Gerrit)
Slobodan Pejic added 1 comment![]( "Open in Gerrit")
: IDS_AUTOFILL_SAVE_CARD_PROMPT_TITLE_TO_CLOUD_V3);Slobodan PejicOnce this usage is removed (see above comment), I think this message ID can be removed entirely (I think there are no other uses in the codebase).
Slobodan PejicIt is still used on Android, apparently, so I've put it behind `<if expr="is_android">...`. Similarly for `..._EXPLANATION_V3`.
From our off-thread discussion. Actually android did not use the _V3 strings, so removed them instead.
And they're back in... They are still needed as evidenced by tests failing in PS19. They are in `...save_card_ui_info.cc` but in a common section which is folded/hidden in gerrit.
Related details
- Stephen McGruer
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen McGruer (Gerrit)
Stephen McGruer voted Code-Review+1![]( "Open in Gerrit")
- Slobodan Pejic
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Slobodan Pejic (Gerrit)
Slobodan Pejic added 1 comment![]( "Open in Gerrit")
Another merge was done due to https://chromium-review.googlesource.com/c/chromium/src/+/7502596. Oddly the gerrit diff [PS20->PS21](https://chromium-review.googlesource.com/c/chromium/src/+/7410687/20..21) shows nothing... that seems to be because all the conflicts were in code that was removed.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Slobodan Pejic (Gerrit)
Slobodan Pejic added 1 comment![]( "Open in Gerrit")
Olivier please take a look for ios/chrome/browser/autofill owners.
Siyu, please take a look for components/autofill_payments_strings.grdp owners.
Related details
- Olivier Robin
- Siyu An
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Robin (Gerrit)
Olivier Robin voted Code-Review+1![]( "Open in Gerrit")
- Siyu An
- Slobodan Pejic
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |