Add TestBrowserThreadBundle into RenderViewHostTestHarness. Kill some unnecessary real threads.

content/public/test/test_browser_thread_bundle.h

+// Copyright (c) 2013 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.
+
+// TestBrowserThreadBundle is a convenience class for creating a set of
+// TestBrowserThreads in unit tests.  For most tests, it is sufficient to
+// just instantiate the TestBrowserThreadBundle as a member variable.
+//
+// By default, all of the created TestBrowserThreads will be backed by a single
+// shared MessageLoop. If a test truly needs separate threads, it can do
+// so by passing the appropriate combination of RealThreadsMask values during
+// the TestBrowserThreadBundle construction.
+//
+// The TestBrowserThreadBundle will attempt to drain the MessageLoop on
+// destruction. If a test needs to drain currently enqueued tasks mid-test,
+// it should call content::RunAllPendingInMessageLoop() or use base::RunLoop
+// (eg., base::RunLoop().RunUntilIdle()) as appropriate.

[jam, 2013/06/05 17:01:39]

I think saying "as appropriate" won't be clear. i.e. almost no developers
reading this would know the difference off the top of their mind, and picking
the wrong one would mean that they get hard to track down hangs on some trybot.

what is the cost of just saying content::RunAllPendingInMessageLoop() only?

[awong, 2013/06/05 17:54:38]

My confusion is as follows:
  - This CL uses base::RunLoop directly, which contradicts the comment.
  - This seems only useful for browser_test, and not unit_tests.
  - Comment in test_util.h says AddPreRunMessageLoopHook is going away. 
  - base::MessageLoop tells you to use RunLoop.

Thus to me, it looks like there are 3 methods of pumping a MessageLoop 
  (1) base::RunLoop().RunUntilIdle()
  (2) MessageLoop::RunUntilIdle()
  (3) content::RunAllPendingInMessageLoop()

On first brush you see that (1) is used heavily by the CL that introduced this
comment contradicting its own advice, (2) is deprecated, and (3) has an unknown
set of dependencies because of the hooks.

For unittests, I'm usually quite aware of the tasks that have been posted and so
I would avoid (3) because it increases my test's dependencies.  

Recommending a more permissive/magic API means the test writes can get lazy
about understanding their dependencies making for flakier tests, etc.

If that still sounds unconvincing, I'm happy to strike base::RunLoop() from the
comment.

In that case, is the recommendation to always prefer
content::RunAllPendingInMessageLoop() even in unit_tests, and should I amend
this CL?

[awong, 2013/06/05 21:16:56]

Comment amended say RunLoop for unit tests and
content::RunAllPendingInMessageLoop() for browser tests.

+//
+// Some tests using the IO thread expect a MessageLoopForIO. Passing
+// USE_IO_LOOP_FOR_UI will use a MessageLoopForIO for the main MessageLoop.
+// Most of the time, this avoids needing to use a REAL_IO_THREAD.
+
+#ifndef CONTENT_PUBLIC_TEST_TEST_BROWSER_THREAD_TEST_BUNDLE_H_
+#define CONTENT_PUBLIC_TEST_TEST_BROWSER_THREAD_TEST_BUNDLE_H_
+
+#include "base/memory/scoped_ptr.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace base {
+class MessageLoop;
+}  // namespace base
+
+namespace content {
+
+class TestBrowserThread;
+
+class TestBrowserThreadBundle {
+ public:
+  // Used to specify the type of MessageLoop that backs the UI thread, and
+  // which of the named BrowserThreads should be backed by a real
+  // threads. The UI thread is always the main thread in a unit test.
+  enum Options {
+    DEFAULT = 0x00,
+    IO_MAINLOOP = 0x01,
+    REAL_DB_THREAD = 0x02,
+    REAL_WEBKIT_DEPRECATED_THREAD = 0x04,
+    REAL_FILE_THREAD = 0x08,
+    REAL_FILE_USER_BLOCKING_THREAD = 0x10,
+    REAL_PROCESS_LAUNCHER_THREAD = 0x20,
+    REAL_CACHE_THREAD = 0x40,
+    REAL_IO_THREAD = 0x80,
+  };
+
+  TestBrowserThreadBundle();
+  explicit TestBrowserThreadBundle(int options);
+
+  ~TestBrowserThreadBundle();
+
+ private:
+  void Init(int options);
+
+  scoped_ptr<base::MessageLoop> message_loop_;
+  scoped_ptr<TestBrowserThread> ui_thread_;
+  scoped_ptr<TestBrowserThread> db_thread_;
+  scoped_ptr<TestBrowserThread> webkit_deprecated_thread_;
+  scoped_ptr<TestBrowserThread> file_thread_;
+  scoped_ptr<TestBrowserThread> file_user_blocking_thread_;
+  scoped_ptr<TestBrowserThread> process_launcher_thread_;
+  scoped_ptr<TestBrowserThread> cache_thread_;
+  scoped_ptr<TestBrowserThread> io_thread_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestBrowserThreadBundle);
+};
+
+}  // namespace content
+
+#endif /* CONTENT_PUBLIC_TEST_TEST_BROWSER_THREAD_TEST_BUNDLE_H_ */