HTTP server for testing Google APIs

HTTP server for testing Google APIs

The patch was copied from http://codereview.chromium.org/11088073/
so satorux could address comments from thestig on behalf of mtomasz.

BUG=148710
TEST=none; the code is only for testing and won't be used for production.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=167627

[satorux1, 2012-11-14 04:41:44]

Continuation from http://codereview.chromium.org/11088073/

[mtomasz, 2012-11-14 05:00:44]

http://codereview.chromium.org/11358227/diff/3001/chrome/browser/google_apis/...
File chrome/browser/google_apis/test_server/http_request.cc (right):

http://codereview.chromium.org/11358227/diff/3001/chrome/browser/google_apis/...
chrome/browser/google_apis/test_server/http_request.cc:71: if (ParseHeaders() ==
ACCEPTED)
There must not be else here. It will fail if we have both headers and content
data in the same chunk.

[satorux1, 2012-11-14 05:04:10]

http://codereview.chromium.org/11358227/diff/3001/chrome/browser/google_apis/...
File chrome/browser/google_apis/test_server/http_request.cc (right):

http://codereview.chromium.org/11358227/diff/3001/chrome/browser/google_apis/...
chrome/browser/google_apis/test_server/http_request.cc:71: if (ParseHeaders() ==
ACCEPTED)
On 2012/11/14 05:00:44, mtomasz wrote:
> There must not be else here. It will fail if we have both headers and content
> data in the same chunk.

Ah, I see. state_ can be changed in ParseHeaders(). Fixed with a comment.

[Lei Zhang, 2012-11-14 05:05:43]

http://codereview.chromium.org/11358227/diff/3001/chrome/browser/google_apis/...
File chrome/browser/google_apis/test_server/http_request.cc (right):

http://codereview.chromium.org/11358227/diff/3001/chrome/browser/google_apis/...
chrome/browser/google_apis/test_server/http_request.cc:71: if (ParseHeaders() ==
ACCEPTED)
On 2012/11/14 05:00:44, mtomasz wrote:
> There must not be else here. It will fail if we have both headers and content
> data in the same chunk.

Ah, did not realize ParseHeaders() changes |state_|.

http://codereview.chromium.org/11358227/diff/3001/chrome/browser/google_apis/...
chrome/browser/google_apis/test_server/http_request.cc:138: "Malformed
Content-Length header's value.";
this fits on the previous line, right?

http://codereview.chromium.org/11358227/diff/3001/chrome/browser/google_apis/...
File chrome/browser/google_apis/test_server/http_response.cc (right):

