Have AutofillManagerDelegate supply the AutofillWebDataService to core code.

Have AutofillManagerDelegate supply the AutofillWebDataService to core code.

In this change, Autofill core code now gets the AutofillWebDataService that it
should use from its embedder (i.e., AutofillManagerDelegate) rather than by
calling AutofillWebDataService::FromBrowserContext().

This change is a prelude to eliminating the
AutofillWebDataService::FromBrowserContext() method in favor of static methods
that live in the embedders as necessary.

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

[blundell, 2013-11-13 22:59:07]

Hi Ilya and Jói,

This is work-in-progress, as I still have some work to do on a couple unittests
(the TODOs in the patch). However, I'm interested in getting your feedback on
the general approach.

Thanks,

Colin

https://codereview.chromium.org/71683003/diff/1/chrome/browser/autofill/perso...
File chrome/browser/autofill/personal_data_manager_factory.cc (right):

https://codereview.chromium.org/71683003/diff/1/chrome/browser/autofill/perso...
chrome/browser/autofill/personal_data_manager_factory.cc:38:
AutofillWebDataService::FromBrowserContext(profile),
The reason that I didn't change PersonalDataManager::Init() to take in the
AutofillManagerDelegate* (as we had discussed on a previous CL IIRC) is that it
didn't seem straightforward to get at the AutofillManagerDelegate* here, unless
I'm missing something.

[Ilya Sherman, 2013-11-14 02:22:02]

Thanks, looks quite reasonable!

https://codereview.chromium.org/71683003/diff/1/chrome/browser/autofill/perso...
File chrome/browser/autofill/personal_data_manager_factory.cc (right):

https://codereview.chromium.org/71683003/diff/1/chrome/browser/autofill/perso...
chrome/browser/autofill/personal_data_manager_factory.cc:38:
AutofillWebDataService::FromBrowserContext(profile),
On 2013/11/13 22:59:07, blundell wrote:
> The reason that I didn't change PersonalDataManager::Init() to take in the
> AutofillManagerDelegate* (as we had discussed on a previous CL IIRC) is that
it
> didn't seem straightforward to get at the AutofillManagerDelegate* here,
unless
> I'm missing something.

Hmm, I see -- the PersonalDataManager is per-profile rather than
per-WebContents, which makes it hard for it to interface with the AutofillDriver
and AutofillManagerDelegate.  Alright, this seems fine in that case.

https://codereview.chromium.org/71683003/diff/1/components/autofill/core/brow...
File components/autofill/core/browser/autofill_manager_delegate.h (right):

https://codereview.chromium.org/71683003/diff/1/components/autofill/core/brow...
components/autofill/core/browser/autofill_manager_delegate.h:53:
GetAutofillWebDataService() = 0;
nit: What do you think of naming this just "GetDatabase()"?  As far as the
Autofill code is concerned, this class is the interface it uses for all DB
interactions.

https://codereview.chromium.org/71683003/diff/1/components/autofill/core/brow...
File components/autofill/core/browser/autofill_manager_unittest.cc (right):

https://codereview.chromium.org/71683003/diff/1/components/autofill/core/brow...
components/autofill/core/browser/autofill_manager_unittest.cc:82: }
nit: Why not just "using PersonalDataManager::set_autofill_webdata_service;"?

https://codereview.chromium.org/71683003/diff/1/components/autofill/core/brow...
File components/autofill/core/browser/autofill_metrics_unittest.cc (right):

https://codereview.chromium.org/71683003/diff/1/components/autofill/core/brow...
components/autofill/core/browser/autofill_metrics_unittest.cc:100:
set_autofill_webdata_service(webdata);
Ditto here.

https://codereview.chromium.org/71683003/diff/1/components/autofill/core/brow...
File components/autofill/core/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/71683003/diff/1/components/autofill/core/brow...
components/autofill/core/browser/personal_data_manager.cc:169: // WebDataService
may not be available in tests.
Out of curiousity, do you know whether this is common?  I'd love to fix this if
possible, so that the production code doesn't need to be aware of how test code
might be playing with its dependencies.

https://codereview.chromium.org/71683003/diff/1/components/autofill/core/brow...
File components/autofill/core/browser/personal_data_manager.h (right):

https://codereview.chromium.org/71683003/diff/1/components/autofill/core/brow...
components/autofill/core/browser/personal_data_manager.h:261:
scoped_refptr<AutofillWebDataService> webdata);
nit: This ought to be inlined.

