Skip to content

Commit

Permalink
Revert "[NTP] Cap search suggestion impressions"
Browse files Browse the repository at this point in the history 
This reverts commit 41e54be.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 624994 as the
culprit for flakes in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/flake/flake-culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyQwsSDEZsYWtlQ3VscHJpdCIxY2hyb21pdW0vNDFlNTRiZTFkOTIzMzUzZjcyNDkwZjE5YTE1ZTczZWIwOTI0OGEzOAw

Sample Failed Build: https://ci.chromium.org/buildbot/chromium.mac/Mac10.13%20Tests/9168

Sample Failed Step: unit_tests on (none) GPU on Mac

Sample Flaky Test: SearchSuggestServiceTest.RequestsFreezeOnEmptyResponse

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}

Change-Id: I73c18a94545e84ee6eb753d3a4643cc55ea32080
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 904565, 924415
Reviewed-on: https://chromium-review.googlesource.com/c/1429622
Cr-Commit-Position: refs/heads/master@{#625087}
  • Loading branch information
Findit committed Jan 23, 2019
1 parent 2854de1 commit a0a28d4
Show file tree
Hide file tree
Showing 11 changed files with 30 additions and 406 deletions.
2 changes: 1 addition & 1 deletion chrome/browser/search/local_ntp_source.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1278,7 +1278,7 @@ void LocalNtpSource::MaybeServeSearchSuggestions(
}

SearchSuggestData suggest_data = *data;
search_suggest_service_->SuggestionsDisplayed();
search_suggest_service_->ClearSearchSuggestData();
scoped_refptr<base::RefCountedString> result;
std::string js;
base::JSONWriter::Write(*ConvertSearchSuggestDataToDict(suggest_data), &js);
Expand Down
6 changes: 1 addition & 5 deletions chrome/browser/search/search_suggest/search_suggest_data.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,7 @@ SearchSuggestData& SearchSuggestData::operator=(SearchSuggestData&&) = default;

bool operator==(const SearchSuggestData& lhs, const SearchSuggestData& rhs) {
return lhs.suggestions_html == rhs.suggestions_html &&
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;
lhs.end_of_body_script == rhs.end_of_body_script;
}

bool operator!=(const SearchSuggestData& lhs, const SearchSuggestData& rhs) {
Expand Down
5 changes: 0 additions & 5 deletions chrome/browser/search/search_suggest/search_suggest_data.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,6 @@ struct SearchSuggestData {
// Javascript for search suggestion that should be appended at the end of the
// New Tab Page <body>.
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);
Expand Down
8 changes: 2 additions & 6 deletions chrome/browser/search/search_suggest/search_suggest_loader.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,8 @@ class SearchSuggestLoader {
// A fatal error occurred, such as the server responding with an error code
// or with invalid data. Any previously cached response should be cleared.
FATAL_ERROR,
// The user has opted out of seeing search suggestions on the NTP
OPTED_OUT,
// The limit for number of impressions was hit.
IMPRESSION_CAP,
// Received an empty response so requests are temporarily frozen.
REQUESTS_FROZEN
// The user has opted out of seeing search suggestions on the NTP.
OPTED_OUT
};
using SearchSuggestionsCallback =
base::OnceCallback<void(Status,
Expand Down
26 changes: 0 additions & 26 deletions chrome/browser/search/search_suggest/search_suggest_loader_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,32 +75,6 @@ base::Optional<SearchSuggestData> JsonToSearchSuggestionData(

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;
}

Expand Down
19 changes: 6 additions & 13 deletions chrome/browser/search/search_suggest/search_suggest_loader_impl_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,7 @@ namespace {
const char kApplicationLocale[] = "us";

const char kMinimalValidResponse[] = R"json({"update": { "query_suggestions": {
"query_suggestions_with_html": "", "script": "",
"impression_cap_expire_time_ms": 0, "request_freeze_time_ms": 0,
"max_impressions": 0
"query_suggestions_with_html": "", "script": ""
}}})json";

// Required to instantiate a GoogleUrlTracker in UNIT_TEST_MODE.
Expand Down Expand Up @@ -172,11 +170,9 @@ TEST_F(SearchSuggestLoaderImplTest, HandlesResponsePreamble) {
}

