[XDeviceSignin] Only suppress the promo if other mobile devices [chromium/src : main]
Ryan Sultanem (Gerrit)
Ryan Sultanem voted and added 3 comments![]( "Open in Gerrit")
Votes added by Ryan Sultanem
| Code-Review | +1 |
3 comments
Thanks for the quick change!
This looks good to me, but I expect some additional tests to need some adaptation.
kHasOtherDevices = 2,I am wondering if we can reuse this - since this has never logged on Stable yet (and we plan on cherry picking this change), then we can probably just override it, without risking any data mix up (starting at Beta/Stable).
The only risk of doing so is if we are not able to cherry-pick for some reason. But I think it should be rather safe.
If you prefer not to do that, then I think we should at least deprecate this value, since it should not be used anymore, if I am not mistaken.
syncer::DeviceInfo::OsType::kWindows);I will expect this test suite to now fail: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/profiles/profile_menu_view_browsertest.cc;l=349
Not sure if it is worth it but we may consider factoring this into some test utils (definitely not for this CL anyway).
Related details
- Pâris Meuleman
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. |
Pâris Meuleman (Gerrit)
Pâris Meuleman voted and added 3 comments![]( "Open in Gerrit")
Votes added by Pâris Meuleman
| Auto-Submit | +1 |
3 comments
fixed, I'll wait for the CQ and then ask for another review
I am wondering if we can reuse this - since this has never logged on Stable yet (and we plan on cherry picking this change), then we can probably just override it, without risking any data mix up (starting at Beta/Stable).
The only risk of doing so is if we are not able to cherry-pick for some reason. But I think it should be rather safe.
If you prefer not to do that, then I think we should at least deprecate this value, since it should not be used anymore, if I am not mistaken.
I was wondering about that too, I'll revert to using the existing enum. renamed instead of new entry.
I will expect this test suite to now fail: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/profiles/profile_menu_view_browsertest.cc;l=349
Not sure if it is worth it but we may consider factoring this into some test utils (definitely not for this CL anyway).
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. |
Ryan Sultanem (Gerrit)
Ryan Sultanem voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ryan Sultanem
| Code-Review | +1 |
| Commit-Queue | +2 |
1 comment
- Pâris Meuleman
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")
Change information
[XDeviceSignin] Only suppress the promo if other mobile devices
Change from suppressing the promos when any other device was present to
suppress only when a phone is present.
- M chrome/browser/signin/cross_device_signin_promo_manager.cc
- M chrome/browser/signin/cross_device_signin_promo_manager.h
- M chrome/browser/signin/cross_device_signin_promo_manager_unittest.cc
- M chrome/browser/ui/views/profiles/profile_menu_view_browsertest.cc
- M chrome/browser/ui/views/profiles/profile_menu_view_ui_browsertest.cc
- M tools/metrics/histograms/enums.xml
Code-Review: +1 by Ryan Sultanem
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |