Skip to content

GitLab

T
tangled
Commit 3e783e26 authored by dgozman's avatar dgozman Committed by Commit bot
Browse files
Options

Fix DevToolsManagerTest.TestObserver flakiness. 

BUG=418258
TBR=pfeldman

Review URL: https://codereview.chromium.org/615533002

Cr-Commit-Position: refs/heads/master@{#297174}
parent 76de7516
Show whitespace changes
Inline Side-by-side
Showing with 61 additions and 38 deletions
content/browser/devtools/devtools_manager.cc
View file @ 3e783e26
...@@ -14,6 +14,12 @@ ...@@ -14,6 +14,12 @@
namespace content { namespace content {
namespace {
int kObserverThrottleInterval = 500; // ms
} // namespace
// static // static
DevToolsManager* DevToolsManager::GetInstance() { DevToolsManager* DevToolsManager::GetInstance() {
return Singleton<DevToolsManager>::get(); return Singleton<DevToolsManager>::get();
...@@ -91,10 +97,14 @@ void DevToolsManager::UpdateTargetListThrottled() { ...@@ -91,10 +97,14 @@ void DevToolsManager::UpdateTargetListThrottled() {
} }
update_target_list_scheduled_ = true; update_target_list_scheduled_ = true;
if (scheduler_.is_null()) {
base::MessageLoop::current()->PostDelayedTask( base::MessageLoop::current()->PostDelayedTask(
FROM_HERE, FROM_HERE,
update_target_list_callback_.callback(), update_target_list_callback_.callback(),
observer_throttle_interval_); base::TimeDelta::FromMilliseconds(kObserverThrottleInterval));
} else {
scheduler_.Run(update_target_list_callback_.callback());
}
update_target_list_required_ = false; update_target_list_required_ = false;
if (!delegate_) { if (!delegate_) {
...@@ -113,14 +123,8 @@ void DevToolsManager::NotifyTargetListChanged( ...@@ -113,14 +123,8 @@ void DevToolsManager::NotifyTargetListChanged(
STLDeleteContainerPointers(targets.begin(), targets.end()); STLDeleteContainerPointers(targets.begin(), targets.end());
} }
// static void DevToolsManager::SetSchedulerForTest(Scheduler scheduler) {
base::TimeDelta DevToolsManager::observer_throttle_interval_ = scheduler_ = scheduler;
base::TimeDelta::FromMilliseconds(500);
// static
void DevToolsManager::SetObserverThrottleIntervalForTest(
base::TimeDelta interval) {
observer_throttle_interval_ = interval;
} }
} // namespace content } // namespace content
content/browser/devtools/devtools_manager.h
View file @ 3e783e26
...@@ -54,7 +54,8 @@ class CONTENT_EXPORT DevToolsManager { ...@@ -54,7 +54,8 @@ class CONTENT_EXPORT DevToolsManager {
void RenderViewCreated(WebContents* web_contents, RenderViewHost* rvh); void RenderViewCreated(WebContents* web_contents, RenderViewHost* rvh);
void AgentHostChanged(scoped_refptr<DevToolsAgentHost> agent_host); void AgentHostChanged(scoped_refptr<DevToolsAgentHost> agent_host);
static void SetObserverThrottleIntervalForTest(base::TimeDelta interval); typedef base::Callback<void(base::Closure)> Scheduler;
void SetSchedulerForTest(Scheduler scheduler);
private: private:
friend struct DefaultSingletonTraits<DevToolsManager>; friend struct DefaultSingletonTraits<DevToolsManager>;
...@@ -69,8 +70,7 @@ class CONTENT_EXPORT DevToolsManager { ...@@ -69,8 +70,7 @@ class CONTENT_EXPORT DevToolsManager {
bool update_target_list_required_; bool update_target_list_required_;
bool update_target_list_scheduled_; bool update_target_list_scheduled_;
base::CancelableClosure update_target_list_callback_; base::CancelableClosure update_target_list_callback_;
Scheduler scheduler_;
static base::TimeDelta observer_throttle_interval_;
DISALLOW_COPY_AND_ASSIGN(DevToolsManager); DISALLOW_COPY_AND_ASSIGN(DevToolsManager);
}; };
......
content/browser/devtools/devtools_manager_unittest.cc
View file @ 3e783e26
...@@ -191,9 +191,6 @@ class TestDevToolsManagerObserver : public DevToolsManager::Observer { ...@@ -191,9 +191,6 @@ class TestDevToolsManagerObserver : public DevToolsManager::Observer {
it != targets.end(); ++it) { it != targets.end(); ++it) {
hosts_.push_back((*it)->GetAgentHost()); hosts_.push_back((*it)->GetAgentHost());
} }
base::MessageLoop::current()->PostTask(
FROM_HERE,
base::MessageLoop::QuitClosure());
} }
private: private:
...@@ -360,14 +357,41 @@ TEST_F(DevToolsManagerTest, TestExternalProxy) { ...@@ -360,14 +357,41 @@ TEST_F(DevToolsManagerTest, TestExternalProxy) {
client_host.Close(); client_host.Close();
} }
class TestDevToolsManagerScheduler {
public:
DevToolsManager::Scheduler callback() {
return base::Bind(&TestDevToolsManagerScheduler::Schedule,
base::Unretained(this));
}
void Run() {
ASSERT_FALSE(closure_.is_null());
base::Closure copy = closure_;
closure_.Reset();
copy.Run();
}
bool IsEmpty() {
return closure_.is_null();
}
private:
void Schedule(base::Closure closure) {
EXPECT_TRUE(closure_.is_null());
closure_ = closure;
}
base::Closure closure_;
};
TEST_F(DevToolsManagerTest, TestObserver) { TEST_F(DevToolsManagerTest, TestObserver) {
GURL url1("data:text/html,<body>Body1</body>"); GURL url1("data:text/html,<body>Body1</body>");
GURL url2("data:text/html,<body>Body2</body>"); GURL url2("data:text/html,<body>Body2</body>");
GURL url3("data:text/html,<body>Body3</body>"); GURL url3("data:text/html,<body>Body3</body>");
TestDevToolsManagerScheduler scheduler;
DevToolsManager* manager = DevToolsManager::GetInstance(); DevToolsManager* manager = DevToolsManager::GetInstance();
DevToolsManager::SetObserverThrottleIntervalForTest( manager->SetSchedulerForTest(scheduler.callback());
base::TimeDelta::FromMilliseconds(200));
contents()->NavigateAndCommit(url1); contents()->NavigateAndCommit(url1);
RunAllPendingInMessageLoop(); RunAllPendingInMessageLoop();
...@@ -375,29 +399,25 @@ TEST_F(DevToolsManagerTest, TestObserver) { ...@@ -375,29 +399,25 @@ TEST_F(DevToolsManagerTest, TestObserver) {
scoped_ptr<TestDevToolsManagerObserver> observer( scoped_ptr<TestDevToolsManagerObserver> observer(
new TestDevToolsManagerObserver()); new TestDevToolsManagerObserver());
manager->AddObserver(observer.get()); manager->AddObserver(observer.get());
RunMessageLoop();
// Added observer should get an update. // Added observer should get an update.
EXPECT_EQ(1, observer->updates_count()); EXPECT_EQ(1, observer->updates_count());
EXPECT_EQ(1u, observer->hosts().size()); ASSERT_EQ(1u, observer->hosts().size());
EXPECT_EQ(contents(), observer->hosts()[0]->GetWebContents()); EXPECT_EQ(contents(), observer->hosts()[0]->GetWebContents());
EXPECT_EQ(url1.spec(), observer->hosts()[0]->GetURL().spec()); EXPECT_EQ(url1.spec(), observer->hosts()[0]->GetURL().spec());
contents()->NavigateAndCommit(url2); contents()->NavigateAndCommit(url2);
RunAllPendingInMessageLoop(); RunAllPendingInMessageLoop();
contents()->NavigateAndCommit(url3); contents()->NavigateAndCommit(url3);
RunMessageLoop(); scheduler.Run();
// Updates should be coalesced. // Updates should be coalesced.
EXPECT_EQ(2, observer->updates_count()); EXPECT_EQ(2, observer->updates_count());
EXPECT_EQ(1u, observer->hosts().size()); ASSERT_EQ(1u, observer->hosts().size());
EXPECT_EQ(contents(), observer->hosts()[0]->GetWebContents()); EXPECT_EQ(contents(), observer->hosts()[0]->GetWebContents());
EXPECT_EQ(url3.spec(), observer->hosts()[0]->GetURL().spec()); EXPECT_EQ(url3.spec(), observer->hosts()[0]->GetURL().spec());
base::MessageLoop::current()->PostDelayedTask(
FROM_HERE,
base::MessageLoop::QuitClosure(),
base::TimeDelta::FromMilliseconds(250));
base::MessageLoop::current()->Run();
// Check there were no extra updates. // Check there were no extra updates.
scheduler.Run();
EXPECT_TRUE(scheduler.IsEmpty());
EXPECT_EQ(2, observer->updates_count()); EXPECT_EQ(2, observer->updates_count());
scoped_ptr<WorkerStoragePartition> partition(new WorkerStoragePartition( scoped_ptr<WorkerStoragePartition> partition(new WorkerStoragePartition(
...@@ -417,9 +437,8 @@ TEST_F(DevToolsManagerTest, TestObserver) { ...@@ -417,9 +437,8 @@ TEST_F(DevToolsManagerTest, TestObserver) {
1, 1, shared_worker); 1, 1, shared_worker);
contents()->NavigateAndCommit(url2); contents()->NavigateAndCommit(url2);
RunMessageLoop();
EXPECT_EQ(3, observer->updates_count()); EXPECT_EQ(3, observer->updates_count());
EXPECT_EQ(2u, observer->hosts().size()); ASSERT_EQ(2u, observer->hosts().size());
EXPECT_EQ(contents(), observer->hosts()[0]->GetWebContents()); EXPECT_EQ(contents(), observer->hosts()[0]->GetWebContents());
EXPECT_EQ(url2.spec(), observer->hosts()[0]->GetURL().spec()); EXPECT_EQ(url2.spec(), observer->hosts()[0]->GetURL().spec());
EXPECT_EQ(DevToolsAgentHost::TYPE_SHARED_WORKER, EXPECT_EQ(DevToolsAgentHost::TYPE_SHARED_WORKER,
...@@ -427,21 +446,21 @@ TEST_F(DevToolsManagerTest, TestObserver) { ...@@ -427,21 +446,21 @@ TEST_F(DevToolsManagerTest, TestObserver) {
EXPECT_EQ(shared_worker_url.spec(), observer->hosts()[1]->GetURL().spec()); EXPECT_EQ(shared_worker_url.spec(), observer->hosts()[1]->GetURL().spec());
EmbeddedWorkerDevToolsManager::GetInstance()->WorkerDestroyed(1, 1); EmbeddedWorkerDevToolsManager::GetInstance()->WorkerDestroyed(1, 1);
RunMessageLoop(); scheduler.Run();
EXPECT_EQ(4, observer->updates_count()); EXPECT_EQ(4, observer->updates_count());
EXPECT_EQ(1u, observer->hosts().size()); ASSERT_EQ(1u, observer->hosts().size());
EXPECT_EQ(contents(), observer->hosts()[0]->GetWebContents()); EXPECT_EQ(contents(), observer->hosts()[0]->GetWebContents());
EXPECT_EQ(url2.spec(), observer->hosts()[0]->GetURL().spec()); EXPECT_EQ(url2.spec(), observer->hosts()[0]->GetURL().spec());
base::MessageLoop::current()->PostDelayedTask(
FROM_HERE,
base::MessageLoop::QuitClosure(),
base::TimeDelta::FromMilliseconds(250));
base::MessageLoop::current()->Run();
// Check there were no extra updates. // Check there were no extra updates.
scheduler.Run();
EXPECT_TRUE(scheduler.IsEmpty());
EXPECT_EQ(4, observer->updates_count()); EXPECT_EQ(4, observer->updates_count());
manager->RemoveObserver(observer.get()); manager->RemoveObserver(observer.get());
EXPECT_TRUE(scheduler.IsEmpty());
manager->SetSchedulerForTest(DevToolsManager::Scheduler());
} }
} // namespace content } // namespace content
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