Decouples certificates viewers from NSS to prepare support for OpenSSL.

Decouples certificates viewers from NSS to prepare support for OpenSSL.

This change is a pre-requisite for http://codereview.chromium.org/3529008/show
There are no functional changes, it's only refactoring existing code.

BUG=None
TEST=Go to an https:// page and check the certificate info.

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

[bulach, 2010-10-04 19:59:11]

Matt: since I'm removing a few TODOs on your name, I thought you may want to
take a look at this patch.

Ryan / Wan-Teh: I split this from http://codereview.chromium.org/3529008/show,
this one now contains only the UI refactoring / decoupling from NSS. This is
required to allow X509Certificate::OSCertHandle to be defined as an OpenSSL
type. would you please take a look?

Thanks,
Marcus

[mattm, 2010-10-04 21:08:07]

Still looking over this, but one note, you could just delete
chrome/browser/gtk/certificate_manager.cc, it's being replaced by a domui
implementation (chrome/browser/dom_ui/options/certificate_manager_handler.cc)

That should allow you to remove GetCertificatesList and maybe a few other
things, since it uses the certificate_manager_model.

[bulach, 2010-10-05 11:30:04]

thanks for the heads up, Matt!
removed certificate_manager.cc and associated functions in the model.
I also moved x509_certificate_model from "net/base" to
"chrome/common/net", as it depends on
"chrome/third_party/mozilla_security_manager".

new patch uploaded, another look please?

[joth, 2010-10-05 13:06:06]

Thanks for taking this on.

http://codereview.chromium.org/3565006/diff/9001/10007
File chrome/browser/gtk/certificate_viewer.cc (right):

http://codereview.chromium.org/3565006/diff/9001/10007#newcode153
chrome/browser/gtk/certificate_viewer.cc:153: //psm::RegisterDynamicOids();
remove?

http://codereview.chromium.org/3565006/diff/9001/10009
File chrome/browser/gtk/ssl_client_certificate_selector.cc (right):

http://codereview.chromium.org/3565006/diff/9001/10009#newcode58
chrome/browser/gtk/ssl_client_certificate_selector.cc:58:
net::X509Certificate::OSCertHandle cert, const std::string& nickname);
\n between params

http://codereview.chromium.org/3565006/diff/9001/10009#newcode207
chrome/browser/gtk/ssl_client_certificate_selector.cc:207:
FormatComboBoxText(cert, nicknames[i]).c_str());
remove .c_str()

(implicit c'tors are evil!)

http://codereview.chromium.org/3565006/diff/9001/10013
File chrome/common/net/x509_certificate_model.h (right):

http://codereview.chromium.org/3565006/diff/9001/10013#newcode14
chrome/common/net/x509_certificate_model.h:14: // (currently NSS or OpenSSL - in
development).
wouldn't mind expanding this a tad to mention how mac & windows fit in, if at
all.

http://codereview.chromium.org/3565006/diff/9001/10013#newcode17
chrome/common/net/x509_certificate_model.h:17: 
excess \n

http://codereview.chromium.org/3565006/diff/9001/10013#newcode19
chrome/common/net/x509_certificate_model.h:19: // UI elements on both NSS and
OpenSSL. The actual
no need to enumerate the individual libraries again here, the comment up top
covers that.

http://codereview.chromium.org/3565006/diff/9001/10013#newcode26
chrome/common/net/x509_certificate_model.h:26: std::string
GetVersion(net::X509Certificate::OSCertHandle cert_handle);
mention the return value range (1 based?)

http://codereview.chromium.org/3565006/diff/9001/10013#newcode85
chrome/common/net/x509_certificate_model.h:85: } ExtensionData;
struct ExtensionData {
};
?

http://codereview.chromium.org/3565006/diff/9001/10013#newcode87
chrome/common/net/x509_certificate_model.h:87: typedef
std::vector<ExtensionData> ExtensionDataList;
nit: FooList makes me think of std::list
maybe Extension and Extensions
or Extension Element and Extensions.