https://codereview.chromium.org/71683003/diff/1/components/autofill/core/brow...
components/autofill/core/browser/personal_data_manager.h:261:
scoped_refptr<AutofillWebDataService> webdata);
nit: You're using three different names here: "autofill_webdata_service" as the
method name, "autofill_webdata" as the instance variable name, and "webdata" as
the parameter name.  Please pick a single name and use it throughout :)

https://codereview.chromium.org/71683003/diff/1/components/autofill/core/brow...
components/autofill/core/browser/personal_data_manager.h:265: // The
AutofillWebDataService that this PersonalDataManager uses.
nit: Might be more useful to document as the backing database that the
PersonalDataManager uses.

https://codereview.chromium.org/71683003/diff/1/components/autofill/core/brow...
components/autofill/core/browser/personal_data_manager.h:266:
scoped_refptr<AutofillWebDataService> autofill_webdata_;
nit: The "autofill_" prefix seems quite redundant here.  Perhaps this ought to
be named something like "web_data_service_" or even more simply as "database_"?

https://codereview.chromium.org/71683003/diff/1/components/autofill/core/brow...
File components/autofill/core/browser/test_autofill_manager_delegate.cc (right):

https://codereview.chromium.org/71683003/diff/1/components/autofill/core/brow...
components/autofill/core/browser/test_autofill_manager_delegate.cc:19: return
scoped_refptr<AutofillWebDataService>(NULL);
Optional nit: return make_scoped_refptr(NULL);

[Jói, 2013-11-14 12:31:48]

General approach looks good to me.

Cheers,
Jói

https://codereview.chromium.org/71683003/diff/1/components/autofill/core/brow...
File components/autofill/core/browser/autofill_manager_delegate.h (right):

https://codereview.chromium.org/71683003/diff/1/components/autofill/core/brow...
components/autofill/core/browser/autofill_manager_delegate.h:53:
GetAutofillWebDataService() = 0;
On 2013/11/14 02:22:02, Ilya Sherman wrote:
> nit: What do you think of naming this just "GetDatabase()"?  As far as the
> Autofill code is concerned, this class is the interface it uses for all DB
> interactions.

+1

[blundell, 2013-11-14 16:46:30]

+zea@ for //chrome/browser/sync
+benm@ for //android_webview

https://codereview.chromium.org/71683003/diff/1/components/autofill/core/brow...
File components/autofill/core/browser/autofill_manager_delegate.h (right):

https://codereview.chromium.org/71683003/diff/1/components/autofill/core/brow...
components/autofill/core/browser/autofill_manager_delegate.h:53:
GetAutofillWebDataService() = 0;
Much better, done.

On 2013/11/14 02:22:02, Ilya Sherman wrote:
> nit: What do you think of naming this just "GetDatabase()"?  As far as the
> Autofill code is concerned, this class is the interface it uses for all DB
> interactions.

https://codereview.chromium.org/71683003/diff/1/components/autofill/core/brow...
File components/autofill/core/browser/autofill_manager_unittest.cc (right):

https://codereview.chromium.org/71683003/diff/1/components/autofill/core/brow...
components/autofill/core/browser/autofill_manager_unittest.cc:82: }
On 2013/11/14 02:22:02, Ilya Sherman wrote:
> nit: Why not just "using PersonalDataManager::set_autofill_webdata_service;"?

Done.

https://codereview.chromium.org/71683003/diff/1/components/autofill/core/brow...
File components/autofill/core/browser/autofill_metrics_unittest.cc (right):

https://codereview.chromium.org/71683003/diff/1/components/autofill/core/brow...
components/autofill/core/browser/autofill_metrics_unittest.cc:100:
set_autofill_webdata_service(webdata);
On 2013/11/14 02:22:02, Ilya Sherman wrote:
> Ditto here.

Done.

https://codereview.chromium.org/71683003/diff/1/components/autofill/core/brow...
File components/autofill/core/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/71683003/diff/1/components/autofill/core/brow...
components/autofill/core/browser/personal_data_manager.cc:169: // WebDataService
may not be available in tests.
Found the offenders (at least in unit_tests) by putting an
EXPECT_TRUE(autofill_webdata) in PersonalDataManager::Init(). Filed
crbug.com/319329.

On 2013/11/14 02:22:02, Ilya Sherman wrote:
> Out of curiousity, do you know whether this is common?  I'd love to fix this
if
> possible, so that the production code doesn't need to be aware of how test
code
> might be playing with its dependencies.

