Remove passwords from the keychain if the profile is destroyed.

Remove passwords from the keychain if the profile is destroyed.

This is a relanded https://codereview.chromium.org/307673002. It was reverted due to concerns that non-Chrome passwords may suffer (which is untrue). After https://codereview.chromium.org/360343002 is landed the only affected passwords are those created by Chrome within the current profile.

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

[vasilii, 2014-07-02 15:17:41]

Hi,

please review the CL. Stuart, the only question is if this functionality should
be enabled on mobile too?

[stuartmorgan, 2014-07-02 18:47:43]

On 2014/07/02 15:17:41, vasilii wrote:
> please review the CL. Stuart, the only question is if this functionality
should
> be enabled on mobile too?

It should be done on iOS certainly; I can't speak for Android since I don't know
anything about Android password storage.

[vasilii, 2014-07-03 15:32:56]

On Android it's SQL database. I excluded Android, Win and CrOS. Please review.

[stuartmorgan, 2014-07-07 18:49:43]

LGTM for concept; I didn't look in detail at the code.

https://codereview.chromium.org/363013003/diff/20001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/363013003/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager.cc:1113: #if !defined(OS_WIN) &&
!defined(OS_ANDROID) && !defined(OS_CHROMEOS)
Is this code actually expensive enough that we need to ifdef it out on these
platforms?

https://codereview.chromium.org/363013003/diff/20001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_manager_browsertest.cc (right):

https://codereview.chromium.org/363013003/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager_browsertest.cc:399: // The test makes
sense on those platforms where the Keychain exists.
The term "Keychain" as a proper name is Apple-specific, so s/Keychain/keychain/

[vasilii, 2014-07-08 09:27:39]

Elliot?

https://codereview.chromium.org/363013003/diff/20001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/363013003/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager.cc:1113: #if !defined(OS_WIN) &&
!defined(OS_ANDROID) && !defined(OS_CHROMEOS)
On 2014/07/07 18:49:42, stuartmorgan wrote:
> Is this code actually expensive enough that we need to ifdef it out on these
> platforms?

Not really.

https://codereview.chromium.org/363013003/diff/20001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_manager_browsertest.cc (right):

https://codereview.chromium.org/363013003/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager_browsertest.cc:399: // The test makes
sense on those platforms where the Keychain exists.
On 2014/07/07 18:49:42, stuartmorgan wrote:
> The term "Keychain" as a proper name is Apple-specific, so
s/Keychain/keychain/

Done.

[vasilii, 2014-07-10 11:04:26]

Hi Stefan,

please do the owner review.

[Mr4D (OOO till 08-26), 2014-07-10 14:20:50]

lgtm

https://codereview.chromium.org/363013003/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_manager_browsertest.cc (right):

https://codereview.chromium.org/363013003/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager_browsertest.cc:92: class
PasswordStoreConsumerVerifier :
Can you please add a comment toward what this class does - and the fact that it
is only used on some platforms?

[vasilii, 2014-07-10 15:23:55]

https://codereview.chromium.org/363013003/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_manager_browsertest.cc (right):

https://codereview.chromium.org/363013003/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager_browsertest.cc:92: class
PasswordStoreConsumerVerifier :
On 2014/07/10 14:20:50, Mr4D wrote:
> Can you please add a comment toward what this class does - and the fact that
it
> is only used on some platforms?

Done.

[vasilii, 2014-07-10 15:24:08]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-07-10 15:25:10]

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

[commit-bot: I haz the power, 2014-07-10 19:32:21]

Change committed as 282397