http://codereview.chromium.org/3565006/diff/9001/10013#newcode123
chrome/common/net/x509_certificate_model.h:123: }  // x509_certificate_model
// namespace foo

http://codereview.chromium.org/3565006/diff/9001/10014
File chrome/common/net/x509_certificate_model_nss.cc (right):

http://codereview.chromium.org/3565006/diff/9001/10014#newcode26
chrome/common/net/x509_certificate_model_nss.cc:26: 
s/\n//

http://codereview.chromium.org/3565006/diff/9001/10014#newcode50
chrome/common/net/x509_certificate_model_nss.cc:50: DCHECK(len <=
HASH_LENGTH_MAX);
_NE, _LE (and _EQ below)

http://codereview.chromium.org/3565006/diff/9001/10014#newcode96
chrome/common/net/x509_certificate_model_nss.cc:96: 
- \n

http://codereview.chromium.org/3565006/diff/9001/10014#newcode102
chrome/common/net/x509_certificate_model_nss.cc:102: using net::X509Certificate;
using std::string maybe worthwhile too.

http://codereview.chromium.org/3565006/diff/9001/10014#newcode157
chrome/common/net/x509_certificate_model_nss.cc:157:
X509Certificate::OSCertHandle cert_handle,
move to previous line

http://codereview.chromium.org/3565006/diff/9001/10014#newcode243
chrome/common/net/x509_certificate_model_nss.cc:243: CERTCertNicknames*
nicknames = CERT_NicknameStringsFromCertList(
nit: 'nicknames' too similar to 'nick_names'

http://codereview.chromium.org/3565006/diff/9001/10014#newcode320
chrome/common/net/x509_certificate_model_nss.cc:320: PR_FALSE));
move to previous line

http://codereview.chromium.org/3565006/diff/9001/10014#newcode387
chrome/common/net/x509_certificate_model_nss.cc:387: 
spare \n

http://codereview.chromium.org/3565006/diff/9001/10015
File chrome/common/net/x509_certificate_model_openssl.cc (right):

http://codereview.chromium.org/3565006/diff/9001/10015#newcode13
chrome/common/net/x509_certificate_model_openssl.cc:13: 
redundant \n

http://codereview.chromium.org/3565006/diff/9001/10015#newcode203
chrome/common/net/x509_certificate_model_openssl.cc:203: 
\n too many

http://codereview.chromium.org/3565006/diff/9001/10017
File net/base/cert_type.h (right):

http://codereview.chromium.org/3565006/diff/9001/10017#newcode10
net/base/cert_type.h:10: 
?

http://codereview.chromium.org/3565006/diff/9001/10018
File net/net.gyp (right):

http://codereview.chromium.org/3565006/diff/9001/10018#newcode8
net/net.gyp:8: 'use_openssl%': 0,
afaict shouldn't need this here (just like javascript_engine and chromeos are
not declared here)

[bulach, 2010-10-05 15:20:24]

thanks joth! all comments addressed / replied, another look please?

http://codereview.chromium.org/3565006/diff/9001/10007
File chrome/browser/gtk/certificate_viewer.cc (right):

http://codereview.chromium.org/3565006/diff/9001/10007#newcode153
chrome/browser/gtk/certificate_viewer.cc:153: //psm::RegisterDynamicOids();
On 2010/10/05 13:06:06, joth wrote:
> remove?

oh, forgot to add this to the model. done.

http://codereview.chromium.org/3565006/diff/9001/10009
File chrome/browser/gtk/ssl_client_certificate_selector.cc (right):

http://codereview.chromium.org/3565006/diff/9001/10009#newcode58
chrome/browser/gtk/ssl_client_certificate_selector.cc:58:
net::X509Certificate::OSCertHandle cert, const std::string& nickname);
On 2010/10/05 13:06:06, joth wrote:
> \n between params

Done.

