Skip to content

GitLab

T
tangled
Commit 41e54be1 authored by Kyle Milka's avatar Kyle Milka Committed by Commit Bot
Browse files
Options

[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: default avatarKristi Park <kristipark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624994}
parent 4e07ff5b
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 @ 41e54be1
...@@ -1278,7 +1278,7 @@ void LocalNtpSource::MaybeServeSearchSuggestions( ...@@ -1278,7 +1278,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 @ 41e54be1
...@@ -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 @ 41e54be1
...@@ -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 @ 41e54be1
...@@ -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 @ 41e54be1
...@@ -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 @ 41e54be1
...@@ -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 @ 41e54be1
...@@ -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 @ 41e54be1
...@@ -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 @ 41e54be1
...@@ -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 @ 41e54be1
...@@ -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