Moved WebDataService to ProfileKeyedService

Moved WebDataService to ProfileKeyedService

James:
chrome\browser\ui\intents
Peter:
chrome\browser\ui\search_engines
chrome\browser\search_engines
Nicolas:
chrome\browser\sync
Rachel:
chrome\browser\profiles and the whole cl

BUG=112234
TEST=unit-tests

TBR=jhawkins@chromium.org,pkasting@chromium.org,zea@chromium.org,rlp@chromium.org,isherman@chromium.org

[commit-bot: I haz the power, 2012-04-06 17:51:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/georgey@chromium.org/10006037/1

[commit-bot: I haz the power, 2012-04-06 19:23:15]

Try job failure for 10006037-1 (retry) on mac_rel for step "unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-04-06 21:18:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/georgey@chromium.org/10006037/8001

[commit-bot: I haz the power, 2012-04-06 22:49:19]

Try job failure for 10006037-8001 (retry) on mac_rel for step "unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-04-06 22:54:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/georgey@chromium.org/10006037/8001

[commit-bot: I haz the power, 2012-04-07 00:25:36]

Try job failure for 10006037-8001 (retry) on mac_rel for step "unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-04-07 01:35:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/georgey@chromium.org/10006037/8001

[commit-bot: I haz the power, 2012-04-07 03:29:10]

Try job failure for 10006037-8001 (retry) (retry) on mac_rel for step
"unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-04-07 07:14:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/georgey@chromium.org/10006037/8001

[commit-bot: I haz the power, 2012-04-07 10:23:22]

Try job failure for 10006037-8001 (retry) (retry) on mac_rel for step
"unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-04-09 17:02:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/georgey@chromium.org/10006037/8001

[commit-bot: I haz the power, 2012-04-09 18:24:52]

Try job failure for 10006037-8001 (retry) on mac_rel for step "unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[GeorgeY, 2012-04-17 20:41:41]

Added Scott for chrome/browser/tabs/tab_strip_model_unittest.cc review

[sky, 2012-04-17 20:45:50]

https://chromiumcodereview.appspot.com/10006037/diff/34001/chrome/browser/tab...
File chrome/browser/tabs/tab_strip_model_unittest.cc (right):

https://chromiumcodereview.appspot.com/10006037/diff/34001/chrome/browser/tab...
chrome/browser/tabs/tab_strip_model_unittest.cc:136: :
browser_thread_(BrowserThread::UI, &message_loop_),
Promote this code to either ChromeRenderViewHostTestHarness , or a new
superclass that takes care of it.

[GeorgeY, 2012-04-17 20:52:24]

https://chromiumcodereview.appspot.com/10006037/diff/34001/chrome/browser/tab...
File chrome/browser/tabs/tab_strip_model_unittest.cc (right):

https://chromiumcodereview.appspot.com/10006037/diff/34001/chrome/browser/tab...
chrome/browser/tabs/tab_strip_model_unittest.cc:136: :
browser_thread_(BrowserThread::UI, &message_loop_),
On 2012/04/17 20:45:51, sky wrote:
> Promote this code to either ChromeRenderViewHostTestHarness , or a new
> superclass that takes care of it.

Done already - forgot about this test - it is reverted now

[sky, 2012-04-17 20:59:20]

https://chromiumcodereview.appspot.com/10006037/diff/39002/chrome/test/base/c...
File chrome/test/base/chrome_render_view_host_test_harness.cc (right):

https://chromiumcodereview.appspot.com/10006037/diff/39002/chrome/test/base/c...
chrome/test/base/chrome_render_view_host_test_harness.cc:46:
WebDataServiceFactory::GetInstance()->SetTestingFactory(
Does this prevent anyone from creating a ChromeRenderViewHostTest that also uses
webdb?

[GeorgeY, 2012-04-17 21:05:18]

In any case creating db thread and creating/closing database slows it down, and
this is a cleaner way -  if somebody needs it it can be enabled for that test
only.

https://chromiumcodereview.appspot.com/10006037/diff/39002/chrome/test/base/c...
File chrome/test/base/chrome_render_view_host_test_harness.cc (right):

https://chromiumcodereview.appspot.com/10006037/diff/39002/chrome/test/base/c...
chrome/test/base/chrome_render_view_host_test_harness.cc:46:
WebDataServiceFactory::GetInstance()->SetTestingFactory(
On 2012/04/17 20:59:20, sky wrote:
> Does this prevent anyone from creating a ChromeRenderViewHostTest that also
uses
> webdb?
Not a single test does apparently. I one would do it in the future, it can be
overriden.

[sky, 2012-04-17 21:29:15]

LGTM

[Ilya Sherman, 2012-04-17 21:32:15]

Thanks for going through all of these tests, George.

https://chromiumcodereview.appspot.com/10006037/diff/39002/chrome/browser/aut...
File chrome/browser/autofill/autofill_external_delegate_unittest.cc (right):

https://chromiumcodereview.appspot.com/10006037/diff/39002/chrome/browser/aut...
chrome/browser/autofill/autofill_external_delegate_unittest.cc:10: #include
"base/synchronization/waitable_event.h"
nit: Why is this needed?

https://chromiumcodereview.appspot.com/10006037/diff/39002/chrome/browser/ext...
File chrome/browser/extensions/app_notify_channel_setup_unittest.cc (right):

https://chromiumcodereview.appspot.com/10006037/diff/39002/chrome/browser/ext...
chrome/browser/extensions/app_notify_channel_setup_unittest.cc:248: done.Wait();
Why is this needed?  It's not clear to me what we're actually waiting on.

https://chromiumcodereview.appspot.com/10006037/diff/39002/chrome/browser/pro...
File chrome/browser/profiles/profile_impl.cc (right):

https://chromiumcodereview.appspot.com/10006037/diff/39002/chrome/browser/pro...
chrome/browser/profiles/profile_impl.cc:489: // HistoryService maintains threads
for background
nit: "HistoryService" -> "The HistoryService".  Also, please re-wrap the comment
lines here.

https://chromiumcodereview.appspot.com/10006037/diff/39002/chrome/browser/ui/...
File chrome/browser/ui/views/first_run_bubble_unittest.cc (right):

https://chromiumcodereview.appspot.com/10006037/diff/39002/chrome/browser/ui/...
chrome/browser/ui/views/first_run_bubble_unittest.cc:42:
WebDataServiceFactory::GetInstance()->SetTestingFactory(&profile_, NULL);
Given that all of these individual tests need to add this bit of logic to avoid
failures, it would be nice to include this snippet in a comment by the DCHECK in
the WebDataService constructor.

[commit-bot: I haz the power, 2012-04-18 17:19:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/georgey@chromium.org/10006037/15018

[commit-bot: I haz the power, 2012-04-18 18:14:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/georgey@chromium.org/10006037/39007

[GeorgeY, 2012-04-18 18:14:46]

https://chromiumcodereview.appspot.com/10006037/diff/39002/chrome/browser/aut...
File chrome/browser/autofill/autofill_external_delegate_unittest.cc (right):

https://chromiumcodereview.appspot.com/10006037/diff/39002/chrome/browser/aut...
chrome/browser/autofill/autofill_external_delegate_unittest.cc:10: #include
"base/synchronization/waitable_event.h"
On 2012/04/17 21:32:15, Ilya Sherman wrote:
> nit: Why is this needed?

not anymore :). Removed

https://chromiumcodereview.appspot.com/10006037/diff/39002/chrome/browser/ext...
File chrome/browser/extensions/app_notify_channel_setup_unittest.cc (right):

https://chromiumcodereview.appspot.com/10006037/diff/39002/chrome/browser/ext...
chrome/browser/extensions/app_notify_channel_setup_unittest.cc:248: done.Wait();
On 2012/04/17 21:32:15, Ilya Sherman wrote:
> Why is this needed?  It's not clear to me what we're actually waiting on.
Please read the comment - it basically forces all pending tasks on db thread to
complete

https://chromiumcodereview.appspot.com/10006037/diff/39002/chrome/browser/pro...
File chrome/browser/profiles/profile_impl.cc (right):

https://chromiumcodereview.appspot.com/10006037/diff/39002/chrome/browser/pro...
chrome/browser/profiles/profile_impl.cc:489: // HistoryService maintains threads
for background
On 2012/04/17 21:32:15, Ilya Sherman wrote:
> nit: "HistoryService" -> "The HistoryService".  Also, please re-wrap the
comment
> lines here.

Done.

https://chromiumcodereview.appspot.com/10006037/diff/39002/chrome/browser/ui/...
File chrome/browser/ui/views/first_run_bubble_unittest.cc (right):

https://chromiumcodereview.appspot.com/10006037/diff/39002/chrome/browser/ui/...
chrome/browser/ui/views/first_run_bubble_unittest.cc:42:
WebDataServiceFactory::GetInstance()->SetTestingFactory(&profile_, NULL);
On 2012/04/17 21:32:15, Ilya Sherman wrote:
> Given that all of these individual tests need to add this bit of logic to
avoid
> failures, it would be nice to include this snippet in a comment by the DCHECK
in
> the WebDataService constructor.

Done.

[Ilya Sherman, 2012-04-18 18:22:25]

https://chromiumcodereview.appspot.com/10006037/diff/39002/chrome/browser/ext...
File chrome/browser/extensions/app_notify_channel_setup_unittest.cc (right):

https://chromiumcodereview.appspot.com/10006037/diff/39002/chrome/browser/ext...
chrome/browser/extensions/app_notify_channel_setup_unittest.cc:248: done.Wait();
On 2012/04/18 18:14:46, GeorgeY wrote:
> On 2012/04/17 21:32:15, Ilya Sherman wrote:
> > Why is this needed?  It's not clear to me what we're actually waiting on.
> Please read the comment - it basically forces all pending tasks on db thread
to
> complete

So, could we just replace this with a call to MessageLoop::RunAllPending()?

More importantly, though: What are the pending tasks that we need to wait on,
and who's adding them into the mix?  How are you guaranteeing that there are no
more pending tasks added after we run all the currently pending tasks?

[Ilya Sherman, 2012-04-18 18:25:05]

https://chromiumcodereview.appspot.com/10006037/diff/39002/chrome/browser/ui/...
File chrome/browser/ui/views/first_run_bubble_unittest.cc (right):

https://chromiumcodereview.appspot.com/10006037/diff/39002/chrome/browser/ui/...
chrome/browser/ui/views/first_run_bubble_unittest.cc:42:
WebDataServiceFactory::GetInstance()->SetTestingFactory(&profile_, NULL);
On 2012/04/18 18:14:46, GeorgeY wrote:
> On 2012/04/17 21:32:15, Ilya Sherman wrote:
> > Given that all of these individual tests need to add this bit of logic to
> avoid
> > failures, it would be nice to include this snippet in a comment by the
DCHECK
> in
> > the WebDataService constructor.
> 
> Done.

I actually meant the full snippet, including
"WebDataServiceFactory::GetInstance()->SetTestingFactory(&profile_, NULL);".  If
I were writing a new test and hit this DCHECK, I doubt that I would immediately
know how to properly mock out the WDS.  Having the boilerplate snippet right
there for copy/paste would save the author of any such new test a lot of time :)

[commit-bot: I haz the power, 2012-04-18 19:32:17]

Try job failure for 10006037-39007 (retry) on linux_rel for step "unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[GeorgeY, 2012-04-19 00:58:40]

https://chromiumcodereview.appspot.com/10006037/diff/39002/chrome/browser/ext...
File chrome/browser/extensions/app_notify_channel_setup_unittest.cc (right):

https://chromiumcodereview.appspot.com/10006037/diff/39002/chrome/browser/ext...
chrome/browser/extensions/app_notify_channel_setup_unittest.cc:248: done.Wait();
On 2012/04/18 18:22:26, Ilya Sherman wrote:
> On 2012/04/18 18:14:46, GeorgeY wrote:
> > On 2012/04/17 21:32:15, Ilya Sherman wrote:
> > > Why is this needed?  It's not clear to me what we're actually waiting on.
> > Please read the comment - it basically forces all pending tasks on db thread
> to
> > complete
> 
> So, could we just replace this with a call to MessageLoop::RunAllPending()?
> 
> More importantly, though: What are the pending tasks that we need to wait on,
> and who's adding them into the mix?  How are you guaranteeing that there are
no
> more pending tasks added after we run all the currently pending tasks?
Need to have access to db_thread_ MessageLoop.
WebDataService, when deleted from UI thread (main thread in test) schedules
actual closing of the DB on db thread.

https://chromiumcodereview.appspot.com/10006037/diff/39002/chrome/browser/ui/...
File chrome/browser/ui/views/first_run_bubble_unittest.cc (right):

https://chromiumcodereview.appspot.com/10006037/diff/39002/chrome/browser/ui/...
chrome/browser/ui/views/first_run_bubble_unittest.cc:42:
WebDataServiceFactory::GetInstance()->SetTestingFactory(&profile_, NULL);
On 2012/04/18 18:25:05, Ilya Sherman wrote:
> On 2012/04/18 18:14:46, GeorgeY wrote:
> > On 2012/04/17 21:32:15, Ilya Sherman wrote:
> > > Given that all of these individual tests need to add this bit of logic to
> > avoid
> > > failures, it would be nice to include this snippet in a comment by the
> DCHECK
> > in
> > > the WebDataService constructor.
> > 
> > Done.
> 
> I actually meant the full snippet, including
> "WebDataServiceFactory::GetInstance()->SetTestingFactory(&profile_, NULL);". 
If
> I were writing a new test and hit this DCHECK, I doubt that I would
immediately
> know how to properly mock out the WDS.  Having the boilerplate snippet right
> there for copy/paste would save the author of any such new test a lot of time
:)

Done.

[commit-bot: I haz the power, 2012-04-19 00:58:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/georgey@chromium.org/10006037/45001

[commit-bot: I haz the power, 2012-04-19 02:03:27]

Try job failure for 10006037-45001 (retry) on linux_rel for step "unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-04-19 18:34:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/georgey@chromium.org/10006037/54001

[commit-bot: I haz the power, 2012-04-19 20:44:21]

Try job failure for 10006037-54001 (retry) on linux_rel for step "unit_tests".
It's a second try, previously, steps "browser_tests, unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-04-19 22:56:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/georgey@chromium.org/10006037/65001

[commit-bot: I haz the power, 2012-04-19 23:55:10]

Try job failure for 10006037-65001 (retry) on linux_rel for step "unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...