[iOS][Signin] Use GoogleServiceAuthError in AccessTokenCallback [chromium/src : main]
Boris Sazonov (Gerrit)
Boris Sazonov added 2 comments![]( "Open in Gerrit")
base::OnceCallback<void(std::optional<AccessTokenInfo>,
GoogleServiceAuthError)>;Would it be possible to use `base::expected<AccessTokenInfo, GoogleServiceAuthError>` instead?
using AccessTokenCallbackWithAuthError =It will be hard to rename this later (as it is used across public and internal repos). Would it be possible to use a shorter name instead (`AccessTokenRequestCallback`)? I don't think it's going to be an issue if we drop `WithAuthError` from the name - in the long run the callback with `NSError` will go away and there won't be any ambiguity.
Related details
- Arnaud Yameogo
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. |
Arnaud Yameogo (Gerrit)
Arnaud Yameogo added 2 comments![]( "Open in Gerrit")
base::OnceCallback<void(std::optional<AccessTokenInfo>,
GoogleServiceAuthError)>;Would it be possible to use `base::expected<AccessTokenInfo, GoogleServiceAuthError>` instead?
Done
It will be hard to rename this later (as it is used across public and internal repos). Would it be possible to use a shorter name instead (`AccessTokenRequestCallback`)? I don't think it's going to be an issue if we drop `WithAuthError` from the name - in the long run the callback with `NSError` will go away and there won't be any ambiguity.
- Boris Sazonov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samar Chehade (Gerrit)
Samar Chehade added 3 comments![]( "Open in Gerrit")
The GoogleServiceAuthErrorFromError conversion method, currently in
device_accounts_provider_impl.mm, will also be moved to ios_internal in
a follow-up CL (https://crrev.com/i/9175936).Sorry if I missed something, why do we need to move this implementation ?
Anything added in ios_internal becomes a pain to update so I would recommend only doing this if it is strictly necessary.
// Asynchronously retrieves access tokens for `identity` with `scopes`. The
// callback is invoked on the calling sequence when the operation completes.Should a TODO be added here to delete this method after launch?
// Same as 'AccessTokenCallback' but with GoogleServiceAuthErrornit: When the feature flag is cleaned up, AccessTokenCallback will be deleted, won't it ? Just to make sure we don't forget to update it later, it might be worth adding a standalone comment here from the get-go.
Related details
- Arnaud Yameogo
- Boris Sazonov
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. |
Arnaud Yameogo (Gerrit)
Arnaud Yameogo added 3 comments![]( "Open in Gerrit")
The GoogleServiceAuthErrorFromError conversion method, currently in
device_accounts_provider_impl.mm, will also be moved to ios_internal in
a follow-up CL (https://crrev.com/i/9175936).Sorry if I missed something, why do we need to move this implementation ?
Anything added in ios_internal becomes a pain to update so I would recommend only doing this if it is strictly necessary.
The move is necessary for the case we have a mdm error: we will need to populate [IOSDeviceManagementErrorDetails](https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/signin/model/ios_device_management_error_details.h?q=IOSDeviceManagementErrorDetails) using GCRDeviceService and some methods exposed from google3 so the conversion has to happen in ios_internal
// Asynchronously retrieves access tokens for `identity` with `scopes`. The
// callback is invoked on the calling sequence when the operation completes.Should a TODO be added here to delete this method after launch?
Done (crbug.com/502126003)
// Same as 'AccessTokenCallback' but with GoogleServiceAuthErrornit: When the feature flag is cleaned up, AccessTokenCallback will be deleted, won't it ? Just to make sure we don't forget to update it later, it might be worth adding a standalone comment here from the get-go.
- Boris Sazonov
- Samar Chehade
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. |
Boris Sazonov (Gerrit)
Boris Sazonov voted and added 4 comments![]( "Open in Gerrit")
Votes added by Boris Sazonov
| Code-Review | +1 |
4 comments
AccessTokenRequestCallback callback) {}Please add a TODO to make this method pure virtual after updating the internal implementation.
virtual void GetAccessToken(id<SystemIdentity> identity,Add a comment to clarify what this does.
// Same as 'AccessTokenCallback' but with GoogleServiceAuthError[Nit] This comment will need to be updated when `AccessTokenCallback` is deleted, please consider writing a proper comment for it now to simplify the future cleanup.
Related details
- Arnaud Yameogo
- Samar Chehade
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. |
Arnaud Yameogo (Gerrit)
Arnaud Yameogo added 4 comments![]( "Open in Gerrit")
The GoogleServiceAuthErrorFromError conversion method, currently in
device_accounts_provider_impl.mm, will also be moved to ios_internal in
a follow-up CL (https://crrev.com/i/9175936).Arnaud YameogoSorry if I missed something, why do we need to move this implementation ?
Anything added in ios_internal becomes a pain to update so I would recommend only doing this if it is strictly necessary.
The move is necessary for the case we have a mdm error: we will need to populate [IOSDeviceManagementErrorDetails](https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/signin/model/ios_device_management_error_details.h?q=IOSDeviceManagementErrorDetails) using GCRDeviceService and some methods exposed from google3 so the conversion has to happen in ios_internal
Marked as resolved.
Please add a TODO to make this method pure virtual after updating the internal implementation.
Done(crbug.com/502440730)
virtual void GetAccessToken(id<SystemIdentity> identity,Add a comment to clarify what this does.
Done
// Same as 'AccessTokenCallback' but with GoogleServiceAuthError[Nit] This comment will need to be updated when `AccessTokenCallback` is deleted, please consider writing a proper comment for it now to simplify the future cleanup.
- Samar Chehade
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samar Chehade (Gerrit)
Samar Chehade voted and added 2 comments![]( "Open in Gerrit")
Votes added by Samar Chehade
| Code-Review | +1 |
2 comments
The GoogleServiceAuthErrorFromError conversion method, currently in
device_accounts_provider_impl.mm, will also be moved to ios_internal in
a follow-up CL (https://crrev.com/i/9175936).Arnaud YameogoSorry if I missed something, why do we need to move this implementation ?
Anything added in ios_internal becomes a pain to update so I would recommend only doing this if it is strictly necessary.
Arnaud YameogoThe move is necessary for the case we have a mdm error: we will need to populate [IOSDeviceManagementErrorDetails](https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/signin/model/ios_device_management_error_details.h?q=IOSDeviceManagementErrorDetails) using GCRDeviceService and some methods exposed from google3 so the conversion has to happen in ios_internal
Marked as resolved.
Ideally we should have the minimum amount of code in ios_internal. If the conversion API is currently public then let's keep it that way and keep only the part of the conversion that calls the DeviceService in ios_internal.
Related details
- Arnaud Yameogo
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. |
Arnaud Yameogo (Gerrit)
Arnaud Yameogo added 1 comment![]( "Open in Gerrit")
The GoogleServiceAuthErrorFromError conversion method, currently in
device_accounts_provider_impl.mm, will also be moved to ios_internal in
a follow-up CL (https://crrev.com/i/9175936).Arnaud YameogoSorry if I missed something, why do we need to move this implementation ?
Anything added in ios_internal becomes a pain to update so I would recommend only doing this if it is strictly necessary.
Arnaud YameogoThe move is necessary for the case we have a mdm error: we will need to populate [IOSDeviceManagementErrorDetails](https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/signin/model/ios_device_management_error_details.h?q=IOSDeviceManagementErrorDetails) using GCRDeviceService and some methods exposed from google3 so the conversion has to happen in ios_internal
Samar ChehadeMarked as resolved.
Arnaud YameogoIdeally we should have the minimum amount of code in ios_internal. If the conversion API is currently public then let's keep it that way and keep only the part of the conversion that calls the DeviceService in ios_internal.
Marked as resolved.
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arnaud Yameogo (Gerrit)
Arnaud Yameogo 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-scoped@luci-project-accounts.iam.gserviceaccount.com (Gerrit)
chromiu...@luci-project-accounts.iam.gserviceaccount.com submitted the change![]( "Open in Gerrit")
Change information
[iOS][Signin] Use GoogleServiceAuthError in AccessTokenCallback
This CL replaces NSError with GoogleServiceAuthError in the
AccessTokenCallback and overloads the GetAccessToken method. The new
implementation with the updated callback will be added in ios_internal.
The GoogleServiceAuthErrorFromError conversion method, currently in
device_accounts_provider_impl.mm, will also be moved to ios_internal in
a follow-up CL (https://crrev.com/i/9175936).
- M ios/chrome/browser/signin/model/system_identity_manager.h
Code-Review: +1 by Boris Sazonov, +1 by Samar Chehade
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |