Componentize several Autofill unit tests.

Componentize several Autofill unit tests.

The PersonalDataManager unittests are the only ones requiring non-trivial
refactoring.

BUG=303083
R=isherman@chromium.org
TBR=jochen

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255541

[blundell, 2014-03-05 15:55:00]

Some questions about the PersonalDataManager unittests...

https://codereview.chromium.org/180953004/diff/1/components/autofill/core/bro...
File components/autofill/core/browser/personal_data_manager_unittest.cc (left):

https://codereview.chromium.org/180953004/diff/1/components/autofill/core/bro...
components/autofill/core/browser/personal_data_manager_unittest.cc:39:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
Is it important to preserve this DCHECK in some way here?

https://codereview.chromium.org/180953004/diff/1/components/autofill/core/bro...
components/autofill/core/browser/personal_data_manager_unittest.cc:105: //
Schedule another task on the DB thread to notify us that it's safe to
Related to the question that I have below, this code seems to indicate that
db_thread_ was using a different MessageLoop before. Is it important to preserve
that?

https://codereview.chromium.org/180953004/diff/1/components/autofill/core/bro...
components/autofill/core/browser/personal_data_manager_unittest.cc:139:
content::TestBrowserThread db_thread_;
I can't remember how TestBrowserThread works off the top of my head -- were
these using the same or different message loops before? If they were different,
is it important to preserve that?

[Ilya Sherman, 2014-03-05 23:59:43]

LGTM w/ nit addressed.  Thanks!

https://codereview.chromium.org/180953004/diff/1/components/autofill/core/bro...
File components/autofill/core/browser/personal_data_manager_unittest.cc (left):

https://codereview.chromium.org/180953004/diff/1/components/autofill/core/bro...
components/autofill/core/browser/personal_data_manager_unittest.cc:39:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2014/03/05 15:55:01, blundell wrote:
> Is it important to preserve this DCHECK in some way here?

Nah, though it might be worth renaming the action to something like
"QuitMainMessageLoop"

https://codereview.chromium.org/180953004/diff/1/components/autofill/core/bro...
components/autofill/core/browser/personal_data_manager_unittest.cc:105: //
Schedule another task on the DB thread to notify us that it's safe to
On 2014/03/05 15:55:01, blundell wrote:
> Related to the question that I have below, this code seems to indicate that
> db_thread_ was using a different MessageLoop before. Is it important to
preserve
> that?

No, this is a test pattern that's discouraged these days, so it's great that
you're cleaning it up :)

https://codereview.chromium.org/180953004/diff/1/components/autofill/core/bro...
components/autofill/core/browser/personal_data_manager_unittest.cc:139:
content::TestBrowserThread db_thread_;
On 2014/03/05 15:55:01, blundell wrote:
> I can't remember how TestBrowserThread works off the top of my head -- were
> these using the same or different message loops before? If they were
different,
> is it important to preserve that?

These were previously different message loops, but it's good that you're
replacing them with just a single loop instead.  It might be appropriate to add
a TestBrowserThreadBundle to this test, though I suppose if things work without
it, then even better :)

https://codereview.chromium.org/180953004/diff/1/components/autofill/core/bro...
File components/autofill/core/browser/personal_data_manager_unittest.cc (right):

https://codereview.chromium.org/180953004/diff/1/components/autofill/core/bro...
components/autofill/core/browser/personal_data_manager_unittest.cc:93: void
ResetPersonalDataManager(bool is_incognito) {
nit: Please define an enum for this parameter, rather than passing a boolean. 
An enum makes the semantics of call sites easier to understand.

[blundell, 2014-03-06 09:32:31]

Thanks!

https://codereview.chromium.org/180953004/diff/1/components/autofill/core/bro...
File components/autofill/core/browser/personal_data_manager_unittest.cc (left):

https://codereview.chromium.org/180953004/diff/1/components/autofill/core/bro...
components/autofill/core/browser/personal_data_manager_unittest.cc:39:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2014/03/05 23:59:44, Ilya Sherman wrote:
> On 2014/03/05 15:55:01, blundell wrote:
> > Is it important to preserve this DCHECK in some way here?
> 
> Nah, though it might be worth renaming the action to something like
> "QuitMainMessageLoop"

Done.

https://codereview.chromium.org/180953004/diff/1/components/autofill/core/bro...
components/autofill/core/browser/personal_data_manager_unittest.cc:139:
content::TestBrowserThread db_thread_;
I can't use TestBrowserThreadBundle here in any case as it's in //content.

On 2014/03/05 23:59:44, Ilya Sherman wrote:
> On 2014/03/05 15:55:01, blundell wrote:
> > I can't remember how TestBrowserThread works off the top of my head -- were
> > these using the same or different message loops before? If they were
> different,
> > is it important to preserve that?
> 
> These were previously different message loops, but it's good that you're
> replacing them with just a single loop instead.  It might be appropriate to
add
> a TestBrowserThreadBundle to this test, though I suppose if things work
without
> it, then even better :)

https://codereview.chromium.org/180953004/diff/1/components/autofill/core/bro...
File components/autofill/core/browser/personal_data_manager_unittest.cc (right):

https://codereview.chromium.org/180953004/diff/1/components/autofill/core/bro...
components/autofill/core/browser/personal_data_manager_unittest.cc:93: void
ResetPersonalDataManager(bool is_incognito) {
On 2014/03/05 23:59:44, Ilya Sherman wrote:
> nit: Please define an enum for this parameter, rather than passing a boolean. 
> An enum makes the semantics of call sites easier to understand.

Done.

[blundell, 2014-03-06 15:42:57]

Hi Ilya,

Could you take another look over the changes to autofill_test_utils and
//chrome? I had taken out the call to autofill::test::DisableSystemServices, but
it's actually necessary. I changed that function to take in a PrefService to
operate on instead of a BrowserContext.

Thanks,

Colin

[Ilya Sherman, 2014-03-07 01:52:15]

LGTM, thanks Colin!

https://codereview.chromium.org/180953004/diff/40001/components/autofill/core...
File components/autofill/core/browser/autofill_test_utils.cc (right):

https://codereview.chromium.org/180953004/diff/40001/components/autofill/core...
components/autofill/core/browser/autofill_test_utils.cc:216: }
nit: No need for curlies anymore.

[blundell, 2014-03-07 06:24:18]

https://codereview.chromium.org/180953004/diff/40001/components/autofill/core...
File components/autofill/core/browser/autofill_test_utils.cc (right):

https://codereview.chromium.org/180953004/diff/40001/components/autofill/core...
components/autofill/core/browser/autofill_test_utils.cc:216: }
On 2014/03/07 01:52:16, Ilya Sherman wrote:
> nit: No need for curlies anymore.

Done.

[blundell, 2014-03-07 06:47:50]

TBR=jochen for //chrome

[blundell, 2014-03-07 08:30:01]

Committed patchset #4 manually as r255541 (presubmit successful).