Commit 38a39f60 authored by Nicolas Ouellet-Payeur's avatar Nicolas Ouellet-Payeur Committed by Commit Bot

Revert "[BrowserSwitcher] Deflake BrowserSwitcherServiceTest"

This reverts commit 54aa10ad.

Reason for revert: crbug.com/1073010

Original change's description:
> [BrowserSwitcher] Deflake BrowserSwitcherServiceTest
> 
> Previously, the tests waited for action_timeout(), to wait for LBS to
> load the rules and write some files. As expected, this is fairly flaky,
> and it slows down the tests.
> 
> Now, `BrowserSwitcherService' exposes a new event for testing, which
> fires once LBS has loaded the rules. This should reduce flakes by
> listening for the event instead of using an unreliable timeout.
> 
> Also rewrote some tests in a synchronous style, which improves
> readability.
> 
> Bug: 1044619, 1070400
> Change-Id: If1a2093cfe3bface73a46896ff2bc95b80ab2f2e
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2152885
> Commit-Queue: Nicolas Ouellet-Payeur <nicolaso@chromium.org>
> Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#760531}

TBR=pastarmovj@chromium.org,nicolaso@chromium.org

Change-Id: If573be05ce1a3d4a34449bd6038f16129cb4d1fa
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1044619, 1070400
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2159313Reviewed-by: default avatarNicolas Ouellet-Payeur <nicolaso@chromium.org>
Commit-Queue: Nicolas Ouellet-Payeur <nicolaso@chromium.org>
Cr-Commit-Position: refs/heads/master@{#760985}
parent 920d8f93
...@@ -239,11 +239,6 @@ void BrowserSwitcherService::Init() { ...@@ -239,11 +239,6 @@ void BrowserSwitcherService::Init() {
StartDownload(fetch_delay()); StartDownload(fetch_delay());
} }
void BrowserSwitcherService::OnAllRulesetsLoadedForTesting(
base::OnceCallback<void()> cb) {
all_rulesets_loaded_callback_for_testing_ = std::move(cb);
}
void BrowserSwitcherService::StartDownload(base::TimeDelta delay) { void BrowserSwitcherService::StartDownload(base::TimeDelta delay) {
// This destroys the previous XmlDownloader, which cancels any scheduled // This destroys the previous XmlDownloader, which cancels any scheduled
// refresh operations. // refresh operations.
...@@ -324,8 +319,6 @@ void BrowserSwitcherService::LoadRulesFromPrefs() { ...@@ -324,8 +319,6 @@ void BrowserSwitcherService::LoadRulesFromPrefs() {
void BrowserSwitcherService::OnAllRulesetsParsed() { void BrowserSwitcherService::OnAllRulesetsParsed() {
callback_list_.Notify(this); callback_list_.Notify(this);
if (all_rulesets_loaded_callback_for_testing_)
std::move(all_rulesets_loaded_callback_for_testing_).Run();
} }
std::unique_ptr<BrowserSwitcherService::CallbackSubscription> std::unique_ptr<BrowserSwitcherService::CallbackSubscription>
......
...@@ -124,8 +124,6 @@ class BrowserSwitcherService : public KeyedService { ...@@ -124,8 +124,6 @@ class BrowserSwitcherService : public KeyedService {
explicit BrowserSwitcherService(Profile* profile); explicit BrowserSwitcherService(Profile* profile);
~BrowserSwitcherService() override; ~BrowserSwitcherService() override;
virtual void Init();
// KeyedService: // KeyedService:
void Shutdown() override; void Shutdown() override;
...@@ -151,10 +149,7 @@ class BrowserSwitcherService : public KeyedService { ...@@ -151,10 +149,7 @@ class BrowserSwitcherService : public KeyedService {
// happens. // happens.
virtual void LoadRulesFromPrefs(); virtual void LoadRulesFromPrefs();
// Called after all XML rulesets finished downloading, and the rules are void Init();
// applied. The XML is downloaded asynchronously, so browser tests use this
// event to check that they applied correctly.
void OnAllRulesetsLoadedForTesting(base::OnceCallback<void()> callback);
protected: protected:
virtual void OnAllRulesetsParsed(); virtual void OnAllRulesetsParsed();
...@@ -199,8 +194,6 @@ class BrowserSwitcherService : public KeyedService { ...@@ -199,8 +194,6 @@ class BrowserSwitcherService : public KeyedService {
// CallbackList for OnAllRulesetsParsed() listeners. // CallbackList for OnAllRulesetsParsed() listeners.
base::CallbackList<AllRulesetsParsedCallbackSignature> callback_list_; base::CallbackList<AllRulesetsParsedCallbackSignature> callback_list_;
base::OnceCallback<void()> all_rulesets_loaded_callback_for_testing_;
// Per-profile helpers. // Per-profile helpers.
std::unique_ptr<AlternativeBrowserDriver> driver_; std::unique_ptr<AlternativeBrowserDriver> driver_;
std::unique_ptr<BrowserSwitcherSitelist> sitelist_; std::unique_ptr<BrowserSwitcherSitelist> sitelist_;
......
...@@ -27,8 +27,6 @@ ...@@ -27,8 +27,6 @@
#include "chrome/browser/browser_switcher/browser_switcher_sitelist.h" #include "chrome/browser/browser_switcher/browser_switcher_sitelist.h"
#include "chrome/browser/browser_switcher/ieem_sitelist_parser.h" #include "chrome/browser/browser_switcher/ieem_sitelist_parser.h"
#include "chrome/browser/profiles/profile.h" #include "chrome/browser/profiles/profile.h"
#include "content/public/browser/browser_task_traits.h"
#include "content/public/browser/browser_thread.h"
#include "extensions/browser/extension_registry.h" #include "extensions/browser/extension_registry.h"
namespace browser_switcher { namespace browser_switcher {
...@@ -138,7 +136,9 @@ BrowserSwitcherServiceWin::BrowserSwitcherServiceWin( ...@@ -138,7 +136,9 @@ BrowserSwitcherServiceWin::BrowserSwitcherServiceWin(
cache_dir_for_testing_(std::move(cache_dir_for_testing)), cache_dir_for_testing_(std::move(cache_dir_for_testing)),
sequenced_task_runner_(base::ThreadPool::CreateSequencedTaskRunner( sequenced_task_runner_(base::ThreadPool::CreateSequencedTaskRunner(
{base::MayBlock(), base::TaskPriority::BEST_EFFORT, {base::MayBlock(), base::TaskPriority::BEST_EFFORT,
base::TaskShutdownBehavior::BLOCK_SHUTDOWN})) {} base::TaskShutdownBehavior::BLOCK_SHUTDOWN})) {
UpdateAllCacheFiles();
}
BrowserSwitcherServiceWin::~BrowserSwitcherServiceWin() = default; BrowserSwitcherServiceWin::~BrowserSwitcherServiceWin() = default;
...@@ -166,16 +166,14 @@ std::vector<RulesetSource> BrowserSwitcherServiceWin::GetRulesetSources() { ...@@ -166,16 +166,14 @@ std::vector<RulesetSource> BrowserSwitcherServiceWin::GetRulesetSources() {
return sources; return sources;
} }
void BrowserSwitcherServiceWin::Init() {
BrowserSwitcherService::Init();
UpdateAllCacheFiles();
}
void BrowserSwitcherServiceWin::LoadRulesFromPrefs() { void BrowserSwitcherServiceWin::LoadRulesFromPrefs() {
BrowserSwitcherService::LoadRulesFromPrefs(); BrowserSwitcherService::LoadRulesFromPrefs();
if (prefs().UseIeSitelist()) if (prefs().UseIeSitelist())
sitelist()->SetIeemSitelist( sitelist()->SetIeemSitelist(
ParsedXml(prefs().GetCachedIeemSitelist(), base::nullopt)); ParsedXml(prefs().GetCachedIeemSitelist(), base::nullopt));
if (!prefs().IsEnabled())
return;
SavePrefsToFile();
} }
base::FilePath BrowserSwitcherServiceWin::GetCacheDir() { base::FilePath BrowserSwitcherServiceWin::GetCacheDir() {
...@@ -234,51 +232,25 @@ void BrowserSwitcherServiceWin::OnIeemSitelistParsed(ParsedXml xml) { ...@@ -234,51 +232,25 @@ void BrowserSwitcherServiceWin::OnIeemSitelistParsed(ParsedXml xml) {
} }
} }
void BrowserSwitcherServiceWin::CacheFileUpdated() { void BrowserSwitcherServiceWin::SavePrefsToFile() {
if (cache_file_updated_callback_for_testing_) DCHECK(prefs().IsEnabled());
std::move(cache_file_updated_callback_for_testing_).Run();
}
void BrowserSwitcherServiceWin::SitelistCacheFileUpdated() {
if (sitelist_cache_file_updated_callback_for_testing_)
std::move(sitelist_cache_file_updated_callback_for_testing_).Run();
}
void BrowserSwitcherServiceWin::OnCacheFileUpdatedForTesting(
base::OnceClosure cb) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
cache_file_updated_callback_for_testing_ = std::move(cb);
}
void BrowserSwitcherServiceWin::OnSitelistCacheFileUpdatedForTesting(
base::OnceClosure cb) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
sitelist_cache_file_updated_callback_for_testing_ = std::move(cb);
}
void BrowserSwitcherServiceWin::DeletePrefsFile() {
base::FilePath path = GetCacheDir(); base::FilePath path = GetCacheDir();
if (path.empty()) if (path.empty())
return; return;
path = path.AppendASCII("cache.dat"); path = path.AppendASCII("cache.dat");
sequenced_task_runner_->PostTaskAndReply( sequenced_task_runner_->PostTask(
FROM_HERE, base::BindOnce(&RemoveFile, std::move(path)), FROM_HERE,
base::BindOnce(&BrowserSwitcherServiceWin::CacheFileUpdated, base::BindOnce(&SaveDataToFile, SerializeCacheFile(prefs(), sitelist()),
base::Unretained(this))); std::move(path)));
} }
void BrowserSwitcherServiceWin::SavePrefsToFile() { void BrowserSwitcherServiceWin::DeletePrefsFile() {
DCHECK(prefs().IsEnabled());
base::FilePath path = GetCacheDir(); base::FilePath path = GetCacheDir();
if (path.empty()) if (path.empty())
return; return;
path = path.AppendASCII("cache.dat"); path = path.AppendASCII("cache.dat");
sequenced_task_runner_->PostTaskAndReply( sequenced_task_runner_->PostTask(
FROM_HERE, FROM_HERE, base::BindOnce(&RemoveFile, std::move(path)));
base::BindOnce(&SaveDataToFile, SerializeCacheFile(prefs(), sitelist()),
std::move(path)),
base::BindOnce(&BrowserSwitcherServiceWin::CacheFileUpdated,
base::Unretained(this)));
} }
void BrowserSwitcherServiceWin::DeleteSitelistCacheFile() { void BrowserSwitcherServiceWin::DeleteSitelistCacheFile() {
...@@ -286,10 +258,8 @@ void BrowserSwitcherServiceWin::DeleteSitelistCacheFile() { ...@@ -286,10 +258,8 @@ void BrowserSwitcherServiceWin::DeleteSitelistCacheFile() {
if (path.empty()) if (path.empty())
return; return;
path = path.AppendASCII("sitelistcache.dat"); path = path.AppendASCII("sitelistcache.dat");
sequenced_task_runner_->PostTaskAndReply( sequenced_task_runner_->PostTask(
FROM_HERE, base::BindOnce(&RemoveFile, std::move(path)), FROM_HERE, base::BindOnce(&RemoveFile, std::move(path)));
base::BindOnce(&BrowserSwitcherServiceWin::SitelistCacheFileUpdated,
base::Unretained(this)));
} }
void BrowserSwitcherServiceWin::UpdateAllCacheFiles() { void BrowserSwitcherServiceWin::UpdateAllCacheFiles() {
......
...@@ -22,8 +22,6 @@ class BrowserSwitcherServiceWin : public BrowserSwitcherService { ...@@ -22,8 +22,6 @@ class BrowserSwitcherServiceWin : public BrowserSwitcherService {
base::FilePath cache_dir_for_testing = base::FilePath()); base::FilePath cache_dir_for_testing = base::FilePath());
~BrowserSwitcherServiceWin() override; ~BrowserSwitcherServiceWin() override;
void Init() override;
static void SetIeemSitelistUrlForTesting(const std::string& url); static void SetIeemSitelistUrlForTesting(const std::string& url);
// BrowserSwitcherService: // BrowserSwitcherService:
...@@ -31,9 +29,6 @@ class BrowserSwitcherServiceWin : public BrowserSwitcherService { ...@@ -31,9 +29,6 @@ class BrowserSwitcherServiceWin : public BrowserSwitcherService {
void LoadRulesFromPrefs() override; void LoadRulesFromPrefs() override;
void OnCacheFileUpdatedForTesting(base::OnceClosure cb);
void OnSitelistCacheFileUpdatedForTesting(base::OnceClosure cb);
protected: protected:
// BrowserSwitcherService: // BrowserSwitcherService:
void OnAllRulesetsParsed() override; void OnAllRulesetsParsed() override;
...@@ -64,16 +59,11 @@ class BrowserSwitcherServiceWin : public BrowserSwitcherService { ...@@ -64,16 +59,11 @@ class BrowserSwitcherServiceWin : public BrowserSwitcherService {
// extension, or from a previous Chrome version. Called during initialization. // extension, or from a previous Chrome version. Called during initialization.
void DeleteSitelistCacheFile(); void DeleteSitelistCacheFile();
void CacheFileUpdated();
void SitelistCacheFileUpdated();
// Updates or cleans up cache.dat and sitelistcache.dat, based on whether // Updates or cleans up cache.dat and sitelistcache.dat, based on whether
// BrowserSwitcher is enabled or disabled. // BrowserSwitcher is enabled or disabled.
void UpdateAllCacheFiles(); void UpdateAllCacheFiles();
base::FilePath cache_dir_for_testing_; base::FilePath cache_dir_for_testing_;
base::OnceClosure cache_file_updated_callback_for_testing_;
base::OnceClosure sitelist_cache_file_updated_callback_for_testing_;
scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner_; scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner_;
......
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