Skip to content

GitLab

T
tangled
Commit 5d52425f authored by Kyle Milka's avatar Kyle Milka Committed by Commit Bot
Browse files
Options

Reland "[NTP] Cap search suggestion impressions" 

This is a reland of 41e54be1

One of the checks is converting the times from ms->s. In practice
seconds is plenty precise but in the test 800ms ended up being 0,
so increase it to 1000ms (similar to ImpressionCountResetsAfterTimeout
which was not flaky).

Original change's description:
> [NTP] Cap search suggestion impressions
>
> The parameters for capping suggestions impressions are provided as part
> of the update proto. Read and update the local pref on each fetch. Use
> these prefs to determine if the impression cap has been reach or if
> fetching is frozen due to an empty response.
>
> Bug: 904565
> Change-Id: Ib5539a99f18e9da2ac1223ecc7aff65cb909bca8
> Reviewed-on: https://chromium-review.googlesource.com/c/1395188
> Commit-Queue: Kyle Milka <kmilka@chromium.org>
> Reviewed-by: Kristi Park <kristipark@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#624994}

Bug: 904565
Change-Id: I424cb51d2a9c1738c0e5168f58ef52b73ed4b223
Reviewed-on: https://chromium-review.googlesource.com/c/1431472Reviewed-by: default avatarKristi Park <kristipark@chromium.org>
Commit-Queue: Kyle Milka <kmilka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#625437}
parent 008a7ae4
Expand all Hide whitespace changes
Inline Side-by-side
Showing with 406 additions and 30 deletions
chrome/browser/search/local_ntp_source.cc
View file @ 5d52425f
...@@ -1281,7 +1281,7 @@ void LocalNtpSource::MaybeServeSearchSuggestions( ...@@ -1281,7 +1281,7 @@ void LocalNtpSource::MaybeServeSearchSuggestions(
} }
SearchSuggestData suggest_data = *data; SearchSuggestData suggest_data = *data;
search_suggest_service_->ClearSearchSuggestData(); search_suggest_service_->SuggestionsDisplayed();
scoped_refptr<base::RefCountedString> result; scoped_refptr<base::RefCountedString> result;
std::string js; std::string js;
base::JSONWriter::Write(*ConvertSearchSuggestDataToDict(suggest_data), &js); base::JSONWriter::Write(*ConvertSearchSuggestDataToDict(suggest_data), &js);
......
chrome/browser/search/search_suggest/search_suggest_data.cc
View file @ 5d52425f
...@@ -15,7 +15,11 @@ SearchSuggestData& SearchSuggestData::operator=(SearchSuggestData&&) = default; ...@@ -15,7 +15,11 @@ SearchSuggestData& SearchSuggestData::operator=(SearchSuggestData&&) = default;
bool operator==(const SearchSuggestData& lhs, const SearchSuggestData& rhs) { bool operator==(const SearchSuggestData& lhs, const SearchSuggestData& rhs) {
return lhs.suggestions_html == rhs.suggestions_html && return lhs.suggestions_html == rhs.suggestions_html &&
lhs.end_of_body_script == rhs.end_of_body_script; lhs.end_of_body_script == rhs.end_of_body_script &&
lhs.impression_cap_expire_time_ms ==
rhs.impression_cap_expire_time_ms &&
lhs.request_freeze_time_ms == rhs.request_freeze_time_ms &&
lhs.max_impressions == rhs.max_impressions;
} }
bool operator!=(const SearchSuggestData& lhs, const SearchSuggestData& rhs) { bool operator!=(const SearchSuggestData& lhs, const SearchSuggestData& rhs) {
......
chrome/browser/search/search_suggest/search_suggest_data.h
View file @ 5d52425f
...@@ -24,6 +24,11 @@ struct SearchSuggestData { ...@@ -24,6 +24,11 @@ struct SearchSuggestData {
// Javascript for search suggestion that should be appended at the end of the // Javascript for search suggestion that should be appended at the end of the
// New Tab Page <body>. // New Tab Page <body>.
std::string end_of_body_script; std::string end_of_body_script;
// Parameters that control impression capping and freezing.
int impression_cap_expire_time_ms;
int request_freeze_time_ms;
int max_impressions;
}; };
bool operator==(const SearchSuggestData& lhs, const SearchSuggestData& rhs); bool operator==(const SearchSuggestData& lhs, const SearchSuggestData& rhs);
......
chrome/browser/search/search_suggest/search_suggest_loader.h
View file @ 5d52425f
...@@ -26,8 +26,12 @@ class SearchSuggestLoader { ...@@ -26,8 +26,12 @@ class SearchSuggestLoader {
// A fatal error occurred, such as the server responding with an error code // A fatal error occurred, such as the server responding with an error code
// or with invalid data. Any previously cached response should be cleared. // or with invalid data. Any previously cached response should be cleared.
FATAL_ERROR, FATAL_ERROR,
// The user has opted out of seeing search suggestions on the NTP. // The user has opted out of seeing search suggestions on the NTP
OPTED_OUT OPTED_OUT,
// The limit for number of impressions was hit.
IMPRESSION_CAP,
// Received an empty response so requests are temporarily frozen.
REQUESTS_FROZEN
}; };
using SearchSuggestionsCallback = using SearchSuggestionsCallback =
base::OnceCallback<void(Status, base::OnceCallback<void(Status,
......
chrome/browser/search/search_suggest/search_suggest_loader_impl.cc
View file @ 5d52425f
...@@ -75,6 +75,32 @@ base::Optional<SearchSuggestData> JsonToSearchSuggestionData( ...@@ -75,6 +75,32 @@ base::Optional<SearchSuggestData> JsonToSearchSuggestionData(
result.end_of_body_script = end_of_body_script; result.end_of_body_script = end_of_body_script;
int impression_cap_expire_time_ms;
if (!query_suggestions->GetInteger("impression_cap_expire_time_ms",
&impression_cap_expire_time_ms)) {
DLOG(WARNING) << "Parse error: no impression_cap_expire_time_ms";
return base::nullopt;
}
result.impression_cap_expire_time_ms = impression_cap_expire_time_ms;
int request_freeze_time_ms;
if (!query_suggestions->GetInteger("request_freeze_time_ms",
&request_freeze_time_ms)) {
DLOG(WARNING) << "Parse error: no request_freeze_time_ms";
return base::nullopt;
}
result.request_freeze_time_ms = request_freeze_time_ms;
int max_impressions;
if (!query_suggestions->GetInteger("max_impressions", &max_impressions)) {
DLOG(WARNING) << "Parse error: no max_impressions";
return base::nullopt;
}
result.max_impressions = max_impressions;
return result; return result;
} }
......
chrome/browser/search/search_suggest/search_suggest_loader_impl_unittest.cc
View file @ 5d52425f
...@@ -39,7 +39,9 @@ namespace { ...@@ -39,7 +39,9 @@ namespace {
const char kApplicationLocale[] = "us"; const char kApplicationLocale[] = "us";
const char kMinimalValidResponse[] = R"json({"update": { "query_suggestions": { const char kMinimalValidResponse[] = R"json({"update": { "query_suggestions": {
"query_suggestions_with_html": "", "script": "" "query_suggestions_with_html": "", "script": "",
"impression_cap_expire_time_ms": 0, "request_freeze_time_ms": 0,
"max_impressions": 0
}}})json"; }}})json";
// Required to instantiate a GoogleUrlTracker in UNIT_TEST_MODE. // Required to instantiate a GoogleUrlTracker in UNIT_TEST_MODE.
...@@ -170,9 +172,11 @@ TEST_F(SearchSuggestLoaderImplTest, HandlesResponsePreamble) { ...@@ -170,9 +172,11 @@ TEST_F(SearchSuggestLoaderImplTest, HandlesResponsePreamble) {
} }
TEST_F(SearchSuggestLoaderImplTest, ParsesFullResponse) { TEST_F(SearchSuggestLoaderImplTest, ParsesFullResponse) {
SetUpResponseWithData( SetUpResponseWithData(R"json({"update": { "query_suggestions": {
R"json({"update": { "query_suggestions": {"query_suggestions_with_html" : "query_suggestions_with_html": "<div></div>",
"<div></div>", "script" : "<script></script>"}}})json"); "script": "<script></script>", "impression_cap_expire_time_ms": 1,
"request_freeze_time_ms": 2, "max_impressions": 3
}}})json");
base::MockCallback<SearchSuggestLoader::SearchSuggestionsCallback> callback; base::MockCallback<SearchSuggestLoader::SearchSuggestionsCallback> callback;
std::string blacklist; std::string blacklist;
...@@ -185,8 +189,11 @@ TEST_F(SearchSuggestLoaderImplTest, ParsesFullResponse) { ...@@ -185,8 +189,11 @@ TEST_F(SearchSuggestLoaderImplTest, ParsesFullResponse) {
loop.Run(); loop.Run();
ASSERT_TRUE(data.has_value()); ASSERT_TRUE(data.has_value());
EXPECT_THAT(data->suggestions_html, Eq("<div></div>")); EXPECT_EQ("<div></div>", data->suggestions_html);
EXPECT_THAT(data->end_of_body_script, Eq("<script></script>")); EXPECT_EQ("<script></script>", data->end_of_body_script);
EXPECT_EQ(1, data->impression_cap_expire_time_ms);
EXPECT_EQ(2, data->request_freeze_time_ms);
EXPECT_EQ(3, data->max_impressions);
} }
TEST_F(SearchSuggestLoaderImplTest, CoalescesMultipleRequests) { TEST_F(SearchSuggestLoaderImplTest, CoalescesMultipleRequests) {
......
chrome/browser/search/search_suggest/search_suggest_service.cc
View file @ 5d52425f
...@@ -13,6 +13,34 @@ ...@@ -13,6 +13,34 @@
#include "components/prefs/scoped_user_pref_update.h" #include "components/prefs/scoped_user_pref_update.h"
#include "services/identity/public/cpp/identity_manager.h" #include "services/identity/public/cpp/identity_manager.h"
namespace {
const char kFirstShownTimeMs[] = "first_shown_time_ms";
const char kImpressionCapExpireTimeMs[] = "impression_cap_expire_time_ms";
const char kImpressionsCount[] = "impressions_count";
const char kIsRequestFrozen[] = "is_request_frozen";
const char kMaxImpressions[] = "max_impressions";
const char kRequestFreezeTimeMs[] = "request_freeze_time_ms";
const char kRequestFrozenTimeMs[] = "request_frozen_time_ms";
// Default value for max_impressions specified by the VASCO team.
const int kDefaultMaxImpressions = 4;
std::unique_ptr<base::DictionaryValue> ImpressionDictDefaults() {
std::unique_ptr<base::DictionaryValue> defaults =
std::make_unique<base::DictionaryValue>();
defaults->SetInteger(kFirstShownTimeMs, 0);
defaults->SetInteger(kImpressionCapExpireTimeMs, 0);
defaults->SetInteger(kImpressionsCount, 0);
defaults->SetBoolean(kIsRequestFrozen, false);
defaults->SetInteger(kMaxImpressions, kDefaultMaxImpressions);
defaults->SetInteger(kRequestFreezeTimeMs, 0);
defaults->SetInteger(kRequestFrozenTimeMs, 0);
return defaults;
}
} // namespace
class SearchSuggestService::SigninObserver class SearchSuggestService::SigninObserver
: public identity::IdentityManager::Observer { : public identity::IdentityManager::Observer {
public: public:
...@@ -72,6 +100,12 @@ void SearchSuggestService::Refresh() { ...@@ -72,6 +100,12 @@ void SearchSuggestService::Refresh() {
} else if (pref_service_->GetBoolean(prefs::kNtpSearchSuggestionsOptOut)) { } else if (pref_service_->GetBoolean(prefs::kNtpSearchSuggestionsOptOut)) {
SearchSuggestDataLoaded(SearchSuggestLoader::Status::OPTED_OUT, SearchSuggestDataLoaded(SearchSuggestLoader::Status::OPTED_OUT,
base::nullopt); base::nullopt);
} else if (RequestsFrozen()) {
SearchSuggestDataLoaded(SearchSuggestLoader::Status::REQUESTS_FROZEN,
base::nullopt);
} else if (ImpressionCapReached()) {
SearchSuggestDataLoaded(SearchSuggestLoader::Status::IMPRESSION_CAP,
base::nullopt);
} else { } else {
const std::string blacklist = GetBlacklistAsString(); const std::string blacklist = GetBlacklistAsString();
loader_->Load(blacklist, loader_->Load(blacklist,
...@@ -102,10 +136,22 @@ void SearchSuggestService::SearchSuggestDataLoaded( ...@@ -102,10 +136,22 @@ void SearchSuggestService::SearchSuggestDataLoaded(
// In case of transient errors, keep our cached data (if any), but still // In case of transient errors, keep our cached data (if any), but still
// notify observers of the finished load (attempt). // notify observers of the finished load (attempt).
if (status != SearchSuggestLoader::Status::TRANSIENT_ERROR) { if (status != SearchSuggestLoader::Status::TRANSIENT_ERROR) {
// TODO(crbug/904565): Verify that cached data is also cleared when the
// impression cap is reached. Including the response from the request made
// on the same load that the cap was hit.
search_suggest_data_ = data; search_suggest_data_ = data;
DictionaryPrefUpdate update(pref_service_,
prefs::kNtpSearchSuggestionsImpressions);
if (data.has_value()) {
base::DictionaryValue* dict = update.Get();
dict->SetInteger(kMaxImpressions, data->max_impressions);
dict->SetInteger(kImpressionCapExpireTimeMs,
data->impression_cap_expire_time_ms);
dict->SetInteger(kRequestFreezeTimeMs, data->request_freeze_time_ms);
} else if (status == SearchSuggestLoader::Status::FATAL_ERROR) {
base::DictionaryValue* dict = update.Get();
dict->SetBoolean(kIsRequestFrozen, true);
dict->SetInteger(kRequestFrozenTimeMs, base::Time::Now().ToTimeT());
}
} }
NotifyObservers(); NotifyObservers();
} }
...@@ -116,6 +162,61 @@ void SearchSuggestService::NotifyObservers() { ...@@ -116,6 +162,61 @@ void SearchSuggestService::NotifyObservers() {
} }
} }
bool SearchSuggestService::ImpressionCapReached() {
const base::DictionaryValue* dict =
pref_service_->GetDictionary(prefs::kNtpSearchSuggestionsImpressions);
int first_shown_time_ms = 0;
int impression_cap_expire_time_ms = 0;
int impression_count = 0;
int max_impressions = 0;
dict->GetInteger(kFirstShownTimeMs, &first_shown_time_ms);
dict->GetInteger(kImpressionCapExpireTimeMs, &impression_cap_expire_time_ms);
dict->GetInteger(kImpressionsCount, &impression_count);
dict->GetInteger(kMaxImpressions, &max_impressions);
int64_t time_delta =
base::TimeDelta(base::Time::Now() -
base::Time::FromTimeT(first_shown_time_ms))
.InMilliseconds();
if (time_delta > impression_cap_expire_time_ms) {
impression_count = 0;
DictionaryPrefUpdate update(pref_service_,
prefs::kNtpSearchSuggestionsImpressions);
update.Get()->SetInteger(kImpressionsCount, impression_count);
}
return impression_count >= max_impressions;
}
bool SearchSuggestService::RequestsFrozen() {
const base::DictionaryValue* dict =
pref_service_->GetDictionary(prefs::kNtpSearchSuggestionsImpressions);
bool is_request_frozen = false;
int request_freeze_time_ms = 0;
int request_frozen_time_ms = 0;
dict->GetBoolean(kIsRequestFrozen, &is_request_frozen);
dict->GetInteger(kRequestFrozenTimeMs, &request_frozen_time_ms);
dict->GetInteger(kRequestFreezeTimeMs, &request_freeze_time_ms);
int64_t time_delta =
base::TimeDelta(base::Time::Now() -
base::Time::FromTimeT(request_frozen_time_ms))
.InMilliseconds();
if (is_request_frozen) {
if (time_delta < request_freeze_time_ms) {
return true;
} else {
DictionaryPrefUpdate update(pref_service_,
prefs::kNtpSearchSuggestionsImpressions);
update.Get()->SetBoolean(kIsRequestFrozen, false);
}
}
return false;
}
void SearchSuggestService::BlacklistSearchSuggestion(int task_version, void SearchSuggestService::BlacklistSearchSuggestion(int task_version,
long task_id) { long task_id) {
std::string task_version_id = std::string task_version_id =
...@@ -178,6 +279,23 @@ std::string SearchSuggestService::GetBlacklistAsString() { ...@@ -178,6 +279,23 @@ std::string SearchSuggestService::GetBlacklistAsString() {
return blacklist_as_string; return blacklist_as_string;
} }
void SearchSuggestService::SuggestionsDisplayed() {
search_suggest_data_ = base::nullopt;
DictionaryPrefUpdate update(pref_service_,
prefs::kNtpSearchSuggestionsImpressions);
base::DictionaryValue* dict = update.Get();
int impression_count = 0;
dict->GetInteger(kImpressionsCount, &impression_count);
dict->SetInteger(kImpressionsCount, impression_count + 1);
// When suggestions are displayed for the first time record the timestamp.
if (impression_count == 0) {
dict->SetInteger(kFirstShownTimeMs, base::Time::Now().ToTimeT());
}
}
void SearchSuggestService::OptOutOfSearchSuggestions() { void SearchSuggestService::OptOutOfSearchSuggestions() {
pref_service_->SetBoolean(prefs::kNtpSearchSuggestionsOptOut, true); pref_service_->SetBoolean(prefs::kNtpSearchSuggestionsOptOut, true);
...@@ -187,5 +305,7 @@ void SearchSuggestService::OptOutOfSearchSuggestions() { ...@@ -187,5 +305,7 @@ void SearchSuggestService::OptOutOfSearchSuggestions() {
// static // static
void SearchSuggestService::RegisterProfilePrefs(PrefRegistrySimple* registry) { void SearchSuggestService::RegisterProfilePrefs(PrefRegistrySimple* registry) {
registry->RegisterDictionaryPref(prefs::kNtpSearchSuggestionsBlacklist); registry->RegisterDictionaryPref(prefs::kNtpSearchSuggestionsBlacklist);
registry->RegisterDictionaryPref(prefs::kNtpSearchSuggestionsImpressions,
ImpressionDictDefaults());
registry->RegisterBooleanPref(prefs::kNtpSearchSuggestionsOptOut, false); registry->RegisterBooleanPref(prefs::kNtpSearchSuggestionsOptOut, false);
} }
chrome/browser/search/search_suggest/search_suggest_service.h
View file @ 5d52425f
...@@ -38,11 +38,11 @@ class SearchSuggestService : public KeyedService { ...@@ -38,11 +38,11 @@ class SearchSuggestService : public KeyedService {
return search_suggest_data_; return search_suggest_data_;
} }
// Clears any currently cached search suggest data. // Determines if a request for search suggestions should be made. If a request
void ClearSearchSuggestData() { search_suggest_data_ = base::nullopt; } // should not be made immediately call SearchSuggestDataLoaded with the
// reason. Otherwise requests an asynchronous refresh from the network. After
// Requests an asynchronous refresh from the network. After the update // the update completes, regardless of success, OnSearchSuggestDataUpdated
// completes, OnSearchSuggestDataUpdated will be called on the observers. // will be called on the observers.
void Refresh(); void Refresh();
// Add/remove observers. All observers must unregister themselves before the // Add/remove observers. All observers must unregister themselves before the
...@@ -82,16 +82,37 @@ class SearchSuggestService : public KeyedService { ...@@ -82,16 +82,37 @@ class SearchSuggestService : public KeyedService {
// "task_id1:hash1,hash2,hash3;task_id2;task_id3:hash1,hash2". // "task_id1:hash1,hash2,hash3;task_id2;task_id3:hash1,hash2".
std::string GetBlacklistAsString(); std::string GetBlacklistAsString();
// Called when suggestions are displayed on the NTP, clears the cached data
// and updates timestamps and impression counts.
void SuggestionsDisplayed();
private: private:
class SigninObserver; class SigninObserver;
void SigninStatusChanged(); void SigninStatusChanged();
// Called when a Refresh() is requested. If |status|==OK, |data| will contain
// the fetched data. Otherwise |data| will be nullopt and |status| will
// indicate if the request failed or the reason it was not sent.
//
// If the |status|==FATAL_ERROR freeze future requests until the request
// freeze interval has elapsed.
void SearchSuggestDataLoaded(SearchSuggestLoader::Status status, void SearchSuggestDataLoaded(SearchSuggestLoader::Status status,
const base::Optional<SearchSuggestData>& data); const base::Optional<SearchSuggestData>& data);
void NotifyObservers(); void NotifyObservers();
// Returns true if the number of impressions has reached the maxmium allowed
// for the impression interval (e.g. 4 impressions / 12 hours), and false
// otherwise.
bool ImpressionCapReached();
// Returns true if requests are frozen and request freeze time interval has
// not elapsed, false otherwise.
//
// Requests are frozen on any request that results in a FATAL_ERROR.
bool RequestsFrozen();
std::unique_ptr<SearchSuggestLoader> loader_; std::unique_ptr<SearchSuggestLoader> loader_;
std::unique_ptr<SigninObserver> signin_observer_; std::unique_ptr<SigninObserver> signin_observer_;
......
chrome/common/pref_names.cc
View file @ 5d52425f
...@@ -1539,6 +1539,8 @@ const char kNtpCustomBackgroundLocalToDevice[] = ...@@ -1539,6 +1539,8 @@ const char kNtpCustomBackgroundLocalToDevice[] =
// Data associated with search suggestions that appear on the NTP. // Data associated with search suggestions that appear on the NTP.
const char kNtpSearchSuggestionsBlacklist[] = const char kNtpSearchSuggestionsBlacklist[] =
"ntp.search_suggestions_blacklist"; "ntp.search_suggestions_blacklist";
const char kNtpSearchSuggestionsImpressions[] =
"ntp.search_suggestions_impressions";
const char kNtpSearchSuggestionsOptOut[] = "ntp.search_suggestions_opt_out"; const char kNtpSearchSuggestionsOptOut[] = "ntp.search_suggestions_opt_out";
#endif // defined(OS_ANDROID) #endif // defined(OS_ANDROID)
......
chrome/common/pref_names.h
View file @ 5d52425f
...@@ -536,6 +536,7 @@ extern const char kContentSuggestionsNotificationsSentCount[]; ...@@ -536,6 +536,7 @@ extern const char kContentSuggestionsNotificationsSentCount[];
extern const char kNtpCustomBackgroundDict[]; extern const char kNtpCustomBackgroundDict[];
extern const char kNtpCustomBackgroundLocalToDevice[]; extern const char kNtpCustomBackgroundLocalToDevice[];
extern const char kNtpSearchSuggestionsBlacklist[]; extern const char kNtpSearchSuggestionsBlacklist[];
extern const char kNtpSearchSuggestionsImpressions[];
extern const char kNtpSearchSuggestionsOptOut[]; extern const char kNtpSearchSuggestionsOptOut[];
#endif // defined(OS_ANDROID) #endif // defined(OS_ANDROID)
extern const char kNtpShownPage[]; extern const char kNtpShownPage[];
......
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