http://codereview.chromium.org/3565006/diff/9001/10009#newcode207
chrome/browser/gtk/ssl_client_certificate_selector.cc:207:
FormatComboBoxText(cert, nicknames[i]).c_str());
On 2010/10/05 13:06:06, joth wrote:
> remove .c_str()
> 
> (implicit c'tors are evil!)

not really... gtk_combo_box_append_text needs a c_str()..

http://codereview.chromium.org/3565006/diff/9001/10013
File chrome/common/net/x509_certificate_model.h (right):

http://codereview.chromium.org/3565006/diff/9001/10013#newcode14
chrome/common/net/x509_certificate_model.h:14: // (currently NSS or OpenSSL - in
development).
On 2010/10/05 13:06:06, joth wrote:
> wouldn't mind expanding this a tad to mention how mac & windows fit in, if at
> all.

Done, let me know if it's any clearer.

http://codereview.chromium.org/3565006/diff/9001/10013#newcode17
chrome/common/net/x509_certificate_model.h:17: 
On 2010/10/05 13:06:06, joth wrote:
> excess \n

Done.

http://codereview.chromium.org/3565006/diff/9001/10013#newcode19
chrome/common/net/x509_certificate_model.h:19: // UI elements on both NSS and
OpenSSL. The actual
On 2010/10/05 13:06:06, joth wrote:
> no need to enumerate the individual libraries again here, the comment up top
> covers that.

Done.

http://codereview.chromium.org/3565006/diff/9001/10013#newcode26
chrome/common/net/x509_certificate_model.h:26: std::string
GetVersion(net::X509Certificate::OSCertHandle cert_handle);
On 2010/10/05 13:06:06, joth wrote:
> mention the return value range (1 based?)

hmm, the fact that the NSS implementation is "+1" is a detail, I think the
comment saying that this is all UI-related should be enough? as in, there should
be no processing done in the string..

http://codereview.chromium.org/3565006/diff/9001/10013#newcode85
chrome/common/net/x509_certificate_model.h:85: } ExtensionData;
On 2010/10/05 13:06:06, joth wrote:
> struct ExtensionData {
> };
> ?

Done.

http://codereview.chromium.org/3565006/diff/9001/10013#newcode87
chrome/common/net/x509_certificate_model.h:87: typedef
std::vector<ExtensionData> ExtensionDataList;
On 2010/10/05 13:06:06, joth wrote:
> nit: FooList makes me think of std::list
> maybe Extension and Extensions
> or Extension Element and Extensions.

we have tons of vector.*list :) but anyway, Extension / Extensions seems
simpler. done.

http://codereview.chromium.org/3565006/diff/9001/10013#newcode123
chrome/common/net/x509_certificate_model.h:123: }  // x509_certificate_model
On 2010/10/05 13:06:06, joth wrote:
> // namespace foo

Done.

http://codereview.chromium.org/3565006/diff/9001/10014
File chrome/common/net/x509_certificate_model_nss.cc (right):

http://codereview.chromium.org/3565006/diff/9001/10014#newcode26
chrome/common/net/x509_certificate_model_nss.cc:26: 
On 2010/10/05 13:06:06, joth wrote:
> s/\n//

Done.

http://codereview.chromium.org/3565006/diff/9001/10014#newcode50
chrome/common/net/x509_certificate_model_nss.cc:50: DCHECK(len <=
HASH_LENGTH_MAX);
On 2010/10/05 13:06:06, joth wrote:
> _NE, _LE (and _EQ below)

done, but the template/macro/signed magic didn't allow the two above.

http://codereview.chromium.org/3565006/diff/9001/10014#newcode96
chrome/common/net/x509_certificate_model_nss.cc:96: 
On 2010/10/05 13:06:06, joth wrote:
> - \n 

Done.

http://codereview.chromium.org/3565006/diff/9001/10014#newcode102
chrome/common/net/x509_certificate_model_nss.cc:102: using net::X509Certificate;
On 2010/10/05 13:06:06, joth wrote:
> using std::string maybe worthwhile too.

Done.

