Add AIM sign-in promo rate limits [chromium/src : main]
Anthi Orfanou (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
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. |
Amelie Schneider (Gerrit)
Amelie Schneider added 13 comments![]( "Open in Gerrit")
EXPECT_FALSE(SearchAiModePromoTabHelper::FromWebContents(new_contents));Can we reuse the `helper` here?
#include "base/test/metrics/histogram_tester.h"please remove
I think tests checking that AI mode is unaffacted by the signinpromolimits experiment flag could also be useful
// For the Search AI Mode there should be a 14 day gap between impressions.Please update this comment to mention the variable rather than the time so that it does not become out of date when the frequency is changed.
if (type == SignInPromoType::kSearchAIMode) {
return profile.GetPrefs()->GetInteger(
prefs::kSearchAIModeSignInPromoDismissCountPerProfile);
}
if (!base::FeatureList::IsEnabled(switches::kSigninPromoLimitsExperiment)) {
return profile.GetPrefs()->GetInteger(
prefs::kAutofillSignInPromoDismissCountPerProfile);
}Same as the comment below, let's use `ShouldUseAutofillPromoLimits()` and put the ai mode into the switch
int GetContextualPromoDismissCountPerAccount(Profile& profile,
SignInPromoType type,
const GaiaId& gaia_id) {
if (type == SignInPromoType::kSearchAIMode) {
return SigninPrefs(*profile.GetPrefs())
.GetSearchAIModeSigninPromoDismissCount(gaia_id);
}
if (!base::FeatureList::IsEnabled(switches::kSigninPromoLimitsExperiment)) {
return SigninPrefs(*profile.GetPrefs())
.GetAutofillSigninPromoDismissCount(gaia_id);
}
switch (type) {
case SignInPromoType::kAddress:
return SigninPrefs(*profile.GetPrefs())
.GetAddressSigninPromoDismissCount(gaia_id);
case SignInPromoType::kPassword:
return SigninPrefs(*profile.GetPrefs())
.GetPasswordSigninPromoDismissCount(gaia_id);
case SignInPromoType::kSearchAIMode:
// Search AI Mode promo's rate limits are independent from the
// `kSigninPromoLimitsExperiment` flag.
NOTREACHED();
case SignInPromoType::kBookmark:
return SigninPrefs(*profile.GetPrefs())
.GetBookmarkSigninPromoDismissCount(gaia_id);
case SignInPromoType::kExtension:
NOTREACHED();
}
}Can we do instead:
```
int GetContextualPromoDismissCountPerAccount(Profile& profile,
SignInPromoType type,
const GaiaId& gaia_id) {
if (ShouldUseAutofillPromoLimits()) {
return SigninPrefs(*profile.GetPrefs())
.GetAutofillSigninPromoDismissCount(gaia_id);
}
switch (type) {
case SignInPromoType::kAddress:
return SigninPrefs(*profile.GetPrefs())
.GetAddressSigninPromoDismissCount(gaia_id);
case SignInPromoType::kPassword:
return SigninPrefs(*profile.GetPrefs())
.GetPasswordSigninPromoDismissCount(gaia_id);
case SignInPromoType::kSearchAIMode:
return SigninPrefs(*profile.GetPrefs())
.GetSearchAIModeSigninPromoDismissCount(gaia_id);
case SignInPromoType::kBookmark:
return SigninPrefs(*profile.GetPrefs())
.GetBookmarkSigninPromoDismissCount(gaia_id);
case SignInPromoType::kExtension:
NOTREACHED();
}
}
``` // - has already been dismissed `kSigninPromoDismissedThreshold` times,Anthi Orfanouplease add a comment as such for the frequency
Also, if you add the function in the utils which i suggested, then we can inverse these two return blocks so that the function can be used.
Amelie SchneiderI updated the comment. However I cannot find the comment where you suggest moving the frequency to a utility/helper. Could you point me to what you suggest moving?
Hmm I was going to suggest to use `ShouldUseAutofillPromoLimits` but I don't think it actually works because the conditions are not the same. Let's leave it as is.
// Needs additional checks depending on the type of the promo (see
// `ShouldShowAddressSignInPromo` and `ShouldShowPasswordSignInPromo`).This is out of date, let's remove
bool ShouldShowSignInPromoCommon(Profile& profile, SignInPromoType type) {What about the checks for ShouldShowSyncPromo and sync_service above? Why are these not needed for AI mode?
edit: Upon checking I saw this CL might be out of date with https://chromium-review.git.corp.google.com/c/chromium/src/+/7695174 . Can you please update?
if (type == SignInPromoType::kSearchAIMode) {Why is it not always shown in pending mode? Is this a product requirement?
if (promo_view_ || !browser_view || !web_contents_) {
return TriggerStatus::kPromoNotTriggered;
}Anthi OrfanouAre these (and the avatar button below) valid scenarios?
I feel like we don't really need the `Maybe` here (and therefore also no trigger status). `ShouldShowSearchAIModeSignInPromo()` is called before and does not need to be repeated. And then we can just show the promo (and include some safeguards if something goes really wrong as e.g. !browser_view or CHECK(browser_view)).
Amelie SchneiderYou are right about the early exits, I think all these conditions should be CHECKs instead. I have changed them now (and of the browser_view the caller ensure it exists in any case).
However for the the need to call ShouldShowSearchAIModeSignInPromo I think the controller is the right entity ensure that this condition is met before showing the promo.
If not in the form of a conditional, we should at least have a check (and leave it to the caller to ensure this).Right now the caller (aim_tab_helper) indeed calls this this method, but the reason is that in that case it can self destruct.
I see your point. I think it's true it makes sense that the controller would decide about the promo showing. I was a bit confused why there are two instances (helper, controller) that both hold conditions for the promo. Why is the check in `DidFinishNavigation()` needed?
If we keep the retur in this function, then let's add the `Maybe` to the function name again!
Related details
- Anthi Orfanou
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. |
Amelie Schneider (Gerrit)
Amelie Schneider added 1 comment![]( "Open in Gerrit")
Oh sorry I just realized I was reviewing on an older patch! I will go over it again
Related details
- Anthi Orfanou
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. |
Amelie Schneider (Gerrit)
Amelie Schneider added 6 comments![]( "Open in Gerrit")
"//chrome/browser/signin:signin_promo",why is this needed?
if (type == SignInPromoType::kSearchAIMode) {
return profile.GetPrefs()->GetInteger(
prefs::kSearchAIModeSignInPromoDismissCountPerProfile);
}
if (!base::FeatureList::IsEnabled(switches::kSigninPromoLimitsExperiment)) {
return profile.GetPrefs()->GetInteger(
prefs::kAutofillSignInPromoDismissCountPerProfile);
}Same as the comment below, let's use `ShouldUseAutofillPromoLimits()` and put the ai mode into the switch
Done
Can we do instead:
```
int GetContextualPromoDismissCountPerAccount(Profile& profile,
SignInPromoType type,
const GaiaId& gaia_id) {
if (ShouldUseAutofillPromoLimits()) {
return SigninPrefs(*profile.GetPrefs())
.GetAutofillSigninPromoDismissCount(gaia_id);
}switch (type) {
case SignInPromoType::kAddress:
return SigninPrefs(*profile.GetPrefs())
.GetAddressSigninPromoDismissCount(gaia_id);
case SignInPromoType::kPassword:
return SigninPrefs(*profile.GetPrefs())
.GetPasswordSigninPromoDismissCount(gaia_id);
case SignInPromoType::kSearchAIMode:
return SigninPrefs(*profile.GetPrefs())
.GetSearchAIModeSigninPromoDismissCount(gaia_id);
case SignInPromoType::kBookmark:
return SigninPrefs(*profile.GetPrefs())
.GetBookmarkSigninPromoDismissCount(gaia_id);
case SignInPromoType::kExtension:
NOTREACHED();
}
}
```
Done
// Don't show the promo again if i:Amelie Schneidertypo
Done
bool ShouldShowSignInPromoCommon(Profile& profile, SignInPromoType type) {What about the checks for ShouldShowSyncPromo and sync_service above? Why are these not needed for AI mode?
edit: Upon checking I saw this CL might be out of date with https://chromium-review.git.corp.google.com/c/chromium/src/+/7695174 . Can you please update?
Maybe just the check for sync service, not sure if it's needed?
if (type == SignInPromoType::kSearchAIMode) {Why is it not always shown in pending mode? Is this a product requirement?
Done
Anthi Orfanou (Gerrit)
Anthi Orfanou added 9 comments![]( "Open in Gerrit")
EXPECT_FALSE(SearchAiModePromoTabHelper::FromWebContents(new_contents));Anthi OrfanouCan we reuse the `helper` here?
No, I think it will be a a dangling pointer at this point (it's a raw pointer), so we have to invoke the method again. This is similar to the test above.
#include "base/test/metrics/histogram_tester.h"Anthi Orfanouplease remove
Kept it, because histograms are now consumed in these tests (after rebasing).
"//chrome/browser/signin:signin_promo",Anthi Orfanouwhy is this needed?
It's because of the inclusion of the #include "chrome/browser/signin/signin_promo.h"
in signin_promo_util.h (o/w the build fails: https://ci.chromium.org/ui/p/chromium/builders/try/linux-libfuzzer-asan-rel/2646155/overview)
I think tests checking that AI mode is unaffacted by the signinpromolimits experiment flag could also be useful
Done
// For the Search AI Mode there should be a 14 day gap between impressions.Please update this comment to mention the variable rather than the time so that it does not become out of date when the frequency is changed.
Done
// - has already been dismissed `kSigninPromoDismissedThreshold` times,Anthi Orfanouplease add a comment as such for the frequency
Also, if you add the function in the utils which i suggested, then we can inverse these two return blocks so that the function can be used.
Amelie SchneiderI updated the comment. However I cannot find the comment where you suggest moving the frequency to a utility/helper. Could you point me to what you suggest moving?
Hmm I was going to suggest to use `ShouldUseAutofillPromoLimits` but I don't think it actually works because the conditions are not the same. Let's leave it as is.
Yes, it's a different condition here (I also got confused and got this wrong in one PS).
// Needs additional checks depending on the type of the promo (see
// `ShouldShowAddressSignInPromo` and `ShouldShowPasswordSignInPromo`).This is out of date, let's remove
Done
bool ShouldShowSignInPromoCommon(Profile& profile, SignInPromoType type) {Amelie SchneiderWhat about the checks for ShouldShowSyncPromo and sync_service above? Why are these not needed for AI mode?
edit: Upon checking I saw this CL might be out of date with https://chromium-review.git.corp.google.com/c/chromium/src/+/7695174 . Can you please update?
Maybe just the check for sync service, not sure if it's needed?
I think this comment is outdated and has been addressed in the latest PS.
if (promo_view_ || !browser_view || !web_contents_) {
return TriggerStatus::kPromoNotTriggered;
}Anthi OrfanouAre these (and the avatar button below) valid scenarios?
I feel like we don't really need the `Maybe` here (and therefore also no trigger status). `ShouldShowSearchAIModeSignInPromo()` is called before and does not need to be repeated. And then we can just show the promo (and include some safeguards if something goes really wrong as e.g. !browser_view or CHECK(browser_view)).
Amelie SchneiderYou are right about the early exits, I think all these conditions should be CHECKs instead. I have changed them now (and of the browser_view the caller ensure it exists in any case).
However for the the need to call ShouldShowSearchAIModeSignInPromo I think the controller is the right entity ensure that this condition is met before showing the promo.
If not in the form of a conditional, we should at least have a check (and leave it to the caller to ensure this).Right now the caller (aim_tab_helper) indeed calls this this method, but the reason is that in that case it can self destruct.
I see your point. I think it's true it makes sense that the controller would decide about the promo showing. I was a bit confused why there are two instances (helper, controller) that both hold conditions for the promo. Why is the check in `DidFinishNavigation()` needed?
If we keep the retur in this function, then let's add the `Maybe` to the function name again!
Restored the name to `Maybe*` and added some comments in the call on the tab_helper (relevant to this comment: https://chromium-review.git.corp.google.com/c/chromium/src/+/7736186/comment/41eb60e6_82e1d9e8/).
I think I might be also to avoid the helper call in the future if we land one upcoming CL (https://chromium-review.git.corp.google.com/c/chromium/src/+/7736189/5): the plan is to pass a callback to destruct the calle to the MaybeShowPromo method. It will be then used when 1) the promo cannot be shown or when 2) the promo is shown but the user closes it.
Related details
- Amelie Schneider
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. |
Anthi Orfanou (Gerrit)
Anthi Orfanou added 1 comment![]( "Open in Gerrit")
+David for the comp/s/p/b/prefs and switches owner's review.
Related details
- Amelie Schneider
- David Roger
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. |
Amelie Schneider (Gerrit)
Amelie Schneider voted and added 5 comments![]( "Open in Gerrit")
Votes added by Amelie Schneider
| Code-Review | +1 |
5 comments
Thanks Anthi!
ASSERT_TRUE(ShouldShowSearchAIModeSignInPromo(*profile()));assert_false that another promo is shown? so that we see the difference between the two
prefs::kSearchAIModeSignInPromoShownCountPerProfile, 0);Maybe assert_true here again to make sure it is due to the dismiss count that the promo is not shown
bool ShouldShowSignInPromoCommon(Profile& profile, SignInPromoType type) {Amelie SchneiderWhat about the checks for ShouldShowSyncPromo and sync_service above? Why are these not needed for AI mode?
edit: Upon checking I saw this CL might be out of date with https://chromium-review.git.corp.google.com/c/chromium/src/+/7695174 . Can you please update?
Anthi OrfanouMaybe just the check for sync service, not sure if it's needed?
I think this comment is outdated and has been addressed in the latest PS.
Acknowledged
NOTREACHED();optional nit: I think `return false;` is fine here, as we do for other promo types
Related details
- Anthi Orfanou
- David Roger
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. |
David Roger (Gerrit)
David Roger voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Anthi Orfanou
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. |
Anthi Orfanou (Gerrit)
Anthi Orfanou added 3 comments![]( "Open in Gerrit")
ASSERT_TRUE(ShouldShowSearchAIModeSignInPromo(*profile()));assert_false that another promo is shown? so that we see the difference between the two
Did you mean assert true that another promo is shown after we set the limit preferences? If yes, I have added that.
If you meant something else, please let me know. At the start of the test all promos should show fine (because we have not written yet an impression/dismissals).
prefs::kSearchAIModeSignInPromoShownCountPerProfile, 0);Maybe assert_true here again to make sure it is due to the dismiss count that the promo is not shown
Done
NOTREACHED();Anthi Orfanouoptional nit: I think `return false;` is fine here, as we do for other promo types
Done
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Amelie Schneider (Gerrit)
Amelie Schneider voted and added 1 comment![]( "Open in Gerrit")
Votes added by Amelie Schneider
| Code-Review | +1 |
1 comment
ASSERT_TRUE(ShouldShowSearchAIModeSignInPromo(*profile()));Anthi Orfanouassert_false that another promo is shown? so that we see the difference between the two
Did you mean assert true that another promo is shown after we set the limit preferences? If yes, I have added that.
If you meant something else, please let me know. At the start of the test all promos should show fine (because we have not written yet an impression/dismissals).
I meant that through the title I would assume that this tests that the limit that applies to other promos does not affect the search AI mode promo. Now it seems like we are more testing the AI promo limits and that it doesn't affect other promos
Related details
- Anthi Orfanou
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. |
Anthi Orfanou (Gerrit)
Anthi Orfanou added 1 comment![]( "Open in Gerrit")
ASSERT_TRUE(ShouldShowSearchAIModeSignInPromo(*profile()));Anthi Orfanouassert_false that another promo is shown? so that we see the difference between the two
Amelie SchneiderDid you mean assert true that another promo is shown after we set the limit preferences? If yes, I have added that.
If you meant something else, please let me know. At the start of the test all promos should show fine (because we have not written yet an impression/dismissals).
I meant that through the title I would assume that this tests that the limit that applies to other promos does not affect the search AI mode promo. Now it seems like we are more testing the AI promo limits and that it doesn't affect other promos
This is a good argument for splitting into two tests.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Amelie Schneider (Gerrit)
Amelie Schneider voted and added 2 comments![]( "Open in Gerrit")
Votes added by Amelie Schneider
| Code-Review | +1 |
2 comments
SearchAIModePromoIgnoresOtherExperimentThresholds) {I think this is not testing the other prefs, and more repeating above tests?
I meant something like
```
ASSERT_TRUE(ShouldShowSearchAIModeSignInPromo(*profile()));
ASSERT_TRUE(ShouldShowBookmarkSignInPromo(*profile()));
profile()->GetPrefs()->SetInteger(
prefs::kAutofillSignInPromoDismissCountPerProfile, 5);
ASSERT_FALSE(ShouldShowBookmarkSignInPromo(*profile()));
EXPECT_TRUE(ShouldShowSearchAIModeSignInPromo(*profile()));
```
SearchAIModePromoLimitsDoNotAffectOtherPromos) {How about:
```
ASSERT_TRUE(ShouldShowSearchAIModeSignInPromo(*profile()));
ASSERT_TRUE(ShouldShowBookmarkSignInPromo(*profile()));
profile()->GetPrefs()->SetInteger(
prefs::kSearchAIModeSignInPromoShownCountPerProfile, 5);
ASSERT_FALSE(ShouldShowSearchAIModeSignInPromo(*profile()));
EXPECT_TRUE(ShouldShowBookmarkSignInPromo(*profile()));
```
Related details
- Anthi Orfanou
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. |
Anthi Orfanou (Gerrit)
Anthi Orfanou added 2 comments![]( "Open in Gerrit")
SearchAIModePromoIgnoresOtherExperimentThresholds) {I think this is not testing the other prefs, and more repeating above tests?
I meant something like```
ASSERT_TRUE(ShouldShowSearchAIModeSignInPromo(*profile()));
ASSERT_TRUE(ShouldShowBookmarkSignInPromo(*profile()));profile()->GetPrefs()->SetInteger(
prefs::kAutofillSignInPromoDismissCountPerProfile, 5);
ASSERT_FALSE(ShouldShowBookmarkSignInPromo(*profile()));
EXPECT_TRUE(ShouldShowSearchAIModeSignInPromo(*profile()));
```
Done, sorry for the multiple rounds of feedback on this test!
How about:
```
ASSERT_TRUE(ShouldShowSearchAIModeSignInPromo(*profile()));
ASSERT_TRUE(ShouldShowBookmarkSignInPromo(*profile()));profile()->GetPrefs()->SetInteger(
prefs::kSearchAIModeSignInPromoShownCountPerProfile, 5);ASSERT_FALSE(ShouldShowSearchAIModeSignInPromo(*profile()));
EXPECT_TRUE(ShouldShowBookmarkSignInPromo(*profile()));
```
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anthi Orfanou (Gerrit)
Anthi Orfanou voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromium-scoped@luci-project-accounts.iam.gserviceaccount.com (Gerrit)
chromiu...@luci-project-accounts.iam.gserviceaccount.com submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
22 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/browser/signin/signin_promo_unittest.cc
Insertions: 14, Deletions: 30.
@@ -1054,59 +1054,43 @@
EXPECT_FALSE(ShouldShowBookmarkSignInPromo(*profile()));
}
-
TEST_F(ShowSigninPromoTestWithFeatureFlagsPromoLimitsExperiment,
SearchAIModePromoIgnoresOtherExperimentThresholds) {
- base::test::ScopedFeatureList features;
- // Enable the experiment with thresholds lower than the default AI limits.
- features.InitAndEnableFeatureWithParameters(
- switches::kSigninPromoLimitsExperiment,
- {{"contextual_signin_promo_shown_threshold", "2"},
- {"contextual_signin_promo_dismissed_threshold", "1"}});
-
- // Default AI limits are 5 impressions and 2 dismissals.
ASSERT_TRUE(ShouldShowSearchAIModeSignInPromo(*profile()));
+ ASSERT_TRUE(ShouldShowBookmarkSignInPromo(*profile()));
- // Set the AI mode promo to have been shown 3 times.
- // This is above the experiment threshold (2) but below the AI threshold (5).
+ // Set the bookmark promo shown limit to max (6 times).
profile()->GetPrefs()->SetInteger(
- prefs::kSearchAIModeSignInPromoShownCountPerProfile, 3);
+ prefs::kBookmarkSignInPromoShownCountPerProfileForLimitsExperiment, 6);
+ EXPECT_FALSE(ShouldShowBookmarkSignInPromo(*profile()));
EXPECT_TRUE(ShouldShowSearchAIModeSignInPromo(*profile()));
- // Set the AI mode promo to have been dismissed 1 time.
- // The promo is shown as we are below the AI threshold (2).
+ // Set the bookmark promo impression and dismissal limit to max (2 times).
profile()->GetPrefs()->SetInteger(
- prefs::kSearchAIModeSignInPromoDismissCountPerProfile, 1);
+ prefs::kBookmarkSignInPromoShownCountPerProfileForLimitsExperiment, 2);
+ profile()->GetPrefs()->SetInteger(
+ prefs::kBookmarkSignInPromoDismissCountPerProfileForLimitsExperiment, 2);
+ EXPECT_FALSE(ShouldShowBookmarkSignInPromo(*profile()));
EXPECT_TRUE(ShouldShowSearchAIModeSignInPromo(*profile()));
-
- // Reaching the AI specific limit hides it.
- profile()->GetPrefs()->SetInteger(
- prefs::kSearchAIModeSignInPromoDismissCountPerProfile, 2);
- EXPECT_FALSE(ShouldShowSearchAIModeSignInPromo(*profile()));
}
TEST_F(ShowSigninPromoTestWithFeatureFlagsPromoLimitsExperiment,
SearchAIModePromoLimitsDoNotAffectOtherPromos) {
- base::test::ScopedFeatureList features;
- features.InitAndEnableFeatureWithParameters(
- switches::kSigninPromoLimitsExperiment,
- {{"contextual_signin_promo_shown_threshold", "10"},
- {"contextual_signin_promo_dismissed_threshold", "5"}});
-
ASSERT_TRUE(ShouldShowSearchAIModeSignInPromo(*profile()));
+ ASSERT_TRUE(ShouldShowBookmarkSignInPromo(*profile()));
- // Verify that reaching the AI limit (5 impressions) doesn't affect other promos.
+ // Set the Search AIM max impression limit (5 times)
profile()->GetPrefs()->SetInteger(
prefs::kSearchAIModeSignInPromoShownCountPerProfile, 5);
- ASSERT_FALSE(ShouldShowSearchAIModeSignInPromo(*profile()));
+ EXPECT_FALSE(ShouldShowSearchAIModeSignInPromo(*profile()));
EXPECT_TRUE(ShouldShowBookmarkSignInPromo(*profile()));
- // Verify that reaching the AI limit (2 dismissals) doesn't affect other promos.
+ // Set the Search AIM max dismissal limit (2 times)
profile()->GetPrefs()->SetInteger(
prefs::kSearchAIModeSignInPromoShownCountPerProfile, 2);
profile()->GetPrefs()->SetInteger(
prefs::kSearchAIModeSignInPromoDismissCountPerProfile, 2);
- ASSERT_FALSE(ShouldShowSearchAIModeSignInPromo(*profile()));
+ EXPECT_FALSE(ShouldShowSearchAIModeSignInPromo(*profile()));
EXPECT_TRUE(ShouldShowBookmarkSignInPromo(*profile()));
}
```
Change information
Add AIM sign-in promo rate limits
Rate limits per the PRD:
- Global contextual promo cap: If a user explicitly dismisses 2 contextual sign-in promos globally, all such promos are permanently dismissed.
- Explicit dismissal (user taps 'x')
The promo can be shown up to 1 more time with a 14-day gap between impressions - see global cap above.
- Implicit dismissal:
The promo is shown up to 4 more times (total 5 impressions) with a 14-day gap between impressions.
- M chrome/browser/contextual_tasks/search_ai_mode_promo_tab_helper.cc
- M chrome/browser/contextual_tasks/search_ai_mode_promo_tab_helper_browsertest.cc
- M chrome/browser/signin/BUILD.gn
- M chrome/browser/signin/chrome_signin_pref_names.h
- M chrome/browser/signin/signin_promo.cc
- M chrome/browser/signin/signin_promo_unittest.cc
- M chrome/browser/signin/signin_promo_util.cc
- M chrome/browser/signin/signin_promo_util.h
- M chrome/browser/ui/signin/promos/bubble_signin_promo_view.cc
- M chrome/browser/ui/views/search_ai_mode/signin_promo_controller.cc
- M chrome/browser/ui/views/search_ai_mode/signin_promo_controller.h
- M components/signin/public/base/signin_prefs.cc
- M components/signin/public/base/signin_prefs.h
- M components/signin/public/base/signin_switches.cc
- M components/signin/public/base/signin_switches.h
Code-Review: +1 by Amelie Schneider, +1 by David Roger
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |