src/crypto should build on the x86_64 architecture.

src/crypto should build on the x86_64 architecture.
This patch solves the build issues that are directly related to building crypto for the x86_64 architecture.

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

[agl, 2012-07-11 16:46:49]

(punting. This seems sane, but rsleevi or mark probably know enough OS X to say
for sure.)

[Ryan Sleevi, 2012-07-11 17:52:46]

Hi Mihai,

First, thanks for writing this.

Since it looks like your first patch (based on the AUTHORS file):
- When requesting a review, you generally only want one reviewer (
http://www.chromium.org/developers/contributing-code/#TOC-Request-review ). I've
removed Ricardo and Mark, but left Adam on since he's already commented.

- I'm unable to find an individual CLA for you, or a corporate CLA authorizing
you to contribute on behalf of Adobe (
http://www.chromium.org/developers/contributing-code/#TOC-Get-your-code-ready ).
Can you please work with your legal team to execute a corporate CLA? We will not
be able to commit this until then.


As to the patch itself, I'm a little concerned with the introduction of
uintptr_t here, since it's still being implicitly downsized to an 'unsigned int'
in the map.find() code.

Because of this, I would suggest a more explicit pattern to indicate this is
intentional:

unsigned int key = static_cast<unsigned
int>(reinterpret_cast<uintptr_t>(key_ref) - 1);

While I realize things 'just work' as you've written them, since key is
ultimately an 'unsigned int' being promoted to a uintptr_t, this makes the
reverse conversion clear.

What do you think?

[wtc, 2012-07-11 21:15:28]

Review comments on patch set 1:

1. The original code seems to be using both "int" and
"unsigned int" for an item reference.  We should fix this
inconsistency.

2. We can either use uintptr_t for an item reference, or
use the two-step casting that rsleevi suggested:

  unsigned int key = static_cast<unsigned
int>(reinterpret_cast<uintptr_t>(key_ref) - 1);

https://chromiumcodereview.appspot.com/10738003/diff/1/crypto/mock_keychain_m...
File crypto/mock_keychain_mac.h (right):

https://chromiumcodereview.appspot.com/10738003/diff/1/crypto/mock_keychain_m...
crypto/mock_keychain_mac.h:173: void SetTestDataPasswordString(int item, const
char* value);

All of these "int item" arguments probably should be changed
to "uintptr_t item".

https://chromiumcodereview.appspot.com/10738003/diff/1/crypto/mock_keychain_m...
crypto/mock_keychain_mac.h:192: mutable unsigned int next_item_key_;
The key type for the std::map's should probably also be
changed to uintptr_t.  Similarly, next_item_key_ should
probably be changed to uintptr_t.

[Mark Mentovai, 2012-07-12 18:47:08]

Ryan, please don’t remove me from any Mac 64-bit changes. I would like to see
them, even if there are other reviewers also on the hook.

I’ll defer passing judgment until Mihai has responded to Wan-Teh’s comments, but
in general this seems fine.

[wtc, 2012-07-12 23:55:07]

mark: it may be better for you or the original author of these
files to fix this.  This seems like a common 64-bit porting
problem where the compiler requires we cast a pointer to an
integer type large enough to avoid truncation.

Please also note the problem that Mihai describes in the
CL's description (which should be removed eventually):

There is also a warning regarding the & and | operator precedence in a specific
expression in a file that is not a member of crypto for which I don't have
enough information to solve:
/chromium/trunk/src/third_party/mach_override/mach_override.c:374:59:{374:25-374:97}{374:98-374:99}:
error: '&' within '|' [-Werror,-Wbitwise-op-parentheses]
vm_address_t first = (uint64_t)originalFunctionAddress &
~(uint64_t)(((uint64_t)1 << 31) - 1) | ((uint64_t)1 << 31); // start in the
middle of the page?

[Mihai Maerean, 2012-07-16 09:55:13]

I have already signed a corporate CLA for contributing to open source projects
and, also, there are already 6 other Adobe contributors in the AUTHORS file (
http://code.google.com/searchframe#OAMlx_jo-ck/src/AUTHORS ). The legal aspect
should be ok. Also, I have sent some emails inside Adobe to make sure all the
required steps are being performed.

Regarding the std::maps (keychain_attr_list_, keychain_data_,
remaining_search_results_), the methods (InitializeKeychainData, SetTestData*)
and other variables (next_item_key_, map iterators), they all should switch from
int (or unsigned int) to uintptr_t.
I will upload a new version with these changes included.

[Mihai Maerean, 2012-07-16 12:59:12]

I have uploaded the new version with the changes as discussed previously.

[Mihai Maerean, 2012-07-16 13:53:50]

the same changes but with no warnings from cpplint.

[Mark Mentovai, 2012-07-16 15:39:40]

https://chromiumcodereview.appspot.com/10738003/diff/11004/AUTHORS
File AUTHORS (right):

https://chromiumcodereview.appspot.com/10738003/diff/11004/AUTHORS#newcode196
AUTHORS:196: Mihai Maerean <mmaerean@adobe.com>
I’m checking on the status of the CLA right now. Sit tight.

[mal, 2012-07-16 18:10:26]

mmaerean@adobe.com is not listed in Adobe's authorized contributors list. I'll
start a thread with our contact at Adobe.

[wtc, 2012-07-16 23:00:40]

Review comments on patch set 3:

How do I do a 64-bit build on Mac OS X to test this patch?

https://chromiumcodereview.appspot.com/10738003/diff/11004/crypto/mock_keycha...
File crypto/mock_keychain_mac.cc (right):

https://chromiumcodereview.appspot.com/10738003/diff/11004/crypto/mock_keycha...
crypto/mock_keychain_mac.cc:115: const char* value) {

Nit: the second and third arguments need to be aligned with
the first argument.

https://chromiumcodereview.appspot.com/10738003/diff/11004/crypto/mock_keycha...
crypto/mock_keychain_mac.cc:134: SecAuthenticationType value) {

Nit: fix indentation of the second argument here and on line 170.

[Mihai Maerean, 2012-07-17 08:32:02]

Q: How do I do a 64-bit build on Mac OS X to test this patch?
A: You need to temporarily add 'ARCHS': 'x86_64' right after 'SDKROOT':
'macosx<(mac_sdk)' in build/common.gypi and then run gclient runhooks to
generate the xcodeproj files. Then you can build src/crypto/crypto.xcodeproj
using Xcode.

[wtc, 2012-07-18 02:34:58]

Mihai: thank you for the build instructions.

For third_party/mach_override/mach_override.c, just add
parentheses around the '&' expression, as the compiler
suggests.

I studied the code, and concluded that rsleevi's suggestion
of
  unsigned int key = static_cast<unsigned
int>(reinterpret_cast<uintptr_t>(key_ref) - 1);

is also correct and I have a working patch of that approach
at https://chromiumcodereview.appspot.com/10798003/ .
(Note: that CL is for illustration only, so I didn't fix
the coding style problems.)

Please take a look and let me know which approach you prefer.
Thanks.

[Mihai Maerean, 2012-07-18 14:02:50]

Regarding the warning related to the & operator in
third_party/mach_override/mach_override.c, should I add the fix to this
patch/review or should I create a separate issue & review because the file is in
a different project/folder?

On 2012/07/18 02:34:58, wtc wrote:
> For third_party/mach_override/mach_override.c, just add
> parentheses around the '&' expression, as the compiler
> suggests.

[Mihai Maerean, 2012-07-18 14:03:55]

The size of "unsigned int" is compiler specific and won't always be able to
store a memory address, while uintptr_t (unsigned long int) will.
Because of this, in the "unsigned int" variant, while very unlikely, it is
possible for 2 memory addresses (keys) to have the 32 least important bits the
same, which will lead to having just one entry in the maps and that will cause
bugs that are very hard to identify and reproduce.
So I prefer to use the type that ensures lossless operations, and that is
uintptr_t (like in the last patch that I have submitted).

On 2012/07/18 02:34:58, wtc wrote:
> I studied the code, and concluded that rsleevi's suggestion
> of
>   unsigned int key = static_cast<unsigned
> int>(reinterpret_cast<uintptr_t>(key_ref) - 1);
> 
> is also correct and I have a working patch of that approach
> at https://chromiumcodereview.appspot.com/10798003/ .
> (Note: that CL is for illustration only, so I didn't fix
> the coding style problems.)
> 
> Please take a look and let me know which approach you prefer.

[Mark Mentovai, 2012-07-18 15:16:37]

For mock_keychain, like Wan-Teh I’m at a loss to understand why you’ve widened
the item handle type to uintptr_t. Based on a quick read, a MockKeychain object
itself is the source of these values and it assigns them starting at 0 and
incrementing from there. I don’t see what the size of a native pointer has to do
with any of this. It would be helpful to share the precise compilation error you
experienced along with the changes you need to make (for this change and for
others) because it’s not always entirely obvious.

For mach_override, we’d want to send any changes we need to make upstream
anyway. It might be easier to just get your changes made upstream first, and
then we can import a new clean copy. The upstream is at
https://github.com/rentzsch/mach_star/tree/master/mach_override .

[wtc, 2012-07-18 17:43:20]

On 2012/07/18 15:16:37, Mark Mentovai wrote:
> It would be helpful to share the precise compilation error you
> experienced along with the changes you need to make (for this change and for
> others) because it’s not always entirely obvious.

I have done a 64-bit build of src/crypto, so I know what the
compilation errors are.  They all have to do with casting a
pointer (SecKeychainItemRef or CFTypeRef) to unsigned int or int
at lines 178, 207, 235, and 486.  You can see the compilation
error sites more clearly in
https://chromiumcodereview.appspot.com/10798003/diff/1/crypto/mock_keychain_m...
Search for the changes with static_cast.

GCC requires that we cast a pointer to an integer type large enough
to avoid truncation.  This is why we have to cast to uintptr_t in
either approach.  The question is whether we might as well change
the ultimate integer type to uintptr_t.

As you did, I've concluded that it is safe for the ultimate
integer type to remain unsigned int.  If you prefer this approach,
do you have any suggestion on dealing with this double cast expression?

  unsigned int key = static_cast<unsigned
int>(reinterpret_cast<uintptr_t>(itemRef)) - 1;

[Ryan Sleevi, 2012-07-18 17:47:20]

On 2012/07/18 17:43:20, wtc wrote:
> On 2012/07/18 15:16:37, Mark Mentovai wrote:
> > It would be helpful to share the precise compilation error you
> > experienced along with the changes you need to make (for this change and for
> > others) because it’s not always entirely obvious.
> 
> I have done a 64-bit build of src/crypto, so I know what the
> compilation errors are.  They all have to do with casting a
> pointer (SecKeychainItemRef or CFTypeRef) to unsigned int or int
> at lines 178, 207, 235, and 486.  You can see the compilation
> error sites more clearly in
>
https://chromiumcodereview.appspot.com/10798003/diff/1/crypto/mock_keychain_m...
> Search for the changes with static_cast.
> 
> GCC requires that we cast a pointer to an integer type large enough
> to avoid truncation.  This is why we have to cast to uintptr_t in
> either approach.  The question is whether we might as well change
> the ultimate integer type to uintptr_t.
> 
> As you did, I've concluded that it is safe for the ultimate
> integer type to remain unsigned int.  If you prefer this approach,
> do you have any suggestion on dealing with this double cast expression?
> 
>   unsigned int key = static_cast<unsigned
> int>(reinterpret_cast<uintptr_t>(itemRef)) - 1;

Right, this is why I suggested it in my original comment #2. We're extending an
int from 32-bits (int size) to 64-bits (SecKeychainRef, a pointer type). Any
SecKeyChainRef we receive via an API call (64-bits) should have been vended by
us, which means there's only 32-bits set (as long as we're not sending negative
values, since sign-bit extension).

The cast was/is my preference

[Mark Mentovai, 2012-07-18 17:57:52]

I see. I would prefer for the underlying type to be uintptr_t in that case.
Although we all agree that an int or unsigned int has enough range for the
application, the double cast you bring up is ugly and obscure. The mock Keychain
is only test code, and although it’s built into the actual application, it’s
only there for test purposes and is not active under normal circumstances, so
I’m not terribly concerned about the additional memory usage.

I’ve added Stuart to the Cc line as he’s the original author of the code, but
I’m fine with the proposed approach here. Still waiting on settling the CLA
before I can give it four magic letters, though.

[Mark Mentovai, 2012-07-18 18:01:14]

Rather than spewing uintptr_t everywhere, I would advise a MockKeychainItemType
typedef to the appropriate type (whatever we ultimately settle on for that
type).

[wtc, 2012-07-18 19:03:16]

Review comments on patch set 4:

I am fine with either approach.

I suggest two more changes for this approach.

https://chromiumcodereview.appspot.com/10738003/diff/15006/crypto/mock_keycha...
File crypto/mock_keychain_mac.h (right):

https://chromiumcodereview.appspot.com/10738003/diff/15006/crypto/mock_keycha...
crypto/mock_keychain_mac.h:215: mutable std::set<unsigned int> added_via_api_;
This should also be changed to uintptr_t (or the
MockKeychainItemType typedef that mark suggested).

https://chromiumcodereview.appspot.com/10738003/diff/15006/crypto/mock_keycha...
crypto/mock_keychain_mac.h:225: mutable unsigned int password_data_count_;
This should be "int".  It is not used by this class
internally.  It is exposed via the password_data_count()
getter, which returns "int".

[Mihai Maerean, 2012-07-19 11:32:24]

On 2012/07/18 18:01:14, Mark Mentovai wrote:
> Rather than spewing uintptr_t everywhere, I would advise a
MockKeychainItemType
> typedef to the appropriate type (whatever we ultimately settle on for that
> type).

Indeed, a typedef would be the proper way to do it.

While we're on the subject, do we agree on using uintptr_t for this
MockKeychainItemType? Personally, I vote in favor of uintptr_t.

[Mihai Maerean, 2012-07-19 11:33:26]

https://chromiumcodereview.appspot.com/10738003/diff/15006/crypto/mock_keycha...
File crypto/mock_keychain_mac.h (right):

https://chromiumcodereview.appspot.com/10738003/diff/15006/crypto/mock_keycha...
crypto/mock_keychain_mac.h:215: mutable std::set<unsigned int> added_via_api_;
True, added_via_api_ uses the same keys as keychain_attr_list_.

On 2012/07/18 19:03:16, wtc wrote:
> This should also be changed to uintptr_t (or the
> MockKeychainItemType typedef that mark suggested).

https://chromiumcodereview.appspot.com/10738003/diff/15006/crypto/mock_keycha...
crypto/mock_keychain_mac.h:225: mutable unsigned int password_data_count_;
Ok, I will change the type to int (although this should be committed as a
different patch than this).

On 2012/07/18 19:03:16, wtc wrote:
> This should be "int".  It is not used by this class
> internally.  It is exposed via the password_data_count()
> getter, which returns "int".

[Mihai Maerean, 2012-07-19 15:05:52]

On 2012/07/18 15:16:37, Mark Mentovai wrote:
> For mach_override, we’d want to send any changes we need to make upstream
> anyway. It might be easier to just get your changes made upstream first, and
> then we can import a new clean copy. The upstream is at
> https://github.com/rentzsch/mach_star/tree/master/mach_override .

I have sent the fix (https://github.com/rentzsch/mach_star/pull/37). When the
change is accepted, will it make it automatically into Chromium or is there
something we have to do by hand?

[Mark Mentovai, 2012-07-19 15:16:25]

Excellent. Once commited upstream, we need to import a new copy of mach_inject
to third_party/mach_override, updating the README.chromium file appropriately.

You should remove the verbiage that discusses mach_inject from this change’s
description.

[Mihai Maerean, 2012-07-19 15:23:32]

On 2012/07/19 15:16:25, Mark Mentovai wrote:
> Excellent. Once commited upstream, we need to import a new copy of mach_inject
> to third_party/mach_override, updating the README.chromium file appropriately.

Is that something I can do or you need to do it?

[Mark Mentovai, 2012-07-19 15:25:40]

Either of us can do it. It’s just a normal reviewable committable change. If
you’d prefer, I can do it once the upstream version lands.

[Mihai Maerean, 2012-07-19 16:30:14]

On 2012/07/19 15:25:40, Mark Mentovai wrote:
> Either of us can do it. It’s just a normal reviewable committable change. If
> you’d prefer, I can do it once the upstream version lands.

As of now, there are non trivial differences between the upstream version and
the one in Chromium.
I used http://www.diffnow.com/ to compare
https://raw.github.com/rentzsch/mach_star/master/mach_override/mach_override.c
and
https://src.chromium.org/viewvc/chrome/trunk/src/third_party/mach_override/ma...

So getting the upstream version will get more than just this fix.

[Mark Mentovai, 2012-07-19 16:37:26]

That’s expected. Code review serves as the audit and will catch anything
undesirable.

[wtc, 2012-07-19 17:59:26]

https://chromiumcodereview.appspot.com/10738003/diff/15006/crypto/mock_keycha...
File crypto/mock_keychain_mac.h (right):

https://chromiumcodereview.appspot.com/10738003/diff/15006/crypto/mock_keycha...
crypto/mock_keychain_mac.h:225: mutable unsigned int password_data_count_;

On 2012/07/19 11:33:26, Mihai Maerean wrote:
> Ok, I will change the type to int (although this should be committed as a
> different patch than this).

You're right.  I will make this change in
https://chromiumcodereview.appspot.com/10800038/

[stuartmorgan, 2012-07-19 18:35:14]

I don't have any opinion on the details of the types here; as Mark said, it's
test code, and the types that were there were just things that seemed convenient
at the time. As evidenced by all the horrible casting, it's not like it was
elegant to start with :)

https://chromiumcodereview.appspot.com/10738003/diff/27001/crypto/mock_keycha...
File crypto/mock_keychain_mac.h (right):

https://chromiumcodereview.appspot.com/10738003/diff/27001/crypto/mock_keycha...
crypto/mock_keychain_mac.h:19: // type used for the keys in the std::map(s)
Please follow Chromium style for comments; this should start with a capital
letter, and ened with a period. Same for the other comment.

[Mark Mentovai, 2012-07-20 12:33:26]

Ordinarily, we don’t just send out blank mails on a code review thread, except
for the first message in the thread where the CL description explains everything
and you’re just seeing review. If there’s something specific you’ve changed, if
you have something to ask or say to someone, or the patch needs another look in
general, you should say so. As it stands, I don’t know what your blank messages
mean.

[wtc, 2012-07-20 17:05:21]

Review comments on patch set 6:

mark: I believe Mihai meant to ask you to review patch set
6.

https://chromiumcodereview.appspot.com/10738003/diff/33001/crypto/mock_keycha...
File crypto/mock_keychain_mac.h (right):

https://chromiumcodereview.appspot.com/10738003/diff/33001/crypto/mock_keycha...
crypto/mock_keychain_mac.h:22: typedef uintptr_t MockKeychainItemType;

I believe these two typedefs should be merged into one.  The
'key' and 'item' arguments are referring to the same thing.

On the other hand, it may make sense to define a separate
typedef for kDummySearchRef, because it is cast to a
SecKeychainSearchRef.

[Mihai Maerean, 2012-07-23 11:15:33]

https://chromiumcodereview.appspot.com/10738003/diff/33001/crypto/mock_keycha...
File crypto/mock_keychain_mac.h (right):

https://chromiumcodereview.appspot.com/10738003/diff/33001/crypto/mock_keycha...
crypto/mock_keychain_mac.h:22: typedef uintptr_t MockKeychainItemType;
On 2012/07/20 17:05:21, wtc wrote: 
> I believe these two typedefs should be merged into one.  The
> 'key' and 'item' arguments are referring to the same thing.

There are 2 typedefs because the keys have to be uintptr_t while the type for
the item could, in theory, even be an uint16_t in this test code.
But since we decided to use uintptr_t for both keys and items, I will merge the
2 typedefs into a single typedef.


> it may make sense to define a separate
> typedef for kDummySearchRef, because it is cast to a
> SecKeychainSearchRef.

Because kDummySearchRef is cast to SecKeychainSearchRef, then it should be
declared of type SecKeychainSearchRef. Right?

[Mihai Maerean, 2012-07-23 11:27:22]

I have uploaded a new patch set: A single typedef and kDummySearchRef of type
SecKeychainSearchRef.

[Mihai Maerean, 2012-07-23 13:11:24]

On 2012/07/19 15:16:25, Mark Mentovai wrote:
> Excellent. Once commited upstream, we need to import a new copy of mach_inject
> to third_party/mach_override, updating the README.chromium file appropriately.

You're mentioning mach_inject. Is this a typing error? mach_inject is a
different folder than mach_override in mach_star
(https://github.com/rentzsch/mach_star) and has a different purpose.

The fix for Chromium has been committed in
https://github.com/rentzsch/mach_star/commit/34048d938af230c2c801e02d6c312f85...

I have created an issue for the need to update mach_override
(https://code.google.com/p/chromium/issues/detail?id=138535).

I will create a new code review to continue the talk about updating
mach_override, which will include all their latest changes, not just the fix for
Chromium.

[wtc, 2012-07-23 18:22:01]

Review comments on patch set 7:

https://chromiumcodereview.appspot.com/10738003/diff/33001/crypto/mock_keycha...
File crypto/mock_keychain_mac.cc (right):

https://chromiumcodereview.appspot.com/10738003/diff/33001/crypto/mock_keycha...
crypto/mock_keychain_mac.cc:99: SecKeychainAttribute* attribute =
AttributeWithTag(keychain_attr_list_[item],

This function is a good example of MockKeychainItemType and
MockKeychainKeyType being the same.

This function uses the 'item' argument as the key for the
std::map keychain_attr_list_.  This is why I think the typedefs
for the 'item' argument and the key for the std::maps should
be the same.  Did I misunderstand it?

https://chromiumcodereview.appspot.com/10738003/diff/21004/crypto/mock_keycha...
File crypto/mock_keychain_mac.cc (right):

https://chromiumcodereview.appspot.com/10738003/diff/21004/crypto/mock_keycha...
crypto/mock_keychain_mac.cc:12: (SecKeychainSearchRef) 1000;

Please use C++ reinterpret_cast<SecKeychainSearchRef>.

https://chromiumcodereview.appspot.com/10738003/diff/21004/crypto/mock_keycha...
File crypto/mock_keychain_mac.h (right):

https://chromiumcodereview.appspot.com/10738003/diff/21004/crypto/mock_keycha...
crypto/mock_keychain_mac.h:19: typedef uintptr_t MockKeychainKeyType;

I think it's better to name this type MockKeychainItemType.

"Key" is ambiguous because it can mean either
1) a cryptographic key, or
2) a key in a key-value pair (used for std::map).

[Mark Mentovai, 2012-07-23 21:46:57]

Note that the CLA stuff has all been addressed, and we can commit this once it
receives four magic letters.

[Mihai Maerean, 2012-07-24 09:03:36]

On 2012/07/23 18:22:01, wtc wrote:
> Please use C++ reinterpret_cast<SecKeychainSearchRef>.
> I think it's better to name this type MockKeychainItemType.

I have uploaded a new patch set that incorporates your feedback.

[Mark Mentovai, 2012-07-24 13:31:13]

https://chromiumcodereview.appspot.com/10738003/diff/28005/crypto/mock_keycha...
File crypto/mock_keychain_mac.cc (right):

https://chromiumcodereview.appspot.com/10738003/diff/28005/crypto/mock_keycha...
crypto/mock_keychain_mac.cc:192: reinterpret_cast<MockKeychainItemType>(itemRef)
- 1;
Continuation lines are indented by 4 spaces, not 2. Compare with lines 142 and
281. Also fix lines 222 and 251.

https://chromiumcodereview.appspot.com/10738003/diff/28005/crypto/mock_keycha...
crypto/mock_keychain_mac.cc:276: ::const_iterator it =
keychain_attr_list_.begin();
You’ve done inconsistent things with rewrapping these long lines in for loop
initializations. You seem to have preferred to break at the ::, but in this case
and on line 312, you put the :: on the second line, and on line 524, you put it
on the first line. You should be consistent. My preference is for the :: to come
on the first line, although in this case, it seems that you have enough
horizontal space to break at the space after const_iterator.

When a for construct grows unwieldy like this one has, it’s important to help
readers spot each statement. That usually means giving each one its own line. It
also means that if a statement eats up more than one line, you should treat
subsequent lines as continuations.

  for (std::map<MockKeychainItemType, SecKeychainAttributeList>::const_iterator
           it = keychain_attr_list_.begin();
       it != keychain_attr_list_.end();
       ++it) {

Don’t worry about using four lines instead of three. It’s more readable now.

As mentioned, the for constructs around lines 312 and 524 need revision too.

https://chromiumcodereview.appspot.com/10738003/diff/28005/crypto/mock_keycha...
File crypto/mock_keychain_mac.h (right):

https://chromiumcodereview.appspot.com/10738003/diff/28005/crypto/mock_keycha...
crypto/mock_keychain_mac.h:7: 
You should #include <stdint.h> to make sure uintptr_t is available.

[Mihai Maerean, 2012-07-24 15:15:12]

On 2012/07/24 13:31:13, Mark Mentovai wrote:
> When a for construct grows unwieldy like this one has, it’s important to help
> readers spot each statement. That usually means giving each one its own line.
It
> also means that if a statement eats up more than one line, you should treat
> subsequent lines as continuations.
> 
>   for (std::map<MockKeychainItemType,
SecKeychainAttributeList>::const_iterator
>            it = keychain_attr_list_.begin();
>        it != keychain_attr_list_.end();
>        ++it) {
> Don’t worry about using four lines instead of three. It’s more readable now.

>
https://chromiumcodereview.appspot.com/10738003/diff/28005/crypto/mock_keycha...
> crypto/mock_keychain_mac.h:7: 
> You should #include <stdint.h> to make sure uintptr_t is available.

I have fixed the indentation (in patch set #9) and I have added stdint.h (in
patch set #10).

Just my $0.02: the 80 characters limit for the length of line of text is way too
restrictive. We shouldn't be forced to write a for statement on 4 lines and a
simple variable declaration on 2 lines. Screens these days can easily display 3
times more characters per line. A limit of 100 characters (or even 120) would be
more appropriate. just saying.

[stuartmorgan, 2012-07-24 15:22:02]

On 2012/07/24 15:15:12, Mihai Maerean wrote:
> Just my $0.02: the 80 characters limit for the length of line of text is way
too
> restrictive.

Everyone has an opinion about what color this bike shed would be; trying to
re-open a general style discussion that's been had over and over in a random CL
isn't productive.  I guarantee that one more person's opinion isn't going to
make us diverge from the Google Style Guide on this.

[Mark Mentovai, 2012-07-24 15:30:00]

Thank you for your input.

https://chromiumcodereview.appspot.com/10738003/diff/25005/crypto/mock_keycha...
File crypto/mock_keychain_mac.h (right):

https://chromiumcodereview.appspot.com/10738003/diff/25005/crypto/mock_keycha...
crypto/mock_keychain_mac.h:10: #include <stdint.h>
C system headers belong in a separate section from C++ system headers, with the
C headers coming first. In Chromium code, there is a blank line between
sections. See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_...
.

[Mihai Maerean, 2012-07-24 15:42:42]

I have fixed the location of C header include.

gcl upload calls "presubmit upload checks" which should include calling lint on
the change.

[Mark Mentovai, 2012-07-24 15:45:07]

LGTM.

Wan-Teh, since you also reviewed this change carefully: are you satisfied with
it now?

[wtc, 2012-07-26 19:07:00]

Patch set 11 LGTM.

https://chromiumcodereview.appspot.com/10738003/diff/28006/crypto/mock_keycha...
File crypto/mock_keychain_mac.cc (right):

https://chromiumcodereview.appspot.com/10738003/diff/28006/crypto/mock_keycha...
crypto/mock_keychain_mac.cc:11: const SecKeychainSearchRef
MockKeychain::kDummySearchRef =
Nit: we often add a
  // static
comment for definitions of static members and static methods.

[Mihai Maerean, 2012-07-30 08:34:53]

On 2012/07/26 19:07:00, wtc wrote:
> Patch set 11 LGTM.
>
> crypto/mock_keychain_mac.cc:11: const SecKeychainSearchRef
> MockKeychain::kDummySearchRef =
> Nit: we often add a  // static
> comment for definitions of static members and static methods.

I have submitted a patch set with the "static" comment for
MockKeychain::kDummySearchRef.

[commit-bot: I haz the power, 2012-07-30 19:16:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmaerean@adobe.com/10738003/40001

[commit-bot: I haz the power, 2012-07-30 22:11:14]

Change committed as 149047