Add AIM sign-in promo rate limits [chromium/src : main]
Anthi Orfanou (Gerrit)
Anthi Orfanou voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Ryan Sultanem
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")
if (type == SignInPromoType::kSearchAIMode) {See also the previously raised topic in this CL: https://chromium-review.git.corp.google.com/c/chromium/src/+/7616929/comment/d3c5de16_98107f0a/
The new AIM promo should have it's own rate limits, independent of the other autofill promos (it is an entirely different flow).
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")
TEST_F(ShowSigninPromoTestWithFeatureFlags,Maybe worth repeating/parametrizing the new test for the case of a know Gaia id?
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 9 comments![]( "Open in Gerrit")
Thanks Anthi!! Added a couple first commments :)
Do you think we need to check `ShouldShowSearchAIModeSignInPromo()` again here, or is the one in line 270 sufficient?
TEST_F(ShowSigninPromoTestWithFeatureFlags,Maybe worth repeating/parametrizing the new test for the case of a know Gaia id?
I think it's fine since the gaia id functionality is tested in different places in this file.
return (gap > base::Days(14));This should be a constant like the ones in line 73
// - has already been dismissed `kSigninPromoDismissedThreshold` times,please 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.
if (!base::FeatureList::IsEnabled(switches::kSigninPromoLimitsExperiment)) {WDYT about creating a function in the signin_promo_util that checks what I think would be:
```
bool ShouldUseAutofillPromoLimits() {
return promo_type != signin::SignInPromoType::kSearchAIMode &&
!base::FeatureList::IsEnabled(switches::kSigninPromoLimitsExperiment);
}
```
I think there are some places now where we could use it, so that you can add the code for AI mode to the switch statement.
if (promo_view_ || !browser_view || !web_contents_) {
return TriggerStatus::kPromoNotTriggered;
}Are 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)).
void IncrementSearchAIModeSigninPromoImpressionCount(const GaiaId& gaia_id);
int GetSearchAIModeSigninPromoImpressionCount(const GaiaId& gaia_id) const;nit: the other types are ordered together by dismiss/impression, let's do the same here
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 9 comments![]( "Open in Gerrit")
Do you think we need to check `ShouldShowSearchAIModeSignInPromo()` again here, or is the one in line 270 sufficient?
The check in line 270 is enough, this method will always run after that. Also the call in line 270 is a "nice to have" but not mandatory call for our functionality: the main advantage here is that it allows us to destruct the present web content observer.
But even without it, we do another call of `ShouldShowSearchAIModeSignInPromo` in `SearchAIModeSignInPromoController::MaybeShowPromo` so that the bubble will only appears if the rate limits allow it.
This should be a constant like the ones in line 73
I turned this into a feature param in case we want to experiment with it. I will also make manual testing easier (the testers can provide a custom value).
See also the previously raised topic in this CL: https://chromium-review.git.corp.google.com/c/chromium/src/+/7616929/comment/d3c5de16_98107f0a/
The new AIM promo should have it's own rate limits, independent of the other autofill promos (it is an entirely different flow).
Acknowledged
// - has already been dismissed `kSigninPromoDismissedThreshold` times,please 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.
I 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?
if (!base::FeatureList::IsEnabled(switches::kSigninPromoLimitsExperiment)) {WDYT about creating a function in the signin_promo_util that checks what I think would be:
```
bool ShouldUseAutofillPromoLimits() {
return promo_type != signin::SignInPromoType::kSearchAIMode &&
!base::FeatureList::IsEnabled(switches::kSigninPromoLimitsExperiment);
}
```I think there are some places now where we could use it, so that you can add the code for AI mode to the switch statement.
Done
// Attempts to triggers the promo subject to its rate limits and otherAnthi Orfanoutypo
Done
if (promo_view_ || !browser_view || !web_contents_) {
return TriggerStatus::kPromoNotTriggered;
}Are 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)).
You 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.
void IncrementSearchAIModeSigninPromoImpressionCount(const GaiaId& gaia_id);
int GetSearchAIModeSigninPromoImpressionCount(const GaiaId& gaia_id) const;nit: the other types are ordered together by dismiss/impression, let's do the same here
Done, If understood correctly you comment you want to move the impression count methods together with the impressions-related methods above.
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)
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.
I 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)).
You 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. |