http://codereview.chromium.org/3565006/diff/9001/10014#newcode157
chrome/common/net/x509_certificate_model_nss.cc:157:
X509Certificate::OSCertHandle cert_handle,
On 2010/10/05 13:06:06, joth wrote:
> move to previous line

Done.

http://codereview.chromium.org/3565006/diff/9001/10014#newcode243
chrome/common/net/x509_certificate_model_nss.cc:243: CERTCertNicknames*
nicknames = CERT_NicknameStringsFromCertList(
On 2010/10/05 13:06:06, joth wrote:
> nit: 'nicknames' too similar to 'nick_names'

CERTCertNicknames cert_nicknames = ...

http://codereview.chromium.org/3565006/diff/9001/10014#newcode320
chrome/common/net/x509_certificate_model_nss.cc:320: PR_FALSE));
On 2010/10/05 13:06:06, joth wrote:
> move to previous line

Done.

http://codereview.chromium.org/3565006/diff/9001/10014#newcode387
chrome/common/net/x509_certificate_model_nss.cc:387: 
On 2010/10/05 13:06:06, joth wrote:
> spare \n

Done.

http://codereview.chromium.org/3565006/diff/9001/10015
File chrome/common/net/x509_certificate_model_openssl.cc (right):

http://codereview.chromium.org/3565006/diff/9001/10015#newcode13
chrome/common/net/x509_certificate_model_openssl.cc:13: 
On 2010/10/05 13:06:06, joth wrote:
> redundant \n

Done.

http://codereview.chromium.org/3565006/diff/9001/10015#newcode203
chrome/common/net/x509_certificate_model_openssl.cc:203: 
On 2010/10/05 13:06:06, joth wrote:
> \n too many

Done.

http://codereview.chromium.org/3565006/diff/9001/10017
File net/base/cert_type.h (right):

http://codereview.chromium.org/3565006/diff/9001/10017#newcode10
net/base/cert_type.h:10: 
On 2010/10/05 13:06:06, joth wrote:
> ?

not sure I understood, but removed the \n anyways.

http://codereview.chromium.org/3565006/diff/9001/10018
File net/net.gyp (right):

http://codereview.chromium.org/3565006/diff/9001/10018#newcode8
net/net.gyp:8: 'use_openssl%': 0,
On 2010/10/05 13:06:06, joth wrote:
> afaict shouldn't need this here (just like javascript_engine and chromeos are
> not declared here)

Done.

[joth, 2010-10-05 16:17:34]

On 5 October 2010 16:20, <bulach@chromium.org> wrote:

