[iOS] Sign in flow for 'SaveToPhotos' [chromium/src : main]
Quentin Pubert (Gerrit)
Quentin Pubert added 6 comments![]( "Open in Gerrit")
Save photos in your Google Photos and access them any time.We might want a different string here since there is no such thing as Choose from Photos so it is not accurate to say "access them any time"
if (_signinCoordinator.viewWillPersist) {```suggestion
CHECK(base::FeatureList::IsEnabled(kIOSSaveToPhotosSignedOut));
if (_signinCoordinator.viewWillPersist) {
```
[self startValidationSpinnerForAccountPicker];
[self hideAccountPicker];If I am not mistaken the account picker is not presented at this point so none of this is doing anything.
[_mediator tryUploadImage];Instead of adding `-setIdentity:` and then directly calling `-tryUploadImage`, I would add a new method similar to `-accountPickerDidSelectIdentity:askEveryTime:` e.g. `-userSignedInToSaveImageWithIdentity:` which would set the identity to use and then call `tryUploadImage`
CHECK(_identityManager->HasPrimaryAccount(signin::ConsentLevel::kSignin),
base::NotFatalUntil::M152);Instead of removing the CHECK, maybe add `|| base::FeatureList::IsEnabled(kIOSSaveToPhotosSignedOut)` to the condition.
[self.delegate openSignIn];Only do that if `base::FeatureList::IsEnabled(kIOSSaveToPhotosSignedOut)`, otherwise `-hideSaveToPhotos`
Related details
- Joseph Lanza
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. |
Joseph Lanza (Gerrit)
Joseph Lanza added 6 comments![]( "Open in Gerrit")
Save photos in your Google Photos and access them any time.We might want a different string here since there is no such thing as Choose from Photos so it is not accurate to say "access them any time"
Even if there is no 'Choose from Photos' option currently, users can still access their photo inside their navigator or using the Photos app.
```suggestion
CHECK(base::FeatureList::IsEnabled(kIOSSaveToPhotosSignedOut));
if (_signinCoordinator.viewWillPersist) {
```
Done
[self startValidationSpinnerForAccountPicker];
[self hideAccountPicker];If I am not mistaken the account picker is not presented at this point so none of this is doing anything.
Done
Instead of adding `-setIdentity:` and then directly calling `-tryUploadImage`, I would add a new method similar to `-accountPickerDidSelectIdentity:askEveryTime:` e.g. `-userSignedInToSaveImageWithIdentity:` which would set the identity to use and then call `tryUploadImage`
Done
CHECK(_identityManager->HasPrimaryAccount(signin::ConsentLevel::kSignin),
base::NotFatalUntil::M152);Instead of removing the CHECK, maybe add `|| base::FeatureList::IsEnabled(kIOSSaveToPhotosSignedOut)` to the condition.
Done
Only do that if `base::FeatureList::IsEnabled(kIOSSaveToPhotosSignedOut)`, otherwise `-hideSaveToPhotos`
- Quentin Pubert
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Quentin Pubert (Gerrit)
Quentin Pubert added 2 comments![]( "Open in Gerrit")
please add a description and bug before submitting
// Tries to upload the image.
- (void)tryUploadImage;this should not be public
Related details
- Joseph Lanza
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. |
Joseph Lanza (Gerrit)
Joseph Lanza added 2 comments![]( "Open in Gerrit")
please add a description and bug before submitting
Done
this should not be public
- Quentin Pubert
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Quentin Pubert (Gerrit)
Quentin Pubert voted Code-Review+1![]( "Open in Gerrit")
- Joseph Lanza
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Milchior (Gerrit)
Arthur Milchior voted and added 1 comment![]( "Open in Gerrit")
Votes added by Arthur Milchior
| Code-Review | +1 |
1 comment
DoNothingContinuationProvider()];You should probably not explicitly in the CL comment and in the code documentation that if the user sign-in with an account belonging to another profile, the "save to photo" action is cancelled, and the user don’t even have access to the photo anymore.
Because I expect it would be non trivial for a reader that don’t know about profil transition that in this case, sign-in can be successful but nothing occurs.
If you don’t want to deal opening the SaveToPhotosCoordinator in the new profile, I would have expected you at least reopen the current URL, so that the user restart the save flow.
If you plan to eventually ensure that the flow restrat from the other profile, then I’d suggest adding a TODO here.
In case you don’t know, you can request a managerchrome.com account to test the behavior of chrome with managed account
Related details
- Joseph Lanza
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. |
Joseph Lanza (Gerrit)
Joseph Lanza added 1 comment![]( "Open in Gerrit")
You should probably not explicitly in the CL comment and in the code documentation that if the user sign-in with an account belonging to another profile, the "save to photo" action is cancelled, and the user don’t even have access to the photo anymore.
Because I expect it would be non trivial for a reader that don’t know about profil transition that in this case, sign-in can be successful but nothing occurs.
If you don’t want to deal opening the SaveToPhotosCoordinator in the new profile, I would have expected you at least reopen the current URL, so that the user restart the save flow.If you plan to eventually ensure that the flow restrat from the other profile, then I’d suggest adding a TODO here.
In case you don’t know, you can request a managerchrome.com account to test the behavior of chrome with managed account
I wanted to try to handle this in another CL, I added a TODO as you suggested
Related details
- Arthur Milchior
- Quentin Pubert
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |