Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1222953002:
Certificate Transparency: Add STH Fetching capability. (Closed)
|
Created:
5 years, 5 months ago by Eran Messeri Modified:
5 years, 3 months ago CC:
Cait (Slow), cbentzel+watch_chromium.org, chromium-reviews, davidben, Ryan Sleevi Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCertificate Transparency: Add STH Fetching capability.
This will be used by the CT's Tree State Tracker to fetch STHs and
consistency proofs from CT logs.
BUG=506227
Committed: https://crrev.com/d86db1082a7f7a90813641397b04e6c3097ab0f6
Cr-Commit-Position: refs/heads/master@{#347254}
Patch Set 1 #Patch Set 2 : Simplified testing #Patch Set 3 : Build file fixes #Patch Set 4 : Merging with master #
Total comments: 28
Patch Set 5 : Addressing review comments #
Total comments: 59
Patch Set 6 : Simplified handling of responses per review comments #
Total comments: 69
Patch Set 7 : Improving tests, addressing review comments #Patch Set 8 : Mac compilation fix #
Total comments: 12
Patch Set 9 : Addressing review comments #
Total comments: 39
Patch Set 10 : Simplified tests, FetchState again an implementation detail #
Total comments: 53
Patch Set 11 : Further test improvements #
Total comments: 10
Patch Set 12 : Addressed comments regarding test #Patch Set 13 : Adding OWNERS #
Total comments: 8
Patch Set 14 : BUILD.gn fixes #Patch Set 15 : Merging with master #
Total comments: 1
Patch Set 16 : Removing explicit base dep #Messages
Total messages: 54 (15 generated)
eranm@chromium.org changed reviewers: + davidben@chromium.org
David - this is the start of the certificate_transparency component, broken off from the draft implementation in https://codereview.chromium.org/1100003006/ (which you reviewed). All comments from that issue were addressed in this issue. It only contains the functionality for fetching Signed Tree Heads, nothing more.
Initial brief pass. I stopped largely because, after consulting with mmenke, he suggested I point you at URLFetcher rather than URLRequest, so save the trouble with Read() being a mess; all you care about is stuffing it into a string anyway. NOTE: Ryan, I'm sure, would not let me give this recommendation without warning that URLFetcher isn't going to handle random things like proxy auth requests and client certs and whatnot. Although no other background requests in Chrome does either, and your code wasn't handling that, so... *shrug*. Not clear you actually want to be showing UI here. https://codereview.chromium.org/1222953002/diff/60001/components/BUILD.gn File components/BUILD.gn (right): https://codereview.chromium.org/1222953002/diff/60001/components/BUILD.gn#new... components/BUILD.gn:232: "//components/certificate_transparency", This doesn't match the gyp which only excludes it on iOS. https://codereview.chromium.org/1222953002/diff/60001/components/certificate_... File components/certificate_transparency/DEPS (right): https://codereview.chromium.org/1222953002/diff/60001/components/certificate_... components/certificate_transparency/DEPS:7: "+net/url_request", This can probably just be "+net" I expect. It doesn't look like anything finer-grained is all that common. Plus you're already pulling in net/url_request. (Or did Ryan request this? I'll defer to him then.) https://codereview.chromium.org/1222953002/diff/60001/components/certificate_... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1222953002/diff/60001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:24: struct LogProofFetcher::FetchParams { FetchState perhaps, since you're modifying it and such. https://codereview.chromium.org/1222953002/diff/60001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:25: explicit FetchParams(const std::string& id, This doesn't need the explicit. https://codereview.chromium.org/1222953002/diff/60001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:40: : request_context_(request_context), weak_factory_(this) { I think you can just define these methods all inline. https://codereview.chromium.org/1222953002/diff/60001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:66: const net::URLRequestStatus& status(request->status()); Nit: I'd probably just use equals rather than parens for these. (You use equals in line 60.) https://codereview.chromium.org/1222953002/diff/60001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:68: auto params(it->second); Mind spelling the type out for this one? I think it's useful to be able to see it's a FetchParams*. (Whereas |it|'s type is totally useless.) https://codereview.chromium.org/1222953002/diff/60001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:78: base::StringPrintf("%d %d", status.status(), status.error()); This seems a slightly unhelpful error string. Where are you envisioning the error being used? (This is unfortunately part of //net that's kind of a mess, but we're trying to get rid of status.status() anyway. We should finish killing that... it *should* be redundant with status.error() but isn't quiiiite. domain_reliability has this crazy thing: https://code.google.com/p/chromium/codesearch#chromium/src/components/domain_... One thought is to do that and then using net::ErrorToString. Though I dunno what you're planning on doing with it. Alternatively, not having an error is stupid, so https://codereview.chromium.org/1239993004/ should take care of it.) https://codereview.chromium.org/1222953002/diff/60001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:93: } This Read loop is somewhat interesting. :-) It doesn't help that URLRequest::Read is kinda ugly. I asked mmenke, and he recommended just using URLFetcher. It might save you some of this grief. https://codereview.chromium.org/1222953002/diff/60001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:121: base::StringPrintf("%d %d", status.status(), status.error())); Ditto about not being a useful error string. https://codereview.chromium.org/1222953002/diff/60001/components/certificate_... File components/certificate_transparency/log_proof_fetcher.h (right): https://codereview.chromium.org/1222953002/diff/60001/components/certificate_... components/certificate_transparency/log_proof_fetcher.h:20: namespace net { Nit: Probably put a newline after this to mirror line 26. https://codereview.chromium.org/1222953002/diff/60001/components/certificate_... components/certificate_transparency/log_proof_fetcher.h:37: typedef base::Callback<void(const std::string&, const std::string&)> Nit: using FetchSTHCallback = base::Callback<..>; (as well as the other) https://codereview.chromium.org/1222953002/diff/60001/components/certificate_... components/certificate_transparency/log_proof_fetcher.h:76: // Owns the contained requests. This also owns the FetchParams, right? So probably mention both. https://codereview.chromium.org/1222953002/diff/60001/components/components.gyp File components/components.gyp (right): https://codereview.chromium.org/1222953002/diff/60001/components/components.g... components/components.gyp:95: 'certificate_transparency.gypi', Just to confirm: there's no intention to implement this on iOS right now? (They don't use much of //chrome, so that makes sense. Although they do use //net. Dunno if they're currently checking SCTs or anything. I guess neither Android nor iOS has an EV inidcator.)
David, PTAL - The use of URLRequest rather than URLFetcher should be OK according to mmenke@, so I hope that unblocks the rest of the review :) https://codereview.chromium.org/1222953002/diff/60001/components/BUILD.gn File components/BUILD.gn (right): https://codereview.chromium.org/1222953002/diff/60001/components/BUILD.gn#new... components/BUILD.gn:232: "//components/certificate_transparency", On 2015/07/16 21:53:38, David Benjamin wrote: > This doesn't match the gyp which only excludes it on iOS. My bad, fixed. https://codereview.chromium.org/1222953002/diff/60001/components/certificate_... File components/certificate_transparency/DEPS (right): https://codereview.chromium.org/1222953002/diff/60001/components/certificate_... components/certificate_transparency/DEPS:7: "+net/url_request", On 2015/07/16 21:53:38, David Benjamin wrote: > This can probably just be "+net" I expect. It doesn't look like anything > finer-grained is all that common. Plus you're already pulling in > net/url_request. (Or did Ryan request this? I'll defer to him then.) Done. https://codereview.chromium.org/1222953002/diff/60001/components/certificate_... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1222953002/diff/60001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:24: struct LogProofFetcher::FetchParams { On 2015/07/16 21:53:38, David Benjamin wrote: > FetchState perhaps, since you're modifying it and such. Done. https://codereview.chromium.org/1222953002/diff/60001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:25: explicit FetchParams(const std::string& id, On 2015/07/16 21:53:38, David Benjamin wrote: > This doesn't need the explicit. Done. https://codereview.chromium.org/1222953002/diff/60001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:40: : request_context_(request_context), weak_factory_(this) { On 2015/07/16 21:53:38, David Benjamin wrote: > I think you can just define these methods all inline. By "these methods", do you mean c'tor and d'tor? I thought this was strongly discouraged: https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-i.... https://codereview.chromium.org/1222953002/diff/60001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:66: const net::URLRequestStatus& status(request->status()); On 2015/07/16 21:53:39, David Benjamin wrote: > Nit: I'd probably just use equals rather than parens for these. (You use equals > in line 60.) Using equals throughout (except when a raw pointer is stored in a scoped_ptr). https://codereview.chromium.org/1222953002/diff/60001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:68: auto params(it->second); On 2015/07/16 21:53:38, David Benjamin wrote: > Mind spelling the type out for this one? I think it's useful to be able to see > it's a FetchParams*. (Whereas |it|'s type is totally useless.) Done. https://codereview.chromium.org/1222953002/diff/60001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:78: base::StringPrintf("%d %d", status.status(), status.error()); On 2015/07/16 21:53:39, David Benjamin wrote: > This seems a slightly unhelpful error string. Where are you envisioning the > error being used? > > (This is unfortunately part of //net that's kind of a mess, but we're trying to > get rid of status.status() anyway. We should finish killing that... it *should* > be redundant with status.error() but isn't quiiiite. domain_reliability has this > crazy thing: > > https://code.google.com/p/chromium/codesearch#chromium/src/components/domain_... > > One thought is to do that and then using net::ErrorToString. Though I dunno what > you're planning on doing with it. > > Alternatively, not having an error is stupid, so > https://codereview.chromium.org/1239993004/ should take care of it.) You're right - the error string is unhelpful, as I expect the callback to ultimately log something to UMA so an integer / enum here is more appropriate. In fact, what I need is very similar to what domain_reliability has. https://codereview.chromium.org/1222953002/diff/60001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:93: } On 2015/07/16 21:53:39, David Benjamin wrote: > This Read loop is somewhat interesting. :-) It doesn't help that > URLRequest::Read is kinda ugly. I asked mmenke, and he recommended just using > URLFetcher. It might save you some of this grief. Interesting as in "there are some odd edge cases it spectacularly fails to handle" or as in "give me a few minutes and I'll find some odd edge cases it spectacularly fails to handle" ? :) Re URLRequest::Delegate vs URLFetcher, see the mail I've followed-up on with why I think a URLFetcher would be painful to use here. But if there's a decent way to wire it in then I could switch to using that. One option I can think of is having my own URLRequestContextGetter that is *not* the same one as the IOThread / ProfileIOData have. It should be OK because I know this object will be freed up before the URLRequestContext it holds is destroyed. Do you agree with this proposed design approach? https://codereview.chromium.org/1222953002/diff/60001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:121: base::StringPrintf("%d %d", status.status(), status.error())); On 2015/07/16 21:53:38, David Benjamin wrote: > Ditto about not being a useful error string. Done. https://codereview.chromium.org/1222953002/diff/60001/components/certificate_... File components/certificate_transparency/log_proof_fetcher.h (right): https://codereview.chromium.org/1222953002/diff/60001/components/certificate_... components/certificate_transparency/log_proof_fetcher.h:20: namespace net { On 2015/07/16 21:53:39, David Benjamin wrote: > Nit: Probably put a newline after this to mirror line 26. Done. https://codereview.chromium.org/1222953002/diff/60001/components/certificate_... components/certificate_transparency/log_proof_fetcher.h:37: typedef base::Callback<void(const std::string&, const std::string&)> On 2015/07/16 21:53:39, David Benjamin wrote: > Nit: > using FetchSTHCallback = base::Callback<..>; > (as well as the other) Done. https://codereview.chromium.org/1222953002/diff/60001/components/certificate_... components/certificate_transparency/log_proof_fetcher.h:76: // Owns the contained requests. On 2015/07/16 21:53:39, David Benjamin wrote: > This also owns the FetchParams, right? So probably mention both. Done. https://codereview.chromium.org/1222953002/diff/60001/components/components.gyp File components/components.gyp (right): https://codereview.chromium.org/1222953002/diff/60001/components/components.g... components/components.gyp:95: 'certificate_transparency.gypi', On 2015/07/16 21:53:39, David Benjamin wrote: > Just to confirm: there's no intention to implement this on iOS right now? (They > don't use much of //chrome, so that makes sense. Although they do use //net. > Dunno if they're currently checking SCTs or anything. I guess neither Android > nor iOS has an EV inidcator.) Correct. Note that STH fetching and consistency checking is independent of EV support - i.e. ultimately inclusion of certificates in the log will be checked for any certificate that is accompanied by SCTs. Not sure which shape it'll take on Android exactly, I think I'll worry about iOS last.
davidben@chromium.org changed reviewers: + mmenke@chromium.org
Matt, mind if I toss this at you? You're more familiar with URLRequest::Read and I have bug triage duty now.
https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:65: ++it) Can just delete these - no need to cancel first. (Should probably have a comment about that, though) https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:75: GURL fetch_url(log_url.Resolve("ct/v1/get-sth")); log_url -> base_log_url, maybe? I assume we really do want to take in a full URL here, and not just a hostname. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:81: it->first->Start(); I think this is sufficiently ugly that you may just want to start the request before adding it to inflight_requests_ (URLRequest won't invoke any callbacks synchronously). https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:85: const net::URLRequestStatus& status(request->status()); Given that this is just a pair of ints, suggest just copying it, or using request->status() everywhere. This is just because references aren't use a whole lot in chrome, unless they're really needed (Except for function arguments and accessor return vales). https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:86: auto it = inflight_requests_.find(request); optional: Suggest avoiding the use of auto unless it's needed - at least I find having to mentally map it to std::pair(blah, blah) adds cognitive overhead. So I'd suggest something like DCHECK(inflight_requests_.count(request)); FetchState* fetch_state = inflight_requests_.find(request)->second; https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:89: DCHECK(it != inflight_requests_.end()); This DCHECK should go before you potentially dereference end()->second. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:99: error_value = request->GetResponseCode(); Packing in a net error and an HTTP response code in the same field just seems like asking for trouble. What do we expect to use this for? https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:103: params->failed_callback.Run(params->log_id, error_value); Can we just invoke the callback before calling CleanupRequest, instead of having CleanupRequest return a scoped_ptr? https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:120: params->read_bytes = bytes_read; I don't think you need the read_bytes field. Can just pass bytes read directly to CheckReadStatus. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:127: } This exactly matches the end of LogProofFetcher::OnResponseStarted. I suggest moving it into KickOffARead. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:130: bool LogProofFetcher::CheckReadStatus(net::URLRequest* request, HandleReadResult maybe? https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:135: auto status = request->status(); Writing out net::URLRequestStatus isn't a huge task, or just inlining request->status() everywhere. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:152: DVLOG(1) << "No more bytes to read, finishing."; Do you plan on keeping all these log statements around? https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:159: DCHECK(bytes_read > 0); DCHECK_GE https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:161: sth_fragment.AppendToString(¶ms->assembled_response); Why not just use: params->assumbed_response.append(params->response_buffer->data(), bytes_read); https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:162: params->total_response_size += bytes_read; Why is this needed? Doesn't params->assumbed_response.length() or size() give you what you want? https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:178: if (it->first != request) { Get rid of this branch - shouldn't handle cases that shouldn't be possible. Better to crash and find the bug. Also, this is a really weird check to rely on. If the DCHECK above passes, and this fails, presumably it's an std::map bug.... https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:186: DCHECK(params->total_response_size < kMaxLogResponseSizeInBytes); DCHECK_LT will produce more useful output on trybots. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:232: net::ct::SignedTreeHead sth; Again, suggest writing this out. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:257: } These should be up with the declaration of FetchState. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... File components/certificate_transparency/log_proof_fetcher.h (right): https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.h:36: using FetchSTHCallback = Google style discourages use of uncommon abbreviations, which I'd say "STH" is. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.h:38: using FetchFailedCallback = base::Callback<void(const std::string&, int)>; Need to include callback header. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.h:38: using FetchFailedCallback = base::Callback<void(const std::string&, int)>; I believe the callback arguments should be named https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.h:38: using FetchFailedCallback = base::Callback<void(const std::string&, int)>; Callbacks should be documented. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.h:39: explicit LogProofFetcher(net::URLRequestContext* request_context); nit: Add blank line above constructor. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.h:47: FetchFailedCallback failed_cb); Write out callback. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.h:47: FetchFailedCallback failed_cb); Callbacks are generally passed via const ref (Even though they technically can be mutated by invoking them). https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.h:79: base::WeakPtrFactory<LogProofFetcher> weak_factory_; Include weak_ptr.h https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.h:81: DISALLOW_COPY_AND_ASSIGN(LogProofFetcher); Include base/macros.h for DISALLOW_COPY_AND_ASSIGN.
Thanks for your patience, PTAL. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:65: ++it) On 2015/07/17 17:57:47, mmenke wrote: > Can just delete these - no need to cancel first. (Should probably have a > comment about that, though) Done. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:75: GURL fetch_url(log_url.Resolve("ct/v1/get-sth")); On 2015/07/17 17:57:47, mmenke wrote: > log_url -> base_log_url, maybe? I assume we really do want to take in a full > URL here, and not just a hostname. Correct, we do want to take in a full URL, not just a hostname (multiple logs can be hosted on the same host). Changed to base_log_url. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:81: it->first->Start(); On 2015/07/17 17:57:46, mmenke wrote: > I think this is sufficiently ugly that you may just want to start the request > before adding it to inflight_requests_ (URLRequest won't invoke any callbacks > synchronously). Done. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:85: const net::URLRequestStatus& status(request->status()); On 2015/07/17 17:57:46, mmenke wrote: > Given that this is just a pair of ints, suggest just copying it, or using > request->status() everywhere. > > This is just because references aren't use a whole lot in chrome, unless they're > really needed (Except for function arguments and accessor return vales). Done - copied the value. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:86: auto it = inflight_requests_.find(request); On 2015/07/17 17:57:47, mmenke wrote: > optional: Suggest avoiding the use of auto unless it's needed - at least I find > having to mentally map it to std::pair(blah, blah) adds cognitive overhead. > > So I'd suggest something like > DCHECK(inflight_requests_.count(request)); > > FetchState* fetch_state = inflight_requests_.find(request)->second; Done - adopted your suggestion. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:89: DCHECK(it != inflight_requests_.end()); On 2015/07/17 17:57:47, mmenke wrote: > This DCHECK should go before you potentially dereference end()->second. Done. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:99: error_value = request->GetResponseCode(); On 2015/07/17 17:57:47, mmenke wrote: > Packing in a net error and an HTTP response code in the same field just seems > like asking for trouble. > > What do we expect to use this for? You're right - I thought I saw this pattern somewhere else but I was, apparently, mistaken. Now separated to net error value and http response code. It will be used to keep track of the reasons communication to a particular CT log is failing - ultimately reported through UMA (or a similar mechanism) for each individual CT log. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:103: params->failed_callback.Run(params->log_id, error_value); On 2015/07/17 17:57:47, mmenke wrote: > Can we just invoke the callback before calling CleanupRequest, instead of having > CleanupRequest return a scoped_ptr? Yes, done. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:120: params->read_bytes = bytes_read; On 2015/07/17 17:57:47, mmenke wrote: > I don't think you need the read_bytes field. Can just pass bytes read directly > to CheckReadStatus. Changed to pass the bytes_read into HandleReadResult. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:127: } On 2015/07/17 17:57:47, mmenke wrote: > This exactly matches the end of LogProofFetcher::OnResponseStarted. I suggest > moving it into KickOffARead. Done. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:130: bool LogProofFetcher::CheckReadStatus(net::URLRequest* request, On 2015/07/17 17:57:46, mmenke wrote: > HandleReadResult maybe? Done. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:135: auto status = request->status(); On 2015/07/17 17:57:47, mmenke wrote: > Writing out net::URLRequestStatus isn't a huge task, or just inlining > request->status() everywhere. Done. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:152: DVLOG(1) << "No more bytes to read, finishing."; On 2015/07/17 17:57:46, mmenke wrote: > Do you plan on keeping all these log statements around? I've removed some, what would you recommend leaving and what should be removed? https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:159: DCHECK(bytes_read > 0); On 2015/07/17 17:57:47, mmenke wrote: > DCHECK_GE Done. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:161: sth_fragment.AppendToString(¶ms->assembled_response); On 2015/07/17 17:57:47, mmenke wrote: > Why not just use: > > params->assumbed_response.append(params->response_buffer->data(), bytes_read); Done. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:162: params->total_response_size += bytes_read; On 2015/07/17 17:57:47, mmenke wrote: > Why is this needed? Doesn't params->assumbed_response.length() or size() give > you what you want? Yes, removed. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:178: if (it->first != request) { On 2015/07/17 17:57:47, mmenke wrote: > Get rid of this branch - shouldn't handle cases that shouldn't be possible. > Better to crash and find the bug. Also, this is a really weird check to rely > on. If the DCHECK above passes, and this fails, presumably it's an std::map > bug.... My bad, this is a leftover check from an older version of the code. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:186: DCHECK(params->total_response_size < kMaxLogResponseSizeInBytes); On 2015/07/17 17:57:46, mmenke wrote: > DCHECK_LT will produce more useful output on trybots. Done. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:232: net::ct::SignedTreeHead sth; On 2015/07/17 17:57:46, mmenke wrote: > Again, suggest writing this out. Done. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:257: } On 2015/07/17 17:57:47, mmenke wrote: > These should be up with the declaration of FetchState. Done. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... File components/certificate_transparency/log_proof_fetcher.h (right): https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.h:36: using FetchSTHCallback = On 2015/07/17 17:57:47, mmenke wrote: > Google style discourages use of uncommon abbreviations, which I'd say "STH" is. Done, renamed to SignedTreeHeadFetchedCallback. Also renamed FetchSTH to FetchSignedTreeHead. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.h:38: using FetchFailedCallback = base::Callback<void(const std::string&, int)>; On 2015/07/17 17:57:47, mmenke wrote: > I believe the callback arguments should be named Done. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.h:38: using FetchFailedCallback = base::Callback<void(const std::string&, int)>; On 2015/07/17 17:57:48, mmenke wrote: > Need to include callback header. Done. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.h:38: using FetchFailedCallback = base::Callback<void(const std::string&, int)>; On 2015/07/17 17:57:47, mmenke wrote: > I believe the callback arguments should be named Done. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.h:39: explicit LogProofFetcher(net::URLRequestContext* request_context); On 2015/07/17 17:57:48, mmenke wrote: > nit: Add blank line above constructor. Done. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.h:47: FetchFailedCallback failed_cb); On 2015/07/17 17:57:47, mmenke wrote: > Write out callback. Done. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.h:47: FetchFailedCallback failed_cb); On 2015/07/17 17:57:47, mmenke wrote: > Callbacks are generally passed via const ref (Even though they technically can > be mutated by invoking them). Done. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.h:79: base::WeakPtrFactory<LogProofFetcher> weak_factory_; On 2015/07/17 17:57:47, mmenke wrote: > Include weak_ptr.h Done. https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.h:81: DISALLOW_COPY_AND_ASSIGN(LogProofFetcher); On 2015/07/17 17:57:47, mmenke wrote: > Include base/macros.h for DISALLOW_COPY_AND_ASSIGN. Done.
Volunteer sheriffing today, and the tree is not in a good state. Probably won't get to this until tomorrow.
https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1222953002/diff/80001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:152: DVLOG(1) << "No more bytes to read, finishing."; On 2015/07/29 15:16:58, Eran wrote: > On 2015/07/17 17:57:46, mmenke wrote: > > Do you plan on keeping all these log statements around? > > I've removed some, what would you recommend leaving and what should be removed? I mostly use breakpoint debugging, so not a big fan of having them in general, particularly when they require you to add more control statements. Others actually use them a lot, and have a rather different opinion. Don't feel like I can offer much guidance here. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... File components/certificate_transparency.gypi (right): https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency.gypi:17: '../content/content.gyp:content_common', What needs this? https://codereview.chromium.org/1222953002/diff/100001/components/certificate... File components/certificate_transparency/BUILD.gn (right): https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/BUILD.gn:14: "//content", What needs this? And is there no content_common in the gn build, like what you use in the other file? https://codereview.chromium.org/1222953002/diff/100001/components/certificate... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:21: const unsigned long kMaxLogResponseSizeInBytes = 600u; This should be static, or in an anonymous namespace. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:21: const unsigned long kMaxLogResponseSizeInBytes = 600u; This should be size_t, to match IOBufferWithSize argument size. Also, "long" and "unsigned" aren't used much. If an int isn't enough, prefer uint32_t / uint64_t, etc. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:54: int read_bytes; This member is now serving no purpose, and can be removed. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:83: const FetchFailedCallback& failed_callback) { Is there some check upstream that base_log_url is HTTP or HTTPS? If so, should DCHECK that's the case here. If not, should probably just fail the request if that's not the case. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:93: void LogProofFetcher::OnResponseStarted(net::URLRequest* request) { I assume we should be following redirects, if any? (This code will do that, just verifying it's what we want) https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:99: if (!status.is_success() || request->GetResponseCode() != 200) { 200 -> net::HTTP_OK? Should be consistent within the file. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:100: int error_value = 0; 0 -> net::OK https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:100: int error_value = 0; Suggest calling this net_error here, and in the declaration of FailedCallback. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:107: } else { // request->GetResponseCode != 200 I suggest removing this comment - it's confusing. I would have guessed it applies to the following code, not the preceding code, which is not the case. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:109: error_value = net::OK; Remove the error_value = line. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:126: bool another_read = HandleReadResult(request, fetch_state, bytes_read); "continue_reading" seems a bit clearer to me, and matches what LogProofFetcher::KickOffARead calls the value. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:127: KickOffARead(request, fetch_state, another_read); Having a bool that means "Should I really kick off a read" passed to KickOffARead seems weird. Suggest getting rid of the last argument to KickOffARead, and just not calling it if another_read is false. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:132: const int bytes_read) { Remove const on bytes_read. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:137: int error_value = GetNetErrorFromURLRequestStatus(status); again, suggest net_error https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:143: // Not an error, but no data available, so wait for OnReadComplete OnReadCompleted https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:147: } optional: Prevailing style in net/ is not to use braces in this case. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:150: if (bytes_read == 0) { Optional: May want to handle the complete case last, so that the code is in the order it occurs. Of course, this way, total indentation is reduced, and the true path is at the end... https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:169: while (continue_reading) { Per earlier comment, should remove the should_read argument. Suggest just making this while(true) as well. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:183: DCHECK_LT(params->assembled_response.size(), kMaxLogResponseSizeInBytes); BUG: While you're making sure each read is <= (Not less than) kMaxLogResponseSizeInBytes, you have no code to make sure they're less than kMaxLogResponseSizeInBytes once you append them all together. Also, you should add a test for this case: Bot a response exactly kMaxLogResponseSizeInBytes and a longer response. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:195: weak_factory_.GetWeakPtr(), request_params)); You're copying request_params 3 times in this function, which seems a bit much. Maybe just keep the request around, and pass along a pointer to it? Or could just pass in the callback and the log_id (Or even use base::Bind with the callback and log id and just pass in the new callback, I suppose). https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:208: scoped_ptr<net::URLRequest> url_request(it->first); Suggest just deleting these - I don't think the scoped_ptrs give us much. Could use STLDeleteContainerPairPointers(it, it->next()), but also not sure that gets us anything. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:221: request->set_method("GET"); Not needed. "GET" is the default method. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... File components/certificate_transparency/log_proof_fetcher.h (right): https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:13: #include "base/memory/ref_counted.h" Only used in the CC file. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:17: #include "url/gurl.h" Can forward declare GURL, and move the include into the cc file. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:43: base::Callback<void(const std::string& log_id, This log ID is an STH thing, right, as opposed to a thing used by the caller to identify the request. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:44: const net::ct::SignedTreeHead& signed_tree_head)>; nit: Suggest a blank line here. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:46: // of the log fetching was requested for and a net error code of the failure. "of the log fetching" -> "that the log fetching" https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:71: void KickOffARead(net::URLRequest* request, Suggest renaming this to something like ReadBody, Read, StartNextRead, StartBodyRead, etc. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:73: bool should_read); Should document these two methods, and their return values. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:84: void OnSTHJsonParseSuccess(FetchState params, FetchState isn't only forward declared at this point...Does this even compile? https://codereview.chromium.org/1222953002/diff/100001/components/certificate... File components/certificate_transparency/log_proof_fetcher_unittest.cc (right): https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:194: Suggested other tests: Network error before we get any headers, network error once we start receiving the body. Non-2xx status code. https://codereview.chromium.org/1222953002/diff/100001/net/base/net_error_list.h File net/base/net_error_list.h (right): https://codereview.chromium.org/1222953002/diff/100001/net/base/net_error_lis... net/base/net_error_list.h:354: NET_ERROR(CT_STH_INCOMPLETE, -168) Hrm...Having a net error exist exclusively for stuff outside net/ seems a bit ugly, though suppose we already do that in a number of places.
Thanks for the quick and through review; It has surfaced the issue of what to do with multiple FetchSignedTreeHead requests to the same log. I'd like to address it in a follow-up CL, though, if necessary. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... File components/certificate_transparency.gypi (right): https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency.gypi:17: '../content/content.gyp:content_common', On 2015/07/29 18:51:50, mmenke wrote: > What needs this? For now, nothing - removed. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... File components/certificate_transparency/BUILD.gn (right): https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/BUILD.gn:14: "//content", On 2015/07/29 18:51:50, mmenke wrote: > What needs this? And is there no content_common in the gn build, like what you > use in the other file? For now nothing uses this, removed - I'll make note to match dependencies between the gn and gyp when it is needed. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:21: const unsigned long kMaxLogResponseSizeInBytes = 600u; On 2015/07/29 18:51:51, mmenke wrote: > This should be static, or in an anonymous namespace. Done. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:21: const unsigned long kMaxLogResponseSizeInBytes = 600u; On 2015/07/29 18:51:51, mmenke wrote: > This should be size_t, to match IOBufferWithSize argument size. Also, "long" > and "unsigned" aren't used much. If an int isn't enough, prefer uint32_t / > uint64_t, etc. Done. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:54: int read_bytes; On 2015/07/29 18:51:51, mmenke wrote: > This member is now serving no purpose, and can be removed. Done. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:83: const FetchFailedCallback& failed_callback) { On 2015/07/29 18:51:51, mmenke wrote: > Is there some check upstream that base_log_url is HTTP or HTTPS? If so, should > DCHECK that's the case here. If not, should probably just fail the request if > that's not the case. Done - added DCHECK. The log URLs are hard-coded into Chrome (https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/ct_known_...) so the DCHECK should be enough. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:93: void LogProofFetcher::OnResponseStarted(net::URLRequest* request) { On 2015/07/29 18:51:51, mmenke wrote: > I assume we should be following redirects, if any? (This code will do that, > just verifying it's what we want) Yes - it is allowed for a CT log to redirect. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:99: if (!status.is_success() || request->GetResponseCode() != 200) { On 2015/07/29 18:51:51, mmenke wrote: > 200 -> net::HTTP_OK? Should be consistent within the file. Done. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:100: int error_value = 0; On 2015/07/29 18:51:50, mmenke wrote: > 0 -> net::OK Done. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:100: int error_value = 0; On 2015/07/29 18:51:50, mmenke wrote: > Suggest calling this net_error here, and in the declaration of FailedCallback. Done. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:107: } else { // request->GetResponseCode != 200 On 2015/07/29 18:51:50, mmenke wrote: > I suggest removing this comment - it's confusing. I would have guessed it > applies to the following code, not the preceding code, which is not the case. Done. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:109: error_value = net::OK; On 2015/07/29 18:51:50, mmenke wrote: > Remove the error_value = line. Done, with it goes the entire else clause. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:126: bool another_read = HandleReadResult(request, fetch_state, bytes_read); On 2015/07/29 18:51:51, mmenke wrote: > "continue_reading" seems a bit clearer to me, and matches what > LogProofFetcher::KickOffARead calls the value. Done. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:127: KickOffARead(request, fetch_state, another_read); On 2015/07/29 18:51:50, mmenke wrote: > Having a bool that means "Should I really kick off a read" passed to > KickOffARead seems weird. Suggest getting rid of the last argument to > KickOffARead, and just not calling it if another_read is false. Good point, apologies for the clunkiness in the first place. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:132: const int bytes_read) { On 2015/07/29 18:51:51, mmenke wrote: > Remove const on bytes_read. Done. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:137: int error_value = GetNetErrorFromURLRequestStatus(status); On 2015/07/29 18:51:50, mmenke wrote: > again, suggest net_error Done. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:143: // Not an error, but no data available, so wait for OnReadComplete On 2015/07/29 18:51:50, mmenke wrote: > OnReadCompleted Done. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:147: } On 2015/07/29 18:51:50, mmenke wrote: > optional: Prevailing style in net/ is not to use braces in this case. Done, removed braces throughout the file where there was only one statement. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:150: if (bytes_read == 0) { On 2015/07/29 18:51:51, mmenke wrote: > Optional: May want to handle the complete case last, so that the code is in the > order it occurs. Of course, this way, total indentation is reduced, and the > true path is at the end... Unless you strongly object, I'd like to keep it this way - I realize it's a matter of personal preference, I personally find code easier to read when there's less indentation. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:169: while (continue_reading) { On 2015/07/29 18:51:50, mmenke wrote: > Per earlier comment, should remove the should_read argument. Suggest just > making this while(true) as well. Removed the should_read argument. Don't have a strong feeling about while(true) - will change if you prefer it this way. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:183: DCHECK_LT(params->assembled_response.size(), kMaxLogResponseSizeInBytes); On 2015/07/29 18:51:50, mmenke wrote: > BUG: While you're making sure each read is <= (Not less than) > kMaxLogResponseSizeInBytes, you have no code to make sure they're less than > kMaxLogResponseSizeInBytes once you append them all together. > > Also, you should add a test for this case: Bot a response exactly > kMaxLogResponseSizeInBytes and a longer response. Good catch, changed the code to check each time some bytes are received that the total does not exceed kMaxLogResponseSizeInBytes (in HandleReadResult). Added a test as well. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:195: weak_factory_.GetWeakPtr(), request_params)); On 2015/07/29 18:51:51, mmenke wrote: > You're copying request_params 3 times in this function, which seems a bit much. > Maybe just keep the request around, and pass along a pointer to it? Or could > just pass in the callback and the log_id (Or even use base::Bind with the > callback and log id and just pass in the new callback, I suppose). One of the copies was redundant and removed, so now the parameters will be copied twice. Note that the callback for successful JSON parsing (OnSTHJsonParseSuccess) needs access to the error callback in case the JSON parsed OK but is missing fields to fill in the SignedTreeHead. In addition to the response_buffer, I've removed the assembled_response so it's not copied when the fetch_state is copied to the callbacks. That should imply that copying the log_id and callbacks around costs almost the same as copying the FetchState structure, because these are the only things left there, right? https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:208: scoped_ptr<net::URLRequest> url_request(it->first); On 2015/07/29 18:51:50, mmenke wrote: > Suggest just deleting these - I don't think the scoped_ptrs give us much. Could > use STLDeleteContainerPairPointers(it, it->next()), but also not sure that gets > us anything. Done. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:221: request->set_method("GET"); On 2015/07/29 18:51:51, mmenke wrote: > Not needed. "GET" is the default method. Done. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... File components/certificate_transparency/log_proof_fetcher.h (right): https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:13: #include "base/memory/ref_counted.h" On 2015/07/29 18:51:51, mmenke wrote: > Only used in the CC file. Done. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:17: #include "url/gurl.h" On 2015/07/29 18:51:51, mmenke wrote: > Can forward declare GURL, and move the include into the cc file. Done. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:43: base::Callback<void(const std::string& log_id, On 2015/07/29 18:51:51, mmenke wrote: > This log ID is an STH thing, right, as opposed to a thing used by the caller to > identify the request. It only makes sense to have one FetchSignedTreeHead request pending per log_id (which uniquely identifies the CT log the request went out to). I have clarified the documentation to indicate that and (probably in a follow-up CL) would also add a check for aborting the previous request to the log if it hasn't completed yet. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:44: const net::ct::SignedTreeHead& signed_tree_head)>; On 2015/07/29 18:51:51, mmenke wrote: > nit: Suggest a blank line here. Done. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:46: // of the log fetching was requested for and a net error code of the failure. On 2015/07/29 18:51:51, mmenke wrote: > "of the log fetching" -> "that the log fetching" Done. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:71: void KickOffARead(net::URLRequest* request, On 2015/07/29 18:51:51, mmenke wrote: > Suggest renaming this to something like ReadBody, Read, StartNextRead, > StartBodyRead, etc. Done. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:73: bool should_read); On 2015/07/29 18:51:51, mmenke wrote: > Should document these two methods, and their return values. Done. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:84: void OnSTHJsonParseSuccess(FetchState params, On 2015/07/29 18:51:51, mmenke wrote: > FetchState isn't only forward declared at this point...Does this even compile? Moved FetchState to be declared in the header. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... File components/certificate_transparency/log_proof_fetcher_unittest.cc (right): https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:194: On 2015/07/29 18:51:51, mmenke wrote: > Suggested other tests: Network error before we get any headers, network error > once we start receiving the body. Non-2xx status code. I've added a test for async IO and non-2xx status code. Any suggestions on how to add tests that simulate network error? I tracked classes extending URLRequestTestJob but could not find any example of how it's done. https://codereview.chromium.org/1222953002/diff/100001/net/base/net_error_list.h File net/base/net_error_list.h (right): https://codereview.chromium.org/1222953002/diff/100001/net/base/net_error_lis... net/base/net_error_list.h:354: NET_ERROR(CT_STH_INCOMPLETE, -168) On 2015/07/29 18:51:51, mmenke wrote: > Hrm...Having a net error exist exclusively for stuff outside net/ seems a bit > ugly, though suppose we already do that in a number of places. Acknowledged - if that helps I can move the code that generates those errors into net/, though I can't guarantee it'd be pretty.
This code doesn't crash and burn with redundant IDs - we just send both requests, so I'm fine with putting off doing something better until another CL. https://codereview.chromium.org/1222953002/diff/100001/components/certificate... File components/certificate_transparency/log_proof_fetcher_unittest.cc (right): https://codereview.chromium.org/1222953002/diff/100001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:194: On 2015/07/31 12:55:53, Eran wrote: > On 2015/07/29 18:51:51, mmenke wrote: > > Suggested other tests: Network error before we get any headers, network error > > once we start receiving the body. Non-2xx status code. > > I've added a test for async IO and non-2xx status code. > Any suggestions on how to add tests that simulate network error? I tracked > classes extending URLRequestTestJob but could not find any example of how it's > done. URLRequestFailedJob https://codereview.chromium.org/1222953002/diff/140001/components/certificate... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1222953002/diff/140001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:191: CleanupRequest(request); Suggestion: Pass in the request to LogProofFetcher::OnSTHJsonParseSuccess / LogProofFetcher::OnSTHJsonParseError, and have them cleanup the request. On the down side, it does mean the request sticks around a little longer (And it may still hold on to some network resources). On the up side, it makes the class invariants clearer, and means fetch_state doesn't have to be copyable (Which is curently is, anyways, of course) https://codereview.chromium.org/1222953002/diff/140001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:204: scoped_ptr<net::URLRequest> LogProofFetcher::CreateURLRequest( optional: May want to just inline this - I don't think we get much from a separate method. https://codereview.chromium.org/1222953002/diff/140001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:206: DCHECK(request_context_); optional: We already DCHECK on this in the constructor, not sure we get much from it here. https://codereview.chromium.org/1222953002/diff/140001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:211: net::LOAD_DO_NOT_SAVE_COOKIES); LOAD_DO_NOT_SEND_AUTH_DATA? That extra flag kinda goes hand-in-hand with privacy mode, and makes it better fit into the privacy mode use case. https://codereview.chromium.org/1222953002/diff/140001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:216: FetchState fetch_state, const FetchState& avoids a copy when invoked. https://codereview.chromium.org/1222953002/diff/140001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:228: void LogProofFetcher::OnSTHJsonParseError(FetchState fetch_state, const FetchState& avoids a copy when invoked.
Thanks for the quick review, accepted all comments. Note that I had to change the behaviour of the FetchSTHTestJob::NextReadAsync method to only return 'true' once, otherwise another read event happens that causes a CHECK in the URLRequestJob to fail (https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur... in particular) https://codereview.chromium.org/1222953002/diff/140001/components/certificate... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1222953002/diff/140001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:191: CleanupRequest(request); On 2015/07/31 15:40:25, mmenke wrote: > Suggestion: Pass in the request to LogProofFetcher::OnSTHJsonParseSuccess / > LogProofFetcher::OnSTHJsonParseError, and have them cleanup the request. On the > down side, it does mean the request sticks around a little longer (And it may > still hold on to some network resources). > > On the up side, it makes the class invariants clearer, and means fetch_state > doesn't have to be copyable (Which is curently is, anyways, of course) Done. https://codereview.chromium.org/1222953002/diff/140001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:204: scoped_ptr<net::URLRequest> LogProofFetcher::CreateURLRequest( On 2015/07/31 15:40:25, mmenke wrote: > optional: May want to just inline this - I don't think we get much from a > separate method. Done. https://codereview.chromium.org/1222953002/diff/140001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:206: DCHECK(request_context_); On 2015/07/31 15:40:25, mmenke wrote: > optional: We already DCHECK on this in the constructor, not sure we get much > from it here. N/A since I inlined this method. https://codereview.chromium.org/1222953002/diff/140001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:211: net::LOAD_DO_NOT_SAVE_COOKIES); On 2015/07/31 15:40:25, mmenke wrote: > LOAD_DO_NOT_SEND_AUTH_DATA? That extra flag kinda goes hand-in-hand with > privacy mode, and makes it better fit into the privacy mode use case. Done. https://codereview.chromium.org/1222953002/diff/140001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:216: FetchState fetch_state, On 2015/07/31 15:40:25, mmenke wrote: > const FetchState& avoids a copy when invoked. N/A since it now takes a URLRequest*. https://codereview.chromium.org/1222953002/diff/140001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:228: void LogProofFetcher::OnSTHJsonParseError(FetchState fetch_state, On 2015/07/31 15:40:25, mmenke wrote: > const FetchState& avoids a copy when invoked. N/A since it now takes a URLRequest*.
https://codereview.chromium.org/1222953002/diff/160001/components/certificate... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:108: CleanupRequest(request); Optional: There are 4 places where you call failed callback and then CleanupRequest (Excluding Json parsing errors). I think it seems a bit more robust to make a method that does both of them. Then could have OnSuccess and OnFailure methods that invoke the right callback, and pass along the corresponding data. https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:134: fetch_state->failed_callback.Run(fetch_state->log_id, net_error, 0); 0 -> net::OK https://codereview.chromium.org/1222953002/diff/160001/components/certificate... File components/certificate_transparency/log_proof_fetcher.h (right): https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:37: // construction. Should mention here, or above the constructor, that it must outlive that URLRequestContext. https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:61: // they are likely to return the same result. Maybe mention that callbacks won't be invoked if destroyed before the fetch completed? https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:77: FetchState(const std::string& id, suggest calling this log_id to match the corresponding field. https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:85: scoped_refptr<net::IOBufferWithSize> response_buffer; Need to include ref_counted.h in this file instead of the cc file, and forward declare IOBufferWithSize. Alternatively, since you're only passing pointers around to this class in this file, you could move back to just forward declaring it here, and defining it in the other file. https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:89: // Handles the result of a URLRequest::Read call on |request|. the result -> the final result / the non-ERR_IO_PENDING result? https://codereview.chromium.org/1222953002/diff/160001/components/certificate... File components/certificate_transparency/log_proof_fetcher_unittest.cc (right): https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:22: #include "testing/gtest/include/gtest/gtest.h" This should go up a line https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:38: } Can't this just be a const char[]? https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:47: std::string(kGetSTHHeaders), BUG: This std::string only includes the first line of kGetSTHHeaders, due to the NULLs. I suggest using URLRequestMockHTTPJob instead. You will need to put the headers and response bodies in separate files, but the class is much easier to use. One downside of this is it doesn't let you test both sync an async responses. https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:128: http_response_code_ = http_response_code; Suggest a sanity check here: if (net_error_ == net::OK) { ASSERT_NE(net::WhateverTheConstantFor200Is, http_response_code_); } else { ASSERT_EQ(net::WhateverTheConstantFor200Is, http_response_code_); } i.e. One and only one should indicate a failure. https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:165: known_sth_.signature.signature_data); Can we just move this into RecordFetchCallbackInvocations, and make a method to set the expected_sth? If it's NULL, and emit an error it STHFetch is invoked, and if it's non-NULL can emit an error when FetcherFailed is invoked. https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:179: context_.Init(); Not reason to do this - just remove the Init() call, and the context_(true), and it will be initialized automatically. https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:189: fetcher_.reset(new LogProofFetcher(&context_)); Can just do this in the constructor (And TearDown in the destructor). No need for the extra methods. https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:210: message_loop_.RunUntilIdle(); Not a big fan of RunUntilIdle - it does generally work when everything's happening on one thread, but why not use a RunLoop instead? Call its run method here, and have the callbacks invoke its quit method. Tends to be much more robust against guture refactorings, though it's not always doable. https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:217: GURL log_url_; const https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:221: TEST_F(LogProofFetcherTest, TestValidGetSTHReply) { Suggest getting rid of STH in test names, to make them more readable. i.e. TestValidGetSTHReply -> TestValidReply LogProofFetcherTest -> TestValidAsynReply TestInvalidGetSTHReplyIncompleteSTH -> TestInvalidReplyIncomplete etc. If the fact that these are about SignedTreeHeads is important enough to be in the names of the tests, then I'd recommend renaming LogProofFetcher and the test fixture to include it (And I'd suggest writing it out as well - does make for a rather long class name, but think it's best to be clear, since people who have no idea what an STH is may be seeing test failures). https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:224: ExpectedSuccessCallback cb; optional: Suggest just writing out callback. Style guide discourages uncommon abbreviations. While cb isn't all that uncommon, think just writing it out is much more readable. Also matches the class name. https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:272: sth_json_reply_data.append(std::string(600, ' ')); It's much better to expose MaxLogResponseSizeInBytes as a public static constant in LogProofFetcher, and just access it here. Nothing else needs it, but hard coding these sorts of constants just makes changing them a pain.
Thanks again for the quick review, accepted almost all comments. PTAL. Hopefully the more through review of the tests indicate this change is not too far from approval :) https://codereview.chromium.org/1222953002/diff/160001/components/certificate... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:108: CleanupRequest(request); On 2015/08/03 18:18:54, mmenke wrote: > Optional: There are 4 places where you call failed callback and then > CleanupRequest (Excluding Json parsing errors). I think it seems a bit more > robust to make a method that does both of them. > > Then could have OnSuccess and OnFailure methods that invoke the right callback, > and pass along the corresponding data. Done - I have a failure handling method (there's only one success path, so I left that in-lined :)) https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:134: fetch_state->failed_callback.Run(fetch_state->log_id, net_error, 0); On 2015/08/03 18:18:54, mmenke wrote: > 0 -> net::OK Done. https://codereview.chromium.org/1222953002/diff/160001/components/certificate... File components/certificate_transparency/log_proof_fetcher.h (right): https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:37: // construction. On 2015/08/03 18:18:54, mmenke wrote: > Should mention here, or above the constructor, that it must outlive that > URLRequestContext. Done. https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:61: // they are likely to return the same result. On 2015/08/03 18:18:54, mmenke wrote: > Maybe mention that callbacks won't be invoked if destroyed before the fetch > completed? Done. https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:77: FetchState(const std::string& id, On 2015/08/03 18:18:54, mmenke wrote: > suggest calling this log_id to match the corresponding field. Done. https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:85: scoped_refptr<net::IOBufferWithSize> response_buffer; On 2015/08/03 18:18:54, mmenke wrote: > Need to include ref_counted.h in this file instead of the cc file, and forward > declare IOBufferWithSize. > > Alternatively, since you're only passing pointers around to this class in this > file, you could move back to just forward declaring it here, and defining it in > the other file. Good point, I've moved back to having a forward declaration here and definition in the .cc file. https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:89: // Handles the result of a URLRequest::Read call on |request|. On 2015/08/03 18:18:54, mmenke wrote: > the result -> the final result / the non-ERR_IO_PENDING result? Changed to "the final result". I didn't go into much details on what this method does in the header file since it's documented in the method body and I didn't want to duplicate this info. https://codereview.chromium.org/1222953002/diff/160001/components/certificate... File components/certificate_transparency/log_proof_fetcher_unittest.cc (right): https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:22: #include "testing/gtest/include/gtest/gtest.h" On 2015/08/03 18:18:54, mmenke wrote: > This should go up a line Done. https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:38: } On 2015/08/03 18:18:54, mmenke wrote: > Can't this just be a const char[]? Done. https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:47: std::string(kGetSTHHeaders), On 2015/08/03 18:18:54, mmenke wrote: > BUG: This std::string only includes the first line of kGetSTHHeaders, due to > the NULLs. I suggest using URLRequestMockHTTPJob instead. You will need to put > the headers and response bodies in separate files, but the class is much easier > to use. > > One downside of this is it doesn't let you test both sync an async responses. Good point, I've passed the size to the std::string c'tor. I don't think the added complexity of the file I/O performed by URLRequestMockHTTPJob is justified in this case - the response returned by this test job is quite small and I do want to test async responses. https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:128: http_response_code_ = http_response_code; On 2015/08/03 18:18:54, mmenke wrote: > Suggest a sanity check here: > > if (net_error_ == net::OK) { > ASSERT_NE(net::WhateverTheConstantFor200Is, http_response_code_); > } else { > ASSERT_EQ(net::WhateverTheConstantFor200Is, http_response_code_); > } > > i.e. One and only one should indicate a failure. Partly done - I've adopted the first clause, as it makes sense to to assert, when recording a failure, that the error codes combined (net::OK & net::HTTP_OK) do not indicate success. However if the net_error is not net::OK I don't want to make any guarantees on the http_response_code_ as the value there may not make sense at all. https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:165: known_sth_.signature.signature_data); On 2015/08/03 18:18:54, mmenke wrote: > Can we just move this into RecordFetchCallbackInvocations, and make a method to > set the expected_sth? If it's NULL, and emit an error it STHFetch is invoked, > and if it's non-NULL can emit an error when FetcherFailed is invoked. Done - since it's the same expected_sth, I've added a boolean flag to indicate whether success is expected or not, if it is, the received STH will always be compared against the known_good one. https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:179: context_.Init(); On 2015/08/03 18:18:54, mmenke wrote: > Not reason to do this - just remove the Init() call, and the context_(true), and > it will be initialized automatically. Done. https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:189: fetcher_.reset(new LogProofFetcher(&context_)); On 2015/08/03 18:18:54, mmenke wrote: > Can just do this in the constructor (And TearDown in the destructor). No need > for the extra methods. Done. https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:210: message_loop_.RunUntilIdle(); On 2015/08/03 18:18:54, mmenke wrote: > Not a big fan of RunUntilIdle - it does generally work when everything's > happening on one thread, but why not use a RunLoop instead? Call its run method > here, and have the callbacks invoke its quit method. Tends to be much more > robust against guture refactorings, though it's not always doable. Not entirely sure why, but the interceptor (added to the URLRequestFilter in the c'tor) wouldn't get invoked unless I create a MessageLoopForIO? Does the URLRequestFilter have any assumptions on the MessageLoop used? https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:217: GURL log_url_; On 2015/08/03 18:18:54, mmenke wrote: > const Done. https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:221: TEST_F(LogProofFetcherTest, TestValidGetSTHReply) { On 2015/08/03 18:18:54, mmenke wrote: > Suggest getting rid of STH in test names, to make them more readable. > > i.e. > > TestValidGetSTHReply -> TestValidReply > LogProofFetcherTest -> TestValidAsynReply > TestInvalidGetSTHReplyIncompleteSTH -> TestInvalidReplyIncomplete > etc. > > If the fact that these are about SignedTreeHeads is important enough to be in > the names of the tests, then I'd recommend renaming LogProofFetcher and the test > fixture to include it (And I'd suggest writing it out as well - does make for a > rather long class name, but think it's best to be clear, since people who have > no idea what an STH is may be seeing test failures). Done - removed STH from test names. https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:224: ExpectedSuccessCallback cb; On 2015/08/03 18:18:54, mmenke wrote: > optional: Suggest just writing out callback. Style guide discourages uncommon > abbreviations. While cb isn't all that uncommon, think just writing it out is > much more readable. Also matches the class name. Done. https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:272: sth_json_reply_data.append(std::string(600, ' ')); On 2015/08/03 18:18:54, mmenke wrote: > It's much better to expose MaxLogResponseSizeInBytes as a public static constant > in LogProofFetcher, and just access it here. Nothing else needs it, but hard > coding these sorts of constants just makes changing them a pain. Done.
Think we're pretty much there, modulo my comments. Want to skim over the CL once you've responded, but expect that to be it. https://codereview.chromium.org/1222953002/diff/160001/components/certificate... File components/certificate_transparency/log_proof_fetcher_unittest.cc (right): https://codereview.chromium.org/1222953002/diff/160001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:210: message_loop_.RunUntilIdle(); On 2015/08/04 16:15:43, Eran wrote: > On 2015/08/03 18:18:54, mmenke wrote: > > Not a big fan of RunUntilIdle - it does generally work when everything's > > happening on one thread, but why not use a RunLoop instead? Call its run > method > > here, and have the callbacks invoke its quit method. Tends to be much more > > robust against guture refactorings, though it's not always doable. > > Not entirely sure why, but the interceptor (added to the URLRequestFilter in the > c'tor) wouldn't get invoked unless I create a MessageLoopForIO? Does the > URLRequestFilter have any assumptions on the MessageLoop used? Not that I'm aware of. You need a MessageLoopForIO for some of the sockets callbacks, but none of that should matter for this test. Feel free to investigate, if you're curious (I am, actually, I just don't have time), but not something to block the CL on. https://codereview.chromium.org/1222953002/diff/180001/components/certificate... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:12: #include "base/strings/string_piece.h" I don't think you're using this? https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:13: #include "base/strings/stringprintf.h" Not being used https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:56: scoped_refptr<net::IOBufferWithSize> response_buffer; Should include the IOBuffer header https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:206: STLDeleteContainerPairPointers(it, next_it); Any reason not to just do: STLDeleteContainerPairPointers(it, it->next()); Think it's clearer than "next_it" https://codereview.chromium.org/1222953002/diff/180001/components/certificate... File components/certificate_transparency/log_proof_fetcher.h (right): https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:35: static const size_t kMaxLogResponseSizeInBytes = 600; optional: Suggest moving this into LogProofFetcher, as a public static member - makes it clearer where it is, though the name does get a bit long. https://codereview.chromium.org/1222953002/diff/180001/components/certificate... File components/certificate_transparency/log_proof_fetcher_unittest.cc (right): https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:50: void SetAsyncIO(bool async_io) { async_io_ = async_io; } set_asnyc_io https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:54: int GetResponseCode() const override { return response_code_; } By default, this will just be grabbed from the headers we set instead. Having this not match when the headers returns seems like a bad idea, so I suggest getting rid of it and changing the header string instead. https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:57: bool NextReadAsync() override { This can be private (parent classes can access private override methods) https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:61: // that the raw_read_buffer_ is not null. Why is raw read buffer NULL? The final read of a request can normally be an asynchronous 0-byte read. Just want to know if there's a bug in the test fixture, in the new code, in TestURLRequestJob, or elsewhere in net/ https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:71: int response_code_; member variables go at the end, after all methods, including the destructor. https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:74: DISALLOW_COPY_AND_ASSIGN(FetchSTHTestJob); nit: Blank line before DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:94: void SetResponseData(std::string response_data) { set_response_data https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:95: response_data_ = response_data; Suggest response_data -> response_body everywhere. response_data is indeed what URLRequestTestJob calls it, but it's much less clear than response_body, and should probably be renamed in the test class as well, at some point. https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:98: void SetAsyncIO(bool async_io) { async_io_ = async_io; } set_asnyc_io https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:100: void SetResponseCode(int response_code) { response_code_ = response_code; } set_response_code (Though, per earlier comment, I think we should get rid of this method) https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:114: virtual void STHFetched(const std::string& log_id, virtual no longer needed https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:115: const net::ct::SignedTreeHead& sth) { ASSERT_FALSE(invoked_); https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:120: net::ct::GetSampleSignedTreeHead(&known_sth); suggest known -> expected (More common for tests, and makes it completely clear which is expected, and which is the you're testing) https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:130: ASSERT_EQ(sth.signature.signature_data, known_sth.signature.signature_data); For all of these, expected should be first. https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:135: int http_response_code) { ASSERT_FALSE(invoked_); https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:145: bool invoked() { return invoked_; } bool invoked() const ... https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:147: int net_error() { return net_error_; } bool net_error() const ... https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:149: int http_response_code() { return http_response_code_; } const https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:156: bool expect_success_; optional: Suggest making this a constructor parameter, and making it const. https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:160: int http_response_code_; All of these should be initialized in the constructor. https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:230: std::string("")); nit: std::string() (x2)
Addressed all comments, PTAL. Regarding the issue around async io tests, I'll follow up separately. https://codereview.chromium.org/1222953002/diff/180001/components/certificate... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:12: #include "base/strings/string_piece.h" On 2015/08/04 19:54:29, mmenke wrote: > I don't think you're using this? Done. https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:13: #include "base/strings/stringprintf.h" On 2015/08/04 19:54:29, mmenke wrote: > Not being used Done, trimmed a few other headers. https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:56: scoped_refptr<net::IOBufferWithSize> response_buffer; On 2015/08/04 19:54:29, mmenke wrote: > Should include the IOBuffer header Done. https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:206: STLDeleteContainerPairPointers(it, next_it); On 2015/08/04 19:54:29, mmenke wrote: > Any reason not to just do: > > STLDeleteContainerPairPointers(it, it->next()); > > Think it's clearer than "next_it" Seems like bidirectional stl iterators don't have a next() method? C++11 has std::next but one of the trybots failed compiling that code, std::advance works on all of them. Am I missing something obvious here? https://codereview.chromium.org/1222953002/diff/180001/components/certificate... File components/certificate_transparency/log_proof_fetcher.h (right): https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.h:35: static const size_t kMaxLogResponseSizeInBytes = 600; On 2015/08/04 19:54:29, mmenke wrote: > optional: Suggest moving this into LogProofFetcher, as a public static member - > makes it clearer where it is, though the name does get a bit long. Done. https://codereview.chromium.org/1222953002/diff/180001/components/certificate... File components/certificate_transparency/log_proof_fetcher_unittest.cc (right): https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:50: void SetAsyncIO(bool async_io) { async_io_ = async_io; } On 2015/08/04 19:54:30, mmenke wrote: > set_asnyc_io Done. https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:54: int GetResponseCode() const override { return response_code_; } On 2015/08/04 19:54:29, mmenke wrote: > By default, this will just be grabbed from the headers we set instead. Having > this not match when the headers returns seems like a bad idea, so I suggest > getting rid of it and changing the header string instead. Done. https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:57: bool NextReadAsync() override { On 2015/08/04 19:54:29, mmenke wrote: > This can be private (parent classes can access private override methods) Done. https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:61: // that the raw_read_buffer_ is not null. On 2015/08/04 19:54:30, mmenke wrote: > Why is raw read buffer NULL? The final read of a request can normally be an > asynchronous 0-byte read. > > Just want to know if there's a bug in the test fixture, in the new code, in > TestURLRequestJob, or elsewhere in net/ I'll follow up on that over email - still trying to track the call sequence that leads to that. As far as I can tell NotifyReadComplete is invoked a third time (after the initial read() invokes the delegate's callback with 0 bytes indicating async IO and the 2nd read invokes the delegate's callback with the actual data) if NextReadAsync() returns true every time. If NextReadAsync() only returns true once then NotifyReadComplete is only invoked twice. https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:71: int response_code_; On 2015/08/04 19:54:30, mmenke wrote: > member variables go at the end, after all methods, including the destructor. Done. https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:74: DISALLOW_COPY_AND_ASSIGN(FetchSTHTestJob); On 2015/08/04 19:54:30, mmenke wrote: > nit: Blank line before DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:94: void SetResponseData(std::string response_data) { On 2015/08/04 19:54:30, mmenke wrote: > set_response_data Done. https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:95: response_data_ = response_data; On 2015/08/04 19:54:30, mmenke wrote: > Suggest response_data -> response_body everywhere. response_data is indeed what > URLRequestTestJob calls it, but it's much less clear than response_body, and > should probably be renamed in the test class as well, at some point. Done. https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:98: void SetAsyncIO(bool async_io) { async_io_ = async_io; } On 2015/08/04 19:54:30, mmenke wrote: > set_asnyc_io Done. https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:100: void SetResponseCode(int response_code) { response_code_ = response_code; } On 2015/08/04 19:54:30, mmenke wrote: > set_response_code (Though, per earlier comment, I think we should get rid of > this method) Done. https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:114: virtual void STHFetched(const std::string& log_id, On 2015/08/04 19:54:30, mmenke wrote: > virtual no longer needed Done. https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:115: const net::ct::SignedTreeHead& sth) { On 2015/08/04 19:54:30, mmenke wrote: > ASSERT_FALSE(invoked_); Done. https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:120: net::ct::GetSampleSignedTreeHead(&known_sth); On 2015/08/04 19:54:30, mmenke wrote: > suggest known -> expected (More common for tests, and makes it completely clear > which is expected, and which is the you're testing) Done. https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:130: ASSERT_EQ(sth.signature.signature_data, known_sth.signature.signature_data); On 2015/08/04 19:54:30, mmenke wrote: > For all of these, expected should be first. Done. https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:135: int http_response_code) { On 2015/08/04 19:54:30, mmenke wrote: > ASSERT_FALSE(invoked_); Done. https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:145: bool invoked() { return invoked_; } On 2015/08/04 19:54:30, mmenke wrote: > bool invoked() const ... Done. https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:147: int net_error() { return net_error_; } On 2015/08/04 19:54:30, mmenke wrote: > bool net_error() const ... Done. https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:149: int http_response_code() { return http_response_code_; } On 2015/08/04 19:54:30, mmenke wrote: > const Done. https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:156: bool expect_success_; On 2015/08/04 19:54:30, mmenke wrote: > optional: Suggest making this a constructor parameter, and making it const. Done. https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:160: int http_response_code_; On 2015/08/04 19:54:30, mmenke wrote: > All of these should be initialized in the constructor. Done. https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:230: std::string("")); On 2015/08/04 19:54:30, mmenke wrote: > nit: std::string() (x2) Done.
Just small suggestions. Still concerned about why only the first read can be async, though. https://codereview.chromium.org/1222953002/diff/180001/components/certificate... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1222953002/diff/180001/components/certificate... components/certificate_transparency/log_proof_fetcher.cc:206: STLDeleteContainerPairPointers(it, next_it); On 2015/08/05 13:10:51, Eran wrote: > On 2015/08/04 19:54:29, mmenke wrote: > > Any reason not to just do: > > > > STLDeleteContainerPairPointers(it, it->next()); > > > > Think it's clearer than "next_it" > > Seems like bidirectional stl iterators don't have a next() method? > C++11 has std::next but one of the trybots failed compiling that code, > std::advance works on all of them. > Am I missing something obvious here? You're not, I am - I had thought that there was an old way to get the next value from all iterator classes without modifying them, but there isn't. https://codereview.chromium.org/1222953002/diff/200001/components/certificate... File components/certificate_transparency/log_proof_fetcher_unittest.cc (right): https://codereview.chromium.org/1222953002/diff/200001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:30: "\0"; nit: Second \0 at the end not needed (C strings are always NULL terminated, and you just need two at the end, not 3) https://codereview.chromium.org/1222953002/diff/200001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:35: "\0"; nit: Remove last \0 https://codereview.chromium.org/1222953002/diff/200001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:69: ~FetchSTHTestJob() override {} nit: Destructors should go before all other methods in their section (Except constructors, of couse) https://codereview.chromium.org/1222953002/diff/200001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:89: DVLOG(1) << "MaybeInterceptRequest, headers: " << response_headers_; Should we check that request->url() is the URL we expect? Or instead of using "AddHostnameInterceptor", use add AddUrlInterceptor, so we only catch the one URL we should be seeing (Nice thing about an EXPECT_EQ line here, though, is displays exactly what the incorrect URL was that we requested, if things are broken). Also maybe throw in some fun stuff in the original URL, if it doesn't break the expected_sth checks? "https://spam:10042/whatever/", and check that the expected path is requested? https://codereview.chromium.org/1222953002/diff/200001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:219: ASSERT_TRUE(callback.invoked()); optional: Think these can all be EXPECTs instead, particularly the ones other than callback.invoked().
Addressed all comments. Thanks for the help figuring out the issue with Async IO and URLRequestTestJob. As discussed over chat, added a TODO to convent the tests to the async case when that issue is resolved. https://codereview.chromium.org/1222953002/diff/200001/components/certificate... File components/certificate_transparency/log_proof_fetcher_unittest.cc (right): https://codereview.chromium.org/1222953002/diff/200001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:30: "\0"; On 2015/08/05 15:37:09, mmenke wrote: > nit: Second \0 at the end not needed (C strings are always NULL terminated, and > you just need two at the end, not 3) Done. https://codereview.chromium.org/1222953002/diff/200001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:35: "\0"; On 2015/08/05 15:37:09, mmenke wrote: > nit: Remove last \0 Done. https://codereview.chromium.org/1222953002/diff/200001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:69: ~FetchSTHTestJob() override {} On 2015/08/05 15:37:09, mmenke wrote: > nit: Destructors should go before all other methods in their section (Except > constructors, of couse) Done. https://codereview.chromium.org/1222953002/diff/200001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:89: DVLOG(1) << "MaybeInterceptRequest, headers: " << response_headers_; On 2015/08/05 15:37:09, mmenke wrote: > Should we check that request->url() is the URL we expect? Or instead of using > "AddHostnameInterceptor", use add AddUrlInterceptor, so we only catch the one > URL we should be seeing (Nice thing about an EXPECT_EQ line here, though, is > displays exactly what the incorrect URL was that we requested, if things are > broken). > > Also maybe throw in some fun stuff in the original URL, if it doesn't break the > expected_sth checks? "https://spam:10042/whatever/", and check that the > expected path is requested? Done - added an EXPECT_EQ check for the URL. Note that the code under test constructs the full URL (i.e. appending /ct/v1/get-sth) according to RFC6962, so checking the received URL is quite useful, but there's limited space for playing with the original URL. I did add another component to the log's path to emulate a real log's address. https://codereview.chromium.org/1222953002/diff/200001/components/certificate... components/certificate_transparency/log_proof_fetcher_unittest.cc:219: ASSERT_TRUE(callback.invoked()); On 2015/08/05 15:37:09, mmenke wrote: > optional: Think these can all be EXPECTs instead, particularly the ones other > than callback.invoked(). Done.
LGTM!
The CQ bit was checked by eranm@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1222953002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1222953002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
eranm@chromium.org changed reviewers: + caitkp@chromium.org, rsesek@chromium.org - davidben@chromium.org
Cait, kindly review this new component for supporting Certificate Transparency (http://code.google.com/p/chromium/issues/detail?id=506227 for more context). mmenke@ has reviewed for proper URL fetching. Robert - added as reviewer since I'm using SafeJson here.
Hi Eran, I'm headed out on vacation next week, and it's unlikely I'll be able to get to this review before I go. You might want to add one of the other components owners (jochen@, sdefresne@, or blundell@) if you'd like it done before then. Also note that in general, for new components, we require a public "intent to implement" or other form of communication about the component being added. If such a doc is available, please link to it in the CL description. Thanks, Cait
eranm@chromium.org changed reviewers: + jochen@chromium.org - caitkp@chromium.org
eranm@chromium.org changed reviewers: + sdefresne@chromium.org - jochen@chromium.org
Per caitpk's suggestion, switching reviewers to sdefresne. Regarding the public intent to implement document - It's on my list, I intend to post it next week. Ryan was also of the opinion that we should post about this publicly, I would like to get the document reviewed before making it public.
safe_json DEPS LGTM
I'd like to see the Intents to Implements once available, especially the answer to whether there is any plan to use this on iOS or not. Re: iOS, if you want to support this on iOS, you should not use SafeJSON directly but let the embedder select which JSON parser to use (via injection). If there is no plan to use this on iOS please explicitly mention it in the document. https://codereview.chromium.org/1222953002/diff/240001/components/certificate... File components/certificate_transparency.gypi (right): https://codereview.chromium.org/1222953002/diff/240001/components/certificate... components/certificate_transparency.gypi:17: '../net/net.gyp:net', There is an #include of url/gurl.h in components/certificate_transparency/log_proof_fetcher.cc, so I think you need the dependency on ../url/url.gyp:url_lib. https://codereview.chromium.org/1222953002/diff/240001/components/certificate... File components/certificate_transparency/BUILD.gn (left): https://codereview.chromium.org/1222953002/diff/240001/components/certificate... components/certificate_transparency/BUILD.gn:13: "//url", There is an #include of url/gurl.h in components/certificate_transparency/log_proof_fetcher.cc, so I think you need the dependency on //url. Didn't gn warn about this? https://codereview.chromium.org/1222953002/diff/240001/components/certificate... File components/certificate_transparency/BUILD.gn (right): https://codereview.chromium.org/1222953002/diff/240001/components/certificate... components/certificate_transparency/BUILD.gn:5: static_library("certificate_transparency") { Any reason for using static_library and not source_set? source_set is generally preferred to static_library (according to dpranke, source_set is what you want to use in 99% of the case). https://codereview.chromium.org/1222953002/diff/240001/components/certificate... components/certificate_transparency/BUILD.gn:16: } Can you also add unit_test target here and into //components/BUILD.gn?
On 2015/08/07 16:39:08, sdefresne wrote: > I'd like to see the Intents to Implements once available, especially the answer > to whether there is any plan to use this on iOS or not. Re: iOS, if you want to > support this on iOS, you should not use SafeJSON directly but let the embedder > select which JSON parser to use (via injection). If there is no plan to use this > on iOS please explicitly mention it in the document. > Still working on the Intent to Implement - there's some uncertainty about the exact conditions this code may be used. Either way the intention was to only deploy it for non-mobile instances of Chrome. On iOS, Apple have implemented Certificate Transparency support at the OS layer, we're still trying to find out if that can be used, but as mentioned, we didn't plan to deploy it there.
FYI still working on the public announcement. Otherwise addressed comments. https://codereview.chromium.org/1222953002/diff/240001/components/certificate... File components/certificate_transparency.gypi (right): https://codereview.chromium.org/1222953002/diff/240001/components/certificate... components/certificate_transparency.gypi:17: '../net/net.gyp:net', On 2015/08/07 16:39:08, sdefresne wrote: > There is an #include of url/gurl.h in > components/certificate_transparency/log_proof_fetcher.cc, so I think you need > the dependency on ../url/url.gyp:url_lib. Done. https://codereview.chromium.org/1222953002/diff/240001/components/certificate... File components/certificate_transparency/BUILD.gn (left): https://codereview.chromium.org/1222953002/diff/240001/components/certificate... components/certificate_transparency/BUILD.gn:13: "//url", On 2015/08/07 16:39:08, sdefresne wrote: > There is an #include of url/gurl.h in > components/certificate_transparency/log_proof_fetcher.cc, so I think you need > the dependency on //url. Didn't gn warn about this? Done - I didn't compile it locally using gn, I relied on the bots. https://codereview.chromium.org/1222953002/diff/240001/components/certificate... File components/certificate_transparency/BUILD.gn (right): https://codereview.chromium.org/1222953002/diff/240001/components/certificate... components/certificate_transparency/BUILD.gn:5: static_library("certificate_transparency") { On 2015/08/07 16:39:08, sdefresne wrote: > Any reason for using static_library and not source_set? source_set is generally > preferred to static_library (according to dpranke, source_set is what you want > to use in 99% of the case). Done - no reason not to use source_set here. https://codereview.chromium.org/1222953002/diff/240001/components/certificate... components/certificate_transparency/BUILD.gn:16: } On 2015/08/07 16:39:08, sdefresne wrote: > Can you also add unit_test target here and into //components/BUILD.gn? Done.
FYI still working on the public announcement. Otherwise addressed comments.
components lgtm
FYI we're still planning on publishing an intent to implement - I was waiting on sleevi@ for some clarifications on longer-term plans.
The CQ bit was checked by eranm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org, rsesek@chromium.org, sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/1222953002/#ps280001 (title: "Merging with master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1222953002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1222953002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
eranm@chromium.org changed reviewers: + thestig@chromium.org
Lei, kindly review for the addition of dependency on base (this is a new component hence the new dependency on base).
https://codereview.chromium.org/1222953002/diff/280001/components/certificate... File components/certificate_transparency/DEPS (right): https://codereview.chromium.org/1222953002/diff/280001/components/certificate... components/certificate_transparency/DEPS:2: "+base", You should remove this rule and the rule in the file this was copied from. Then you won't need my approval and the DEPS check will still pass because there's already a rule that says base is awesome and everyone can use base.
The CQ bit was checked by eranm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org, sdefresne@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1222953002/#ps300001 (title: "Removing explicit base dep")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1222953002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1222953002/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1222953002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1222953002/300001
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/d86db1082a7f7a90813641397b04e6c3097ab0f6 Cr-Commit-Position: refs/heads/master@{#347254} |
