Add TestBrowserThreadBundle into RenderViewHostTestHarness. Kill some unnecessary real threads.

Add TestBrowserThreadBundle into RenderViewHostTestHarness. Kill some unnecessary real threads.

This CL creates a new class, TestBrowserThreadBundle, that creates a TestBrowserThread for the most commonly needed BrowserThreads. It also adds this thread bundle into RenderViewHostTestHarness because most tests that use this harness need these threads in order to run.  To support TestBrowserThreadBundle, BrowserThreadImpl's test construction was also modified to understand a NULL message_loop.

Aside from introducing the new class, this CL also removes:
  (1) unnecessary constructors in test
  (2) DISALLOW_COPY_AND_ASSIGNS in test fixtures*
  (3) now redundant TestBrowserThreads from tests
  (4) bad access-level changes for SetUp() and TearDown()
  (5) using declarations that root off the global scope
  (6) uses of MessageLoop's RunUntilIdle() and Quit()
  (7) as many real threads as possible

There are also a changes to MediaCaptureDevicesDispatcher and OneClickSigninHelper that allow unittests to cut dependencies on IO thread activity.  DesktopNotificationServiceTest (and a couple of others) were also made single threaded because the synchronization logic required for a non-flaky test meant the parallelism only really exercised the extra code in the test that forced the serialization.

* DISALLOW_COPY_AND_ASSIGN does not serve much purpose in GTest fixture types. However, using it removes the compile-provided default constructor which forces the fixture writer to provide an empty default constructor. Since GTest recommends the default constructor as the preferred method to do setup for a test (as opposed to the SetUp() method), it's confusing to have bunch of classes with both SetUp() and a default constructor. It's simpler and uses less code to just remove the DISALLOW_COPY_AND_ASSIGN. An alternative would be to use the default constructor for initialization, but this is not possible because of our test harnesses are inheritance based which means it is impossible for a derived fixture to perform initialization before the harness if we use the GTest preferred default constructor initialization pattern.

TBR=avi,battre,ben,benwells,brettw,dbeam,dimich,joi,joth,mad,marja,markusheintz,noelallen,phajdan,rdsmith,satorux,scherkus,sky,stevenjb,stuartmorgan,timsteele
BUG=159193
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=204603

[awong, 2013-05-18 01:20:41]

avi: Since jam@'s out, do you mind doing the overarching review?  It's largely
deletes.

xians: Can you look at MediaCaptureDevicesDispatcher?
akalin: Can you look at OneClickSigninHelper?

[no longer working on chromium, 2013-05-20 08:57:07]

lgtm to the media_capture_devices_dispatcher

[Ilya Sherman, 2013-05-20 21:38:49]

https://codereview.chromium.org/14197014/diff/120001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc (left):

https://codereview.chromium.org/14197014/diff/120001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:361:
DISALLOW_COPY_AND_ASSIGN(AutofillDialogControllerTest);
Why did you remove this?  I see in your CL description "removes problematic
DISALLOW_COPY_AND_ASSIGNS in tests" -- could you explain why these are
problematic?  Thanks.

[awong, 2013-05-20 21:46:07]

On 2013/05/20 21:38:49, Ilya Sherman wrote:
>
https://codereview.chromium.org/14197014/diff/120001/chrome/browser/ui/autofi...
> File chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc (left):
> 
>
https://codereview.chromium.org/14197014/diff/120001/chrome/browser/ui/autofi...
> chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:361:
> DISALLOW_COPY_AND_ASSIGN(AutofillDialogControllerTest);
> Why did you remove this?  I see in your CL description "removes problematic
> DISALLOW_COPY_AND_ASSIGNS in tests" -- could you explain why these are
> problematic?  Thanks.

Sorry, I'll add an explanation to the description.

The TEST_F() macro creates a subclass of the top-level Fixture you create. 
These subclasses need access to the default constructor.  If you add
DISALLOW_COPY_AND_ASSIGN() to the fixture, that implies the need for a default
constructor to also be declared, even if it's empty.  This just adds extra code
for something that isn't adding any real protection anyways.  Furthermore, in
GoogleTest, the constructor and SetUp() are nearly equivalent entry points into
initializing a single TEST_F:

https://code.google.com/p/googletest/wiki/FAQ#Should_I_use_the_constructor/de...

Most tests I saw were declaring both a SetUp() and a constructor which, IMO, is
somewhat of a confusing antipattern since now you have 2 initialization blocks. 
I initially toyed with making everything use the constructor initialization
pattern, but due to the fact that we use inheritance for a test harness
(probably another gtest anti-pattern) we cannot override globals early enough in
the init sequence.  Thus, I tried to make tests normalize on SetUp().

[awong, 2013-05-20 21:57:56]

