Kill WebDataService, move (WIN only) Password code into separate class.

Following https://codereview.chromium.org/355573008, the only code left in WebDataService deals with IE 7/8 password data, so move this into a separate (WIN only class) and kill WebDataService.

BUG=181277
TEST=Everything still works
TBR=noms@chromium.org, zea@chromium.org, phajdan.jr@chromium.org
TBR'ing for removal of unneeded includes and API changes in (c/b/profile, c/b/sync, and  c/test/base respectively).

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

[Cait (Slow), 2014-07-07 13:42:41]

pkasting@chromium.org: Please review changes in c/b/webdata and
components/webdata

gab@chromium.org: Please review changes in c/b/importer

gcasto@chromium.org: Please review changes in c/b/password_manager

Thanks all!

[gab, 2014-07-07 14:37:21]

Nice cleanup! One general question below.

(otherwise chrome/browser/importer/profile_writer.cc lgtm stamp)

Cheers,
Gab

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

https://codereview.chromium.org/364343002/diff/40001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.h:47: virtual
scoped_refptr<PasswordWebDataService> GetPasswordWebData();
So this is more specific than a "password service" IIUC, this is the "IE7
password service", don't we also have a generic "password service" already?
(i.e. where do we store all other passwords?)

Why do IE7/8 imported passwords need to be stored differently than other
passwords?

Just trying to understand whether we even still need to make the distinction or
if we can remove even more code :-)!

[Cait (Slow), 2014-07-07 17:04:53]

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

https://codereview.chromium.org/364343002/diff/40001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.h:47: virtual
scoped_refptr<PasswordWebDataService> GetPasswordWebData();
On 2014/07/07 14:37:21, gab wrote:
> So this is more specific than a "password service" IIUC, this is the "IE7
> password service", don't we also have a generic "password service" already?
> (i.e. where do we store all other passwords?)
> 
> Why do IE7/8 imported passwords need to be stored differently than other
> passwords?
> 
> Just trying to understand whether we even still need to make the distinction
or
> if we can remove even more code :-)!
 Yeah, it would be great if we could remove this class altogether. AIUI, when we
import from IE7/8, passwords originally get stored in WebData, then when the
LoginDatabase loads later, PasswordStore moves them into it -- maybe to minimize
File iO during first run?  Either way, it would be nice if these could go into
the LoginDB directly.

[gab, 2014-07-07 17:14:45]

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

https://codereview.chromium.org/364343002/diff/40001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.h:47: virtual
scoped_refptr<PasswordWebDataService> GetPasswordWebData();
On 2014/07/07 17:04:52, Cait Phillips wrote:
> On 2014/07/07 14:37:21, gab wrote:
> > So this is more specific than a "password service" IIUC, this is the "IE7
> > password service", don't we also have a generic "password service" already?
> > (i.e. where do we store all other passwords?)
> > 
> > Why do IE7/8 imported passwords need to be stored differently than other
> > passwords?
> > 
> > Just trying to understand whether we even still need to make the distinction
> or
> > if we can remove even more code :-)!
>  Yeah, it would be great if we could remove this class altogether. AIUI, when
we
> import from IE7/8, passwords originally get stored in WebData, then when the
> LoginDatabase loads later, PasswordStore moves them into it -- maybe to
minimize
> File iO during first run?  Either way, it would be nice if these could go into
> the LoginDB directly. 