> thanks joth! all comments addressed / replied, another look please?
>
>
>
> http://codereview.chromium.org/3565006/diff/9001/10007
> File chrome/browser/gtk/certificate_viewer.cc (right):
>
> http://codereview.chromium.org/3565006/diff/9001/10007#newcode153
> chrome/browser/gtk/certificate_viewer.cc:153:
> //psm::RegisterDynamicOids();
> On 2010/10/05 13:06:06, joth wrote:
>
>> remove?
>>
>
> oh, forgot to add this to the model. done.
>
>
> http://codereview.chromium.org/3565006/diff/9001/10009
> File chrome/browser/gtk/ssl_client_certificate_selector.cc (right):
>
> http://codereview.chromium.org/3565006/diff/9001/10009#newcode58
> chrome/browser/gtk/ssl_client_certificate_selector.cc:58:
> net::X509Certificate::OSCertHandle cert, const std::string& nickname);
> On 2010/10/05 13:06:06, joth wrote:
>
>> \n between params
>>
>
> Done.
>
>
> http://codereview.chromium.org/3565006/diff/9001/10009#newcode207
> chrome/browser/gtk/ssl_client_certificate_selector.cc:207:
> FormatComboBoxText(cert, nicknames[i]).c_str());
> On 2010/10/05 13:06:06, joth wrote:
>
>> remove .c_str()
>>
>
>  (implicit c'tors are evil!)
>>
>
> not really... gtk_combo_box_append_text needs a c_str()..
>
>
Ugh, my mistake, I didn't see the close paren before the dot.


>
> http://codereview.chromium.org/3565006/diff/9001/10013
> File chrome/common/net/x509_certificate_model.h (right):
>
> http://codereview.chromium.org/3565006/diff/9001/10013#newcode14
> chrome/common/net/x509_certificate_model.h:14: // (currently NSS or
> OpenSSL - in development).
> On 2010/10/05 13:06:06, joth wrote:
>
>> wouldn't mind expanding this a tad to mention how mac & windows fit
>>
> in, if at
>
>> all.
>>
>
> Done, let me know if it's any clearer.
>
>
> http://codereview.chromium.org/3565006/diff/9001/10013#newcode17
> chrome/common/net/x509_certificate_model.h:17:
> On 2010/10/05 13:06:06, joth wrote:
>
>> excess \n
>>
>
> Done.
>
>
> http://codereview.chromium.org/3565006/diff/9001/10013#newcode19
> chrome/common/net/x509_certificate_model.h:19: // UI elements on both
> NSS and OpenSSL. The actual
> On 2010/10/05 13:06:06, joth wrote:
>
>> no need to enumerate the individual libraries again here, the comment
>>
> up top
>
>> covers that.
>>
>
> Done.
>
>
> http://codereview.chromium.org/3565006/diff/9001/10013#newcode26
> chrome/common/net/x509_certificate_model.h:26: std::string
> GetVersion(net::X509Certificate::OSCertHandle cert_handle);
> On 2010/10/05 13:06:06, joth wrote:
>
>> mention the return value range (1 based?)
>>
>
> hmm, the fact that the NSS implementation is "+1" is a detail, I think
> the comment saying that this is all UI-related should be enough? as in,
> there should be no processing done in the string..
>
>
> http://codereview.chromium.org/3565006/diff/9001/10013#newcode85
> chrome/common/net/x509_certificate_model.h:85: } ExtensionData;
> On 2010/10/05 13:06:06, joth wrote:
>
>> struct ExtensionData {
>> };
>> ?
>>
>
> Done.
>
>
> http://codereview.chromium.org/3565006/diff/9001/10013#newcode87
> chrome/common/net/x509_certificate_model.h:87: typedef
> std::vector<ExtensionData> ExtensionDataList;
> On 2010/10/05 13:06:06, joth wrote:
>
>> nit: FooList makes me think of std::list
>> maybe Extension and Extensions
>> or Extension Element and Extensions.
>>
>
> we have tons of vector.*list :) but anyway, Extension / Extensions seems
> simpler. done.
>
> Sorry I forgot to say, it was foo_list_[i] that really caught my eye more
than the name of the typedef.


> http://codereview.chromium.org/3565006/diff/9001/10013#newcode123
> chrome/common/net/x509_certificate_model.h:123: }  //
> x509_certificate_model
> On 2010/10/05 13:06:06, joth wrote:
>
>> // namespace foo
>>
>
> Done.
>
>
> http://codereview.chromium.org/3565006/diff/9001/10014
> File chrome/common/net/x509_certificate_model_nss.cc (right):
>
> http://codereview.chromium.org/3565006/diff/9001/10014#newcode26
> chrome/common/net/x509_certificate_model_nss.cc:26:
> On 2010/10/05 13:06:06, joth wrote:
>
>> s/\n//
>>
>
> Done.
>
>
> http://codereview.chromium.org/3565006/diff/9001/10014#newcode50
> chrome/common/net/x509_certificate_model_nss.cc:50: DCHECK(len <=
> HASH_LENGTH_MAX);
> On 2010/10/05 13:06:06, joth wrote:
>
>> _NE, _LE (and _EQ below)
>>
>
> done, but the template/macro/signed magic didn't allow the two above.

the pointer is not interesting anyway. for the length, how about:
DCHECK_EQ(0u, cert->derCert.len);


