Yeat another test web server. Supports speed throttle and dump of the traffic...

Yet another test web server. Supports speed throttle and dump of the traffic.
Allows binding on 0.0.0.0 to allow connections from a VM (for example).
Simple GMock class and Actions.



Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=49266

[kkania, 2010-06-08 00:24:09]

http://codereview.chromium.org/2620006/diff/9001/10002
File chrome_frame/test/test_server.h (right):

http://codereview.chromium.org/2620006/diff/9001/10002#newcode324
chrome_frame/test/test_server.h:324: class Connection2 : public
base::RefCounted<Connection2> {
I can't really think of a better name, how about ResponseSender? However, I
don't think appending a 2 is acceptable.

http://codereview.chromium.org/2620006/diff/9001/10002#newcode327
chrome_frame/test/test_server.h:327: enum Speed {FAST               = 0x00,
I do not think NOISY should be included in this enum. Why do you want noisy to
be configurable anyways? Is there a problem with logging to INFO all the time?
If there is, perhaps add another bool. Also how about FAST -> IMMEDIATE and SLOW
-> TIMED/DELAYED?

http://codereview.chromium.org/2620006/diff/9001/10002#newcode332
chrome_frame/test/test_server.h:332: SendOptions(enum Speed speed = FAST, int
chunk_size = 0, int64 timeout = 0)
Default args are not allowed by style guidelines. Use static Create... methods
instead. See HTTPTestServer under net/url_request/url_request_unittest.h for an
example.

http://codereview.chromium.org/2620006/diff/9001/10002#newcode344
chrome_frame/test/test_server.h:344: void Send(const std::string& headers, const
std::string& content,
comment this method

http://codereview.chromium.org/2620006/diff/9001/10002#newcode347
chrome_frame/test/test_server.h:347: friend class YAWS;
newline above

http://codereview.chromium.org/2620006/diff/9001/10002#newcode363
chrome_frame/test/test_server.h:363: class YAWS : public
ListenSocket::ListenSocketDelegate {
need better name

http://codereview.chromium.org/2620006/diff/9001/10002#newcode365
chrome_frame/test/test_server.h:365: explicit YAWS(int port, const char*
address, bool noisy);
Use static Create... methods instead of so many overloaded constructors.

http://codereview.chromium.org/2620006/diff/9001/10002#newcode371
chrome_frame/test/test_server.h:371: virtual void Get(Connection2* connection,
add comment

http://codereview.chromium.org/2620006/diff/9001/10002#newcode373
chrome_frame/test/test_server.h:373: virtual void Post(Connection2* connection,
add comment

http://codereview.chromium.org/2620006/diff/9001/10002#newcode379
chrome_frame/test/test_server.h:379: scoped_refptr<ListenSocket> server_;
move variables below methods

http://codereview.chromium.org/2620006/diff/9001/10002#newcode383
chrome_frame/test/test_server.h:383:
std::list<scoped_refptr<Connection2>>::iterator Find(
Change this to FindConnection

http://codereview.chromium.org/2620006/diff/9001/10003
File chrome_frame/test/test_with_web_server.h (right):

http://codereview.chromium.org/2620006/diff/9001/10003#newcode199
chrome_frame/test/test_with_web_server.h:199: struct MockWS : public
test_server::YAWS {
change WS to WebServer

[amit, 2010-06-08 03:10:37]

drive by lgtm, with ken's concerns addressed.

http://codereview.chromium.org/2620006/diff/9001/10001
File chrome_frame/test/test_server.cc (right):

http://codereview.chromium.org/2620006/diff/9001/10001#newcode271
chrome_frame/test/test_server.cc:271: s->r_.OnDataReceived(data);
if (s)?

http://codereview.chromium.org/2620006/diff/9001/10001#newcode282
chrome_frame/test/test_server.cc:282: connection_list_.erase(it);
DCHECK(*it)?

http://codereview.chromium.org/2620006/diff/9001/10001#newcode292
chrome_frame/test/test_server.cc:292: int size = (int)data.size();
why not keep all the sizes as size_t?

http://codereview.chromium.org/2620006/diff/9001/10001#newcode340
chrome_frame/test/test_server.cc:340: if (speed == SendOptions::SLOW) {
FAST_HEADERS and SLOW are mutually exclusive? In that case assert that and/or
use else if?

http://codereview.chromium.org/2620006/diff/9001/10003
File chrome_frame/test/test_with_web_server.h (right):

http://codereview.chromium.org/2620006/diff/9001/10003#newcode179
chrome_frame/test/test_with_web_server.h:179: // Simple Gmock friendly web
server. Sample usage:
Awesome!

[stoyan, 2010-06-08 17:03:28]

http://codereview.chromium.org/2620006/diff/9001/10001
File chrome_frame/test/test_server.cc (right):

http://codereview.chromium.org/2620006/diff/9001/10001#newcode271
chrome_frame/test/test_server.cc:271: s->r_.OnDataReceived(data);
On 2010/06/08 03:10:37, amit wrote:
> if (s)?

Done.

http://codereview.chromium.org/2620006/diff/9001/10001#newcode282
chrome_frame/test/test_server.cc:282: connection_list_.erase(it);
On 2010/06/08 03:10:37, amit wrote:
> DCHECK(*it)?

Done.

http://codereview.chromium.org/2620006/diff/9001/10001#newcode292
chrome_frame/test/test_server.cc:292: int size = (int)data.size();
On 2010/06/08 03:10:37, amit wrote:
> why not keep all the sizes as size_t?

socket->Send() wants int

http://codereview.chromium.org/2620006/diff/9001/10001#newcode340
chrome_frame/test/test_server.cc:340: if (speed == SendOptions::SLOW) {
On 2010/06/08 03:10:37, amit wrote:
> FAST_HEADERS and SLOW are mutually exclusive? In that case assert that and/or
> use else if?

Done.

http://codereview.chromium.org/2620006/diff/9001/10002
File chrome_frame/test/test_server.h (right):

http://codereview.chromium.org/2620006/diff/9001/10002#newcode324
chrome_frame/test/test_server.h:324: class Connection2 : public
base::RefCounted<Connection2> {
On 2010/06/08 00:24:09, kkania wrote:
> I can't really think of a better name, how about ResponseSender? However, I
> don't think appending a 2 is acceptable.

Renamed to SimpleConnection. :)

http://codereview.chromium.org/2620006/diff/9001/10002#newcode327
chrome_frame/test/test_server.h:327: enum Speed {FAST               = 0x00,
On 2010/06/08 00:24:09, kkania wrote:
> I do not think NOISY should be included in this enum. Why do you want noisy to
> be configurable anyways? 

I wanted to avoid logging if large files are sent. And only for the things you
care if testwebserver is used to reproduce conditions for a specific bug.

> Is there a problem with logging to INFO all the time?

Done. 

> If there is, perhaps add another bool. Also how about FAST -> IMMEDIATE and
SLOW
> -> TIMED/DELAYED?

Done.

http://codereview.chromium.org/2620006/diff/9001/10002#newcode332
chrome_frame/test/test_server.h:332: SendOptions(enum Speed speed = FAST, int
chunk_size = 0, int64 timeout = 0)
On 2010/06/08 00:24:09, kkania wrote:
> Default args are not allowed by style guidelines. Use static Create... methods
> instead. See HTTPTestServer under net/url_request/url_request_unittest.h for
an
> example.

Done. (Static creators are the creating objects in heap. I'm trying to avoid
that)

http://codereview.chromium.org/2620006/diff/9001/10002#newcode344
chrome_frame/test/test_server.h:344: void Send(const std::string& headers, const
std::string& content,
On 2010/06/08 00:24:09, kkania wrote:
> comment this method

Done.

http://codereview.chromium.org/2620006/diff/9001/10002#newcode347
chrome_frame/test/test_server.h:347: friend class YAWS;
On 2010/06/08 00:24:09, kkania wrote:
> newline above

Done.

http://codereview.chromium.org/2620006/diff/9001/10002#newcode363
chrome_frame/test/test_server.h:363: class YAWS : public
ListenSocket::ListenSocketDelegate {
On 2010/06/08 00:24:09, kkania wrote:
> need better name

renamed to HTTPTestServer

http://codereview.chromium.org/2620006/diff/9001/10002#newcode365
chrome_frame/test/test_server.h:365: explicit YAWS(int port, const char*
address, bool noisy);
On 2010/06/08 00:24:09, kkania wrote:
> Use static Create... methods instead of so many overloaded constructors.

Done. Instead of creating static Create I prefer to delete these. :)

http://codereview.chromium.org/2620006/diff/9001/10002#newcode371
chrome_frame/test/test_server.h:371: virtual void Get(Connection2* connection,
On 2010/06/08 00:24:09, kkania wrote:
> add comment

Done.

http://codereview.chromium.org/2620006/diff/9001/10002#newcode373
chrome_frame/test/test_server.h:373: virtual void Post(Connection2* connection,
On 2010/06/08 00:24:09, kkania wrote:
> add comment

Done.

http://codereview.chromium.org/2620006/diff/9001/10002#newcode379
chrome_frame/test/test_server.h:379: scoped_refptr<ListenSocket> server_;
On 2010/06/08 00:24:09, kkania wrote:
> move variables below methods

Done.

http://codereview.chromium.org/2620006/diff/9001/10002#newcode383
chrome_frame/test/test_server.h:383:
std::list<scoped_refptr<Connection2>>::iterator Find(
On 2010/06/08 00:24:09, kkania wrote:
> Change this to FindConnection

Done.

http://codereview.chromium.org/2620006/diff/9001/10003
File chrome_frame/test/test_with_web_server.h (right):

http://codereview.chromium.org/2620006/diff/9001/10003#newcode199
chrome_frame/test/test_with_web_server.h:199: struct MockWS : public
test_server::YAWS {
On 2010/06/08 00:24:09, kkania wrote:
> change WS to WebServer

Done.

[kkania, 2010-06-08 18:35:07]

LGTM after fixes...

http://codereview.chromium.org/2620006/diff/15001/16001
File chrome_frame/test/test_server.cc (right):

http://codereview.chromium.org/2620006/diff/15001/16001#newcode226
chrome_frame/test/test_server.cc:226:
std::list<scoped_refptr<SimpleConnection>>::iterator
does this compile?  I thought you need to put a space between the two >>. 
Although if you typedef the list you don't have to worry about it

http://codereview.chromium.org/2620006/diff/15001/16001#newcode231
chrome_frame/test/test_server.cc:231: return it;
if you are going to "return it" in both cases, how about just break here...

http://codereview.chromium.org/2620006/diff/15001/16001#newcode252
chrome_frame/test/test_server.cc:252: scoped_refptr<SimpleConnection> s =
ConnectionFromSocket(socket);
why is this variable named 's'? how about connection

http://codereview.chromium.org/2620006/diff/15001/16001#newcode273
chrome_frame/test/test_server.cc:273: const char* p = data.c_str() + cur_pos_;
p? how about a better name

http://codereview.chromium.org/2620006/diff/15001/16002
File chrome_frame/test/test_server.h (right):

http://codereview.chromium.org/2620006/diff/15001/16002#newcode324
chrome_frame/test/test_server.h:324: class SimpleConnection : public
base::RefCounted<SimpleConnection> {
SimpleConnection? this doesn't seem to be any better than Connection2. How about
ConfigurableConnection? Can you add a comment too?

http://codereview.chromium.org/2620006/diff/15001/16002#newcode341
chrome_frame/test/test_server.h:341: // Send HTTP response with provided
|headers| and |content|. Appends
can you add newlines, here and elsewhere, for these cases:
1) between constructor/destructor section and methods section
2) between methods section and variables section
3) between variables section and DISALLOW_COPY... section
4) before accessor level declaration (eg private:)

http://codereview.chromium.org/2620006/diff/15001/16002#newcode345
chrome_frame/test/test_server.h:345: const SendOptions& options =
DefaultSendOptions);
according to the style guide, only case where default args is allowed is to
simulate variable-length args. Break this into two funcs.

http://codereview.chromium.org/2620006/diff/15001/16002#newcode349
chrome_frame/test/test_server.h:349: void SendChunk();
comment this

http://codereview.chromium.org/2620006/diff/15001/16002#newcode353
chrome_frame/test/test_server.h:353: std::string data;
data_

http://codereview.chromium.org/2620006/diff/15001/16002#newcode358
chrome_frame/test/test_server.h:358: class HTTPTestServer : public
ListenSocket::ListenSocketDelegate {
comment for this class

http://codereview.chromium.org/2620006/diff/15001/16002#newcode382
chrome_frame/test/test_server.h:382: std::list<scoped_refptr<SimpleConnection>>
connection_list_;
How about typedef this list similar to what SimpleWebServer does?

[stoyan, 2010-06-09 16:50:42]

http://codereview.chromium.org/2620006/diff/15001/16001
File chrome_frame/test/test_server.cc (right):

http://codereview.chromium.org/2620006/diff/15001/16001#newcode226
chrome_frame/test/test_server.cc:226:
std::list<scoped_refptr<SimpleConnection>>::iterator
On 2010/06/08 18:35:07, kkania wrote:
> does this compile?  I thought you need to put a space between the two >>. 
> Although if you typedef the list you don't have to worry about it

Done.

http://codereview.chromium.org/2620006/diff/15001/16001#newcode231
chrome_frame/test/test_server.cc:231: return it;
On 2010/06/08 18:35:07, kkania wrote:
> if you are going to "return it" in both cases, how about just break here...

Done.

http://codereview.chromium.org/2620006/diff/15001/16001#newcode252
chrome_frame/test/test_server.cc:252: scoped_refptr<SimpleConnection> s =
ConnectionFromSocket(socket);
On 2010/06/08 18:35:07, kkania wrote:
> why is this variable named 's'? how about connection

Done.

http://codereview.chromium.org/2620006/diff/15001/16001#newcode273
chrome_frame/test/test_server.cc:273: const char* p = data.c_str() + cur_pos_;
On 2010/06/08 18:35:07, kkania wrote:
> p? how about a better name

Done.

http://codereview.chromium.org/2620006/diff/15001/16002
File chrome_frame/test/test_server.h (right):

http://codereview.chromium.org/2620006/diff/15001/16002#newcode324
chrome_frame/test/test_server.h:324: class SimpleConnection : public
base::RefCounted<SimpleConnection> {
On 2010/06/08 18:35:07, kkania wrote:
> SimpleConnection? this doesn't seem to be any better than Connection2. How
about
> ConfigurableConnection? Can you add a comment too?

Done.

http://codereview.chromium.org/2620006/diff/15001/16002#newcode341
chrome_frame/test/test_server.h:341: // Send HTTP response with provided
|headers| and |content|. Appends
On 2010/06/08 18:35:07, kkania wrote:
> can you add newlines, here and elsewhere, for these cases:
> 1) between constructor/destructor section and methods section
> 2) between methods section and variables section
> 3) between variables section and DISALLOW_COPY... section
> 4) before accessor level declaration (eg private:)

Done.

http://codereview.chromium.org/2620006/diff/15001/16002#newcode345
chrome_frame/test/test_server.h:345: const SendOptions& options =
DefaultSendOptions);
On 2010/06/08 18:35:07, kkania wrote:
> according to the style guide, only case where default args is allowed is to
> simulate variable-length args. Break this into two funcs.

Done.

http://codereview.chromium.org/2620006/diff/15001/16002#newcode349
chrome_frame/test/test_server.h:349: void SendChunk();
On 2010/06/08 18:35:07, kkania wrote:
> comment this

Done.

http://codereview.chromium.org/2620006/diff/15001/16002#newcode353
chrome_frame/test/test_server.h:353: std::string data;
On 2010/06/08 18:35:07, kkania wrote:
> data_

Done.

http://codereview.chromium.org/2620006/diff/15001/16002#newcode358
chrome_frame/test/test_server.h:358: class HTTPTestServer : public
ListenSocket::ListenSocketDelegate {
On 2010/06/08 18:35:07, kkania wrote:
> comment for this class

Done.

http://codereview.chromium.org/2620006/diff/15001/16002#newcode382
chrome_frame/test/test_server.h:382: std::list<scoped_refptr<SimpleConnection>>
connection_list_;
On 2010/06/08 18:35:07, kkania wrote:
> How about typedef this list similar to what SimpleWebServer does?

Done.