[iOS] Sign in flow for 'Save to Drive' [chromium/src : main]
Quentin Pubert (Gerrit)
Quentin Pubert added 10 comments![]( "Open in Gerrit")
Please read all the comments before proceeding. My first comments suggested how to adjust your current approach (let the mediator ask the coordinator to show sign-in through a delegate protocol) but I then realized this may not be necessary, since `saveWithSelectedIdentity:` should probably not be called if the selected destination is Drive and the user is not signed-in.
"+ios/chrome/browser/authentication/ui_bundled",
"+ios/chrome/browser/authentication/ui_bundled/signin",One is probably enough
```suggestion
"+ios/chrome/browser/authentication/ui_bundled",
```
- (void)openSignIn;This should be declared a delegate protocol file, with a comment to describe what it does. Since it is a delegate method it should be named accordingly e.g. ``` // Called when `mediator` wants to open sign-in.
- (void)mediatorWantsToOpenSignIn:(SaveToDriveMediator*)mediator;
- ```
ManageStorageAlertCommands>You will probably have
```suggestion
ManageStorageAlertCommands,
SaveToDriveMediatorDelegate>
```
SigninCoordinator* _signinCoordinator;This looks unused
```suggestion
```
__weak __typeof(self) weakSelf = self;
SigninCoordinatorCompletionCallback completion =
^(SigninCoordinator* coordinator, SigninCoordinatorResult result,
id<SystemIdentity> identity) {
if (!weakSelf) {
return;
}
SaveToDriveCoordinator* strongSelf = weakSelf;
strongSelf->_isSigningIn = NO;
if (result == SigninCoordinatorResultSuccess) {
[strongSelf->_mediator saveWithSelectedIdentity:identity];
} else {
id<SaveToDriveCommands> saveToDriveHandler = HandlerForProtocol(
strongSelf.browser->GetCommandDispatcher(), SaveToDriveCommands);
[saveToDriveHandler hideSaveToDrive];
}
};
AuthenticationOperation operation = AuthenticationOperation::kSigninOnly;
ShowSigninCommand* command = [[ShowSigninCommand alloc]
initWithOperation:operation
identity:nil
accessPoint:signin_metrics::AccessPoint::kSaveToDriveIos
promoAction:signin_metrics::PromoAction::
PROMO_ACTION_NO_SIGNIN_PROMO
completion:completion];Better to avoid long blocks
```suggestion
__weak __typeof(self) weakSelf = self;
ShowSigninCommand* command = [[ShowSigninCommand alloc]
initWithOperation:AuthenticationOperation::kSigninOnly
identity:nil
accessPoint:signin_metrics::AccessPoint::kSaveToDriveIos
promoAction:signin_metrics::PromoAction::
PROMO_ACTION_NO_SIGNIN_PROMO
completion:^(SigninCoordinator* coordinator, SigninCoordinatorResult result, id<SystemIdentity> identity) {
[weakSelf doSigninCompletionWithResult:result identity:identity];
}];
```
Then lower in the file (in #pragma mark - Private)
```
- (void)doSigninCompletionWithResult:(SigninCoordinatorResult)result identity:(id<SystemIdentity>)identity {
_isSigningIn = NO;
if (result == SigninCoordinatorResultSuccess) {
[_mediator saveWithSelectedIdentity:identity];
} else {
id<SaveToDriveCommands> saveToDriveHandler = HandlerForProtocol(
strongSelf.browser->GetCommandDispatcher(), SaveToDriveCommands);
[saveToDriveHandler hideSaveToDrive];
}
}
```
@property(nonatomic, strong) SaveToDriveCoordinator* delegate;This should be a weak reference since strong references are used to define ownership relations, in this case the coordinator owns the mediator. Also this should be a protocol implemented privately by the coordinator
```suggestion
@property(nonatomic, weak) id<SaveToDriveMediatorDelegate> delegate;
```
#import "ios/chrome/browser/save_to_drive/ui_bundled/save_to_drive_coordinator.h"It is best to avoid circular dependencies, a mediator never depends on its own coordinator.
#import "ios/chrome/browser/signin/model/identity_manager_factory.h"Probably unnecessary, the identity manager is a service injected into the mediator by the coordinator. The coordinator is the one using the factory to extract the service from a profile
bool signedIn = [self isSignedIn];It think it would be best to avoid calling `saveWithSelectedIdentity:` if there is no identity and Drive is selected since this method was written with the assumption that this was the case. Instead I think when `didSelectIdentity:` is called on the coordinator, then the coordinator should ask the mediator `- (BOOL)selectedFileDestinationRequiresSignin` and if it returns YES then the coordinator shows sign-in.
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 9 comments![]( "Open in Gerrit")
"+ios/chrome/browser/authentication/ui_bundled",
"+ios/chrome/browser/authentication/ui_bundled/signin",One is probably enough
```suggestion
"+ios/chrome/browser/authentication/ui_bundled",
```
Marked as resolved.
This should be declared a delegate protocol file, with a comment to describe what it does. Since it is a delegate method it should be named accordingly e.g. ``` // Called when `mediator` wants to open sign-in.
- (void)mediatorWantsToOpenSignIn:(SaveToDriveMediator*)mediator;
- ```
Acknowledged
You will probably have
```suggestion
ManageStorageAlertCommands,
SaveToDriveMediatorDelegate>
```
Acknowledged
SigninCoordinator* _signinCoordinator;Joseph LanzaThis looks unused
```suggestion
```
Removed
__weak __typeof(self) weakSelf = self;
SigninCoordinatorCompletionCallback completion =
^(SigninCoordinator* coordinator, SigninCoordinatorResult result,
id<SystemIdentity> identity) {
if (!weakSelf) {
return;
}
SaveToDriveCoordinator* strongSelf = weakSelf;
strongSelf->_isSigningIn = NO;
if (result == SigninCoordinatorResultSuccess) {
[strongSelf->_mediator saveWithSelectedIdentity:identity];
} else {
id<SaveToDriveCommands> saveToDriveHandler = HandlerForProtocol(
strongSelf.browser->GetCommandDispatcher(), SaveToDriveCommands);
[saveToDriveHandler hideSaveToDrive];
}
};
AuthenticationOperation operation = AuthenticationOperation::kSigninOnly;
ShowSigninCommand* command = [[ShowSigninCommand alloc]
initWithOperation:operation
identity:nil
accessPoint:signin_metrics::AccessPoint::kSaveToDriveIos
promoAction:signin_metrics::PromoAction::
PROMO_ACTION_NO_SIGNIN_PROMO
completion:completion];Better to avoid long blocks
```suggestion
__weak __typeof(self) weakSelf = self;
ShowSigninCommand* command = [[ShowSigninCommand alloc]
initWithOperation:AuthenticationOperation::kSigninOnly
identity:nil
accessPoint:signin_metrics::AccessPoint::kSaveToDriveIos
promoAction:signin_metrics::PromoAction::
PROMO_ACTION_NO_SIGNIN_PROMO
completion:^(SigninCoordinator* coordinator, SigninCoordinatorResult result, id<SystemIdentity> identity) {
[weakSelf doSigninCompletionWithResult:result identity:identity];
}];
```Then lower in the file (in #pragma mark - Private)
```
- (void)doSigninCompletionWithResult:(SigninCoordinatorResult)result identity:(id<SystemIdentity>)identity {
_isSigningIn = NO;
if (result == SigninCoordinatorResultSuccess) {
[_mediator saveWithSelectedIdentity:identity];
} else {
id<SaveToDriveCommands> saveToDriveHandler = HandlerForProtocol(
strongSelf.browser->GetCommandDispatcher(), SaveToDriveCommands);
[saveToDriveHandler hideSaveToDrive];
}
}
```
Done
@property(nonatomic, strong) SaveToDriveCoordinator* delegate;This should be a weak reference since strong references are used to define ownership relations, in this case the coordinator owns the mediator. Also this should be a protocol implemented privately by the coordinator
```suggestion
@property(nonatomic, weak) id<SaveToDriveMediatorDelegate> delegate;
```
Acknowledged
#import "ios/chrome/browser/save_to_drive/ui_bundled/save_to_drive_coordinator.h"It is best to avoid circular dependencies, a mediator never depends on its own coordinator.
Done
#import "ios/chrome/browser/signin/model/identity_manager_factory.h"Probably unnecessary, the identity manager is a service injected into the mediator by the coordinator. The coordinator is the one using the factory to extract the service from a profile
Done
It think it would be best to avoid calling `saveWithSelectedIdentity:` if there is no identity and Drive is selected since this method was written with the assumption that this was the case. Instead I think when `didSelectIdentity:` is called on the coordinator, then the coordinator should ask the mediator `- (BOOL)selectedFileDestinationRequiresSignin` and if it returns YES then the coordinator shows sign-in.
- 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 6 comments![]( "Open in Gerrit")
if ([_mediator selectedFileDestinationRequiresSignin]) {
[self openSignIn];
return;
}Everything that modifies the behaviour should be behind the flag
```suggestion
if (base::FeatureList::IsEnabled(kIOSSaveToDriveSignedOut)) {
if ([_mediator selectedFileDestinationRequiresSignin]) {
[self openSignIn];
return;
}
}
```
UIViewController* presenter =
_accountPickerCoordinator.viewController ?: self.baseViewController;
CHECK(presenter);
[presenter presentViewController:_alertController
animated:YES
completion:nil];ditto
UIViewController* presenter =
_accountPickerCoordinator.viewController ?: self.baseViewController;
[googleOneHandler
showGoogleOneForIdentity:identity
entryPoint:GoogleOneEntryPoint::kSaveToDriveAlert
baseViewController:presenter];ditto
SaveToDriveCoordinator* strongSelf = self;This is unnecessary, `self` can be used directly.
bool signedIn = [self isSignedIn];
bool identityButtonHidden =
_fileDestination == FileDestination::kFiles || !signedIn;
[self.accountPickerConsumer setIdentityButtonHidden:identityButtonHiddenditto
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")
if ([_mediator selectedFileDestinationRequiresSignin]) {
[self openSignIn];
return;
}Everything that modifies the behaviour should be behind the flag
```suggestion
if (base::FeatureList::IsEnabled(kIOSSaveToDriveSignedOut)) {
if ([_mediator selectedFileDestinationRequiresSignin]) {
[self openSignIn];
return;
}
}
```
Done
UIViewController* presenter =
_accountPickerCoordinator.viewController ?: self.baseViewController;
CHECK(presenter);
[presenter presentViewController:_alertController
animated:YES
completion:nil];Joseph Lanzaditto
Done
UIViewController* presenter =
_accountPickerCoordinator.viewController ?: self.baseViewController;
[googleOneHandler
showGoogleOneForIdentity:identity
entryPoint:GoogleOneEntryPoint::kSaveToDriveAlert
baseViewController:presenter];Joseph Lanzaditto
Done
This is unnecessary, `self` can be used directly.
Done
bool signedIn = [self isSignedIn];
bool identityButtonHidden =
_fileDestination == FileDestination::kFiles || !signedIn;
[self.accountPickerConsumer setIdentityButtonHidden:identityButtonHidden- 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 1 comment![]( "Open in Gerrit")
LGTM, will +1 once tests have been added
Related details
- Joseph Lanza
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")
| Code-Review | +1 |
Related details
- Joseph Lanza
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")
| Code-Review | +1 |
Related details
- Joseph Lanza
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")
| Code-Review | +1 |
Related details
- Joseph Lanza
- Olivier Robin
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 added 6 comments![]( "Open in Gerrit")
[self openSignIn];Why do we need to signin when an identity is selected? Is it only if `identity == nil` ?
_accountPickerCoordinator.viewController ?: self.baseViewController;why is this needed? Do we add a way to access storage management?
Is it possible that baseViewController presents another VC?
[_accountPickerCoordinator stopAnimated:YES];can there be a problem with the animation here? Does the show signin command wait for this dismissal to be complete?
[_accountPickerCoordinator stopAnimated:YES];can you check what happens if sign in is disabled by policy?
[saveToDriveHandler hideSaveToDrive];do we want to go back to the method selection? Is it possible that we lose the download here?
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")
Why do we need to signin when an identity is selected? Is it only if `identity == nil` ?
Local file downloads must not require user signing in.
The `saveWithSelectedIdentity:identity` method manages both local and drive storage logic; however, a sign-in prompt should only be triggered if the user attempts to save to Drive while signed out.
_accountPickerCoordinator.viewController ?: self.baseViewController;why is this needed? Do we add a way to access storage management?
Is it possible that baseViewController presents another VC?
When the sign-in screen opens, the account picker must be dismissed first.
To show the storage manager after a sign in, it should be presented on top of the baseViewController, and not the account picker.
[_accountPickerCoordinator stopAnimated:YES];can you check what happens if sign in is disabled by policy?
If "sign in" is disabled by policy, Drive is not an option you can select, therefore this code path is unreachable.
[_accountPickerCoordinator stopAnimated:YES];can there be a problem with the animation here? Does the show signin command wait for this dismissal to be complete?
I don't think so but I am not sure how to test this.
[saveToDriveHandler hideSaveToDrive];do we want to go back to the method selection? Is it possible that we lose the download here?
If the login fails, the sign in screen close and you need to tap the "Save..." button again to display the account picker again. The download is not lost.
anf the user is not signed in.
- Olivier Robin
- Quentin Pubert
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. |
Olivier Robin (Gerrit)
Olivier Robin voted and added 4 comments![]( "Open in Gerrit")
Votes added by Olivier Robin
| Code-Review | +1 |
4 comments
_accountPickerCoordinator.viewController ?: self.baseViewController;Joseph Lanzawhy is this needed? Do we add a way to access storage management?
Is it possible that baseViewController presents another VC?
When the sign-in screen opens, the account picker must be dismissed first.
To show the storage manager after a sign in, it should be presented on top of the baseViewController, and not the account picker.
Would it be better to have a variable "isPresentingAccountPicker" to check?
[_accountPickerCoordinator stopAnimated:YES];Joseph Lanzacan there be a problem with the animation here? Does the show signin command wait for this dismissal to be complete?
I don't think so but I am not sure how to test this.
Ack. It may still cause issues in the future, so maybe try to see why it works and why the new VC is not blocked by the dismissed account picker.
[_accountPickerCoordinator stopAnimated:YES];Joseph Lanzacan you check what happens if sign in is disabled by policy?
If "sign in" is disabled by policy, Drive is not an option you can select, therefore this code path is unreachable.
Acknowledged
[saveToDriveHandler hideSaveToDrive];Joseph Lanzado we want to go back to the method selection? Is it possible that we lose the download here?
If the login fails, the sign in screen close and you need to tap the "Save..." button again to display the account picker again. The download is not lost.
- Joseph Lanza
- Quentin Pubert
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")
_accountPickerCoordinator.viewController ?: self.baseViewController;Joseph Lanzawhy is this needed? Do we add a way to access storage management?
Is it possible that baseViewController presents another VC?
Olivier RobinWhen the sign-in screen opens, the account picker must be dismissed first.
To show the storage manager after a sign in, it should be presented on top of the baseViewController, and not the account picker.
Would it be better to have a variable "isPresentingAccountPicker" to check?
changed to check _accountPickerCoordinator == nil, and use self.baseViewController as fallback.
[_accountPickerCoordinator stopAnimated:YES];Joseph Lanzacan there be a problem with the animation here? Does the show signin command wait for this dismissal to be complete?
Olivier RobinI don't think so but I am not sure how to test this.
Ack. It may still cause issues in the future, so maybe try to see why it works and why the new VC is not blocked by the dismissed account picker.
- Olivier Robin
- Quentin Pubert
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. |
Olivier Robin (Gerrit)
Olivier Robin added 1 comment![]( "Open in Gerrit")
[_accountPickerCoordinator stopAnimated:YES];should you set it to nil then?
same for other places where it is stopped.
Related details
- Joseph Lanza
- Quentin Pubert
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. |
Quentin Pubert (Gerrit)
Quentin Pubert added 1 comment![]( "Open in Gerrit")
[_accountPickerCoordinator stopAnimated:YES];should you set it to nil then?
same for other places where it is stopped.
It is already set to nil in `accountPickerCoordinatorDidStop`
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. |
Olivier Robin (Gerrit)
Olivier Robin voted and added 1 comment![]( "Open in Gerrit")
Votes added by Olivier Robin
| Code-Review | +1 |
1 comment
[_accountPickerCoordinator stopAnimated:YES];Quentin Pubertshould you set it to nil then?
same for other places where it is stopped.
It is already set to nil in `accountPickerCoordinatorDidStop`
- Joseph Lanza
- Quentin Pubert
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. |