Commit 5e631edd authored by Brandon Wylie's avatar Brandon Wylie Committed by Commit Bot

Revert "Prepare explore_sites unittests for imminent switching on of the...

Revert "Prepare explore_sites unittests for imminent switching on of the HistoryServiceUsesTaskScheduler experiment."

This reverts commit 02be9723.

Reason for revert: Broke the build here: 
https://ci.chromium.org/p/chromium/builders/ci/Android%20x86%20Builder%20(dbg)/42327

Original change's description:
> Prepare explore_sites unittests for imminent switching on of the HistoryServiceUsesTaskScheduler experiment.
> 
> The feature will soon be flipped to ENABLED_BY_DEFAULT. This will make HistoryServiceBackend's task runner
> dispatch to the ThreadPool instead of a dedicated base::Thread. The tests need to be
> prepared to stay functional when that happens.
> 
> This is done as the last mile of the chrome-wide effort to coalesce as much work as possible
> to the ThreadPool.
> 
> Bug: 661143
> Change-Id: I7e08e21c212c2852891d30d0db70b946d932dd4d
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1865595
> Reviewed-by: Justin DeWitt <dewittj@chromium.org>
> Commit-Queue: Oliver Li <olivierli@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#707832}

TBR=dewittj@chromium.org,olivierli@chromium.org

