[Signin][SAS] Return std::optional from GetPreferredAccountForPromo [chromium/src : main]
Menghan Yang (Gerrit)
Menghan Yang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Menghan Yang
| Commit-Queue | +1 |
1 comment
Hi Pâris,
Here is the CL I mentioned earlier, would you mind having a look?
I feel that I don't have enough context yet and am not used to work in this part of code, please don't hesitate to be as picky as you want so I can better learn!
Thank you in advance🙏
Related details
- 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. |
Pâris Meuleman (Gerrit)
Pâris Meuleman voted and added 1 comment![]( "Open in Gerrit")
Votes added by Pâris Meuleman
| Code-Review | +1 |
1 comment
// fetched yet, the first account is returned by default. Returns std::nulloptnot your change but this is not true given
```
AccountPreviewDataService::AccountPreviewPreference
AccountPreviewDataServiceImpl::ComputePreferredAccount() const {
CHECK(base::FeatureList::IsEnabled(
switches::kEnableAccountPreviewPreferredAccount));
// TODO(crbug.com/530144650): Implement heuristic to compute the preferred
// account and preferred data types.
return AccountPreviewPreference();
}
```
Related details
- Menghan Yang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Menghan Yang (Gerrit)
Menghan Yang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Menghan Yang
| Auto-Submit | +1 |
1 comment
// fetched yet, the first account is returned by default. Returns std::nulloptnot your change but this is not true given
```
AccountPreviewDataService::AccountPreviewPreference
AccountPreviewDataServiceImpl::ComputePreferredAccount() const {
CHECK(base::FeatureList::IsEnabled(
switches::kEnableAccountPreviewPreferredAccount));// TODO(crbug.com/530144650): Implement heuristic to compute the preferred
// account and preferred data types.
return AccountPreviewPreference();
}
```
Thanks a lot for the review! 😊
Indeed, the comment was here before the CL, I though it was still the plan even if it's not implemented yet (I guess it makes sense to return the first account to avoid custom handling at the platform UI level to simplify the code, but it can also reduce flexibility in the future if we want to add custom logic at the os level)
Wdyt about leaving it here for now, and update if the implementation changes direction in the future? (re-opening this comment for confirming before submit)
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 added 1 comment![]( "Open in Gerrit")
// fetched yet, the first account is returned by default. Returns std::nulloptMenghan Yangnot your change but this is not true given
```
AccountPreviewDataService::AccountPreviewPreference
AccountPreviewDataServiceImpl::ComputePreferredAccount() const {
CHECK(base::FeatureList::IsEnabled(
switches::kEnableAccountPreviewPreferredAccount));// TODO(crbug.com/530144650): Implement heuristic to compute the preferred
// account and preferred data types.
return AccountPreviewPreference();
}
```
Thanks a lot for the review! 😊
Indeed, the comment was here before the CL, I though it was still the plan even if it's not implemented yet (I guess it makes sense to return the first account to avoid custom handling at the platform UI level to simplify the code, but it can also reduce flexibility in the future if we want to add custom logic at the os level)Wdyt about leaving it here for now, and update if the implementation changes direction in the future? (re-opening this comment for confirming before submit)
SGTM
I'll try to keep it mind if we implement UI changes before heuristic changes 😊
Related details
- Menghan Yang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Menghan Yang (Gerrit)
Menghan Yang added 1 comment![]( "Open in Gerrit")
// fetched yet, the first account is returned by default. Returns std::nulloptMenghan Yangnot your change but this is not true given
```
AccountPreviewDataService::AccountPreviewPreference
AccountPreviewDataServiceImpl::ComputePreferredAccount() const {
CHECK(base::FeatureList::IsEnabled(
switches::kEnableAccountPreviewPreferredAccount));// TODO(crbug.com/530144650): Implement heuristic to compute the preferred
// account and preferred data types.
return AccountPreviewPreference();
}
```
Pâris MeulemanThanks a lot for the review! 😊
Indeed, the comment was here before the CL, I though it was still the plan even if it's not implemented yet (I guess it makes sense to return the first account to avoid custom handling at the platform UI level to simplify the code, but it can also reduce flexibility in the future if we want to add custom logic at the os level)Wdyt about leaving it here for now, and update if the implementation changes direction in the future? (re-opening this comment for confirming before submit)
SGTM
I'll try to keep it mind if we implement UI changes before heuristic changes 😊
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Menghan Yang (Gerrit)
Menghan Yang voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
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
[Signin][SAS] Return std::optional from GetPreferredAccountForPromo
Previously, GetPreferredAccountForPromo always returns a non-null
preference object event if the gaiaID string is not valid. This can be
confusing (preferred data types list is not meaningful if the preferred
account ID is not valid) and complexifies slightly the data conversion
from native to java, which forbid conversion when the ID string is empty
[1].
This CL updates AccountPreviewDataService::GetPreferredAccountForPromo
to return std::optional<AccountPreviewPreference> when necessary (e.g.
no account on the device) or if the saved/computed gaia_id is not valid.
If we realise that we prefer that the service shouldn't be called in
cases when it can return null, we can remove the `optional` from the
return type later.
[1]
https://source.chromium.org/chromium/chromium/src/+/main:google_apis/gaia/gaia_id.cc;l=36;drc=60df9044934a2841875904f21e9673afe970da1e
- M components/signin/core/browser/account_preview_data_service.h
- M components/signin/core/browser/account_preview_data_service_impl.cc
- M components/signin/core/browser/account_preview_data_service_impl.h
- M components/signin/core/browser/account_preview_data_service_impl_unittest.cc
Code-Review: +1 by Pâris Meuleman
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |