Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
337093003:
QuicServer: Use Chrome's header parsing rather than Balsa (Closed)
DescriptionQuicServer: Use Chrome's header parsing rather than Balsa
It used to be that the QuicInMemoryCache mapped Balsa request headers to
Balsa responses, initializing the cache by using Balsa to parse
responses out of files in a specified directory. Then,
QuicSpdyServerStream would use Balsa to parse the incoming request
headers to pass of to the QuicInMemoryCache.
Now, instead of BalsaHeaders, I've made the QuicInMemoryCache be keyed by GURL, since it was ignoring all the other parts of the request headers anyway. Thus, QuicSpdyServerStream now constructs GURLs
by converting them from SPDY headers that are parsed out of the request,
and converts the resulting HttpResponseHeaders from the
QuicInMemoryCache into writable SPDY headers by using a new helper
function SpdyHttpUtils::CreateSpdyHeadersFromHttpResponse.
I removed the dependency on Balsa and tested the code by running the
quic_server binary.
R=wtc,rch@chromium.org
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=280901
Patch Set 1 #Patch Set 2 : (mark StringPieceUtils as copied, not added) #
Total comments: 16
Patch Set 3 : Address review comments #
Total comments: 2
Patch Set 4 : Reorder parameters in spdy_http_utils; cleanup #Patch Set 5 : Rebase onto previous CL and update GN build #Patch Set 6 : Rebase onto newest changes and restore QuicSpdyServerStream::body_ #Patch Set 7 : Lowercase SPDY header #
Total comments: 76
Patch Set 8 : Address review comments #Patch Set 9 : Small cleanup #
Total comments: 29
Patch Set 10 : Address review comments #Patch Set 11 : One missing review comment #
Total comments: 8
Patch Set 12 : Fix C++03 & 64-bit Windows build #Patch Set 13 : Fix build on many platforms #
Total comments: 4
Messages
Total messages: 27 (0 generated)
Please review https://codereview.chromium.org/340433002/ first -- this CL depends on it.
https://codereview.chromium.org/337093003/diff/20001/net/quic/http_request_li... File net/quic/http_request_line.cc (right): https://codereview.chromium.org/337093003/diff/20001/net/quic/http_request_li... net/quic/http_request_line.cc:13: HttpRequestLine::HttpRequestLine(StringPiece method, I agree that HttpRequestInfo does not add a ton of value beyond what this class provides. However, it's the "expected" class to use here. I think it would be better to use HttpRequestInfo instead of this class. (This will require adding back the GetKey() method used in the QuicInMemoryServer) https://codereview.chromium.org/337093003/diff/20001/net/quic/quic_in_memory_... File net/quic/quic_in_memory_cache.cc (right): https://codereview.chromium.org/337093003/diff/20001/net/quic/quic_in_memory_... net/quic/quic_in_memory_cache.cc:26: : response_type_(REGULAR_RESPONSE), headers_(NULL) { One per line, unless the whole thing fits on one line. That being said, does headers_ need to be explicitly initialized? https://codereview.chromium.org/337093003/diff/20001/net/spdy/spdy_http_utils.cc File net/spdy/spdy_http_utils.cc (right): https://codereview.chromium.org/337093003/diff/20001/net/spdy/spdy_http_utils... net/spdy/spdy_http_utils.cc:33: (*headers)[name] = new_value; Would this work: (*headers)[name] += "\0" + value https://codereview.chromium.org/337093003/diff/20001/net/spdy/spdy_http_utils... net/spdy/spdy_http_utils.cc:36: } newline before. Add a comment: } // namespace Ah, style guides :> https://codereview.chromium.org/337093003/diff/20001/net/spdy/spdy_http_utils.h File net/spdy/spdy_http_utils.h (right): https://codereview.chromium.org/337093003/diff/20001/net/spdy/spdy_http_utils... net/spdy/spdy_http_utils.h:31: // Create a SpdyHeaderBlock for a Spdy SYN_STREAM Frame from I suspect this is actually for a SYN_REPLY since this is a response. But same comment as below about SYN_STREAM being un-cool :> https://codereview.chromium.org/337093003/diff/20001/net/spdy/spdy_http_utils... net/spdy/spdy_http_utils.h:36: SpdyMajorVersion protocol_version); In-params should come before out-params. https://codereview.chromium.org/337093003/diff/20001/net/spdy/spdy_http_utils... net/spdy/spdy_http_utils.h:38: // Create a SpdyHeaderBlock for a Spdy SYN_STREAM Frame from SYN_STREAM is *so* SPDY/3. All the cool kids used HEADERS_WITH_PRIORITY. That being said, I might be inclined to drop the "for a Spdy ... frame" from this comment.
PTAL https://codereview.chromium.org/337093003/diff/20001/net/quic/http_request_li... File net/quic/http_request_line.cc (right): https://codereview.chromium.org/337093003/diff/20001/net/quic/http_request_li... net/quic/http_request_line.cc:13: HttpRequestLine::HttpRequestLine(StringPiece method, On 2014/06/19 00:15:44, Ryan Hamilton wrote: > I agree that HttpRequestInfo does not add a ton of value beyond what this class > provides. However, it's the "expected" class to use here. I think it would be > better to use HttpRequestInfo instead of this class. (This will require adding > back the GetKey() method used in the QuicInMemoryServer) You're right, I think I had seen only HttpRequestHeaders and not HttpRequestInfo when I made this class. Done. https://codereview.chromium.org/337093003/diff/20001/net/quic/quic_in_memory_... File net/quic/quic_in_memory_cache.cc (right): https://codereview.chromium.org/337093003/diff/20001/net/quic/quic_in_memory_... net/quic/quic_in_memory_cache.cc:26: : response_type_(REGULAR_RESPONSE), headers_(NULL) { On 2014/06/19 00:15:44, Ryan Hamilton wrote: > One per line, unless the whole thing fits on one line. That being said, does > headers_ need to be explicitly initialized? You're right, it does not. https://codereview.chromium.org/337093003/diff/20001/net/spdy/spdy_http_utils.cc File net/spdy/spdy_http_utils.cc (right): https://codereview.chromium.org/337093003/diff/20001/net/spdy/spdy_http_utils... net/spdy/spdy_http_utils.cc:33: (*headers)[name] = new_value; On 2014/06/19 00:15:44, Ryan Hamilton wrote: > Would this work: > > (*headers)[name] += "\0" + value No, but this does: (*headers)[name] += '\0' + value; Turns out the comment in the old code was a bit misleading -- it's not that += doesn't append 0's, but that "\0" is not all that distinguishable from an empty string :). https://codereview.chromium.org/337093003/diff/20001/net/spdy/spdy_http_utils... net/spdy/spdy_http_utils.cc:36: } On 2014/06/19 00:15:44, Ryan Hamilton wrote: > newline before. Add a comment: > > } // namespace > > Ah, style guides :> Done. https://codereview.chromium.org/337093003/diff/20001/net/spdy/spdy_http_utils.h File net/spdy/spdy_http_utils.h (right): https://codereview.chromium.org/337093003/diff/20001/net/spdy/spdy_http_utils... net/spdy/spdy_http_utils.h:31: // Create a SpdyHeaderBlock for a Spdy SYN_STREAM Frame from On 2014/06/19 00:15:44, Ryan Hamilton wrote: > I suspect this is actually for a SYN_REPLY since this is a response. But same > comment as below about SYN_STREAM being un-cool :> Ah, yes. https://codereview.chromium.org/337093003/diff/20001/net/spdy/spdy_http_utils... net/spdy/spdy_http_utils.h:36: SpdyMajorVersion protocol_version); On 2014/06/19 00:15:44, Ryan Hamilton wrote: > In-params should come before out-params. I was imitating CreateSpdyHeadersFromHttpRequest. Do you want me to change both? https://codereview.chromium.org/337093003/diff/20001/net/spdy/spdy_http_utils... net/spdy/spdy_http_utils.h:38: // Create a SpdyHeaderBlock for a Spdy SYN_STREAM Frame from On 2014/06/19 00:15:44, Ryan Hamilton wrote: > SYN_STREAM is *so* SPDY/3. All the cool kids used HEADERS_WITH_PRIORITY. That > being said, I might be inclined to drop the "for a Spdy ... frame" from this > comment. Haha, done.
On 2014/06/19 21:17:03, dmz wrote: > PTAL > > https://codereview.chromium.org/337093003/diff/20001/net/quic/http_request_li... > File net/quic/http_request_line.cc (right): > > https://codereview.chromium.org/337093003/diff/20001/net/quic/http_request_li... > net/quic/http_request_line.cc:13: HttpRequestLine::HttpRequestLine(StringPiece > method, > On 2014/06/19 00:15:44, Ryan Hamilton wrote: > > I agree that HttpRequestInfo does not add a ton of value beyond what this > class > > provides. However, it's the "expected" class to use here. I think it would be > > better to use HttpRequestInfo instead of this class. (This will require adding > > back the GetKey() method used in the QuicInMemoryServer) > > You're right, I think I had seen only HttpRequestHeaders and not HttpRequestInfo > when I made this class. Done. Oh, I actually forgot to explain: QuicInMemoryCache wasn't using anything but the URL in the HttpRequestInfo, so I just simplified its interface and used GURL everywhere. (It seems bad to implicitly advertise more functionality in the interface when the extra information is actually just thrown away.)
Looks good, modulo two small comments. https://codereview.chromium.org/337093003/diff/20001/net/spdy/spdy_http_utils.h File net/spdy/spdy_http_utils.h (right): https://codereview.chromium.org/337093003/diff/20001/net/spdy/spdy_http_utils... net/spdy/spdy_http_utils.h:36: SpdyMajorVersion protocol_version); On 2014/06/19 21:17:03, dmz wrote: > On 2014/06/19 00:15:44, Ryan Hamilton wrote: > > In-params should come before out-params. > > I was imitating CreateSpdyHeadersFromHttpRequest. Do you want me to change both? Doh! yeah, that'd be great, thanks. https://codereview.chromium.org/337093003/diff/40001/net/quic/quic_server_ses... File net/quic/quic_server_session.cc (right): https://codereview.chromium.org/337093003/diff/40001/net/quic/quic_server_ses... net/quic/quic_server_session.cc:21: visitor_(visitor) { I'd revert this, unless you're planning to merge this in google3.
Wan-Teh, do you want to take a look? It's not urgent since my previous CL has to be committed first anyway. https://codereview.chromium.org/337093003/diff/20001/net/spdy/spdy_http_utils.h File net/spdy/spdy_http_utils.h (right): https://codereview.chromium.org/337093003/diff/20001/net/spdy/spdy_http_utils... net/spdy/spdy_http_utils.h:36: SpdyMajorVersion protocol_version); On 2014/06/20 00:24:09, Ryan Hamilton wrote: > On 2014/06/19 21:17:03, dmz wrote: > > On 2014/06/19 00:15:44, Ryan Hamilton wrote: > > > In-params should come before out-params. > > > > I was imitating CreateSpdyHeadersFromHttpRequest. Do you want me to change > both? > > Doh! yeah, that'd be great, thanks. Done. https://codereview.chromium.org/337093003/diff/40001/net/quic/quic_server_ses... File net/quic/quic_server_session.cc (right): https://codereview.chromium.org/337093003/diff/40001/net/quic/quic_server_ses... net/quic/quic_server_session.cc:21: visitor_(visitor) { On 2014/06/20 00:24:09, Ryan Hamilton wrote: > I'd revert this, unless you're planning to merge this in google3. Done.
Patch set 7 LGTM. https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_in_memory... File net/quic/quic_in_memory_cache.cc (right): https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:38: ResponseMap::const_iterator it = responses_.find(url.spec()); The GetKey method in the original code ignores the http:// or https:// prefix of the URL. Should we preserve that behavior? https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:49: GURL url(path.as_string()); 1. IMPORTANT: |path| is missing the scheme ("http" or "https") and host. Can you find out how the GURL constructor supply them? 2. Find out whether we should always prepend "http://", as on line 149. https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:52: new HttpResponseHeaders(""); 1. Please add a TODO comment to add a default HttpResponseHeaders constructor that allows us to build HTTP response headers more elegantly. 2. Does it work to replace "" with std::string()? https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:55: body.as_string()); 1. BUG: I think this should use |version|, and NOT use |body|. This requires restoring the |version| parameter. 2. Should we just pass the status line to the HttpResponseHeaders constructor? https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:82: AddResponse(url, new HttpResponseHeaders(""), ""); 1. IMPORTANT: do we need to put new HttpResponseHeaders("") inside a scoped_refptr? 2. IMPORTANT: Is HttpResponseHeaders("") equivalent to a default BalsaHeaders object in the original code? https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:110: while (!file.empty()) { I think this should be written as a for loop: for (; !file.empty(); file = file_list.Next()) { } https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:120: int headers_end = HttpUtil::LocateEndOfHeaders(file_contents.c_str(), Nit: use data() instead of c_str() if you also supply the string length. Also fix lines 127 and 153. https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:123: LOG(DFATAL) << "Could not find end of headers in file: " << file.value(); 1. I think we should have a "continue" here. 2. Why don't you test headers_end == -1 or headers_end < 0? https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:134: if (!response_headers->GetNormalizedHeader("X-Original-Url", &base)) { I think you can replace the nested if statements with just a response_headers->GetNormalizedHeader call: string base; if (response_headers->GetNormalizedHeader("X-Original-Url", &base)) { response_headers->RemoveHeader("X-Original-Url"); // Remove the protocol so we can add it below. ... } else { base = file.AsUTF8Unsafe(); } GetNormalizedHeader returns false if the header is not found, so we don't need a separate HasHeader check. https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_in_memory... File net/quic/quic_in_memory_cache.h (right): https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.h:15: #include "url/gurl.h" A forward declaration of class GURL should be enough because this header only uses (const) references to GURL. https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.h:47: scoped_refptr<HttpResponseHeaders> headers() const { return headers_; } The headers() getter method can still return a const HttpResponseHeaders reference. Would that be better? https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_spdy_serv... File net/quic/quic_spdy_server_stream.cc (left): https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.cc:86: } IMPORTANT: Why is this code deleted? https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_spdy_serv... File net/quic/quic_spdy_server_stream.cc (right): https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.cc:43: total_bytes_processed = ParseRequestHeaders(); Check for -1 return value. https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.cc:59: SpdyHeaderBlock::iterator it = headers_.find("content-length"); Use a const_iterator. https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.cc:61: if(it != headers_.end() && Add a space after "if". https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.cc:62: (!base::StringToUint64(it->second, &content_length) || I wonder if it's better to use base::StringToSizeT instead. https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.cc:68: } Nit: to avoid nesting the if statements, it may be better to use early returns: if (!request_headers_received_) { SendErrorResponse(); // We're not done reading headers. return; } SpdyHeaderBlock::const_iterator it = headers_.find("content-length"); uint64 content_length; if (it != headers_.end() && ...) { SendErrorResponse(); // Invalid content length return; } SendResponse(); https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.cc:71: size_t QuicSpdyServerStream::ParseRequestHeaders() { IMPORTANT: since this function may return -1, the size_t return type may be inappropriate. https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.cc:74: char* data = read_buf_->StartOfBuffer(); Move line 72 after this line. https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.cc:80: request_url_ = GetUrlFromHeaderBlock(headers_, SPDY3, false); Is it OK to hardcode the version SPDY3? https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.cc:118: "content-length: 3\n"); IMPORTANT: according to the comments in http_response_headers.h, the lines in this string should be \0-terminated, and the string should end with two \0s in a row. https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.cc:131: CreateSpdyHeadersFromHttpResponse(response_headers, SPDY3, &header_block); IMPORTANT: is it OK to hardcode the version SPDY3? https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_spdy_serv... File net/quic/quic_spdy_server_stream.h (right): https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.h:12: #include "net/http/http_response_headers.h" A forward declaration of class HttpResponseHeaders should be enough (because of the change I suggest below). https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.h:50: void SendHeadersAndBody(scoped_refptr<HttpResponseHeaders> response_headers, Change this to const HttpResponseHeaders& response_headers, Passing a scoped_refptr parameter implies transferring a reference to the method. https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.h:54: GURL request_url_; Nit: move request_url_ forward, so that we don't separate headers_ and body_. https://codereview.chromium.org/337093003/diff/120001/net/quic/string_piece_u... File net/quic/string_piece_utils.h (right): https://codereview.chromium.org/337093003/diff/120001/net/quic/string_piece_u... net/quic/string_piece_utils.h:33: static bool StartsWithIgnoreCase(const base::StringPiece& text, IMPORTANT: It seems that the only thing we need from this file is StringPieceUtils::StartsWithIgnoreCase. If we don't need to share code with the internal source tree, this can be replaced by Chromium's base::StartsWith(text, starts_with, false) (declared in base/strings/string_util.h"), except that base::StartsWith() takes std::string inputs. https://codereview.chromium.org/337093003/diff/120001/net/quic/string_piece_u... net/quic/string_piece_utils.h:40: } // namespace net Add a blank line before this line. https://codereview.chromium.org/337093003/diff/120001/net/spdy/spdy_http_util... File net/spdy/spdy_http_utils.cc (right): https://codereview.chromium.org/337093003/diff/120001/net/spdy/spdy_http_util... net/spdy/spdy_http_utils.cc:24: void AddSpdyHeader(SpdyHeaderBlock* headers, Nit: |headers| is an output parameter, so it should be listed after the inputs. https://codereview.chromium.org/337093003/diff/120001/net/spdy/spdy_http_util... net/spdy/spdy_http_utils.cc:30: (*headers)[name] += '\0' + value; IMPORTANT: Is the use of '\0' correct? Are you using this form of operator+? string operator+ (char c, const string& str) https://codereview.chromium.org/337093003/diff/120001/net/spdy/spdy_http_util... net/spdy/spdy_http_utils.cc:108: std::string::iterator after_version = Use a const_iterator. https://codereview.chromium.org/337093003/diff/120001/net/spdy/spdy_http_utils.h File net/spdy/spdy_http_utils.h (right): https://codereview.chromium.org/337093003/diff/120001/net/spdy/spdy_http_util... net/spdy/spdy_http_utils.h:10: #include "net/http/http_response_headers.h" A forward declaration of class HttpResponseHeaders should be enough. See my suggestion on line 33. https://codereview.chromium.org/337093003/diff/120001/net/spdy/spdy_http_util... net/spdy/spdy_http_utils.h:32: void NET_EXPORT_PRIVATE CreateSpdyHeadersFromHttpResponse( 1. Put NET_EXPORT_PRIVATE before the return type (void). Also fix line 38. 2. Nit: it seems nicer to list the *FromHttpResponse function after the *FromHttpRequest, to match the time line (request, then response). https://codereview.chromium.org/337093003/diff/120001/net/spdy/spdy_http_util... net/spdy/spdy_http_utils.h:33: scoped_refptr<HttpResponseHeaders> response_headers, Change this to const HttpResponseHeaders& response_headers, Passing a scoped_refptr parameter implies transferring a reference to the method.
Additional review comments on patch set 7: https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_in_memory... File net/quic/quic_in_memory_cache.cc (left): https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:206: path.remove_suffix(1); Should we also remove the trailing ',' in the file path? https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_in_memory... File net/quic/quic_in_memory_cache.cc (right): https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:121: file_contents.size()); Nit: use length() instead of size(). https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:146: base = file.AsUTF8Unsafe(); 1. We should make sure this is an absolute pathname. 2. We should find out if this starts with '/', even on Windows. So the drive letter such as C: needs to be escaped. https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_spdy_serv... File net/quic/quic_spdy_server_stream.cc (right): https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.cc:33: uint32 total_bytes_processed = 0; The internal version of this code indeed has a while loop. It looks very different. https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.cc:46: data_len - total_bytes_processed); IMPORTANT: I think this code is wrong if read_buf_ already has some data when we memcpy more data into read_buf_ on line 41. In that case, ParseRequestHeaders() will process all the data, not just the |data| added in this call. So total_bytes_processed may be > data_len. So the original way of appending to data_ inside ParseRequestHeaders seems safer. https://codereview.chromium.org/337093003/diff/120001/net/spdy/spdy_http_util... File net/spdy/spdy_http_utils.cc (right): https://codereview.chromium.org/337093003/diff/120001/net/spdy/spdy_http_util... net/spdy/spdy_http_utils.cc:110: if (protocol_version < SPDY4) { The internal version tests version <= SPDY3. I think these two tests are equivalent because there is no major version between SPDY3 and SPDY4.
PTAL https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_in_memory... File net/quic/quic_in_memory_cache.cc (left): https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:206: path.remove_suffix(1); On 2014/06/25 23:17:32, wtc wrote: > > Should we also remove the trailing ',' in the file path? Oh yes. https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_in_memory... File net/quic/quic_in_memory_cache.cc (right): https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:38: ResponseMap::const_iterator it = responses_.find(url.spec()); On 2014/06/25 22:22:23, wtc wrote: > > The GetKey method in the original code ignores the http:// or https:// prefix of > the URL. Should we preserve that behavior? Done. https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:49: GURL url(path.as_string()); On 2014/06/25 22:22:23, wtc wrote: > > 1. IMPORTANT: |path| is missing the scheme ("http" or "https") and host. Can you > find out how the GURL constructor supply them? > > 2. Find out whether we should always prepend "http://", as on line 149. Done. https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:52: new HttpResponseHeaders(""); On 2014/06/25 22:22:23, wtc wrote: > > 1. Please add a TODO comment to add a default HttpResponseHeaders constructor > that allows us to build HTTP response headers more elegantly. > > 2. Does it work to replace "" with std::string()? 1. I think this is not necessary anymore. 2. I think it would (but I've also eliminated that). https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:55: body.as_string()); On 2014/06/25 22:22:24, wtc wrote: > > 1. BUG: I think this should use |version|, and NOT use |body|. This requires > restoring the |version| parameter. > > 2. Should we just pass the status line to the HttpResponseHeaders constructor? 1. Done. 2. Yes, in fact I've passed in the content-length header as well. https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:82: AddResponse(url, new HttpResponseHeaders(""), ""); On 2014/06/25 22:22:23, wtc wrote: > > 1. IMPORTANT: do we need to put new HttpResponseHeaders("") inside a > scoped_refptr? > > 2. IMPORTANT: Is HttpResponseHeaders("") equivalent to a default BalsaHeaders > object in the original code? 1. This should call the scoped_refptr constructor implicitly. 2. It doesn't really matter, since the HttpResponseHeaders that are passed in here are never used. I've changed it to NULL. https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:110: while (!file.empty()) { On 2014/06/25 22:22:23, wtc wrote: > > I think this should be written as a for loop: > > for (; !file.empty(); file = file_list.Next()) { > } Done. https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:120: int headers_end = HttpUtil::LocateEndOfHeaders(file_contents.c_str(), On 2014/06/25 22:22:24, wtc wrote: > > Nit: use data() instead of c_str() if you also supply the string length. > > Also fix lines 127 and 153. Done. https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:123: LOG(DFATAL) << "Could not find end of headers in file: " << file.value(); On 2014/06/25 22:22:23, wtc wrote: > > 1. I think we should have a "continue" here. > > 2. Why don't you test headers_end == -1 or headers_end < 0? > 1. Ah, I see, LOG(DFATAL) only FATALs in debug mode. 2. I copied this from other pieces of code that used HttpUtil::LocateEndOfHeaders -- if the headers are empty, then that also means something is wrong. The error message now makes this clear. https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:134: if (!response_headers->GetNormalizedHeader("X-Original-Url", &base)) { On 2014/06/25 22:22:23, wtc wrote: > > I think you can replace the nested if statements with just a > response_headers->GetNormalizedHeader call: > > string base; > if (response_headers->GetNormalizedHeader("X-Original-Url", &base)) { > response_headers->RemoveHeader("X-Original-Url"); > // Remove the protocol so we can add it below. > ... > } else { > base = file.AsUTF8Unsafe(); > } > > GetNormalizedHeader returns false if the header is not found, so we don't need a > separate HasHeader check. Done. https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:146: base = file.AsUTF8Unsafe(); On 2014/06/25 23:17:32, wtc wrote: > > 1. We should make sure this is an absolute pathname. > > 2. We should find out if this starts with '/', even on Windows. So the drive > letter such as C: needs to be escaped. I don't think it's supposed to be absolute. The DCHECK in the old code makes sure that the first '/' is *not* the first character. When I've tested this, it has just generated urls like "http://relative/path/to/cache/index.html". I've made an equivalent DCHECK in the new code. https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_in_memory... File net/quic/quic_in_memory_cache.h (right): https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.h:15: #include "url/gurl.h" On 2014/06/25 22:22:24, wtc wrote: > > A forward declaration of class GURL should be enough because this header only > uses (const) references to GURL. Done. https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.h:47: scoped_refptr<HttpResponseHeaders> headers() const { return headers_; } On 2014/06/25 22:22:24, wtc wrote: > > The headers() getter method can still return a const HttpResponseHeaders > reference. Would that be better? Yes, done. https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_spdy_serv... File net/quic/quic_spdy_server_stream.cc (left): https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.cc:86: } On 2014/06/25 22:22:24, wtc wrote: > > IMPORTANT: Why is this code deleted? No longer deleted. https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_spdy_serv... File net/quic/quic_spdy_server_stream.cc (right): https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.cc:43: total_bytes_processed = ParseRequestHeaders(); On 2014/06/25 22:22:24, wtc wrote: > > Check for -1 return value. Now it's void. https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.cc:46: data_len - total_bytes_processed); On 2014/06/25 23:17:32, wtc wrote: > > IMPORTANT: I think this code is wrong if read_buf_ already has some data when we > memcpy more data into read_buf_ on line 41. In that case, ParseRequestHeaders() > will process all the data, not just the |data| added in this call. So > total_bytes_processed may be > data_len. > > So the original way of appending to data_ inside ParseRequestHeaders seems > safer. Done. https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.cc:59: SpdyHeaderBlock::iterator it = headers_.find("content-length"); On 2014/06/25 22:22:24, wtc wrote: > > Use a const_iterator. Done. https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.cc:61: if(it != headers_.end() && On 2014/06/25 22:22:24, wtc wrote: > > Add a space after "if". Done. https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.cc:62: (!base::StringToUint64(it->second, &content_length) || On 2014/06/25 22:22:24, wtc wrote: > > I wonder if it's better to use base::StringToSizeT instead. Done. https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.cc:68: } On 2014/06/25 22:22:24, wtc wrote: > > Nit: to avoid nesting the if statements, it may be better to use early returns: > > if (!request_headers_received_) { > SendErrorResponse(); // We're not done reading headers. > return; > } > > SpdyHeaderBlock::const_iterator it = headers_.find("content-length"); > uint64 content_length; > if (it != headers_.end() && > ...) { > SendErrorResponse(); // Invalid content length > return; > } > > SendResponse(); Done. https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.cc:71: size_t QuicSpdyServerStream::ParseRequestHeaders() { On 2014/06/25 22:22:24, wtc wrote: > > IMPORTANT: since this function may return -1, the size_t return type may be > inappropriate. Now I've made it void since the return value wasn't used anyway. https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.cc:74: char* data = read_buf_->StartOfBuffer(); On 2014/06/25 22:22:24, wtc wrote: > > Move line 72 after this line. Done. https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.cc:80: request_url_ = GetUrlFromHeaderBlock(headers_, SPDY3, false); On 2014/06/25 22:22:24, wtc wrote: > > Is it OK to hardcode the version SPDY3? All the test code seems to standardize on SPDY3 -- I've added a constant to spdy_utils.h . https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.cc:118: "content-length: 3\n"); On 2014/06/25 22:22:24, wtc wrote: > > IMPORTANT: according to the comments in http_response_headers.h, the lines in > this string should be \0-terminated, and the string should end with two \0s in a > row. Done. https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.cc:131: CreateSpdyHeadersFromHttpResponse(response_headers, SPDY3, &header_block); On 2014/06/25 22:22:24, wtc wrote: > > IMPORTANT: is it OK to hardcode the version SPDY3? Seems to be, since it's used only used for testing purposes and done consistently. I've made a constant for it in quic/spdy_utils.h . https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_spdy_serv... File net/quic/quic_spdy_server_stream.h (right): https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.h:12: #include "net/http/http_response_headers.h" On 2014/06/25 22:22:24, wtc wrote: > > A forward declaration of class HttpResponseHeaders should be enough (because of > the change I suggest below). Done. https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.h:50: void SendHeadersAndBody(scoped_refptr<HttpResponseHeaders> response_headers, On 2014/06/25 22:22:24, wtc wrote: > > Change this to > const HttpResponseHeaders& response_headers, > > Passing a scoped_refptr parameter implies transferring a reference to the > method. Done. https://codereview.chromium.org/337093003/diff/120001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.h:54: GURL request_url_; On 2014/06/25 22:22:24, wtc wrote: > > Nit: move request_url_ forward, so that we don't separate headers_ and body_. Done. https://codereview.chromium.org/337093003/diff/120001/net/quic/string_piece_u... File net/quic/string_piece_utils.h (right): https://codereview.chromium.org/337093003/diff/120001/net/quic/string_piece_u... net/quic/string_piece_utils.h:33: static bool StartsWithIgnoreCase(const base::StringPiece& text, On 2014/06/25 22:22:25, wtc wrote: > > IMPORTANT: It seems that the only thing we need from this file is > StringPieceUtils::StartsWithIgnoreCase. > > If we don't need to share code with the internal source tree, this can be > replaced by Chromium's base::StartsWith(text, starts_with, false) (declared in > base/strings/string_util.h"), except that base::StartsWith() takes std::string > inputs. Done. https://codereview.chromium.org/337093003/diff/120001/net/spdy/spdy_http_util... File net/spdy/spdy_http_utils.cc (right): https://codereview.chromium.org/337093003/diff/120001/net/spdy/spdy_http_util... net/spdy/spdy_http_utils.cc:24: void AddSpdyHeader(SpdyHeaderBlock* headers, On 2014/06/25 22:22:25, wtc wrote: > > Nit: |headers| is an output parameter, so it should be listed after the inputs. It's really an in-out parameter that is modified -- should that actually be listed at the end? https://codereview.chromium.org/337093003/diff/120001/net/spdy/spdy_http_util... net/spdy/spdy_http_utils.cc:30: (*headers)[name] += '\0' + value; On 2014/06/25 22:22:25, wtc wrote: > > IMPORTANT: Is the use of '\0' correct? Are you using this form of operator+? > > string operator+ (char c, const string& str) Yes. I intentionally used '\0' and not "\0". https://codereview.chromium.org/337093003/diff/120001/net/spdy/spdy_http_util... net/spdy/spdy_http_utils.cc:108: std::string::iterator after_version = On 2014/06/25 22:22:25, wtc wrote: > > Use a const_iterator. Done. https://codereview.chromium.org/337093003/diff/120001/net/spdy/spdy_http_util... net/spdy/spdy_http_utils.cc:110: if (protocol_version < SPDY4) { On 2014/06/25 23:17:32, wtc wrote: > > The internal version tests version <= SPDY3. I think these two tests are > equivalent because there is no major version between SPDY3 and SPDY4. Okay. I am simply inverting SpdyHeadersToHttpResponse: if (protocol_version >= SPDY4) { version = "HTTP/1.1"; } else { it = headers.find(version_key); if (it == headers.end()) return false; version = it->second; } https://codereview.chromium.org/337093003/diff/120001/net/spdy/spdy_http_utils.h File net/spdy/spdy_http_utils.h (right): https://codereview.chromium.org/337093003/diff/120001/net/spdy/spdy_http_util... net/spdy/spdy_http_utils.h:10: #include "net/http/http_response_headers.h" On 2014/06/25 22:22:25, wtc wrote: > > A forward declaration of class HttpResponseHeaders should be enough. See my > suggestion on line 33. Done. https://codereview.chromium.org/337093003/diff/120001/net/spdy/spdy_http_util... net/spdy/spdy_http_utils.h:32: void NET_EXPORT_PRIVATE CreateSpdyHeadersFromHttpResponse( On 2014/06/25 22:22:25, wtc wrote: > > 1. Put NET_EXPORT_PRIVATE before the return type (void). > > Also fix line 38. > > 2. Nit: it seems nicer to list the *FromHttpResponse function after the > *FromHttpRequest, to match the time line (request, then response). Done. https://codereview.chromium.org/337093003/diff/120001/net/spdy/spdy_http_util... net/spdy/spdy_http_utils.h:33: scoped_refptr<HttpResponseHeaders> response_headers, On 2014/06/25 22:22:25, wtc wrote: > > Change this to > const HttpResponseHeaders& response_headers, > > Passing a scoped_refptr parameter implies transferring a reference to the > method. Done.
Patch set 9 LGTM. I didn't review quic_spdy_server_stream.cc completely. I'll do that in the next round. https://codereview.chromium.org/337093003/diff/120001/net/spdy/spdy_http_util... File net/spdy/spdy_http_utils.cc (right): https://codereview.chromium.org/337093003/diff/120001/net/spdy/spdy_http_util... net/spdy/spdy_http_utils.cc:24: void AddSpdyHeader(SpdyHeaderBlock* headers, On 2014/06/26 01:23:30, dmziegler wrote: > > It's really an in-out parameter that is modified -- should that actually be > listed at the end? Yes, I would list an in-out parameter after pure-input parameters. https://codereview.chromium.org/337093003/diff/160001/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/337093003/diff/160001/net/net.gyp#newcode1119 net/net.gyp:1119: # dependency on balsa is eliminated and the classes are actually used. If this comment is right, we should be able to merge quic_ported_server into net now, right? https://codereview.chromium.org/337093003/diff/160001/net/net.gyp#newcode1449 net/net.gyp:1449: 'target_name': 'quic_base', Do we still need the quic_base target? https://codereview.chromium.org/337093003/diff/160001/net/quic/quic_in_memory... File net/quic/quic_in_memory_cache.cc (right): https://codereview.chromium.org/337093003/diff/160001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:51: GURL url("http://" + path.as_string()); |url| doesn't have a |host| component. Is that OK? https://codereview.chromium.org/337093003/diff/160001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:70: VLOG(1) << "Adding response for: " << GetKey(url); Nit: we should store the return value of GetKey(url) in a local variable, rather than calling it three times. https://codereview.chromium.org/337093003/diff/160001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:85: AddResponse(url, NULL, string()); I seem to remember it is necessary to pass scoped_refptr<>() rather than NULL... https://codereview.chromium.org/337093003/diff/160001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:116: file = file_list.Next(); IMPORTANT: delete this line (now done by the for loop). https://codereview.chromium.org/337093003/diff/160001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:146: base = file.AsUTF8Unsafe(); If this generates a URL like "http://relative/path/to/cache/index.html", that has a syntax error -- it is missing a '/' after "http://". https://codereview.chromium.org/337093003/diff/160001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:149: if (base[base.length() - 1] == ',') { We should test !base.empty(), otherwise the release builds may do base[-1]. https://codereview.chromium.org/337093003/diff/160001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:155: VLOG(1) << "Inserting 'http://" << GetKey(url) Nit: we probably should omit "http://" in this log message. https://codereview.chromium.org/337093003/diff/160001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:171: return url.host() + url.PathForRequest(); Do we need a '/' in between? I seem to remember there is a better way to omit the scheme part of a GURL. It seems to have "Components" in the name. https://codereview.chromium.org/337093003/diff/160001/net/quic/quic_spdy_serv... File net/quic/quic_spdy_server_stream.cc (right): https://codereview.chromium.org/337093003/diff/160001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.cc:11: #include "net/quic/spdy_utils.h" Nit: put this header in the sorted order. https://codereview.chromium.org/337093003/diff/160001/net/quic/spdy_utils.h File net/quic/spdy_utils.h (right): https://codereview.chromium.org/337093003/diff/160001/net/quic/spdy_utils.h#n... net/quic/spdy_utils.h:15: const SpdyMajorVersion kDefaultSpdyVersion = SPDY3; Nit: kDefaultSpdyVersion => kDefaultSpdyMajorVersion
PTAL https://codereview.chromium.org/337093003/diff/160001/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/337093003/diff/160001/net/net.gyp#newcode1119 net/net.gyp:1119: # dependency on balsa is eliminated and the classes are actually used. On 2014/06/26 21:23:16, wtc wrote: > > If this comment is right, we should be able to merge quic_ported_server into net > now, right? It would be *possible*, but do we actually want to bloat the binary before it's necessary? https://codereview.chromium.org/337093003/diff/160001/net/net.gyp#newcode1449 net/net.gyp:1449: 'target_name': 'quic_base', On 2014/06/26 21:23:16, wtc wrote: > > Do we still need the quic_base target? For now, yes -- it still runs more unit tests than the ported code, and quic_client still depends on it. That will change soon, though. https://codereview.chromium.org/337093003/diff/160001/net/quic/quic_in_memory... File net/quic/quic_in_memory_cache.cc (right): https://codereview.chromium.org/337093003/diff/160001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:51: GURL url("http://" + path.as_string()); On 2014/06/26 21:23:17, wtc wrote: > > |url| doesn't have a |host| component. Is that OK? Yes, that would be included in the path string. https://codereview.chromium.org/337093003/diff/160001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:70: VLOG(1) << "Adding response for: " << GetKey(url); On 2014/06/26 21:23:17, wtc wrote: > > Nit: we should store the return value of GetKey(url) in a local variable, rather > than calling it three times. Done. https://codereview.chromium.org/337093003/diff/160001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:85: AddResponse(url, NULL, string()); On 2014/06/26 21:23:17, wtc wrote: > > I seem to remember it is necessary to pass scoped_refptr<>() rather than NULL... This implicitly calls the scoped_refptr<>() constructor with NULL. NULL is a valid value of a scoped_refptr. If you like, I can make this more explicit -- it's now not allowed to call .headers() on the Response object, since that would try to dereference the NULL pointer. https://codereview.chromium.org/337093003/diff/160001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:116: file = file_list.Next(); On 2014/06/26 21:23:16, wtc wrote: > > IMPORTANT: delete this line (now done by the for loop). Done. https://codereview.chromium.org/337093003/diff/160001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:146: base = file.AsUTF8Unsafe(); On 2014/06/26 21:23:17, wtc wrote: > > If this generates a URL like "http://relative/path/to/cache/index.html", that > has a syntax error -- it is missing a '/' after "http://". Actually, it's fine the way it is. "relative" just becomes the hostname. (It's perfectly valid for hostnames to not have normal TLD part like ".com" -- you just don't get hostnames like that from a public DNS server.) https://codereview.chromium.org/337093003/diff/160001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:149: if (base[base.length() - 1] == ',') { On 2014/06/26 21:23:17, wtc wrote: > > We should test !base.empty(), otherwise the release builds may do base[-1]. Since that really shouldn't happen, I've just made the DCHECK an actual check. https://codereview.chromium.org/337093003/diff/160001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:155: VLOG(1) << "Inserting 'http://" << GetKey(url) On 2014/06/26 21:23:17, wtc wrote: > > Nit: we probably should omit "http://" in this log message. Done. https://codereview.chromium.org/337093003/diff/160001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:171: return url.host() + url.PathForRequest(); On 2014/06/26 21:23:17, wtc wrote: > > Do we need a '/' in between? > > I seem to remember there is a better way to omit the scheme part of a GURL. It > seems to have "Components" in the name. No, PathForRequest() includes a slash. And I can't find a better way -- where should it be? https://codereview.chromium.org/337093003/diff/160001/net/quic/quic_spdy_serv... File net/quic/quic_spdy_server_stream.cc (right): https://codereview.chromium.org/337093003/diff/160001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.cc:11: #include "net/quic/spdy_utils.h" On 2014/06/26 21:23:17, wtc wrote: > > Nit: put this header in the sorted order. It is, actually. (I couldn't have passed presubmits otherwise.) https://codereview.chromium.org/337093003/diff/160001/net/quic/spdy_utils.h File net/quic/spdy_utils.h (right): https://codereview.chromium.org/337093003/diff/160001/net/quic/spdy_utils.h#n... net/quic/spdy_utils.h:15: const SpdyMajorVersion kDefaultSpdyVersion = SPDY3; On 2014/06/26 21:23:17, wtc wrote: > > Nit: kDefaultSpdyVersion => kDefaultSpdyMajorVersion Done.
Patch set 11 LGTM. I still need to review quic_spdy_server_stream.*, but I think it is fine for you to commit this first. https://codereview.chromium.org/337093003/diff/160001/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/337093003/diff/160001/net/net.gyp#newcode1119 net/net.gyp:1119: # dependency on balsa is eliminated and the classes are actually used. On 2014/06/27 01:11:37, dmziegler wrote: > > It would be *possible*, but do we actually want to bloat the binary before it's > necessary? The binary will only be bigger when 'net' is built as a shared library. If 'net' is built as a static library, the unused object files are discarded when the static library is linked with an executable program or shared library. https://codereview.chromium.org/337093003/diff/160001/net/quic/quic_in_memory... File net/quic/quic_in_memory_cache.cc (right): https://codereview.chromium.org/337093003/diff/160001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:171: return url.host() + url.PathForRequest(); On 2014/06/27 01:11:37, dmziegler wrote: > > No, PathForRequest() includes a slash. And I can't find a better way -- where > should it be? If url.PathForRequest() starts with a slash, then that's fine. The slash should separate the host and the path. I just checked the URI RFC: http://tools.ietf.org/html/rfc3986#section-3 The example shows that the path part includes the initial slash. So I think your code is correct.
Patch set 11 LGTM. I reviewed quic_spdy_server_stream.*. You may address these minor issues in a follow-up CL. https://codereview.chromium.org/337093003/diff/200001/net/quic/quic_spdy_serv... File net/quic/quic_spdy_server_stream.cc (right): https://codereview.chromium.org/337093003/diff/200001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.cc:38: while (read_buf_->RemainingCapacity() < (int)data_len) { Hmm... if read_buf_ has no bugs, read_buf_->RemainingCapacity() is a nonnegative 'int' value, so it's safer to cast read_buf_->RemainingCapacity() to uint32 than casting data_len to int. Actually, we should check if data_len > INT_MAX and handle that as an error, because the capacity of read_buf_ cannot be > INT_MAX. https://codereview.chromium.org/337093003/diff/200001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.cc:80: char* data = read_buf_->StartOfBuffer(); Nit: use "const char*". https://codereview.chromium.org/337093003/diff/200001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.cc:85: // the stream, then we'll error above.) Nit: change "above" to a concrete location, such as the method's name? https://codereview.chromium.org/337093003/diff/200001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.cc:135: "content-length: 3" + '\0' + '\0'); Nit: concatenating strings this way could be very inefficient if a lot of temporary strings need to be created. If this code is only used in tests, this could be an acceptable trade-off (in exchange for more readable code). Just something to keep in mind when writing C++ code -- the succinct syntax may hide a lot of function calls and copying.
https://codereview.chromium.org/337093003/diff/160001/net/quic/quic_in_memory... File net/quic/quic_in_memory_cache.cc (right): https://codereview.chromium.org/337093003/diff/160001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:85: AddResponse(url, NULL, string()); On 2014/06/27 01:11:37, dmziegler wrote: > If you like, I can make this more explicit -- > it's now not allowed to call .headers() on the Response object, since that would > try to dereference the NULL pointer. Ah, I finally realized what you meant by this. Should we change headers() to return a const, plain pointer? const HttpResponseHeaders* headers() const { return headers_.get(); } Or should we pass a pointer to an empty HttpResponseHeaders object here? scoped_refptr<HttpResponseHeaders> headers = new HttpResponseHeaders(""); AddResponse(url, headers, string());
The CQ bit was checked by dmziegler@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmziegler@chromium.org/337093003/200001
I'm going to try to commit now (although it looks like I need to fix the build on other platforms again), and I'll improve all the things you mentioned in another CL. https://codereview.chromium.org/337093003/diff/160001/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/337093003/diff/160001/net/net.gyp#newcode1119 net/net.gyp:1119: # dependency on balsa is eliminated and the classes are actually used. On 2014/06/27 21:00:38, wtc wrote: > > On 2014/06/27 01:11:37, dmziegler wrote: > > > > It would be *possible*, but do we actually want to bloat the binary before > it's > > necessary? > > The binary will only be bigger when 'net' is built as a shared library. If 'net' > is built as a static library, the unused object files are discarded when the > static library is linked with an executable program or shared library. Ah, I didn't know that. Great, I'll move it into 'net' in another CL. https://codereview.chromium.org/337093003/diff/160001/net/quic/quic_in_memory... File net/quic/quic_in_memory_cache.cc (right): https://codereview.chromium.org/337093003/diff/160001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:85: AddResponse(url, NULL, string()); On 2014/06/27 22:08:04, wtc wrote: > > On 2014/06/27 01:11:37, dmziegler wrote: > > If you like, I can make this more explicit -- > > it's now not allowed to call .headers() on the Response object, since that > would > > try to dereference the NULL pointer. > > Ah, I finally realized what you meant by this. > > Should we change headers() to return a const, plain pointer? > > const HttpResponseHeaders* headers() const { return headers_.get(); } > > Or should we pass a pointer to an empty HttpResponseHeaders object here? > > scoped_refptr<HttpResponseHeaders> headers = new HttpResponseHeaders(""); > > AddResponse(url, headers, string()); A const pointer looks like the way to go. Will do in cleanup commit. https://codereview.chromium.org/337093003/diff/200001/net/quic/quic_spdy_serv... File net/quic/quic_spdy_server_stream.cc (right): https://codereview.chromium.org/337093003/diff/200001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.cc:38: while (read_buf_->RemainingCapacity() < (int)data_len) { On 2014/06/27 21:57:13, wtc wrote: > > Hmm... if read_buf_ has no bugs, read_buf_->RemainingCapacity() is a nonnegative > 'int' value, so it's safer to cast read_buf_->RemainingCapacity() to uint32 than > casting data_len to int. > > Actually, we should check if data_len > INT_MAX and handle that as an error, > because the capacity of read_buf_ cannot be > INT_MAX. Will do in cleanup CL. https://codereview.chromium.org/337093003/diff/200001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.cc:80: char* data = read_buf_->StartOfBuffer(); On 2014/06/27 21:57:13, wtc wrote: > > Nit: use "const char*". Will do in cleanup CL. https://codereview.chromium.org/337093003/diff/200001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.cc:85: // the stream, then we'll error above.) On 2014/06/27 21:57:13, wtc wrote: > > Nit: change "above" to a concrete location, such as the method's name? Will do in cleanup CL. https://codereview.chromium.org/337093003/diff/200001/net/quic/quic_spdy_serv... net/quic/quic_spdy_server_stream.cc:135: "content-length: 3" + '\0' + '\0'); On 2014/06/27 21:57:13, wtc wrote: > > Nit: concatenating strings this way could be very inefficient if a lot of > temporary strings need to be created. > > If this code is only used in tests, this could be an acceptable trade-off (in > exchange for more readable code). Just something to keep in mind when writing > C++ code -- the succinct syntax may hide a lot of function calls and copying. Good point. Presumably C++ optimizers aren't clever enough for this code.
The CQ bit was checked by dmziegler@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmziegler@chromium.org/337093003/220001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
The CQ bit was checked by dmziegler@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmziegler@chromium.org/337093003/240001
Message was sent while issue was closed.
Change committed as 280901
Message was sent while issue was closed.
Patch set 13 LGTM. https://codereview.chromium.org/337093003/diff/240001/net/quic/quic_in_memory... File net/quic/quic_in_memory_cache.cc (right): https://codereview.chromium.org/337093003/diff/240001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:131: file_len); I think it would be better to change these HttpUtil::* functions to declare their |buf_len| parameter as size_t instead of int. This could be a lot of work though. https://codereview.chromium.org/337093003/diff/240001/net/spdy/spdy_http_util... File net/spdy/spdy_http_utils.cc (right): https://codereview.chromium.org/337093003/diff/240001/net/spdy/spdy_http_util... net/spdy/spdy_http_utils.cc:153: (*headers)[status_key] = std::string(after_version + 1, status_line.end()); Can you explain this change? Is it because cbegin and cend are not available on some platforms? (They seem to be C++11 new features.)
Message was sent while issue was closed.
https://codereview.chromium.org/337093003/diff/240001/net/quic/quic_in_memory... File net/quic/quic_in_memory_cache.cc (right): https://codereview.chromium.org/337093003/diff/240001/net/quic/quic_in_memory... net/quic/quic_in_memory_cache.cc:131: file_len); On 2014/07/08 17:54:41, wtc wrote: > > I think it would be better to change these HttpUtil::* functions to declare > their |buf_len| parameter as size_t instead of int. This could be a lot of work > though. You're right. If you want, I can make a CL for this, although the functions are used in dozens of places, so it would be quite a bit of work. https://codereview.chromium.org/337093003/diff/240001/net/spdy/spdy_http_util... File net/spdy/spdy_http_utils.cc (right): https://codereview.chromium.org/337093003/diff/240001/net/spdy/spdy_http_util... net/spdy/spdy_http_utils.cc:153: (*headers)[status_key] = std::string(after_version + 1, status_line.end()); On 2014/07/08 17:54:42, wtc wrote: > > Can you explain this change? Is it because cbegin and cend are not available on > some platforms? (They seem to be C++11 new features.) Yes, that's exactly right. I just made |status_line| const instead, which has the same effect. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