Change-Id: I0d07815c8e1c7c145c970df7b173e58c660e41ce
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 661143
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1872127Reviewed-by: default avatarBrandon Wylie <wylieb@chromium.org>
Commit-Queue: Brandon Wylie <wylieb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#707862}
parent 734ea5e7
...@@ -85,7 +85,7 @@ KeyedService* ExploreSitesServiceFactory::BuildServiceInstanceFor( ...@@ -85,7 +85,7 @@ KeyedService* ExploreSitesServiceFactory::BuildServiceInstanceFor(
HistoryServiceFactory::GetForProfile(profile, HistoryServiceFactory::GetForProfile(profile,
ServiceAccessType::EXPLICIT_ACCESS); ServiceAccessType::EXPLICIT_ACCESS);
auto history_stats_reporter = std::make_unique<HistoryStatisticsReporter>( auto history_stats_reporter = std::make_unique<HistoryStatisticsReporter>(
history_service, profile->GetPrefs()); history_service, profile->GetPrefs(), base::DefaultClock::GetInstance());
return new ExploreSitesServiceImpl(std::move(explore_sites_store), return new ExploreSitesServiceImpl(std::move(explore_sites_store),
std::move(url_loader_factory_getter), std::move(url_loader_factory_getter),
......
...@@ -48,7 +48,7 @@ class ExploreSitesServiceImplTest : public testing::Test { ...@@ -48,7 +48,7 @@ class ExploreSitesServiceImplTest : public testing::Test {
std::make_unique<ExploreSitesStore>( std::make_unique<ExploreSitesStore>(
task_environment_.GetMainThreadTaskRunner()); task_environment_.GetMainThreadTaskRunner());
auto history_stats_reporter = auto history_stats_reporter =
std::make_unique<HistoryStatisticsReporter>(nullptr, nullptr); std::make_unique<HistoryStatisticsReporter>(nullptr, nullptr, nullptr);
service_ = std::make_unique<ExploreSitesServiceImpl>( service_ = std::make_unique<ExploreSitesServiceImpl>(
std::move(store), std::move(store),
std::make_unique<TestURLLoaderFactoryGetter>( std::make_unique<TestURLLoaderFactoryGetter>(
......
...@@ -9,12 +9,14 @@ ...@@ -9,12 +9,14 @@
#include "base/bind.h" #include "base/bind.h"
#include "base/metrics/histogram_macros.h" #include "base/metrics/histogram_macros.h"
#include "base/threading/thread_task_runner_handle.h" #include "base/threading/thread_task_runner_handle.h"
#include "base/time/time.h"
#include "chrome/common/pref_names.h" #include "chrome/common/pref_names.h"
#include "components/history/core/browser/history_service.h" #include "components/history/core/browser/history_service.h"
#include "components/history/core/browser/history_types.h" #include "components/history/core/browser/history_types.h"
namespace { namespace {
// Delay between the scheduling and actual computing/reporting of stats.
const int kComputeStatisticsDelaySeconds = 5;
// Pref name for the persistent timestamp of the last stats reporting. // Pref name for the persistent timestamp of the last stats reporting.
const char kWeeklyStatsReportingTimestamp[] = const char kWeeklyStatsReportingTimestamp[] =
"explore_sites.weekly_stats_reporting_timestamp"; "explore_sites.weekly_stats_reporting_timestamp";
...@@ -31,9 +33,11 @@ void HistoryStatisticsReporter::RegisterPrefs(PrefRegistrySimple* registry) { ...@@ -31,9 +33,11 @@ void HistoryStatisticsReporter::RegisterPrefs(PrefRegistrySimple* registry) {
// from HistoryService::CountUniqueHostsVisitedLastMonth) // from HistoryService::CountUniqueHostsVisitedLastMonth)
HistoryStatisticsReporter::HistoryStatisticsReporter( HistoryStatisticsReporter::HistoryStatisticsReporter(
history::HistoryService* history_service, history::HistoryService* history_service,
PrefService* prefs) PrefService* prefs,
base::Clock* clock)
: history_service_(history_service), : history_service_(history_service),
prefs_(prefs), prefs_(prefs),
clock_(clock),
history_service_observer_(this) {} history_service_observer_(this) {}
HistoryStatisticsReporter::~HistoryStatisticsReporter() { HistoryStatisticsReporter::~HistoryStatisticsReporter() {
...@@ -48,14 +52,17 @@ void HistoryStatisticsReporter::ScheduleReportStatistics() { ...@@ -48,14 +52,17 @@ void HistoryStatisticsReporter::ScheduleReportStatistics() {
// If we've already reported metrics during last week, bail out. // If we've already reported metrics during last week, bail out.
base::Time last_report_time = prefs_->GetTime(kWeeklyStatsReportingTimestamp); base::Time last_report_time = prefs_->GetTime(kWeeklyStatsReportingTimestamp);
if (last_report_time > base::Time::Now() - base::TimeDelta::FromDays(7)) if (last_report_time > clock_->Now() - base::TimeDelta::FromDays(7))
return; return;
base::TimeDelta computeStatisticsDelay =
base::TimeDelta::FromSeconds(kComputeStatisticsDelaySeconds);
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask( base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
FROM_HERE, FROM_HERE,
base::BindOnce(&HistoryStatisticsReporter::MaybeReportStatistics, base::BindOnce(&HistoryStatisticsReporter::MaybeReportStatistics,
weak_ptr_factory_.GetWeakPtr()), weak_ptr_factory_.GetWeakPtr()),
kComputeStatisticsDelay); computeStatisticsDelay);
} }
void HistoryStatisticsReporter::OnHistoryServiceLoaded( void HistoryStatisticsReporter::OnHistoryServiceLoaded(
...@@ -94,6 +101,6 @@ void HistoryStatisticsReporter::ReportStatistics( ...@@ -94,6 +101,6 @@ void HistoryStatisticsReporter::ReportStatistics(
return; return;
UMA_HISTOGRAM_COUNTS_1000("ExploreSites.MonthlyHostCount", result.count); UMA_HISTOGRAM_COUNTS_1000("ExploreSites.MonthlyHostCount", result.count);
// Remember when stats were reported to skip attempts until next week. // Remember when stats were reported to skip attempts until next week.
prefs_->SetTime(kWeeklyStatsReportingTimestamp, base::Time::Now()); prefs_->SetTime(kWeeklyStatsReportingTimestamp, clock_->Now());
} }
} // namespace explore_sites } // namespace explore_sites
...@@ -11,6 +11,7 @@ ...@@ -11,6 +11,7 @@
#include "base/memory/weak_ptr.h" #include "base/memory/weak_ptr.h"
#include "base/scoped_observer.h" #include "base/scoped_observer.h"
#include "base/task/cancelable_task_tracker.h" #include "base/task/cancelable_task_tracker.h"
#include "base/time/clock.h"
#include "components/history/core/browser/history_service.h" #include "components/history/core/browser/history_service.h"
#include "components/history/core/browser/history_service_observer.h" #include "components/history/core/browser/history_service_observer.h"
#include "components/prefs/pref_registry_simple.h" #include "components/prefs/pref_registry_simple.h"
...@@ -19,14 +20,11 @@ ...@@ -19,14 +20,11 @@
namespace explore_sites { namespace explore_sites {
class HistoryStatisticsReporter : public history::HistoryServiceObserver { class HistoryStatisticsReporter : public history::HistoryServiceObserver {
public: public:
// Delay between the scheduling and actual computing/reporting of stats.
static constexpr base::TimeDelta kComputeStatisticsDelay =
base::TimeDelta::FromSeconds(5);
static void RegisterPrefs(PrefRegistrySimple* registry); static void RegisterPrefs(PrefRegistrySimple* registry);
HistoryStatisticsReporter(history::HistoryService* history_service, HistoryStatisticsReporter(history::HistoryService* history_service,
PrefService* prefs); PrefService* prefs,
base::Clock* clock);
~HistoryStatisticsReporter() override; ~HistoryStatisticsReporter() override;
// Schedules delayed task to compute/report history statistics. // Schedules delayed task to compute/report history statistics.
...@@ -48,6 +46,7 @@ class HistoryStatisticsReporter : public history::HistoryServiceObserver { ...@@ -48,6 +46,7 @@ class HistoryStatisticsReporter : public history::HistoryServiceObserver {
history::HistoryService* const history_service_; history::HistoryService* const history_service_;
PrefService* prefs_; PrefService* prefs_;
base::Clock* clock_;
base::CancelableTaskTracker cancelable_task_tracker_; base::CancelableTaskTracker cancelable_task_tracker_;
ScopedObserver<history::HistoryService, history::HistoryServiceObserver> ScopedObserver<history::HistoryService, history::HistoryServiceObserver>
......
...@@ -8,10 +8,7 @@ ...@@ -8,10 +8,7 @@
#include "base/files/scoped_temp_dir.h" #include "base/files/scoped_temp_dir.h"
#include "base/test/bind_test_util.h" #include "base/test/bind_test_util.h"
#include "base/test/metrics/histogram_tester.h" #include "base/test/metrics/histogram_tester.h"
#include "base/test/scoped_feature_list.h" #include "base/test/test_mock_time_task_runner.h"
#include "base/test/task_environment.h"
#include "base/time/clock.h"
#include "base/time/time.h"
#include "components/history/core/browser/history_database_params.h" #include "components/history/core/browser/history_database_params.h"
#include "components/history/core/browser/history_service.h" #include "components/history/core/browser/history_service.h"
#include "components/history/core/test/history_service_test_util.h" #include "components/history/core/test/history_service_test_util.h"
...@@ -33,34 +30,30 @@ namespace explore_sites { ...@@ -33,34 +30,30 @@ namespace explore_sites {
class HistoryStatisticsReporterTest : public testing::Test { class HistoryStatisticsReporterTest : public testing::Test {
public: public:
HistoryStatisticsReporterTest() HistoryStatisticsReporterTest()
: task_environment_( : task_runner_(new base::TestMockTimeTaskRunner(
base::test::SingleThreadTaskEnvironment::TimeSource::MOCK_TIME) {} base::TestMockTimeTaskRunner::Type::kBoundToThread)) {}
~HistoryStatisticsReporterTest() override {} ~HistoryStatisticsReporterTest() override {}
void SetUp() override { void SetUp() override {
feature_list_.InitWithFeatures(
{history::HistoryService::kHistoryServiceUsesTaskScheduler}, {});
HistoryStatisticsReporter::RegisterPrefs(pref_service_.registry()); HistoryStatisticsReporter::RegisterPrefs(pref_service_.registry());
ASSERT_TRUE(history_dir_.CreateUniqueTempDir()); ASSERT_TRUE(history_dir_.CreateUniqueTempDir());
// Creates HistoryService, but does not load it yet. Use LoadHistory() from // Creates HistoryService, but does not load it yet. Use LoadHistory() from
// tests to control loading of HistoryService. // tests to control loading of HistoryService.
history_service_ = std::make_unique<history::HistoryService>(); history_service_ = std::make_unique<history::HistoryService>();
reporter_ = std::make_unique<HistoryStatisticsReporter>(history_service(), reporter_ = std::make_unique<HistoryStatisticsReporter>(
&pref_service_); history_service(), &pref_service_, task_runner_->GetMockClock());
} }
// Wait for separate background task runner in HistoryService to complete // Wait for separate background task runner in HistoryService to complete
// all tasks and then all the tasks on the current one to complete as well. // all tasks and then all the tasks on the current one to complete as well.
void RunUntilIdle() { void RunUntilIdle() {
history::BlockUntilHistoryProcessesPendingRequests(history_service()); history::BlockUntilHistoryProcessesPendingRequests(history_service());
task_environment_.RunUntilIdle(); task_runner_->RunUntilIdle();
} }
void ScheduleReportAndRunUntilIdle() { void ScheduleReportAndRunUntilIdle() {
reporter()->ScheduleReportStatistics(); reporter()->ScheduleReportStatistics();
task_environment_.FastForwardBy( task_runner()->FastForwardUntilNoTasksRemain();
HistoryStatisticsReporter::kComputeStatisticsDelay);
RunUntilIdle(); RunUntilIdle();
} }
...@@ -75,13 +68,11 @@ class HistoryStatisticsReporterTest : public testing::Test { ...@@ -75,13 +68,11 @@ class HistoryStatisticsReporterTest : public testing::Test {
HistoryStatisticsReporter* reporter() const { return reporter_.get(); } HistoryStatisticsReporter* reporter() const { return reporter_.get(); }
const base::HistogramTester& histograms() const { return histogram_tester_; } const base::HistogramTester& histograms() const { return histogram_tester_; }
history::HistoryService* history_service() { return history_service_.get(); } history::HistoryService* history_service() { return history_service_.get(); }
base::TestMockTimeTaskRunner* task_runner() { return task_runner_.get(); }
TestingPrefServiceSimple* prefs() { return &pref_service_; } TestingPrefServiceSimple* prefs() { return &pref_service_; }
protected:
base::test::TaskEnvironment task_environment_;
private: private:
base::test::ScopedFeatureList feature_list_; scoped_refptr<base::TestMockTimeTaskRunner> task_runner_;
base::ScopedTempDir history_dir_; base::ScopedTempDir history_dir_;
TestingPrefServiceSimple pref_service_; TestingPrefServiceSimple pref_service_;
base::HistogramTester histogram_tester_; base::HistogramTester histogram_tester_;
...@@ -96,8 +87,7 @@ TEST_F(HistoryStatisticsReporterTest, HistoryNotLoaded) { ...@@ -96,8 +87,7 @@ TEST_F(HistoryStatisticsReporterTest, HistoryNotLoaded) {
reporter()->ScheduleReportStatistics(); reporter()->ScheduleReportStatistics();
// Move past initial delay of reporter. // Move past initial delay of reporter.
task_environment_.FastForwardBy( task_runner()->FastForwardUntilNoTasksRemain();
HistoryStatisticsReporter::kComputeStatisticsDelay);
// Since History is not yet loaded, there should be no histograms. // Since History is not yet loaded, there should be no histograms.
histograms().ExpectTotalCount("History.DatabaseMonthlyHostCountTime", 0); histograms().ExpectTotalCount("History.DatabaseMonthlyHostCountTime", 0);
...@@ -118,8 +108,7 @@ TEST_F(HistoryStatisticsReporterTest, HistoryLoaded) { ...@@ -118,8 +108,7 @@ TEST_F(HistoryStatisticsReporterTest, HistoryLoaded) {
reporter()->ScheduleReportStatistics(); reporter()->ScheduleReportStatistics();
// Move past initial delay of reporter. // Move past initial delay of reporter.
task_environment_.FastForwardBy( task_runner()->FastForwardUntilNoTasksRemain();
HistoryStatisticsReporter::kComputeStatisticsDelay);
RunUntilIdle(); RunUntilIdle();
// Since History is already loaded, there should be a sample reported. // Since History is already loaded, there should be a sample reported.
...@@ -137,8 +126,7 @@ TEST_F(HistoryStatisticsReporterTest, HistoryLoadedTimeDelay) { ...@@ -137,8 +126,7 @@ TEST_F(HistoryStatisticsReporterTest, HistoryLoadedTimeDelay) {
histograms().ExpectTotalCount("History.DatabaseMonthlyHostCountTime", 0); histograms().ExpectTotalCount("History.DatabaseMonthlyHostCountTime", 0);
// Move past initial delay of reporter. // Move past initial delay of reporter.
task_environment_.FastForwardBy( task_runner()->FastForwardUntilNoTasksRemain();
HistoryStatisticsReporter::kComputeStatisticsDelay);
RunUntilIdle(); RunUntilIdle();
// Since History is already loaded, there should be a sample reported. // Since History is already loaded, there should be a sample reported.
...@@ -212,14 +200,15 @@ TEST_F(HistoryStatisticsReporterTest, OneRunPerWeekSaveTimestamp) { ...@@ -212,14 +200,15 @@ TEST_F(HistoryStatisticsReporterTest, OneRunPerWeekSaveTimestamp) {
histograms().ExpectTotalCount("History.DatabaseMonthlyHostCountTime", 1); histograms().ExpectTotalCount("History.DatabaseMonthlyHostCountTime", 1);
// Reporter should have left the time of request in Prefs. // Reporter should have left the time of request in Prefs.
EXPECT_EQ(base::Time::Now(), base::Time time_now = task_runner()->GetMockClock()->Now();
prefs()->GetTime(kWeeklyStatsReportingTimestamp)); EXPECT_EQ(time_now, prefs()->GetTime(kWeeklyStatsReportingTimestamp));
} }
TEST_F(HistoryStatisticsReporterTest, OneRunPerWeekReadTimestamp) { TEST_F(HistoryStatisticsReporterTest, OneRunPerWeekReadTimestamp) {
ASSERT_TRUE(LoadHistory()); ASSERT_TRUE(LoadHistory());
prefs()->SetTime(kWeeklyStatsReportingTimestamp, base::Time::Now()); prefs()->SetTime(kWeeklyStatsReportingTimestamp,
task_runner()->GetMockClock()->Now());
ScheduleReportAndRunUntilIdle(); ScheduleReportAndRunUntilIdle();
// No queries, a week did not pass yet. // No queries, a week did not pass yet.
...@@ -229,15 +218,16 @@ TEST_F(HistoryStatisticsReporterTest, OneRunPerWeekReadTimestamp) { ...@@ -229,15 +218,16 @@ TEST_F(HistoryStatisticsReporterTest, OneRunPerWeekReadTimestamp) {
TEST_F(HistoryStatisticsReporterTest, OneRunPerWeekReadTimestampAfterWeek) { TEST_F(HistoryStatisticsReporterTest, OneRunPerWeekReadTimestampAfterWeek) {
ASSERT_TRUE(LoadHistory()); ASSERT_TRUE(LoadHistory());
prefs()->SetTime(kWeeklyStatsReportingTimestamp, prefs()->SetTime(
base::Time::Now() - base::TimeDelta::FromDays(8)); kWeeklyStatsReportingTimestamp,
task_runner()->GetMockClock()->Now() - base::TimeDelta::FromDays(8));
ScheduleReportAndRunUntilIdle(); ScheduleReportAndRunUntilIdle();
// More than a week since last query, should have gone through. // More than a week since last query, should have gone through.
histograms().ExpectTotalCount("History.DatabaseMonthlyHostCountTime", 1); histograms().ExpectTotalCount("History.DatabaseMonthlyHostCountTime", 1);
// Reporter should have left the time of request in Prefs. // Reporter should have left the time of request in Prefs.
EXPECT_EQ(base::Time::Now(), base::Time time_now = task_runner()->GetMockClock()->Now();
prefs()->GetTime(kWeeklyStatsReportingTimestamp)); EXPECT_EQ(time_now, prefs()->GetTime(kWeeklyStatsReportingTimestamp));
} }
} // namespace explore_sites } // namespace explore_sites
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