Make USE_OPENSSL and USE_NSS mutually exclusive

Make USE_OPENSSL and USE_NSS mutually exclusive
- that is, defining use_openssl=1 will now remove all dependency on NSS.

This does not impact any of the standard, non-openssl builds.

Adds stub implementations of several files that need to be fully implemented in follow up patchs.
Firefox import code will need some more substatial refactoring, as it makes little sense to be
attempting a firefox import without NSS libraries to hand, however the UI etc has numerous assumption
about the presence of this importer.


BUG=None
TEST=None

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

[joth, 2010-10-19 20:23:09]

I still need to go through this one more time (mostly adding all the missing
TODOs in) but just sending it now in case you wanted an early look.

[joth, 2010-10-20 19:35:55]

Wan-Teh,

Sorry to land this large patch on you, it's all the loose-ends to remove NSS
from the OpenSSL build.
Maybe if you could skim it and highlight which bits if any are outside your
review zone, and let me know, I can split them out. Or else copy more people
into this bug, if it's a manageable number.

Thanks in advance,
Joth

[bulach, 2010-10-21 11:23:57]

LGTM (but let Wan-Teh have the last word on it).

one important comment about the ScopedOpenSSLMinSizeBuffer and a few nits below.

it'd be nice to add a note to the patch description that there are no functional
changes for NSS / default builds.

http://codereview.chromium.org/3855004/diff/15001/16002
File base/base.gypi (right):

http://codereview.chromium.org/3855004/diff/15001/16002#newcode596
base/base.gypi:596: [ 'use_openssl==1', {
nit: spaces around ==

http://codereview.chromium.org/3855004/diff/15001/16002#newcode600
base/base.gypi:600: }, {  # use_openssl==0
nit: spaces around ==

http://codereview.chromium.org/3855004/diff/15001/16004
File base/crypto/rsa_private_key_openssl.cc (right):

http://codereview.chromium.org/3855004/diff/15001/16004#newcode15
base/crypto/rsa_private_key_openssl.cc:15: return new RSAPrivateKey;
nit: add ()

http://codereview.chromium.org/3855004/diff/15001/16004#newcode28
base/crypto/rsa_private_key_openssl.cc:28: false /* permanent */,
see 20-21 and 43-44: I'd prefer removing the "not" everywhere, otherwise it'll
get quite confusing.

http://codereview.chromium.org/3855004/diff/15001/16004#newcode79
base/crypto/rsa_private_key_openssl.cc:79: 
--\n

http://codereview.chromium.org/3855004/diff/15001/16005
File base/crypto/signature_creator_openssl.cc (right):

http://codereview.chromium.org/3855004/diff/15001/16005#newcode13
base/crypto/signature_creator_openssl.cc:13: return new SignatureCreator;
()

http://codereview.chromium.org/3855004/diff/15001/16005#newcode33
base/crypto/signature_creator_openssl.cc:33: 
--\n

http://codereview.chromium.org/3855004/diff/15001/16008
File base/crypto/symmetric_key_openssl.cc (right):

http://codereview.chromium.org/3855004/diff/15001/16008#newcode47
base/crypto/symmetric_key_openssl.cc:47: 
--n

http://codereview.chromium.org/3855004/diff/15001/16009
File base/hmac_openssl.cc (right):

http://codereview.chromium.org/3855004/diff/15001/16009#newcode53
base/hmac_openssl.cc:53: result.buffer(), NULL);
looks like this won't work if digest_length < EVP_MAX_MD_SIZE, as it'll pass
digest (rather than the output_buffer).

http://codereview.chromium.org/3855004/diff/15001/16010
File base/openssl_util.h (right):

http://codereview.chromium.org/3855004/diff/15001/16010#newcode17
base/openssl_util.h:17: // destruction, with truncation as appropriate.
s/with truncation/if the caller output buffer is large enough.

http://codereview.chromium.org/3855004/diff/15001/16010#newcode22
base/openssl_util.h:22: : output_(reinterpret_cast<unsigned char*>(output)),
len_(len) {}
{\n
}

http://codereview.chromium.org/3855004/diff/15001/16010#newcode27
base/openssl_util.h:27: }
\n