http://codereview.chromium.org/11358227/diff/3001/chrome/browser/google_apis/...
chrome/browser/google_apis/test_server/http_response.cc:39: it++) {
nit: ++it

http://codereview.chromium.org/11358227/diff/3001/chrome/browser/google_apis/...
chrome/browser/google_apis/test_server/http_response.cc:42: std::string
header_value = it->second.c_str();
why can't this be const ref as well?

http://codereview.chromium.org/11358227/diff/3001/chrome/browser/google_apis/...
chrome/browser/google_apis/test_server/http_response.cc:53: response_builder +=
content;
just write: return response_builder + content

[satorux1, 2012-11-14 05:13:39]

http://codereview.chromium.org/11358227/diff/3001/chrome/browser/google_apis/...
File chrome/browser/google_apis/test_server/http_request.cc (right):

http://codereview.chromium.org/11358227/diff/3001/chrome/browser/google_apis/...
chrome/browser/google_apis/test_server/http_request.cc:138: "Malformed
Content-Length header's value.";
On 2012/11/14 05:05:43, Lei Zhang wrote:
> this fits on the previous line, right?

Done.

http://codereview.chromium.org/11358227/diff/3001/chrome/browser/google_apis/...
File chrome/browser/google_apis/test_server/http_response.cc (right):

http://codereview.chromium.org/11358227/diff/3001/chrome/browser/google_apis/...
chrome/browser/google_apis/test_server/http_response.cc:39: it++) {
On 2012/11/14 05:05:43, Lei Zhang wrote:
> nit: ++it

Done.

http://codereview.chromium.org/11358227/diff/3001/chrome/browser/google_apis/...
chrome/browser/google_apis/test_server/http_response.cc:42: std::string
header_value = it->second.c_str();
On 2012/11/14 05:05:43, Lei Zhang wrote:
> why can't this be const ref as well?

Done. Removed c_str() from the two lines too.

http://codereview.chromium.org/11358227/diff/3001/chrome/browser/google_apis/...
chrome/browser/google_apis/test_server/http_response.cc:53: response_builder +=
content;
On 2012/11/14 05:05:43, Lei Zhang wrote:
> just write: return response_builder + content

Done.

[Lei Zhang, 2012-11-14 05:30:07]

https://chromiumcodereview.appspot.com/11358227/diff/6004/chrome/browser/goog...
File chrome/browser/google_apis/test_server/http_response.h (right):

https://chromiumcodereview.appspot.com/11358227/diff/6004/chrome/browser/goog...
chrome/browser/google_apis/test_server/http_response.h:9: #include <map>
nit: map before string, separate system headers with blank line.

https://chromiumcodereview.appspot.com/11358227/diff/6004/chrome/browser/goog...
chrome/browser/google_apis/test_server/http_response.h:24: struct HttpResponse {
This should either be a class, or ToResponseString() should be a static function
that operates on struct HttpResponse.

https://chromiumcodereview.appspot.com/11358227/diff/6004/chrome/browser/goog...
File chrome/browser/google_apis/test_server/http_server.cc (right):

https://chromiumcodereview.appspot.com/11358227/diff/6004/chrome/browser/goog...
chrome/browser/google_apis/test_server/http_server.cc:9: #include
"base/file_util.h"
nit: sort this list in alphabetical order

https://chromiumcodereview.appspot.com/11358227/diff/6004/chrome/browser/goog...
File chrome/browser/google_apis/test_server/http_server.h (right):

https://chromiumcodereview.appspot.com/11358227/diff/6004/chrome/browser/goog...
chrome/browser/google_apis/test_server/http_server.h:17: #include
"chrome/browser/google_apis/gdata_test_util.h"
nit: alphabetical order

https://chromiumcodereview.appspot.com/11358227/diff/6004/chrome/browser/goog...
chrome/browser/google_apis/test_server/http_server.h:117: // |server| is the
original listening Socket, connection is the new
nit: No need to document this here. Presumably already documented in the
interface you are overriding.

https://chromiumcodereview.appspot.com/11358227/diff/6004/chrome/browser/goog...
chrome/browser/google_apis/test_server/http_server.h:120: void
DidAccept(net::StreamListenSocket* server,
virtual

[satorux1, 2012-11-14 05:47:30]

http://codereview.chromium.org/11358227/diff/6004/chrome/browser/google_apis/...
File chrome/browser/google_apis/test_server/http_response.h (right):

http://codereview.chromium.org/11358227/diff/6004/chrome/browser/google_apis/...
chrome/browser/google_apis/test_server/http_response.h:9: #include <map>
On 2012/11/14 05:30:07, Lei Zhang wrote:
> nit: map before string, separate system headers with blank line.

Done.

http://codereview.chromium.org/11358227/diff/6004/chrome/browser/google_apis/...
chrome/browser/google_apis/test_server/http_response.h:24: struct HttpResponse {
On 2012/11/14 05:30:07, Lei Zhang wrote:
> This should either be a class, or ToResponseString() should be a static
function
> that operates on struct HttpResponse.

Changed it to a class.

http://codereview.chromium.org/11358227/diff/6004/chrome/browser/google_apis/...
File chrome/browser/google_apis/test_server/http_server.cc (right):

http://codereview.chromium.org/11358227/diff/6004/chrome/browser/google_apis/...
chrome/browser/google_apis/test_server/http_server.cc:9: #include
"base/file_util.h"
On 2012/11/14 05:30:07, Lei Zhang wrote:
> nit: sort this list in alphabetical order

Done.

http://codereview.chromium.org/11358227/diff/6004/chrome/browser/google_apis/...
File chrome/browser/google_apis/test_server/http_server.h (right):

http://codereview.chromium.org/11358227/diff/6004/chrome/browser/google_apis/...
chrome/browser/google_apis/test_server/http_server.h:17: #include
"chrome/browser/google_apis/gdata_test_util.h"
On 2012/11/14 05:30:07, Lei Zhang wrote:
> nit: alphabetical order

Done.

http://codereview.chromium.org/11358227/diff/6004/chrome/browser/google_apis/...
chrome/browser/google_apis/test_server/http_server.h:117: // |server| is the
original listening Socket, connection is the new
On 2012/11/14 05:30:07, Lei Zhang wrote:
> nit: No need to document this here. Presumably already documented in the
> interface you are overriding.

Done.

http://codereview.chromium.org/11358227/diff/6004/chrome/browser/google_apis/...
chrome/browser/google_apis/test_server/http_server.h:120: void
DidAccept(net::StreamListenSocket* server,
On 2012/11/14 05:30:07, Lei Zhang wrote:
> virtual

Done.

http://codereview.chromium.org/11358227/diff/6004/chrome/browser/google_apis/...
chrome/browser/google_apis/test_server/http_server.h:120: void
DidAccept(net::StreamListenSocket* server,
On 2012/11/14 05:30:07, Lei Zhang wrote:
> virtual

Done.

[Lei Zhang, 2012-11-14 05:54:23]

lgtm

[satorux1, 2012-11-14 10:19:31]

http://codereview.chromium.org/11358227/diff/10004/chrome/browser/google_apis...
File chrome/browser/google_apis/test_server/http_request.h (right):

http://codereview.chromium.org/11358227/diff/10004/chrome/browser/google_apis...
chrome/browser/google_apis/test_server/http_request.h:25: DELETE,
Sigh. This seems to be a macro defined on Windows...

FAILED: ninja -t msvc -r . -o
obj\chrome\browser\google_apis\test_server\test_support_common.http_server.obj
-e environment.x86 -- E:\b\build\goma\gomacc.exe cl.exe /nologo /showIncludes
/FC
@obj\chrome\browser\google_apis\test_server\test_support_common.http_server.obj.rsp
 /c ..\..\chrome\browser\google_apis\test_server\http_server.cc
/Foobj\chrome\browser\google_apis\test_server\test_support_common.http_server.obj
 /Fdtest_support_common.pdb 
e:\b\build\slave\win\build\src\chrome\browser\google_apis\test_server\http_request.h(25)
: error C2059: syntax error : '('

e:\b\build\slave\win\build\src\chrome\browser\google_apis\test_server\http_request.h(25)
: error C3805: '(': unexpected token, expected either '}' or a ','

e:\b\build\slave\win\build\src\chrome\browser\google_apis\test_server\http_request.h(26)
: error C2143: syntax error : missing ')' before '}'