Skip to content

Commit

Permalink
[iOS][Bkmrk] Block the UI when the sign-in is progress
Browse files Browse the repository at this point in the history
This patch adds an UI blocker on other windows when sign-in is started
from the bookmark or reading list promo with the account storage
enabled.
This fix is not ideal, but this patch needs to be cherry-picked in
M115.

The best fix is to create a SigninCoordinator sub class that would
take care of showing the identity picker coordinator or/and showing
the add account coordinator and trigger the sign-in flow.
With this new Signincoordinator sub class, SigninPromoViewMediator
would need to send a sign-in command, and it would not need to block
the UI.

This cleanup is captured in crbug.com/1448830.

(cherry picked from commit 2faf3a3)

Fixed: 1447949
Change-Id: I2e02e6f6d0428371b782314e623ba2549bbe3909
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4565279
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
Auto-Submit: Jérôme Lebel <jlebel@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1149182}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4598016
Reviewed-by: Menghan Yang <myuu@google.com>
Reviewed-by: Arthur Milchior <arthurmilchior@chromium.org>
Cr-Commit-Position: refs/branch-heads/5790@{#533}
Cr-Branched-From: 1d71a33-refs/heads/main@{#1148114}
  • Loading branch information
Jérôme Lebel authored and Chromium LUCI CQ committed Jun 9, 2023
1 parent 4d5ce49 commit a0f7a75
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 1 deletion.
3 changes: 3 additions & 0 deletions ios/chrome/browser/ui/authentication/BUILD.gn
Expand Up @@ -46,6 +46,8 @@ source_set("authentication") {
"//ios/chrome/browser/infobars",
"//ios/chrome/browser/policy",
"//ios/chrome/browser/shared/coordinator/alert",
"//ios/chrome/browser/shared/coordinator/scene:scene_state_browser_agent",
"//ios/chrome/browser/shared/coordinator/scene:scene_state_header",
"//ios/chrome/browser/shared/model/application_context",
"//ios/chrome/browser/shared/model/browser",
"//ios/chrome/browser/shared/model/browser_state",
Expand All @@ -64,6 +66,7 @@ source_set("authentication") {
"//ios/chrome/browser/ui/authentication/signin:signin_headers",
"//ios/chrome/browser/ui/authentication/unified_consent/identity_chooser",
"//ios/chrome/browser/ui/ntp:feature_flags",
"//ios/chrome/browser/ui/scoped_ui_blocker",
"//ios/chrome/browser/ui/settings:settings_root",
"//ios/chrome/browser/unified_consent",
"//ios/chrome/common",
Expand Down
3 changes: 3 additions & 0 deletions ios/chrome/browser/ui/authentication/DEPS
Expand Up @@ -6,4 +6,7 @@ include_rules = [
"+ios/chrome/browser/ui/first_run",
"+ios/chrome/browser/ui/recent_tabs/recent_tabs_constants.h",
"+ios/chrome/browser/ui/ntp/new_tab_page_feature.h",

# TODO(crbug.com/1448830): Remove this deps.
"+ios/chrome/browser/ui/scoped_ui_blocker",
]
Expand Up @@ -15,6 +15,8 @@
#import "components/signin/public/base/signin_metrics.h"
#import "ios/chrome/browser/discover_feed/feed_constants.h"
#import "ios/chrome/browser/flags/system_flags.h"
#import "ios/chrome/browser/shared/coordinator/scene/scene_state.h"
#import "ios/chrome/browser/shared/coordinator/scene/scene_state_browser_agent.h"
#import "ios/chrome/browser/shared/model/prefs/pref_names.h"
#import "ios/chrome/browser/shared/public/commands/application_commands.h"
#import "ios/chrome/browser/shared/public/commands/show_signin_command.h"
Expand All @@ -31,6 +33,7 @@
#import "ios/chrome/browser/ui/authentication/unified_consent/identity_chooser/identity_chooser_coordinator.h"
#import "ios/chrome/browser/ui/authentication/unified_consent/identity_chooser/identity_chooser_coordinator_delegate.h"
#import "ios/chrome/browser/ui/ntp/new_tab_page_feature.h"
#import "ios/chrome/browser/ui/scoped_ui_blocker/scoped_ui_blocker.h"
#import "ios/chrome/grit/ios_strings.h"
#import "ui/base/l10n/l10n_util.h"

Expand Down Expand Up @@ -534,6 +537,9 @@ @implementation SigninPromoViewMediator {
IdentityChooserCoordinator* _identityChooserCoordinator;
// Coordinator to add an account.
SigninCoordinator* _signinCoordinator;
// TODO(crbug.com/1448830): This class should not need to block the UI.
// The UI blocker is only used in sign-in only cases.
std::unique_ptr<ScopedUIBlocker> _uiBlocker;
}

+ (void)registerBrowserStatePrefs:(user_prefs::PrefRegistrySyncable*)registry {
Expand Down Expand Up @@ -883,6 +889,9 @@ - (void)showSigninWithIdentity:(id<SystemIdentity>)identity
// Triggers the primary action when `signInOnly` is at YES: starts sign-in flow.
- (void)primaryActionForSignInOnly {
DCHECK(self.signInOnly) << base::SysNSStringToUTF8([self description]);
SceneState* sceneState =
SceneStateBrowserAgent::FromBrowser(_browser)->GetSceneState();
_uiBlocker = std::make_unique<ScopedUIBlocker>(sceneState);
signin_metrics::RecordSigninUserActionForAccessPoint(self.accessPoint);
self.signinPromoViewState = ios::SigninPromoViewState::UsedAtLeastOnce;
self.signinInProgress = YES;
Expand All @@ -893,6 +902,9 @@ - (void)primaryActionForSignInOnly {
// add account dialog.
- (void)secondaryActionForSignInOnly {
DCHECK(self.signInOnly) << base::SysNSStringToUTF8([self description]);
SceneState* sceneState =
SceneStateBrowserAgent::FromBrowser(_browser)->GetSceneState();
_uiBlocker = std::make_unique<ScopedUIBlocker>(sceneState);
signin_metrics::RecordSigninUserActionForAccessPoint(self.accessPoint);
self.signinPromoViewState = ios::SigninPromoViewState::UsedAtLeastOnce;
self.signinInProgress = YES;
Expand All @@ -916,17 +928,29 @@ - (void)startSignInOnlyFlow {
__weak id<SigninPromoViewConsumer> weakConsumer = self.consumer;
__weak __typeof(self) weakSelf = self;
[_authenticationFlow startSignInWithCompletion:^(BOOL success) {
[weakSelf signInFlowCompletedForSignInOnly];
if ([weakConsumer respondsToSelector:@selector(signinDidFinish)]) {
weakSelf.signinInProgress = NO;
[weakConsumer signinDidFinish];
}
}];
}

// Called when the sign-in flow is over. This method should only be called
// when this is a sign-in only flow.
- (void)signInFlowCompletedForSignInOnly {
DCHECK(self.signInOnly) << base::SysNSStringToUTF8([self description]);
_uiBlocker.reset();
self.signinInProgress = NO;
}

- (void)startAddAccountForSignInOnly {
DCHECK(!_signinCoordinator)
<< base::SysNSStringToUTF8([_signinCoordinator description]) << " "
<< base::SysNSStringToUTF8([self description]);
DCHECK(self.signInOnly) << base::SysNSStringToUTF8([self description]);
SceneState* sceneState =
SceneStateBrowserAgent::FromBrowser(_browser)->GetSceneState();
_uiBlocker = std::make_unique<ScopedUIBlocker>(sceneState);
self.signinPromoViewState = ios::SigninPromoViewState::UsedAtLeastOnce;
self.signinInProgress = YES;
_signinCoordinator = [SigninCoordinator
Expand All @@ -952,6 +976,7 @@ - (void)addAccountDoneWithResult:(SigninCoordinatorResult)result
break;
case SigninCoordinatorResultInterrupted:
case SigninCoordinatorResultCanceledByUser:
_uiBlocker.reset();
self.signinInProgress = NO;
break;
}
Expand Down Expand Up @@ -1125,6 +1150,7 @@ - (void)identityChooserCoordinator:(IdentityChooserCoordinator*)coordinator
_identityChooserCoordinator = nil;
if (!identity) {
self.signinInProgress = NO;
_uiBlocker.reset();
return;
}
self.identity = identity;
Expand Down

0 comments on commit a0f7a75

Please sign in to comment.