Add TestBrowserThreadBundle into RenderViewHostTestHarness. Kill some unnecessary real threads.

content/browser/browser_thread_impl.cc

 // Copyright (c) 2012 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.
 
 #include "content/browser/browser_thread_impl.h"
 
 #include <string>
 
 #include "base/atomicops.h"
 #include "base/bind.h"
@@ skipping 52 matching lines @@
 }  // namespace
 
 BrowserThreadImpl::BrowserThreadImpl(ID identifier)
     : Thread(g_browser_thread_names[identifier]),
       identifier_(identifier) {
   Initialize();
 }
 
 BrowserThreadImpl::BrowserThreadImpl(ID identifier,
                                      base::MessageLoop* message_loop)
-    : Thread(message_loop->thread_name().c_str()), identifier_(identifier) {
-  set_message_loop(message_loop);
+    : Thread(message_loop ? message_loop->thread_name().c_str() :

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

it seems odd using this constructor and passing in a NULL message loop. i
understand you're doing this to slightly make things easier in
TestBrowserThreadBundle's constructor. it seems that instead that one could just
use scoped pointers and call the different constructors manually depending on
the flags

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

I considered that initially, but I think this is cleaner.

Why make the caller have to change the code used to instantiate a BrowserThread
when they want to support 2 different configurations? Also, if these were
overloaded functions rather than constructors, I would want to make one just
call the other. We only have this weird duplicated code issue because C++98
doesn't have delegated constructor like Java and C++11.

So to me, the semantics of making this understand NULL feels like the right API
reflection of the configuration options.

There's also the small implied cost of N dynamic allocations per TEST_F with
associated heap fragmentation, but that's probably ignorable.

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

I really don't like having two ways of doing the same thing. In this case, it
means that someone who's trying to use this constructor with a MessageLoop, but
for some error condition passes a NULL ML, would now not get a crash and find
out right away.

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

I'm trying to imagine a situation when someone could cause a NULL MessageLoop
and am having a hard time seeing it since this api is only used in tests.

In either case, I can't really see a cost to either API.  I've moved
TestBrowserThreadBundle to dynamically allocate things.

+             g_browser_thread_names[identifier]),
+      identifier_(identifier) {
+  if (message_loop)
+    set_message_loop(message_loop);
   Initialize();
 }
 
 // static
 void BrowserThreadImpl::ShutdownThreadPool() {
   // The goal is to make it impossible for chrome to 'infinite loop' during
   // shutdown, but to reasonably expect that all BLOCKING_SHUTDOWN tasks queued
   // during shutdown get run. There's nothing particularly scientific about the
   // number chosen.
   const int kMaxNewShutdownBlockingTasks = 1000;
@@ skipping 392 matching lines @@
   AtomicWord* storage = reinterpret_cast<AtomicWord*>(
       &globals.thread_delegates[identifier]);
   AtomicWord old_pointer = base::subtle::NoBarrier_AtomicExchange(
       storage, reinterpret_cast<AtomicWord>(delegate));
 
   // This catches registration when previously registered.
   DCHECK(!delegate || !old_pointer);
 }
 
 }  // namespace content