Correctly integrate StoragePartition into TestingProfile.

Correctly integrate StoragePartition into TestingProfile.

Until this CL, TestingProfile had 3 distinct URLRequestContexts it would return to the user: one via profile->GetDefaultStoragePartition()->GetRequestContext(), one via profile->GetRequestContext(), and one via profile->GetResourceContext->GetRequestContext().  All of these had different cookie stores.  This CL unified them so they all return the one from profile->GetDefaultStoragePartition()->GetRequestContext(). This correctly mimics how production code works.

Doing this had a bunch of implications:
 (1) Removes TestingProfile::CreateRequestContext()/ResetRequestContext()
 (2) Changes MockRequestContext to share the URLRequestContext with TestingProfile.
 (3) TestingProfile now requires all BrowserThreads.

Point (3) effectively adds TestBrowserThreadBundle into a bunch more spots. Because of that, we also have the following changes:
  (a) AshTestBase now has a TestBrowserThreadBundle
  (b) Removed a bunch of real threads from tests.
  (c) TemplateUrlService has reworked synchronization semantics.
  (d) Removed MultiThreadTestHelper.
  (e) Added TestingIOThread class + testing API in IOThread to mock out various IO thread tasks which enervated with the new TestBrowserThreadBundle.

TBR=bauerb,brettw,isherman,joth,jyasskin,mattm,mmenke,mnissler,pkasting,rkc,rlp,satorux,tim,xians,joth
BUG=159193
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213272

[awong, 2013-07-11 01:29:55]

Another massive review.

[awong, 2013-07-11 01:33:00]

Before moving onto OWNERS reviews for individual unittests, John, Pawel, can you
do the top-level review?

Pawel, files of interest for you are:
  chrome/test/base/testing_profile.{h,cc}