Ah I see, meh :(... I'm looking into making first run import asynchronous
(http://crbug.com/366116); maybe one day this will allow us to get rid of this
weird hook into Web Data for IE passwords...

Not a blocker for you for now though.

[Peter Kasting, 2014-07-07 23:20:04]

LGTM

https://codereview.chromium.org/364343002/diff/40001/chrome/browser/webdata/p...
File chrome/browser/webdata/password_web_data_service_win.cc (right):

https://codereview.chromium.org/364343002/diff/40001/chrome/browser/webdata/p...
chrome/browser/webdata/password_web_data_service_win.cc:17:
////////////////////////////////////////////////////////////////////////////////
Nit: This comment seems completely unnecessary, as does the line of slashes down
below.

https://codereview.chromium.org/364343002/diff/40001/chrome/browser/webdata/p...
chrome/browser/webdata/password_web_data_service_win.cc:20: using
content::BrowserThread;
Nit: Try to avoid using directives unless they make the code noticeably more
readable

https://codereview.chromium.org/364343002/diff/40001/chrome/browser/webdata/p...
chrome/browser/webdata/password_web_data_service_win.cc:55: return
WebDatabase::COMMIT_NOT_NEEDED;
Nit: I might collapse this to:

  return LoginsTable::FromWebDatabase(db)->AddIE7Login(info) ?
      WebDatabase::COMMIT_NEEDED : WebDatabase::COMMIT_NOT_NEEDED;

(2 places)

https://codereview.chromium.org/364343002/diff/40001/chrome/browser/webdata/p...
File chrome/browser/webdata/password_web_data_service_win.h (right):

https://codereview.chromium.org/364343002/diff/40001/chrome/browser/webdata/p...
chrome/browser/webdata/password_web_data_service_win.h:7: // through Chromium
defined APIs.
Nit: This comment seems kinda random and unhelpful to readers of this file.  I
don't think we should have it here or in any of the other files that contain it.

https://codereview.chromium.org/364343002/diff/40001/chrome/browser/webdata/p...
chrome/browser/webdata/password_web_data_service_win.h:9: #ifndef 
CHROME_BROWSER_WEBDATA_PASSWORD_WEB_DATA_SERVICE_WIN_H_
Nit: Extra space (2 places)

https://codereview.chromium.org/364343002/diff/40001/chrome/browser/webdata/p...
chrome/browser/webdata/password_web_data_service_win.h:34:
////////////////////////////////////////////////////////////////////////////////
Nit: The big

///////
//
//
///////


...comment frames are relatively unusual in Chrome code; I'd remove them and
simply put the comments here flush above the class.

https://codereview.chromium.org/364343002/diff/40001/chrome/browser/webdata/p...
chrome/browser/webdata/password_web_data_service_win.h:39: // All requests
return a handle. The handle can be used to cancel the request.
Nit: This information seems like it belongs, if anywhere, on the lone function
that returns a Handle, rather than on the whole class.

https://codereview.chromium.org/364343002/diff/40001/chrome/browser/webdata/p...
chrome/browser/webdata/password_web_data_service_win.h:47: // Retrieve a
WebDataService for the given context.
Nit: Retrieves

https://codereview.chromium.org/364343002/diff/40001/chrome/browser/webdata/p...
chrome/browser/webdata/password_web_data_service_win.h:60: // Get the login
matching the information in |info|. |consumer| will be
Nit: Gets

https://codereview.chromium.org/364343002/diff/40001/chrome/browser/webdata/p...
chrome/browser/webdata/password_web_data_service_win.h:74:
//////////////////////////////////////////////////////////////////////////////
Nit: Similarly, I'd remove the frame here and the blank line below it.

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

https://codereview.chromium.org/364343002/diff/40001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.cc:46: :
IDS_PROFILE_TOO_NEW_ERROR);
Nit: The old wrapping is correct, the new wrapping is incorrect.  (clang-format
error)  (3 places)

The only change that should have been made here is indenting the last line 4
more spaces from what it was.

https://codereview.chromium.org/364343002/diff/40001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.cc:150:
web_database_->ShutdownDatabase();
Nit: Wrong indenting

https://codereview.chromium.org/364343002/diff/40001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.cc:253: // |wrapper| can be NULL
in Incognito mode.
Nit: I'd move this comment either to the top of the function or above the if.

https://codereview.chromium.org/364343002/diff/40001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.cc:254: return
scoped_refptr<PasswordWebDataService>(NULL);
Nit: Could also be this, though the two forms aren't significantly different:

  return wrapper ?
      wrapper->GetPasswordWebData() :
      scoped_refptr<PasswordWebDataService>(NULL);

[Garrett Casto, 2014-07-08 22:34:59]

LGTM for chrome/browser/password_manager/*

[Cait (Slow), 2014-07-09 15:23:16]

Thanks all!

noms@, zea@ and phajdan.jr@: you're TBR'ed for removal of unneeded includes and
minor API changes in c/b/profile, c/b/sync, and c/test/base respectively.

https://codereview.chromium.org/364343002/diff/40001/chrome/browser/webdata/p...
File chrome/browser/webdata/password_web_data_service_win.cc (right):

https://codereview.chromium.org/364343002/diff/40001/chrome/browser/webdata/p...
chrome/browser/webdata/password_web_data_service_win.cc:17:
////////////////////////////////////////////////////////////////////////////////
On 2014/07/07 23:20:03, Peter Kasting wrote:
> Nit: This comment seems completely unnecessary, as does the line of slashes
down
> below.

Done.

https://codereview.chromium.org/364343002/diff/40001/chrome/browser/webdata/p...
chrome/browser/webdata/password_web_data_service_win.cc:20: using
content::BrowserThread;
On 2014/07/07 23:20:03, Peter Kasting wrote:
> Nit: Try to avoid using directives unless they make the code noticeably more
> readable

Done.

https://codereview.chromium.org/364343002/diff/40001/chrome/browser/webdata/p...
chrome/browser/webdata/password_web_data_service_win.cc:55: return
WebDatabase::COMMIT_NOT_NEEDED;
On 2014/07/07 23:20:03, Peter Kasting wrote:
> Nit: I might collapse this to:
> 
>   return LoginsTable::FromWebDatabase(db)->AddIE7Login(info) ?
>       WebDatabase::COMMIT_NEEDED : WebDatabase::COMMIT_NOT_NEEDED;
> 
> (2 places)

Done.

https://codereview.chromium.org/364343002/diff/40001/chrome/browser/webdata/p...
File chrome/browser/webdata/password_web_data_service_win.h (right):

https://codereview.chromium.org/364343002/diff/40001/chrome/browser/webdata/p...
chrome/browser/webdata/password_web_data_service_win.h:7: // through Chromium
defined APIs.
On 2014/07/07 23:20:03, Peter Kasting wrote:
> Nit: This comment seems kinda random and unhelpful to readers of this file.  I
> don't think we should have it here or in any of the other files that contain
it.

Done.

https://codereview.chromium.org/364343002/diff/40001/chrome/browser/webdata/p...
chrome/browser/webdata/password_web_data_service_win.h:9: #ifndef 
CHROME_BROWSER_WEBDATA_PASSWORD_WEB_DATA_SERVICE_WIN_H_
On 2014/07/07 23:20:03, Peter Kasting wrote:
> Nit: Extra space (2 places)

Done.

https://codereview.chromium.org/364343002/diff/40001/chrome/browser/webdata/p...
chrome/browser/webdata/password_web_data_service_win.h:34:
////////////////////////////////////////////////////////////////////////////////
On 2014/07/07 23:20:03, Peter Kasting wrote:
> Nit: The big
> 
> ///////
> //
> //
> ///////
> 
> 
> ...comment frames are relatively unusual in Chrome code; I'd remove them and
> simply put the comments here flush above the class.

Done.

https://codereview.chromium.org/364343002/diff/40001/chrome/browser/webdata/p...
chrome/browser/webdata/password_web_data_service_win.h:39: // All requests
return a handle. The handle can be used to cancel the request.
On 2014/07/07 23:20:03, Peter Kasting wrote:
> Nit: This information seems like it belongs, if anywhere, on the lone function
> that returns a Handle, rather than on the whole class.

Done.

https://codereview.chromium.org/364343002/diff/40001/chrome/browser/webdata/p...
chrome/browser/webdata/password_web_data_service_win.h:47: // Retrieve a
WebDataService for the given context.
On 2014/07/07 23:20:03, Peter Kasting wrote:
> Nit: Retrieves

Done.

https://codereview.chromium.org/364343002/diff/40001/chrome/browser/webdata/p...
chrome/browser/webdata/password_web_data_service_win.h:60: // Get the login
matching the information in |info|. |consumer| will be
On 2014/07/07 23:20:03, Peter Kasting wrote:
> Nit: Gets

Done.

https://codereview.chromium.org/364343002/diff/40001/chrome/browser/webdata/p...
chrome/browser/webdata/password_web_data_service_win.h:74:
//////////////////////////////////////////////////////////////////////////////
On 2014/07/07 23:20:03, Peter Kasting wrote:
> Nit: Similarly, I'd remove the frame here and the blank line below it.

Done.

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

https://codereview.chromium.org/364343002/diff/40001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.cc:46: :
IDS_PROFILE_TOO_NEW_ERROR);
On 2014/07/07 23:20:03, Peter Kasting wrote:
> Nit: The old wrapping is correct, the new wrapping is incorrect. 
(clang-format
> error)  (3 places)
> 
> The only change that should have been made here is indenting the last line 4
> more spaces from what it was.

Done.

https://codereview.chromium.org/364343002/diff/40001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.cc:150:
web_database_->ShutdownDatabase();
On 2014/07/07 23:20:03, Peter Kasting wrote:
> Nit: Wrong indenting

Done.

https://codereview.chromium.org/364343002/diff/40001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.cc:253: // |wrapper| can be NULL
in Incognito mode.
On 2014/07/07 23:20:03, Peter Kasting wrote:
> Nit: I'd move this comment either to the top of the function or above the if.

Done.

https://codereview.chromium.org/364343002/diff/40001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.cc:254: return
scoped_refptr<PasswordWebDataService>(NULL);
On 2014/07/07 23:20:03, Peter Kasting wrote:
> Nit: Could also be this, though the two forms aren't significantly different:
> 
>   return wrapper ?
>       wrapper->GetPasswordWebData() :
>       scoped_refptr<PasswordWebDataService>(NULL);

Done.

[Cait (Slow), 2014-07-09 15:23:27]

The CQ bit was checked by caitkp@chromium.org

[commit-bot: I haz the power, 2014-07-09 15:24:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/caitkp@chromium.org/364343002/60001

[noms (inactive), 2014-07-09 15:27:13]

profiles lgtm

[commit-bot: I haz the power, 2014-07-09 18:56:26]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-07-09 19:20:06]

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

[commit-bot: I haz the power, 2014-07-09 19:20:07]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[Cait (Slow), 2014-07-09 20:37:39]

The CQ bit was checked by caitkp@chromium.org

[commit-bot: I haz the power, 2014-07-09 20:39:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/caitkp@chromium.org/364343002/60001

[commit-bot: I haz the power, 2014-07-09 22:17:09]

Change committed as 282143