Move *keychain_mac* files to crypto/

Move *keychain_mac* files to crypto/

This is in preparation for moving password_manager/encryptor* to
crypto/.

This also fixes an ODR violation; mock_keychain_mac.cc was being
compiled in two targets which are linked together.

BUG=118564
TEST=


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

[akalin, 2012-03-16 08:44:52]

+ryan for review

[Ryan Sleevi, 2012-03-16 20:58:37]

I like it!

Mostly because once or twice I've wanted to use this in net/ for tests (although
I don't have any pressing need).

LGTM, but a few stylistic nits as it moves in.

http://codereview.chromium.org/9699112/diff/1/chrome/browser/password_manager...
File chrome/browser/password_manager/password_store_mac_internal.h (right):

http://codereview.chromium.org/9699112/diff/1/chrome/browser/password_manager...
chrome/browser/password_manager/password_store_mac_internal.h:27: const
MacKeychain* keychain);
Does this fit on one line?

http://codereview.chromium.org/9699112/diff/1/crypto/crypto.gyp
File crypto/crypto.gyp (right):

http://codereview.chromium.org/9699112/diff/1/crypto/crypto.gyp#newcode163
crypto/crypto.gyp:163: 'mock_keychain_mac.h',
ordering (after mac_security_services)

http://codereview.chromium.org/9699112/diff/1/crypto/keychain_mac.cc
File crypto/keychain_mac.cc (right):

http://codereview.chromium.org/9699112/diff/1/crypto/keychain_mac.cc#newcode109
crypto/keychain_mac.cc:109: CFRelease(ref);
nit: no braces

http://codereview.chromium.org/9699112/diff/1/crypto/keychain_mac.h
File crypto/keychain_mac.h (right):

http://codereview.chromium.org/9699112/diff/1/crypto/keychain_mac.h#newcode31
crypto/keychain_mac.h:31: UInt32 *length, void **outData) const;
Can you update these type names to have the * with the typename, since it's part
of the type, as per
http://www.chromium.org/developers/coding-style#TOC-Code-formatting

eg:

SecKeychainAttributeInfo *info -> SecKeychainAttributeInfo* info

Also, line breaks/whitespace since the function declaration doesn't all fit on
one line.

http://codereview.chromium.org/9699112/diff/1/crypto/mock_keychain_mac.cc
File crypto/mock_keychain_mac.cc (right):