>
> http://codereview.chromium.org/3565006/diff/9001/10014#newcode96
> chrome/common/net/x509_certificate_model_nss.cc:96:
> On 2010/10/05 13:06:06, joth wrote:
>
>> - \n
>>
>
> Done.
>
>
> http://codereview.chromium.org/3565006/diff/9001/10014#newcode102
> chrome/common/net/x509_certificate_model_nss.cc:102: using
> net::X509Certificate;
> On 2010/10/05 13:06:06, joth wrote:
>
>> using std::string maybe worthwhile too.
>>
>
> Done.
>
>
> http://codereview.chromium.org/3565006/diff/9001/10014#newcode157
> chrome/common/net/x509_certificate_model_nss.cc:157:
> X509Certificate::OSCertHandle cert_handle,
> On 2010/10/05 13:06:06, joth wrote:
>
>> move to previous line
>>
>
> Done.
>
>
> http://codereview.chromium.org/3565006/diff/9001/10014#newcode243
> chrome/common/net/x509_certificate_model_nss.cc:243: CERTCertNicknames*
> nicknames = CERT_NicknameStringsFromCertList(
> On 2010/10/05 13:06:06, joth wrote:
>
>> nit: 'nicknames' too similar to 'nick_names'
>>
>
> CERTCertNicknames cert_nicknames = ...
>
>
> http://codereview.chromium.org/3565006/diff/9001/10014#newcode320
> chrome/common/net/x509_certificate_model_nss.cc:320: PR_FALSE));
> On 2010/10/05 13:06:06, joth wrote:
>
>> move to previous line
>>
>
> Done.
>
>
> http://codereview.chromium.org/3565006/diff/9001/10014#newcode387
> chrome/common/net/x509_certificate_model_nss.cc:387:
> On 2010/10/05 13:06:06, joth wrote:
>
>> spare \n
>>
>
> Done.
>
>
> http://codereview.chromium.org/3565006/diff/9001/10015
> File chrome/common/net/x509_certificate_model_openssl.cc (right):
>
> http://codereview.chromium.org/3565006/diff/9001/10015#newcode13
> chrome/common/net/x509_certificate_model_openssl.cc:13:
> On 2010/10/05 13:06:06, joth wrote:
>
>> redundant \n
>>
>
> Done.
>
>
> http://codereview.chromium.org/3565006/diff/9001/10015#newcode203
> chrome/common/net/x509_certificate_model_openssl.cc:203:
> On 2010/10/05 13:06:06, joth wrote:
>
>> \n too many
>>
>
> Done.
>
>
> http://codereview.chromium.org/3565006/diff/9001/10017
> File net/base/cert_type.h (right):
>
> http://codereview.chromium.org/3565006/diff/9001/10017#newcode10
> net/base/cert_type.h:10:
> On 2010/10/05 13:06:06, joth wrote:
>
>> ?
>>
>
> not sure I understood, but removed the \n anyways.

yep. I was loosing confidence I was saying anything useful....


>
>
> http://codereview.chromium.org/3565006/diff/9001/10018
> File net/net.gyp (right):
>
> http://codereview.chromium.org/3565006/diff/9001/10018#newcode8
> net/net.gyp:8: 'use_openssl%': 0,
> On 2010/10/05 13:06:06, joth wrote:
>
>> afaict shouldn't need this here (just like javascript_engine and
>>
> chromeos are
>
>> not declared here)
>>
>
> Done.
>
>
> http://codereview.chromium.org/3565006/show
>

[joth, 2010-10-05 20:55:52]

http://codereview.chromium.org/3565006/diff/22001/23014
File chrome/common/net/x509_certificate_model.h (right):

http://codereview.chromium.org/3565006/diff/22001/23014#newcode75
chrome/common/net/x509_certificate_model.h:75: const
std::vector<scoped_refptr<net::X509Certificate> >& certs,
nit: const CertificateList& certs

[joth, 2010-10-07 20:01:42]

[wtc, rsleevi: moved to cc for clarity, feel free to add yourself back to
reviewer if you have comments]

Matt - did you have any more comments on this, or are you happy for me to a last
pass and give the final LG?