https://codereview.chromium.org/71683003/diff/1/components/autofill/core/brow...
File components/autofill/core/browser/personal_data_manager.h (right):

https://codereview.chromium.org/71683003/diff/1/components/autofill/core/brow...
components/autofill/core/browser/personal_data_manager.h:261:
scoped_refptr<AutofillWebDataService> webdata);
On 2013/11/14 02:22:02, Ilya Sherman wrote:
> nit: You're using three different names here: "autofill_webdata_service" as
the
> method name, "autofill_webdata" as the instance variable name, and "webdata"
as
> the parameter name.  Please pick a single name and use it throughout :)

Done.

https://codereview.chromium.org/71683003/diff/1/components/autofill/core/brow...
components/autofill/core/browser/personal_data_manager.h:261:
scoped_refptr<AutofillWebDataService> webdata);
On 2013/11/14 02:22:02, Ilya Sherman wrote:
> nit: This ought to be inlined.

Done.

https://codereview.chromium.org/71683003/diff/1/components/autofill/core/brow...
components/autofill/core/browser/personal_data_manager.h:265: // The
AutofillWebDataService that this PersonalDataManager uses.
On 2013/11/14 02:22:02, Ilya Sherman wrote:
> nit: Might be more useful to document as the backing database that the
> PersonalDataManager uses.

Done.

https://codereview.chromium.org/71683003/diff/1/components/autofill/core/brow...
components/autofill/core/browser/personal_data_manager.h:266:
scoped_refptr<AutofillWebDataService> autofill_webdata_;
On 2013/11/14 02:22:02, Ilya Sherman wrote:
> nit: The "autofill_" prefix seems quite redundant here.  Perhaps this ought to
> be named something like "web_data_service_" or even more simply as
"database_"?

Done.

https://codereview.chromium.org/71683003/diff/120001/android_webview/native/a...
File android_webview/native/aw_autofill_manager_delegate.h (right):

https://codereview.chromium.org/71683003/diff/120001/android_webview/native/a...
android_webview/native/aw_autofill_manager_delegate.h:62: GetDatabase()
OVERRIDE;
This doesn't all fit on one line but the GetDatabase() would fit on the first
line. Don't know which is preferred.

https://codereview.chromium.org/71683003/diff/120001/components/autofill/core...
File components/autofill/core/browser/autocomplete_history_manager_unittest.cc
(left):

https://codereview.chromium.org/71683003/diff/120001/components/autofill/core...
components/autofill/core/browser/autocomplete_history_manager_unittest.cc:115:
web_data_service_ = NULL;
This doesn't seem to be necessary.

https://codereview.chromium.org/71683003/diff/120001/components/autofill/core...
File components/autofill/core/browser/autocomplete_history_manager_unittest.cc
(right):

https://codereview.chromium.org/71683003/diff/120001/components/autofill/core...
components/autofill/core/browser/autocomplete_history_manager_unittest.cc:56:
scoped_refptr<MockWebDataService> web_data_service)
Since AutocompleteHistoryManager now gets the AutofillWebDataService from the
AutofillManagerDelegate rather than from the BrowserContext, the wiring up of
the mock webdata service needs to happen through the AutofillManagerDelegate
rather than through the factory.

https://codereview.chromium.org/71683003/diff/120001/components/autofill/core...
File components/autofill/core/browser/personal_data_manager_unittest.cc (right):

https://codereview.chromium.org/71683003/diff/120001/components/autofill/core...
components/autofill/core/browser/personal_data_manager_unittest.cc:69:
ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
I took this code from similar code in
//components/autofill/core/browser/webdata/web_data_service_unittest.cc. I'm not
sure all of it is strictly necessary, but I'm not an expert here.

[Ilya Sherman, 2013-11-14 23:45:54]

https://codereview.chromium.org/71683003/diff/230001/components/autofill/core...
File components/autofill/core/browser/autocomplete_history_manager_unittest.cc
(right):

https://codereview.chromium.org/71683003/diff/230001/components/autofill/core...
components/autofill/core/browser/autocomplete_history_manager_unittest.cc:44: }
nit: Please move the closing curly brace to the previous line, now that this
method body is empty.

https://codereview.chromium.org/71683003/diff/230001/components/autofill/core...
File components/autofill/core/browser/autofill_manager_delegate.h (right):

