Profile refactoring: Remove all PasswordStore code from the Profile interface.

Profile refactoring: Remove all PasswordStore code from the Profile interface.

This PKSizes PasswordStore. All of the gnarly initialization code now
lives in a PasswordStoreFactory instead of in the Profile. This patch
also does small cleanups to the PasswordStore unit tests, like
factoring out common mocks.

BUG=112561
R=mdm,rlp
TBR=jhawkins,atwilson

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=126600

[Mike Mammarella, 2012-03-10 05:11:52]

I'm not a ProfileKeyedService expert but this all looks reasonable to me.

https://chromiumcodereview.appspot.com/9665007/diff/2006/chrome/browser/passw...
File chrome/browser/password_manager/password_store_default.cc (right):

https://chromiumcodereview.appspot.com/9665007/diff/2006/chrome/browser/passw...
chrome/browser/password_manager/password_store_default.cc:122:
migrate_helper_.reset();
Should there be a DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); here?

https://chromiumcodereview.appspot.com/9665007/diff/2006/chrome/browser/passw...
File chrome/browser/password_manager/password_store_default.h (right):

https://chromiumcodereview.appspot.com/9665007/diff/2006/chrome/browser/passw...
chrome/browser/password_manager/password_store_default.h:32: virtual void
ShutdownOnUIThread() OVERRIDE;
Does this actually override now? The method is removed from PasswordStore.

Edit: ah, I see, it's from RefcountedProfileKeyedService. Can you move it out
from under the comment about implementing PasswordStore? :)

https://chromiumcodereview.appspot.com/9665007/diff/2006/chrome/browser/passw...
File chrome/browser/password_manager/password_store_factory.cc (right):

https://chromiumcodereview.appspot.com/9665007/diff/2006/chrome/browser/passw...
chrome/browser/password_manager/password_store_factory.cc:67: //
DependsOn(WebDataServiceFactory::GetInstance());
Do you mean for this to be uncommented, then? Alternately, it's been a *very*
long time since the move from storing passwords in WebDB. Maybe we can drop this
migration code now. Probably not in this particular CL though... it's already
big enough!

[Elliot Glaysher, 2012-03-12 19:59:40]

mdm: ptal
rlp: profiles review
jhawkins, atwilson: TBR for one line changes of the profile->GetPasswordStore()
to PasswordStoreFactory::GetForProfile() sort.

https://chromiumcodereview.appspot.com/9665007/diff/2006/chrome/browser/passw...
File chrome/browser/password_manager/password_store_default.cc (right):

https://chromiumcodereview.appspot.com/9665007/diff/2006/chrome/browser/passw...
chrome/browser/password_manager/password_store_default.cc:122:
migrate_helper_.reset();
On 2012/03/10 05:11:52, Mike Mammarella wrote:
> Should there be a DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); here?

I've added this, but just temporarily forever.

What I want is for there to be a DCHECK in the caller (and eventually there will
be), but because so many of our unit tests don't set up mock threads, it is
infeasable. :(

https://chromiumcodereview.appspot.com/9665007/diff/2006/chrome/browser/passw...
File chrome/browser/password_manager/password_store_factory.cc (right):

https://chromiumcodereview.appspot.com/9665007/diff/2006/chrome/browser/passw...
chrome/browser/password_manager/password_store_factory.cc:67: //
DependsOn(WebDataServiceFactory::GetInstance());
On 2012/03/10 05:11:52, Mike Mammarella wrote:
> Do you mean for this to be uncommented, then? Alternately, it's been a *very*
> long time since the move from storing passwords in WebDB. Maybe we can drop
this
> migration code now. Probably not in this particular CL though... it's already
> big enough!

I mean for this to be uncommented eventually. WebDB hasn't been ported yet.

[rpetterson, 2012-03-13 19:43:44]

Profiles LGTM. Thanks for taking care of this!!

[Mike Mammarella, 2012-03-13 20:11:24]

LGTM

[commit-bot: I haz the power, 2012-03-13 20:15:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erg@chromium.org/9665007/8001

[commit-bot: I haz the power, 2012-03-13 23:40:57]

Try job failure for 9665007-8001 (retry) on win_rel for step "unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-03-13 23:54:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erg@chromium.org/9665007/16001

[commit-bot: I haz the power, 2012-03-14 06:52:06]

Change committed as 126600