https://codereview.chromium.org/14197014/diff/120001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc (left):

https://codereview.chromium.org/14197014/diff/120001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:361:
DISALLOW_COPY_AND_ASSIGN(AutofillDialogControllerTest);
On 2013/05/20 21:38:49, Ilya Sherman wrote:
> Why did you remove this?  I see in your CL description "removes problematic
> DISALLOW_COPY_AND_ASSIGNS in tests" -- could you explain why these are
> problematic?  Thanks.

Replied on main thread and added explanation to the CL description.  Basically,
DISALLOW_COPY_AND_ASSIGN() in a GoogleTest fixture forces you to also have to
declare a default constructor which is just extra code.

[Ilya Sherman, 2013-05-20 22:33:01]

*autofill* LGTM, thanks.

https://codereview.chromium.org/14197014/diff/120001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc (left):

https://codereview.chromium.org/14197014/diff/120001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:361:
DISALLOW_COPY_AND_ASSIGN(AutofillDialogControllerTest);
On 2013/05/20 21:57:56, awong wrote:
> On 2013/05/20 21:38:49, Ilya Sherman wrote:
> > Why did you remove this?  I see in your CL description "removes problematic
> > DISALLOW_COPY_AND_ASSIGNS in tests" -- could you explain why these are
> > problematic?  Thanks.
> 
> Replied on main thread and added explanation to the CL description. 
Basically,
> DISALLOW_COPY_AND_ASSIGN() in a GoogleTest fixture forces you to also have to
> declare a default constructor which is just extra code.

Ok, thanks for the explanation -- makes sense :)

[Avi (use Gerrit), 2013-05-21 20:30:43]

jam still wants to get content/public reviews. He's out this week, though...

I'm not familiar enough with the threading that I want to take this. Can you
find another expert? I'll stamp.

[awong, 2013-05-21 20:37:53]

Hmm...unfortunately, I think jam@ will need to be the reviewer then. I can
wait the week for him to get back.

-Albert


On Tue, May 21, 2013 at 1:30 PM, <avi@chromium.org> wrote:

> jam still wants to get content/public reviews. He's out this week,
> though...
>
> I'm not familiar enough with the threading that I want to take this. Can
> you
> find another expert? I'll stamp.
>
>
https://codereview.chromium.**org/14197014/<https://codereview.chromium.org/1...
>

[Greg Billock, 2013-05-22 00:05:25]

https://codereview.chromium.org/14197014/diff/138001/chrome/browser/media_gal...
File chrome/browser/media_galleries/media_file_system_registry_unittest.cc
(right):

https://codereview.chromium.org/14197014/diff/138001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_file_system_registry_unittest.cc:726:
base::RunLoop().RunUntilIdle();
Moved to monitor_->Initialize(run_loop.QuitClosure()) correct?

https://codereview.chromium.org/14197014/diff/138001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_file_system_registry_unittest.cc:732:
ChromeRenderViewHostTestHarness::SetUp();
You moved this up to line 715 right?

[awong, 2013-05-22 20:08:36]

https://codereview.chromium.org/14197014/diff/138001/chrome/browser/media_gal...
File chrome/browser/media_galleries/media_file_system_registry_unittest.cc
(right):

https://codereview.chromium.org/14197014/diff/138001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_file_system_registry_unittest.cc:726:
base::RunLoop().RunUntilIdle();
On 2013/05/22 00:05:25, Greg Billock wrote:
> Moved to monitor_->Initialize(run_loop.QuitClosure()) correct?

Done.

https://codereview.chromium.org/14197014/diff/138001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_file_system_registry_unittest.cc:732:
ChromeRenderViewHostTestHarness::SetUp();
On 2013/05/22 00:05:25, Greg Billock wrote:
> You moved this up to line 715 right?

Good point. Fixed.

[Jeffrey Yasskin, 2013-05-22 22:24:58]

I only looked at test_browser_thread_bundle and test_renderer_host.

https://codereview.chromium.org/14197014/diff/179001/content/public/test/test...
File content/public/test/test_browser_thread_bundle.cc (right):