John, files of interest for you are:
  content/public/test/*
  chrome/browser/io_thread.{h,cc}
  chrome/test/base/testing_io_thread_state.{h,cc}

For context on TestingIOThreadState, look at wrench_menu_model_unittest.cc, and
off_the_record_profile_impl_unittests.cc. There's a few others, but those should
show you the basic usage.

[tfarina, 2013-07-11 01:35:44]

I was planning to get rid of uses of
CreateRequestContext()/ResetRequestContext() in components/autofill.

It seems this will make it easier?

https://codereview.chromium.org/17127002/diff/148001/components/autofill/cont...
File components/autofill/content/browser/autocheckout_manager_unittest.cc
(right):

https://codereview.chromium.org/17127002/diff/148001/components/autofill/cont...
components/autofill/content/browser/autocheckout_manager_unittest.cc:10:
#include "chrome/test/base/testing_profile.h"
can you remove this now?

https://codereview.chromium.org/17127002/diff/148001/components/autofill/core...
File components/autofill/core/browser/autofill_download_unittest.cc (right):

https://codereview.chromium.org/17127002/diff/148001/components/autofill/core...
components/autofill/core/browser/autofill_download_unittest.cc:120:
TestingProfile profile_;
do you still need profile_?

https://codereview.chromium.org/17127002/diff/148001/components/autofill/core...
File components/autofill/core/browser/autofill_manager_unittest.cc (right):

https://codereview.chromium.org/17127002/diff/148001/components/autofill/core...
components/autofill/core/browser/autofill_manager_unittest.cc:743:
TestingProfile* profile = CreateProfile();
can we create TestBrowserContext now?

https://codereview.chromium.org/17127002/diff/148001/components/autofill/core...
File components/autofill/core/browser/autofill_metrics_unittest.cc (right):

https://codereview.chromium.org/17127002/diff/148001/components/autofill/core...
components/autofill/core/browser/autofill_metrics_unittest.cc:285:
TestingProfile* profile = new TestingProfile();
can we create a TestBrowserContext now?

[awong, 2013-07-11 01:46:02]

jcivelli: Pawel's out. Can you look at chrome/test changes?

[jam, 2013-07-11 16:28:44]

this looks fine

btw i looked at all the files in chrome/test and it looks fine as well (no need
for Jay to take a look)

https://codereview.chromium.org/17127002/diff/148001/chrome/test/base/browser...
File chrome/test/base/browser_with_test_window_test.cc (right):

https://codereview.chromium.org/17127002/diff/148001/chrome/test/base/browser...
chrome/test/base/browser_with_test_window_test.cc:36: //TODO(ajwong): REal Db
huh?
is this temporary?

https://codereview.chromium.org/17127002/diff/148001/chrome/test/base/testing...
File chrome/test/base/testing_io_thread_state.cc (right):

https://codereview.chromium.org/17127002/diff/148001/chrome/test/base/testing...
chrome/test/base/testing_io_thread_state.cc:49: io_thread_state_.reset(
nit: indentation seems off

[awong, 2013-07-11 21:04:15]

comments addressed.

https://codereview.chromium.org/17127002/diff/148001/chrome/test/base/browser...
File chrome/test/base/browser_with_test_window_test.cc (right):

https://codereview.chromium.org/17127002/diff/148001/chrome/test/base/browser...
chrome/test/base/browser_with_test_window_test.cc:36: //TODO(ajwong): REal Db
huh?
On 2013/07/11 16:28:44, jam wrote:
> is this temporary? 

Oops. Removed the REAL_DB_THREAD. I think I cleaned everything up that needed it
already...

https://codereview.chromium.org/17127002/diff/148001/chrome/test/base/testing...
File chrome/test/base/testing_io_thread_state.cc (right):

https://codereview.chromium.org/17127002/diff/148001/chrome/test/base/testing...
chrome/test/base/testing_io_thread_state.cc:49: io_thread_state_.reset(
On 2013/07/11 16:28:44, jam wrote:
> nit: indentation seems off

Done.

https://codereview.chromium.org/17127002/diff/148001/components/autofill/cont...
File components/autofill/content/browser/autocheckout_manager_unittest.cc
(right):

https://codereview.chromium.org/17127002/diff/148001/components/autofill/cont...
components/autofill/content/browser/autocheckout_manager_unittest.cc:10:
#include "chrome/test/base/testing_profile.h"
On 2013/07/11 01:35:45, tfarina wrote:
> can you remove this now?

Done.

https://codereview.chromium.org/17127002/diff/148001/components/autofill/core...
File components/autofill/core/browser/autofill_download_unittest.cc (right):

https://codereview.chromium.org/17127002/diff/148001/components/autofill/core...
components/autofill/core/browser/autofill_download_unittest.cc:120:
TestingProfile profile_;
On 2013/07/11 01:35:45, tfarina wrote:
> do you still need profile_?

Seems to be used by download_manager_.

https://codereview.chromium.org/17127002/diff/148001/components/autofill/core...
File components/autofill/core/browser/autofill_manager_unittest.cc (right):

https://codereview.chromium.org/17127002/diff/148001/components/autofill/core...
components/autofill/core/browser/autofill_manager_unittest.cc:743:
TestingProfile* profile = CreateProfile();
On 2013/07/11 01:35:45, tfarina wrote:
> can we create TestBrowserContext now?

This ended up not being a super simple change so I'd rather leave it out of this
CL which is big enough as is.

Also, since AutofillManagerTest derives from ChromeRenderViewHostTestHarness,
using TestingProfile seems like it makes more sense...

https://codereview.chromium.org/17127002/diff/148001/components/autofill/core...
File components/autofill/core/browser/autofill_metrics_unittest.cc (right):

https://codereview.chromium.org/17127002/diff/148001/components/autofill/core...
components/autofill/core/browser/autofill_metrics_unittest.cc:285:
TestingProfile* profile = new TestingProfile();
On 2013/07/11 01:35:45, tfarina wrote:
> can we create a TestBrowserContext now?

Same answer to previous comment. I'm not familiar enough with the changeover
from TestingProfile -> TestBrowserContext that I'm comfortable grouping in such
a change with this CL.

[awong, 2013-07-12 00:52:29]

Adding reviewers for directories where one or more tests had a significant
change:

markusheintz: chrome/browser/browsing_data/*
      ivankr: chrome/browser/chromeos/login/*
    isherman: chrome/browser/password_manager/*
     thestig: chrome/browser/printing/cloud_print/*
    pkasting: chrome/browser/search_engines/*
     rogerta: chrome/browser/signin/*
         ben: chrome/browser/ui/*
      kinuko: chrome/browser/sync_file_system/*

Will TBR other directories, but this set had more than trivial modifications.

[Peter Kasting, 2013-07-12 01:20:19]

I don't actually understand the change from a functional perspective, but things
seemed to get far simpler, which is always good.  So LGTM.

https://codereview.chromium.org/17127002/diff/5891733057437696/chrome/browser...
File chrome/browser/search_engines/search_provider_install_data_unittest.cc
(left):

https://codereview.chromium.org/17127002/diff/5891733057437696/chrome/browser...
chrome/browser/search_engines/search_provider_install_data_unittest.cc:277: 
Nit: Either remove all the cases of double-blank-line-between-tests in this
file, or none of them.

https://codereview.chromium.org/17127002/diff/5891733057437696/chrome/browser...
File chrome/browser/search_engines/template_url_service_unittest.cc (right):

https://codereview.chromium.org/17127002/diff/5891733057437696/chrome/browser...
chrome/browser/search_engines/template_url_service_unittest.cc:163:
TemplateURLService::GenerateSearchURLUsingTermsData(&t_url,
Nit: Put first arg on next line with second arg, so we don't have misaligned
lines of args

https://codereview.chromium.org/17127002/diff/5891733057437696/chrome/browser...
File chrome/browser/signin/oauth2_token_service_unittest.cc (right):

https://codereview.chromium.org/17127002/diff/5891733057437696/chrome/browser...
chrome/browser/signin/oauth2_token_service_unittest.cc:69:
BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO))) {
Nit: I'm not sure, but I _think_ the old indenting was better.

[Ilya Sherman, 2013-07-12 01:59:39]

LGTM with a question:

https://codereview.chromium.org/17127002/diff/5891733057437696/chrome/browser...
File chrome/browser/password_manager/password_store_x_unittest.cc (right):

https://codereview.chromium.org/17127002/diff/5891733057437696/chrome/browser...
chrome/browser/password_manager/password_store_x_unittest.cc:263:
base::RunLoop().RunUntilIdle();
When is it appropriate to use this vs. RunAllPendingInMessageLoop() from
content/public/test/test_utils.h?

[kinuko, 2013-07-12 04:35:43]

https://codereview.chromium.org/17127002/diff/5891733057437696/chrome/browser...
File chrome/browser/sync_file_system/local_file_sync_service_unittest.cc
(right):

https://codereview.chromium.org/17127002/diff/5891733057437696/chrome/browser...
chrome/browser/sync_file_system/local_file_sync_service_unittest.cc:175:
content::TestBrowserThreadBundle thread_bundle_;
MultiThreadTestHelper internally uses TestBrowserThreadBundle, and I'm not very
sure if this change is really necessary.

This change is also making some tests from multi-threaded to single-threaded,
and thus losing some test coverage.

Does your change require test code which uses TestProfile to run
single-threaded? If not can we continue to use the test helper?

[awong, 2013-07-12 07:17:38]

https://codereview.chromium.org/17127002/diff/5891733057437696/chrome/browser...
File chrome/browser/password_manager/password_store_x_unittest.cc (right):

https://codereview.chromium.org/17127002/diff/5891733057437696/chrome/browser...
chrome/browser/password_manager/password_store_x_unittest.cc:263:
base::RunLoop().RunUntilIdle();
On 2013/07/12 01:59:40, Ilya Sherman wrote:
> When is it appropriate to use this vs. RunAllPendingInMessageLoop() from
> content/public/test/test_utils.h?

The two are interchangeable in a unittest.  In a browser test,
RunAllPendingInMessageLoop is required on certain platforms since it enables
nested message loops.

Note this is different from RunAllPendingInMessageLoop(BrowserThread::ID).  That
particular variation is smart enough to flush the target thread safely whether
or not it is real.

https://codereview.chromium.org/17127002/diff/5891733057437696/chrome/browser...
File chrome/browser/sync_file_system/local_file_sync_service_unittest.cc
(right):

https://codereview.chromium.org/17127002/diff/5891733057437696/chrome/browser...
chrome/browser/sync_file_system/local_file_sync_service_unittest.cc:175:
content::TestBrowserThreadBundle thread_bundle_;
On 2013/07/12 04:35:44, kinuko wrote:
> MultiThreadTestHelper internally uses TestBrowserThreadBundle, and I'm not
very
> sure if this change is really necessary.

Nope...it's not required. I thought it was just extra cleanup I could do while
here.
 
> This change is also making some tests from multi-threaded to single-threaded,
> and thus losing some test coverage.

Regarding this, my general experience has been that multi-threaded tests don't
actually increase test coverage much unless the test was very specifically
designed to test parallelsim.  I had a long write-up on this in the FAQ here:
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/TestBr...
(search for "too bland").  For brevity, the summary is If a test mostly has a
pattern of

  SpawnAsyncTask();
  WaitForTaskCompleteion();
  SpawnNextAsyncTask();
  WaitForTaskCompleteion();

Then there really isn't much parallelism and spawning extra threads just adds
overhead (esp on valgrind) + complexity/flakiness.

At first glance, I didn't see much coverage lost. Having said that, I don't
understand this test super well.  If you tell me that the assertions these tests
are guaranteeing partially rely on having real threads, then I'll believe you
and add it back in. 

> Does your change require test code which uses TestProfile to run
> single-threaded? If not can we continue to use the test helper?

Nope...not required to be single threaded.  If you find the helper to be a
meaningful abstraction (regardless of whether we use real threads or not), I can
totally add it back in.  Your code, your call. :)

[kinuko, 2013-07-12 07:28:26]

https://codereview.chromium.org/17127002/diff/5891733057437696/chrome/browser...
File chrome/browser/sync_file_system/local_file_sync_service_unittest.cc
(right):

https://codereview.chromium.org/17127002/diff/5891733057437696/chrome/browser...
chrome/browser/sync_file_system/local_file_sync_service_unittest.cc:175:
content::TestBrowserThreadBundle thread_bundle_;
On 2013/07/12 07:17:39, awong wrote:
> On 2013/07/12 04:35:44, kinuko wrote:
> > MultiThreadTestHelper internally uses TestBrowserThreadBundle, and I'm not
> very
> > sure if this change is really necessary.
> 
> Nope...it's not required. I thought it was just extra cleanup I could do while
> here.
>  
> > This change is also making some tests from multi-threaded to
single-threaded,
> > and thus losing some test coverage.
> 
> Regarding this, my general experience has been that multi-threaded tests don't
> actually increase test coverage much unless the test was very specifically
> designed to test parallelsim.  I had a long write-up on this in the FAQ here:
>
https://groups.google.com/a/chromium.org/forum/#%21searchin/chromium-dev/Test...
> (search for "too bland").  For brevity, the summary is If a test mostly has a
> pattern of
> 
>   SpawnAsyncTask();
>   WaitForTaskCompleteion();
>   SpawnNextAsyncTask();
>   WaitForTaskCompleteion();
> 
> Then there really isn't much parallelism and spawning extra threads just adds
> overhead (esp on valgrind) + complexity/flakiness.
> 
> At first glance, I didn't see much coverage lost. Having said that, I don't
> understand this test super well.  If you tell me that the assertions these
tests
> are guaranteeing partially rely on having real threads, then I'll believe you
> and add it back in. 
> 
> > Does your change require test code which uses TestProfile to run
> > single-threaded? If not can we continue to use the test helper?
> 
> Nope...not required to be single threaded.  If you find the helper to be a
> meaningful abstraction (regardless of whether we use real threads or not), I
can
> totally add it back in.  Your code, your call. :)

Right, in most cases I'd agree with you and just use single-thread, but this
test goes through a heavily threaded class under the surface, which chains tasks
spanning on three threads.  And multi-threaded tests actually helped me find out
some edge cases while I'm developing.

Putting aside the fact that such class should be cleaned up in a simpler model
(there's a TODO for that :)), I'd like to keep this test multi-threaded for now.
 (Reg: to use a helper class or not, I don't have strong opinion on it. Now that
we have ThreadBundle the helper class is not adding much)

[awong, 2013-07-12 07:38:02]

On 2013/07/12 07:28:26, kinuko wrote:
>
https://codereview.chromium.org/17127002/diff/5891733057437696/chrome/browser...
> File chrome/browser/sync_file_system/local_file_sync_service_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/17127002/diff/5891733057437696/chrome/browser...
> chrome/browser/sync_file_system/local_file_sync_service_unittest.cc:175:
> content::TestBrowserThreadBundle thread_bundle_;
> On 2013/07/12 07:17:39, awong wrote:
> > On 2013/07/12 04:35:44, kinuko wrote:
> > > MultiThreadTestHelper internally uses TestBrowserThreadBundle, and I'm not
> > very
> > > sure if this change is really necessary.
> > 
> > Nope...it's not required. I thought it was just extra cleanup I could do
while
> > here.
> >  
> > > This change is also making some tests from multi-threaded to
> single-threaded,
> > > and thus losing some test coverage.
> > 
> > Regarding this, my general experience has been that multi-threaded tests
don't
> > actually increase test coverage much unless the test was very specifically
> > designed to test parallelsim.  I had a long write-up on this in the FAQ
here:
> >
>
https://groups.google.com/a/chromium.org/forum/#%2521searchin/chromium-dev/Te...
> > (search for "too bland").  For brevity, the summary is If a test mostly has
a
> > pattern of
> > 
> >   SpawnAsyncTask();
> >   WaitForTaskCompleteion();
> >   SpawnNextAsyncTask();
> >   WaitForTaskCompleteion();
> > 
> > Then there really isn't much parallelism and spawning extra threads just
adds
> > overhead (esp on valgrind) + complexity/flakiness.
> > 
> > At first glance, I didn't see much coverage lost. Having said that, I don't
> > understand this test super well.  If you tell me that the assertions these
> tests
> > are guaranteeing partially rely on having real threads, then I'll believe
you
> > and add it back in. 
> > 
> > > Does your change require test code which uses TestProfile to run
> > > single-threaded? If not can we continue to use the test helper?
> > 
> > Nope...not required to be single threaded.  If you find the helper to be a
> > meaningful abstraction (regardless of whether we use real threads or not), I
> can
> > totally add it back in.  Your code, your call. :)
> 
> Right, in most cases I'd agree with you and just use single-thread, but this
> test goes through a heavily threaded class under the surface, which chains
tasks
> spanning on three threads.  And multi-threaded tests actually helped me find
out
> some edge cases while I'm developing.
> 
> Putting aside the fact that such class should be cleaned up in a simpler model
> (there's a TODO for that :)), I'd like to keep this test multi-threaded for
now.
>  (Reg: to use a helper class or not, I don't have strong opinion on it. Now
that
> we have ThreadBundle the helper class is not adding much)

Sounds good to me.  Thanks for verifying. I'll update it to use a real FILE and
IO thread on the next patch.

[Ivan Korotkov, 2013-07-12 07:42:33]

s/ivankr/nkostylev/

[Roger Tawa OOO till Jul 10th, 2013-07-12 13:52:30]

lgtm for chrome\browser\signin and google_apis

https://codereview.chromium.org/17127002/diff/5891733057437696/chrome/browser...
File chrome/browser/signin/token_service_unittest.cc (right):

https://codereview.chromium.org/17127002/diff/5891733057437696/chrome/browser...
chrome/browser/signin/token_service_unittest.cc:90: // carry on with the test.
This comment no longer applies.  Reword it say why we need to run the message
loop at this point?

[jam, 2013-07-12 16:38:33]

lgtm

https://codereview.chromium.org/17127002/diff/5891733057437696/chrome/test/ba...
File chrome/test/base/ui_test_utils.cc (right):

https://codereview.chromium.org/17127002/diff/5891733057437696/chrome/test/ba...
chrome/test/base/ui_test_utils.cc:398:
content::Source<TemplateURLService>(service));
you can use content::WindowedNotificationObserver here instead of most of this
code, i.e.

content::WindowedNotificationObserver observer(
    chrome::NOTIFICATION_TEMPLATE_URL_SERVICE_LOADED,
    content::Source<TemplateURLService>(service));
service->Load();
observer.Wait();

[Nikita (slow), 2013-07-15 08:19:37]

chrome/browser/chromeos/* lgtm

[Lei Zhang, 2013-07-15 22:29:18]

c/b/printing lgtm with nits below.

https://codereview.chromium.org/17127002/diff/5891733057437696/chrome/browser...
File
chrome/browser/printing/cloud_print/test/cloud_print_proxy_process_browsertest.cc
(right):

https://codereview.chromium.org/17127002/diff/5891733057437696/chrome/browser...
chrome/browser/printing/cloud_print/test/cloud_print_proxy_process_browsertest.cc:224:
CommandLine* command_line = CommandLine::ForCurrentProcess();
Can you just move the same code on line 215 out of the #if block instead?

https://codereview.chromium.org/17127002/diff/5891733057437696/chrome/browser...
chrome/browser/printing/cloud_print/test/cloud_print_proxy_process_browsertest.cc:304:
return content::BrowserThread::GetMessageLoopProxyForThread(
nit: Since you added "using content::BrowserThread;", you can omit content::
here and get this to fit on 1 line.

[awong, 2013-07-16 21:27:27]

Addressing comments and adjusting owners to swap out people that are OOO.

sky: ash/* and chrome/ui/*
mikewst: chrome/browser/browsing_data/*

https://codereview.chromium.org/17127002/diff/5891733057437696/chrome/browser...
File
chrome/browser/printing/cloud_print/test/cloud_print_proxy_process_browsertest.cc
(right):

https://codereview.chromium.org/17127002/diff/5891733057437696/chrome/browser...
chrome/browser/printing/cloud_print/test/cloud_print_proxy_process_browsertest.cc:224:
CommandLine* command_line = CommandLine::ForCurrentProcess();
On 2013/07/15 22:29:19, Lei Zhang wrote:
> Can you just move the same code on line 215 out of the #if block instead?

Done.

https://codereview.chromium.org/17127002/diff/5891733057437696/chrome/browser...
chrome/browser/printing/cloud_print/test/cloud_print_proxy_process_browsertest.cc:304:
return content::BrowserThread::GetMessageLoopProxyForThread(
On 2013/07/15 22:29:19, Lei Zhang wrote:
> nit: Since you added "using content::BrowserThread;", you can omit content::
> here and get this to fit on 1 line.

Done.

https://codereview.chromium.org/17127002/diff/5891733057437696/chrome/browser...
File chrome/browser/search_engines/search_provider_install_data_unittest.cc
(left):

https://codereview.chromium.org/17127002/diff/5891733057437696/chrome/browser...
chrome/browser/search_engines/search_provider_install_data_unittest.cc:277: 
On 2013/07/12 01:20:20, Peter Kasting wrote:
> Nit: Either remove all the cases of double-blank-line-between-tests in this
> file, or none of them.

Done.

https://codereview.chromium.org/17127002/diff/5891733057437696/chrome/browser...
File chrome/browser/search_engines/template_url_service_unittest.cc (right):

https://codereview.chromium.org/17127002/diff/5891733057437696/chrome/browser...
chrome/browser/search_engines/template_url_service_unittest.cc:163:
TemplateURLService::GenerateSearchURLUsingTermsData(&t_url,
On 2013/07/12 01:20:20, Peter Kasting wrote:
> Nit: Put first arg on next line with second arg, so we don't have misaligned
> lines of args

Broke the ternary into an if statement. Seemed saner.

https://codereview.chromium.org/17127002/diff/5891733057437696/chrome/browser...
File chrome/browser/signin/oauth2_token_service_unittest.cc (right):

https://codereview.chromium.org/17127002/diff/5891733057437696/chrome/browser...
chrome/browser/signin/oauth2_token_service_unittest.cc:69:
BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO))) {
On 2013/07/12 01:20:20, Peter Kasting wrote:
> Nit: I'm not sure, but I _think_ the old indenting was better.

I think you are right.

https://codereview.chromium.org/17127002/diff/5891733057437696/chrome/browser...
File chrome/browser/signin/token_service_unittest.cc (right):

https://codereview.chromium.org/17127002/diff/5891733057437696/chrome/browser...
chrome/browser/signin/token_service_unittest.cc:90: // carry on with the test.
On 2013/07/12 13:52:30, Roger Tawa wrote:
> This comment no longer applies.  Reword it say why we need to run the message
> loop at this point?

Hmm...actually it shouldn't be required. Removed.

https://codereview.chromium.org/17127002/diff/5891733057437696/chrome/test/ba...
File chrome/test/base/ui_test_utils.cc (right):

https://codereview.chromium.org/17127002/diff/5891733057437696/chrome/test/ba...
chrome/test/base/ui_test_utils.cc:398:
content::Source<TemplateURLService>(service));
On 2013/07/12 16:38:34, jam wrote:
> you can use content::WindowedNotificationObserver here instead of most of this
> code, i.e.
> 
> content::WindowedNotificationObserver observer(
>     chrome::NOTIFICATION_TEMPLATE_URL_SERVICE_LOADED,
>     content::Source<TemplateURLService>(service));
> service->Load();
> observer.Wait();

Done.

[sky, 2013-07-16 23:27:28]

ash and chrome/browser/ui LGTM

[Mike West, 2013-07-17 06:34:52]

browsingdataremover LGTM.

[kinuko, 2013-07-18 04:22:29]

sync_file_system lgtm, thanks!

https://codereview.chromium.org/17127002/diff/209001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/local_file_sync_service_unittest.cc
(right):

https://codereview.chromium.org/17127002/diff/209001/chrome/browser/sync_file...
chrome/browser/sync_file_system/local_file_sync_service_unittest.cc:104:
content::TestBrowserThreadBundle::REAL_IO_THREAD),
nit: indent

[Ben Goodger (Google), 2013-07-18 20:07:24]

browser/ui lgtm

[awong, 2013-07-19 22:19:49]

On 2013/07/18 20:07:24, Ben Goodger (Google) wrote:
> browser/ui lgtm

Looked through the various failures. mac_asan is just the bot being completely
utterly broken. linux_asan linux_chromeos, and linux_chromeos_asan didn't repro
locally. win7_aura did repro, but I also don't believe I touched any of the
code.

As is, I'm going to dump into CQ and see what it does.

[awong, 2013-07-19 23:28:09]

TBRing the following (some of your who've reviewed once are also getting listed
as TBRs for certain subdirectories):

bauerb:
  chrome/browser/managed_mode

brettw:
  chrome/browser/page_cycler
  chrome/browser/search
  chrome/browser/tab_contents

isherman:
  components/autofill

joth:
  components/navigation_interception

jyasskin:
  chrome/browser/extensions

mattm:
  chrome/browser/safe_browsing

mmenke:
  chrome/browser/captive_portal

mnissler:
  chrome/browser/chromeos/policy
  chrome/browser/policy

pkasting:
  chrome/browser/autocomplete
  chrome/browser/google

rkc:
  chrome/browser/chromeos/extensions

rlp:
  chrome/browser/spellchecker

satorux:
  chrome/browser/chromeos/

tim:
  chrome/browser/net/gaia
  chrome/browser/sync
  google_apis/gaia

xians:
  chrome/browser/speech

[commit-bot: I haz the power, 2013-07-19 23:29:33]

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

[commit-bot: I haz the power, 2013-07-20 01:05:50]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[rpetterson, 2013-07-22 21:21:40]

spellcheck LGTM

[commit-bot: I haz the power, 2013-07-22 22:49:10]

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

[mattm, 2013-07-22 23:17:02]

safe_browsing lgtm

[commit-bot: I haz the power, 2013-07-23 00:11:03]

Retried try job too often on linux_chromeos for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[awong, 2013-07-23 19:45:39]

joth:
components/navigation_interception/intercept_navigation_resource_throttle_unittest.cc

[commit-bot: I haz the power, 2013-07-23 19:47:40]

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

[joth, 2013-07-23 19:58:23]

On 2013/07/23 19:45:39, awong wrote:
> joth:
>
components/navigation_interception/intercept_navigation_resource_throttle_unittest.cc

lgtm

[commit-bot: I haz the power, 2013-07-23 20:53:18]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-07-23 22:29:27]

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

[commit-bot: I haz the power, 2013-07-23 23:39:43]

Change committed as 213272