TEST_F(SearchSuggestLoaderImplTest, ParsesFullResponse) {
SetUpResponseWithData(R"json({"update": { "query_suggestions": {
"query_suggestions_with_html": "<div></div>",
"script": "<script></script>", "impression_cap_expire_time_ms": 1,
"request_freeze_time_ms": 2, "max_impressions": 3
}}})json");
SetUpResponseWithData(
R"json({"update": { "query_suggestions": {"query_suggestions_with_html" :
"<div></div>", "script" : "<script></script>"}}})json");

base::MockCallback<SearchSuggestLoader::SearchSuggestionsCallback> callback;
std::string blacklist;
Expand All @@ -189,11 +185,8 @@ TEST_F(SearchSuggestLoaderImplTest, ParsesFullResponse) {
loop.Run();

ASSERT_TRUE(data.has_value());
EXPECT_EQ("<div></div>", data->suggestions_html);
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);
EXPECT_THAT(data->suggestions_html, Eq("<div></div>"));
EXPECT_THAT(data->end_of_body_script, Eq("<script></script>"));
}

TEST_F(SearchSuggestLoaderImplTest, CoalescesMultipleRequests) {
Expand Down
126 changes: 3 additions & 123 deletions chrome/browser/search/search_suggest/search_suggest_service.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,34 +13,6 @@
#include "components/prefs/scoped_user_pref_update.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
: public identity::IdentityManager::Observer {
public:
Expand Down Expand Up @@ -100,12 +72,6 @@ void SearchSuggestService::Refresh() {
} else if (pref_service_->GetBoolean(prefs::kNtpSearchSuggestionsOptOut)) {
SearchSuggestDataLoaded(SearchSuggestLoader::Status::OPTED_OUT,
base::nullopt);
} else if (RequestsFrozen()) {
SearchSuggestDataLoaded(SearchSuggestLoader::Status::REQUESTS_FROZEN,
base::nullopt);
} else if (ImpressionCapReached()) {
SearchSuggestDataLoaded(SearchSuggestLoader::Status::IMPRESSION_CAP,
base::nullopt);
} else {
const std::string blacklist = GetBlacklistAsString();
loader_->Load(blacklist,
Expand Down Expand Up @@ -136,22 +102,10 @@ void SearchSuggestService::SearchSuggestDataLoaded(
// In case of transient errors, keep our cached data (if any), but still
// notify observers of the finished load (attempt).
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;

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();
}
Expand All @@ -162,61 +116,6 @@ 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,
long task_id) {
std::string task_version_id =
Expand Down Expand Up @@ -279,23 +178,6 @@ std::string SearchSuggestService::GetBlacklistAsString() {
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() {
pref_service_->SetBoolean(prefs::kNtpSearchSuggestionsOptOut, true);

Expand All @@ -305,7 +187,5 @@ void SearchSuggestService::OptOutOfSearchSuggestions() {
// static
void SearchSuggestService::RegisterProfilePrefs(PrefRegistrySimple* registry) {
registry->RegisterDictionaryPref(prefs::kNtpSearchSuggestionsBlacklist);
registry->RegisterDictionaryPref(prefs::kNtpSearchSuggestionsImpressions,
ImpressionDictDefaults());
registry->RegisterBooleanPref(prefs::kNtpSearchSuggestionsOptOut, false);
}
31 changes: 5 additions & 26 deletions chrome/browser/search/search_suggest/search_suggest_service.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,11 @@ class SearchSuggestService : public KeyedService {
return search_suggest_data_;
}

// Determines if a request for search suggestions should be made. If a request
// should not be made immediately call SearchSuggestDataLoaded with the
// reason. Otherwise requests an asynchronous refresh from the network. After
// the update completes, regardless of success, OnSearchSuggestDataUpdated
// will be called on the observers.
// Clears any currently cached search suggest data.
void ClearSearchSuggestData() { search_suggest_data_ = base::nullopt; }

// Requests an asynchronous refresh from the network. After the update
// completes, OnSearchSuggestDataUpdated will be called on the observers.
void Refresh();

// Add/remove observers. All observers must unregister themselves before the
Expand Down Expand Up @@ -82,37 +82,16 @@ class SearchSuggestService : public KeyedService {
// "task_id1:hash1,hash2,hash3;task_id2;task_id3:hash1,hash2".
std::string GetBlacklistAsString();

// Called when suggestions are displayed on the NTP, clears the cached data
// and updates timestamps and impression counts.
void SuggestionsDisplayed();

private:
class SigninObserver;

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,
const base::Optional<SearchSuggestData>& data);

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<SigninObserver> signin_observer_;
Expand Down
Loading

0 comments on commit a0a28d4

Please sign in to comment.