Commit cae35cf1 authored by Hanfeng Zhu's avatar Hanfeng Zhu Committed by Commit Bot

Revert "Disable signin promos for special users."

This reverts commit 6a7745fd.

Reason for revert: http://crbug/960172

Bug: 960172

Original change's description:
> Disable signin promos for special users.
>
> Add a new method "isSignInPromoAllowed()" in SigninPromoController to
> check whether promos are allowed. The reason is that in some regions
> signing in may be problematic, thus promos should be suppressed.
>
> Bug: 948983
> Change-Id: I389b1d2d693a0bcb63571e78991f7287f734672f
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1548875
> Commit-Queue: Hanfeng Zhu <hanfeng@google.com>
> Reviewed-by: Boris Sazonov <bsazonov@chromium.org>
> Reviewed-by: Ted Choc <tedchoc@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#647994}

TBR=msarda@chromium.org,tedchoc@chromium.org,bsazonov@chromium.org,hanfeng@google.com

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 948983
Change-Id: I5b5ee040891272ae5010becf11529055a08c1324
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1596790Reviewed-by: default avatarMihai Sardarescu <msarda@chromium.org>
Reviewed-by: default avatarTed Choc <tedchoc@chromium.org>
Commit-Queue: Hanfeng Zhu <hanfeng@google.com>
Cr-Commit-Position: refs/heads/master@{#657551}
parent e442cb75
...@@ -201,8 +201,8 @@ class BookmarkPromoHeader implements AndroidSyncSettingsObserver, SignInStateObs ...@@ -201,8 +201,8 @@ class BookmarkPromoHeader implements AndroidSyncSettingsObserver, SignInStateObs
if (!ChromeSigninController.get().isSignedIn()) { if (!ChromeSigninController.get().isSignedIn()) {
boolean impressionLimitReached = !SigninPromoController.hasNotReachedImpressionLimit( boolean impressionLimitReached = !SigninPromoController.hasNotReachedImpressionLimit(
SigninAccessPoint.BOOKMARK_MANAGER); SigninAccessPoint.BOOKMARK_MANAGER);
if (!mSignInManager.isSignInAllowed() || !SigninPromoController.isSignInPromoAllowed() if (!mSignInManager.isSignInAllowed() || impressionLimitReached
|| impressionLimitReached || wasPersonalizedSigninPromoDeclined()) { || wasPersonalizedSigninPromoDeclined()) {
return PromoState.PROMO_NONE; return PromoState.PROMO_NONE;
} }
return PromoState.PROMO_SIGNIN_PERSONALIZED; return PromoState.PROMO_SIGNIN_PERSONALIZED;
......
...@@ -26,7 +26,6 @@ import org.chromium.chrome.browser.preferences.PrefServiceBridge; ...@@ -26,7 +26,6 @@ import org.chromium.chrome.browser.preferences.PrefServiceBridge;
import org.chromium.chrome.browser.preferences.privacy.PrivacyPreferencesManager; import org.chromium.chrome.browser.preferences.privacy.PrivacyPreferencesManager;
import org.chromium.chrome.browser.services.AndroidEduAndChildAccountHelper; import org.chromium.chrome.browser.services.AndroidEduAndChildAccountHelper;
import org.chromium.chrome.browser.signin.SigninManager; import org.chromium.chrome.browser.signin.SigninManager;
import org.chromium.chrome.browser.signin.SigninPromoController;
import org.chromium.chrome.browser.util.FeatureUtilities; import org.chromium.chrome.browser.util.FeatureUtilities;
import org.chromium.chrome.browser.util.IntentUtils; import org.chromium.chrome.browser.util.IntentUtils;
import org.chromium.chrome.browser.vr.VrModuleProvider; import org.chromium.chrome.browser.vr.VrModuleProvider;
...@@ -105,11 +104,6 @@ public abstract class FirstRunFlowSequencer { ...@@ -105,11 +104,6 @@ public abstract class FirstRunFlowSequencer {
&& signinManager.isSigninSupported(); && signinManager.isSigninSupported();
} }
@VisibleForTesting
protected boolean isSigninPromoAllowed() {
return SigninPromoController.isSignInPromoAllowed();
}
@VisibleForTesting @VisibleForTesting
protected List<Account> getGoogleAccounts() { protected List<Account> getGoogleAccounts() {
return AccountManagerFacade.get().tryGetGoogleAccounts(); return AccountManagerFacade.get().tryGetGoogleAccounts();
...@@ -201,7 +195,6 @@ public abstract class FirstRunFlowSequencer { ...@@ -201,7 +195,6 @@ public abstract class FirstRunFlowSequencer {
// - no "skip the first use hints" is set, or // - no "skip the first use hints" is set, or
// - "skip the first use hints" is set, but there is at least one account. // - "skip the first use hints" is set, but there is at least one account.
boolean offerSignInOk = isSyncAllowed() && !isSignedIn() && !mForceEduSignIn boolean offerSignInOk = isSyncAllowed() && !isSignedIn() && !mForceEduSignIn
&& isSigninPromoAllowed()
&& (!shouldSkipFirstUseHints() || !mGoogleAccounts.isEmpty()); && (!shouldSkipFirstUseHints() || !mGoogleAccounts.isEmpty());
freProperties.putBoolean(FirstRunActivity.SHOW_SIGNIN_PAGE, offerSignInOk); freProperties.putBoolean(FirstRunActivity.SHOW_SIGNIN_PAGE, offerSignInOk);
if (mForceEduSignIn || ChildAccountStatus.isChild(mChildAccountStatus)) { if (mForceEduSignIn || ChildAccountStatus.isChild(mChildAccountStatus)) {
......
...@@ -367,8 +367,7 @@ public class RecentTabsManager implements AndroidSyncSettingsObserver, SignInSta ...@@ -367,8 +367,7 @@ public class RecentTabsManager implements AndroidSyncSettingsObserver, SignInSta
@PromoState @PromoState
int getPromoType() { int getPromoType() {
if (!ChromeSigninController.get().isSignedIn()) { if (!ChromeSigninController.get().isSignedIn()) {
if (!SigninManager.get().isSignInAllowed() if (!SigninManager.get().isSignInAllowed()) {
|| !SigninPromoController.isSignInPromoAllowed()) {
return PromoState.PROMO_NONE; return PromoState.PROMO_NONE;
} }
return PromoState.PROMO_SIGNIN_PERSONALIZED; return PromoState.PROMO_SIGNIN_PERSONALIZED;
......
...@@ -41,9 +41,9 @@ public class SignInPromo extends OptionalLeaf { ...@@ -41,9 +41,9 @@ public class SignInPromo extends OptionalLeaf {
private boolean mDismissed; private boolean mDismissed;
/** /**
* Whether signin promo can be shown. * Whether the signin status means that the user has the possibility to sign in.
*/ */
private boolean mShowSigninPromo; private boolean mCanSignIn;
/** /**
* Whether personalized suggestions can be shown. If it's not the case, we have no reason to * Whether personalized suggestions can be shown. If it's not the case, we have no reason to
...@@ -59,8 +59,7 @@ public class SignInPromo extends OptionalLeaf { ...@@ -59,8 +59,7 @@ public class SignInPromo extends OptionalLeaf {
Context context = ContextUtils.getApplicationContext(); Context context = ContextUtils.getApplicationContext();
SigninManager signinManager = SigninManager.get(); SigninManager signinManager = SigninManager.get();
mShowSigninPromo = signinManager.isSignInAllowed() && !signinManager.isSignedInOnNative() mCanSignIn = signinManager.isSignInAllowed() && !signinManager.isSignedInOnNative();
&& SigninPromoController.isSignInPromoAllowed();
updateVisibility(); updateVisibility();
int imageSize = context.getResources().getDimensionPixelSize(R.dimen.user_picture_size); int imageSize = context.getResources().getDimensionPixelSize(R.dimen.user_picture_size);
...@@ -139,7 +138,7 @@ public class SignInPromo extends OptionalLeaf { ...@@ -139,7 +138,7 @@ public class SignInPromo extends OptionalLeaf {
} }
private void updateVisibility() { private void updateVisibility() {
setVisibilityInternal(!mDismissed && mShowSigninPromo && mCanShowPersonalizedSuggestions); setVisibilityInternal(!mDismissed && mCanSignIn && mCanShowPersonalizedSuggestions);
} }
@Override @Override
...@@ -205,22 +204,20 @@ public class SignInPromo extends OptionalLeaf { ...@@ -205,22 +204,20 @@ public class SignInPromo extends OptionalLeaf {
// Listening to onSignInAllowedChanged is important for the FRE. Sign in is not allowed // Listening to onSignInAllowedChanged is important for the FRE. Sign in is not allowed
// until it is completed, but the NTP is initialised before the FRE is even shown. By // until it is completed, but the NTP is initialised before the FRE is even shown. By
// implementing this we can show the promo if the user did not sign in during the FRE. // implementing this we can show the promo if the user did not sign in during the FRE.
mShowSigninPromo = mSigninManager.isSignInAllowed() mCanSignIn = mSigninManager.isSignInAllowed();
&& SigninPromoController.isSignInPromoAllowed();
updateVisibility(); updateVisibility();
} }
// SignInStateObserver implementation. // SignInStateObserver implementation.
@Override @Override
public void onSignedIn() { public void onSignedIn() {
mShowSigninPromo = false; mCanSignIn = false;
updateVisibility(); updateVisibility();
} }
@Override @Override
public void onSignedOut() { public void onSignedOut() {
mShowSigninPromo = mSigninManager.isSignInAllowed() mCanSignIn = mSigninManager.isSignInAllowed();
&& SigninPromoController.isSignInPromoAllowed();
updateVisibility(); updateVisibility();
} }
......
...@@ -168,8 +168,7 @@ public class SignInPreference ...@@ -168,8 +168,7 @@ public class SignInPreference
boolean personalizedPromoDismissed = ChromePreferenceManager.getInstance().readBoolean( boolean personalizedPromoDismissed = ChromePreferenceManager.getInstance().readBoolean(
ChromePreferenceManager.SETTINGS_PERSONALIZED_SIGNIN_PROMO_DISMISSED, false); ChromePreferenceManager.SETTINGS_PERSONALIZED_SIGNIN_PROMO_DISMISSED, false);
if (!SigninPromoController.isSignInPromoAllowed() || !mPersonalizedPromoEnabled if (!mPersonalizedPromoEnabled || personalizedPromoDismissed) {
|| personalizedPromoDismissed) {
setupGenericPromo(); setupGenericPromo();
return; return;
} }
......
...@@ -20,11 +20,11 @@ import org.chromium.base.metrics.RecordHistogram; ...@@ -20,11 +20,11 @@ import org.chromium.base.metrics.RecordHistogram;
import org.chromium.base.metrics.RecordUserAction; import org.chromium.base.metrics.RecordUserAction;
import org.chromium.chrome.R; import org.chromium.chrome.R;
import org.chromium.chrome.browser.ChromeFeatureList; import org.chromium.chrome.browser.ChromeFeatureList;
import org.chromium.chrome.browser.locale.LocaleManager;
import org.chromium.chrome.browser.metrics.ImpressionTracker; import org.chromium.chrome.browser.metrics.ImpressionTracker;
import org.chromium.chrome.browser.metrics.OneShotImpressionListener; import org.chromium.chrome.browser.metrics.OneShotImpressionListener;
import org.chromium.chrome.browser.signin.AccountSigninActivity.AccessPoint; import org.chromium.chrome.browser.signin.AccountSigninActivity.AccessPoint;
/** /**
* A controller for configuring the sign in promo. It sets up the sign in promo depending on the * A controller for configuring the sign in promo. It sets up the sign in promo depending on the
* context: whether there are any Google accounts on the device which have been previously signed in * context: whether there are any Google accounts on the device which have been previously signed in
...@@ -273,14 +273,6 @@ public class SigninPromoController { ...@@ -273,14 +273,6 @@ public class SigninPromoController {
return mProfileData == null ? mDescriptionStringIdNoAccount : mDescriptionStringId; return mProfileData == null ? mDescriptionStringIdNoAccount : mDescriptionStringId;
} }
/**
* Returns true if signin promos should be shown on this device.
* In some regions signing in may be problematic, thus promos should be suppressed.
*/
public static boolean isSignInPromoAllowed() {
return !LocaleManager.getInstance().isSpecialUser();
}
private void setupColdState(final Context context, PersonalizedSigninPromoView view) { private void setupColdState(final Context context, PersonalizedSigninPromoView view) {
view.getImage().setImageResource(R.drawable.chrome_sync_logo); view.getImage().setImageResource(R.drawable.chrome_sync_logo);
setImageSize(context, view, R.dimen.signin_promo_cold_state_image_size); setImageSize(context, view, R.dimen.signin_promo_cold_state_image_size);
......
...@@ -52,7 +52,6 @@ public class FirstRunFlowSequencerTest { ...@@ -52,7 +52,6 @@ public class FirstRunFlowSequencerTest {
public boolean isFirstRunFlowComplete; public boolean isFirstRunFlowComplete;
public boolean isSignedIn; public boolean isSignedIn;
public boolean isSyncAllowed; public boolean isSyncAllowed;
public boolean isSigninPromoAllowed;
public List<Account> googleAccounts; public List<Account> googleAccounts;
public boolean hasAnyUserSeenToS; public boolean hasAnyUserSeenToS;
public boolean shouldSkipFirstUseHints; public boolean shouldSkipFirstUseHints;
...@@ -86,11 +85,6 @@ public class FirstRunFlowSequencerTest { ...@@ -86,11 +85,6 @@ public class FirstRunFlowSequencerTest {
return isSyncAllowed; return isSyncAllowed;
} }
@Override
public boolean isSigninPromoAllowed() {
return isSigninPromoAllowed;
}
@Override @Override
public List<Account> getGoogleAccounts() { public List<Account> getGoogleAccounts() {
return googleAccounts; return googleAccounts;
...@@ -152,7 +146,6 @@ public class FirstRunFlowSequencerTest { ...@@ -152,7 +146,6 @@ public class FirstRunFlowSequencerTest {
mSequencer.isFirstRunFlowComplete = true; mSequencer.isFirstRunFlowComplete = true;
mSequencer.isSignedIn = false; mSequencer.isSignedIn = false;
mSequencer.isSyncAllowed = true; mSequencer.isSyncAllowed = true;
mSequencer.isSigninPromoAllowed = true;
mSequencer.googleAccounts = mSequencer.googleAccounts =
Collections.singletonList(new Account(DEFAULT_ACCOUNT, GOOGLE_ACCOUNT_TYPE)); Collections.singletonList(new Account(DEFAULT_ACCOUNT, GOOGLE_ACCOUNT_TYPE));
mSequencer.hasAnyUserSeenToS = true; mSequencer.hasAnyUserSeenToS = true;
...@@ -173,7 +166,6 @@ public class FirstRunFlowSequencerTest { ...@@ -173,7 +166,6 @@ public class FirstRunFlowSequencerTest {
mSequencer.isFirstRunFlowComplete = false; mSequencer.isFirstRunFlowComplete = false;
mSequencer.isSignedIn = false; mSequencer.isSignedIn = false;
mSequencer.isSyncAllowed = true; mSequencer.isSyncAllowed = true;
mSequencer.isSigninPromoAllowed = true;
mSequencer.googleAccounts = Collections.emptyList(); mSequencer.googleAccounts = Collections.emptyList();
mSequencer.hasAnyUserSeenToS = false; mSequencer.hasAnyUserSeenToS = false;
mSequencer.shouldSkipFirstUseHints = false; mSequencer.shouldSkipFirstUseHints = false;
...@@ -202,7 +194,6 @@ public class FirstRunFlowSequencerTest { ...@@ -202,7 +194,6 @@ public class FirstRunFlowSequencerTest {
mSequencer.isFirstRunFlowComplete = false; mSequencer.isFirstRunFlowComplete = false;
mSequencer.isSignedIn = false; mSequencer.isSignedIn = false;
mSequencer.isSyncAllowed = true; mSequencer.isSyncAllowed = true;
mSequencer.isSigninPromoAllowed = true;
mSequencer.googleAccounts = mSequencer.googleAccounts =
Collections.singletonList(new Account(DEFAULT_ACCOUNT, GOOGLE_ACCOUNT_TYPE)); Collections.singletonList(new Account(DEFAULT_ACCOUNT, GOOGLE_ACCOUNT_TYPE));
mSequencer.hasAnyUserSeenToS = false; mSequencer.hasAnyUserSeenToS = false;
...@@ -234,7 +225,6 @@ public class FirstRunFlowSequencerTest { ...@@ -234,7 +225,6 @@ public class FirstRunFlowSequencerTest {
mSequencer.isFirstRunFlowComplete = false; mSequencer.isFirstRunFlowComplete = false;
mSequencer.isSignedIn = false; mSequencer.isSignedIn = false;
mSequencer.isSyncAllowed = true; mSequencer.isSyncAllowed = true;
mSequencer.isSigninPromoAllowed = true;
mSequencer.googleAccounts = Collections.emptyList(); mSequencer.googleAccounts = Collections.emptyList();
mSequencer.hasAnyUserSeenToS = false; mSequencer.hasAnyUserSeenToS = false;
mSequencer.shouldSkipFirstUseHints = false; mSequencer.shouldSkipFirstUseHints = false;
...@@ -264,7 +254,6 @@ public class FirstRunFlowSequencerTest { ...@@ -264,7 +254,6 @@ public class FirstRunFlowSequencerTest {
mSequencer.isFirstRunFlowComplete = false; mSequencer.isFirstRunFlowComplete = false;
mSequencer.isSignedIn = false; mSequencer.isSignedIn = false;
mSequencer.isSyncAllowed = true; mSequencer.isSyncAllowed = true;
mSequencer.isSigninPromoAllowed = true;
mSequencer.googleAccounts = Collections.emptyList(); mSequencer.googleAccounts = Collections.emptyList();
mSequencer.hasAnyUserSeenToS = false; mSequencer.hasAnyUserSeenToS = false;
mSequencer.shouldSkipFirstUseHints = false; mSequencer.shouldSkipFirstUseHints = false;
...@@ -287,34 +276,4 @@ public class FirstRunFlowSequencerTest { ...@@ -287,34 +276,4 @@ public class FirstRunFlowSequencerTest {
bundle.getInt(AccountFirstRunFragment.CHILD_ACCOUNT_STATUS)); bundle.getInt(AccountFirstRunFragment.CHILD_ACCOUNT_STATUS));
assertEquals(5, bundle.size()); assertEquals(5, bundle.size());
} }
@Test
@Feature({"FirstRun"})
public void testStandardFlowSignInPageNotSeen() {
mSequencer.isFirstRunFlowComplete = false;
mSequencer.isSignedIn = false;
mSequencer.isSyncAllowed = true;
mSequencer.isSigninPromoAllowed = false;
mSequencer.googleAccounts = Collections.emptyList();
mSequencer.hasAnyUserSeenToS = false;
mSequencer.shouldSkipFirstUseHints = false;
mSequencer.shouldShowDataReductionPage = true;
mSequencer.shouldShowSearchEnginePage = false;
mSequencer.initializeSharedState(
false /* androidEduDevice */, ChildAccountStatus.NOT_CHILD);
mSequencer.processFreEnvironmentPreNative();
assertTrue(mSequencer.calledOnFlowIsKnown);
assertTrue(mSequencer.calledSetDefaultMetricsAndCrashReporting);
assertFalse(mSequencer.calledSetFirstRunFlowSignInComplete);
Bundle bundle = mSequencer.returnedBundle;
assertTrue(bundle.getBoolean(FirstRunActivityBase.SHOW_WELCOME_PAGE));
assertFalse(bundle.getBoolean(FirstRunActivityBase.SHOW_SIGNIN_PAGE));
assertTrue(bundle.getBoolean(FirstRunActivityBase.SHOW_DATA_REDUCTION_PAGE));
assertFalse(bundle.getBoolean(FirstRunActivityBase.SHOW_SEARCH_ENGINE_PAGE));
assertEquals(ChildAccountStatus.NOT_CHILD,
bundle.getInt(AccountFirstRunFragment.CHILD_ACCOUNT_STATUS));
assertEquals(5, bundle.size());
}
} }
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment