From 087644f1eab41927823a1a2fc2df08bd4e10fe18 Mon Sep 17 00:00:00 2001 From: skyostil Date: Wed, 3 Jun 2015 14:20:58 -0700 Subject: [PATCH] scheduler: Always create a real scheduler in unit tests Previously we would create a dummy scheduler in any test which uses TestBlinkWebUnitTestSupport without first initializing a message loop. This causes problems because the dummy scheduler ignores all tasks it is given. This patch makes the tests more realistic by always creating a real renderer scheduler regardless of whether we have a message loop or not. This is achieved by lazily binding the scheduler to the message loop the first time it is needed. Longer term we would like to refactor these test suites to ensure Blink always has a valid message loop when it is initialized, but this will involve rewiring several tests. BUG=463143,495659 Review URL: https://codereview.chromium.org/1152623008 Cr-Commit-Position: refs/heads/master@{#332685} --- components/scheduler/BUILD.gn | 11 +++ components/scheduler/scheduler.gyp | 11 +++ components/scheduler/scheduler.gypi | 4 ++ components/scheduler/test/DEPS | 3 + ...zy_scheduler_message_loop_delegate_for_tests.cc | 82 ++++++++++++++++++++++ ...azy_scheduler_message_loop_delegate_for_tests.h | 55 +++++++++++++++ content/content_tests.gypi | 1 + content/test/BUILD.gn | 1 + content/test/DEPS | 1 + content/test/test_blink_web_unit_test_support.cc | 44 ++---------- 10 files changed, 176 insertions(+), 37 deletions(-) create mode 100644 components/scheduler/test/DEPS create mode 100644 components/scheduler/test/lazy_scheduler_message_loop_delegate_for_tests.cc create mode 100644 components/scheduler/test/lazy_scheduler_message_loop_delegate_for_tests.h diff --git a/components/scheduler/BUILD.gn b/components/scheduler/BUILD.gn index f55b542fe95c..587516f3ecb5 100644 --- a/components/scheduler/BUILD.gn +++ b/components/scheduler/BUILD.gn @@ -60,3 +60,14 @@ source_set("unit_tests") { "//testing/gtest", ] } + +# GYP version: components/scheduler.gypi:scheduler_test_support +static_library("test_support") { + sources = rebase_path(scheduler_gypi_values.scheduler_test_support_sources, + ".", + "//components/scheduler") + + deps = [ + "//base", + ] +} diff --git a/components/scheduler/scheduler.gyp b/components/scheduler/scheduler.gyp index e77be8530d62..cffc0726e6e4 100644 --- a/components/scheduler/scheduler.gyp +++ b/components/scheduler/scheduler.gyp @@ -50,5 +50,16 @@ '../../third_party/WebKit/public/blink.gyp:blink', ], }, + { + # GN version: //components/scheduler:test_support + 'target_name': 'scheduler_test_support', + 'type': 'static_library', + 'include_dirs': [ + '../..', + ], + 'sources': [ + '<@(scheduler_test_support_sources)', + ], + }, ], } diff --git a/components/scheduler/scheduler.gypi b/components/scheduler/scheduler.gypi index 684688c6f43a..813272547229 100644 --- a/components/scheduler/scheduler.gypi +++ b/components/scheduler/scheduler.gypi @@ -58,5 +58,9 @@ 'renderer/webthread_impl_for_renderer_scheduler.h', 'scheduler_export.h', ], + 'scheduler_test_support_sources': [ + 'test/lazy_scheduler_message_loop_delegate_for_tests.cc', + 'test/lazy_scheduler_message_loop_delegate_for_tests.h', + ], }, } diff --git a/components/scheduler/test/DEPS b/components/scheduler/test/DEPS new file mode 100644 index 000000000000..7fba883fe3d4 --- /dev/null +++ b/components/scheduler/test/DEPS @@ -0,0 +1,3 @@ +include_rules = [ + "+components/scheduler/child", +] diff --git a/components/scheduler/test/lazy_scheduler_message_loop_delegate_for_tests.cc b/components/scheduler/test/lazy_scheduler_message_loop_delegate_for_tests.cc new file mode 100644 index 000000000000..1b2ee5b1a66b --- /dev/null +++ b/components/scheduler/test/lazy_scheduler_message_loop_delegate_for_tests.cc @@ -0,0 +1,82 @@ +// Copyright 2015 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "components/scheduler/test/lazy_scheduler_message_loop_delegate_for_tests.h" + +namespace scheduler { + +// static +scoped_refptr +LazySchedulerMessageLoopDelegateForTests::Create() { + return make_scoped_refptr(new LazySchedulerMessageLoopDelegateForTests()); +} + +LazySchedulerMessageLoopDelegateForTests:: + LazySchedulerMessageLoopDelegateForTests() + : thread_id_(base::PlatformThread::CurrentId()) { +} + +LazySchedulerMessageLoopDelegateForTests:: + ~LazySchedulerMessageLoopDelegateForTests() { +} + +base::MessageLoop* LazySchedulerMessageLoopDelegateForTests::EnsureMessageLoop() + const { + base::MessageLoop* message_loop = base::MessageLoop::current(); + DCHECK(message_loop); + for (auto& observer : pending_observers_) { + message_loop->AddTaskObserver(observer); + } + pending_observers_.clear(); + return message_loop; +} + +bool LazySchedulerMessageLoopDelegateForTests::HasMessageLoop() const { + return base::MessageLoop::current() != nullptr; +} + +bool LazySchedulerMessageLoopDelegateForTests::PostDelayedTask( + const tracked_objects::Location& from_here, + const base::Closure& task, + base::TimeDelta delay) { + return EnsureMessageLoop()->task_runner()->PostDelayedTask(from_here, task, + delay); +} + +bool LazySchedulerMessageLoopDelegateForTests::PostNonNestableDelayedTask( + const tracked_objects::Location& from_here, + const base::Closure& task, + base::TimeDelta delay) { + return EnsureMessageLoop()->task_runner()->PostNonNestableDelayedTask( + from_here, task, delay); +} + +bool LazySchedulerMessageLoopDelegateForTests::RunsTasksOnCurrentThread() + const { + return thread_id_ == base::PlatformThread::CurrentId(); +} + +bool LazySchedulerMessageLoopDelegateForTests::IsNested() const { + return EnsureMessageLoop()->IsNested(); +} + +void LazySchedulerMessageLoopDelegateForTests::AddTaskObserver( + base::MessageLoop::TaskObserver* task_observer) { + if (!HasMessageLoop()) { + pending_observers_.insert(task_observer); + return; + } + EnsureMessageLoop()->AddTaskObserver(task_observer); +} + +void LazySchedulerMessageLoopDelegateForTests::RemoveTaskObserver( + base::MessageLoop::TaskObserver* task_observer) { + if (!HasMessageLoop()) { + pending_observers_.erase(task_observer); + return; + } + EnsureMessageLoop()->RemoveTaskObserver(task_observer); +} + +} // namespace scheduler diff --git a/components/scheduler/test/lazy_scheduler_message_loop_delegate_for_tests.h b/components/scheduler/test/lazy_scheduler_message_loop_delegate_for_tests.h new file mode 100644 index 000000000000..146eb9010520 --- /dev/null +++ b/components/scheduler/test/lazy_scheduler_message_loop_delegate_for_tests.h @@ -0,0 +1,55 @@ +// Copyright 2015 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef COMPONENTS_SCHEDULER_TEST_LAZY_SCHEDULER_MESSAGE_LOOP_DELEGATE_FOR_TESTS_H_ +#define COMPONENTS_SCHEDULER_TEST_LAZY_SCHEDULER_MESSAGE_LOOP_DELEGATE_FOR_TESTS_H_ + +#include "base/message_loop/message_loop.h" +#include "components/scheduler/child/nestable_single_thread_task_runner.h" + +namespace scheduler { + +// This class connects the scheduler to a MessageLoop, but unlike +// SchedulerMessageLoopDelegate it allows the message loop to be created lazily +// after the scheduler has been brought up. This is needed in testing scenarios +// where Blink is initialized before a MessageLoop has been created. +// +// TODO(skyostil): Fix the relevant test suites and remove this class +// (crbug.com/495659). +class LazySchedulerMessageLoopDelegateForTests + : public NestableSingleThreadTaskRunner { + public: + static scoped_refptr Create(); + + // NestableSingleThreadTaskRunner implementation + bool PostDelayedTask(const tracked_objects::Location& from_here, + const base::Closure& task, + base::TimeDelta delay) override; + bool PostNonNestableDelayedTask(const tracked_objects::Location& from_here, + const base::Closure& task, + base::TimeDelta delay) override; + bool RunsTasksOnCurrentThread() const override; + bool IsNested() const override; + void AddTaskObserver(base::MessageLoop::TaskObserver* task_observer) override; + void RemoveTaskObserver( + base::MessageLoop::TaskObserver* task_observer) override; + + private: + LazySchedulerMessageLoopDelegateForTests(); + ~LazySchedulerMessageLoopDelegateForTests() override; + + bool HasMessageLoop() const; + base::MessageLoop* EnsureMessageLoop() const; + + base::PlatformThreadId thread_id_; + // Task observers which have not yet been registered to a message loop. Not + // owned. + mutable base::hash_set pending_observers_; + + DISALLOW_COPY_AND_ASSIGN(LazySchedulerMessageLoopDelegateForTests); +}; + +} // namespace scheduler + +#endif // COMPONENTS_SCHEDULER_TEST_LAZY_SCHEDULER_MESSAGE_LOOP_DELEGATE_FOR_TESTS_H_ diff --git a/content/content_tests.gypi b/content/content_tests.gypi index 0abee4bc9d3a..6f791bcb66d2 100644 --- a/content/content_tests.gypi +++ b/content/content_tests.gypi @@ -872,6 +872,7 @@ '../cc/cc.gyp:cc', '../cc/cc_tests.gyp:cc_test_support', '../components/scheduler/scheduler.gyp:scheduler', + '../components/scheduler/scheduler.gyp:scheduler_test_support', '../gpu/blink/gpu_blink.gyp:gpu_blink', '../ipc/mojo/ipc_mojo.gyp:*', '../media/blink/media_blink.gyp:media_blink', diff --git a/content/test/BUILD.gn b/content/test/BUILD.gn index 7a4377a1a601..848dea1b87bd 100644 --- a/content/test/BUILD.gn +++ b/content/test/BUILD.gn @@ -56,6 +56,7 @@ source_set("test_support") { public_deps += [ "//third_party/WebKit/public:blink" ] deps += [ "//components/scheduler:scheduler", + "//components/scheduler:test_support", "//content/browser/speech/proto", "//content/public/child", "//content/gpu", diff --git a/content/test/DEPS b/content/test/DEPS index 760b35864af6..e1d0d50953c5 100644 --- a/content/test/DEPS +++ b/content/test/DEPS @@ -2,6 +2,7 @@ include_rules = [ # Allow inclusion of specific components that we depend on. We may only # depend on components which we share with the mojo html_viewer. "+components/scheduler/renderer", + "+components/scheduler/test", "+cc/blink", "+chromeos/audio", # For WebRTC tests. diff --git a/content/test/test_blink_web_unit_test_support.cc b/content/test/test_blink_web_unit_test_support.cc index 4dbce4054cc9..05a6b9bcce2f 100644 --- a/content/test/test_blink_web_unit_test_support.cc +++ b/content/test/test_blink_web_unit_test_support.cc @@ -12,8 +12,9 @@ #include "base/strings/utf_string_conversions.h" #include "base/thread_task_runner_handle.h" #include "base/threading/platform_thread.h" -#include "components/scheduler/renderer/renderer_scheduler.h" +#include "components/scheduler/renderer/renderer_scheduler_impl.h" #include "components/scheduler/renderer/webthread_impl_for_renderer_scheduler.h" +#include "components/scheduler/test/lazy_scheduler_message_loop_delegate_for_tests.h" #include "content/test/mock_webclipboard_impl.h" #include "content/test/web_gesture_curve_mock.h" #include "content/test/web_layer_tree_view_impl_for_testing.h" @@ -24,7 +25,6 @@ #include "storage/browser/database/vfs_backend.h" #include "third_party/WebKit/public/platform/WebData.h" #include "third_party/WebKit/public/platform/WebFileSystem.h" -#include "third_party/WebKit/public/platform/WebScheduler.h" #include "third_party/WebKit/public/platform/WebStorageArea.h" #include "third_party/WebKit/public/platform/WebStorageNamespace.h" #include "third_party/WebKit/public/platform/WebString.h" @@ -77,35 +77,6 @@ class DummyTaskRunner : public base::SingleThreadTaskRunner { DISALLOW_COPY_AND_ASSIGN(DummyTaskRunner); }; -class DummyWebThread : public blink::WebThread { - public: - DummyWebThread() - : thread_id_(base::PlatformThread::CurrentId()), - m_dummyScheduler(new blink::WebScheduler()) {} - - virtual void postTask(const blink::WebTraceLocation&, Task*) { NOTREACHED(); } - - virtual void postDelayedTask(const blink::WebTraceLocation&, - Task*, - long long delayMs) { - NOTREACHED(); - } - - virtual bool isCurrentThread() const { - return thread_id_ == base::PlatformThread::CurrentId(); - } - - virtual blink::WebScheduler* scheduler() const { - return m_dummyScheduler.get(); - } - - private: - base::PlatformThreadId thread_id_; - scoped_ptr m_dummyScheduler; - - DISALLOW_COPY_AND_ASSIGN(DummyWebThread); -}; - } // namespace namespace content { @@ -124,11 +95,7 @@ TestBlinkWebUnitTestSupport::TestBlinkWebUnitTestSupport() { scoped_refptr dummy_task_runner; scoped_ptr dummy_task_runner_handle; - if (base::MessageLoopProxy::current()) { - renderer_scheduler_ = scheduler::RendererScheduler::Create(); - web_thread_.reset(new scheduler::WebThreadImplForRendererScheduler( - renderer_scheduler_.get())); - } else { + if (!base::MessageLoopProxy::current()) { // Dummy task runner is initialized here because the blink::initialize // creates IsolateHolder which needs the current task runner handle. There // should be no task posted to this task runner. The message loop is not @@ -139,8 +106,11 @@ TestBlinkWebUnitTestSupport::TestBlinkWebUnitTestSupport() { dummy_task_runner = make_scoped_refptr(new DummyTaskRunner()); dummy_task_runner_handle.reset( new base::ThreadTaskRunnerHandle(dummy_task_runner)); - web_thread_.reset(new DummyWebThread()); } + renderer_scheduler_ = make_scoped_ptr(new scheduler::RendererSchedulerImpl( + scheduler::LazySchedulerMessageLoopDelegateForTests::Create())); + web_thread_.reset(new scheduler::WebThreadImplForRendererScheduler( + renderer_scheduler_.get())); blink::initialize(this); blink::setLayoutTestMode(true); -- 2.11.4.GIT