https://codereview.chromium.org/14197014/diff/179001/content/public/test/test...
content/public/test/test_browser_thread_bundle.cc:17:
io_thread_(BrowserThread::IO, &message_loop_) {
Did you find a way to deal with things DCHECK'ing that they're running on an IO
loop? Might want to comment about how to deal with such things in the future.

https://codereview.chromium.org/14197014/diff/179001/content/public/test/test...
File content/public/test/test_browser_thread_bundle.h (right):

https://codereview.chromium.org/14197014/diff/179001/content/public/test/test...
content/public/test/test_browser_thread_bundle.h:10: // one MessageLoop running
on the instantiating thread. If a test truly
How about "all of the created TestBrowserThreads will be backed by a single
shared MessageLoop". I think the current sentence admits the parsing that each
of the threads is backed by its own MessageLoop, even though that's not really
implementable.

https://codereview.chromium.org/14197014/diff/179001/content/public/test/test...
content/public/test/test_browser_thread_bundle.h:15: // The
TestBrowserThreadBundle will attempt to drain the MessageLoop on
Could you document how to drain the MessageLoop in the middle of the test?
(content::RunAllPendingInMessageLoop(), right?) Otherwise I'd probably look for
a MessageLoop accessor and be confused.

https://codereview.chromium.org/14197014/diff/179001/content/public/test/test...
content/public/test/test_browser_thread_bundle.h:32: enum RealThreadsMask {
Instead of "real" maybe "separate" or "concurrent"? I guess "real" is accurate
too, so if you want to keep it, that's fine with me.

https://codereview.chromium.org/14197014/diff/179001/content/public/test/test...
content/public/test/test_browser_thread_bundle.h:45: TestBrowserThread*
ui_thread() { return &ui_thread_; }
You could return a reference to be clear that these can't be NULL. The style
guide only prohibits non-const reference _parameters_:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Refere...

https://codereview.chromium.org/14197014/diff/179001/content/public/test/test...
File content/public/test/test_renderer_host.cc (right):

https://codereview.chromium.org/14197014/diff/179001/content/public/test/test...
content/public/test/test_renderer_host.cc:187:
BrowserThread::PostTask(content::BrowserThread::UI,
BrowserThread::DeleteSoon exists.

https://codereview.chromium.org/14197014/diff/179001/content/public/test/test...
content/public/test/test_renderer_host.cc:191: base::RunLoop().RunUntilIdle();
You can remove this line, I think.

https://codereview.chromium.org/14197014/diff/179001/content/public/test/test...
File content/public/test/test_renderer_host.h (right):

https://codereview.chromium.org/14197014/diff/179001/content/public/test/test...
content/public/test/test_renderer_host.h:159: // Configures which when to use
real thraeads inside |thread_bundle|.
gr: which when
sp: thraeads

[awong, 2013-05-24 23:39:27]

Comments addressed. PTAL

https://codereview.chromium.org/14197014/diff/179001/content/public/test/test...
File content/public/test/test_browser_thread_bundle.cc (right):

https://codereview.chromium.org/14197014/diff/179001/content/public/test/test...
content/public/test/test_browser_thread_bundle.cc:17:
io_thread_(BrowserThread::IO, &message_loop_) {
On 2013/05/22 22:24:58, Jeffrey Yasskin wrote:
> Did you find a way to deal with things DCHECK'ing that they're running on an
IO
> loop? Might want to comment about how to deal with such things in the future.

For now, the solution is to start a real IO thread. I'll add a comment.

https://codereview.chromium.org/14197014/diff/179001/content/public/test/test...
File content/public/test/test_browser_thread_bundle.h (right):

https://codereview.chromium.org/14197014/diff/179001/content/public/test/test...
content/public/test/test_browser_thread_bundle.h:10: // one MessageLoop running
on the instantiating thread. If a test truly
On 2013/05/22 22:24:58, Jeffrey Yasskin wrote:
> How about "all of the created TestBrowserThreads will be backed by a single
> shared MessageLoop". I think the current sentence admits the parsing that each
> of the threads is backed by its own MessageLoop, even though that's not really
> implementable.

Done.

https://codereview.chromium.org/14197014/diff/179001/content/public/test/test...
content/public/test/test_browser_thread_bundle.h:15: // The
TestBrowserThreadBundle will attempt to drain the MessageLoop on
On 2013/05/22 22:24:58, Jeffrey Yasskin wrote:
> Could you document how to drain the MessageLoop in the middle of the test?
> (content::RunAllPendingInMessageLoop(), right?) Otherwise I'd probably look
for
> a MessageLoop accessor and be confused. 

Added a comment. I think calling base::RunLoop directly is better.

https://codereview.chromium.org/14197014/diff/179001/content/public/test/test...
content/public/test/test_browser_thread_bundle.h:32: enum RealThreadsMask {
On 2013/05/22 22:24:58, Jeffrey Yasskin wrote:
> Instead of "real" maybe "separate" or "concurrent"? I guess "real" is accurate
> too, so if you want to keep it, that's fine with me.

I like real in this case. Shorter.

https://codereview.chromium.org/14197014/diff/179001/content/public/test/test...
content/public/test/test_browser_thread_bundle.h:45: TestBrowserThread*
ui_thread() { return &ui_thread_; }
On 2013/05/22 22:24:58, Jeffrey Yasskin wrote:
> You could return a reference to be clear that these can't be NULL. The style
> guide only prohibits non-const reference _parameters_:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Refere...

I generally dislike non-const references (or really references in general).  In
this particular case, returning a non-const reference gets kinda funny if
someone was to hold a copy of it.

TestBrowserThread& db = bundle_.db_thread();
TestBrowserThread* db = &bundle_.db_thread();

both aren't very common patterns.

https://codereview.chromium.org/14197014/diff/179001/content/public/test/test...
File content/public/test/test_renderer_host.cc (right):

https://codereview.chromium.org/14197014/diff/179001/content/public/test/test...
content/public/test/test_renderer_host.cc:187:
BrowserThread::PostTask(content::BrowserThread::UI,
On 2013/05/22 22:24:58, Jeffrey Yasskin wrote:
> BrowserThread::DeleteSoon exists.

Done.

https://codereview.chromium.org/14197014/diff/179001/content/public/test/test...
content/public/test/test_renderer_host.cc:191: base::RunLoop().RunUntilIdle();
On 2013/05/22 22:24:58, Jeffrey Yasskin wrote:
> You can remove this line, I think.

I'm not certain I can.  What if the browser_context_ posts tasks on deletion
that need ole_initializer_?

https://codereview.chromium.org/14197014/diff/179001/content/public/test/test...
File content/public/test/test_renderer_host.h (right):

https://codereview.chromium.org/14197014/diff/179001/content/public/test/test...
content/public/test/test_renderer_host.h:159: // Configures which when to use
real thraeads inside |thread_bundle|.
On 2013/05/22 22:24:58, Jeffrey Yasskin wrote:
> gr: which when
> sp: thraeads

Done.

[Jeffrey Yasskin, 2013-05-27 03:12:12]

LGTM for extensions and content/public, although you'll still have to get a real
lgtm for content/public.

https://codereview.chromium.org/14197014/diff/179001/content/public/test/test...
File content/public/test/test_renderer_host.cc (right):

https://codereview.chromium.org/14197014/diff/179001/content/public/test/test...
content/public/test/test_renderer_host.cc:191: base::RunLoop().RunUntilIdle();
On 2013/05/24 23:39:28, awong wrote:
> On 2013/05/22 22:24:58, Jeffrey Yasskin wrote:
> > You can remove this line, I think.
> 
> I'm not certain I can.  What if the browser_context_ posts tasks on deletion
> that need ole_initializer_?

Ew. Good point. Would it be better to reset the thread_bundle_ before the
ole_initializer_, or can the ole_initializer use the threads?

https://codereview.chromium.org/14197014/diff/189001/chrome/browser/extension...
File chrome/browser/extensions/activity_log/activity_database_unittest.cc
(right):

https://codereview.chromium.org/14197014/diff/189001/chrome/browser/extension...
chrome/browser/extensions/activity_log/activity_database_unittest.cc:44:
test_user_manager_.reset(new chromeos::ScopedTestUserManager());
I don't like having to move value objects into scoped_ptrs, especially if you're
also required to manually destroy them in TearDown(). I guess you had to do this
in order to allow SetRealThreadMask() to control the thread startup. You could
give RenderViewHostTestHarness a constructor with an argument, but that's pretty
uncommon. All the more reason that designing testing utilities to require
inheritance is a bad idea, I guess.

https://codereview.chromium.org/14197014/diff/189001/content/public/test/test...
File content/public/test/test_renderer_host.h (right):

https://codereview.chromium.org/14197014/diff/189001/content/public/test/test...
content/public/test/test_renderer_host.h:159: // Configures when
TestBrowserThreads inside |thread_bundle| are backed by
s/when/which/?

[awong, 2013-05-30 21:46:08]

John, I think you're probably the best for this review.

This is the first of a few large CLs needed to fix URLRequestContext
initialization.

The description explains the motivation for all the changes.  It's probably
easiest to start with the content/public changes.  I can also find individual
component owners for the rest of the CL if you'd prefer, but 90% of the time,
it's just deleting various TestBrowserThread member variables.

https://codereview.chromium.org/14197014/diff/179001/content/public/test/test...
File content/public/test/test_renderer_host.cc (right):

https://codereview.chromium.org/14197014/diff/179001/content/public/test/test...
content/public/test/test_renderer_host.cc:191: base::RunLoop().RunUntilIdle();
On 2013/05/27 03:12:13, Jeffrey Yasskin wrote:
> On 2013/05/24 23:39:28, awong wrote:
> > On 2013/05/22 22:24:58, Jeffrey Yasskin wrote:
> > > You can remove this line, I think.
> > 
> > I'm not certain I can.  What if the browser_context_ posts tasks on deletion
> > that need ole_initializer_?
> 
> Ew. Good point. Would it be better to reset the thread_bundle_ before the
> ole_initializer_, or can the ole_initializer use the threads?

Per IRC conversation with cpu@, requiring OLE during teardown is already a no-no
since you aren't going to be able to sanely handle a drag'n'drop if half the
system is gone.

Moving the reset up before the rph_factory_.reset() and removing this
RunUntilIdle().  Now we only drain the loop twice during TearDown (one
explicitly, and one via the thread_bundle_.reset()).

https://codereview.chromium.org/14197014/diff/189001/chrome/browser/extension...
File chrome/browser/extensions/activity_log/activity_database_unittest.cc
(right):

https://codereview.chromium.org/14197014/diff/189001/chrome/browser/extension...
chrome/browser/extensions/activity_log/activity_database_unittest.cc:44:
test_user_manager_.reset(new chromeos::ScopedTestUserManager());
On 2013/05/27 03:12:13, Jeffrey Yasskin wrote:
> I don't like having to move value objects into scoped_ptrs, especially if
you're
> also required to manually destroy them in TearDown(). I guess you had to do
this
> in order to allow SetRealThreadMask() to control the thread startup. You could
> give RenderViewHostTestHarness a constructor with an argument, but that's
pretty
> uncommon. All the more reason that designing testing utilities to require
> inheritance is a bad idea, I guess.

Yeah. All choices are bad.
  (1) We can't move all initialization into the constructor 
      due to harness initialization ordering issues 
      listed in CL.
  (2) We can't call RVHTH::SetUp() in the constructor 
      because it's virtual which, while legally doable, has 
      subtle behavior that will likely bite most people.
  (3) If we add a parameter to the constructor, then we have
      split duties for harness initialization between the
      constructor and the SetUp() method.

Basically, because RVHTH is invoked via inheritance, the initialization order is
constrained which makes this API wart hard to fix.

https://codereview.chromium.org/14197014/diff/189001/content/public/test/test...
File content/public/test/test_renderer_host.h (right):

https://codereview.chromium.org/14197014/diff/189001/content/public/test/test...
content/public/test/test_renderer_host.h:159: // Configures when
TestBrowserThreads inside |thread_bundle| are backed by
On 2013/05/27 03:12:13, Jeffrey Yasskin wrote:
> s/when/which/?

Done.

[jam, 2013-05-31 17:29:24]

this is great, thank you!

https://codereview.chromium.org/14197014/diff/197001/chrome/browser/automatio...
File chrome/browser/automation/automation_tab_helper_unittest.cc (right):

https://codereview.chromium.org/14197014/diff/197001/chrome/browser/automatio...
chrome/browser/automation/automation_tab_helper_unittest.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
i think this is a merge conflict, this file is gone now

https://codereview.chromium.org/14197014/diff/197001/chrome/browser/browsing_...
File chrome/browser/browsing_data/browsing_data_appcache_helper_unittest.cc
(right):

https://codereview.chromium.org/14197014/diff/197001/chrome/browser/browsing_...
chrome/browser/browsing_data/browsing_data_appcache_helper_unittest.cc:35:
content::TestBrowserThreadBundle thread_bundle_;
why does this need it now?

https://codereview.chromium.org/14197014/diff/197001/content/browser/browser_...
File content/browser/browser_thread_impl.cc (right):

https://codereview.chromium.org/14197014/diff/197001/content/browser/browser_...
content/browser/browser_thread_impl.cc:73: : Thread(message_loop ?
message_loop->thread_name().c_str() :
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

https://codereview.chromium.org/14197014/diff/197001/content/public/test/test...
File content/public/test/test_browser_thread_bundle.cc (right):

https://codereview.chromium.org/14197014/diff/197001/content/public/test/test...
content/public/test/test_browser_thread_bundle.cc:25: real_threads_mask &
REAL_FILE_THREAD ? NULL : &message_loop_),
nit: here and below, indentation

https://codereview.chromium.org/14197014/diff/197001/content/public/test/test...
File content/public/test/test_browser_thread_bundle.h (right):

https://codereview.chromium.org/14197014/diff/197001/content/public/test/test...
content/public/test/test_browser_thread_bundle.h:10: // all of the created
TestBrowserThreads will be backed by a single
nit: fix comment

https://codereview.chromium.org/14197014/diff/197001/content/public/test/test...
content/public/test/test_browser_thread_bundle.h:12: // so by passing the
apprpriate combination of RealThreadsMask values during
nit: spelling

https://codereview.chromium.org/14197014/diff/197001/content/public/test/test...
content/public/test/test_browser_thread_bundle.h:18: // the RunLoop (eg.,
base::RunLoop().RunUntilIdle()). Very rarely should a
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)

https://codereview.chromium.org/14197014/diff/197001/content/public/test/test...
content/public/test/test_browser_thread_bundle.h:27: #ifndef
BROWSER_THREAD_TEST_BUNDLE_H_
nit: fix guard (i.e. add CONTENT_PUBLIC_TEST_)

https://codereview.chromium.org/14197014/diff/197001/content/public/test/test...
content/public/test/test_browser_thread_bundle.h:42: NO_REAL_THREAD = 0x0,
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)

https://codereview.chromium.org/14197014/diff/197001/content/public/test/test...
content/public/test/test_browser_thread_bundle.h:75: #endif /*
BROWSER_THREAD_TEST_BUNDLE_H_ */
nit: please follow the convention for this line

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

PTAL

https://codereview.chromium.org/14197014/diff/197001/chrome/browser/automatio...
File chrome/browser/automation/automation_tab_helper_unittest.cc (right):

https://codereview.chromium.org/14197014/diff/197001/chrome/browser/automatio...
chrome/browser/automation/automation_tab_helper_unittest.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2013/05/31 17:29:25, jam wrote:
> i think this is a merge conflict, this file is gone now

Done.

https://codereview.chromium.org/14197014/diff/197001/chrome/browser/browsing_...
File chrome/browser/browsing_data/browsing_data_appcache_helper_unittest.cc
(right):

https://codereview.chromium.org/14197014/diff/197001/chrome/browser/browsing_...
chrome/browser/browsing_data/browsing_data_appcache_helper_unittest.cc:35:
content::TestBrowserThreadBundle thread_bundle_;
On 2013/05/31 17:29:25, jam wrote:
> why does this need it now?

Leaked in from the CL I split out out of. Next CL will need it, but I've removed
it for now.

https://codereview.chromium.org/14197014/diff/197001/content/browser/browser_...
File content/browser/browser_thread_impl.cc (right):

https://codereview.chromium.org/14197014/diff/197001/content/browser/browser_...
content/browser/browser_thread_impl.cc:73: : Thread(message_loop ?
message_loop->thread_name().c_str() :
On 2013/05/31 17:29:25, jam wrote:
> 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

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.

https://codereview.chromium.org/14197014/diff/197001/content/public/test/test...
File content/public/test/test_browser_thread_bundle.cc (right):

https://codereview.chromium.org/14197014/diff/197001/content/public/test/test...
content/public/test/test_browser_thread_bundle.cc:25: real_threads_mask &
REAL_FILE_THREAD ? NULL : &message_loop_),
On 2013/05/31 17:29:25, jam wrote:
> nit: here and below, indentation

Done.

https://codereview.chromium.org/14197014/diff/197001/content/public/test/test...
File content/public/test/test_browser_thread_bundle.h (right):

https://codereview.chromium.org/14197014/diff/197001/content/public/test/test...
content/public/test/test_browser_thread_bundle.h:10: // all of the created
TestBrowserThreads will be backed by a single
On 2013/05/31 17:29:25, jam wrote:
> nit: fix comment

Done.

https://codereview.chromium.org/14197014/diff/197001/content/public/test/test...
content/public/test/test_browser_thread_bundle.h:12: // so by passing the
apprpriate combination of RealThreadsMask values during
On 2013/05/31 17:29:25, jam wrote:
> nit: spelling

Done.

https://codereview.chromium.org/14197014/diff/197001/content/public/test/test...
content/public/test/test_browser_thread_bundle.h:18: // the RunLoop (eg.,
base::RunLoop().RunUntilIdle()). Very rarely should a
On 2013/05/31 17:29:25, jam wrote:
> 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)

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.

https://codereview.chromium.org/14197014/diff/197001/content/public/test/test...
content/public/test/test_browser_thread_bundle.h:27: #ifndef
BROWSER_THREAD_TEST_BUNDLE_H_
On 2013/05/31 17:29:25, jam wrote:
> nit: fix guard (i.e. add CONTENT_PUBLIC_TEST_)

Done.

https://codereview.chromium.org/14197014/diff/197001/content/public/test/test...
content/public/test/test_browser_thread_bundle.h:42: NO_REAL_THREAD = 0x0,
On 2013/05/31 17:29:25, jam wrote:
> 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)

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.