https://codereview.chromium.org/71683003/diff/230001/components/autofill/core...
components/autofill/core/browser/autofill_manager_delegate.h:53: GetDatabase() =
0;
nit: Looks like this ought to fit on a single line now.

https://codereview.chromium.org/71683003/diff/230001/components/autofill/core...
File components/autofill/core/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/71683003/diff/230001/components/autofill/core...
components/autofill/core/browser/personal_data_manager.cc:156:
scoped_refptr<AutofillWebDataService> database,
nit: Looks like this doesn't need to wrap now.

https://codereview.chromium.org/71683003/diff/230001/components/autofill/core...
File components/autofill/core/browser/personal_data_manager.h (right):

https://codereview.chromium.org/71683003/diff/230001/components/autofill/core...
components/autofill/core/browser/personal_data_manager.h:21: #include
"components/autofill/core/browser/webdata/autofill_webdata_service.h"
These should remain as forward-declarations in the header file.

https://codereview.chromium.org/71683003/diff/230001/components/autofill/core...
File components/autofill/core/browser/personal_data_manager_unittest.cc (right):

https://codereview.chromium.org/71683003/diff/230001/components/autofill/core...
components/autofill/core/browser/personal_data_manager_unittest.cc:134:
content::TestBrowserThread db_thread_;
We should really use a TestBrowserThreadBundle rather than carefully pumping
these.  IMO we should just use the profile's AutofillWebDataService for now; and
then clean up the test in a separate, more focused change.

https://codereview.chromium.org/71683003/diff/230001/components/autofill/core...
components/autofill/core/browser/personal_data_manager_unittest.cc:137:
scoped_refptr<WebDatabaseService> wdbs_;
Please give these more descriptive names, e.g. "autofill_database_service" and
"web_database".  (Unlike everywhere else that uses AutofillWebDataService, I do
think it's useful to include the "autofill_" prefix here, to contrast vs. the
"web_" prefix for the other one.)

[Nicolas Zea, 2013-11-15 01:20:59]

sync LGTM

[blundell, 2013-11-15 16:04:53]

Thanks for the reviews. I think the only remaining open question is the
PersonalDataManager unittest.

https://codereview.chromium.org/71683003/diff/230001/components/autofill/core...
File components/autofill/core/browser/autocomplete_history_manager_unittest.cc
(right):

https://codereview.chromium.org/71683003/diff/230001/components/autofill/core...
components/autofill/core/browser/autocomplete_history_manager_unittest.cc:44: }
On 2013/11/14 23:45:55, Ilya Sherman wrote:
> nit: Please move the closing curly brace to the previous line, now that this
> method body is empty.

Done.

https://codereview.chromium.org/71683003/diff/230001/components/autofill/core...
File components/autofill/core/browser/autofill_manager_delegate.h (right):

https://codereview.chromium.org/71683003/diff/230001/components/autofill/core...
components/autofill/core/browser/autofill_manager_delegate.h:53: GetDatabase() =
0;
On 2013/11/14 23:45:55, Ilya Sherman wrote:
> nit: Looks like this ought to fit on a single line now.

Done.

https://codereview.chromium.org/71683003/diff/230001/components/autofill/core...
File components/autofill/core/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/71683003/diff/230001/components/autofill/core...
components/autofill/core/browser/personal_data_manager.cc:156:
scoped_refptr<AutofillWebDataService> database,
On 2013/11/14 23:45:55, Ilya Sherman wrote:
> nit: Looks like this doesn't need to wrap now.

Done.

https://codereview.chromium.org/71683003/diff/230001/components/autofill/core...
File components/autofill/core/browser/personal_data_manager.h (right):

https://codereview.chromium.org/71683003/diff/230001/components/autofill/core...
components/autofill/core/browser/personal_data_manager.h:21: #include
"components/autofill/core/browser/webdata/autofill_webdata_service.h"
Done for PrefService. The other two changes are needed due to the
scoped_(ref)ptr operations now happening in the .h file.

On 2013/11/14 23:45:55, Ilya Sherman wrote:
> These should remain as forward-declarations in the header file.

https://codereview.chromium.org/71683003/diff/230001/components/autofill/core...
File components/autofill/core/browser/personal_data_manager_unittest.cc (right):