Sorry to bug you, just we now have several patches backed up behind this one and
it's starting to "get complicated" !

Cheers,
Joth

[joth, 2010-10-07 20:04:17]

http://codereview.chromium.org/3565006/diff/10020/29
File chrome/common/net/x509_certificate_model.h (right):

http://codereview.chromium.org/3565006/diff/10020/29#newcode76
chrome/common/net/x509_certificate_model.h:76: const net::CertificateList&
certs,
nit: now it will fit on the previous line

[mattm, 2010-10-07 20:30:08]

lgtm with a few comments

http://codereview.chromium.org/3565006/diff/10020/18
File chrome/browser/gtk/certificate_dialogs.cc (right):

http://codereview.chromium.org/3565006/diff/10020/18#newcode8
chrome/browser/gtk/certificate_dialogs.cc:8: // x509_certificate_nss.
Could remove this TODO now.

http://codereview.chromium.org/3565006/diff/10020/30
File chrome/common/net/x509_certificate_model_nss.cc (right):

http://codereview.chromium.org/3565006/diff/10020/30#newcode280
chrome/common/net/x509_certificate_model_nss.cc:280: return
psm::ProcessIDN(input);
Actually ProcessIDN could be moved out of
chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp to some shared
place.  It doesn't depend on any NSS stuff, and isn't based on mozilla code.  I
don't know why I put it there :/

Could just put a TODO if you don't want to do it in this CL.

http://codereview.chromium.org/3565006/diff/10020/30#newcode304
chrome/common/net/x509_certificate_model_nss.cc:304: cert_handle->derCert.len);
indentation

[bulach, 2010-10-08 09:25:15]

thanks all!

comments addressed, will land as soon as the try-servers are green.

http://codereview.chromium.org/3565006/diff/22001/23014
File chrome/common/net/x509_certificate_model.h (right):

http://codereview.chromium.org/3565006/diff/22001/23014#newcode75
chrome/common/net/x509_certificate_model.h:75: const
std::vector<scoped_refptr<net::X509Certificate> >& certs,
On 2010/10/05 20:55:52, joth wrote:
> nit: const CertificateList& certs
> 
> 

Done.

http://codereview.chromium.org/3565006/diff/10020/18
File chrome/browser/gtk/certificate_dialogs.cc (right):

http://codereview.chromium.org/3565006/diff/10020/18#newcode8
chrome/browser/gtk/certificate_dialogs.cc:8: // x509_certificate_nss.
On 2010/10/07 20:30:08, mattm wrote:
> Could remove this TODO now.

:) done

http://codereview.chromium.org/3565006/diff/10020/29
File chrome/common/net/x509_certificate_model.h (right):

http://codereview.chromium.org/3565006/diff/10020/29#newcode76
chrome/common/net/x509_certificate_model.h:76: const net::CertificateList&
certs,
On 2010/10/07 20:04:17, joth wrote:
> nit: now it will fit on the previous line

Done.

http://codereview.chromium.org/3565006/diff/10020/30
File chrome/common/net/x509_certificate_model_nss.cc (right):

http://codereview.chromium.org/3565006/diff/10020/30#newcode280
chrome/common/net/x509_certificate_model_nss.cc:280: return
psm::ProcessIDN(input);
On 2010/10/07 20:30:08, mattm wrote:
> Actually ProcessIDN could be moved out of
> chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp to some shared
> place.  It doesn't depend on any NSS stuff, and isn't based on mozilla code. 
I
> don't know why I put it there :/
> 
> Could just put a TODO if you don't want to do it in this CL.

good point!  I added a new file, x509_certificate_model.cc, and moved ProcessIDN
in there. it's in chrome/common/net, but nsNSSCertCertHelper already depends on
strings, so hopefully DEPS won't complain.

http://codereview.chromium.org/3565006/diff/10020/30#newcode304
chrome/common/net/x509_certificate_model_nss.cc:304: cert_handle->derCert.len);
On 2010/10/07 20:30:08, mattm wrote:
> indentation

Done.