services/ui: Allow injecting the IO thread task runner for gpu channel.

services/ui/public/cpp/gpu_service.cc

Index: services/ui/public/cpp/gpu_service.cc
diff --git a/services/ui/public/cpp/gpu_service.cc b/services/ui/public/cpp/gpu_service.cc
index 6c70440f940189eca61194151bf4c52152597284..5adeb60423ff7dcf878bc21b15dcc976ca201ac2 100644
--- a/services/ui/public/cpp/gpu_service.cc
+++ b/services/ui/public/cpp/gpu_service.cc
@@ -35,15 +35,11 @@ GpuService::GpuService(shell::Connector* connector)
       connector_(connector),
       shutdown_event_(base::WaitableEvent::ResetPolicy::AUTOMATIC,
                       base::WaitableEvent::InitialState::NOT_SIGNALED),
-      io_thread_("GPUIOThread"),
       gpu_memory_buffer_manager_(new MojoGpuMemoryBufferManager),
       is_establishing_(false),
       establishing_condition_(&lock_) {
   DCHECK(main_task_runner_);
   DCHECK(connector_);
-  base::Thread::Options thread_options(base::MessageLoop::TYPE_IO, 0);
-  thread_options.priority = base::ThreadPriority::NORMAL;
-  CHECK(io_thread_.StartWithOptions(thread_options));
 }
 
 GpuService::~GpuService() {
@@ -53,11 +49,19 @@ GpuService::~GpuService() {
 }
 
 // static
-std::unique_ptr<GpuService> GpuService::Initialize(
+std::unique_ptr<GpuService> GpuService::Create(
     shell::Connector* connector) {
   return base::WrapUnique(new GpuService(connector));
 }
 
+void GpuService::SetIOThreadTaskRunner(
+    scoped_refptr<base::SingleThreadTaskRunner> io_task_runner) {
+  DCHECK(!io_task_runner_)

[piman, 2016/08/22 19:42:03]

Would it make sense instead to pass the task runner explicitly to Create, and
creating the thread in the constructor if the task runner is null?

In particular, I'm concerned that GetIOThreadTaskRunner can be called on any
thread, making the logic there unsafe.

[sadrul, 2016/08/22 20:05:54]

Ah, interesting. Yes, Fady brought up the issue earlier. It's not entirely
straight forward because of how GpuService is created for the chrome browser
process (see
https://codereview.chromium.org/2249413004/diff/1/services/ui/public/cpp/gpu_...).
Would it make more sense if I create this thread in EstablishGpuChannel[Sync]
instead?

[piman, 2016/08/22 20:31:50]

I feel like plumbing the task runner through WindowManagerConnection is easy
enough, and feels (to me) cleaner than the Law-of-Demeter-cringy logic in
ChromeBrowserMainExtraPartsViews::MojoShellConnectionStarted - but I guess
opinions can differ on that.


I think deferring creating the thread to EstablishGpuChannel* would work, but
then I would prefer making GetIOThreadTaskRunner private (and even making the
GpuChannelHostFactory inheritance private, but that's iffy by the style guide) -
making it clear that it's only called by the GpuChannelHost, which by
construction would only be after the thread is created.

But in general, I prefer "obviously correct" code rather than defending with
documentation.

[sadrul, 2016/08/22 21:20:38]

Agreed. And contrary to popular meme, obviously correct is the best kind of
correct. Done.

+      << "The task runner cannot be changed once set.";
+  DCHECK(io_task_runner);
+  io_task_runner_ = std::move(io_task_runner);
+}
+
 void GpuService::EstablishGpuChannel(
     const gpu::GpuChannelEstablishedCallback& callback) {
   base::AutoLock auto_lock(lock_);
@@ -205,7 +209,14 @@ bool GpuService::IsMainThread() {
 
 scoped_refptr<base::SingleThreadTaskRunner>
 GpuService::GetIOThreadTaskRunner() {
-  return io_thread_.task_runner();
+  if (!io_task_runner_) {
+    io_thread_.reset(new base::Thread("GPUIOThread"));
+    base::Thread::Options thread_options(base::MessageLoop::TYPE_IO, 0);
+    thread_options.priority = base::ThreadPriority::NORMAL;
+    CHECK(io_thread_->StartWithOptions(thread_options));
+    io_task_runner_ = io_thread_->task_runner();
+  }
+  return io_task_runner_;
 }
 
 std::unique_ptr<base::SharedMemory> GpuService::AllocateSharedMemory(