https://codereview.chromium.org/71683003/diff/230001/components/autofill/core...
components/autofill/core/browser/personal_data_manager_unittest.cc:134:
content::TestBrowserThread db_thread_;
I looked at using the //chrome-level concepts for now. I would need to create a
TabAutofillManagerDelegate, for which this test would need to become a
ChromeTestRenderViewHostHarness so that I had a WebContents. Then the test would
have a TestBrowserThreadBundle, so I would need to eliminate the manually-setup
MessageLoops used here. I'd rather not do those changes, because it's just
taking this test in the wrong direction from eventually having it be able to be
shared by iOS (even the usage of TestBrowserThreadBundle, since that's a
//content-level concept).

What do you think?

On 2013/11/14 23:45:55, Ilya Sherman wrote:
> We should really use a TestBrowserThreadBundle rather than carefully pumping
> these.  IMO we should just use the profile's AutofillWebDataService for now;
and
> then clean up the test in a separate, more focused change.

https://codereview.chromium.org/71683003/diff/230001/components/autofill/core...
components/autofill/core/browser/personal_data_manager_unittest.cc:137:
scoped_refptr<WebDatabaseService> wdbs_;
On 2013/11/14 23:45:55, Ilya Sherman wrote:
> Please give these more descriptive names, e.g. "autofill_database_service" and
> "web_database".  (Unlike everywhere else that uses AutofillWebDataService, I
do
> think it's useful to include the "autofill_" prefix here, to contrast vs. the
> "web_" prefix for the other one.)

Done.

[Jói, 2013-11-15 16:51:33]

https://codereview.chromium.org/71683003/diff/230001/components/autofill/core...
File components/autofill/core/browser/personal_data_manager_unittest.cc (right):

https://codereview.chromium.org/71683003/diff/230001/components/autofill/core...
components/autofill/core/browser/personal_data_manager_unittest.cc:134:
content::TestBrowserThread db_thread_;
On 2013/11/15 16:04:54, blundell wrote:
> I looked at using the //chrome-level concepts for now. I would need to create
a
> TabAutofillManagerDelegate, for which this test would need to become a
> ChromeTestRenderViewHostHarness so that I had a WebContents. Then the test
would
> have a TestBrowserThreadBundle, so I would need to eliminate the
manually-setup
> MessageLoops used here. I'd rather not do those changes, because it's just
> taking this test in the wrong direction from eventually having it be able to
be
> shared by iOS (even the usage of TestBrowserThreadBundle, since that's a
> //content-level concept).
> 
> What do you think?
> 
> On 2013/11/14 23:45:55, Ilya Sherman wrote:
> > We should really use a TestBrowserThreadBundle rather than carefully pumping
> > these.  IMO we should just use the profile's AutofillWebDataService for now;
> and
> > then clean up the test in a separate, more focused change.
> 

I agree, don't go back to concepts only available in //chrome.

The ideal would be if you can get rid of the TestingProfile and have this built
in the components_unittests target instead of the unit_tests target in //chrome.

[Ilya Sherman, 2013-11-15 23:10:20]

Alright, LGTM.  Please do make sure to come back to clean up the tests sooner
rather than later.

[blundell, 2013-11-18 09:46:50]

benm@: ping.

[joth, 2013-11-18 19:36:52]

aw/ lgtm rubberstamp, but one drive-by on the API itself

https://codereview.chromium.org/71683003/diff/420001/android_webview/native/a...
File android_webview/native/aw_autofill_manager_delegate.h (right):

https://codereview.chromium.org/71683003/diff/420001/android_webview/native/a...
android_webview/native/aw_autofill_manager_delegate.h:62: GetDatabase()
OVERRIDE;
this thread seems to suggest returning raw pointer is safer than returning
refptr
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/uuaKaTzaCwE/dis...

[blundell, 2013-11-19 11:17:13]

https://codereview.chromium.org/71683003/diff/420001/android_webview/native/a...
File android_webview/native/aw_autofill_manager_delegate.h (right):

https://codereview.chromium.org/71683003/diff/420001/android_webview/native/a...
android_webview/native/aw_autofill_manager_delegate.h:62: GetDatabase()
OVERRIDE;
Thanks. I'd prefer to keep that change separate from this refactoring to limit
the number of moving parts. I filed crbug.com/321059.

On 2013/11/18 19:36:53, joth wrote:
> this thread seems to suggest returning raw pointer is safer than returning
> refptr
>
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/uuaKaTzaCwE/dis...

[commit-bot: I haz the power, 2013-11-19 11:18:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/71683003/420001

[commit-bot: I haz the power, 2013-11-19 14:51:27]

Change committed as 235994