Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11088073:
HTTP server for testing Google Drive. (Closed)
|
Created:
8 years, 2 months ago by mtomasz Modified:
8 years, 1 month ago CC:
chromium-reviews, achuith+watch_chromium.org, oshima+watch_chromium.org, stevenjb+watch_chromium.org, nkostylev+watch_chromium.org, davemoore+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionHTTP server for testing Google Drive.
BUG=148710
TEST=none; the code is only for testing and won't be used for production.
Patch Set 1 #Patch Set 2 : Added the forgotten file. #
Total comments: 33
Patch Set 3 : Addressed comments. #
Total comments: 59
Patch Set 4 : Addressed comments and simplified. Added tests. #
Total comments: 28
Patch Set 5 : Fixed for clang. #
Total comments: 87
Patch Set 6 : Addressed most of the comments. #Patch Set 7 : Fixed response generation. #
Total comments: 34
Patch Set 8 : Addressed some comments. #Patch Set 9 : Moved to chrome/browser/google_apis. #
Total comments: 10
Patch Set 10 : Addressed most comments. #
Total comments: 21
Messages
Total messages: 35 (0 generated)
Sorry for the belated response. I took a quick look and here's the initial comments. http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/test_servers/http_connection.cc (right): http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/test_servers/http_connection.cc:30: std::stringstream response_builder; like google3, we don't use streams. Please use StringAppendF() instead. http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/test_servers/http_connection.cc:52: while ((parser_state = request_parser_.ParseRequest()) == assignment in while() looks a bit tricky. maybe just a matter of taste, but the following may be good? while (true) { const HttpRequestParser::STATE parser_state = request_parser_.ParseRequest(); if (parser_state != READY) return parser_state != SYNTAX_ERROR; ... } NOTREACHED(); return false; http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/test_servers/http_connection.cc:53: HttpRequestParser::READY) { indentation is off? should look like: while ((parser_state = request_parser_.ParseRequest()) == HttpRequestParser::READY) { http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/test_servers/http_connection.h (right): http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/test_servers/http_connection.h:24: virtual bool HandleRequest(HttpConnection& connection, function comment missing. http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/test_servers/http_request.h (right): http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/test_servers/http_request.h:17: enum HTTP_METHOD { HttpMethod http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/test_servers/http_request.h:21: HEAD = 4, The following would be better: OPTIONS = 1 << 0, GET = 1 << 1, HEAD = 1 << 2, However, why these have to be 2^x? What's wrong with just 0,1,2,3,4? http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/test_servers/http_request.h:27: CUSTOM = 256 Do we need all the method types? I'd suggest to remove method types which we don't use, in favor of less code. We can add more as needed. http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/test_servers/http_request.h:64: enum STATE { Enum names should look like State http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/test_servers/http_request.h:69: SYNTAX_ERROR Please add , at the end. it makes slightly easier to add new members. http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/test_servers/http_request.h:70: }; Please add comments for each state. http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/test_servers/http_request.h:122: virtual const char* ShiftData(int length); You might want to use StringPiece instead of char*. StringPiece is simpler but very useful for things like parsing. http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/test_servers/http_server.cc (right): http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/test_servers/http_server.cc:27: base::WaitableEvent waiter(false, false); WaitableEvent should almost always be avoided. I think you can achieve the same thing by PostTaskAndReply. http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/test_servers/http_server.cc:51: void HttpServer::CreateServerOnIOThread( I guess you can make it a free function in anonymous namespace. Less things to have in .h files are usually a good thing. http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/test_servers/http_test_server.cc (right): http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/test_servers/http_test_server.cc:22: const int kPort = 8040; The port may already be in use by another test instance. We should be able to pick an unused port.
Thanks for the initial comments! http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/test_servers/http_connection.cc (right): http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/test_servers/http_connection.cc:30: std::stringstream response_builder; On 2012/10/12 08:29:18, satorux1 wrote: > like google3, we don't use streams. Please use StringAppendF() instead. Done. http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/test_servers/http_connection.cc:52: while ((parser_state = request_parser_.ParseRequest()) == On 2012/10/12 08:29:18, satorux1 wrote: > assignment in while() looks a bit tricky. maybe just a matter of taste, but the > following may be good? > > while (true) { > const HttpRequestParser::STATE parser_state = request_parser_.ParseRequest(); > if (parser_state != READY) > return parser_state != SYNTAX_ERROR; > ... > } > NOTREACHED(); > return false; > > I know that this while is non-trivial, but personally while (true) with NOTREACHED() is more tricky for me... http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/test_servers/http_connection.cc:53: HttpRequestParser::READY) { On 2012/10/12 08:29:18, satorux1 wrote: > indentation is off? should look like: > > while ((parser_state = request_parser_.ParseRequest()) == > HttpRequestParser::READY) { Here I made a 4-spaces indentation as for too long line. Do we have any rule when do 4-spaces, and when to align? http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/test_servers/http_connection.h (right): http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/test_servers/http_connection.h:24: virtual bool HandleRequest(HttpConnection& connection, On 2012/10/12 08:29:18, satorux1 wrote: > function comment missing. Done. http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/test_servers/http_request.h (right): http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/test_servers/http_request.h:17: enum HTTP_METHOD { On 2012/10/12 08:29:18, satorux1 wrote: > HttpMethod Here I am confused. On chromium website they suggest to keep MACRO_STYLE naming. "Though the Google C++ Style Guide now says to use kConstantNaming for enums, Chromium was written using MACRO_STYLE naming. Continue to use this style for consistency." http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/test_servers/http_request.h:21: HEAD = 4, On 2012/10/12 08:29:18, satorux1 wrote: > The following would be better: > > OPTIONS = 1 << 0, > GET = 1 << 1, > HEAD = 1 << 2, > > However, why these have to be 2^x? What's wrong with just 0,1,2,3,4? I wanted to use it in the future for bit sets, like GET | POST, to tell the server that some request handler should only answer to that kind of requests. However, I don't think I am going to implement it. Done. http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/test_servers/http_request.h:27: CUSTOM = 256 On 2012/10/12 08:29:18, satorux1 wrote: > Do we need all the method types? I'd suggest to remove method types which we > don't use, in favor of less code. We can add more as needed. Done. http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/test_servers/http_request.h:69: SYNTAX_ERROR On 2012/10/12 08:29:18, satorux1 wrote: > Please add , at the end. it makes slightly easier to add new members. Done. http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/test_servers/http_request.h:70: }; On 2012/10/12 08:29:18, satorux1 wrote: > Please add comments for each state. Done. http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/test_servers/http_request.h:122: virtual const char* ShiftData(int length); On 2012/10/12 08:29:18, satorux1 wrote: > You might want to use StringPiece instead of char*. StringPiece is simpler but > very useful for things like parsing. Done. http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/test_servers/http_server.cc (right): http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/test_servers/http_server.cc:27: base::WaitableEvent waiter(false, false); On 2012/10/12 08:29:18, satorux1 wrote: > WaitableEvent should almost always be avoided. > > I think you can achieve the same thing by PostTaskAndReply. Correct me if I am wrong, but I think I really need to use WaitableEvent here, since I want to block, since I want to return the created object in another thread. I don't want it async on purpose. I just want to run it synchronously in another thread. http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/test_servers/http_server.cc:51: void HttpServer::CreateServerOnIOThread( On 2012/10/12 08:29:18, satorux1 wrote: > I guess you can make it a free function in anonymous namespace. Less things to > have in .h files are usually a good thing. I wanted to keep the constructor private. Also Listen() is private in net::TCPListenSocket, so it would be very problematic to keep this outside of the class. http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/test_servers/http_test_server.cc (right): http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/test_servers/http_test_server.cc:22: const int kPort = 8040; On 2012/10/12 08:29:18, satorux1 wrote: > The port may already be in use by another test instance. We should be able to > pick an unused port. It is already implemented. The server tries succeeding ports if this one is not available.
Please write unit tests for the new code. http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/test_servers/http_connection.cc (right): http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/test_servers/http_connection.cc:52: while ((parser_state = request_parser_.ParseRequest()) == On 2012/10/12 11:09:46, mtomasz wrote: > On 2012/10/12 08:29:18, satorux1 wrote: > > assignment in while() looks a bit tricky. maybe just a matter of taste, but > the > > following may be good? > > > > while (true) { > > const HttpRequestParser::STATE parser_state = > request_parser_.ParseRequest(); > > if (parser_state != READY) > > return parser_state != SYNTAX_ERROR; > > ... > > } > > NOTREACHED(); > > return false; > > > > > > I know that this while is non-trivial, but personally while (true) with > NOTREACHED() is more tricky for me... Then that's fine. http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/test_servers/http_connection.cc:53: HttpRequestParser::READY) { On 2012/10/12 11:09:46, mtomasz wrote: > On 2012/10/12 08:29:18, satorux1 wrote: > > indentation is off? should look like: > > > > while ((parser_state = request_parser_.ParseRequest()) == > > HttpRequestParser::READY) { > > Here I made a 4-spaces indentation as for too long line. Do we have any rule > when do 4-spaces, and when to align? In conditionals, we should align vertically. http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/test_servers/http_request.h (right): http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/test_servers/http_request.h:17: enum HTTP_METHOD { On 2012/10/12 11:09:46, mtomasz wrote: > On 2012/10/12 08:29:18, satorux1 wrote: > > HttpMethod > > Here I am confused. On chromium website they suggest to keep MACRO_STYLE naming. > > "Though the Google C++ Style Guide now says to use kConstantNaming for enums, > Chromium was written using MACRO_STYLE naming. Continue to use this style for > consistency." MACRO_STYLE is for members. The enum types should be named like TypeName. http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/test_servers/http_server.cc (right): http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/test_servers/http_server.cc:27: base::WaitableEvent waiter(false, false); On 2012/10/12 11:09:46, mtomasz wrote: > On 2012/10/12 08:29:18, satorux1 wrote: > > WaitableEvent should almost always be avoided. > > > > I think you can achieve the same thing by PostTaskAndReply. > > Correct me if I am wrong, but I think I really need to use WaitableEvent here, > since I want to block, since I want to return the created object in another > thread. > > I don't want it async on purpose. I just want to run it synchronously in another > thread. I think you can make the object creation asynchronous, like: void CreateServer(..., const OnCreateCallback& callback) { and OnCreateCallback will receive the created object. http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/test_servers/http_server.cc:51: void HttpServer::CreateServerOnIOThread( On 2012/10/12 11:09:46, mtomasz wrote: > On 2012/10/12 08:29:18, satorux1 wrote: > > I guess you can make it a free function in anonymous namespace. Less things to > > have in .h files are usually a good thing. > > I wanted to keep the constructor private. Also Listen() is private in > net::TCPListenSocket, so it would be very problematic to keep this outside of > the class. Then that's fine. Thank you for the explanation. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_wapi_service_browsertest.cc (right): http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_wapi_service_browsertest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Please create a separate patch for changes in this file. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/test_servers/http_connection.cc (right): http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_connection.cc:19: delegate_(delegate) { indentation is awkward. please do this: HttpConnection::HttpConnection(net::StreamListenSocket* socket, HttpConnectionDelegate* delegate) : socket_(socket), delegate_(delegate) http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_connection.cc:35: response->content.length()); Is length() 64-bit? If so %lu is not correct. See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=64-bit... http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_connection.cc:39: for (it = response->custom_headers.begin(); Can you declare it here? for (std::map<std::string, std::string>::const_iterator it http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/test_servers/http_connection.h (right): http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_connection.h:9: #include <sstream> unnecessary? http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_connection.h:31: }; Since this delegate class only has one function, I think you can just use base::Callbackin instead in favor of less code, like: typedef base::Callback<...> OnRequestCallback; HttpConnection(net::StreamListenSocket* socket, const OnRequestCallback& on_request); http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_connection.h:48: // Accepts raw chunk of data from the client. Internally, passes it to the Can we assume that |data| and |length| given here contain the entire HTTP request? Could it be partial data? Please document. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/test_servers/http_request.cc (right): http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.cc:18: int kRequestSizeLimit = 64 * 1024 * 1024; // 64 mb. This server is used only for testing, so I think we don't need this. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.cc:44: HttpRequestParser::STATE HttpRequestParser::ParseRequest() { The state machine code looks efficient, but for the test server, we'd prefer simpler code than faster code. Would it be possible to simplify the code like: 0) assuming |buffer_| contains the entire HTTP request, 1) process the first line (method) 2) find the first blank line (the separator between the headers and the body) 3) go through headers and process each header (be wary of multi-line headers) 3.1) split key and value using SplitString(s, ':') and add it to map 4) copy the body I think we don't need a state machine if we are to go with simpler code http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/test_servers/http_request.h (right): http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.h:27: CUSTOM, What's custom? Please add a comment. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.h:32: class HttpRequest { class -> struct http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.h:40: std::string protocol; Please add some comment about "protocol".
http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/test_servers/http_connection.cc (right): http://codereview.chromium.org/11088073/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/test_servers/http_connection.cc:53: HttpRequestParser::READY) { On 2012/10/16 03:12:18, satorux1 wrote: > On 2012/10/12 11:09:46, mtomasz wrote: > > On 2012/10/12 08:29:18, satorux1 wrote: > > > indentation is off? should look like: > > > > > > while ((parser_state = request_parser_.ParseRequest()) == > > > HttpRequestParser::READY) { > > > > Here I made a 4-spaces indentation as for too long line. Do we have any rule > > when do 4-spaces, and when to align? > > In conditionals, we should align vertically. Not only in conditionals, if stuff in parens is longer than one line, we should align vertically like: SomeLooooooooooooooooooooooooooongNameFunction(12345, 67890); or SomeLooooooooooooooooooooooooooongNameFunction( 12345, 67890);
Took a close look at the parser. If I understand correctly, the state machine becomes more complex to support multi-line headers properly. That said, I think the server should just always close the connection per one request. Then we can read the entire HTTP request without parsing. That'd simplify the code a lot. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/test_servers/http_request.cc (right): http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.cc:41: FindNextCrLf(); I was confused about this. Why do we want to find CRLF here? I guess the answer is to ensure that we are ready to parse a header. However, even if we find a CRLF here, it's not yet ready to parse a header as values can be multi-line. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.cc:59: std::transform(token.begin(), token.end(), token.begin(), ::toupper); StringToUpperASCII() http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.cc:71: // TODO(mtomasz): Implement other methods. You can do NOTREACHED() << "Unsupported method: " << token http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.cc:73: } Please create a function to convert string to method type like request_builder_->method = GetMethodType(token); http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.cc:82: request_builder_->raw_url = token; raw_url seems to be a misnomer. This is path + query, right? path_and_query may be more descriptive. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.cc:103: state_ = SYNTAX_ERROR; I think you don't have to check this. We can assume our clients do this right (i.e. we are not interested in testing the low-level networking stack). http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.cc:108: GURL host = GURL("http://" + request_builder_->headers["host"]); Let's just use http://localhost for now http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.cc:119: state_ = READY; There is something called "chunked encoding" but we don't need to support it. Please add some comment saying // Chunked encoding is not supported. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.cc:125: std::string header_name = ShiftToken(HEADER_KEY).as_string(); This assumes that |buffer_| contains the key name entirely. Would it be possible that |buffer_| ends in the middle of a header key? http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.cc:129: ::tolower); StringToLowerASCII http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.cc:137: StringPiece header_value = ShiftToken(LINE); I think the multi-line values are not supported yet, right? multi-line headers are fairly common so not supporting it is bad. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.cc:150: buffer_position_) { I'm confused. What does the following value mean? request_builder_->content.length() - buffer_position_ I thought you wanted to compute the remaining data in buffer_. If so, don't you want to do this instead: const int available_bytes = buffer_.size() - buffer_position_; to_read = std::min(to_read, available_bytes); If you switch from 'int' to StringPiece, the code can be as simple as: to_read = std::min(to_read, buffer_position_.size()); http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.cc:179: current_content_length_ = 0; Please add Reset() function that resets everything. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.cc:186: size_t found_position; please remove this here. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.cc:191: found_position = buffer_.find(' ', buffer_position_); const size_t found_position = .. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.cc:209: int token_length = crlf_position_ - token_start_position; I'm confused about this. Why don't we find the CRLF here? If we don't need to special case the LINE, we can generalize this function like const StringPiece HttpRequestParser::ConsumeUntil(const std::string& separator) and the client code will look like std::string header_name = ConsumeUntil(":").as_string(); http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/test_servers/http_request.h (right): http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.h:102: scoped_ptr<HttpRequest> request_builder_; request_builder_ -> http_request_ http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.h:105: int buffer_position_; This is an index in |buffer_|, right? I think this is tricky. StringPiece can be also useful for cursors like this.
On 2012/10/16 06:39:48, satorux1 wrote: > Took a close look at the parser. If I understand correctly, the state machine > becomes more complex to support multi-line headers properly. > > That said, I think the server should just always close the connection per one > request. Then we can read the entire HTTP request without parsing. That'd > simplify the code a lot. I was wrong. The server needs to parse the Content-Length header to detect the end of the body, hence parsing is needed. I think what we should do is to first detect the first blank line (the separator between the headers and the body), then parse headers (incl. Content-Length), and then read the body. I guess we don't need to close the connection. > > http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... > File chrome/browser/chromeos/gdata/test_servers/http_request.cc (right): > > http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... > chrome/browser/chromeos/gdata/test_servers/http_request.cc:41: FindNextCrLf(); > I was confused about this. Why do we want to find CRLF here? > > I guess the answer is to ensure that we are ready to parse a header. > > However, even if we find a CRLF here, it's not yet ready to parse a header as > values can be multi-line. > > http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... > chrome/browser/chromeos/gdata/test_servers/http_request.cc:59: > std::transform(token.begin(), token.end(), token.begin(), ::toupper); > StringToUpperASCII() > > http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... > chrome/browser/chromeos/gdata/test_servers/http_request.cc:71: // TODO(mtomasz): > Implement other methods. > You can do > > NOTREACHED() << "Unsupported method: " << token > > http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... > chrome/browser/chromeos/gdata/test_servers/http_request.cc:73: } > Please create a function to convert string to method type like > > request_builder_->method = GetMethodType(token); > > http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... > chrome/browser/chromeos/gdata/test_servers/http_request.cc:82: > request_builder_->raw_url = token; > raw_url seems to be a misnomer. This is path + query, right? path_and_query may > be more descriptive. > > http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... > chrome/browser/chromeos/gdata/test_servers/http_request.cc:103: state_ = > SYNTAX_ERROR; > I think you don't have to check this. We can assume our clients do this right > (i.e. we are not interested in testing the low-level networking stack). > > http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... > chrome/browser/chromeos/gdata/test_servers/http_request.cc:108: GURL host = > GURL("http://" + request_builder_->headers["host"]); > Let's just use http://localhost for now > > http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... > chrome/browser/chromeos/gdata/test_servers/http_request.cc:119: state_ = READY; > There is something called "chunked encoding" but we don't need to support it. > Please add some comment saying > > // Chunked encoding is not supported. > > http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... > chrome/browser/chromeos/gdata/test_servers/http_request.cc:125: std::string > header_name = ShiftToken(HEADER_KEY).as_string(); > This assumes that |buffer_| contains the key name entirely. > > Would it be possible that |buffer_| ends in the middle of a header key? > > http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... > chrome/browser/chromeos/gdata/test_servers/http_request.cc:129: ::tolower); > StringToLowerASCII > > http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... > chrome/browser/chromeos/gdata/test_servers/http_request.cc:137: StringPiece > header_value = ShiftToken(LINE); > I think the multi-line values are not supported yet, right? > > multi-line headers are fairly common so not supporting it is bad. > > http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... > chrome/browser/chromeos/gdata/test_servers/http_request.cc:150: > buffer_position_) { > I'm confused. What does the following value mean? > > request_builder_->content.length() - buffer_position_ > > > I thought you wanted to compute the remaining data in buffer_. If so, don't you > want to do this instead: > > const int available_bytes = buffer_.size() - buffer_position_; > to_read = std::min(to_read, available_bytes); > > If you switch from 'int' to StringPiece, the code can be as simple as: > > to_read = std::min(to_read, buffer_position_.size()); > > http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... > chrome/browser/chromeos/gdata/test_servers/http_request.cc:179: > current_content_length_ = 0; > Please add Reset() function that resets everything. > > http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... > chrome/browser/chromeos/gdata/test_servers/http_request.cc:186: size_t > found_position; > please remove this here. > > http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... > chrome/browser/chromeos/gdata/test_servers/http_request.cc:191: found_position = > buffer_.find(' ', buffer_position_); > const size_t found_position = .. > > http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... > chrome/browser/chromeos/gdata/test_servers/http_request.cc:209: int token_length > = crlf_position_ - token_start_position; > I'm confused about this. Why don't we find the CRLF here? > > If we don't need to special case the LINE, we can generalize this function like > > const StringPiece HttpRequestParser::ConsumeUntil(const std::string& separator) > > and the client code will look like > > std::string header_name = ConsumeUntil(":").as_string(); > > http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... > File chrome/browser/chromeos/gdata/test_servers/http_request.h (right): > > http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... > chrome/browser/chromeos/gdata/test_servers/http_request.h:102: > scoped_ptr<HttpRequest> request_builder_; > request_builder_ -> http_request_ > > http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... > chrome/browser/chromeos/gdata/test_servers/http_request.h:105: int > buffer_position_; > This is an index in |buffer_|, right? I think this is tricky. > > StringPiece can be also useful for cursors like this.
http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/test_servers/http_connection.cc (right): http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_connection.cc:19: delegate_(delegate) { On 2012/10/16 03:12:18, satorux1 wrote: > indentation is awkward. please do this: > > HttpConnection::HttpConnection(net::StreamListenSocket* socket, > HttpConnectionDelegate* delegate) > : socket_(socket), > delegate_(delegate) Done. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_connection.cc:35: response->content.length()); On 2012/10/16 03:12:18, satorux1 wrote: > Is length() 64-bit? If so %lu is not correct. See > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=64-bit... Done. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_connection.cc:39: for (it = response->custom_headers.begin(); On 2012/10/16 03:12:18, satorux1 wrote: > Can you declare it here? > > for (std::map<std::string, std::string>::const_iterator it Done. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/test_servers/http_connection.h (right): http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_connection.h:9: #include <sstream> On 2012/10/16 03:12:18, satorux1 wrote: > unnecessary? Done. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_connection.h:31: }; On 2012/10/16 03:12:18, satorux1 wrote: > Since this delegate class only has one function, I think you can just use > base::Callbackin instead in favor of less code, like: > > typedef base::Callback<...> OnRequestCallback; > HttpConnection(net::StreamListenSocket* socket, > const OnRequestCallback& on_request); Done. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_connection.h:48: // Accepts raw chunk of data from the client. Internally, passes it to the On 2012/10/16 03:12:18, satorux1 wrote: > Can we assume that |data| and |length| given here contain the entire HTTP > request? Could it be partial data? Please document. We can't assume that. It can be any piece of the request. Especially for big requests, eg. uploading a file. Also, we can get two requests at once. It can be any chunk. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/test_servers/http_request.cc (right): http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.cc:18: int kRequestSizeLimit = 64 * 1024 * 1024; // 64 mb. On 2012/10/16 03:12:18, satorux1 wrote: > This server is used only for testing, so I think we don't need this. Discussed offline, leaving as it is. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.cc:41: FindNextCrLf(); On 2012/10/16 06:39:48, satorux1 wrote: > I was confused about this. Why do we want to find CRLF here? > > I guess the answer is to ensure that we are ready to parse a header. > > However, even if we find a CRLF here, it's not yet ready to parse a header as > values can be multi-line. You were right. Parser rewritten and simplified. Done. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.cc:44: HttpRequestParser::STATE HttpRequestParser::ParseRequest() { On 2012/10/16 03:12:18, satorux1 wrote: > The state machine code looks efficient, but for the test server, we'd prefer > simpler code than faster code. Would it be possible to simplify the code like: > > 0) assuming |buffer_| contains the entire HTTP request, > 1) process the first line (method) > 2) find the first blank line (the separator between the headers and the body) > 3) go through headers and process each header (be wary of multi-line headers) > 3.1) split key and value using SplitString(s, ':') and add it to map > 4) copy the body > > I think we don't need a state machine if we are to go with simpler code Done. Please check the new parser. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.cc:59: std::transform(token.begin(), token.end(), token.begin(), ::toupper); On 2012/10/16 06:39:48, satorux1 wrote: > StringToUpperASCII() Done. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.cc:71: // TODO(mtomasz): Implement other methods. On 2012/10/16 06:39:48, satorux1 wrote: > You can do > > NOTREACHED() << "Unsupported method: " << token Done. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.cc:73: } On 2012/10/16 06:39:48, satorux1 wrote: > Please create a function to convert string to method type like > > request_builder_->method = GetMethodType(token); Done. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.cc:82: request_builder_->raw_url = token; On 2012/10/16 06:39:48, satorux1 wrote: > raw_url seems to be a misnomer. This is path + query, right? path_and_query may > be more descriptive. Path_and_query looks to be quite long. How about just uri? http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.cc:103: state_ = SYNTAX_ERROR; On 2012/10/16 06:39:48, satorux1 wrote: > I think you don't have to check this. We can assume our clients do this right > (i.e. we are not interested in testing the low-level networking stack). Done. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.cc:108: GURL host = GURL("http://" + request_builder_->headers["host"]); On 2012/10/16 06:39:48, satorux1 wrote: > Let's just use http://localhost for now Done. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.cc:119: state_ = READY; On 2012/10/16 06:39:48, satorux1 wrote: > There is something called "chunked encoding" but we don't need to support it. > Please add some comment saying > > // Chunked encoding is not supported. Done in the header file. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.cc:125: std::string header_name = ShiftToken(HEADER_KEY).as_string(); On 2012/10/16 06:39:48, satorux1 wrote: > This assumes that |buffer_| contains the key name entirely. > > Would it be possible that |buffer_| ends in the middle of a header key? I don't think so. We were parsing headers line by line. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.cc:129: ::tolower); On 2012/10/16 06:39:48, satorux1 wrote: > StringToLowerASCII Done. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.cc:137: StringPiece header_value = ShiftToken(LINE); On 2012/10/16 06:39:48, satorux1 wrote: > I think the multi-line values are not supported yet, right? > > multi-line headers are fairly common so not supporting it is bad. Rewritten parser. Not it is supported. Done. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.cc:150: buffer_position_) { On 2012/10/16 06:39:48, satorux1 wrote: > I'm confused. What does the following value mean? > > request_builder_->content.length() - buffer_position_ > > > I thought you wanted to compute the remaining data in buffer_. If so, don't you > want to do this instead: > > const int available_bytes = buffer_.size() - buffer_position_; > to_read = std::min(to_read, available_bytes); > > If you switch from 'int' to StringPiece, the code can be as simple as: > > to_read = std::min(to_read, buffer_position_.size()); You were right. Removed. Done. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.cc:179: current_content_length_ = 0; On 2012/10/16 06:39:48, satorux1 wrote: > Please add Reset() function that resets everything. Do we need it? Now there is less stuff there. Also, such Reset method would never be called in any other situation. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.cc:186: size_t found_position; On 2012/10/16 06:39:48, satorux1 wrote: > please remove this here. Done. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.cc:191: found_position = buffer_.find(' ', buffer_position_); On 2012/10/16 06:39:48, satorux1 wrote: > const size_t found_position = .. Done. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.cc:209: int token_length = crlf_position_ - token_start_position; On 2012/10/16 06:39:48, satorux1 wrote: > I'm confused about this. Why don't we find the CRLF here? > > If we don't need to special case the LINE, we can generalize this function like > > const StringPiece HttpRequestParser::ConsumeUntil(const std::string& separator) > > and the client code will look like > > std::string header_name = ConsumeUntil(":").as_string(); This was to keep the parser's complexity O(n). Now it is O(n^2) without such tricks. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/test_servers/http_request.h (right): http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.h:27: CUSTOM, On 2012/10/16 03:12:18, satorux1 wrote: > What's custom? Please add a comment. Removed. Done. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.h:32: class HttpRequest { On 2012/10/16 03:12:18, satorux1 wrote: > class -> struct Done. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.h:40: std::string protocol; On 2012/10/16 03:12:18, satorux1 wrote: > Please add some comment about "protocol". eg. HTTP/1.1. Removed, since we support that only one protocol. Done. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.h:102: scoped_ptr<HttpRequest> request_builder_; On 2012/10/16 06:39:48, satorux1 wrote: > request_builder_ -> http_request_ Done. http://codereview.chromium.org/11088073/diff/11001/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/test_servers/http_request.h:105: int buffer_position_; On 2012/10/16 06:39:48, satorux1 wrote: > This is an index in |buffer_|, right? I think this is tricky. > > StringPiece can be also useful for cursors like this. StringPiece is not perfect here I think. I would have to convert to std::string very often. I think that in this new simplified code it is not so tricky anymore. Please check again. http://codereview.chromium.org/11088073/diff/33001/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_test_server_unittest.cc (right): http://codereview.chromium.org/11088073/diff/33001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_test_server_unittest.cc:146: GURL url1 = server_->RegisterTextResponse("Raspberry chocolate", If this dummy response text is inappropriate, I'll change it.
@satorux: I've rewritten the parser and also simplified the code a lot. Also added some unit tests. PTAL.
The parser looks a lot simpler. I'd still suggest using StringPiece. Please see below. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_connection.h (right): http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_connection.h:34: virtual void SendResponse(scoped_ptr<HttpResponse> response) const; Should this be a virtual function? There are no classes inheriting HttpConnection class in this patch. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_connection.h:41: // called. ditto. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_connection.h:42: virtual void ReceiveData(const char* data, size_t length); Can we use StringPiece instead? One less parameter would be good. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_request.cc (right): http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:32: current_content_length_(0) { Maybe: HttpRequestParser::HttpRequestParser() : http_request_(new HttpRequest()), .... { } http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:39: buffer_.append(data, length); If you use StringPiece for buffer_position_, it should also be updated here: buffer_position_.set(buffer_position_.data(), buffer_position_.size() + length, http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:40: DCHECK(buffer_.length() + length <= kRequestSizeLimit) << DCHECK_LE http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:51: return result; I think it'd be simpler to use StringPiece for buffer_position_. The code will look like: size_t line_length = buffer_position_.find("\r\n"); if (line_length == std::string::npos) return ""; std::string result = buffer_position_.substr(line_length).as_string(); buffer_position_.remove_prefix(line_length + 2_; return result; http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:55: DCHECK(state_ != STATE_ACCEPTED); DCHECK_NE http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:70: // Check if the whole request input is available. request input is -> request headers are http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:71: if (buffer_.find("\r\n\r\n", buffer_position_) == std::string::npos) if you use StringPiece: buffer_position_.find("\r\n\r\n") http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:79: Tokenize(header_line, " ", &header_line_tokens); Can we use SplitString() instead? SplitString() is more commonly used, and the name is more obvious about what it does. :) http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:97: while (header_line != "") { !header_line.empty() http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:98: if (header_line[0] == ' ') { || header_line[0] == '\t' http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:100: http_request_->headers[header_name] += "\n" + IIRC, I think white spaces should be interpreted as one space, http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:101: header_line.substr(1, header_line.length() - 1); Please use size(). I think size() is more widely used in Chrome code base. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:117: header_line = ShiftLine(); Maybe the loop is easier to read with: while (true) { header_line = ShiftLine(); if (header_line.empty()) break; ... } so ShiftLine is called only at one place. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:122: if (http_request_->headers.find("Content-Length") != more like a matter of taste, but the following is shorter: http_request_->headers.count("Content-Length") > 0 http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:125: atoi(http_request_->headers["Content-Length"].c_str()); Please use StringToSizeT in base/string_number_conversions.h http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:126: if (!current_content_length_) current_content_length_ == 0 http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:128: } If Content-Length: is not present, I think we can assume that the content data is not present, hence: } else { return ACCEPTED; } http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:138: current_content_length_); Are you trying to limit the size of |http_request_->content| to be at most |current_content_length_| bytes? If so, did you mean: std::min(available_bytes, current_content_length - http_request_->content.size()) However, I don't know if we want to handle this case. Maybe a DCHECK would suffice: http_request_->content.append(...); DCHECK_LE(http_request_->content.size(), current_content_length); http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:141: buffer_position_ += fetch_bytes; If you use StringPiece for buffer_position_, the code will look like: http_request_->content.append(buffer_position_.data(), buffer_position_.size()); DCHECK_LE(http_request_->content.size(), current_content_length); buffer_position_.clear(); http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:158: buffer_ = buffer_.substr(buffer_position_, Why not just buffer_.clear()? At this moment, All the content in buffer_ should be consumed, right? If so, we can add a DCHECK like: DCHECK_EQ(buffer_.size(), buffer_position_); or DCHECK(buffer_position_.empty()) if you use StringPiece. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_request.h (right): http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.h:29: // Wraps the HTTP request. Since it can be big, use scoped_ptr to pass it Wraps -> Represents ? http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.h:54: class HttpRequestParser { I think we can write a unit test for this class. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.h:73: virtual void ProcessChunk(const char *data, size_t length); Can we use StringPiece instead? One less parameter would be good. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.h:79: virtual ParseResult ParseRequest(); should this be virtual? Are there any classes inheriting this class? http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.h:91: size_t current_content_length_; This is the content length declared in the Content-Length: header, right? Then, "declared_content_length" may be a better name? http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.h:103: std::string ShiftLine(); Function comment is missing. Does the returned string includes CRLF? When it returns an empty string? Please document. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_response.h (right): http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_response.h:22: // Wraps the HTTP response. Since it can be big, it may be better to use Wraps -> Represents ? http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_response.h:24: class HttpResponse { class -> struct http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_response.h:35: virtual std::string ToResponseString() const; why virtual?
http://codereview.chromium.org/11088073/diff/33001/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_test_server.cc (right): http://codereview.chromium.org/11088073/diff/33001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_test_server.cc:65: MessageLoop::current()->Run(); Can you do this instead? content::RunAllPendingInMessageLoop(BriwserThread::IO); MessageLoop::current()->RunAllPending(); Then you don't need to call Quit() in the callback. http://codereview.chromium.org/11088073/diff/33001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_test_server.cc:77: while (retries_left) { retries_left > 0 http://codereview.chromium.org/11088073/diff/33001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_test_server.cc:101: weak_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); http://codereview.chromium.org/11088073/diff/33001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_test_server.cc:106: HttpTestServer::~HttpTestServer() { on what thread this has to be deleted? I guess it's IO: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); http://codereview.chromium.org/11088073/diff/33001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_test_server.cc:117: while (current_handler != request_handlers_.end()) { Why not a for loop? for (size_t i = 0; i < request_handlers_.size(); ++i) { http://codereview.chromium.org/11088073/diff/33001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_test_server.cc:167: GURL HttpTestServer::RegisterTextResponse( Function overloading like this is not allowed per the style guide. Please give it a different name. http://codereview.chromium.org/11088073/diff/33001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_test_server.cc:192: GURL HttpTestServer::RegisterFileResponse( ditto. http://codereview.chromium.org/11088073/diff/33001/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_test_server.h (right): http://codereview.chromium.org/11088073/diff/33001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_test_server.h:48: // hello_world_url = test_server->RegisterTextResponse("<b>Hello world!</b>", So the URL is automatically generated? I thought it'd be more flexible to specify a path: test_server->RegisterTextResponse("/hello", ...); hello_world_url_ = test_server->GetURL("/hello");
http://codereview.chromium.org/11088073/diff/33001/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_test_server.h (right): http://codereview.chromium.org/11088073/diff/33001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_test_server.h:56: class HttpTestServer : private net::StreamListenSocket::Delegate { TestHttpServer? http://codereview.chromium.org/11088073/diff/33001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_test_server.h:66: static scoped_ptr<HttpTestServer> CreateForTesting(); CreateForTesting() sounded a bit awkward as the server is only used for testing. Instead of having the factory method, the following may be clearer: TEST_F(..., ...) { HttpTestServer server; ASSERT_TRUE(server.StartAndBlockUntilReady()); ... } http://codereview.chromium.org/11088073/diff/33001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_test_server.h:71: static void CreateOnIOThread(const CreateCallback& callback); Move it anonymous namespace in .cc file? The function doesn't seem to be called from other files. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_test_server_unittest.cc (right): http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_test_server_unittest.cc:143: DCHECK(server_.get()); ASSERT_TRUE()
http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_test_server_unittest.cc (right): http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_test_server_unittest.cc:64: TEST_F(HttpTestServerTest, ParseRequest) { I think the paser should be tested in http_request_unittest.cc http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_test_server_unittest.cc:117: TEST_F(HttpTestServerTest, GenerateResponse) { Should test in http_response_unittest.cc http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_test_server_unittest.cc:142: HttpTestServer::CreateForTesting(); I think we should do this in SetUp(). This is easily if the tests above are moved to separate files.
Thanks for the review. I've addressed most of the comments. I'd like to discuss the rest tomorrow. http://codereview.chromium.org/11088073/diff/33001/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_test_server.cc (right): http://codereview.chromium.org/11088073/diff/33001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_test_server.cc:77: while (retries_left) { On 2012/11/12 06:34:15, satorux1 wrote: > retries_left > 0 Done. http://codereview.chromium.org/11088073/diff/33001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_test_server.cc:117: while (current_handler != request_handlers_.end()) { On 2012/11/12 06:34:15, satorux1 wrote: > Why not a for loop? > > for (size_t i = 0; i < request_handlers_.size(); ++i) { Done. Do we in general prefer simple loops over iterators? http://codereview.chromium.org/11088073/diff/33001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_test_server.cc:167: GURL HttpTestServer::RegisterTextResponse( On 2012/11/12 06:34:15, satorux1 wrote: > Function overloading like this is not allowed per the style guide. Please give > it a different name. Done. http://codereview.chromium.org/11088073/diff/33001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_test_server.cc:192: GURL HttpTestServer::RegisterFileResponse( On 2012/11/12 06:34:15, satorux1 wrote: > ditto. Done. http://codereview.chromium.org/11088073/diff/33001/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_test_server.h (right): http://codereview.chromium.org/11088073/diff/33001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_test_server.h:48: // hello_world_url = test_server->RegisterTextResponse("<b>Hello world!</b>", On 2012/11/12 06:34:15, satorux1 wrote: > So the URL is automatically generated? I thought it'd be more flexible to > specify a path: > > test_server->RegisterTextResponse("/hello", ...); > hello_world_url_ = test_server->GetURL("/hello"); It can be done either way. In line #50, there is a request with passed path (not auto-generated). I think that in most cases we do not care about specific url address. http://codereview.chromium.org/11088073/diff/33001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_test_server.h:56: class HttpTestServer : private net::StreamListenSocket::Delegate { On 2012/11/12 06:43:47, satorux1 wrote: > TestHttpServer? How about just HttpServer? It is already in test_servers namespace. http://codereview.chromium.org/11088073/diff/33001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_test_server.h:71: static void CreateOnIOThread(const CreateCallback& callback); On 2012/11/12 06:43:47, satorux1 wrote: > Move it anonymous namespace in .cc file? The function doesn't seem to be called > from other files. The constructor is private, so we need it here. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_connection.h (right): http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_connection.h:34: virtual void SendResponse(scoped_ptr<HttpResponse> response) const; On 2012/11/12 06:07:00, satorux1 wrote: > Should this be a virtual function? There are no classes inheriting > HttpConnection class in this patch. Why don't we make public methods virtual by default for classes which could be overriden in the future (eg. for mocks)? Done. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_connection.h:41: // called. On 2012/11/12 06:07:00, satorux1 wrote: > ditto. Done. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_connection.h:42: virtual void ReceiveData(const char* data, size_t length); On 2012/11/12 06:07:00, satorux1 wrote: > Can we use StringPiece instead? One less parameter would be good. How about just std::string since we don't need any internal cursors? http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_request.cc (right): http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:32: current_content_length_(0) { On 2012/11/12 06:07:00, satorux1 wrote: > Maybe: > > HttpRequestParser::HttpRequestParser() > : http_request_(new HttpRequest()), > .... { > } Done. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:39: buffer_.append(data, length); On 2012/11/12 06:07:00, satorux1 wrote: > If you use StringPiece for buffer_position_, it should also be updated here: > > buffer_position_.set(buffer_position_.data(), > buffer_position_.size() + length, > I am not sure if I understand correctly. So, we will have two variables: std::string buffer_ and StringPiece buffer_position_ (an iterator on buffer_)? Isn't it then bug prone? Eg. when we do buffer_ = "", then working on buffer_position_ would sigsegv. Also, after appending to buffer_, its internal array may be reallocated causing a sigsegv in buffer_position_. Please correct me if I am wrong. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:40: DCHECK(buffer_.length() + length <= kRequestSizeLimit) << On 2012/11/12 06:07:00, satorux1 wrote: > DCHECK_LE Why is it better? Done. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:51: return result; On 2012/11/12 06:07:00, satorux1 wrote: > I think it'd be simpler to use StringPiece for buffer_position_. The code will > look like: > > size_t line_length = buffer_position_.find("\r\n"); > if (line_length == std::string::npos) > return ""; > std::string result = buffer_position_.substr(line_length).as_string(); > buffer_position_.remove_prefix(line_length + 2_; > return result; See above. This would be great if we could easily append. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:55: DCHECK(state_ != STATE_ACCEPTED); On 2012/11/12 06:07:00, satorux1 wrote: > DCHECK_NE Done. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:70: // Check if the whole request input is available. On 2012/11/12 06:07:00, satorux1 wrote: > request input is -> request headers are Done. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:71: if (buffer_.find("\r\n\r\n", buffer_position_) == std::string::npos) On 2012/11/12 06:07:00, satorux1 wrote: > if you use StringPiece: buffer_position_.find("\r\n\r\n") Ack. Let's discuss offline. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:79: Tokenize(header_line, " ", &header_line_tokens); On 2012/11/12 06:07:00, satorux1 wrote: > Can we use SplitString() instead? SplitString() is more commonly used, and the > name is more obvious about what it does. :) Done! http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:97: while (header_line != "") { On 2012/11/12 06:07:00, satorux1 wrote: > !header_line.empty() Done. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:98: if (header_line[0] == ' ') { On 2012/11/12 06:07:00, satorux1 wrote: > || header_line[0] == '\t' Done. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:100: http_request_->headers[header_name] += "\n" + On 2012/11/12 06:07:00, satorux1 wrote: > IIRC, I think white spaces should be interpreted as one space, You're right. Done. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:101: header_line.substr(1, header_line.length() - 1); On 2012/11/12 06:07:00, satorux1 wrote: > Please use size(). I think size() is more widely used in Chrome code base. Done. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:117: header_line = ShiftLine(); On 2012/11/12 06:07:00, satorux1 wrote: > Maybe the loop is easier to read with: > > while (true) { > header_line = ShiftLine(); > if (header_line.empty()) > break; > ... > } > > so ShiftLine is called only at one place. Done. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:122: if (http_request_->headers.find("Content-Length") != On 2012/11/12 06:07:00, satorux1 wrote: > more like a matter of taste, but the following is shorter: > > http_request_->headers.count("Content-Length") > 0 Done. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:125: atoi(http_request_->headers["Content-Length"].c_str()); On 2012/11/12 06:07:00, satorux1 wrote: > Please use StringToSizeT in base/string_number_conversions.h Done. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:126: if (!current_content_length_) On 2012/11/12 06:07:00, satorux1 wrote: > current_content_length_ == 0 Done. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:128: } On 2012/11/12 06:07:00, satorux1 wrote: > If Content-Length: is not present, I think we can assume that the content data > is not present, hence: > > } else { > return ACCEPTED; > } Right! Done. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:138: current_content_length_); On 2012/11/12 06:07:00, satorux1 wrote: > Are you trying to limit the size of |http_request_->content| to be at most > |current_content_length_| bytes? If so, did you mean: > > std::min(available_bytes, > current_content_length - http_request_->content.size()) > > However, I don't know if we want to handle this case. Maybe a DCHECK would > suffice: > > http_request_->content.append(...); > DCHECK_LE(http_request_->content.size(), current_content_length); Fixed std::min(). Done. As for the DCHECK, I think it may not work since it is quite possible that we get two requests in one chunk. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:141: buffer_position_ += fetch_bytes; On 2012/11/12 06:07:00, satorux1 wrote: > If you use StringPiece for buffer_position_, the code will look like: > > http_request_->content.append(buffer_position_.data(), > buffer_position_.size()); > DCHECK_LE(http_request_->content.size(), current_content_length); > buffer_position_.clear(); Ack. Let's discuss. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:158: buffer_ = buffer_.substr(buffer_position_, On 2012/11/12 06:07:00, satorux1 wrote: > Why not just buffer_.clear()? > > At this moment, All the content in buffer_ should be consumed, right? > > If so, we can add a DCHECK like: > > DCHECK_EQ(buffer_.size(), buffer_position_); > > or > > DCHECK(buffer_position_.empty()) > > if you use StringPiece. This is in case, we get two (or more) requests in one chunk. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_request.h (right): http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.h:29: // Wraps the HTTP request. Since it can be big, use scoped_ptr to pass it On 2012/11/12 06:07:00, satorux1 wrote: > Wraps -> Represents ? Done. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.h:54: class HttpRequestParser { On 2012/11/12 06:07:00, satorux1 wrote: > I think we can write a unit test for this class. It is already done. I've extracted it to a separate file. Please take a look at http_request_unittest.cc. Done. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.h:73: virtual void ProcessChunk(const char *data, size_t length); On 2012/11/12 06:07:00, satorux1 wrote: > Can we use StringPiece instead? One less parameter would be good. How about std::string? Done. Please let me know what do you think. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.h:79: virtual ParseResult ParseRequest(); On 2012/11/12 06:07:00, satorux1 wrote: > should this be virtual? Are there any classes inheriting this class? Done. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.h:91: size_t current_content_length_; On 2012/11/12 06:07:00, satorux1 wrote: > This is the content length declared in the Content-Length: header, right? Then, > "declared_content_length" may be a better name? Right! Done. Also, added a comment. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.h:103: std::string ShiftLine(); On 2012/11/12 06:07:00, satorux1 wrote: > Function comment is missing. Does the returned string includes CRLF? When it > returns an empty string? Please document. Done. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_response.h (right): http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_response.h:22: // Wraps the HTTP response. Since it can be big, it may be better to use On 2012/11/12 06:07:00, satorux1 wrote: > Wraps -> Represents ? Done. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_response.h:24: class HttpResponse { On 2012/11/12 06:07:00, satorux1 wrote: > class -> struct Done. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_response.h:35: virtual std::string ToResponseString() const; On 2012/11/12 06:07:00, satorux1 wrote: > why virtual? Done. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_test_server_unittest.cc (right): http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_test_server_unittest.cc:64: TEST_F(HttpTestServerTest, ParseRequest) { On 2012/11/12 06:47:40, satorux1 wrote: > I think the paser should be tested in http_request_unittest.cc Done. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_test_server_unittest.cc:117: TEST_F(HttpTestServerTest, GenerateResponse) { On 2012/11/12 06:47:40, satorux1 wrote: > Should test in http_response_unittest.cc Done. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_test_server_unittest.cc:142: HttpTestServer::CreateForTesting(); On 2012/11/12 06:47:40, satorux1 wrote: > I think we should do this in SetUp(). This is easily if the tests above are > moved to separate files. Done. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_test_server_unittest.cc:143: DCHECK(server_.get()); On 2012/11/12 06:43:47, satorux1 wrote: > ASSERT_TRUE() Done.
http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_connection.h (right): http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_connection.h:34: virtual void SendResponse(scoped_ptr<HttpResponse> response) const; On 2012/11/12 12:17:44, mtomasz wrote: > On 2012/11/12 06:07:00, satorux1 wrote: > > Should this be a virtual function? There are no classes inheriting > > HttpConnection class in this patch. > > Why don't we make public methods virtual by default for classes which could be > overriden in the future (eg. for mocks)? > > Done. See YAGNI on wikipedia. :) Besides: 1) It's helpful to know what functions are overridden and what are not 2) For mocking, we should usually make functions pure-virtual. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_connection.h:42: virtual void ReceiveData(const char* data, size_t length); On 2012/11/12 12:17:44, mtomasz wrote: > On 2012/11/12 06:07:00, satorux1 wrote: > > Can we use StringPiece instead? One less parameter would be good. > > How about just std::string since we don't need any internal cursors? const std::string& will make a copy if the input is not a std::string. On the other hand, StringPiece will not make a copy hence it's often more desirable for parameters like this. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_request.cc (right): http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:39: buffer_.append(data, length); On 2012/11/12 12:17:44, mtomasz wrote: > On 2012/11/12 06:07:00, satorux1 wrote: > > If you use StringPiece for buffer_position_, it should also be updated here: > > > > buffer_position_.set(buffer_position_.data(), > > buffer_position_.size() + length, > > > > I am not sure if I understand correctly. So, we will have two variables: > std::string buffer_ and StringPiece buffer_position_ (an iterator on buffer_)? Yes, but it's no worse than now. We already have two. > Isn't it then bug prone? Eg. when we do buffer_ = "", then working on > buffer_position_ would sigsegv. This is also unsafe with the current code. buffer_position_ should be set to 0 in this case, right? > Also, after appending to buffer_, its internal > array may be reallocated causing a sigsegv in buffer_position_. Please correct > me if I am wrong. That's a good point, but this is the only place where data is appended to buffer_, so I think it's not very error-prone. StringPiece carries two states: the current position and the remaining length. Here, the remaining space is extended, so we should update StringPiece here. On the other hand, size_t carries only a single state, so you need to compute the remaining length, which I think is tricky. Overall, I think StringPiece allows us to write cleaner code. What do you think? http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:40: DCHECK(buffer_.length() + length <= kRequestSizeLimit) << On 2012/11/12 12:17:44, mtomasz wrote: > On 2012/11/12 06:07:00, satorux1 wrote: > > DCHECK_LE > > Why is it better? Done. The failure message is better. You'll see the two values. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:158: buffer_ = buffer_.substr(buffer_position_, On 2012/11/12 12:17:44, mtomasz wrote: > On 2012/11/12 06:07:00, satorux1 wrote: > > Why not just buffer_.clear()? > > > > At this moment, All the content in buffer_ should be consumed, right? > > > > If so, we can add a DCHECK like: > > > > DCHECK_EQ(buffer_.size(), buffer_position_); > > > > or > > > > DCHECK(buffer_position_.empty()) > > > > if you use StringPiece. > > This is in case, we get two (or more) requests in one chunk. I think we don't need to support the case. The server does not support Keep-alive, so the client won't reuse the same connection. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_request.h (right): http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.h:73: virtual void ProcessChunk(const char *data, size_t length); On 2012/11/12 12:17:44, mtomasz wrote: > On 2012/11/12 06:07:00, satorux1 wrote: > > Can we use StringPiece instead? One less parameter would be good. > > How about std::string? Done. Please let me know what do you think. I think StringPiece is better. http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_request.cc (right): http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:130: declared_content_length_ = 0; maybe set this to -1? see below http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:136: if (declared_content_length_ == 0) { Content-Length: 0 is valid. It means that the request carries and empty body. I think we should be able to handle an empty body. It'd be nice to have a test for this. http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_request.h (right): http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.h:49: // parser.ProcessChunk(std::string(data, size)); I think StringPiece is better. It won't make a copy. http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_request_unittest.cc (right): http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request_unittest.cc:63: } Please also add a test for an empty body (i.e. Content-Length: 0) http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_response_unittest.cc (right): http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_response_unittest.cc:20: std::string request_response_string = const std::string kExpectedRequestResponseString http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_server.h (right): http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_server.h:93: const ResponseCode response_code); let's add a blank line. http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_server.h:97: const std::string& content_type); indentation is off. http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_server.h:104: const ResponseCode response_code); blank line.
http://codereview.chromium.org/11088073/diff/33001/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_test_server.h (right): http://codereview.chromium.org/11088073/diff/33001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_test_server.h:56: class HttpTestServer : private net::StreamListenSocket::Delegate { On 2012/11/12 12:17:44, mtomasz wrote: > On 2012/11/12 06:43:47, satorux1 wrote: > > TestHttpServer? > > How about just HttpServer? It is already in test_servers namespace. Speaking of the namespace, do you expect to add more servers? Otherwise, "test_server" would be a better name. http://codereview.chromium.org/11088073/diff/33001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_test_server.h:66: static scoped_ptr<HttpTestServer> CreateForTesting(); On 2012/11/12 06:43:47, satorux1 wrote: > CreateForTesting() sounded a bit awkward as the server is only used for testing. > Instead of having the factory method, the following may be clearer: > > TEST_F(..., ...) { > HttpTestServer server; > ASSERT_TRUE(server.StartAndBlockUntilReady()); > ... > } did you miss this comment? http://codereview.chromium.org/11088073/diff/33001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_test_server.h:71: static void CreateOnIOThread(const CreateCallback& callback); On 2012/11/12 12:17:44, mtomasz wrote: > On 2012/11/12 06:43:47, satorux1 wrote: > > Move it anonymous namespace in .cc file? The function doesn't seem to be > called > > from other files. > > The constructor is private, so we need it here. Then we can at least make it private? http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_server.cc (right): http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_server.cc:69: BrowserThread::PostTask( You want to call OnCreatedForTesting on UI thread, right? Please use PostTaskAndReply. http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_server.cc:97: retries_left--; nit: --retries_left; post-increment/decrement is more common in chrome code. http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_server.h (right): http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_server.h:79: void RegisterRequestHandler(const HandleRequestCallback& callback); Hmm, I think this function should take |relative_path|. http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_server.h:96: GURL RegisterSimpleTextResponse(const std::string& content, I think the "simple" version is unnecessary. I'd rather want to see less public functions. Besides, we can add it later if we want. http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_server.h:107: GURL RegisterSimpleFileResponse( Likewise, I think we don't need the simple version. http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_server.h:123: std::string GenerateUniqueIdentifier(); We can remove this if we get rid of the simple versions. http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_server.h:148: base::WeakPtrFactory<HttpServer> weak_factory_; weak_ptr_factory_ Please also add: // Note: This should remain the last member so it'll be destroyed and // invalidate its weak pointers before any other members are destroyed.
I've addressed most of the comments. Please take a look and let's discuss the rest tomorrow. http://codereview.chromium.org/11088073/diff/33001/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_test_server.cc (right): http://codereview.chromium.org/11088073/diff/33001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_test_server.cc:101: weak_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) { On 2012/11/12 06:34:15, satorux1 wrote: > DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); Done. http://codereview.chromium.org/11088073/diff/33001/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_test_server.h (right): http://codereview.chromium.org/11088073/diff/33001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_test_server.h:56: class HttpTestServer : private net::StreamListenSocket::Delegate { On 2012/11/13 05:28:34, satorux1 wrote: > On 2012/11/12 12:17:44, mtomasz wrote: > > On 2012/11/12 06:43:47, satorux1 wrote: > > > TestHttpServer? > > > > How about just HttpServer? It is already in test_servers namespace. > > Speaking of the namespace, do you expect to add more servers? Otherwise, > "test_server" would be a better name. Done. http://codereview.chromium.org/11088073/diff/33001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_test_server.h:66: static scoped_ptr<HttpTestServer> CreateForTesting(); On 2012/11/13 05:28:34, satorux1 wrote: > On 2012/11/12 06:43:47, satorux1 wrote: > > CreateForTesting() sounded a bit awkward as the server is only used for > testing. > > Instead of having the factory method, the following may be clearer: > > > > TEST_F(..., ...) { > > HttpTestServer server; > > ASSERT_TRUE(server.StartAndBlockUntilReady()); > > ... > > } > > did you miss this comment? I have some questions about this comment. http://codereview.chromium.org/11088073/diff/33001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_test_server.h:71: static void CreateOnIOThread(const CreateCallback& callback); On 2012/11/13 05:28:34, satorux1 wrote: > On 2012/11/12 12:17:44, mtomasz wrote: > > On 2012/11/12 06:43:47, satorux1 wrote: > > > Move it anonymous namespace in .cc file? The function doesn't seem to be > > called > > > from other files. > > > > The constructor is private, so we need it here. > > Then we can at least make it private? Sure! Done. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_connection.h (right): http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_connection.h:34: virtual void SendResponse(scoped_ptr<HttpResponse> response) const; On 2012/11/13 05:13:18, satorux1 wrote: > On 2012/11/12 12:17:44, mtomasz wrote: > > On 2012/11/12 06:07:00, satorux1 wrote: > > > Should this be a virtual function? There are no classes inheriting > > > HttpConnection class in this patch. > > > > Why don't we make public methods virtual by default for classes which could be > > overriden in the future (eg. for mocks)? > > > > Done. > > See YAGNI on wikipedia. :) Besides: > > 1) It's helpful to know what functions are overridden and what are not > 2) For mocking, we should usually make functions pure-virtual. Got it. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_connection.h:42: virtual void ReceiveData(const char* data, size_t length); On 2012/11/13 05:13:18, satorux1 wrote: > On 2012/11/12 12:17:44, mtomasz wrote: > > On 2012/11/12 06:07:00, satorux1 wrote: > > > Can we use StringPiece instead? One less parameter would be good. > > > > How about just std::string since we don't need any internal cursors? > > const std::string& will make a copy if the input is not a std::string. On the > other hand, StringPiece will not make a copy hence it's often more desirable for > parameters like this. Done. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_request.cc (right): http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:39: buffer_.append(data, length); On 2012/11/13 05:13:18, satorux1 wrote: > On 2012/11/12 12:17:44, mtomasz wrote: > > On 2012/11/12 06:07:00, satorux1 wrote: > > > If you use StringPiece for buffer_position_, it should also be updated here: > > > > > > buffer_position_.set(buffer_position_.data(), > > > buffer_position_.size() + length, > > > > > > > I am not sure if I understand correctly. So, we will have two variables: > > std::string buffer_ and StringPiece buffer_position_ (an iterator on buffer_)? > > Yes, but it's no worse than now. We already have two. > > > Isn't it then bug prone? Eg. when we do buffer_ = "", then working on > > buffer_position_ would sigsegv. > > This is also unsafe with the current code. buffer_position_ should be set to 0 > in this case, right? > > > > Also, after appending to buffer_, its internal > > array may be reallocated causing a sigsegv in buffer_position_. Please correct > > me if I am wrong. > > That's a good point, but this is the only place where data is appended to > buffer_, so I think it's not very error-prone. > > StringPiece carries two states: the current position and the remaining length. > Here, the remaining space is extended, so we should update StringPiece here. > > On the other hand, size_t carries only a single state, so you need to compute > the remaining length, which I think is tricky. Overall, I think StringPiece > allows us to write cleaner code. What do you think? I am still not convinced. To append, the code will look very tricky. void HttpRequestParser::ProcessChunk(const base::StringPiece& data) { // Pointer arithmetic. size_t last_index = buffer_.data() - buffer_position_.data(); // (or tricky last_index = buffer_.size() - buffer_position_.size()) data.AppendToString(buffer_); // Recreate buffer_position_. buffer_position_.set(buffer_.data() + last_index, buffer_position.size() + data.size()); } Also, if we accidentally call any non-const method on the buffer_ member, or data() or c_str() anywhere outside outside ProcessChunk(), then we may get a segfault. If you see any safer way to implement it with StringPiece then I'll change it. How about writing a small HttpRequestBuffer class, which would support appending and fetching lines, or chunk of bytes? http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:40: DCHECK(buffer_.length() + length <= kRequestSizeLimit) << On 2012/11/13 05:13:18, satorux1 wrote: > On 2012/11/12 12:17:44, mtomasz wrote: > > On 2012/11/12 06:07:00, satorux1 wrote: > > > DCHECK_LE > > > > Why is it better? Done. > > The failure message is better. You'll see the two values. Got it. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:158: buffer_ = buffer_.substr(buffer_position_, On 2012/11/13 05:13:18, satorux1 wrote: > On 2012/11/12 12:17:44, mtomasz wrote: > > On 2012/11/12 06:07:00, satorux1 wrote: > > > Why not just buffer_.clear()? > > > > > > At this moment, All the content in buffer_ should be consumed, right? > > > > > > If so, we can add a DCHECK like: > > > > > > DCHECK_EQ(buffer_.size(), buffer_position_); > > > > > > or > > > > > > DCHECK(buffer_position_.empty()) > > > > > > if you use StringPiece. > > > > This is in case, we get two (or more) requests in one chunk. > > I think we don't need to support the case. The server does not support > Keep-alive, so the client won't reuse the same connection. I've just checked the rfc. In http/1.1 all connections are considered persistent unless declared otherwise. However, server can close the connection after the first request and ignore further request in the chunk. So, we can not implement handling multiple requests, but we have to assume that there may be more than one request in a data chunk. However, in both cases the amount of code is the same. So is it worth removing support for it? What do you think? http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_request.h (right): http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.h:73: virtual void ProcessChunk(const char *data, size_t length); On 2012/11/13 05:13:18, satorux1 wrote: > On 2012/11/12 12:17:44, mtomasz wrote: > > On 2012/11/12 06:07:00, satorux1 wrote: > > > Can we use StringPiece instead? One less parameter would be good. > > > > How about std::string? Done. Please let me know what do you think. > > I think StringPiece is better. Done. http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_request.cc (right): http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:130: declared_content_length_ = 0; On 2012/11/13 05:13:18, satorux1 wrote: > maybe set this to -1? see below See below. http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:136: if (declared_content_length_ == 0) { On 2012/11/13 05:13:18, satorux1 wrote: > Content-Length: 0 is valid. It means that the request carries and empty body. I > think we should be able to handle an empty body. It'd be nice to have a test for > this. I think this is already implemented well. |declared_content_length_| is internal variable for parser to know how much content data to read from the buffer. In HttpRequest instance we have all request headers, including Content-Length if provided. http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_request.h (right): http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.h:49: // parser.ProcessChunk(std::string(data, size)); On 2012/11/13 05:13:18, satorux1 wrote: > I think StringPiece is better. It won't make a copy. Done. http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_request_unittest.cc (right): http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request_unittest.cc:63: } On 2012/11/13 05:13:18, satorux1 wrote: > Please also add a test for an empty body (i.e. Content-Length: 0) Done. http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_response_unittest.cc (right): http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_response_unittest.cc:20: std::string request_response_string = On 2012/11/13 05:13:18, satorux1 wrote: > const std::string kExpectedRequestResponseString Done. http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_server.cc (right): http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_server.cc:69: BrowserThread::PostTask( On 2012/11/13 05:28:34, satorux1 wrote: > You want to call OnCreatedForTesting on UI thread, right? Please use > PostTaskAndReply. Hm. I am concerned that passing a result may be quite tricky. We will need to pass a pointer to result to CreateOnIOThread(HttpServer** result) and update it from it. And the same pointer to the reply callback. Formerly CreateOnIOThread(callback) was public so we could use it, if we do not want to block, and want the HttpServer pointer via a callback. Now it is private, so we could change it (as above in the comment), but I am concerned if it is better here. What is your opinion? http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_server.cc:97: retries_left--; On 2012/11/13 05:28:34, satorux1 wrote: > nit: --retries_left; > > post-increment/decrement is more common in chrome code. Done. http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_server.h (right): http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_server.h:79: void RegisterRequestHandler(const HandleRequestCallback& callback); On 2012/11/13 05:28:34, satorux1 wrote: > Hmm, I think this function should take |relative_path|. Callback decides if it will handle that request. I believe it is way more flexible. I think we are going to use this method only if we want some very special behavior, eg. depending on the url. If we set a relative path here, then why would we use this method? http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_server.h:93: const ResponseCode response_code); On 2012/11/13 05:13:18, satorux1 wrote: > let's add a blank line. Done. http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_server.h:96: GURL RegisterSimpleTextResponse(const std::string& content, On 2012/11/13 05:28:34, satorux1 wrote: > I think the "simple" version is unnecessary. I'd rather want to see less public > functions. > > Besides, we can add it later if we want. The reason was that in most cases (I think in all current tests) we don't need a specific relative_path nor any custom response_code. Deleted. Done. http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_server.h:97: const std::string& content_type); On 2012/11/13 05:13:18, satorux1 wrote: > indentation is off. Done. http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_server.h:104: const ResponseCode response_code); On 2012/11/13 05:13:18, satorux1 wrote: > blank line. Done. http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_server.h:107: GURL RegisterSimpleFileResponse( On 2012/11/13 05:28:34, satorux1 wrote: > Likewise, I think we don't need the simple version. Done. http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_server.h:123: std::string GenerateUniqueIdentifier(); On 2012/11/13 05:28:34, satorux1 wrote: > We can remove this if we get rid of the simple versions. Done. http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_server.h:148: base::WeakPtrFactory<HttpServer> weak_factory_; On 2012/11/13 05:28:34, satorux1 wrote: > weak_ptr_factory_ > > Please also add: > > // Note: This should remain the last member so it'll be destroyed and > // invalidate its weak pointers before any other members are destroyed. Done.
Please move this to chrome/browser/google_apis/test_server
http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_request.cc (right): http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:158: buffer_ = buffer_.substr(buffer_position_, On 2012/11/13 12:23:07, mtomasz wrote: > On 2012/11/13 05:13:18, satorux1 wrote: > > On 2012/11/12 12:17:44, mtomasz wrote: > > > On 2012/11/12 06:07:00, satorux1 wrote: > > > > Why not just buffer_.clear()? > > > > > > > > At this moment, All the content in buffer_ should be consumed, right? > > > > > > > > If so, we can add a DCHECK like: > > > > > > > > DCHECK_EQ(buffer_.size(), buffer_position_); > > > > > > > > or > > > > > > > > DCHECK(buffer_position_.empty()) > > > > > > > > if you use StringPiece. > > > > > > This is in case, we get two (or more) requests in one chunk. > > > > I think we don't need to support the case. The server does not support > > Keep-alive, so the client won't reuse the same connection. > > I've just checked the rfc. In http/1.1 all connections are considered persistent > unless declared otherwise. However, server > can close the connection after the first request and ignore further request in > the chunk. > > So, we can not implement handling multiple requests, but we have to assume that > there may be more than one request in a data chunk. However, in both cases the > amount of code is the same. So is it worth removing support for it? What do you > think? Let's remove the support for the multiple requests case. Less things to worry about is a good thing (ex. less things to test). Besides, we can add this at a later time if needed. http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_server.h (right): http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_server.h:79: void RegisterRequestHandler(const HandleRequestCallback& callback); On 2012/11/13 12:23:07, mtomasz wrote: > On 2012/11/13 05:28:34, satorux1 wrote: > > Hmm, I think this function should take |relative_path|. > > Callback decides if it will handle that request. I believe it is way more > flexible. I think we are going to use this method only if we want some very > special behavior, eg. depending on the url. If we set a relative path here, then > why would we use this method? I think it's good to make it clear what code handles a particular path. With this API, it's hard to find out what code processes a particular path. With my approach, you can replace the loop in HandleRequest() with a table like: iter = method_table_.find(path); if (iter != method_table.end()) { iter->second.Run(request); }
http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_server.cc (right): http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_server.cc:69: BrowserThread::PostTask( On 2012/11/13 12:23:07, mtomasz wrote: > On 2012/11/13 05:28:34, satorux1 wrote: > > You want to call OnCreatedForTesting on UI thread, right? Please use > > PostTaskAndReply. > > Hm. I am concerned that passing a result may be quite tricky. We will need to > pass a pointer to result to CreateOnIOThread(HttpServer** result) and update it > from it. And the same pointer to the reply callback. > > Formerly CreateOnIOThread(callback) was public so we could use it, if we do not > want to block, and want the HttpServer pointer via a callback. Now it is > private, so we could change it (as above in the comment), but I am concerned if > it is better here. What is your opinion? What about doing something like this: HttpServer* result = new HttpServer; BrowserThread::PostTaskAndReply( ..., base::Bind(&HttpServer::InitializeOnIOThread, result); ...); void HttpServer::InitializeOnIOThread(...) { while (...) { if (socket_descriptor != net::TCPListenSocket::kInvalidSocket) { server->Initialize(try_port, socket_descriptor); } ... } } http://codereview.chromium.org/11088073/diff/41003/chrome/browser/google_apis... File chrome/browser/google_apis/test_server/http_server.h (right): http://codereview.chromium.org/11088073/diff/41003/chrome/browser/google_apis... chrome/browser/google_apis/test_server/http_server.h:58: typedef base::Callback<scoped_ptr<HttpResponse>(const HttpRequest* request)> let's make it a const reference: const HttpRequest& as this is always non-null http://codereview.chromium.org/11088073/diff/41003/chrome/browser/google_apis... chrome/browser/google_apis/test_server/http_server.h:128: int last_unique_id_; remove this.
http://codereview.chromium.org/11088073/diff/41003/chrome/browser/google_apis... File chrome/browser/google_apis/test_server/http_request_unittest.cc (right): http://codereview.chromium.org/11088073/diff/41003/chrome/browser/google_apis... chrome/browser/google_apis/test_server/http_request_unittest.cc:75: EXPECT_EQ("0", request->headers["Content-Length"]); Thank you for adding the test. I think we should add has_content() method to HttpRequest, so that the client can tell if request->content carries an empty content, or there is no content.
http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_request.cc (right): http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:39: buffer_.append(data, length); On 2012/11/13 12:23:07, mtomasz wrote: > On 2012/11/13 05:13:18, satorux1 wrote: > > On 2012/11/12 12:17:44, mtomasz wrote: > > > On 2012/11/12 06:07:00, satorux1 wrote: > > > > If you use StringPiece for buffer_position_, it should also be updated > here: > > > > > > > > buffer_position_.set(buffer_position_.data(), > > > > buffer_position_.size() + length, > > > > > > > > > > I am not sure if I understand correctly. So, we will have two variables: > > > std::string buffer_ and StringPiece buffer_position_ (an iterator on > buffer_)? > > > > Yes, but it's no worse than now. We already have two. > > > > > Isn't it then bug prone? Eg. when we do buffer_ = "", then working on > > > buffer_position_ would sigsegv. > > > > This is also unsafe with the current code. buffer_position_ should be set to 0 > > in this case, right? > > > > > > > Also, after appending to buffer_, its internal > > > array may be reallocated causing a sigsegv in buffer_position_. Please > correct > > > me if I am wrong. > > > > That's a good point, but this is the only place where data is appended to > > buffer_, so I think it's not very error-prone. > > > > StringPiece carries two states: the current position and the remaining length. > > Here, the remaining space is extended, so we should update StringPiece here. > > > > On the other hand, size_t carries only a single state, so you need to compute > > the remaining length, which I think is tricky. Overall, I think StringPiece > > allows us to write cleaner code. What do you think? > > I am still not convinced. To append, the code will look very tricky. > > void HttpRequestParser::ProcessChunk(const base::StringPiece& data) { > // Pointer arithmetic. > size_t last_index = buffer_.data() - buffer_position_.data(); > // (or tricky last_index = buffer_.size() - buffer_position_.size()) You are right that this is tricky. Let's keep buffer_postion_ as size_t. However, we can change the parameter to StringPiece. The first line will be - buffer_.append(data, length); + buffer_.append(data.data(), data.size()); > > data.AppendToString(buffer_); > > // Recreate buffer_position_. > buffer_position_.set(buffer_.data() + last_index, > buffer_position.size() + data.size()); > } > > Also, if we accidentally call any non-const method on the buffer_ member, or > data() or c_str() anywhere outside outside ProcessChunk(), then we may get a > segfault. > > If you see any safer way to implement it with StringPiece then I'll change it. > > How about writing a small HttpRequestBuffer class, which would support appending > and fetching lines, or chunk of bytes? http://codereview.chromium.org/11088073/diff/41003/chrome/browser/google_apis... File chrome/browser/google_apis/test_server/http_request.h (right): http://codereview.chromium.org/11088073/diff/41003/chrome/browser/google_apis... chrome/browser/google_apis/test_server/http_request.h:35: GURL uri; uri -> url. The term URI is not used in Chrome code.
http://codereview.chromium.org/11088073/diff/41003/chrome/browser/google_apis... File chrome/browser/google_apis/test_server/http_request_unittest.cc (right): http://codereview.chromium.org/11088073/diff/41003/chrome/browser/google_apis... chrome/browser/google_apis/test_server/http_request_unittest.cc:75: EXPECT_EQ("0", request->headers["Content-Length"]); On 2012/11/14 01:44:39, satorux1 wrote: > Thank you for adding the test. > > I think we should add has_content() method to HttpRequest, so that the client > can tell if request->content carries an empty content, or there is no content. Or just has_content member, as HttpRequest is a struct.
http://codereview.chromium.org/11088073/diff/33001/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_test_server.cc (right): http://codereview.chromium.org/11088073/diff/33001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_test_server.cc:65: MessageLoop::current()->Run(); On 2012/11/12 06:34:15, satorux1 wrote: > Can you do this instead? > > content::RunAllPendingInMessageLoop(BriwserThread::IO); > MessageLoop::current()->RunAllPending(); > > Then you don't need to call Quit() in the callback. Done. http://codereview.chromium.org/11088073/diff/33001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_test_server.cc:106: HttpTestServer::~HttpTestServer() { On 2012/11/12 06:34:15, satorux1 wrote: > on what thread this has to be deleted? I guess it's IO: > > DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); UI. Done. http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_request.cc (right): http://codereview.chromium.org/11088073/diff/38003/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_request.cc:158: buffer_ = buffer_.substr(buffer_position_, On 2012/11/14 01:34:32, satorux1 wrote: > On 2012/11/13 12:23:07, mtomasz wrote: > > On 2012/11/13 05:13:18, satorux1 wrote: > > > On 2012/11/12 12:17:44, mtomasz wrote: > > > > On 2012/11/12 06:07:00, satorux1 wrote: > > > > > Why not just buffer_.clear()? > > > > > > > > > > At this moment, All the content in buffer_ should be consumed, right? > > > > > > > > > > If so, we can add a DCHECK like: > > > > > > > > > > DCHECK_EQ(buffer_.size(), buffer_position_); > > > > > > > > > > or > > > > > > > > > > DCHECK(buffer_position_.empty()) > > > > > > > > > > if you use StringPiece. > > > > > > > > This is in case, we get two (or more) requests in one chunk. > > > > > > I think we don't need to support the case. The server does not support > > > Keep-alive, so the client won't reuse the same connection. > > > > I've just checked the rfc. In http/1.1 all connections are considered > persistent > > unless declared otherwise. However, server > > can close the connection after the first request and ignore further request in > > the chunk. > > > > So, we can not implement handling multiple requests, but we have to assume > that > > there may be more than one request in a data chunk. However, in both cases the > > amount of code is the same. So is it worth removing support for it? What do > you > > think? > > Let's remove the support for the multiple requests case. Less things to worry > about is a good thing (ex. less things to test). Besides, we can add this at a > later time if needed. Done. http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_server.cc (right): http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_server.cc:69: BrowserThread::PostTask( On 2012/11/14 01:42:23, satorux1 wrote: > On 2012/11/13 12:23:07, mtomasz wrote: > > On 2012/11/13 05:28:34, satorux1 wrote: > > > You want to call OnCreatedForTesting on UI thread, right? Please use > > > PostTaskAndReply. > > > > Hm. I am concerned that passing a result may be quite tricky. We will need to > > pass a pointer to result to CreateOnIOThread(HttpServer** result) and update > it > > from it. And the same pointer to the reply callback. > > > > Formerly CreateOnIOThread(callback) was public so we could use it, if we do > not > > want to block, and want the HttpServer pointer via a callback. Now it is > > private, so we could change it (as above in the comment), but I am concerned > if > > it is better here. What is your opinion? > > What about doing something like this: > > HttpServer* result = new HttpServer; > BrowserThread::PostTaskAndReply( > ..., > base::Bind(&HttpServer::InitializeOnIOThread, result); > ...); > > void HttpServer::InitializeOnIOThread(...) { > while (...) { > if (socket_descriptor != net::TCPListenSocket::kInvalidSocket) { > server->Initialize(try_port, socket_descriptor); > } > ... > } > } This is a good idea. Done sth like this. http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/test_servers/http_server.h (right): http://codereview.chromium.org/11088073/diff/41001/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/test_servers/http_server.h:79: void RegisterRequestHandler(const HandleRequestCallback& callback); On 2012/11/14 01:34:32, satorux1 wrote: > On 2012/11/13 12:23:07, mtomasz wrote: > > On 2012/11/13 05:28:34, satorux1 wrote: > > > Hmm, I think this function should take |relative_path|. > > > > Callback decides if it will handle that request. I believe it is way more > > flexible. I think we are going to use this method only if we want some very > > special behavior, eg. depending on the url. If we set a relative path here, > then > > why would we use this method? > > I think it's good to make it clear what code handles a particular path. With > this API, it's hard to find out what code processes a particular path. > > With my approach, you can replace the loop in HandleRequest() with a table like: > > iter = method_table_.find(path); > if (iter != method_table.end()) { > iter->second.Run(request); > } I agree that your idea is simpler. But I don't think mine is so bad :), it is just common chain of responsibility pattern. One of requests in this crbug was to make it extensible using eg. regexps. If we allow callbacks to decide if they want to accept a request, then they can do regex stuff on the url. I think we will need it. In the drive v2 api urls are built using file id, change id, etc, and with the current approach we can do it with one handler, not with separate one for each file id/change id: http://developers.google.com/drive/v2/reference/. However, I will change it if you want. http://codereview.chromium.org/11088073/diff/41003/chrome/browser/google_apis... File chrome/browser/google_apis/test_server/http_request.h (right): http://codereview.chromium.org/11088073/diff/41003/chrome/browser/google_apis... chrome/browser/google_apis/test_server/http_request.h:35: GURL uri; On 2012/11/14 01:54:31, satorux1 wrote: > uri -> url. The term URI is not used in Chrome code. Before we had discussion that url is misleading since url by definition does not contain a query string, and here it can. Then I refactored urls to uris where can it contain a query string. So, what's the final decision with this? http://codereview.chromium.org/11088073/diff/41003/chrome/browser/google_apis... File chrome/browser/google_apis/test_server/http_request_unittest.cc (right): http://codereview.chromium.org/11088073/diff/41003/chrome/browser/google_apis... chrome/browser/google_apis/test_server/http_request_unittest.cc:75: EXPECT_EQ("0", request->headers["Content-Length"]); On 2012/11/14 01:44:39, satorux1 wrote: > Thank you for adding the test. > > I think we should add has_content() method to HttpRequest, so that the client > can tell if request->content carries an empty content, or there is no content. Is the method fine? Done. http://codereview.chromium.org/11088073/diff/41003/chrome/browser/google_apis... File chrome/browser/google_apis/test_server/http_server.h (right): http://codereview.chromium.org/11088073/diff/41003/chrome/browser/google_apis... chrome/browser/google_apis/test_server/http_server.h:58: typedef base::Callback<scoped_ptr<HttpResponse>(const HttpRequest* request)> On 2012/11/14 01:42:23, satorux1 wrote: > let's make it a const reference: const HttpRequest& as this is always non-null Done. http://codereview.chromium.org/11088073/diff/41003/chrome/browser/google_apis... chrome/browser/google_apis/test_server/http_server.h:128: int last_unique_id_; On 2012/11/14 01:42:23, satorux1 wrote: > remove this. Done.
http://codereview.chromium.org/11088073/diff/41003/chrome/browser/google_apis... File chrome/browser/google_apis/test_server/http_request_unittest.cc (right): http://codereview.chromium.org/11088073/diff/41003/chrome/browser/google_apis... chrome/browser/google_apis/test_server/http_request_unittest.cc:75: EXPECT_EQ("0", request->headers["Content-Length"]); On 2012/11/14 03:23:35, mtomasz wrote: > On 2012/11/14 01:44:39, satorux1 wrote: > > Thank you for adding the test. > > > > I think we should add has_content() method to HttpRequest, so that the client > > can tell if request->content carries an empty content, or there is no content. > > Is the method fine? Done. Changed to member.
LGTM with nits http://codereview.chromium.org/11088073/diff/37028/chrome/browser/google_apis... File chrome/browser/google_apis/test_server/http_request.cc (right): http://codereview.chromium.org/11088073/diff/37028/chrome/browser/google_apis... chrome/browser/google_apis/test_server/http_request.cc:175: buffer_ = ""; nit: buffer_.clear(); http://codereview.chromium.org/11088073/diff/37028/chrome/browser/google_apis... File chrome/browser/google_apis/test_server/http_request_unittest.cc (right): http://codereview.chromium.org/11088073/diff/37028/chrome/browser/google_apis... chrome/browser/google_apis/test_server/http_request_unittest.cc:79: } Thank you for adding this! http://codereview.chromium.org/11088073/diff/37028/chrome/browser/google_apis... File chrome/browser/google_apis/test_server/http_server.h (right): http://codereview.chromium.org/11088073/diff/37028/chrome/browser/google_apis... chrome/browser/google_apis/test_server/http_server.h:73: bool Started() { nit: bool Started() const
The nits are minor enough, so let's submit this now, so we can start writing tests on top of this.
+thestig@ for .gypi changes.
http://codereview.chromium.org/11088073/diff/37028/chrome/browser/google_apis... File chrome/browser/google_apis/test_server/http_connection.h (right): http://codereview.chromium.org/11088073/diff/37028/chrome/browser/google_apis... chrome/browser/google_apis/test_server/http_connection.h:8: #include <string> nit: add a blank line after. Ditto in other files. http://codereview.chromium.org/11088073/diff/37028/chrome/browser/google_apis... File chrome/browser/google_apis/test_server/http_request.cc (right): http://codereview.chromium.org/11088073/diff/37028/chrome/browser/google_apis... chrome/browser/google_apis/test_server/http_request.cc:73: if (state_ == STATE_CONTENT) { Can this block be combined with the one immediately above? http://codereview.chromium.org/11088073/diff/37028/chrome/browser/google_apis... chrome/browser/google_apis/test_server/http_request.cc:91: DCHECK_EQ(header_line_tokens.size(), 3u); nit: Preferably, keep the order identical to EXPECT_EQ. i.e. DCHECK_EQ(expected, actual). http://codereview.chromium.org/11088073/diff/37028/chrome/browser/google_apis... chrome/browser/google_apis/test_server/http_request.cc:134: DCHECK(base::StringToSizeT(http_request_->headers["Content-Length"], This line won't get executed in a release build. Is that ok? http://codereview.chromium.org/11088073/diff/37028/chrome/browser/google_apis... File chrome/browser/google_apis/test_server/http_request.h (right): http://codereview.chromium.org/11088073/diff/37028/chrome/browser/google_apis... chrome/browser/google_apis/test_server/http_request.h:18: using base::StringPiece; nit: shouldn't be used in headers. Also not needed. http://codereview.chromium.org/11088073/diff/37028/chrome/browser/google_apis... File chrome/browser/google_apis/test_server/http_server.cc (right): http://codereview.chromium.org/11088073/diff/37028/chrome/browser/google_apis... chrome/browser/google_apis/test_server/http_server.cc:172: DCHECK(file_util::ReadFileToString(file_path, &default_response.content)) << Again, is it ok if a release build did not read file to string? http://codereview.chromium.org/11088073/diff/37028/chrome/chrome_tests_unit.gypi File chrome/chrome_tests_unit.gypi (right): http://codereview.chromium.org/11088073/diff/37028/chrome/chrome_tests_unit.g... chrome/chrome_tests_unit.gypi:133: 'browser/google_apis/test_server/http_server.cc', This got added twice.
Lei, thank you for the great comments. The author Tomasz just left the office and will be ooo for about two days. Can we submit this now, and address your comments in a separate patch? I'll be happy to address your comments on behalf of Tomasz, as soon as possible.
http://codereview.chromium.org/11088073/diff/37028/chrome/browser/google_apis... File chrome/browser/google_apis/test_server/http_request.cc (right): http://codereview.chromium.org/11088073/diff/37028/chrome/browser/google_apis... chrome/browser/google_apis/test_server/http_request.cc:134: DCHECK(base::StringToSizeT(http_request_->headers["Content-Length"], On 2012/11/14 04:24:15, Lei Zhang wrote: > This line won't get executed in a release build. Is that ok? No. This is not OK. We should fix this. Great catch!
I take it back. Submitting this as-is doesn't seem to be a good idea. Let me copy this patch and address your comments _right now_. http://codereview.chromium.org/11088073/diff/37028/chrome/chrome_tests_unit.gypi File chrome/chrome_tests_unit.gypi (right): http://codereview.chromium.org/11088073/diff/37028/chrome/chrome_tests_unit.g... chrome/chrome_tests_unit.gypi:133: 'browser/google_apis/test_server/http_server.cc', On 2012/11/14 04:24:15, Lei Zhang wrote: > This got added twice. Hmm, not good.
Addressed comments in http://codereview.chromium.org/11358227/ http://codereview.chromium.org/11088073/diff/37028/chrome/browser/google_apis... File chrome/browser/google_apis/test_server/http_connection.h (right): http://codereview.chromium.org/11088073/diff/37028/chrome/browser/google_apis... chrome/browser/google_apis/test_server/http_connection.h:8: #include <string> On 2012/11/14 04:24:15, Lei Zhang wrote: > nit: add a blank line after. Ditto in other files. Done. http://codereview.chromium.org/11088073/diff/37028/chrome/browser/google_apis... File chrome/browser/google_apis/test_server/http_request.cc (right): http://codereview.chromium.org/11088073/diff/37028/chrome/browser/google_apis... chrome/browser/google_apis/test_server/http_request.cc:73: if (state_ == STATE_CONTENT) { On 2012/11/14 04:24:15, Lei Zhang wrote: > Can this block be combined with the one immediately above? Done. http://codereview.chromium.org/11088073/diff/37028/chrome/browser/google_apis... chrome/browser/google_apis/test_server/http_request.cc:91: DCHECK_EQ(header_line_tokens.size(), 3u); On 2012/11/14 04:24:15, Lei Zhang wrote: > nit: Preferably, keep the order identical to EXPECT_EQ. i.e. DCHECK_EQ(expected, > actual). Done. http://codereview.chromium.org/11088073/diff/37028/chrome/browser/google_apis... chrome/browser/google_apis/test_server/http_request.cc:134: DCHECK(base::StringToSizeT(http_request_->headers["Content-Length"], On 2012/11/14 04:28:00, satorux1 wrote: > On 2012/11/14 04:24:15, Lei Zhang wrote: > > This line won't get executed in a release build. Is that ok? > > No. This is not OK. We should fix this. Great catch! Done. http://codereview.chromium.org/11088073/diff/37028/chrome/browser/google_apis... chrome/browser/google_apis/test_server/http_request.cc:175: buffer_ = ""; On 2012/11/14 03:33:57, satorux1 wrote: > nit: buffer_.clear(); Done. http://codereview.chromium.org/11088073/diff/37028/chrome/browser/google_apis... File chrome/browser/google_apis/test_server/http_request.h (right): http://codereview.chromium.org/11088073/diff/37028/chrome/browser/google_apis... chrome/browser/google_apis/test_server/http_request.h:18: using base::StringPiece; On 2012/11/14 04:24:15, Lei Zhang wrote: > nit: shouldn't be used in headers. Also not needed. Done. http://codereview.chromium.org/11088073/diff/37028/chrome/browser/google_apis... File chrome/browser/google_apis/test_server/http_server.cc (right): http://codereview.chromium.org/11088073/diff/37028/chrome/browser/google_apis... chrome/browser/google_apis/test_server/http_server.cc:172: DCHECK(file_util::ReadFileToString(file_path, &default_response.content)) << On 2012/11/14 04:24:15, Lei Zhang wrote: > Again, is it ok if a release build did not read file to string? Done. Good catch! http://codereview.chromium.org/11088073/diff/37028/chrome/browser/google_apis... File chrome/browser/google_apis/test_server/http_server.h (right): http://codereview.chromium.org/11088073/diff/37028/chrome/browser/google_apis... chrome/browser/google_apis/test_server/http_server.h:73: bool Started() { On 2012/11/14 03:33:57, satorux1 wrote: > nit: bool Started() const Done. http://codereview.chromium.org/11088073/diff/37028/chrome/chrome_tests_unit.gypi File chrome/chrome_tests_unit.gypi (right): http://codereview.chromium.org/11088073/diff/37028/chrome/chrome_tests_unit.g... chrome/chrome_tests_unit.gypi:133: 'browser/google_apis/test_server/http_server.cc', On 2012/11/14 04:29:35, satorux1 wrote: > On 2012/11/14 04:24:15, Lei Zhang wrote: > > This got added twice. > > Hmm, not good. Done.
This code will never be run in release but I agree that that dcheck is very ugly. Great catch! I'll be in the MTV office in 24 hours and then I can resume work on this cl. Thanks, Tomasz On Wednesday, November 14, 2012, wrote: > I take it back. Submitting this as-is doesn't seem to be a good idea. Let > me > copy this patch and address your comments _right now_. > > > http://codereview.chromium.**org/11088073/diff/37028/** > chrome/chrome_tests_unit.gypi<http://codereview.chromium.org/11088073/diff/37028/chrome/chrome_tests_unit.gypi> > File chrome/chrome_tests_unit.gypi (right): > > http://codereview.chromium.**org/11088073/diff/37028/** > chrome/chrome_tests_unit.gypi#**newcode133<http://codereview.chromium.org/11088073/diff/37028/chrome/chrome_tests_unit.gypi#newcode133> > chrome/chrome_tests_unit.gypi:**133: > 'browser/google_apis/test_**server/http_server.cc', > On 2012/11/14 04:24:15, Lei Zhang wrote: > >> This got added twice. >> > > Hmm, not good. > > http://codereview.chromium.**org/11088073/<http://codereview.chromium.org/110... >
I created a patch that addressed Lei's comments: http://codereview.chromium.org/11358227/ The code will be run on Release build. IIRC, try bots run tests in Release build. It's not uncommon for developers to use Release build for development too. On Wed, Nov 14, 2012 at 1:48 PM, Tomasz Mikolajewski <mtomasz@google.com>wrote: > This code will never be run in release but I agree that that dcheck is > very ugly. Great catch! I'll be in the MTV office in 24 hours and then I > can resume work on this cl. > Thanks, Tomasz > On Wednesday, November 14, 2012, wrote: >> I take it back. Submitting this as-is doesn't seem to be a good idea. Let >> me >> copy this patch and address your comments _right now_. >> http://codereview.chromium.**org/11088073/diff/37028/** >> chrome/chrome_tests_unit.gypi<http://codereview.chromium.org/11088073/diff/37028/chrome/chrome_tests_unit.gypi> >> File chrome/chrome_tests_unit.gypi (right): >> http://codereview.chromium.**org/11088073/diff/37028/** >> chrome/chrome_tests_unit.gypi#**newcode133<http://codereview.chromium.org/11088073/diff/37028/chrome/chrome_tests_unit.gypi#newcode133> >> chrome/chrome_tests_unit.gypi:**133: >> 'browser/google_apis/test_**server/http_server.cc', >> On 2012/11/14 04:24:15, Lei Zhang wrote: >>> This got added twice. >> Hmm, not good. >> http://codereview.chromium.**org/11088073/<http://codereview.chromium.org/110...
I didn't know that. Thanks for addressing the comments! On Wednesday, November 14, 2012, Satoru Takabayashi wrote: > I created a patch that addressed Lei's comments: > http://codereview.chromium.org/11358227/ > > The code will be run on Release build. IIRC, try bots run tests in Release > build. It's not uncommon for developers to use Release build for > development too. > > On Wed, Nov 14, 2012 at 1:48 PM, Tomasz Mikolajewski <mtomasz@google.com<javascript:_e({}, 'cvml', 'mtomasz@google.com');> > > wrote: > >> This code will never be run in release but I agree that that dcheck is >> very ugly. Great catch! I'll be in the MTV office in 24 hours and then I >> can resume work on this cl. >> >> Thanks, Tomasz >> >> >> On Wednesday, November 14, 2012, wrote: >> >>> I take it back. Submitting this as-is doesn't seem to be a good idea. >>> Let me >>> copy this patch and address your comments _right now_. >>> >>> >>> http://codereview.chromium.**org/11088073/diff/37028/** >>> chrome/chrome_tests_unit.gypi<http://codereview.chromium.org/11088073/diff/37028/chrome/chrome_tests_unit.gypi> >>> File chrome/chrome_tests_unit.gypi (right): >>> >>> http://codereview.chromium.**org/11088073/diff/37028/** >>> chrome/chrome_tests_unit.gypi#**newcode133<http://codereview.chromium.org/11088073/diff/37028/chrome/chrome_tests_unit.gypi#newcode133> >>> chrome/chrome_tests_unit.gypi:**133: >>> 'browser/google_apis/test_**server/http_server.cc', >>> On 2012/11/14 04:24:15, Lei Zhang wrote: >>> >>>> This got added twice. >>>> >>> >>> Hmm, not good. >>> >>> http://codereview.chromium.**org/11088073/<http://codereview.chromium.org/110... >>> >> > |
