First phase of Mac Keychain integration. For the overall plan, see the design...

First phase of Mac Keychain integration. For the overall plan, see the design doc linked from the bug. This implements the ability to read existing Keychain passwords, but no storage/updating yet, and no adding of our own metadata as passwords are used; all that will be done in follow-up patches.

This also includes a Keychain wrapper for the functionality necessary so far, and a mock Keychain sufficient to unit-test essentially all of the code.

This patch deliberately excludes the step of instantiating a PasswordStoreMac and hooking it up to the profile, because a bug in autocomplete itself prevents passwords we load from actually being used, and we don't want to trigger Keychain UI for passwords that will be ignored. So this won't be used in builds yet, but it will be unit tested.

BUG=11745
TEST=none (no user-visible effect yet)

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

[stuartmorgan, 2009-05-21 20:46:09]

One other note: this won't actually build yet, since it relies on the Linux work
to create PasswordStore in the first place
(<http://codereview.chromium.org/113189>), but since that has passed review it's
unlikely to change.

[Mark Mentovai, 2009-05-21 21:10:54]

Not a thorough review, just passin' through.

http://codereview.chromium.org/115658/diff/17/1005
File chrome/browser/keychain_mac.h (right):

http://codereview.chromium.org/115658/diff/17/1005#newcode1
Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved.
Make sure to svn propset svn:eol-style LF on all of your new files.

http://codereview.chromium.org/115658/diff/17/1005#newcode22
Line 22: MacKeychain();
Do you need this?

http://codereview.chromium.org/115658/diff/17/1005#newcode23
Line 23: virtual ~MacKeychain();
You can put the {} here since it's a do-nothing, that might result in slightly
better code generation.

http://codereview.chromium.org/115658/diff/17/1009
File chrome/browser/password_manager/password_store_mac.mm (right):

http://codereview.chromium.org/115658/diff/17/1009#newcode68
Line 68: NSAutoreleasePool* pool = [[NSAutoreleasePool alloc] init];
You can use ScopedNSAutoreleasePool from base/scoped_nsautorelease_pool.h to
relieve yourself of the burden of popping the pool manually before returning.

http://codereview.chromium.org/115658/diff/17/1009#newcode160
Line 160: LOG(ERROR) << "Keychcain lookup failed for " << server << " with error
"
Should this really be LOG(ERROR)?

http://codereview.chromium.org/115658/diff/17/1008
File chrome/browser/password_manager/password_store_mac_unittest.mm (right):

http://codereview.chromium.org/115658/diff/17/1008#newcode41
Line 41: private:
Indent by one.

http://codereview.chromium.org/115658/diff/17/1008#newcode393
Line 393: public:
This too.

[stuartmorgan, 2009-05-21 22:11:42]

http://codereview.chromium.org/115658/diff/17/1005
File chrome/browser/keychain_mac.h (right):

http://codereview.chromium.org/115658/diff/17/1005#newcode1
Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved.
On 2009/05/21 21:10:54, Mark Mentovai wrote:
> Make sure to svn propset svn:eol-style LF on all of your new files.

Done.

http://codereview.chromium.org/115658/diff/17/1005#newcode22
Line 22: MacKeychain();
On 2009/05/21 21:10:54, Mark Mentovai wrote:
> Do you need this?

Yep, DISALLOW_COPY_AND_ASSIGN makes a (copy) constructor. I inlined the empty
constructor though.

http://codereview.chromium.org/115658/diff/17/1005#newcode23
Line 23: virtual ~MacKeychain();
On 2009/05/21 21:10:54, Mark Mentovai wrote:
> You can put the {} here since it's a do-nothing, that might result in slightly
> better code generation.

Done.

http://codereview.chromium.org/115658/diff/17/1009
File chrome/browser/password_manager/password_store_mac.mm (right):

http://codereview.chromium.org/115658/diff/17/1009#newcode68
Line 68: NSAutoreleasePool* pool = [[NSAutoreleasePool alloc] init];
On 2009/05/21 21:10:54, Mark Mentovai wrote:
> You can use ScopedNSAutoreleasePool

Done, good to know about.

http://codereview.chromium.org/115658/diff/17/1009#newcode160
Line 160: LOG(ERROR) << "Keychcain lookup failed for " << server << " with error
"
On 2009/05/21 21:10:54, Mark Mentovai wrote:
> Should this really be LOG(ERROR)?

I'm assuming ERROR shows up in release; my experience with Camino is that
logging like this is invaluable when people have bizarre issues with their
keychain, because only they can reproduce them.

http://codereview.chromium.org/115658/diff/17/1008
File chrome/browser/password_manager/password_store_mac_unittest.mm (right):

http://codereview.chromium.org/115658/diff/17/1008#newcode41
Line 41: private:
On 2009/05/21 21:10:54, Mark Mentovai wrote:
> Indent by one.

Done (stupid Xcode).

http://codereview.chromium.org/115658/diff/17/1008#newcode393
Line 393: public:
On 2009/05/21 21:10:54, Mark Mentovai wrote:
> This too.

Done.

[TVL, 2009-05-22 14:54:09]

http://codereview.chromium.org/115658/diff/1014/1018
File chrome/browser/password_manager/password_store_mac.h (right):

http://codereview.chromium.org/115658/diff/1014/1018#newcode9
Line 9: #include <vector>
why do you need vector/map here?

http://codereview.chromium.org/115658/diff/1014/1018#newcode14
Line 14: #include "base/scoped_ptr.h"
why do you need all these headers?

http://codereview.chromium.org/115658/diff/1014/1018#newcode15
Line 15: #include "chrome/browser/keychain_mac.h"
just do class MacKeychain and include the header in the impl file.

http://codereview.chromium.org/115658/diff/1014/1018#newcode20
Line 20: class Task;
why are these needed?

http://codereview.chromium.org/115658/diff/1014/1018#newcode34
Line 34: MacKeychain* keychain_;
use a helper to auto delete it?

http://codereview.chromium.org/115658/diff/1014/1020
File chrome/browser/password_manager/password_store_mac.mm (right):

http://codereview.chromium.org/115658/diff/1014/1020#newcode21
Line 21: // TODO: signon_realm for proxies is not yet supported.
nit: add your name to todos

http://codereview.chromium.org/115658/diff/1014/1020#newcode40
Line 40: *security_domain = realm_as_url.path().substr(1);
where does 1 come from?

http://codereview.chromium.org/115658/diff/1014/1020#newcode69
Line 69: base::ScopedNSAutoreleasePool scoped_pool;
base/time.h has FromUTCExploded which seems like it could be done with a scanf
off the string, avoiding the pool overhead here.

(might also mean this can be a .cc file then)

http://codereview.chromium.org/115658/diff/1014/1020#newcode189
Line 189: attrInfo.count = sizeof(tags)/sizeof(UInt32);
basictypes.h:arraysize/ARRAYSIZE_UNSAFE?

http://codereview.chromium.org/115658/diff/1014/1020#newcode222
Line 222: if (attr.tag == kSecAccountItemAttr) {
seems like a switch would be better hear, might also be a little more readable?

http://codereview.chromium.org/115658/diff/1014/1020#newcode284
Line 284: PasswordStoreMac::~PasswordStoreMac() {
if you use a helper, you can inline this in the header and keep it empty.

http://codereview.chromium.org/115658/diff/1014/1019
File chrome/browser/password_manager/password_store_mac_unittest.mm (right):

http://codereview.chromium.org/115658/diff/1014/1019#newcode3
Line 3: // found in the LICENSE file.
why is this a .mm?

http://codereview.chromium.org/115658/diff/1014/1019#newcode145
Line 145: ++item;
CHECK against item_count_? (and the rest of the way down?)

http://codereview.chromium.org/115658/diff/1014/1019#newcode291
Line 291: keychain_data_[item].data = malloc(data_size);
check result?

http://codereview.chromium.org/115658/diff/1014/1019#newcode392
Line 392: class PasswordStoreMacTest : public testing::Test {
why do you need this?

http://codereview.chromium.org/115658/diff/1014/1019#newcode409
Line 409: // HTML form signon realms.
table drive and a loop might be easer to read
{ INPUT, PARSES, SECURE, HOST, PORT, DOMAIN },
...

in the current tests, you don't check for security_domain not being set except
when it should, etc.

[stuartmorgan, 2009-05-22 22:07:58]

http://codereview.chromium.org/115658/diff/1014/1018
File chrome/browser/password_manager/password_store_mac.h (right):

http://codereview.chromium.org/115658/diff/1014/1018#newcode9
Line 9: #include <vector>
On 2009/05/22 14:54:10, TVL wrote:
> why do you need vector/map here?

I don't ;) The class declaration had a lot of churn, and these are cruft.

http://codereview.chromium.org/115658/diff/1014/1018#newcode14
Line 14: #include "base/scoped_ptr.h"
On 2009/05/22 14:54:10, TVL wrote:
> why do you need all these headers?

Cruft from an earlier snapshot of the Linux refactoring work; fixed.

http://codereview.chromium.org/115658/diff/1014/1018#newcode15
Line 15: #include "chrome/browser/keychain_mac.h"
On 2009/05/22 14:54:10, TVL wrote:
> just do class MacKeychain and include the header in the impl file.

Done; wasn't possible with an earlier iteration and I forgot to update it.

http://codereview.chromium.org/115658/diff/1014/1018#newcode20
Line 20: class Task;
On 2009/05/22 14:54:10, TVL wrote:
> why are these needed?

Cruft from earlier Linux snapshot again.

http://codereview.chromium.org/115658/diff/1014/1018#newcode34
Line 34: MacKeychain* keychain_;
On 2009/05/22 14:54:10, TVL wrote:
> use a helper to auto delete it?

Done.

http://codereview.chromium.org/115658/diff/1014/1020
File chrome/browser/password_manager/password_store_mac.mm (right):

http://codereview.chromium.org/115658/diff/1014/1020#newcode21
Line 21: // TODO: signon_realm for proxies is not yet supported.
On 2009/05/22 14:54:10, TVL wrote:
> nit: add your name to todos

Done.

http://codereview.chromium.org/115658/diff/1014/1020#newcode40
Line 40: *security_domain = realm_as_url.path().substr(1);
On 2009/05/22 14:54:10, TVL wrote:
> where does 1 come from?

Comment added

http://codereview.chromium.org/115658/diff/1014/1020#newcode69
Line 69: base::ScopedNSAutoreleasePool scoped_pool;
On 2009/05/22 14:54:10, TVL wrote:
> base/time.h has FromUTCExploded which seems like it could be done with a scanf
> off the string, avoiding the pool overhead here.
> 
> (might also mean this can be a .cc file then)

Done (and done).

http://codereview.chromium.org/115658/diff/1014/1020#newcode189
Line 189: attrInfo.count = sizeof(tags)/sizeof(UInt32);
On 2009/05/22 14:54:10, TVL wrote:
> basictypes.h:arraysize/ARRAYSIZE_UNSAFE?

Missed this when I went back through after learning about arraysize; done.

http://codereview.chromium.org/115658/diff/1014/1020#newcode222
Line 222: if (attr.tag == kSecAccountItemAttr) {
On 2009/05/22 14:54:10, TVL wrote:
> seems like a switch would be better hear, might also be a little more
readable?

Done.

http://codereview.chromium.org/115658/diff/1014/1020#newcode284
Line 284: PasswordStoreMac::~PasswordStoreMac() {
On 2009/05/22 14:54:10, TVL wrote:
> if you use a helper, you can inline this in the header and keep it empty.

Done.

http://codereview.chromium.org/115658/diff/1014/1019
File chrome/browser/password_manager/password_store_mac_unittest.mm (right):

http://codereview.chromium.org/115658/diff/1014/1019#newcode3
Line 3: // found in the LICENSE file.
On 2009/05/22 14:54:10, TVL wrote:
> why is this a .mm?

Habit ;) Fixed.

http://codereview.chromium.org/115658/diff/1014/1019#newcode145
Line 145: ++item;
On 2009/05/22 14:54:10, TVL wrote:
> CHECK against item_count_? (and the rest of the way down?)

Done.

http://codereview.chromium.org/115658/diff/1014/1019#newcode291
Line 291: keychain_data_[item].data = malloc(data_size);
On 2009/05/22 14:54:10, TVL wrote:
> check result?

Done.

http://codereview.chromium.org/115658/diff/1014/1019#newcode392
Line 392: class PasswordStoreMacTest : public testing::Test {
On 2009/05/22 14:54:10, TVL wrote:
> why do you need this?

Removed.

http://codereview.chromium.org/115658/diff/1014/1019#newcode409
Line 409: // HTML form signon realms.
On 2009/05/22 14:54:10, TVL wrote:
> table drive and a loop might be easer to read
> { INPUT, PARSES, SECURE, HOST, PORT, DOMAIN },
> ...
> 
> in the current tests, you don't check for security_domain not being set except
> when it should, etc.

Done; I also did the same in TestKeychainToFormTranslation, and also in fleshing
out TestKeychainTime for rewriting the time parsing.

[TVL, 2009-05-26 12:57:35]

lgtm