Commit c9e213f4 authored by Wenyu Fu's avatar Wenyu Fu Committed by Commit Bot

Reland "[HomepagePromo] Add finch param supressing_sign_in_promo"

This reverts commit c41c5c46.

Reason for revert: Changed to a different implementation, and fix the flaky test failed on M+ bots.

Original change's description:
> Revert "[HomepagePromo] Add finch param supressing_sign_in_promo"
>
> This reverts commit 25269f24.
>
> Reason for revert: test testSetUp_SuppressingSignInPromo fails constantly in android-arm64-proguard-rel (and fyi builder android-marshmallow-x86-fyi-rel) since
> * https://ci.chromium.org/p/chromium/builders/ci/android-arm64-proguard-rel/2020
> * https://ci.chromium.org/p/chromium/builders/ci/android-marshmallow-x86-fyi-rel/767
>
> Original change's description:
> > [HomepagePromo] Add finch param supressing_sign_in_promo
> >
> > Adding the option for homepage promo to suppress the sign in promo when
> > user is not signed in.
> >
> > Bug: 1068831
> > Change-Id: I854109c4623c47f98aab1faa34f94f2d55961474
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2252379
> > Commit-Queue: Wenyu Fu <wenyufu@chromium.org>
> > Reviewed-by: Theresa  <twellington@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#781109}
>
> TBR=twellington@chromium.org,wenyufu@chromium.org
>
> # Not skipping CQ checks because original CL landed > 1 day ago.
>
> Bug: 1068831
> Change-Id: I97f226e93962469cc8a705e922a3db72d7e84cd8
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2265398
> Reviewed-by: Haiyang Pan <hypan@google.com>
> Reviewed-by: Wenyu Fu <wenyufu@chromium.org>
> Commit-Queue: Haiyang Pan <hypan@google.com>
> Cr-Commit-Position: refs/heads/master@{#782189}

TBR=twellington@chromium.org,hypan@google.com,wenyufu@chromium.org

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

Bug: 1068831
Change-Id: Id652d450f84f44f4d15b2409c61c3cf84fa7c48e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2269484
Commit-Queue: Wenyu Fu <wenyufu@chromium.org>
Reviewed-by: default avatarTheresa  <twellington@chromium.org>
Cr-Commit-Position: refs/heads/master@{#785953}
parent 90d2f5c0
......@@ -233,20 +233,14 @@ public class FeedSurfaceCoordinator implements FeedSurfaceProvider {
mTracker = TrackerFactory.getTrackerForProfile(profile);
// Mediator should be created before any Stream changes.
mMediator = new FeedSurfaceMediator(this, snapScrollHelper, mPageNavigationDelegate);
// Add the homepage promo card when the feed is enabled and the feature is enabled. A null
// mStream object means that the feed is disabled. The intialization of the mStream object
// is handled during the construction of the FeedSurfaceMediator where there might be cases
// where the mStream object remains null because the feed is disabled, in which case the
// homepage promo card cannot be added to the feed even if the feature is enabled.
if (mStream != null && ChromeFeatureList.isEnabled(ChromeFeatureList.HOMEPAGE_PROMO_CARD)) {
if (ChromeFeatureList.isEnabled(ChromeFeatureList.HOMEPAGE_PROMO_CARD)) {
mHomepagePromoController =
new HomepagePromoController(mActivity, mSnackbarManager, mTracker, mMediator);
mMediator.onHomepagePromoStateChange();
new HomepagePromoController(mActivity, mSnackbarManager, mTracker);
}
// Mediator should be created before any Stream changes.
mMediator = new FeedSurfaceMediator(this, snapScrollHelper, mPageNavigationDelegate);
mUserEducationHelper = new UserEducationHelper(mActivity, mHandler);
}
......@@ -449,8 +443,10 @@ public class FeedSurfaceCoordinator implements FeedSurfaceProvider {
return mSigninPromoView;
}
/** Update header views in the Stream. */
void updateHeaderViews(boolean isSignInPromoVisible) {
/**
* Update header views in the Stream.
* */
void updateHeaderViews(boolean isSignInPromoVisible, View homepagePromoView) {
if (mStream == null) return;
List<Header> headers = new ArrayList<>();
......@@ -459,14 +455,10 @@ public class FeedSurfaceCoordinator implements FeedSurfaceProvider {
headers.add(new NonDismissibleHeader(mNtpHeader));
}
// TODO(wenyufu): check Finch flag for whether sign-in takes precedence over homepage promo
if (!isSignInPromoVisible && mHomepagePromoController != null) {
View promoView = mHomepagePromoController.getPromoView();
if (promoView != null) {
mHomepagePromoView = promoView;
if (homepagePromoView != null) {
mHomepagePromoView = homepagePromoView;
headers.add(new HomepagePromoHeader());
}
}
if (mSectionHeaderView != null) {
headers.add(new NonDismissibleHeader(mSectionHeaderView));
......@@ -519,6 +511,10 @@ public class FeedSurfaceCoordinator implements FeedSurfaceProvider {
return mUserEducationHelper;
}
HomepagePromoController getHomepagePromoController() {
return mHomepagePromoController;
}
@VisibleForTesting
FeedSurfaceMediator getMediatorForTesting() {
return mMediator;
......
......@@ -28,9 +28,11 @@ import org.chromium.chrome.browser.ntp.NewTabPageLayout;
import org.chromium.chrome.browser.ntp.SnapScrollHelper;
import org.chromium.chrome.browser.ntp.cards.SignInPromo;
import org.chromium.chrome.browser.ntp.cards.promo.HomepagePromoController.HomepagePromoStateListener;
import org.chromium.chrome.browser.ntp.cards.promo.HomepagePromoVariationManager;
import org.chromium.chrome.browser.ntp.snippets.SectionHeader;
import org.chromium.chrome.browser.preferences.Pref;
import org.chromium.chrome.browser.preferences.PrefChangeRegistrar;
import org.chromium.chrome.browser.preferences.PrefServiceBridge;
import org.chromium.chrome.browser.profiles.Profile;
import org.chromium.chrome.browser.search_engines.TemplateUrlServiceFactory;
import org.chromium.chrome.browser.signin.IdentityServicesProvider;
......@@ -253,15 +255,58 @@ class FeedSurfaceMediator implements NewTabPageLayout.ScrollDelegate,
// This is currently only relevant for the two panes start surface.
stream.setStreamContentVisibility(mHasHeader ? mSectionHeader.isExpanded() : true);
if (SignInPromo.shouldCreatePromo()) {
initStreamHeaderViews();
mMemoryPressureCallback = pressure -> mCoordinator.getStream().trim();
MemoryPressureListener.addCallback(mMemoryPressureCallback);
}
private void initStreamHeaderViews() {
View homepagePromoView = null;
boolean signInPromoVisible = false;
if (!HomepagePromoVariationManager.getInstance().isSuppressingSignInPromo()) {
signInPromoVisible = createSignInPromoIfNeeded();
if (!signInPromoVisible) homepagePromoView = createHomepagePromoIfNeeded();
} else {
homepagePromoView = createHomepagePromoIfNeeded();
if (homepagePromoView == null) signInPromoVisible = createSignInPromoIfNeeded();
}
// Post processing - if HomepagePromo is showing, then we set the SignInPromo to null.
if (homepagePromoView != null && mSignInPromo != null) {
mSignInPromo.destroy();
mSignInPromo = null;
}
// We are not going to show two promos at the same time.
mCoordinator.updateHeaderViews(signInPromoVisible, homepagePromoView);
}
/**
* Create and setup the SignInPromo if necessary.
* @return Whether the SignPromo is visible.
*/
private boolean createSignInPromoIfNeeded() {
if (!SignInPromo.shouldCreatePromo()) return false;
if (mSignInPromo == null) {
boolean suggestionsVisible =
PrefServiceBridge.getInstance().getBoolean(Pref.ARTICLES_LIST_VISIBLE);
mSignInPromo = new FeedSignInPromo(mSigninManager);
mSignInPromo.setCanShowPersonalizedSuggestions(suggestionsVisible);
}
return mSignInPromo.isVisible();
}
mCoordinator.updateHeaderViews(mSignInPromo != null && mSignInPromo.isVisible());
private View createHomepagePromoIfNeeded() {
if (mCoordinator.getHomepagePromoController() == null) return null;
mMemoryPressureCallback = pressure -> mCoordinator.getStream().trim();
MemoryPressureListener.addCallback(mMemoryPressureCallback);
View homepagePromoView = mCoordinator.getHomepagePromoController().getPromoView();
if (homepagePromoView != null) {
mCoordinator.getHomepagePromoController().setHomepagePromoStateListener(this);
}
return homepagePromoView;
}
/** Clear any dependencies related to the {@link Stream}. */
......@@ -508,7 +553,10 @@ class FeedSurfaceMediator implements NewTabPageLayout.ScrollDelegate,
@Override
public void onHomepagePromoStateChange() {
mCoordinator.updateHeaderViews(mSignInPromo != null && mSignInPromo.isVisible());
// If the homepage has status update, we'll not show the HomepagePromo again.
// There are cases where the user has their homepage reset to default. This is an edge case
// and we don't have to reflect that change immediately.
mCoordinator.updateHeaderViews(false, null);
}
// IdentityManager.Delegate interface.
......@@ -542,7 +590,7 @@ class FeedSurfaceMediator implements NewTabPageLayout.ScrollDelegate,
if (isVisible() == visible) return;
super.setVisibilityInternal(visible);
mCoordinator.updateHeaderViews(visible);
mCoordinator.updateHeaderViews(visible, null);
maybeUpdateSignInPromo();
}
......
......@@ -63,17 +63,21 @@ public class HomepagePromoController implements HomepageStateListener {
* @param context Context from the activity.
* @param snackbarManager SnackbarManager used to display snackbar.
* @param tracker Tracker for the feature engagement system.
* @param listener Listener to be notified when the promo should be removed from the parent
* view.
*/
public HomepagePromoController(Context context, SnackbarManager snackbarManager,
Tracker tracker, HomepagePromoStateListener listener) {
public HomepagePromoController(
Context context, SnackbarManager snackbarManager, Tracker tracker) {
mContext = context;
mSnackbarController = new HomepagePromoSnackbarController(context, snackbarManager);
// Inform event of creation.
mTracker = tracker;
}
/**
* @param listener Listener to be notified when the promo should be removed from the parent
* view.
*/
public void setHomepagePromoStateListener(@Nullable HomepagePromoStateListener listener) {
mStateListener = listener;
}
......
......@@ -122,6 +122,14 @@ public class HomepagePromoVariationManager {
return LayoutStyle.COMPACT;
}
/**
* @return Whether the homepage promo has a higher priority than SignInPromo.
*/
public boolean isSuppressingSignInPromo() {
return ChromeFeatureList.getFieldTrialParamByFeatureAsBoolean(
ChromeFeatureList.HOMEPAGE_PROMO_CARD, "suppressing_sign_in_promo", false);
}
/**
* If the user has ever seen the homepage promo, add a synthetic tag to the user.
*/
......
......@@ -1488,6 +1488,9 @@ const FeatureEntry::FeatureParam kHomepagePromoCardCompact[] = {
{"promo-card-variation", "Compact"}};
const FeatureEntry::FeatureParam kHomepagePromoCardSlim[] = {
{"promo-card-variation", "Slim"}};
const FeatureEntry::FeatureParam kHomepagePromoCardSuppressing[] = {
{"promo-card-variation", "Compact"},
{"suppressing_sign_in_promo", "true"}};
const FeatureEntry::FeatureVariation kHomepagePromoCardVariations[] = {
{"Large", kHomepagePromoCardLarge, base::size(kHomepagePromoCardLarge),
......@@ -1495,7 +1498,9 @@ const FeatureEntry::FeatureVariation kHomepagePromoCardVariations[] = {
{"Compact", kHomepagePromoCardCompact,
base::size(kHomepagePromoCardCompact), nullptr},
{"Slim", kHomepagePromoCardSlim, base::size(kHomepagePromoCardSlim),
nullptr}};
nullptr},
{"Compact_SuppressingSignInPromo", kHomepagePromoCardSuppressing,
base::size(kHomepagePromoCardSuppressing), nullptr}};
#endif // defined(OS_ANDROID)
#if defined(OS_ANDROID)
......@@ -5417,7 +5422,7 @@ const FeatureEntry kFeatureEntries[] = {
flag_descriptions::kHomepagePromoCardDescription, kOsAndroid,
FEATURE_WITH_PARAMS_VALUE_TYPE(chrome::android::kHomepagePromoCard,
kHomepagePromoCardVariations,
"HomepagePromoCard")},
"HomepagePromoAndroid")},
#endif // defined(OS_ANDROID)
#if defined(OS_CHROMEOS)
......
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