Encrypt password values in LoginDatabase on Mac.

Encrypt password values in LoginDatabase on Mac.

PasswordStoreMac should continue writing empty password values into LoginDatabase.

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

[vasilii, 2015-06-18 10:42:48]

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

[vasilii, 2015-06-18 10:42:49]

Hi Vaclav,

please review.

[vabr (Chromium), 2015-06-18 13:39:09]

Hi Vasilii,

This LGTM.

Please correct the typo "chould" in the CL description.

Cheers,
Vaclav

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

https://codereview.chromium.org/1192493005/diff/20001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac_unittest.cc:1223: // Whatever
a test did, PasswordStoreMac store only empty password values in
nit: stores

https://codereview.chromium.org/1192493005/diff/20001/components/password_man...
File components/password_manager/core/browser/login_database.h (right):

https://codereview.chromium.org/1192493005/diff/20001/components/password_man...
components/password_manager/core/browser/login_database.h:114: // If set to
'true', then the password values are cleared before encrypting
This comment should go to the variable declaration. Not everybody looking at the
decl. will think about finding the setter, but everybody looking at the setter
can look up the declaration.

https://codereview.chromium.org/1192493005/diff/20001/components/password_man...
components/password_manager/core/browser/login_database.h:177: bool
clear_password_values_;
Also, is this just some temporary measure until we stop using the keychain? If
so, it should be marked as such, including the link to http://crbug.com/466638.

https://codereview.chromium.org/1192493005/diff/20001/components/password_man...
File components/password_manager/core/browser/login_database_mac.cc (right):

https://codereview.chromium.org/1192493005/diff/20001/components/password_man...
components/password_manager/core/browser/login_database_mac.cc:15: // Handling
the empty string shouldn't use the Keychain.
It is not clear to me why it is important to mention not using Keychain here.
Also, the reference to the empty string might be unclear for readers without the
context of the changes in login_database.cc above.

[vasilii, 2015-06-18 14:28:14]

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

https://codereview.chromium.org/1192493005/diff/20001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac_unittest.cc:1223: // Whatever
a test did, PasswordStoreMac store only empty password values in
On 2015/06/18 13:39:08, vabr (Chromium) wrote:
> nit: stores

Done.

https://codereview.chromium.org/1192493005/diff/20001/components/password_man...
File components/password_manager/core/browser/login_database.h (right):

https://codereview.chromium.org/1192493005/diff/20001/components/password_man...
components/password_manager/core/browser/login_database.h:114: // If set to
'true', then the password values are cleared before encrypting
On 2015/06/18 13:39:09, vabr (Chromium) wrote:
> This comment should go to the variable declaration. Not everybody looking at
the
> decl. will think about finding the setter, but everybody looking at the setter
> can look up the declaration.

Done.

https://codereview.chromium.org/1192493005/diff/20001/components/password_man...
components/password_manager/core/browser/login_database.h:177: bool
clear_password_values_;
On 2015/06/18 13:39:09, vabr (Chromium) wrote:
> Also, is this just some temporary measure until we stop using the keychain? If
> so, it should be marked as such, including the link to
http://crbug.com/466638.

Done.

https://codereview.chromium.org/1192493005/diff/20001/components/password_man...
File components/password_manager/core/browser/login_database_mac.cc (right):

https://codereview.chromium.org/1192493005/diff/20001/components/password_man...
components/password_manager/core/browser/login_database_mac.cc:15: // Handling
the empty string shouldn't use the Keychain.
On 2015/06/18 13:39:09, vabr (Chromium) wrote:
> It is not clear to me why it is important to mention not using Keychain here.
> Also, the reference to the empty string might be unclear for readers without
the
> context of the changes in login_database.cc above.

The comment is obsolete.

[vasilii, 2015-06-18 14:29:19]

The CQ bit was checked by vasilii@chromium.org

[vasilii, 2015-06-18 14:29:20]

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

[commit-bot: I haz the power, 2015-06-18 14:29:38]

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

[commit-bot: I haz the power, 2015-06-18 16:10:07]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-06-18 16:11:04]

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