http://codereview.chromium.org/9699112/diff/1/crypto/mock_keychain_mac.cc#new...
crypto/mock_keychain_mac.cc:67: if (it->second.attr[i].data) {
nit: Per http://www.chromium.org/developers/coding-style#TOC-Code-formatting 

Other formatting "rules" are not required, but are fairly common across Chrome
code, and thus it's probably best to stick to them unless you have a reason not
to.  In particular, if existing code follows these styles, maintain them:
Prefer (foo == 0) to (0 == foo)
Omit {} for one-line conditional or loop bodies
Function declaration order should match function definition order

wtc tends to ding me on these, so it should be true in crypto/ land that this is
fairly consistent. I haven't done an audit, but something to consider.

http://codereview.chromium.org/9699112/diff/1/crypto/mock_keychain_mac.h
File crypto/mock_keychain_mac.h (right):

http://codereview.chromium.org/9699112/diff/1/crypto/mock_keychain_mac.h#newc...
crypto/mock_keychain_mac.h:19: // Mock Keychain wrapper for testing code that
interacts with the OS Keychain.
nit: OS X Keychain?

http://codereview.chromium.org/9699112/diff/1/crypto/mock_keychain_mac.h#newc...
crypto/mock_keychain_mac.h:30: virtual OSStatus ItemCopyAttributesAndData(
Per http://www.chromium.org/developers/coding-style#TOC-Code-formatting , and
ultimately a small nit, can you explicitly specify the interface this implements
(yes yes, it's "obvious")

Also, line breaks as previously mentioned.

[akalin, 2012-03-17 07:19:44]

All comments addressed, committing

http://codereview.chromium.org/9699112/diff/1/chrome/browser/password_manager...
File chrome/browser/password_manager/password_store_mac_internal.h (right):

http://codereview.chromium.org/9699112/diff/1/chrome/browser/password_manager...
chrome/browser/password_manager/password_store_mac_internal.h:27: const
MacKeychain* keychain);
On 2012/03/16 20:58:37, Ryan Sleevi wrote:
> Does this fit on one line?

Done.

http://codereview.chromium.org/9699112/diff/1/crypto/crypto.gyp
File crypto/crypto.gyp (right):

http://codereview.chromium.org/9699112/diff/1/crypto/crypto.gyp#newcode163
crypto/crypto.gyp:163: 'mock_keychain_mac.h',
On 2012/03/16 20:58:37, Ryan Sleevi wrote:
> ordering (after mac_security_services)

Done.

http://codereview.chromium.org/9699112/diff/1/crypto/keychain_mac.cc
File crypto/keychain_mac.cc (right):

http://codereview.chromium.org/9699112/diff/1/crypto/keychain_mac.cc#newcode109
crypto/keychain_mac.cc:109: CFRelease(ref);
On 2012/03/16 20:58:37, Ryan Sleevi wrote:
> nit: no braces

Done.

http://codereview.chromium.org/9699112/diff/1/crypto/keychain_mac.h
File crypto/keychain_mac.h (right):

http://codereview.chromium.org/9699112/diff/1/crypto/keychain_mac.h#newcode31
crypto/keychain_mac.h:31: UInt32 *length, void **outData) const;
On 2012/03/16 20:58:37, Ryan Sleevi wrote:
> Can you update these type names to have the * with the typename, since it's
part
> of the type, as per
> http://www.chromium.org/developers/coding-style#TOC-Code-formatting
> 
> eg:
> 
> SecKeychainAttributeInfo *info -> SecKeychainAttributeInfo* info
> 
> Also, line breaks/whitespace since the function declaration doesn't all fit on
> one line.

I assume you mean the recommended one-arg-per-line style?  Done.

http://codereview.chromium.org/9699112/diff/1/crypto/mock_keychain_mac.cc
File crypto/mock_keychain_mac.cc (right):

http://codereview.chromium.org/9699112/diff/1/crypto/mock_keychain_mac.cc#new...
crypto/mock_keychain_mac.cc:67: if (it->second.attr[i].data) {
On 2012/03/16 20:58:37, Ryan Sleevi wrote:
> nit: Per http://www.chromium.org/developers/coding-style#TOC-Code-formatting 
> 
> Other formatting "rules" are not required, but are fairly common across Chrome
> code, and thus it's probably best to stick to them unless you have a reason
not
> to.  In particular, if existing code follows these styles, maintain them:
> Prefer (foo == 0) to (0 == foo)
> Omit {} for one-line conditional or loop bodies
> Function declaration order should match function definition order
> 
> wtc tends to ding me on these, so it should be true in crypto/ land that this
is
> fairly consistent. I haven't done an audit, but something to consider.

I made a pass to clean up simple if statements and indentation.  I didn't want
to go further than that, though, as this class is pretty complex and has no unit
tests, and reordering functions would make it hard to see the diffs.

http://codereview.chromium.org/9699112/diff/1/crypto/mock_keychain_mac.h
File crypto/mock_keychain_mac.h (right):

http://codereview.chromium.org/9699112/diff/1/crypto/mock_keychain_mac.h#newc...
crypto/mock_keychain_mac.h:19: // Mock Keychain wrapper for testing code that
interacts with the OS Keychain.
On 2012/03/16 20:58:37, Ryan Sleevi wrote:
> nit: OS X Keychain?

Done.

http://codereview.chromium.org/9699112/diff/1/crypto/mock_keychain_mac.h#newc...
crypto/mock_keychain_mac.h:30: virtual OSStatus ItemCopyAttributesAndData(
On 2012/03/16 20:58:37, Ryan Sleevi wrote:
> Per http://www.chromium.org/developers/coding-style#TOC-Code-formatting , and
> ultimately a small nit, can you explicitly specify the interface this
implements
> (yes yes, it's "obvious")
> 
> Also, line breaks as previously mentioned.

Done.

[commit-bot: I haz the power, 2012-03-17 07:19:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/9699112/17004

[commit-bot: I haz the power, 2012-03-17 12:14:51]

Change committed as 127358

[wtc, 2012-03-19 22:42:28]

I agree it is a good idea to move these files to crypto
so they can be used by lower-level components.  I have a
question below.

http://codereview.chromium.org/9699112/diff/17004/crypto/keychain_mac.h
File crypto/keychain_mac.h (right):

http://codereview.chromium.org/9699112/diff/17004/crypto/keychain_mac.h#newco...
crypto/keychain_mac.h:23: class MacKeychain {

The class is named MacKeychain, but the file is named
keychain_mac.{cc,h} rather than mac_keychain.{cc,h}.
I guess this is because we want to rely on the existing GYP
rule that treat all _mac.* files as Mac only?

[akalin, 2012-03-19 22:50:03]

http://codereview.chromium.org/9699112/diff/17004/crypto/keychain_mac.h
File crypto/keychain_mac.h (right):

http://codereview.chromium.org/9699112/diff/17004/crypto/keychain_mac.h#newco...
crypto/keychain_mac.h:23: class MacKeychain {
On 2012/03/19 22:42:28, wtc wrote:
> 
> The class is named MacKeychain, but the file is named
> keychain_mac.{cc,h} rather than mac_keychain.{cc,h}.
> I guess this is because we want to rely on the existing GYP
> rule that treat all _mac.* files as Mac only?

Yeah, that's what I would guess.  Although come to think of it, MockKeychain
should probably be mock_keychain_mac.*, too.  Maybe mac_keychain_mac would work
better?

[wtc, 2012-03-19 23:03:21]

http://codereview.chromium.org/9699112/diff/17004/crypto/keychain_mac.h
File crypto/keychain_mac.h (right):

http://codereview.chromium.org/9699112/diff/17004/crypto/keychain_mac.h#newco...
crypto/keychain_mac.h:23: class MacKeychain {

On 2012/03/19 22:50:03, akalin wrote:
> Maybe mac_keychain_mac would work better?

mac_keychain_mac looks weird.  I am OK with keychain_mac
(less work) or mac_keychain (requires a GYP change to list
thses files in source! for non-mac platforms).

I guess MockKeychain can be renamed MockMacKeychain and
the files renamed mock_mac_keychain.

Probably not worth the trouble...