Introduce new component webdata_services

Introduce new component webdata_services

Move WebDataServiceWrapper into its own component webdata_services so
that the code can be shared with iOS.

The code cannot go into webdata component since it depends on other
components (autofill, password_manager, search_engine, signin) that
depends on webdata (as they implements the WebDataService interface).

BUG=437508
Committed: https://crrev.com/cb955cd06650ee281f745b5a86aff7864fe44066
Cr-Commit-Position: refs/heads/master@{#308451}

[sdefresne, 2014-12-04 13:36:14]

sdefresne@chromium.org changed reviewers:
+ blundell@chromium.org, erikwright@chromium.org, pkasting@chromium.org,
thakis@chromium.org

[sdefresne, 2014-12-04 13:36:15]

pkasting: please take a look
blundell: OWNERS of //components for the addition of the new component and DEPS

erikwright: for the new DEPS on //sql
thakis: for the new DEPS on //ui

[blundell, 2014-12-04 13:44:42]

//components lgtm

I wonder if we could come up with a more descriptive name for the component and
maybe even the classes in a followup. webdata_wrapper seems a little better to
me than webdata_services. or maybe user_database since basically what this
component is doing is associating a bunch of database instances with a user
(i.e., a profile)? I present these as suggestions, but leave it up to Peter.

[sdefresne, 2014-12-04 17:21:51]

sdefresne@chromium.org changed reviewers:
- thakis@chromium.org

[sdefresne, 2014-12-04 17:22:30]

On 2014/12/04 at 17:21:51, sdefresne wrote:
> sdefresne@chromium.org changed reviewers:
> - thakis@chromium.org

The dependency on //ui was incorrect, thakis@ there is no need for you to review
this CL. Sorry.

[erikwright (departed), 2014-12-05 16:56:24]

deps on sql/ LGTM

[Peter Kasting, 2014-12-05 21:47:56]

LGTM

https://codereview.chromium.org/777863002/diff/20001/chrome/browser/webdata/w...
File chrome/browser/webdata/web_data_service_factory.cc (right):

https://codereview.chromium.org/777863002/diff/20001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.cc:28: using
autofill::AutofillWebDataBackend;
Nit: Avoid using directives unless they noticeably improve readability.  In this
case, you can just qualify this directly below.  You could probably also
eliminate at least the AutofillWebDataService using directive as well.

https://codereview.chromium.org/777863002/diff/20001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.cc:51: NOTREACHED() << "Unknown
WebDataServiceWrapper::ErrorType: " << error_type;
Nit: Please move these two lines to a default clause, since you're not
explicitly listing every type of wrapper error in the switch.

https://codereview.chromium.org/777863002/diff/20001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.cc:58:
ShowProfileErrorDialog(ProfileErrorFromWebDataServiceWrapperError(error_type),
Nit: Don't change the wrapping of this call; the old wrapping was more correct
than the new.

https://codereview.chromium.org/777863002/diff/20001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.cc:128: // |wrapper| can be
nullptr in Incognito mode.
Nit: Say "null" instead of "nullptr" in these comments.

https://codereview.chromium.org/777863002/diff/20001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.cc:130: :
scoped_refptr<AutofillWebDataService>(nullptr);
Nit: Don't change the wrapping of these expressions; the old wrapping was more
correct than the new.  (several places)

https://codereview.chromium.org/777863002/diff/20001/chrome/browser/webdata/w...
File chrome/browser/webdata/web_data_service_factory.h (right):

https://codereview.chromium.org/777863002/diff/20001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.h:14: struct
DefaultSingletonTraits;
Please put this on the same line as above so it's clear the two are connected.

https://codereview.chromium.org/777863002/diff/20001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.h:29: // (visible for testing)
Nit: Use sentences.  Perhaps "Exposed here for use in tests."

https://codereview.chromium.org/777863002/diff/20001/chrome/test/base/testing...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/777863002/diff/20001/chrome/test/base/testing...
chrome/test/base/testing_profile.cc:598: static KeyedService*
BuildWebDataService(content::BrowserContext* profile) {
Nit: While here: It's weird that this file has both an anonymous namespace and
non-namespaced static functions.  Can these statics all be moved into the
anonymous namespace?  Then you can declare TestProfileErrorCallback() just above
this function and omit the comment on it saying what it's used for.

https://codereview.chromium.org/777863002/diff/20001/components/webdata_servi...
File components/webdata_services/web_data_service_wrapper.cc (right):

https://codereview.chromium.org/777863002/diff/20001/components/webdata_servi...
components/webdata_services/web_data_service_wrapper.cc:25: using
autofill::AutofillTable;
Nit: Avoid using directives unless they noticeably improve readability.

I think both these should probably be omitted.

https://codereview.chromium.org/777863002/diff/20001/components/webdata_servi...
components/webdata_services/web_data_service_wrapper.cc:47: // the old logins
table. We can remove this after a while, e.g. in M22 or so.
This milestone is long in the past.  Can we obey it now?  (Can do in a separate
CL)

https://codereview.chromium.org/777863002/diff/20001/components/webdata_servi...
File components/webdata_services/web_data_service_wrapper.h (right):

https://codereview.chromium.org/777863002/diff/20001/components/webdata_servi...
components/webdata_services/web_data_service_wrapper.h:35: //
WebDataServiceWrapper is a KeyedService that owns multiple WebDataService
Nit: multiple WebDataServices

https://codereview.chromium.org/777863002/diff/20001/components/webdata_servi...
components/webdata_services/web_data_service_wrapper.h:39: // ErrorType
indicates which web-data service encountered an error
Nit: Remove "web-data"

https://codereview.chromium.org/777863002/diff/20001/components/webdata_servi...
components/webdata_services/web_data_service_wrapper.h:42:
ERROR_DB_AUTOFILL_WEB_DATA,
Nit: I wonder if "ERROR_LOADING_AUTOFILL" or the like might be a more readable
naming scheme.

https://codereview.chromium.org/777863002/diff/20001/components/webdata_servi...
components/webdata_services/web_data_service_wrapper.h:49: // some of the
web-data service data.
Nit: How about: "Shows an error message if a loading error occurs."

https://codereview.chromium.org/777863002/diff/20001/components/webdata_servi...
components/webdata_services/web_data_service_wrapper.h:66: // (virtual for
testing)
Nit: I would remove this parenthetical note and the individual comments below
and instead do something like:

  // Create the various types of service instances.  These methods are virtual
  // for testing purposes.
  virtual scoped_refptr<autofill::AutofillWebDataService>
      GetAutofillWebDataService();
  virtual scoped_refptr<KeywordWebDataService> GetKeywordWebDataService();
  ...

https://codereview.chromium.org/777863002/diff/20001/components/webdata_servi...
File components/webdata_services/webdata_services_export.h (right):

https://codereview.chromium.org/777863002/diff/20001/components/webdata_servi...
components/webdata_services/webdata_services_export.h:15: #endif  //
defined(WEBDATA_IMPLEMENTATION)
This comment doesn't match the actual #if above

https://codereview.chromium.org/777863002/diff/20001/components/webdata_servi...
components/webdata_services/webdata_services_export.h:17: #else  //
defined(WIN32)
Nit: Unlike comments on the endif, I think comments on the else should reverse
the condition, i.e.:

#if FOO
...
#else  // !FOO
...
#endif  // FOO

(2 places)

[sdefresne, 2014-12-08 10:54:27]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2014-12-08 10:54:43]

Thank you for the review.

https://codereview.chromium.org/777863002/diff/20001/chrome/browser/webdata/w...
File chrome/browser/webdata/web_data_service_factory.cc (right):

https://codereview.chromium.org/777863002/diff/20001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.cc:28: using
autofill::AutofillWebDataBackend;
On 2014/12/05 at 21:47:56, Peter Kasting wrote:
> Nit: Avoid using directives unless they noticeably improve readability.  In
this case, you can just qualify this directly below.  You could probably also
eliminate at least the AutofillWebDataService using directive as well.

Done.

https://codereview.chromium.org/777863002/diff/20001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.cc:51: NOTREACHED() << "Unknown
WebDataServiceWrapper::ErrorType: " << error_type;
On 2014/12/05 at 21:47:55, Peter Kasting wrote:
> Nit: Please move these two lines to a default clause, since you're not
explicitly listing every type of wrapper error in the switch.

Done.

https://codereview.chromium.org/777863002/diff/20001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.cc:58:
ShowProfileErrorDialog(ProfileErrorFromWebDataServiceWrapperError(error_type),
On 2014/12/05 at 21:47:55, Peter Kasting wrote:
> Nit: Don't change the wrapping of this call; the old wrapping was more correct
than the new.

This was formatted by clang-format. Reverted.

https://codereview.chromium.org/777863002/diff/20001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.cc:128: // |wrapper| can be
nullptr in Incognito mode.
On 2014/12/05 at 21:47:56, Peter Kasting wrote:
> Nit: Say "null" instead of "nullptr" in these comments.

Done.

https://codereview.chromium.org/777863002/diff/20001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.cc:130: :
scoped_refptr<AutofillWebDataService>(nullptr);
On 2014/12/05 at 21:47:55, Peter Kasting wrote:
> Nit: Don't change the wrapping of these expressions; the old wrapping was more
correct than the new.  (several places)

This was formatted by clang-format. Reverted.

https://codereview.chromium.org/777863002/diff/20001/chrome/browser/webdata/w...
File chrome/browser/webdata/web_data_service_factory.h (right):

https://codereview.chromium.org/777863002/diff/20001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.h:14: struct
DefaultSingletonTraits;
On 2014/12/05 at 21:47:56, Peter Kasting wrote:
> Please put this on the same line as above so it's clear the two are connected.

Done.

https://codereview.chromium.org/777863002/diff/20001/chrome/test/base/testing...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/777863002/diff/20001/chrome/test/base/testing...
chrome/test/base/testing_profile.cc:598: static KeyedService*
BuildWebDataService(content::BrowserContext* profile) {
On 2014/12/05 at 21:47:56, Peter Kasting wrote:
> Nit: While here: It's weird that this file has both an anonymous namespace and
non-namespaced static functions.  Can these statics all be moved into the
anonymous namespace?  Then you can declare TestProfileErrorCallback() just above
this function and omit the comment on it saying what it's used for.

Done.

https://codereview.chromium.org/777863002/diff/20001/components/webdata_servi...
File components/webdata_services/web_data_service_wrapper.cc (right):

https://codereview.chromium.org/777863002/diff/20001/components/webdata_servi...
components/webdata_services/web_data_service_wrapper.cc:25: using
autofill::AutofillTable;
On 2014/12/05 at 21:47:56, Peter Kasting wrote:
> Nit: Avoid using directives unless they noticeably improve readability.
> 
> I think both these should probably be omitted.

Done.

https://codereview.chromium.org/777863002/diff/20001/components/webdata_servi...
components/webdata_services/web_data_service_wrapper.cc:47: // the old logins
table. We can remove this after a while, e.g. in M22 or so.
On 2014/12/05 at 21:47:56, Peter Kasting wrote:
> This milestone is long in the past.  Can we obey it now?  (Can do in a
separate CL)

I'd prefer to leave this for a separate CL in order to avoid mixing refactoring
and bug fixes.

https://codereview.chromium.org/777863002/diff/20001/components/webdata_servi...
File components/webdata_services/web_data_service_wrapper.h (right):

https://codereview.chromium.org/777863002/diff/20001/components/webdata_servi...
components/webdata_services/web_data_service_wrapper.h:35: //
WebDataServiceWrapper is a KeyedService that owns multiple WebDataService
On 2014/12/05 at 21:47:56, Peter Kasting wrote:
> Nit: multiple WebDataServices

Done.

https://codereview.chromium.org/777863002/diff/20001/components/webdata_servi...
components/webdata_services/web_data_service_wrapper.h:39: // ErrorType
indicates which web-data service encountered an error
On 2014/12/05 at 21:47:56, Peter Kasting wrote:
> Nit: Remove "web-data"

Done.

https://codereview.chromium.org/777863002/diff/20001/components/webdata_servi...
components/webdata_services/web_data_service_wrapper.h:42:
ERROR_DB_AUTOFILL_WEB_DATA,
On 2014/12/05 at 21:47:56, Peter Kasting wrote:
> Nit: I wonder if "ERROR_LOADING_AUTOFILL" or the like might be a more readable
naming scheme.

Done.

https://codereview.chromium.org/777863002/diff/20001/components/webdata_servi...
components/webdata_services/web_data_service_wrapper.h:42:
ERROR_DB_AUTOFILL_WEB_DATA,
On 2014/12/05 at 21:47:56, Peter Kasting wrote:
> Nit: I wonder if "ERROR_LOADING_AUTOFILL" or the like might be a more readable
naming scheme.

Done.

https://codereview.chromium.org/777863002/diff/20001/components/webdata_servi...
components/webdata_services/web_data_service_wrapper.h:49: // some of the
web-data service data.
On 2014/12/05 at 21:47:56, Peter Kasting wrote:
> Nit: How about: "Shows an error message if a loading error occurs."

Done.

https://codereview.chromium.org/777863002/diff/20001/components/webdata_servi...
components/webdata_services/web_data_service_wrapper.h:66: // (virtual for
testing)
On 2014/12/05 at 21:47:56, Peter Kasting wrote:
> Nit: I would remove this parenthetical note and the individual comments below
and instead do something like:
> 
>   // Create the various types of service instances.  These methods are virtual
>   // for testing purposes.
>   virtual scoped_refptr<autofill::AutofillWebDataService>
>       GetAutofillWebDataService();
>   virtual scoped_refptr<KeywordWebDataService> GetKeywordWebDataService();
>   ...

Done.

https://codereview.chromium.org/777863002/diff/20001/components/webdata_servi...
File components/webdata_services/webdata_services_export.h (right):

https://codereview.chromium.org/777863002/diff/20001/components/webdata_servi...
components/webdata_services/webdata_services_export.h:15: #endif  //
defined(WEBDATA_IMPLEMENTATION)
On 2014/12/05 at 21:47:56, Peter Kasting wrote:
> This comment doesn't match the actual #if above

Done.

https://codereview.chromium.org/777863002/diff/20001/components/webdata_servi...
components/webdata_services/webdata_services_export.h:17: #else  //
defined(WIN32)
On 2014/12/05 at 21:47:56, Peter Kasting wrote:
> Nit: Unlike comments on the endif, I think comments on the else should reverse
the condition, i.e.:
> 
> #if FOO
> ...
> #else  // !FOO
> ...
> #endif  // FOO
> 
> (2 places)

Done.

[commit-bot: I haz the power, 2014-12-08 10:55:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/777863002/40001

[commit-bot: I haz the power, 2014-12-08 10:59:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-08 10:59:16]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[sdefresne, 2014-12-08 11:07:51]

sdefresne@chromium.org changed reviewers:
+ jochen@chromium.org

[sdefresne, 2014-12-08 11:07:52]

jochen: OWNERS of
- chrome/browser/DEPS
- chrome/browser/sync
- chrome/test/base/testing_profile.cc

[jochen (gone - plz use gerrit), 2014-12-09 15:43:17]

https://codereview.chromium.org/777863002/diff/80001/chrome/chrome_tests_unit...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/777863002/diff/80001/chrome/chrome_tests_unit...
chrome/chrome_tests_unit.gypi:1421:
'../components/webdata_services/web_data_service_test_util.cc',
why aren't those compiled into components_unittests?

https://codereview.chromium.org/777863002/diff/80001/components/webdata_servi...
File components/webdata_services.gypi (right):

https://codereview.chromium.org/777863002/diff/80001/components/webdata_servi...
components/webdata_services.gypi:14: '../base/base.gyp:base',
you'll need  DEPS entries at least for the other components in
components/webdata_services

https://codereview.chromium.org/777863002/diff/80001/components/webdata_servi...
components/webdata_services.gypi:22: 'defines': [
not needed

https://codereview.chromium.org/777863002/diff/80001/components/webdata_servi...
components/webdata_services.gypi:28:
'webdata_services/webdata_services_export.h',
not needed. since you declare it as a static library, you don't need the export
stuff

[tfarina, 2014-12-09 15:48:27]

https://codereview.chromium.org/777863002/diff/80001/components/webdata_servi...
File components/webdata_services/BUILD.gn (right):

https://codereview.chromium.org/777863002/diff/80001/components/webdata_servi...
components/webdata_services/BUILD.gn:5: component("webdata_services") {
could you run `gn check //components/webdata_services` if you haven't?

[jochen (gone - plz use gerrit), 2014-12-09 15:52:29]

https://codereview.chromium.org/777863002/diff/80001/components/webdata_servi...
File components/webdata_services/BUILD.gn (right):

https://codereview.chromium.org/777863002/diff/80001/components/webdata_servi...
components/webdata_services/BUILD.gn:5: component("webdata_services") {
uh, right, the GN configuration says component and the gyp configuration
static_library. You have to decide for one :)

[sdefresne, 2014-12-12 16:53:37]

PTAL

https://codereview.chromium.org/777863002/diff/80001/chrome/chrome_tests_unit...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/777863002/diff/80001/chrome/chrome_tests_unit...
chrome/chrome_tests_unit.gypi:1421:
'../components/webdata_services/web_data_service_test_util.cc',
On 2014/12/09 at 15:43:17, jochen (slow) wrote:
> why aren't those compiled into components_unittests?

Those files used to have dependencies on //chrome. They are not tests but test
helper, so I created a new target for them and added the correct dependencies.

https://codereview.chromium.org/777863002/diff/80001/components/webdata_servi...
File components/webdata_services.gypi (right):

https://codereview.chromium.org/777863002/diff/80001/components/webdata_servi...
components/webdata_services.gypi:14: '../base/base.gyp:base',
On 2014/12/09 at 15:43:17, jochen (slow) wrote:
> you'll need  DEPS entries at least for the other components in
components/webdata_services

//components/webdata_services/DEPS already has the other components. Did you
miss the file?

https://codereview.chromium.org/777863002/diff/80001/components/webdata_servi...
components/webdata_services.gypi:22: 'defines': [
On 2014/12/09 at 15:43:17, jochen (slow) wrote:
> not needed

Done.

https://codereview.chromium.org/777863002/diff/80001/components/webdata_servi...
components/webdata_services.gypi:28:
'webdata_services/webdata_services_export.h',
On 2014/12/09 at 15:43:17, jochen (slow) wrote:
> not needed. since you declare it as a static library, you don't need the
export stuff

Done.

https://codereview.chromium.org/777863002/diff/80001/components/webdata_servi...
File components/webdata_services/BUILD.gn (right):

https://codereview.chromium.org/777863002/diff/80001/components/webdata_servi...
components/webdata_services/BUILD.gn:5: component("webdata_services") {
> could you run `gn check //components/webdata_services` if you haven't?

Done, and added the missing dependency.

> uh, right, the GN configuration says component and the gyp configuration
static_library. You have to decide for one :)

Changed to static_library.

[sdefresne, 2014-12-12 16:54:18]

Forgot to say that the PTAL applied only to jochen.

[jochen (gone - plz use gerrit), 2014-12-15 15:34:12]

lgtm

[sdefresne, 2014-12-15 18:34:40]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2014-12-15 18:36:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/777863002/120001

[commit-bot: I haz the power, 2014-12-15 20:53:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-15 20:53:56]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[sdefresne, 2014-12-15 22:39:05]

On 2014/12/15 at 20:53:56, commit-bot wrote:
> Try jobs failed on following builders:
>   win_chromium_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

According to https://code.google.com/p/chromium/issues/detail?id=442407 this is
a flake and it has been fixed, retrying the CQ.

[sdefresne, 2014-12-15 22:39:11]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2014-12-15 22:39:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/777863002/120001

[commit-bot: I haz the power, 2014-12-15 23:22:04]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-12-15 23:22:56]

Patchset 7 (id:??) landed as
https://crrev.com/cb955cd06650ee281f745b5a86aff7864fe44066
Cr-Commit-Position: refs/heads/master@{#308451}