https://codereview.chromium.org/14197014/diff/197001/content/public/test/test...
content/public/test/test_browser_thread_bundle.h:75: #endif /*
BROWSER_THREAD_TEST_BUNDLE_H_ */
On 2013/05/31 17:29:25, jam wrote:
> nit: please follow the convention for this line

Done.

[Jeffrey Yasskin, 2013-05-31 21:11:32]

https://codereview.chromium.org/14197014/diff/197001/content/public/test/test...
File content/public/test/test_browser_thread_bundle.h (right):

https://codereview.chromium.org/14197014/diff/197001/content/public/test/test...
content/public/test/test_browser_thread_bundle.h:18: // the RunLoop (eg.,
base::RunLoop().RunUntilIdle()). Very rarely should a
On 2013/05/31 20:57:17, awong wrote:
> On 2013/05/31 17:29:25, jam wrote:
> > 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)
> 
> 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.

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:03]

https://codereview.chromium.org/14197014/diff/197001/content/browser/browser_...
File content/browser/browser_thread_impl.cc (right):

https://codereview.chromium.org/14197014/diff/197001/content/browser/browser_...
content/browser/browser_thread_impl.cc:73: : Thread(message_loop ?
message_loop->thread_name().c_str() :
On 2013/05/31 20:57:17, awong wrote:
> On 2013/05/31 17:29:25, jam wrote:
> > 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
> 
> 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.

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.

https://codereview.chromium.org/14197014/diff/197001/content/public/test/test...
File content/public/test/test_browser_thread_bundle.h (right):

https://codereview.chromium.org/14197014/diff/197001/content/public/test/test...
content/public/test/test_browser_thread_bundle.h:18: // the RunLoop (eg.,
base::RunLoop().RunUntilIdle()). Very rarely should a
On 2013/05/31 21:11:33, Jeffrey Yasskin wrote:
> On 2013/05/31 20:57:17, awong wrote:
> > On 2013/05/31 17:29:25, jam wrote:
> > > 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)
> > 
> > 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.
> 
> 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.


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.

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

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.