http://codereview.chromium.org/3855004/diff/15001/16010#newcode28
base/openssl_util.h:28: unsigned char* buffer() { return using_buffer() ?
buffer_ : output_; }
see above.. perhaps:
unsigned char* min_sized_buffer() { return buffer_; }

http://codereview.chromium.org/3855004/diff/15001/16010#newcode31
base/openssl_util.h:31: bool using_buffer() const { return len_ < SIZE; }
maybe: has_enough_output_space()?

http://codereview.chromium.org/3855004/diff/15001/16010#newcode35
base/openssl_util.h:35: unsigned char buffer_[SIZE];
maybe: min_sized_buffer_

http://codereview.chromium.org/3855004/diff/15001/16012
File build/all.gyp (right):

http://codereview.chromium.org/3855004/diff/15001/16012#newcode136
build/all.gyp:136: ['use_openssl==1', {
perhaps invert to != 1 and remove the first {} and the comment?

http://codereview.chromium.org/3855004/diff/15001/16013
File build/build_config.h (right):

http://codereview.chromium.org/3855004/diff/15001/16013#newcode53
build/build_config.h:53: #endif
#endif // USE_NSS

http://codereview.chromium.org/3855004/diff/15001/16017
File chrome/browser/importer/nss_decryptor_null.h (right):

http://codereview.chromium.org/3855004/diff/15001/16017#newcode18
chrome/browser/importer/nss_decryptor_null.h:18: struct PasswordForm;
perhaps one line?

http://codereview.chromium.org/3855004/diff/15001/16020
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/3855004/diff/15001/16020#newcode217
chrome/common/pref_names.cc:217: // and used by NSS-using OSes.
maybe: OpenSSL support is being implemented.

http://codereview.chromium.org/3855004/diff/15001/16024
File net/http/des.cc (right):

http://codereview.chromium.org/3855004/diff/15001/16024#newcode91
net/http/des.cc:91: // TODO(joth): implement me.
may be add: consider splitting into multiple files rather than large #if defined
sections.

[joth, 2010-10-21 14:47:58]

Thanks, think those are all addressed, although I've preferred to keep
consistent with existing in several cases.

http://codereview.chromium.org/3855004/diff/15001/16002
File base/base.gypi (right):

http://codereview.chromium.org/3855004/diff/15001/16002#newcode596
base/base.gypi:596: [ 'use_openssl==1', {
On 2010/10/21 11:23:57, bulach wrote:
> nit: spaces around ==

spaces only needed for OS comparisons AFAICT.
I fixed my other change to not have the spaces.

http://codereview.chromium.org/3855004/diff/15001/16002#newcode600
base/base.gypi:600: }, {  # use_openssl==0
On 2010/10/21 11:23:57, bulach wrote:
> nit: spaces around ==

ditto

http://codereview.chromium.org/3855004/diff/15001/16004
File base/crypto/rsa_private_key_openssl.cc (right):

http://codereview.chromium.org/3855004/diff/15001/16004#newcode15
base/crypto/rsa_private_key_openssl.cc:15: return new RSAPrivateKey;
On 2010/10/21 11:23:57, bulach wrote:
> nit: add ()

not done - Leaving consistent with NSS

http://codereview.chromium.org/3855004/diff/15001/16004#newcode28
base/crypto/rsa_private_key_openssl.cc:28: false /* permanent */,
On 2010/10/21 11:23:57, bulach wrote:
> see 20-21 and 43-44: I'd prefer removing the "not" everywhere, otherwise it'll
> get quite confusing.

good spot. this is meant to be true.

http://codereview.chromium.org/3855004/diff/15001/16004#newcode79
base/crypto/rsa_private_key_openssl.cc:79: 
On 2010/10/21 11:23:57, bulach wrote:
> --\n

Done.

http://codereview.chromium.org/3855004/diff/15001/16005
File base/crypto/signature_creator_openssl.cc (right):

http://codereview.chromium.org/3855004/diff/15001/16005#newcode13
base/crypto/signature_creator_openssl.cc:13: return new SignatureCreator;
On 2010/10/21 11:23:57, bulach wrote:
> ()

not done - Leaving consistent with NSS

http://codereview.chromium.org/3855004/diff/15001/16005#newcode33
base/crypto/signature_creator_openssl.cc:33: 
On 2010/10/21 11:23:57, bulach wrote:
> --\n

Done.

http://codereview.chromium.org/3855004/diff/15001/16008
File base/crypto/symmetric_key_openssl.cc (right):

http://codereview.chromium.org/3855004/diff/15001/16008#newcode47
base/crypto/symmetric_key_openssl.cc:47: 
On 2010/10/21 11:23:57, bulach wrote:
> --n

Done.

http://codereview.chromium.org/3855004/diff/15001/16009
File base/hmac_openssl.cc (right):

http://codereview.chromium.org/3855004/diff/15001/16009#newcode53
base/hmac_openssl.cc:53: result.buffer(), NULL);
On 2010/10/21 11:23:57, bulach wrote:
> looks like this won't work if digest_length < EVP_MAX_MD_SIZE, as it'll pass
> digest (rather than the output_buffer).

It does work. tests never lie.
Clarified the safe buffer impl to make this clearer.

http://codereview.chromium.org/3855004/diff/15001/16010
File base/openssl_util.h (right):

http://codereview.chromium.org/3855004/diff/15001/16010#newcode17
base/openssl_util.h:17: // destruction, with truncation as appropriate.
On 2010/10/21 11:23:57, bulach wrote:
> s/with truncation/if the caller output buffer is large enough.

Still stated it is truncating, but renamed stuff and added comments to make it
clearer.

http://codereview.chromium.org/3855004/diff/15001/16010#newcode22
base/openssl_util.h:22: : output_(reinterpret_cast<unsigned char*>(output)),
len_(len) {}
On 2010/10/21 11:23:57, bulach wrote:
> {\n
> }

Done.

http://codereview.chromium.org/3855004/diff/15001/16010#newcode27
base/openssl_util.h:27: }
On 2010/10/21 11:23:57, bulach wrote:
> \n

Done.

http://codereview.chromium.org/3855004/diff/15001/16010#newcode28
base/openssl_util.h:28: unsigned char* buffer() { return using_buffer() ?
buffer_ : output_; }
On 2010/10/21 11:23:57, bulach wrote:
> see above.. perhaps:
> unsigned char* min_sized_buffer() { return buffer_; }
safe_buffer()

http://codereview.chromium.org/3855004/diff/15001/16010#newcode31
base/openssl_util.h:31: bool using_buffer() const { return len_ < SIZE; }
On 2010/10/21 11:23:57, bulach wrote:
> maybe: has_enough_output_space()?

I now think it's easier to read without this method at all.

http://codereview.chromium.org/3855004/diff/15001/16010#newcode35
base/openssl_util.h:35: unsigned char buffer_[SIZE];
On 2010/10/21 11:23:57, bulach wrote:
> maybe: min_sized_buffer_

Done.

http://codereview.chromium.org/3855004/diff/15001/16012
File build/all.gyp (right):

http://codereview.chromium.org/3855004/diff/15001/16012#newcode136
build/all.gyp:136: ['use_openssl==1', {
On 2010/10/21 11:23:57, bulach wrote:
> perhaps invert to != 1 and remove the first {} and the comment?

Done.

http://codereview.chromium.org/3855004/diff/15001/16013
File build/build_config.h (right):

http://codereview.chromium.org/3855004/diff/15001/16013#newcode53
build/build_config.h:53: #endif
On 2010/10/21 11:23:57, bulach wrote:
> #endif // USE_NSS

No other endif has a comment in this file.
I reworked it to promote self-evidence and reduce nesting. WDYT?

http://codereview.chromium.org/3855004/diff/15001/16017
File chrome/browser/importer/nss_decryptor_null.h (right):

http://codereview.chromium.org/3855004/diff/15001/16017#newcode18
chrome/browser/importer/nss_decryptor_null.h:18: struct PasswordForm;
On 2010/10/21 11:23:57, bulach wrote:
> perhaps one line?

Non standard for chrome, not done.

http://codereview.chromium.org/3855004/diff/15001/16020
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/3855004/diff/15001/16020#newcode217
chrome/common/pref_names.cc:217: // and used by NSS-using OSes.
On 2010/10/21 11:23:57, bulach wrote:
> maybe: OpenSSL support is being implemented.

Done.
Sort of. I just added OpenSSL to the existing comment, to avoid it rotting.

FWIW I notice the build-check in chrome_browser for
ssl_config_service_manager_pref.cc is based on OS=linux, not nss/openssl. I
guess the *BSDs aren't building at the moment.

http://codereview.chromium.org/3855004/diff/15001/16024
File net/http/des.cc (right):

http://codereview.chromium.org/3855004/diff/15001/16024#newcode91
net/http/des.cc:91: // TODO(joth): implement me.
On 2010/10/21 11:23:57, bulach wrote:
> may be add: consider splitting into multiple files rather than large #if
defined
> sections.

Done.

[agl, 2010-10-21 15:26:19]

http://codereview.chromium.org/3855004/diff/30001/31003
File base/crypto/encryptor_openssl.cc (right):

http://codereview.chromium.org/3855004/diff/30001/31003#newcode18
base/crypto/encryptor_openssl.cc:18: // TODO(joth): implement me.
We have a NOTIMPLEMENTED macro which should be used in these functions.

http://codereview.chromium.org/3855004/diff/30001/31004
File base/crypto/rsa_private_key_openssl.cc (right):

http://codereview.chromium.org/3855004/diff/30001/31004#newcode36
base/crypto/rsa_private_key_openssl.cc:36: return NULL;
Ditto about NOTIMPLEMENTED

http://codereview.chromium.org/3855004/diff/30001/31005
File base/crypto/signature_creator_openssl.cc (right):

http://codereview.chromium.org/3855004/diff/30001/31005#newcode24
base/crypto/signature_creator_openssl.cc:24: return false;
NOTIMPLEMENTED

http://codereview.chromium.org/3855004/diff/30001/31006
File base/crypto/signature_verifier_openssl.cc (right):

http://codereview.chromium.org/3855004/diff/30001/31006#newcode27
base/crypto/signature_verifier_openssl.cc:27: // TODO(joth): implement me.
...

http://codereview.chromium.org/3855004/diff/30001/31008
File base/crypto/symmetric_key_openssl.cc (right):

http://codereview.chromium.org/3855004/diff/30001/31008#newcode20
base/crypto/symmetric_key_openssl.cc:20: // TODO(joth): implement me.
...

http://codereview.chromium.org/3855004/diff/30001/31010
File base/openssl_util.h (right):

http://codereview.chromium.org/3855004/diff/30001/31010#newcode26
base/openssl_util.h:26: min_sized_buffer_() {
I think that you can just remove this line.

http://codereview.chromium.org/3855004/diff/30001/31010#newcode34
base/openssl_util.h:34: // else any writing alreday happen directly into
|output_|.
typos: 'alreday' and 'happened'

http://codereview.chromium.org/3855004/diff/30001/31024
File net/http/des.cc (right):

http://codereview.chromium.org/3855004/diff/30001/31024#newcode91
net/http/des.cc:91: // TODO(joth): implement me. And consider splitting this
file by platform.
NOTIMPLEMENTED

[wtc, 2010-10-21 16:25:18]

Joth: I will try to review this CL today.

[joth, 2010-10-21 17:06:21]

http://codereview.chromium.org/3855004/diff/30001/31003
File base/crypto/encryptor_openssl.cc (right):

http://codereview.chromium.org/3855004/diff/30001/31003#newcode18
base/crypto/encryptor_openssl.cc:18: // TODO(joth): implement me.
On 2010/10/21 15:26:20, agl wrote:
> We have a NOTIMPLEMENTED macro which should be used in these functions.

Done.

http://codereview.chromium.org/3855004/diff/30001/31005
File base/crypto/signature_creator_openssl.cc (right):

http://codereview.chromium.org/3855004/diff/30001/31005#newcode24
base/crypto/signature_creator_openssl.cc:24: return false;
On 2010/10/21 15:26:20, agl wrote:
> NOTIMPLEMENTED

Done.

http://codereview.chromium.org/3855004/diff/30001/31006
File base/crypto/signature_verifier_openssl.cc (right):

http://codereview.chromium.org/3855004/diff/30001/31006#newcode27
base/crypto/signature_verifier_openssl.cc:27: // TODO(joth): implement me.
On 2010/10/21 15:26:20, agl wrote:
> ...

Done.

http://codereview.chromium.org/3855004/diff/30001/31008
File base/crypto/symmetric_key_openssl.cc (right):

http://codereview.chromium.org/3855004/diff/30001/31008#newcode20
base/crypto/symmetric_key_openssl.cc:20: // TODO(joth): implement me.
On 2010/10/21 15:26:20, agl wrote:
> ...

Done.

http://codereview.chromium.org/3855004/diff/30001/31010
File base/openssl_util.h (right):

http://codereview.chromium.org/3855004/diff/30001/31010#newcode26
base/openssl_util.h:26: min_sized_buffer_() {
On 2010/10/21 15:26:20, agl wrote:
> I think that you can just remove this line.

Done.

http://codereview.chromium.org/3855004/diff/30001/31010#newcode34
base/openssl_util.h:34: // else any writing alreday happen directly into
|output_|.
On 2010/10/21 15:26:20, agl wrote:
> typos: 'alreday' and 'happened'

Done.

[joth, 2010-10-21 17:08:42]

On 21 October 2010 16:25, <wtc@chromium.org> wrote:

> Joth: I will try to review this CL today.
>
>
>
Great, thanks!
I just made all Adam's suggestions, so it should be getting pretty close
now...

[wtc, 2010-10-21 19:29:32]

LGTM.

There is a bug in base/base.gyp, marked with "BUG" below.
I have two other comments marked with "BUG", but they are
less serious.

All other comments are just nits.

You can commit this CL after addressing my comments.
You can ignore some of the nits.

http://codereview.chromium.org/3855004/diff/41001/42001
File base/base.gyp (left):

http://codereview.chromium.org/3855004/diff/41001/42001#oldcode73
base/base.gyp:73: 'crypto/rsa_private_key_nss_unittest.cc',
BUG: do not remove this line.  You already exclude this file
from sources at line 229 below.

http://codereview.chromium.org/3855004/diff/41001/42002
File base/base.gypi (right):

http://codereview.chromium.org/3855004/diff/41001/42002#newcode464
base/base.gypi:464: 'sha2.h',
Nit: undo this change since .h sorts after .cc.

http://codereview.chromium.org/3855004/diff/41001/42002#newcode524
base/base.gypi:524: 'crypto/encryptor_nss.cc',
Nit: you can exclude these _nss.cc files using a pattern.
See lines 492-495 above for an example.

You can also exclude the third_party/nss and the
_openssl.cc files using a pattern.

http://codereview.chromium.org/3855004/diff/41001/42002#newcode529
base/base.gypi:529: 'third_party/nss/blapi.h',
Nit: list these third_party files last (in alphabetical order).

http://codereview.chromium.org/3855004/diff/41001/42003
File base/crypto/encryptor_openssl.cc (right):

http://codereview.chromium.org/3855004/diff/41001/42003#newcode30
base/crypto/encryptor_openssl.cc:30: 
Nit: delete the blank line.

http://codereview.chromium.org/3855004/diff/41001/42004
File base/crypto/rsa_private_key_openssl.cc (right):

http://codereview.chromium.org/3855004/diff/41001/42004#newcode16
base/crypto/rsa_private_key_openssl.cc:16: return new RSAPrivateKey;
Nit: you may want to return NULL instead of new RSAPrivateKey.

http://codereview.chromium.org/3855004/diff/41001/42005
File base/crypto/signature_creator_openssl.cc (right):

http://codereview.chromium.org/3855004/diff/41001/42005#newcode13
base/crypto/signature_creator_openssl.cc:13: return new SignatureCreator;
Nit: return NULL instead?

http://codereview.chromium.org/3855004/diff/41001/42007
File base/crypto/symmetric_key.h (right):

http://codereview.chromium.org/3855004/diff/41001/42007#newcode69
base/crypto/symmetric_key.h:69: SymmetricKey();
BUG: this looks incorrect.  The constructor should take an
OpenSSL symmetric key as input.  Add a comment to note that
this is not implemented?

http://codereview.chromium.org/3855004/diff/41001/42008
File base/crypto/symmetric_key_openssl.cc (right):

http://codereview.chromium.org/3855004/diff/41001/42008#newcode21
base/crypto/symmetric_key_openssl.cc:21: return new SymmetricKey();
Nit: it may be better to return NULL from these functions.

http://codereview.chromium.org/3855004/diff/41001/42009
File base/hmac_openssl.cc (right):

http://codereview.chromium.org/3855004/diff/41001/42009#newcode7
base/hmac_openssl.cc:7: #include <algorithm>
Nit: the Style Guide recommends that we list C system headers
before C++ system headers, so these should be separated into
two groups:

#include <openssl/hmac.h>

#include <algorithm>
#include <vector>

http://codereview.chromium.org/3855004/diff/41001/42010
File base/openssl_util.h (right):

http://codereview.chromium.org/3855004/diff/41001/42010#newcode18
base/openssl_util.h:18: // that of required size and then its content is
automatically copied out on
Nit: remove 'that'?

http://codereview.chromium.org/3855004/diff/41001/42010#newcode26
base/openssl_util.h:26: }
Nit: indentation of closing '}'.

http://codereview.chromium.org/3855004/diff/41001/42011
File base/sha2_openssl.cc (right):

http://codereview.chromium.org/3855004/diff/41001/42011#newcode1
base/sha2_openssl.cc:1: // Copyright (c) 2006-2008 The Chromium Authors. All
rights reserved.
Nit: 2006-2008 => 2010

http://codereview.chromium.org/3855004/diff/41001/42011#newcode19
base/sha2_openssl.cc:19: ::SHA256(reinterpret_cast<const unsigned
char*>(str.c_str()), str.size(),
Nit: str.c_str() => str.data()
to avoid the conversion to a C string.

http://codereview.chromium.org/3855004/diff/41001/42013
File build/build_config.h (right):

http://codereview.chromium.org/3855004/diff/41001/42013#newcode51
build/build_config.h:51: #define USE_NSS 1  // Default to use NSS for crypto,
unless OPENSSL is chosen.
Nit: do not spell "OpenSSL" in all caps (here and line 57 below).

http://codereview.chromium.org/3855004/diff/41001/42016
File chrome/browser/importer/nss_decryptor.h (right):

http://codereview.chromium.org/3855004/diff/41001/42016#newcode17
chrome/browser/importer/nss_decryptor.h:17: // defined. (Unless there is someway
to do nss decrypt with OpenSSL). Ideally
Nit: someway => some way

http://codereview.chromium.org/3855004/diff/41001/42018
File chrome/browser/sync/util/nigori.cc (right):

http://codereview.chromium.org/3855004/diff/41001/42018#newcode10
chrome/browser/sync/util/nigori.cc:10: #include <arpa/inet.h>
BUG: does this change belong in this CL?

Why do we need to include <arpa/inet.h>?  For htonl?
http://www.opengroup.org/onlinepubs/9699919799/functions/htonl.html

[joth, 2010-10-22 13:57:34]

Many thanks for the detailed comments. All addressed except for using a pattern
to exclude files in the gypi -- I'll do that in a follow up.
I've gone ahead and landed (to get it through the builders before MTV wakes
up...)

Cheers!

http://codereview.chromium.org/3855004/diff/41001/42001
File base/base.gyp (left):

http://codereview.chromium.org/3855004/diff/41001/42001#oldcode73
base/base.gyp:73: 'crypto/rsa_private_key_nss_unittest.cc',
On 2010/10/21 19:29:33, wtc wrote:
> BUG: do not remove this line.  You already exclude this file
> from sources at line 229 below.

Done.
Thanks, that was a silly ctrl-X / ctrl-C error.

http://codereview.chromium.org/3855004/diff/41001/42002
File base/base.gypi (right):

http://codereview.chromium.org/3855004/diff/41001/42002#newcode464
base/base.gypi:464: 'sha2.h',
On 2010/10/21 19:29:33, wtc wrote:
> Nit: undo this change since .h sorts after .cc.

Done.

http://codereview.chromium.org/3855004/diff/41001/42002#newcode524
base/base.gypi:524: 'crypto/encryptor_nss.cc',
On 2010/10/21 19:29:33, wtc wrote:
> Nit: you can exclude these _nss.cc files using a pattern.
> See lines 492-495 above for an example.
> 
> You can also exclude the third_party/nss and the
> _openssl.cc files using a pattern.

Good point. I've found it very useful to be able to match files explicitly with
grep etc whilst doing this, so if it's OK I've added a TODO to use a pattern in
a followup once these changes have safely landed

http://codereview.chromium.org/3855004/diff/41001/42002#newcode529
base/base.gypi:529: 'third_party/nss/blapi.h',
On 2010/10/21 19:29:33, wtc wrote:
> Nit: list these third_party files last (in alphabetical order).

Done.

http://codereview.chromium.org/3855004/diff/41001/42003
File base/crypto/encryptor_openssl.cc (right):

http://codereview.chromium.org/3855004/diff/41001/42003#newcode30
base/crypto/encryptor_openssl.cc:30: 
On 2010/10/21 19:29:33, wtc wrote:
> Nit: delete the blank line.

Done.

http://codereview.chromium.org/3855004/diff/41001/42004
File base/crypto/rsa_private_key_openssl.cc (right):

http://codereview.chromium.org/3855004/diff/41001/42004#newcode16
base/crypto/rsa_private_key_openssl.cc:16: return new RSAPrivateKey;
On 2010/10/21 19:29:33, wtc wrote:
> Nit: you may want to return NULL instead of new RSAPrivateKey.

Done.

http://codereview.chromium.org/3855004/diff/41001/42005
File base/crypto/signature_creator_openssl.cc (right):

http://codereview.chromium.org/3855004/diff/41001/42005#newcode13
base/crypto/signature_creator_openssl.cc:13: return new SignatureCreator;
On 2010/10/21 19:29:33, wtc wrote:
> Nit: return NULL instead?

Done.

http://codereview.chromium.org/3855004/diff/41001/42007
File base/crypto/symmetric_key.h (right):

http://codereview.chromium.org/3855004/diff/41001/42007#newcode69
base/crypto/symmetric_key.h:69: SymmetricKey();
On 2010/10/21 19:29:33, wtc wrote:
> BUG: this looks incorrect.  The constructor should take an
> OpenSSL symmetric key as input.  Add a comment to note that
> this is not implemented?

Done.
Yes, this is a placeholder.

http://codereview.chromium.org/3855004/diff/41001/42008
File base/crypto/symmetric_key_openssl.cc (right):

http://codereview.chromium.org/3855004/diff/41001/42008#newcode21
base/crypto/symmetric_key_openssl.cc:21: return new SymmetricKey();
On 2010/10/21 19:29:33, wtc wrote:
> Nit: it may be better to return NULL from these functions.

Done.

http://codereview.chromium.org/3855004/diff/41001/42009
File base/hmac_openssl.cc (right):

http://codereview.chromium.org/3855004/diff/41001/42009#newcode7
base/hmac_openssl.cc:7: #include <algorithm>
On 2010/10/21 19:29:33, wtc wrote:
> Nit: the Style Guide recommends that we list C system headers
> before C++ system headers, so these should be separated into
> two groups:
> 
> #include <openssl/hmac.h>
> 
> #include <algorithm>
> #include <vector>

Done.

http://codereview.chromium.org/3855004/diff/41001/42010
File base/openssl_util.h (right):

http://codereview.chromium.org/3855004/diff/41001/42010#newcode18
base/openssl_util.h:18: // that of required size and then its content is
automatically copied out on
On 2010/10/21 19:29:33, wtc wrote:
> Nit: remove 'that'?

Done.

http://codereview.chromium.org/3855004/diff/41001/42010#newcode26
base/openssl_util.h:26: }
On 2010/10/21 19:29:33, wtc wrote:
> Nit: indentation of closing '}'.

Done.

http://codereview.chromium.org/3855004/diff/41001/42011
File base/sha2_openssl.cc (right):

http://codereview.chromium.org/3855004/diff/41001/42011#newcode1
base/sha2_openssl.cc:1: // Copyright (c) 2006-2008 The Chromium Authors. All
rights reserved.
On 2010/10/21 19:29:33, wtc wrote:
> Nit: 2006-2008 => 2010

Done.

http://codereview.chromium.org/3855004/diff/41001/42011#newcode19
base/sha2_openssl.cc:19: ::SHA256(reinterpret_cast<const unsigned
char*>(str.c_str()), str.size(),
On 2010/10/21 19:29:33, wtc wrote:
> Nit: str.c_str() => str.data()
> to avoid the conversion to a C string.

Done. (And in hmac_openssl HMAC::Sign)

http://codereview.chromium.org/3855004/diff/41001/42013
File build/build_config.h (right):

http://codereview.chromium.org/3855004/diff/41001/42013#newcode51
build/build_config.h:51: #define USE_NSS 1  // Default to use NSS for crypto,
unless OPENSSL is chosen.
On 2010/10/21 19:29:33, wtc wrote:
> Nit: do not spell "OpenSSL" in all caps (here and line 57 below).

Done.

http://codereview.chromium.org/3855004/diff/41001/42016
File chrome/browser/importer/nss_decryptor.h (right):

http://codereview.chromium.org/3855004/diff/41001/42016#newcode17
chrome/browser/importer/nss_decryptor.h:17: // defined. (Unless there is someway
to do nss decrypt with OpenSSL). Ideally
On 2010/10/21 19:29:33, wtc wrote:
> Nit: someway => some way

Done.

http://codereview.chromium.org/3855004/diff/41001/42018
File chrome/browser/sync/util/nigori.cc (right):

http://codereview.chromium.org/3855004/diff/41001/42018#newcode10
chrome/browser/sync/util/nigori.cc:10: #include <arpa/inet.h>
On 2010/10/21 19:29:33, wtc wrote:
> BUG: does this change belong in this CL?
> 

It is needed as symmetric_key.h does not pull in htonl on openssl builds.

> Why do we need to include <arpa/inet.h>?  For htonl?
> http://www.opengroup.org/onlinepubs/9699919799/functions/htonl.html

Exactly. 
I /could/ work around it in symmetric_key.h, but it seems better to make the
dependency explicit in here. This now seems consistent with other files that use
htonl

[wtc, 2010-10-22 22:46:03]

LGTM.

It is a good idea to include <arpa/inet.h> for htonl
explicitly.