Adding task runner/ message loop to tests that use IsolateHolder.

content/test/test_blink_web_unit_test_support.cc

Index: content/test/test_blink_web_unit_test_support.cc
diff --git a/content/test/test_blink_web_unit_test_support.cc b/content/test/test_blink_web_unit_test_support.cc
index 7405be6f417d040cb42b0292c52396f86b370043..fd333fb8c9559b597084684441881c8d63db1673 100644
--- a/content/test/test_blink_web_unit_test_support.cc
+++ b/content/test/test_blink_web_unit_test_support.cc
@@ -7,8 +7,11 @@
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
+#include "base/message_loop/message_loop.h"

[Sami, 2015/05/05 13:55:54]

Do you need this for anything?

[ssid, 2015/05/05 16:00:06]

Done.

 #include "base/path_service.h"
+#include "base/single_thread_task_runner.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/thread_task_runner_handle.h"
 #include "components/scheduler/renderer/renderer_scheduler.h"
 #include "components/scheduler/renderer/webthread_impl_for_renderer_scheduler.h"
 #include "content/test/mock_webclipboard_impl.h"
@@ -41,6 +44,37 @@
 #include "gin/v8_initializer.h"
 #endif
 
+namespace {
+
+class DummyTaskRunner : public base::SingleThreadTaskRunner {
+ public:
+  DummyTaskRunner() {}
+
+  bool PostDelayedTask(const tracked_objects::Location& from_here,
+                       const base::Closure& task,
+                       base::TimeDelta delay) override {
+    NOTREACHED();
+    return false;
+  }
+
+  bool PostNonNestableDelayedTask(const tracked_objects::Location& from_here,
+                                  const base::Closure& task,
+                                  base::TimeDelta delay) override {
+    NOTREACHED();
+    return false;
+  }
+
+  // Always returns true to avoid triggering DCHECKs.

[Sami, 2015/05/05 13:55:54]

Could you make this do the right thing instead? Some call sites change their
behavior based on what this method returns.

[ssid, 2015/05/05 16:00:06]

Done.

+  bool RunsTasksOnCurrentThread() const override { return true; }
+
+ protected:
+  ~DummyTaskRunner() override {}
+
+  DISALLOW_COPY_AND_ASSIGN(DummyTaskRunner);
+};
+
+}  // namespace
+
 namespace content {
 
 TestBlinkWebUnitTestSupport::TestBlinkWebUnitTestSupport() {
@@ -55,13 +89,31 @@ TestBlinkWebUnitTestSupport::TestBlinkWebUnitTestSupport() {
   gin::V8Initializer::LoadV8Snapshot();
 #endif
 
+  scoped_refptr<base::SingleThreadTaskRunner> dummy_task_runner;
+  scoped_ptr<base::ThreadTaskRunnerHandle> handle;

[Sami, 2015/05/05 13:55:54]

nit: |handle| is a little too nonspecific for a variable name I think.

[ssid, 2015/05/05 16:00:06]

Done.

   if (base::MessageLoopProxy::current()) {
     renderer_scheduler_ = scheduler::RendererScheduler::Create();
     web_thread_.reset(new scheduler::WebThreadImplForRendererScheduler(
         renderer_scheduler_.get()));
+  } else {
+    // 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 handle. The message loop is not created
+    // before this initialization because tests need different kinds of message
+    // loop which is not known upfront and each test suite that uses thread

[Sami, 2015/05/05 13:55:54]

nit: "posted to this handle" => "posted to this task runner"

The final sentence is a little hard to read. How about:

The message loop is not created before this initialization because some tests
need specific kinds of message loops, and their types are not known upfront.
Some tests also create their own thread bundles and message loops, and doing the
same in TestBlinkWebUnitTestSupport would introduce a conflict.

[ssid, 2015/05/05 16:00:06]

Done.

+    // bundle or message loop initializes its own.
+    dummy_task_runner = make_scoped_refptr(new DummyTaskRunner());
+    handle.reset(new base::ThreadTaskRunnerHandle(dummy_task_runner));
   }
 
   blink::initialize(this);
+
+  if (dummy_task_runner.get()) {

[Sami, 2015/05/05 13:55:54]

There's no need to reset the task explicitly is there? Please mention in the
comment though that this task runner will only exist within the scope of this
function.

[Primiano Tucci (use gerrit), 2015/05/05 14:26:51]

Nit: just if (dummy_task_runner), scoped_refptr is already boolean-testable.

On 2015/05/05 13:55:54, Sami wrote:
UH? I think he actually needs to reset the TLS slot (by resetting
dummy_task_runner_handle) and to destroy the dummy_task_runner itself, right?

[Sami, 2015/05/05 14:35:46]

I meant the destructors of both variables would automatically do that when they
go out of scope of this function. I don't see any harm in leaving the task
runner around for a few more lines of initialization.

[ssid, 2015/05/05 16:00:06]

Done.

+    // The dummy task runner is destroyed after the initialization is done.
+    handle.reset();
+    dummy_task_runner = nullptr;
+  }
+
   blink::setLayoutTestMode(true);
   blink::WebSecurityPolicy::registerURLSchemeAsLocal(
       blink::WebString::fromUTF8("test-shell-resource"));