> 
> 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.

https://codereview.chromium.org/14197014/diff/197001/content/public/test/test...
content/public/test/test_browser_thread_bundle.h:42: NO_REAL_THREAD = 0x0,
On 2013/05/31 20:57:17, awong wrote:
> On 2013/05/31 17:29:25, jam wrote:
> > 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)
> 
> 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.

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

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

jam@: Addressed your comments.

gbillock: can you look at new changes to
media_galleries/media_file_system_registry_unittest.cc?

rogerta: can you look at one_click_signin_helper*?

jyasskin: FYI, I changed TestBrowserThreadBundle to be able to handle IO message
loops.

https://codereview.chromium.org/14197014/diff/197001/content/browser/browser_...
File content/browser/browser_thread_impl.cc (right):

https://codereview.chromium.org/14197014/diff/197001/content/browser/browser_...
content/browser/browser_thread_impl.cc:73: : Thread(message_loop ?
message_loop->thread_name().c_str() :
On 2013/05/31 22:53:04, jam wrote:
> On 2013/05/31 20:57:17, awong wrote:
> > On 2013/05/31 17:29:25, jam wrote:
> > > 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
> > 
> > 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.
> 
> 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.

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.

https://codereview.chromium.org/14197014/diff/197001/content/public/test/test...
File content/public/test/test_browser_thread_bundle.h (right):

https://codereview.chromium.org/14197014/diff/197001/content/public/test/test...
content/public/test/test_browser_thread_bundle.h:18: // the RunLoop (eg.,
base::RunLoop().RunUntilIdle()). Very rarely should a
On 2013/05/31 22:53:04, jam wrote:
> On 2013/05/31 21:11:33, Jeffrey Yasskin wrote:
> > On 2013/05/31 20:57:17, awong wrote:
> > > On 2013/05/31 17:29:25, jam wrote:
> > > > 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)
> > > 
> > > 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.
> > 
> > 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.
> 
> 
> 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.
> 
> > 
> > Also note that content::RunAllPendingInMessageLoop() uses QuitWhenIdle:
> >
>
https://code.google.com/p/chromium/codesearch/#chromium/src/content/public/te...
> 
> 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.
> 
> > 
> > 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.
> 

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

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

