Move UI dependency from WebDataService to WebDataServiceFactory.

Move UI dependency from WebDataService to WebDataServiceFactory.

Depends on https://codereview.chromium.org/12543034/

TBR=isherman@chromium.org
BUG=181277
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=189469

[Jói, 2013-03-18 14:56:12]

caitkp: Main reviewer.
dhollowa: OWNERS stamp for chrome/browser/webdata.

Cheers,
Jói

[Cait (Slow), 2013-03-18 16:16:37]

LG overall. I think you'll probably need to update
password_store_win_unittest.cc too, as it also creates a WebDataService.

https://codereview.chromium.org/12780012/diff/1/chrome/browser/api/webdata/we...
File chrome/browser/api/webdata/web_data_service_base.h (right):

https://codereview.chromium.org/12780012/diff/1/chrome/browser/api/webdata/we...
chrome/browser/api/webdata/web_data_service_base.h:35: typedef
base::Callback<void(sql::InitStatus)> ProfileErrorCallback;
ATM we have a WebDataServiceBase::ProfileErrorCallback and a
WebDatabaseService::InitCallback type which are defined identically. We should
probably merge them and name them something more general, but I think this can
wait for a later CL.

https://codereview.chromium.org/12780012/diff/1/chrome/browser/api/webdata/we...
chrome/browser/api/webdata/web_data_service_base.h:38: // ||callback.is_null()|
evaluates to true.
nit: |callback.is_null() --> !callback.is_null()

https://codereview.chromium.org/12780012/diff/1/components/autofill/browser/a...
File components/autofill/browser/autocomplete_history_manager_unittest.cc
(right):

https://codereview.chromium.org/12780012/diff/1/components/autofill/browser/a...
components/autofill/browser/autocomplete_history_manager_unittest.cc:36: :
WebDataService(WebDataServiceBase::ProfileErrorCallback()) {
I wonder if it's worth having a default ctor on WDS that constructs the WDS with
a NULL callback (and  eventually a NULL WebDatabaseService), to avoid
continually having to touch these same unit_tests?

[dhollowa, 2013-03-18 19:02:10]

lgtm

[Jói, 2013-03-18 23:21:23]

Thanks guys. Cait, I uploaded a new patch to address your comments.

Cheers,
Jói

https://codereview.chromium.org/12780012/diff/1/chrome/browser/api/webdata/we...
File chrome/browser/api/webdata/web_data_service_base.h (right):

https://codereview.chromium.org/12780012/diff/1/chrome/browser/api/webdata/we...
chrome/browser/api/webdata/web_data_service_base.h:35: typedef
base::Callback<void(sql::InitStatus)> ProfileErrorCallback;
On 2013/03/18 16:16:37, caitkp wrote:
> ATM we have a WebDataServiceBase::ProfileErrorCallback and a
> WebDatabaseService::InitCallback type which are defined identically. We should
> probably merge them and name them something more general, but I think this can
> wait for a later CL. 

Yes, I think we should wait for all the pieces to fall in place before deciding
whether to combine them. I've added a TODO here.

https://codereview.chromium.org/12780012/diff/1/chrome/browser/api/webdata/we...
chrome/browser/api/webdata/web_data_service_base.h:38: // ||callback.is_null()|
evaluates to true.
On 2013/03/18 16:16:37, caitkp wrote:
> nit: |callback.is_null() --> !callback.is_null()

Thanks for the catch. Switched "evaluates to true" to "evaluates to false"
instead.

https://codereview.chromium.org/12780012/diff/1/components/autofill/browser/a...
File components/autofill/browser/autocomplete_history_manager_unittest.cc
(right):

https://codereview.chromium.org/12780012/diff/1/components/autofill/browser/a...
components/autofill/browser/autocomplete_history_manager_unittest.cc:36: :
WebDataService(WebDataServiceBase::ProfileErrorCallback()) {
On 2013/03/18 16:16:37, caitkp wrote:
> I wonder if it's worth having a default ctor on WDS that constructs the WDS
with
> a NULL callback (and  eventually a NULL WebDatabaseService), to avoid
> continually having to touch these same unit_tests?

Added such a protected constructor, so mock versions of WebDataService don't
need to specify this.

[Cait (Slow), 2013-03-19 05:24:40]

lgtm

[Jói, 2013-03-20 23:30:39]

TBR=isherman@chromium.org for trivial usage update to
password_store_win_unittest.cc

I'm about to land this manually. I had to work around problems with using the
trybots on a pipelined change (normally I would use [ git try ] instead of [ git
cl try ] but that hasn't been working for me), so the trybot results are on a
separate code review that rolls up this patch and the previous patch in the
pipeline, which just landed: https://codereview.chromium.org/12957002/

Cheers,
Jói

[Jói, 2013-03-20 23:36:12]

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