content: Add an overridable task runner to ChildThreadImpl

content/public/child/child_thread.h

 // 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 CONTENT_PUBLIC_CHILD_CHILD_THREAD_H_
 #define CONTENT_PUBLIC_CHILD_CHILD_THREAD_H_
 
+#include "base/memory/ref_counted.h"
 #include "content/common/content_export.h"
 #include "ipc/ipc_sender.h"
 
 #if defined(OS_WIN)
 #include <windows.h>
 #endif
 
+namespace base {
+class SingleThreadTaskRunner;
+}
+
 namespace content {
 
 class CONTENT_EXPORT ChildThread : public IPC::Sender {
  public:
   // Returns the one child thread for this process.  Note that this can only be
   // accessed when running on the child thread itself.
   static ChildThread* Get();
 
   ~ChildThread() override {}
 
+  // Returns the task runner which should be used to post tasks to be executed
+  // on this child thread. Note that this may be different from the underlying
+  // message loop for this thread, and should therefore be used instead of
+  // going to the message loop directly.

[jabdelmalek, 2015/03/06 18:06:13]

this seems like quite a large change, as there are lots of places that get the
current message loop in the renderer. i.e. see
https://code.google.com/p/chromium/codesearch#search/&q=%22MessageLoop::curre...

1) are you planning on updating all of them?
2) what would enforce that people use this method instead of getting the raw
MessageLoop?

[Sami, 2015/03/06 18:41:46]

Yes, it's unfortunate that a lot of code is hard coded to use the global message
loop. The other popular way to do that is ThreadTaskRunnerHandle::Get(). We're
incrementally migrating code to use and pass around the correct the task runner
explicitly. Longer term we'd like to delete the global getters completely.
A presubmit check for content/ is the best way I can think of. I don't think it
can be fully automated because it's not obvious on which thread a given piece of
code is running.

[Ryan Sleevi, 2015/03/06 19:00:31]

Then you really should have been having this design discussion on chromium-dev.

I won't pile on to this review, but I think that's a HUGE statement and, while
admirable, definitely deserves discussion of the trade-offs and whether that
goal is good (from a code health perspective)

Having more code use ThreadTaskRunnerHandle::Get() and is something I ding
everyone on during reviews. But I would encourage you to be public about that
discussion, rather than subtlely through CLs like this. If I've missed the
discussion, apologies, but you're fundamentally talking about a change to the
API guarantees of //base, even if it may not be obvious as such.

+  virtual scoped_refptr<base::SingleThreadTaskRunner> GetTaskRunner() = 0;

[jabdelmalek, 2015/03/06 18:06:13]

why is this in the public interface, if only content-internal code uses it? this
should be on ChildThreadImpl only. RenderThread can still expose it if it's used
by code outside of content, and it can go through ChildThreadImpl

[Sami, 2015/03/06 18:41:46]

Good point, I've now moved GetTaskRunner() back to RenderThread.

+
 #if defined(OS_WIN)
   // Request that the given font be loaded by the browser so it's cached by the
   // OS. Please see ChildProcessHost::PreCacheFont for details.
   virtual void PreCacheFont(const LOGFONT& log_font) = 0;
 
   // Release cached font.
   virtual void ReleaseCachedFonts() = 0;
 #endif
 };
 
 }  // namespace content
 
 #endif  // CONTENT_PUBLIC_CHILD_CHILD_THREAD_H_