https://codereview.chromium.org/14197014/diff/240003/content/public/test/test...
File content/public/test/test_browser_thread_bundle.h (right):

https://codereview.chromium.org/14197014/diff/240003/content/public/test/test...
content/public/test/test_browser_thread_bundle.h:17: // (eg.,
base::RunLoop().RunUntilIdle()) as appropriate.
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]

https://codereview.chromium.org/14197014/diff/240003/content/public/test/test...
File content/public/test/test_browser_thread_bundle.h (right):

https://codereview.chromium.org/14197014/diff/240003/content/public/test/test...
content/public/test/test_browser_thread_bundle.h:17: // (eg.,
base::RunLoop().RunUntilIdle()) as appropriate.
On 2013/06/05 17:01:39, jam wrote:
> 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?

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?

[jam, 2013-06-05 18:14:55]

lgtm with comment change we discussed over IM

[Roger Tawa OOO till Jul 10th, 2013-06-05 19:10:53]

lgtm for one_click_signin changes.

[awong, 2013-06-05 21:16:55]

Comment change done.

need explicit OWNERS review:
thakis:  
 chrome/browser/notifications/desktop_notification_service_unittest.cc

michaeln:
 webkit/browser/appcache/appcache_storage_impl.cc

mattm:
 chrome/browser/safe_browsing/*
 net/url_request/*

https://codereview.chromium.org/14197014/diff/240003/content/public/test/test...
File content/public/test/test_browser_thread_bundle.h (right):

https://codereview.chromium.org/14197014/diff/240003/content/public/test/test...
content/public/test/test_browser_thread_bundle.h:17: // (eg.,
base::RunLoop().RunUntilIdle()) as appropriate.
On 2013/06/05 17:54:38, awong wrote:
> On 2013/06/05 17:01:39, jam wrote:
> > 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?
> 
> 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?

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

[michaeln, 2013-06-05 21:17:33]

appcache lgtm (glad to see we don't hit that in assert in tests where it
matters)

[Greg Billock, 2013-06-05 23:31:32]

lgtm media_galleries

https://codereview.chromium.org/14197014/diff/258001/chrome/browser/media_gal...
File chrome/browser/media_galleries/media_file_system_registry_unittest.cc
(right):

https://codereview.chromium.org/14197014/diff/258001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_file_system_registry_unittest.cc:741: //
TODO(ajwong): ask gbillock if we need this MarkInitialized?
The TestStorageMonitor is set up so the tests run MarkInitialized manually. So
yes.

[mattm, 2013-06-05 23:44:06]

safe_browsing and net lgtm

[awong, 2013-06-06 00:43:44]

Minus one confirmation from Nico, this CL is about to be submitting.

I'm TBRing the following 22 OWNERS for the listed directories below:

avi:
  chrome/browser/tab_contents
  chrome/browser/ui/cocoa/content_settings
  chrome/browser/ui/cocoa/notifications

battre:
  chrome/browser/prefs

ben:
  chrome/browser/ui/find_bar
  chrome/browser/ui/toolbar

benwells:
  chrome/browser/web_applications

brettw:
  components/visitedlink/test

dbeam:
  chrome/browser/ui/zoom

dimich:
  chrome/browser/notifications

joi:
  components/web_modal

joth:
  chrome/browser/geolocation
  components/navigation_interception

mad:
  chrome/browser/translate

marja:
  chrome/browser/sessions

markusheintz:
  chrome/browser/content_settings
  chrome/browser/ui/content_settings
  chrome/browser/ui/website_settings

mmenke:
  chrome/browser/captive_portal
  chrome/browser/net

noelallen:
  chrome/browser/nacl_host

phajdan.jr:
  chrome/test/base

rdsmith:
  chrome/browser/download

satorux:
  chrome/browser/chromeos/drive

scherkus:
  chrome/browser/media

sky:
  chrome/browser/ui/tabs

stevenjb:
  chrome/browser/ui/ash/launcher

stuartmorgan:
  chrome/browser/password_manager

timsteele:
  chrome/browser/sync/glue
  chrome/browser/ui/sync

[satorux1, 2013-06-06 00:47:08]

chrome/browser/chromeos/drive LGTM. Thank you for simplifying the tests
significantly.

[marja, 2013-06-06 07:44:53]

chrome/browser/sessions/persistent_tab_restore_service_browsertest.cc lgtm

[battre, 2013-06-06 08:14:21]

chrome/browser/prefs LGTM

[Jói, 2013-06-06 11:32:53]

//components/web_modal LGTM.

[Nico, 2013-06-06 16:36:48]

desktop_notification_service_unittest lgtm

[commit-bot: I haz the power, 2013-06-06 18:01:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajwong@chromium.org/14197014/284001

[commit-bot: I haz the power, 2013-06-06 18:03:23]

Failed to request the patch to try. Please note that binary files are still
unsupported at the moment, this is being worked on.

Thanks for your patience.

HTTP Error 500: 
<html><head>
<meta http-equiv="content-type" content="text/html;charset=utf-8">
<title>500 Server Error</title>
</head>
<body text=#000000 bgcolor=#ffffff>
<h1>Error: Server Error</h1>
<h2>The server encountered an error and could not complete your request.<p>If
the problem persists, please <A
HREF="http://code.google.com/appengine/community.html">report</A> your problem
and mention this error message and the query that caused it.</h2>
<h2></h2>
</body></html>

[commit-bot: I haz the power, 2013-06-06 18:31:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajwong@chromium.org/14197014/174006

[commit-bot: I haz the power, 2013-06-06 21:31:43]

Change committed as 204603