Implement PasswordStoreProxyMac and SimplePasswordStoreMac.

Implement PasswordStoreProxyMac and SimplePasswordStoreMac.

They aren't instantiated yet in the code base. SimplePasswordStoreMac is a PasswordStore implementation on Mac in the future. PasswordStoreProxyMac is a proxy used for migration from PasswordStoreMac to SimplePasswordStoreMac.

BUG=466638
Committed: https://crrev.com/19209e174a4249faac3f6ee9051399f5b8639515
Cr-Commit-Position: refs/heads/master@{#335502}

[vasilii, 2015-06-19 19:39:56]

vasilii@chromium.org changed reviewers:
+ gcasto@chromium.org

[vasilii, 2015-06-19 19:42:08]

Hi Garrett,

please review the CL.

[vabr (Chromium), 2015-06-19 20:19:08]

vabr@chromium.org changed reviewers:
+ vabr@chromium.org

[vabr (Chromium), 2015-06-19 20:19:09]

LGTM with some comments.
Cheers,
Vaclav

https://codereview.chromium.org/1192963002/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/password_store_proxy_mac.cc (right):

https://codereview.chromium.org/1192963002/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/password_store_proxy_mac.cc:36: if
(password_store_mac_)
Why you need GetBackend() and do not just have
scoped_refptr<password_manager::PasswordStore> backend_
to which the constructor would currently assign the created PasswordStoreMac
instead? Do you expect that the proxy will need to hold both stores in the
future?

https://codereview.chromium.org/1192963002/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/simple_password_store_mac.h (right):

https://codereview.chromium.org/1192963002/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/simple_password_store_mac.h:20: // proxy.
nit: In this context, it is not clear what "the proxy" means. At the very least,
mention the full class name PasswordStoreProxy, optionally clarifying how the
proxy and the instance of SimplePasswordStoreMac are related.

https://codereview.chromium.org/1192963002/diff/1/components/password_manager...
File components/password_manager/core/browser/password_store.h (right):

https://codereview.chromium.org/1192963002/diff/1/components/password_manager...
components/password_manager/core/browser/password_store.h:296: friend class
::PasswordStoreProxyMac;
One thing I did not understand -- why do you need to forward the protected calls
in the proxy? Is it because of tests? In production, nobody should call
protected methods on the proxy, and the proxy does not call them itself (all is
done in the backend). Would it work if you remove the friend declaration and
just forward the public methods in the proxy?

[Garrett Casto, 2015-06-22 08:24:08]

LGTM

I'm not going to hold this up since it clearly a work in progress, but it's not
obvious to me why SimplePasswordStoreMac isn't just PasswordStoreDefault. The
current overrides don't seem to do anything different than PasswordStoreDefault,
and I don't see a particular reason why they should in the future.

https://codereview.chromium.org/1192963002/diff/20001/chrome/browser/password...
File chrome/browser/password_manager/password_store_proxy_mac.h (right):

https://codereview.chromium.org/1192963002/diff/20001/chrome/browser/password...
chrome/browser/password_manager/password_store_proxy_mac.h:77:
DISALLOW_COPY_AND_ASSIGN(PasswordStoreProxyMac);
Should this be private?

https://codereview.chromium.org/1192963002/diff/20001/chrome/browser/password...
File chrome/browser/password_manager/simple_password_store_mac.cc (right):

https://codereview.chromium.org/1192963002/diff/20001/chrome/browser/password...
chrome/browser/password_manager/simple_password_store_mac.cc:11: :
PasswordStoreDefault(main_thread_runner, nullptr, login_db.Pass()),
I'm not sure I understand what you are doing with the threading model here. It
looks like |background_thread_runner| to PasswordStoreDefault would give the
same behavior as you get from passing |nullptr| and overriding
GetBackgroundTaskRunner(). What am I missing here?

https://codereview.chromium.org/1192963002/diff/20001/chrome/browser/password...
chrome/browser/password_manager/simple_password_store_mac.cc:21: // All the
initialization has to be done by the owner of the object.
This doesn't seem to agree with PasswordStoreProxyMac(), which just tries to
call Init() on it's backend. Are you planning on treating Init() specially for
some reason?

[vasilii, 2015-06-22 13:34:17]

SimplePasswordStoreMac is different from PasswordStoreDefault in Init() method.
SimplePasswordStoreMac::Init() is noop as the database and the thread has been
set up before.

https://codereview.chromium.org/1192963002/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/password_store_proxy_mac.cc (right):

https://codereview.chromium.org/1192963002/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/password_store_proxy_mac.cc:36: if
(password_store_mac_)
On 2015/06/19 20:19:09, vabr (OOO until 29 June) wrote:
> Why you need GetBackend() and do not just have
> scoped_refptr<password_manager::PasswordStore> backend_
> to which the constructor would currently assign the created PasswordStoreMac
> instead? Do you expect that the proxy will need to hold both stores in the
> future?

Only one store is active at any moment. However, I need a pointer to a complete
class in order to perform the migration. I don't want to have an enum and
PasswordStore* instead.

https://codereview.chromium.org/1192963002/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/simple_password_store_mac.h (right):

https://codereview.chromium.org/1192963002/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/simple_password_store_mac.h:20: // proxy.
On 2015/06/19 20:19:09, vabr (OOO until 29 June) wrote:
> nit: In this context, it is not clear what "the proxy" means. At the very
least,
> mention the full class name PasswordStoreProxy, optionally clarifying how the
> proxy and the instance of SimplePasswordStoreMac are related.

Done.

https://codereview.chromium.org/1192963002/diff/1/components/password_manager...
File components/password_manager/core/browser/password_store.h (right):

https://codereview.chromium.org/1192963002/diff/1/components/password_manager...
components/password_manager/core/browser/password_store.h:296: friend class
::PasswordStoreProxyMac;
On 2015/06/19 20:19:09, vabr (OOO until 29 June) wrote:
> One thing I did not understand -- why do you need to forward the protected
calls
> in the proxy? Is it because of tests? In production, nobody should call
> protected methods on the proxy, and the proxy does not call them itself (all
is
> done in the backend). Would it work if you remove the friend declaration and
> just forward the public methods in the proxy?

Actually forwarding the protected calls is most important. There are things like
PasswordStore::observers_, PasswordStore::syncable_service_ etc. They should
live in PasswordStoreProxyMac then switching the backend is easier. In addition
the logic like notifying the observers just works. PasswordStoreMac becomes an
implementation detail of PasswordStoreProxyMac. Its public methods inherited
from PasswordStore are not needed anymore.

https://codereview.chromium.org/1192963002/diff/20001/chrome/browser/password...
File chrome/browser/password_manager/password_store_proxy_mac.h (right):

https://codereview.chromium.org/1192963002/diff/20001/chrome/browser/password...
chrome/browser/password_manager/password_store_proxy_mac.h:77:
DISALLOW_COPY_AND_ASSIGN(PasswordStoreProxyMac);
On 2015/06/22 08:24:08, Garrett Casto wrote:
> Should this be private?

Done.

https://codereview.chromium.org/1192963002/diff/20001/chrome/browser/password...
File chrome/browser/password_manager/simple_password_store_mac.cc (right):

https://codereview.chromium.org/1192963002/diff/20001/chrome/browser/password...
chrome/browser/password_manager/simple_password_store_mac.cc:11: :
PasswordStoreDefault(main_thread_runner, nullptr, login_db.Pass()),
On 2015/06/22 08:24:08, Garrett Casto wrote:
> I'm not sure I understand what you are doing with the threading model here. It
> looks like |background_thread_runner| to PasswordStoreDefault would give the
> same behavior as you get from passing |nullptr| and overriding
> GetBackgroundTaskRunner(). What am I missing here?

Yes. I was experimenting with clearing |background_thread_runner_| earlier.

https://codereview.chromium.org/1192963002/diff/20001/chrome/browser/password...
chrome/browser/password_manager/simple_password_store_mac.cc:21: // All the
initialization has to be done by the owner of the object.
On 2015/06/22 08:24:08, Garrett Casto wrote:
> This doesn't seem to agree with PasswordStoreProxyMac(), which just tries to
> call Init() on it's backend. Are you planning on treating Init() specially for
> some reason?

Yes. PasswordStoreProxyMac instantiate only PasswordStoreMac for now. In the
future patches PasswordStoreProxyMac::Init should be smarter.
SimplePasswordStoreMac::Init can be called after migration. The database and the
background thread are already inited. Therefore SimplePasswordStoreMac::Init
should be a noop.

[vasilii, 2015-06-22 13:34:33]

The CQ bit was checked by vasilii@chromium.org

[vasilii, 2015-06-22 13:34:33]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
gcasto@chromium.org
Link to the patchset: https://codereview.chromium.org/1192963002/#ps40001
(title: "addressed the comments")

[commit-bot: I haz the power, 2015-06-22 13:34:57]

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

[commit-bot: I haz the power, 2015-06-22 14:16:13]

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

[commit-bot: I haz the power, 2015-06-22 14:16:15]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[vasilii, 2015-06-22 14:25:27]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2015-06-22 14:25:51]

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

[commit-bot: I haz the power, 2015-06-22 15:01:31]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-06-22 15:02:24]

Patchset 3 (id:??) landed as
https://crrev.com/19209e174a4249faac3f6ee9051399f5b8639515
Cr-Commit-Position: refs/heads/master@{#335502}