Made testserver communicate to parent process with JSON

Made testserver communicate to parent process with JSON

This is so that if the testserver needs to communicate anything more than
the port in the future (e.g., xmpp port for the test sync server), it
can do so in a flexible manner.

BUG=53934
TEST=manually

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

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

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

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

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

[akalin, 2010-11-17 21:11:24]

+cbentzel for review

[cbentzel, 2010-11-17 21:39:20]

Added Pawel, taking a look now.

[cbentzel, 2010-11-17 22:32:50]

http://codereview.chromium.org/5196001/diff/2001/net/test/test_server.cc
File net/test/test_server.cc (right):

http://codereview.chromium.org/5196001/diff/2001/net/test/test_server.cc#newc...
net/test/test_server.cc:285: AppendToPythonPath(third_party_dir);
This should be third_party_dir.Append(FILE_PATH_LITERAL("simplejson"))

http://codereview.chromium.org/5196001/diff/2001/net/test/test_server.cc#newc...
net/test/test_server.cc:382: bool TestServer::ParseServerData(const std::string&
server_data) {
Should this be non-mutating static and have an out-value argument for port?

http://codereview.chromium.org/5196001/diff/2001/net/test/test_server.cc#newc...
net/test/test_server.cc:385: scoped_ptr<Value>
value(json_reader.JsonToValue(server_data, true, true));
Why is "allow trailing comma" needed - the parser does not need to be
permissive.

http://codereview.chromium.org/5196001/diff/2001/net/test/test_server.cc#newc...
net/test/test_server.cc:394: if (!dict->GetInteger("port", &port) ||
I thought you were working on multiple ports? Or is that going to be in a
separate CL?

http://codereview.chromium.org/5196001/diff/2001/net/test/test_server.cc#newc...
net/test/test_server.cc:395: (port <= 0) || (port >= kuint16max)) {
I'd prefer splitting this into two different conditionals - one to check for
existence of "port" and one for doing the range checking.

http://codereview.chromium.org/5196001/diff/2001/net/test/test_server_posix.cc
File net/test/test_server_posix.cc (right):

http://codereview.chromium.org/5196001/diff/2001/net/test/test_server_posix.c...
net/test/test_server_posix.cc:102: bool ReadData(base::TimeDelta*
remaining_time,
General style I've seen is only one anonymous namespace per file. Not sure if
that's a "style guide" standard or just convention.

http://codereview.chromium.org/5196001/diff/2001/net/test/test_server_posix.c...
net/test/test_server_posix.cc:138: file_util::ScopedFD
child_fd_closer(child_fd_closer_.release());
Nice change over explicit reset.

http://codereview.chromium.org/5196001/diff/2001/net/test/test_server_posix.c...
net/test/test_server_posix.cc:143: if (!ReadData(&remaining_time, child_fd_,
sizeof server_data_len,
I haven't seen "sizeof <expression>" before, but it looks valid for unary
expressions.

I'd suggest sizeof(server_data_len) so it looks less like a typo, but not
strongly

http://codereview.chromium.org/5196001/diff/2001/net/test/test_server_posix.c...
net/test/test_server_posix.cc:146: std::string server_data(server_data_len,
'\0');
Why can't you just reserve server_data_len bytes instead of setting characters?
Not worried about perf, but reading this code leads me to believe there will be
<= server_data_len bytes in server_data.

http://codereview.chromium.org/5196001/diff/2001/net/test/test_server_win.cc
File net/test/test_server_win.cc (right):

http://codereview.chromium.org/5196001/diff/2001/net/test/test_server_win.cc#...
net/test/test_server_win.cc:150: bool ReadData(bool *unblocked, HANDLE fd, DWORD
bytes_max, uint8* buffer) {
Nit: bool* unblocked rather than bool *unblocked

http://codereview.chromium.org/5196001/diff/2001/net/tools/testserver/run_tes...
File net/tools/testserver/run_testserver.cc (right):

http://codereview.chromium.org/5196001/diff/2001/net/tools/testserver/run_tes...
net/tools/testserver/run_testserver.cc:27:
(void)logging::InitLogging(FILE_PATH_LITERAL("testserver.log"),
Do this in a separate CL?

http://codereview.chromium.org/5196001/diff/2001/net/tools/testserver/testser...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/5196001/diff/2001/net/tools/testserver/testser...
net/tools/testserver/testserver.py:24: import simplejson as json
Just import simplejson - simple enough to search-and-replace after python2.6
change.

[akalin, 2010-11-17 23:21:35]

Addressed all comments, please take another look.

http://codereview.chromium.org/5196001/diff/2001/net/test/test_server.cc
File net/test/test_server.cc (right):

http://codereview.chromium.org/5196001/diff/2001/net/test/test_server.cc#newc...
net/test/test_server.cc:285: AppendToPythonPath(third_party_dir);
On 2010/11/17 22:32:50, cbentzel wrote:
> This should be third_party_dir.Append(FILE_PATH_LITERAL("simplejson"))

It should not -- the 'simplejson' directory, unlike all the other third_party
python modules, does not have another 'simplejson' directory underneath it so I
needed to include its parents.

http://codereview.chromium.org/5196001/diff/2001/net/test/test_server.cc#newc...
net/test/test_server.cc:382: bool TestServer::ParseServerData(const std::string&
server_data) {
On 2010/11/17 22:32:50, cbentzel wrote:
> Should this be non-mutating static and have an out-value argument for port? 

Hmm, in a future CL, the dict value is stashed in a member variable so clients
can access any non-port data (e.g., xmpp_port)

http://codereview.chromium.org/5196001/diff/2001/net/test/test_server.cc#newc...
net/test/test_server.cc:385: scoped_ptr<Value>
value(json_reader.JsonToValue(server_data, true, true));
On 2010/11/17 22:32:50, cbentzel wrote:
> Why is "allow trailing comma" needed - the parser does not need to be
> permissive.

Done.

http://codereview.chromium.org/5196001/diff/2001/net/test/test_server.cc#newc...
net/test/test_server.cc:394: if (!dict->GetInteger("port", &port) ||
On 2010/11/17 22:32:50, cbentzel wrote:
> I thought you were working on multiple ports? Or is that going to be in a
> separate CL? 

Yeah, it'll be in a separate CL (i'll send it to you, too)

http://codereview.chromium.org/5196001/diff/2001/net/test/test_server.cc#newc...
net/test/test_server.cc:395: (port <= 0) || (port >= kuint16max)) {
On 2010/11/17 22:32:50, cbentzel wrote:
> I'd prefer splitting this into two different conditionals - one to check for
> existence of "port" and one for doing the range checking.

Done.

http://codereview.chromium.org/5196001/diff/2001/net/test/test_server_posix.cc
File net/test/test_server_posix.cc (right):

http://codereview.chromium.org/5196001/diff/2001/net/test/test_server_posix.c...
net/test/test_server_posix.cc:102: bool ReadData(base::TimeDelta*
remaining_time,
On 2010/11/17 22:32:50, cbentzel wrote:
> General style I've seen is only one anonymous namespace per file. Not sure if
> that's a "style guide" standard or just convention.

Yeah...I like putting utility functions close to where they're used.

http://codereview.chromium.org/5196001/diff/2001/net/test/test_server_posix.c...
net/test/test_server_posix.cc:143: if (!ReadData(&remaining_time, child_fd_,
sizeof server_data_len,
On 2010/11/17 22:32:50, cbentzel wrote:
> I haven't seen "sizeof <expression>" before, but it looks valid for unary
> expressions.
> 
> I'd suggest sizeof(server_data_len) so it looks less like a typo, but not
> strongly

Done.

http://codereview.chromium.org/5196001/diff/2001/net/test/test_server_posix.c...
net/test/test_server_posix.cc:146: std::string server_data(server_data_len,
'\0');
On 2010/11/17 22:32:50, cbentzel wrote:
> Why can't you just reserve server_data_len bytes instead of setting
characters?
> Not worried about perf, but reading this code leads me to believe there will
be
> <= server_data_len bytes in server_data.

reserve() doesn't actually change the size of the string.  Although &str[0]
would probably still work, it's probably still technically undefined behavior.

http://codereview.chromium.org/5196001/diff/2001/net/test/test_server_win.cc
File net/test/test_server_win.cc (right):

http://codereview.chromium.org/5196001/diff/2001/net/test/test_server_win.cc#...
net/test/test_server_win.cc:150: bool ReadData(bool *unblocked, HANDLE fd, DWORD
bytes_max, uint8* buffer) {
On 2010/11/17 22:32:50, cbentzel wrote:
> Nit: bool* unblocked rather than bool *unblocked

Done.

http://codereview.chromium.org/5196001/diff/2001/net/tools/testserver/testser...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/5196001/diff/2001/net/tools/testserver/testser...
net/tools/testserver/testserver.py:24: import simplejson as json
On 2010/11/17 22:32:50, cbentzel wrote:
> Just import simplejson - simple enough to search-and-replace after python2.6
> change.

It turns out that we run under both 2.4 and 2.6, and running under the latter
produces some warnings (as simplejson is already included).  I changed this to
have a version check, but in that case it's simpler to import simplejson as json
as they both have identical APIs.

[cbentzel, 2010-11-18 01:47:19]

LGTM

http://codereview.chromium.org/5196001/diff/2001/net/test/test_server.cc
File net/test/test_server.cc (right):

http://codereview.chromium.org/5196001/diff/2001/net/test/test_server.cc#newc...
net/test/test_server.cc:285: AppendToPythonPath(third_party_dir);
On 2010/11/17 23:21:35, akalin wrote:
> On 2010/11/17 22:32:50, cbentzel wrote:
> > This should be third_party_dir.Append(FILE_PATH_LITERAL("simplejson"))
> 
> It should not -- the 'simplejson' directory, unlike all the other third_party
> python modules, does not have another 'simplejson' directory underneath it so
I
> needed to include its parents.

Thanks. Could you expand the "For simplejson" comment about it?

http://codereview.chromium.org/5196001/diff/2001/net/test/test_server_posix.cc
File net/test/test_server_posix.cc (right):

http://codereview.chromium.org/5196001/diff/2001/net/test/test_server_posix.c...
net/test/test_server_posix.cc:102: bool ReadData(base::TimeDelta*
remaining_time,
On 2010/11/17 23:21:35, akalin wrote:
> On 2010/11/17 22:32:50, cbentzel wrote:
> > General style I've seen is only one anonymous namespace per file. Not sure
if
> > that's a "style guide" standard or just convention.
> 
> Yeah...I like putting utility functions close to where they're used.

I prefer your approach as well.

http://codereview.chromium.org/5196001/diff/2001/net/test/test_server_posix.c...
net/test/test_server_posix.cc:146: std::string server_data(server_data_len,
'\0');
On 2010/11/17 23:21:35, akalin wrote:
> On 2010/11/17 22:32:50, cbentzel wrote:
> > Why can't you just reserve server_data_len bytes instead of setting
> characters?
> > Not worried about perf, but reading this code leads me to believe there will
> be
> > <= server_data_len bytes in server_data.
> 
> reserve() doesn't actually change the size of the string.  Although &str[0]
> would probably still work, it's probably still technically undefined behavior.

You're right, I wasn't thinking.

[akalin, 2010-11-18 01:57:19]

On 2010/11/18 01:47:19, cbentzel wrote:
> LGTM
> 
> Thanks. Could you expand the "For simplejson" comment about it?

Done.

Pawel, any comments?

[Paweł Hajdan Jr., 2010-11-18 10:35:12]

http://codereview.chromium.org/5196001/diff/20001/net/test/test_server_posix.cc
File net/test/test_server_posix.cc (right):

http://codereview.chromium.org/5196001/diff/20001/net/test/test_server_posix....
net/test/test_server_posix.cc:100: namespace {
nit: Move the anonymous namespace to the beginning of the file.

http://codereview.chromium.org/5196001/diff/20001/net/test/test_server_posix....
net/test/test_server_posix.cc:102: bool ReadData(base::TimeDelta*
remaining_time,
nit: This function should have a comment.

http://codereview.chromium.org/5196001/diff/20001/net/test/test_server_posix....
net/test/test_server_posix.cc:151: if (!ParseServerData(server_data))
nit: Just return ParseServerData(...).

http://codereview.chromium.org/5196001/diff/20001/net/test/test_server_win.cc
File net/test/test_server_win.cc (right):

http://codereview.chromium.org/5196001/diff/20001/net/test/test_server_win.cc...
net/test/test_server_win.cc:191: if (!ParseServerData(server_data))
nit: Similar comments apply: move anonymous namespace to the beginning of the
file, comment the extracted function, and just return ParseServerData.

http://codereview.chromium.org/5196001/diff/20001/net/tools/testserver/run_te...
File net/tools/testserver/run_testserver.cc (right):

http://codereview.chromium.org/5196001/diff/20001/net/tools/testserver/run_te...
net/tools/testserver/run_testserver.cc:27:
(void)logging::InitLogging(FILE_PATH_LITERAL("testserver.log"),
nit: I think we have a macro to explicitly ignore a value. However, do we want
to do that here? Let's check the return value.

http://codereview.chromium.org/5196001/diff/20001/net/tools/testserver/testse...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/5196001/diff/20001/net/tools/testserver/testse...
net/tools/testserver/testserver.py:30: if sys.version_info < (2, 6):
Hmmm. Can we use the same version of the library, always? "Magic" dependencies
like this can result in a weirdly different behavior, sometimes.

http://codereview.chromium.org/5196001/diff/20001/net/tools/testserver/testse...
net/tools/testserver/testserver.py:1314: print 'sending server_data: %s' %
server_data_json
nit: Only for debugging?

[cbentzel, 2010-11-18 12:34:30]

http://codereview.chromium.org/5196001/diff/20001/net/test/test_server_posix.cc
File net/test/test_server_posix.cc (right):

http://codereview.chromium.org/5196001/diff/20001/net/test/test_server_posix....
net/test/test_server_posix.cc:100: namespace {
On 2010/11/18 10:35:12, Paweł Hajdan Jr. wrote:
> nit: Move the anonymous namespace to the beginning of the file.

This isn't in the style guide - is this just an agreed-upon convention for
Chrome. I actually prefer placing this closer to where it is used [there are
also multiple anonymous namespaces in this file]

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces

[akalin, 2010-11-18 22:38:34]

All comments addressed, please take another look

http://codereview.chromium.org/5196001/diff/20001/net/test/test_server_posix.cc
File net/test/test_server_posix.cc (right):

http://codereview.chromium.org/5196001/diff/20001/net/test/test_server_posix....
net/test/test_server_posix.cc:100: namespace {
On 2010/11/18 10:35:12, Paweł Hajdan Jr. wrote:
> nit: Move the anonymous namespace to the beginning of the file.

Done.

http://codereview.chromium.org/5196001/diff/20001/net/test/test_server_posix....
net/test/test_server_posix.cc:100: namespace {
I'm fine with either way.  I've gotten pushback from my multiple anonymous
namespaces before, but I don't care too much about it.

On 2010/11/18 12:34:30, cbentzel wrote:
> On 2010/11/18 10:35:12, Paweł Hajdan Jr. wrote:
> > nit: Move the anonymous namespace to the beginning of the file.
> 
> This isn't in the style guide - is this just an agreed-upon convention for
> Chrome. I actually prefer placing this closer to where it is used [there are
> also multiple anonymous namespaces in this file]
> 
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces

http://codereview.chromium.org/5196001/diff/20001/net/test/test_server_posix....
net/test/test_server_posix.cc:102: bool ReadData(base::TimeDelta*
remaining_time,
On 2010/11/18 10:35:12, Paweł Hajdan Jr. wrote:
> nit: This function should have a comment.

Done.

http://codereview.chromium.org/5196001/diff/20001/net/test/test_server_posix....
net/test/test_server_posix.cc:151: if (!ParseServerData(server_data))
On 2010/11/18 10:35:12, Paweł Hajdan Jr. wrote:
> nit: Just return ParseServerData(...).

Done.

http://codereview.chromium.org/5196001/diff/20001/net/test/test_server_win.cc
File net/test/test_server_win.cc (right):

http://codereview.chromium.org/5196001/diff/20001/net/test/test_server_win.cc...
net/test/test_server_win.cc:191: if (!ParseServerData(server_data))
On 2010/11/18 10:35:12, Paweł Hajdan Jr. wrote:
> nit: Similar comments apply: move anonymous namespace to the beginning of the
> file, comment the extracted function, and just return ParseServerData.

Done.

http://codereview.chromium.org/5196001/diff/20001/net/tools/testserver/testse...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/5196001/diff/20001/net/tools/testserver/testse...
net/tools/testserver/testserver.py:30: if sys.version_info < (2, 6):
On 2010/11/18 10:35:12, Paweł Hajdan Jr. wrote:
> Hmmm. Can we use the same version of the library, always? "Magic" dependencies
> like this can result in a weirdly different behavior, sometimes.

I get warnings if I try to use simplejson only.  I can revert it if you think we
can live with that, but I think json stuff is low-risk enough for it not to
matter.

[cbentzel, 2010-11-19 14:54:56]

LGTM

Please remove the mention of InitLogging in run_testserver from the CL
description.

http://codereview.chromium.org/5196001/diff/29001/net/test/test_server_posix.cc
File net/test/test_server_posix.cc (right):

http://codereview.chromium.org/5196001/diff/29001/net/test/test_server_posix....
net/test/test_server_posix.cc:59: bool ReadData(base::TimeDelta* remaining_time,
Nit: remaining_time is an input/output argument. According to style guide, these
don't necessarily need to appear after input arguments, but maybe it would make
sense here?

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Param...

[akalin, 2010-11-19 16:45:17]

All comments addressed.  Pawel, anything else?

http://codereview.chromium.org/5196001/diff/29001/net/test/test_server_posix.cc
File net/test/test_server_posix.cc (right):

http://codereview.chromium.org/5196001/diff/29001/net/test/test_server_posix....
net/test/test_server_posix.cc:59: bool ReadData(base::TimeDelta* remaining_time,
On 2010/11/19 14:54:57, cbentzel wrote:
> Nit: remaining_time is an input/output argument. According to style guide,
these
> don't necessarily need to appear after input arguments, but maybe it would
make
> sense here?
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Param...

Done.

[Paweł Hajdan Jr., 2010-11-19 20:59:59]

LGTM

[akalin, 2010-11-25 06:41:28]

Fixed race condition on Windows, re-submitting.  Hopefully it'll stick this
time.

On 2010/11/19 20:59:59, Paweł Hajdan Jr. wrote:
> LGTM

[akalin, 2010-11-25 21:10:33]

Okay, looks like the XP bots are the only ones left choking.  Since I don't have
access to an XP machine, I will split up this CL into a few pieces to narrow
down the problem.  I'll TBR those CLs to cbentzel.

On 2010/11/25 06:41:28, akalin wrote:
> Fixed race condition on Windows, re-submitting.  Hopefully it'll stick this
> time.
> 
> On 2010/11/19 20:59:59, Paweł Hajdan Jr. wrote:
> > LGTM