Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
303133006:
Added net_log logging statments for CertVerifyResult (Closed)
|
Created:
6 years, 6 months ago by mshelley Modified:
6 years, 6 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, erikwright+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdded net_log logging statments for CertVerifyResult
R=rsleevi@chromium.org,wtc@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274456
Patch Set 1 #Patch Set 2 : removed changes to base/base.gyp #
Total comments: 21
Patch Set 3 : Fixed various comments on last CL #
Total comments: 14
Patch Set 4 : Fixed formatting errors, removed unnecessary declaration from header file, and deleted unneeded com… #
Total comments: 36
Patch Set 5 : git diff #Patch Set 6 : Fixed various formatting and style issues. #Patch Set 7 : Removed extraneous character. #
Total comments: 2
Patch Set 8 : Moved 'certificates' explanation to BEGIN phase comment" #
Total comments: 5
Patch Set 9 : Fixed minor formatting issue #
Total comments: 2
Messages
Total messages: 21 (0 generated)
Please ignore the base.gyp file -- it shouldn't be on the cl.
This is what the current output looks like:
t=22 [st=13] CERT_VERIFIER_JOB
--> cert_status = {"flags_set":[],"value":0}
--> common_name_fallback_used = false
--> has_md2 = false
--> has_md4 = false
--> has_md5 = false
--> is_issued_by_additional_trust_anchor = false
--> is_issued_by_known_root = true
--> public_key_hashes =
["sha1/7FbJsn55NGoits1TFDAELZX24jY=","sha256/DDDkHNOJuhbnI4jB1hEhxjVbSSwkAoplL5KvWirvn+8=","sha1/Q9rWMO5T+KmAym79hfRqo3mQ4Oo=","sha256/7HIpactkIAq2Y49orFOOQKurWxmmSFZhBCoQYcRhJ3Y=","sha1/wHqYaI2J+6sFZAwRfap9ZbjKzE4=","sha256/h6801m+z8v3zbgkRHpq6L29Esgfzhj89C1SyUCOQmqU=","sha1/SOZo+SvSspXXR9gjIBBPM5iQn9Q=","sha256//1aAzXOlcD2gSBegdf1GJQanNQbEuBoVg+9UlHjSZHY="]
t=23 [st=14] -CERT_VERIFIER_JOB
I haven't been able to figure out how to get rid of the quotation marks inside
of the lists.
A great start! Please don't be daunted by the length of the comments - I tried to provide short answer / long answer for why things are. Normally it'd just be "short answer", but I think it's helpful to understand "why" and not just "what". https://codereview.chromium.org/303133006/diff/20001/net/cert/multi_threaded_... File net/cert/multi_threaded_cert_verifier.cc (right): https://codereview.chromium.org/303133006/diff/20001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:9: #include <iostream> Unnecessary includes, I suspect? https://codereview.chromium.org/303133006/diff/20001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:33: using namespace std; STYLE: This is forbidden by the style guide. See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp... and search for "Using-directive" within the Decision section of Named Namespaces. https://codereview.chromium.org/303133006/diff/20001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:85: STYLE: You would move lines 590-660 here Longer explanation: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar... documents the recommended order for declarations. While this section is specific to classes, it also generally applies to unnamed namespaces. The only exception is with respect to the "inline in the class definition" - we generally inline functions within an unnamed namespace if they're free functions, but *recommend* (but not require) that functions be out of line when within classes within unnamed namespaces. https://codereview.chromium.org/303133006/diff/20001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:360: verify_result.result.verified_cert)); Instead of logging this as a distinct event, use base::DictionaryValue::Set to store the result of NetLogX509CertificateCallback Say, on line 612 results->Set("verified_cert", NetLogX509CertificateCallback(verify_result.result.verified_cert)); https://codereview.chromium.org/303133006/diff/20001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:363: base::Bind(&CertVerifyResultCallback, verify_result.result)); Instead of using AddEvent to do this, update the EndEvent (line 365) to use the CertVerifyResultCallback. Note: You will also need to update the documentation in net_log_event_type_list.h, at https://code.google.com/p/chromium/codesearch#chromium/src/net/base/net_log_e... https://codereview.chromium.org/303133006/diff/20001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:598: namespace { STYLE: Add a newline between 598 and 599. Long Answer: The general namespace formatting, which is not *required* so much as *suggested*, is at http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp... Within this, we usually place a blank line between the "namespace {" and the first symbol within that namespace. https://codereview.chromium.org/303133006/diff/20001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:611: results->SetInteger("cert_status.value", verify_result.cert_status); Just call this results->SetInteger("cert_status", verify_result.cert_status) I explain why below. https://codereview.chromium.org/303133006/diff/20001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:616: flags->AppendString("CERT_STATUS_ALL_ERRORS"); This is a mask value for processing in conditionals. You don't need to log this. https://codereview.chromium.org/303133006/diff/20001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:619: flags->AppendString("CERT_STATUS_COMMON_NAME_INVALID"); So, turns out I steered you wrong, and this particular logging is actually going to be a bit more complex, but we can defer that to a follow-up commit. For this commit, just log the numeric value of the cert_status You'd need to match how LoadFlags are handled. You can see how this works in https://code.google.com/p/chromium/codesearch#chromium/src/net/base/net_log_l... This would involve changing how CertStatus is declared (in net/cert/cert_status_flags.h) to behave like load_flags.h / load_flags_list.h ( see https://code.google.com/p/chromium/codesearch#chromium/src/net/base/load_flag... and https://code.google.com/p/chromium/codesearch#chromium/src/net/base/net_log_l... ) Once this is in place (and it's creating symbolic names for CERT_STATUS), you'd then need to update the .js responsible for chrome://net-internals (see https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... for the starting point) Once updated, you'd then also update https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... https://codereview.chromium.org/303133006/diff/20001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:650: for (vector<net::HashValue>::iterator it = When iterating in loops, prefer a const_iterator to an iterator. Not only does it allow the compiler to generate more efficient code (or at least spend less time doing the same code), it also helps clarify that you're not modifying anything. https://codereview.chromium.org/303133006/diff/20001/net/cert/multi_threaded_... File net/cert/multi_threaded_cert_verifier.h (right): https://codereview.chromium.org/303133006/diff/20001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.h:174: namespace { Short answer: Don't use unnamed namespaces in .h files (see "Decision" in http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp... ) Long answer (feel free to skip): When the C++ compiler encounters an unnamed namespace, it generates a new unique namespace within the translation unit (logical file being compiled). What this means is that when two files - a.cc and b.cc - include b.h (which has an unnamed namespace), they will create new names for this namespace. Assuming the definition is within b.cc, this creates an issue. All the functions within b.cc can see the 'compiler-generated' namespace (let's call it __b__) from including b.h, and it knows to place CertVerifyResultCallback within that namespace when it encounters the definition in b.cc. However, when a.cc is compiled, it generates a different namespace (let's call it __a__) When the linker tries to link a.cc and b.cc to produce a.exe, it attempts to find __a__::CertVerifyResultCallback, which a.cc depends on, but it can't. This is because it was actually instantiated in b.cc, which placed it in __b__::CertVerifyResultCallback. This is the much longer-winded way of explaining why it doesn't work :) The solution is to move this into the .cc file, which I'll annotate.
Hi Ryan, I fixed all of the things you mentioned in your comments. One thing I wasn't sure about though was what LogLevel to use when I call the X509Certificate call back function? Thanks, Mackenzie https://codereview.chromium.org/303133006/diff/20001/net/cert/multi_threaded_... File net/cert/multi_threaded_cert_verifier.cc (right): https://codereview.chromium.org/303133006/diff/20001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:9: #include <iostream> On 2014/05/30 22:46:24, Ryan Sleevi wrote: > Unnecessary includes, I suspect? Done. https://codereview.chromium.org/303133006/diff/20001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:33: using namespace std; On 2014/05/30 22:46:24, Ryan Sleevi wrote: > STYLE: This is forbidden by the style guide. > > See > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp... > and search for "Using-directive" within the Decision section of Named > Namespaces. Done. https://codereview.chromium.org/303133006/diff/20001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:360: verify_result.result.verified_cert)); On 2014/05/30 22:46:24, Ryan Sleevi wrote: > Instead of logging this as a distinct event, use base::DictionaryValue::Set to > store the result of NetLogX509CertificateCallback > > Say, on line 612 > > results->Set("verified_cert", > NetLogX509CertificateCallback(verify_result.result.verified_cert)); When I explicitly call NetLogX509CC, I have to pass it a LogLevel -- I know the options for this are: LOG_ALL, LOG_ALL_BUT_BYTES, LOG_STRIP_PRIVATE_DATA, and LOG_NONE, but I'm not sure which I'm supposed to use? https://codereview.chromium.org/303133006/diff/20001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:363: base::Bind(&CertVerifyResultCallback, verify_result.result)); On 2014/05/30 22:46:24, Ryan Sleevi wrote: > Instead of using AddEvent to do this, update the EndEvent (line 365) to use the > CertVerifyResultCallback. > > Note: You will also need to update the documentation in > net_log_event_type_list.h, at > https://code.google.com/p/chromium/codesearch#chromium/src/net/base/net_log_e... Done. https://codereview.chromium.org/303133006/diff/20001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:598: namespace { On 2014/05/30 22:46:24, Ryan Sleevi wrote: > STYLE: Add a newline between 598 and 599. > > Long Answer: The general namespace formatting, which is not *required* so much > as *suggested*, is at > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp... > > Within this, we usually place a blank line between the "namespace {" and the > first symbol within that namespace. Done. https://codereview.chromium.org/303133006/diff/20001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:611: results->SetInteger("cert_status.value", verify_result.cert_status); On 2014/05/30 22:46:24, Ryan Sleevi wrote: > Just call this > > results->SetInteger("cert_status", verify_result.cert_status) > > I explain why below. Done. https://codereview.chromium.org/303133006/diff/20001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:616: flags->AppendString("CERT_STATUS_ALL_ERRORS"); On 2014/05/30 22:46:24, Ryan Sleevi wrote: > This is a mask value for processing in conditionals. You don't need to log this. Done. https://codereview.chromium.org/303133006/diff/20001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:650: for (vector<net::HashValue>::iterator it = On 2014/05/30 22:46:24, Ryan Sleevi wrote: > When iterating in loops, prefer a const_iterator to an iterator. Not only does > it allow the compiler to generate more efficient code (or at least spend less > time doing the same code), it also helps clarify that you're not modifying > anything. Done. https://codereview.chromium.org/303133006/diff/20001/net/cert/multi_threaded_... File net/cert/multi_threaded_cert_verifier.h (right): https://codereview.chromium.org/303133006/diff/20001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.h:174: namespace { On 2014/05/30 22:46:24, Ryan Sleevi wrote: > Short answer: Don't use unnamed namespaces in .h files (see "Decision" in > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp... > ) > > Long answer (feel free to skip): When the C++ compiler encounters an unnamed > namespace, it generates a new unique namespace within the translation unit > (logical file being compiled). What this means is that when two files - a.cc and > b.cc - include b.h (which has an unnamed namespace), they will create new names > for this namespace. Assuming the definition is within b.cc, this creates an > issue. > > All the functions within b.cc can see the 'compiler-generated' namespace (let's > call it __b__) from including b.h, and it knows to place > CertVerifyResultCallback within that namespace when it encounters the definition > in b.cc. However, when a.cc is compiled, it generates a different namespace > (let's call it __a__) > > When the linker tries to link a.cc and b.cc to produce a.exe, it attempts to > find __a__::CertVerifyResultCallback, which a.cc depends on, but it can't. This > is because it was actually instantiated in b.cc, which placed it in > __b__::CertVerifyResultCallback. > > This is the much longer-winded way of explaining why it doesn't work :) > > The solution is to move this into the .cc file, which I'll annotate. Done.
https://codereview.chromium.org/303133006/diff/20001/net/cert/multi_threaded_... File net/cert/multi_threaded_cert_verifier.cc (right): https://codereview.chromium.org/303133006/diff/20001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:85: On 2014/05/30 22:46:24, Ryan Sleevi wrote: > STYLE: You would move lines 590-660 here > > Longer explanation: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar... > documents the recommended order for declarations. While this section is specific > to classes, it also generally applies to unnamed namespaces. > > The only exception is with respect to the "inline in the class definition" - we > generally inline functions within an unnamed namespace if they're free > functions, but *recommend* (but not require) that functions be out of line when > within classes within unnamed namespaces. Still need to do this one. https://codereview.chromium.org/303133006/diff/40001/net/base/net_log_event_t... File net/base/net_log_event_type_list.h (right): https://codereview.chromium.org/303133006/diff/40001/net/base/net_log_event_t... net/base/net_log_event_type_list.h:1898: // of additional trust anchors.> indentation issue on 1896 and 1900 https://codereview.chromium.org/303133006/diff/40001/net/base/net_log_event_t... net/base/net_log_event_type_list.h:1906: // was not trusted.> weird wrap. https://codereview.chromium.org/303133006/diff/40001/net/base/net_log_event_t... net/base/net_log_event_type_list.h:1919: // sandbox.> weird wrapping here too. https://codereview.chromium.org/303133006/diff/40001/net/cert/multi_threaded_... File net/cert/multi_threaded_cert_verifier.cc (right): https://codereview.chromium.org/303133006/diff/40001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:356: base::Bind(&CertVerifyResultCallback, verify_result.result));*/ Delete, not comment :) https://codereview.chromium.org/303133006/diff/40001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:610: net::NetLog::LOG_ALL)); Just pass your log level that you receive as the parameter (|log_level|, on line 596) https://codereview.chromium.org/303133006/diff/40001/net/cert/multi_threaded_... File net/cert/multi_threaded_cert_verifier.h (right): https://codereview.chromium.org/303133006/diff/40001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.h:16: #include "base/values.h" So, had we needed 174-175 (which we don't, below), the fun part is, we wouldn't need to include this header. http://www.chromium.org/developers/coding-style/cpp-dos-and-donts lists a variety of cases where forward declaration ( http://en.wikipedia.org/wiki/Forward_declaration#Classes ) would suffice. In this world (read: you do NOT need to do this for this CL, because of comments below), you would do namespace base { class Value; } // namespace base You can see the example of forward declaration in use on lines 29-33 - we can declare these classes, without having to include their respective headers / declarations. https://codereview.chromium.org/303133006/diff/40001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.h:175: net::NetLog::LogLevel log_level); No need to declare this in the .h file - this is a function internal to the .cc, so you can let the definition be your declaration. Sorry I wasn't clearer on that earlier.
https://codereview.chromium.org/303133006/diff/40001/net/base/net_log_event_t... File net/base/net_log_event_type_list.h (right): https://codereview.chromium.org/303133006/diff/40001/net/base/net_log_event_t... net/base/net_log_event_type_list.h:1898: // of additional trust anchors.> On 2014/05/31 00:53:02, Ryan Sleevi wrote: > indentation issue on 1896 and 1900 Done. Sorry about all of that -- I think those lines must have been over 80 chars, so the linter just cut them off and wrapped them. I'll be sure to pay more attention to that next time! https://codereview.chromium.org/303133006/diff/40001/net/base/net_log_event_t... net/base/net_log_event_type_list.h:1906: // was not trusted.> On 2014/05/31 00:53:02, Ryan Sleevi wrote: > weird wrap. Done. https://codereview.chromium.org/303133006/diff/40001/net/base/net_log_event_t... net/base/net_log_event_type_list.h:1919: // sandbox.> On 2014/05/31 00:53:02, Ryan Sleevi wrote: > weird wrapping here too. Done. https://codereview.chromium.org/303133006/diff/40001/net/cert/multi_threaded_... File net/cert/multi_threaded_cert_verifier.cc (right): https://codereview.chromium.org/303133006/diff/40001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:356: base::Bind(&CertVerifyResultCallback, verify_result.result));*/ On 2014/05/31 00:53:02, Ryan Sleevi wrote: > Delete, not comment :) Done. My mistake -- I meant to go back and delete it later and then forgot! https://codereview.chromium.org/303133006/diff/40001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:610: net::NetLog::LOG_ALL)); On 2014/05/31 00:53:02, Ryan Sleevi wrote: > Just pass your log level that you receive as the parameter (|log_level|, on line > 596) Done. https://codereview.chromium.org/303133006/diff/40001/net/cert/multi_threaded_... File net/cert/multi_threaded_cert_verifier.h (right): https://codereview.chromium.org/303133006/diff/40001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.h:16: #include "base/values.h" On 2014/05/31 00:53:02, Ryan Sleevi wrote: > So, had we needed 174-175 (which we don't, below), the fun part is, we wouldn't > need to include this header. > > http://www.chromium.org/developers/coding-style/cpp-dos-and-donts lists a > variety of cases where forward declaration ( > http://en.wikipedia.org/wiki/Forward_declaration#Classes ) would suffice. > > In this world (read: you do NOT need to do this for this CL, because of comments > below), you would do > > namespace base { > class Value; > } // namespace base > > You can see the example of forward declaration in use on lines 29-33 - we can > declare these classes, without having to include their respective headers / > declarations. Done. https://codereview.chromium.org/303133006/diff/40001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.h:175: net::NetLog::LogLevel log_level); On 2014/05/31 00:53:02, Ryan Sleevi wrote: > No need to declare this in the .h file - this is a function internal to the .cc, > so you can let the definition be your declaration. > > Sorry I wasn't clearer on that earlier. Done.
A few more style comments. This is normal for the first few weeks (or, in my case with wtc, months...) https://codereview.chromium.org/303133006/diff/60001/net/base/net_log_event_t... File net/base/net_log_event_type_list.h (right): https://codereview.chromium.org/303133006/diff/60001/net/base/net_log_event_t... net/base/net_log_event_type_list.h:1896: // this Still unaligned (it should match up with "True" on 1895) https://codereview.chromium.org/303133006/diff/60001/net/base/net_log_event_t... net/base/net_log_event_type_list.h:1900: // standard Ditto here. https://codereview.chromium.org/303133006/diff/60001/net/base/net_log_event_t... net/base/net_log_event_type_list.h:1906: // was not trusted.> You should be able to fold this onto 1905 https://codereview.chromium.org/303133006/diff/60001/net/base/net_log_event_t... net/base/net_log_event_type_list.h:1910: // of the SubjectPublicKeyInfos of the chain.> You should be able to fold this with 1909 https://codereview.chromium.org/303133006/diff/60001/net/base/net_log_event_t... net/base/net_log_event_type_list.h:1915: // the intermediate certificates stored within may be You should be able to fold this onto 1914 (and adjust the lines below) https://codereview.chromium.org/303133006/diff/60001/net/cert/multi_threaded_... File net/cert/multi_threaded_cert_verifier.cc (right): https://codereview.chromium.org/303133006/diff/60001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:22: #include "net/cert/cert_status_flags.h" I don't believe you need this header anymore? We're just using the numeric value from verify_result.cert_status for now https://codereview.chromium.org/303133006/diff/60001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:35: base::Value* CertVerifyResultCallback(net::CertVerifyResult verify_result, STYLE: So, in the previous comment, I mentioned you could move this entire function to line 115 Reasoning: 1) We generally prefer one unnamed namespace in a file, that contains all of the 'helper' functions 2) The choice of 115 (versus, say, 109 or 112) is to match the recommended order of declarations 3) It allows you to delete the "net::" on lines 35, 36, and 49
Review comments on patch set 4: All of my comments are coding style issues. https://codereview.chromium.org/303133006/diff/60001/net/base/net_log_event_t... File net/base/net_log_event_type_list.h (right): https://codereview.chromium.org/303133006/diff/60001/net/base/net_log_event_t... net/base/net_log_event_type_list.h:1894: // "has_md5": <Property of the certificate chain.> The description of these three booleans should be more informative. For example: "has_md2": <True if a certificate in the certificate chain is signed with a MD2 signature.> https://codereview.chromium.org/303133006/diff/60001/net/base/net_log_event_t... net/base/net_log_event_type_list.h:1911: // "verified_cert": <The certificate and chain that was constructed Remove "and" from "certificate and chain". https://codereview.chromium.org/303133006/diff/60001/net/base/net_log_event_t... net/base/net_log_event_type_list.h:1912: // during verification. Note that the though the verified In "the though the", I think the first "the" should be deleted. https://codereview.chromium.org/303133006/diff/60001/net/cert/multi_threaded_... File net/cert/multi_threaded_cert_verifier.cc (right): https://codereview.chromium.org/303133006/diff/60001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:33: namespace { I suggest that you nest this unnamed namespace inside the net namespace, so that the CertVerifyResultCallback function can omit the net:: prefix. namespace net { namespace { base::Value* CertVerifyResultCallback(const CertVerifyResult& verify_result, ... } } // namespace ... } // namespace net https://codereview.chromium.org/303133006/diff/60001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:35: base::Value* CertVerifyResultCallback(net::CertVerifyResult verify_result, Since CertVerifyResult is a struct with several members, I think this parameter should be declared as a const reference: const net::CertVerifyResult& verify_result See the Style Guide: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Reference_Argu... https://codereview.chromium.org/303133006/diff/60001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:55: it++) { Use pre-increment to increment an iterator. See the Style Guide: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Preincrement_a... https://codereview.chromium.org/303133006/diff/60001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:63: } 1. Add a blank line before this line. 2. This line is the closing '}' of a namespace. Our Style Guide recommends that we add a comment: } // namespace See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespace_Form... https://codereview.chromium.org/303133006/diff/60001/net/cert/multi_threaded_... File net/cert/multi_threaded_cert_verifier.h (right): https://codereview.chromium.org/303133006/diff/60001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.h:173: Did cpplint.py or clang-format suggest adding this blank line at the end of the file?
https://codereview.chromium.org/303133006/diff/60001/net/base/net_log_event_t... File net/base/net_log_event_type_list.h (right): https://codereview.chromium.org/303133006/diff/60001/net/base/net_log_event_t... net/base/net_log_event_type_list.h:1894: // "has_md5": <Property of the certificate chain.> On 2014/05/31 02:48:03, wtc wrote: > > The description of these three booleans should be more informative. > For example: > > "has_md2": <True if a certificate in the certificate chain is signed with a > MD2 signature.> Done. https://codereview.chromium.org/303133006/diff/60001/net/base/net_log_event_t... net/base/net_log_event_type_list.h:1896: // this On 2014/05/31 02:41:11, Ryan Sleevi wrote: > Still unaligned (it should match up with "True" on 1895) Done. https://codereview.chromium.org/303133006/diff/60001/net/base/net_log_event_t... net/base/net_log_event_type_list.h:1900: // standard On 2014/05/31 02:41:11, Ryan Sleevi wrote: > Ditto here. Done. https://codereview.chromium.org/303133006/diff/60001/net/base/net_log_event_t... net/base/net_log_event_type_list.h:1906: // was not trusted.> On 2014/05/31 02:41:11, Ryan Sleevi wrote: > You should be able to fold this onto 1905 Done. https://codereview.chromium.org/303133006/diff/60001/net/base/net_log_event_t... net/base/net_log_event_type_list.h:1910: // of the SubjectPublicKeyInfos of the chain.> On 2014/05/31 02:41:11, Ryan Sleevi wrote: > You should be able to fold this with 1909 Done. https://codereview.chromium.org/303133006/diff/60001/net/base/net_log_event_t... net/base/net_log_event_type_list.h:1911: // "verified_cert": <The certificate and chain that was constructed On 2014/05/31 02:48:03, wtc wrote: > > Remove "and" from "certificate and chain". Done. https://codereview.chromium.org/303133006/diff/60001/net/base/net_log_event_t... net/base/net_log_event_type_list.h:1912: // during verification. Note that the though the verified On 2014/05/31 02:48:03, wtc wrote: > > In "the though the", I think the first "the" should be deleted. Done. https://codereview.chromium.org/303133006/diff/60001/net/base/net_log_event_t... net/base/net_log_event_type_list.h:1915: // the intermediate certificates stored within may be On 2014/05/31 02:41:11, Ryan Sleevi wrote: > You should be able to fold this onto 1914 (and adjust the lines below) Done. https://codereview.chromium.org/303133006/diff/60001/net/cert/multi_threaded_... File net/cert/multi_threaded_cert_verifier.cc (right): https://codereview.chromium.org/303133006/diff/60001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:22: #include "net/cert/cert_status_flags.h" On 2014/05/31 02:41:11, Ryan Sleevi wrote: > I don't believe you need this header anymore? We're just using the numeric value > from verify_result.cert_status for now Done. https://codereview.chromium.org/303133006/diff/60001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:33: namespace { On 2014/05/31 02:48:03, wtc wrote: > > I suggest that you nest this unnamed namespace inside the net namespace, so that > the CertVerifyResultCallback function can omit the net:: prefix. > > namespace net { > > namespace { > > base::Value* CertVerifyResultCallback(const CertVerifyResult& verify_result, > ... > } > > } // namespace > > ... > > } // namespace net Done. https://codereview.chromium.org/303133006/diff/60001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:35: base::Value* CertVerifyResultCallback(net::CertVerifyResult verify_result, On 2014/05/31 02:41:11, Ryan Sleevi wrote: > STYLE: So, in the previous comment, I mentioned you could move this entire > function to line 115 > > Reasoning: > > 1) We generally prefer one unnamed namespace in a file, that contains all of the > 'helper' functions > 2) The choice of 115 (versus, say, 109 or 112) is to match the recommended order > of declarations > 3) It allows you to delete the "net::" on lines 35, 36, and 49 Done. https://codereview.chromium.org/303133006/diff/60001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:35: base::Value* CertVerifyResultCallback(net::CertVerifyResult verify_result, On 2014/05/31 02:41:11, Ryan Sleevi wrote: > STYLE: So, in the previous comment, I mentioned you could move this entire > function to line 115 > > Reasoning: > > 1) We generally prefer one unnamed namespace in a file, that contains all of the > 'helper' functions > 2) The choice of 115 (versus, say, 109 or 112) is to match the recommended order > of declarations > 3) It allows you to delete the "net::" on lines 35, 36, and 49 Done. https://codereview.chromium.org/303133006/diff/60001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:35: base::Value* CertVerifyResultCallback(net::CertVerifyResult verify_result, On 2014/05/31 02:41:11, Ryan Sleevi wrote: > STYLE: So, in the previous comment, I mentioned you could move this entire > function to line 115 > > Reasoning: > > 1) We generally prefer one unnamed namespace in a file, that contains all of the > 'helper' functions > 2) The choice of 115 (versus, say, 109 or 112) is to match the recommended order > of declarations > 3) It allows you to delete the "net::" on lines 35, 36, and 49 Done. https://codereview.chromium.org/303133006/diff/60001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:35: base::Value* CertVerifyResultCallback(net::CertVerifyResult verify_result, On 2014/05/31 02:48:03, wtc wrote: > > Since CertVerifyResult is a struct with several members, I think this parameter > should be declared as a const reference: > > const net::CertVerifyResult& verify_result > > See the Style Guide: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Reference_Argu... Done. https://codereview.chromium.org/303133006/diff/60001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:35: base::Value* CertVerifyResultCallback(net::CertVerifyResult verify_result, On 2014/05/31 02:48:03, wtc wrote: > > Since CertVerifyResult is a struct with several members, I think this parameter > should be declared as a const reference: > > const net::CertVerifyResult& verify_result > > See the Style Guide: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Reference_Argu... Done. https://codereview.chromium.org/303133006/diff/60001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:35: base::Value* CertVerifyResultCallback(net::CertVerifyResult verify_result, On 2014/05/31 02:41:11, Ryan Sleevi wrote: > STYLE: So, in the previous comment, I mentioned you could move this entire > function to line 115 > > Reasoning: > > 1) We generally prefer one unnamed namespace in a file, that contains all of the > 'helper' functions > 2) The choice of 115 (versus, say, 109 or 112) is to match the recommended order > of declarations > 3) It allows you to delete the "net::" on lines 35, 36, and 49 Done. https://codereview.chromium.org/303133006/diff/60001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:35: base::Value* CertVerifyResultCallback(net::CertVerifyResult verify_result, On 2014/05/31 02:48:03, wtc wrote: > > Since CertVerifyResult is a struct with several members, I think this parameter > should be declared as a const reference: > > const net::CertVerifyResult& verify_result > > See the Style Guide: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Reference_Argu... Done. https://codereview.chromium.org/303133006/diff/60001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:35: base::Value* CertVerifyResultCallback(net::CertVerifyResult verify_result, On 2014/05/31 02:41:11, Ryan Sleevi wrote: > STYLE: So, in the previous comment, I mentioned you could move this entire > function to line 115 > > Reasoning: > > 1) We generally prefer one unnamed namespace in a file, that contains all of the > 'helper' functions > 2) The choice of 115 (versus, say, 109 or 112) is to match the recommended order > of declarations > 3) It allows you to delete the "net::" on lines 35, 36, and 49 Done. https://codereview.chromium.org/303133006/diff/60001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:55: it++) { On 2014/05/31 02:48:03, wtc wrote: > > Use pre-increment to increment an iterator. See the Style Guide: > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Preincrement_a... Done. https://codereview.chromium.org/303133006/diff/60001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.cc:63: } On 2014/05/31 02:48:03, wtc wrote: > > 1. Add a blank line before this line. > > 2. This line is the closing '}' of a namespace. Our Style Guide recommends that > we add a comment: > > } // namespace > > See > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespace_Form... Done. https://codereview.chromium.org/303133006/diff/60001/net/cert/multi_threaded_... File net/cert/multi_threaded_cert_verifier.h (right): https://codereview.chromium.org/303133006/diff/60001/net/cert/multi_threaded_... net/cert/multi_threaded_cert_verifier.h:173: On 2014/05/31 02:48:03, wtc wrote: > > Did cpplint.py or clang-format suggest adding this blank line at the end of the > file? Done.
Almost there. One small nit, then it looks good. I'll let Wan-Teh have final say in case I missed something (inevitably, I have...) https://codereview.chromium.org/303133006/diff/120001/net/base/net_log_event_... File net/base/net_log_event_type_list.h (right): https://codereview.chromium.org/303133006/diff/120001/net/base/net_log_event_... net/base/net_log_event_type_list.h:1924: // building. Only present when byte logging is enabled.> This block ("certificates"), is part of the BEGIN phase event That is, around line 1884 // The BEGIN phase event parameters are: // { // "certificates": <...> // } // The END phase event parameters are: // { // ... // } Use lines 49 or 61 as templates
https://codereview.chromium.org/303133006/diff/120001/net/base/net_log_event_... File net/base/net_log_event_type_list.h (right): https://codereview.chromium.org/303133006/diff/120001/net/base/net_log_event_... net/base/net_log_event_type_list.h:1924: // building. Only present when byte logging is enabled.> On 2014/06/02 20:44:01, Ryan Sleevi wrote: > This block ("certificates"), is part of the BEGIN phase event > > That is, around line 1884 > > // The BEGIN phase event parameters are: > // { > // "certificates": <...> > // } > // The END phase event parameters are: > // { > // ... > // } > > Use lines 49 or 61 as templates Done.
Patch set 8 LGTM. Please fix the nits, upload a new snapshot, and check the Commit box. https://codereview.chromium.org/303133006/diff/140001/net/base/net_log_event_... File net/base/net_log_event_type_list.h (right): https://codereview.chromium.org/303133006/diff/140001/net/base/net_log_event_... net/base/net_log_event_type_list.h:1890: // } Please fix the indentation of lines 1885-1890. They should look like this: The BEGIN phase event parameters are: { "certificates": <A list of PEM encoded certificates, the first one being the certificate to verify and the remaining ... building. Only present when byte logging is enabled.> } https://codereview.chromium.org/303133006/diff/140001/net/base/net_log_event_... net/base/net_log_event_type_list.h:1921: // "verified_cert": <The certificate chain that was constructed It would be better to log the struct members in the same order they are declared in net/cert/cert_verify_result.h. Update: ah, I see. These are ordered alphabetically by the dictionary. Never mind. https://codereview.chromium.org/303133006/diff/140001/net/base/net_log_event_... net/base/net_log_event_type_list.h:1929: // Delete this blank line. https://codereview.chromium.org/303133006/diff/140001/net/cert/multi_threaded... File net/cert/multi_threaded_cert_verifier.cc (right): https://codereview.chromium.org/303133006/diff/140001/net/cert/multi_threaded... net/cert/multi_threaded_cert_verifier.cc:107: results->Set("public_key_hashes", hashes); Please use blank lines to suggest that lines 99-107 belong to the same group of statements that operate on public_key_hashes: ... NetLogX509CertificateCallback(verify_result.verified_cert, log_level)); base::ListValue* hashes = new base::ListValue(); for (std::vector<HashValue>::const_iterator it = ... } results->Set("public_key_hashes", hashes); return results; https://codereview.chromium.org/303133006/diff/140001/net/cert/multi_threaded... net/cert/multi_threaded_cert_verifier.cc:619: Nit: if this line was not added by git cl format, you can delete it.
The CQ bit was checked by mshelley@google.com
The CQ bit was checked by mshelley@google.com
The CQ bit was unchecked by mshelley@google.com
The CQ bit was checked by mshelley@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mshelley@chromium.org/303133006/180001
The CQ bit was checked by mshelley@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mshelley@chromium.org/303133006/180001
Message was sent while issue was closed.
Change committed as 274456
Message was sent while issue was closed.
https://codereview.chromium.org/303133006/diff/180001/net/cert/multi_threaded... File net/cert/multi_threaded_cert_verifier.cc (right): https://codereview.chromium.org/303133006/diff/180001/net/cert/multi_threaded... net/cert/multi_threaded_cert_verifier.cc:83: base::Value* CertVerifyResultCallback(const CertVerifyResult& verify_result, Please make this a "const CertVerifyResult*" rather than a const reference. It will avoid making a copy of |verify_result|. Unfortunately the usage of these callbacks is fairly subtle. Since this function is used with base::Bind() a copy ends up being made of each parameter. For more information see "How custom parameters are attached to events" in http://www.chromium.org/developers/design-documents/network-stack/netlog As an aside: This design was originally done for convenience, but in my opinion has turned out to be too cumbersome. Alternative proposed in https://code.google.com/p/chromium/issues/detail?id=236915#c5
Message was sent while issue was closed.
https://codereview.chromium.org/303133006/diff/180001/net/cert/multi_threaded... File net/cert/multi_threaded_cert_verifier.cc (right): https://codereview.chromium.org/303133006/diff/180001/net/cert/multi_threaded... net/cert/multi_threaded_cert_verifier.cc:83: base::Value* CertVerifyResultCallback(const CertVerifyResult& verify_result, On 2014/06/04 19:41:53, eroman wrote: > Please make this a "const CertVerifyResult*" rather than a const reference. It > will avoid making a copy of |verify_result|. > > Unfortunately the usage of these callbacks is fairly subtle. Since this function > is used with base::Bind() a copy ends up being made of each parameter. Or just use base::ConstRef(). I feel like the pointer change has the potential of making callers worry about life time management themselves and what it might mean, while base::ConstRef() on line 389 allows specific instantiations (eg: this place, which is Known Safe) to avoid the extra copy. > > For more information see "How custom parameters are attached to events" in > http://www.chromium.org/developers/design-documents/network-stack/netlog > > As an aside: This design was originally done for convenience, but in my opinion > has turned out to be too cumbersome. Alternative proposed in > https://code.google.com/p/chromium/issues/detail?id=236915#c5 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
