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, each of the created TestBrowserThreads will be backed by
+// all of the created TestBrowserThreads will be backed by a single

[jam, 2013/05/31 17:29:25]

nit: fix comment

[awong, 2013/05/31 20:57:17]

Done.

+// shared MessageLoop. If a test truly needs separate threads, it can do
+// so by passing the apprpriate combination of RealThreadsMask values during

[jam, 2013/05/31 17:29:25]

nit: spelling

[awong, 2013/05/31 20:57:17]

Done.

+// the TestBrowserThreadBundle construction.
+//
+// The TestBrowserThreadBundle will attempt to drain the MessageLoop on
+// destruction. If a test needs to drain currently enqueued tasks, the best
+// pattern is to instantiate base::RunLoop and call the appropriate methods
+// the RunLoop (eg., base::RunLoop().RunUntilIdle()). Very rarely should a

[jam, 2013/05/31 17:29:25]

can you elaborate more on this? my first impression is that this will make tests
slower.

in general, a test will send a task to a thread, and then flush that thread. why
should it wait until it's idle instead? that would do more work (i.e. in case
the task that it posted then posted more tasks)

[awong, 2013/05/31 20:57:17]

Hmm...I added this to try and get more defined behavior.

In the best case, where there task queue is empty, this call should have minimal
overhead. In the other case, where a test is leaving tasks in the queue during
shutdown, that sounds like an unclean teardown to me.

One consistent issue I've been running into has been unit tests implicitly
"cutting dependencies in their libraries" by not instantiating a particular
BrowserThread thereby causing a PostTask to fail and get dropped. This is
fragile because when someone does end up adding a BrowserThread higher in stack,
bunches of code can get activated causing all sorts of grief.

Similarly, without this, if someone did BrowserThread::PostTask(...) in their
TEST_F without adding an explicit RunLoop() flush, it just wouldn't get run. 
Delayed tasks are even more funky -- I recall MessageLoop does strange dances to
decide if it should leak tasks that are still in the queue when it's destructed.

It seems like a simpler invariant to say that the TestBrowserThreadBundle is
going to try and run whatever you've posted to it before shutting down.

[Jeffrey Yasskin, 2013/05/31 21:11:33]

To elaborate a bit more about the non-destruction case, having tests use
RunUntilIdle by default makes them less fragile. If a test enqueues a task and
then calls "loop->PostTask(loop->QuitClosure()); loop->Run();" (i.e.
"RunPending"), that asserts that the enqueued task doesn't need to wait for
anything asynchronous in order to finish. If the task needs to change in the
future, this test will break and need to be fixed, either to call RunPending
twice or to call RunUntilIdle. Some tests do want to assert that their task
doesn't yield, in which case they should ignore the advice here, but other tests
are correct to default to RunUntilIdle.

Also note that content::RunAllPendingInMessageLoop() uses QuitWhenIdle:
https://code.google.com/p/chromium/codesearch/#chromium/src/content/public/te...

The destructor absolutely needs to use RunUntilIdle because object deletion can
use DeleteSoon onto another BrowserThread, and we don't want that to cause
leaks.

[jam, 2013/05/31 22:53:04]

I think if someone changes code such that posting a task isn't enough (i.e. that
code also needs to wait for another posted task), then it makes sense that tests
for that code would need to be updated. I don't think this is a common scenario,
and so not sure why we should slow down running tests to support it.
speaking of content::RunAllPendingInMessageLoop(), that calls
TestLauncherDelegate::PreRunMessageLoop and
TestLauncherDelegate::PostRunMessageLoop which creates a
views::AcceleratorHandler during running nested message loop. the comment to say
to call base::RunUntilIdle would skip that, which would cause some browser tests
to hang i believe.

[awong, 2013/06/05 00:18:48]

I've updated the comment to also reference content::RunAllPendingInMessageLoop()
and to avoid giving a preference on which method to use.

+// test talk directly to the underlying MessageLoop.
+//
+// Some tests using the IO thread expect a MessageLoopForIO. Currently, the
+// best workaround is to create a REAL_IO_THREAD to get the appropriate mesasge
+// loop. This class could conceivably create a MessageLoopForIO instead of
+// a MessageLoopForUI as the main loop, but there are not enough cases to
+// make it work the API complexity.
+
+#ifndef BROWSER_THREAD_TEST_BUNDLE_H_

[jam, 2013/05/31 17:29:25]

nit: fix guard (i.e. add CONTENT_PUBLIC_TEST_)

[awong, 2013/05/31 20:57:17]

Done.

+#define BROWSER_THREAD_TEST_BUNDLE_H_
+
+#include "base/message_loop.h"
+#include "content/public/test/test_browser_thread.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace content {
+
+class TestBrowserThreadBundle {
+ public:
+  // Used to specifying which named BrowserThreads should be backed by a real
+  // thread rather than main main thread.  The UI thread is always the main
+  // thread in a unit test so it is left out of this enum.
+  enum RealThreadsMask {
+    NO_REAL_THREAD = 0x0,

[jam, 2013/05/31 17:29:25]

nit: i believe you're adding this for the convenience of
RenderViewHostTestHarness's implementation. but it seems that since this class
shouldn't know about RenderViewHostTestHarness, it shouldn't have NO_REAL_THREAD
as a value. otherwise it's a bit odd that there are two ways to start with no
default threads (either the default ctor, or the explict one with this value)

[awong, 2013/05/31 20:57:17]

I could removed this enum and add a DCHECK for a mask of 0.  However, this seems
like an unnecessary input domain restriction for something that could have a
pretty clear behavior.

The fact that it makes RVHTH a little simpler is a side benefit, but to me it
seems sensible to define the 0x0 case since there is a pretty sane behavior.

[jam, 2013/05/31 22:53:04]

ok, since this is personal style, I'll defer to you.

+    REAL_DB_THREAD = 0x01,
+    REAL_FILE_THREAD = 0x2,
+    REAL_FILE_USER_BLOCKING_THREAD = 0x4,
+    REAL_IO_THREAD = 0x8
+  };
+
+  TestBrowserThreadBundle();
+  explicit TestBrowserThreadBundle(int real_threads_mask);
+
+  ~TestBrowserThreadBundle();
+
+  TestBrowserThread* ui_thread() { return &ui_thread_; }
+  TestBrowserThread* db_thread() { return &db_thread_; }
+  TestBrowserThread* file_thread() { return &file_thread_; }
+  TestBrowserThread* file_user_blocking_thread() {
+    return &file_user_blocking_thread_;
+  }
+  TestBrowserThread* io_thread() { return &io_thread_; }
+
+ private:
+  MessageLoopForUI message_loop_;
+  TestBrowserThread ui_thread_;
+  TestBrowserThread db_thread_;
+  TestBrowserThread file_thread_;
+  TestBrowserThread file_user_blocking_thread_;
+  TestBrowserThread io_thread_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestBrowserThreadBundle);
+};
+
+}  // namespace content
+
+#endif /* BROWSER_THREAD_TEST_BUNDLE_H_ */

[jam, 2013/05/31 17:29:25]

nit: please follow the convention for this line

[awong, 2013/05/31 20:57:17]

Done.