Run safebrowsing_service_test through the net testserver code.

Run safebrowsing_service_test through the net testserver code.

Allows us to use ephemeral ports.

Also run the testserver.py python process with -u to avoid buffering issues.

BUG=96459, 119403
TEST=none

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

[mattm, 2012-04-14 02:24:59]

Hi Pawel,

Would like your thoughts on this.  I'm working on de-flaking this test, and two
of the issues were:
a. Using fixed port.  It looked like the easiest way to fix this is to add
safebrowsing as another server type to the testserver since it already has all
the logic for passing back the actual port number.   But let me know if you
think there is a better approach.

b. Incomplete tracebacks in the logs.  I already committed r132028 to fix this
for safe_browsing_test.cc, but that code gets deleted with this change so I
added it to the main test server launching code.  (See logs like
http://chromium-build-logs.appspot.com/viewlog/raw/AMIfv950hfXLql2hmtGtzrwP_e...
for an example of the issue that is addressing.)

[mattm, 2012-04-14 02:51:43]

On 2012/04/14 02:24:59, mattm wrote:
> Hi Pawel,
> 
> Would like your thoughts on this.  I'm working on de-flaking this test, and
two
> of the issues were:
> a. Using fixed port.  It looked like the easiest way to fix this is to add
> safebrowsing as another server type to the testserver since it already has all
> the logic for passing back the actual port number.   But let me know if you
> think there is a better approach.
> 
> b. Incomplete tracebacks in the logs.  I already committed r132028 to fix this
> for safe_browsing_test.cc, but that code gets deleted with this change so I
> added it to the main test server launching code.  (See logs like
>
http://chromium-build-logs.appspot.com/viewlog/raw/AMIfv950hfXLql2hmtGtzrwP_e...
> for an example of the issue that is addressing.)

Oh, forgot to mention this requires a few changes to safebrowsing_test_server.py
that I'm still upstreaming.

[mattm, 2012-04-16 23:23:18]

On 2012/04/14 02:51:43, mattm wrote:
> On 2012/04/14 02:24:59, mattm wrote:
> > Hi Pawel,
> > 
> > Would like your thoughts on this.  I'm working on de-flaking this test, and
> two
> > of the issues were:
> > a. Using fixed port.  It looked like the easiest way to fix this is to add
> > safebrowsing as another server type to the testserver since it already has
all
> > the logic for passing back the actual port number.   But let me know if you
> > think there is a better approach.
> > 
> > b. Incomplete tracebacks in the logs.  I already committed r132028 to fix
this
> > for safe_browsing_test.cc, but that code gets deleted with this change so I
> > added it to the main test server launching code.  (See logs like
> >
>
http://chromium-build-logs.appspot.com/viewlog/raw/AMIfv950hfXLql2hmtGtzrwP_e...
> > for an example of the issue that is addressing.)
> 
> Oh, forgot to mention this requires a few changes to
safebrowsing_test_server.py
> that I'm still upstreaming.

FYI, the changes were landed and DEPS were rolled (r132484).

[Paweł Hajdan Jr., 2012-04-17 06:38:05]

http://codereview.chromium.org/10073033/diff/4001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_test.cc (right):

http://codereview.chromium.org/10073033/diff/4001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_test.cc:213: return server_host_;
nit: Why don't you just propagate things from test_server_? Those local
server_host_ and server_port_ variables might get out of sync.

http://codereview.chromium.org/10073033/diff/4001/net/DEPS
File net/DEPS (right):

http://codereview.chromium.org/10073033/diff/4001/net/DEPS#newcode6
net/DEPS:6: "+third_party/safe_browsing/testing",
nit: Keep it sorted. Make sure this is reviewed by net/ OWNERS.

http://codereview.chromium.org/10073033/diff/4001/net/test/base_test_server.cc
File net/test/base_test_server.cc (right):

http://codereview.chromium.org/10073033/diff/4001/net/test/base_test_server.c...
net/test/base_test_server.cc:146: case TYPE_SAFEBROWSING:
nit: Sort.

http://codereview.chromium.org/10073033/diff/4001/net/test/base_test_server.h
File net/test/base_test_server.h (right):

http://codereview.chromium.org/10073033/diff/4001/net/test/base_test_server.h...
net/test/base_test_server.h:42: TYPE_SAFEBROWSING,
nit: Keep this sorted.

http://codereview.chromium.org/10073033/diff/4001/net/test/local_test_server.cc
File net/test/local_test_server.cc (right):

http://codereview.chromium.org/10073033/diff/4001/net/test/local_test_server....
net/test/local_test_server.cc:193:
AppendToPythonPath(pyproto_dir.AppendASCII("google"));
nit: Move it above "sync/protocol" if possible. Otherwise it's easy to misread a
"chrome/browser/policy/proto/google" path.

http://codereview.chromium.org/10073033/diff/4001/net/test/local_test_server_...
File net/test/local_test_server_posix.cc (right):

http://codereview.chromium.org/10073033/diff/4001/net/test/local_test_server_...
net/test/local_test_server_posix.cc:104: // Make python stdout and stderr
unbuffered, to prevent incomplete stderr on
I'm not sure this is the right fix. How about just making stderr and stdout go
to the same fd?

Also, the comment mentions Windows, but this file is POSIX-specific.

http://codereview.chromium.org/10073033/diff/4001/net/test/local_test_server_...
File net/test/local_test_server_win.cc (right):

http://codereview.chromium.org/10073033/diff/4001/net/test/local_test_server_...
net/test/local_test_server_win.cc:98: // Make python stdout and stderr
unbuffered, to prevent incomplete stderr on
Do not duplicate this code if possible (and it is).

http://codereview.chromium.org/10073033/diff/4001/net/test/remote_test_server.cc
File net/test/remote_test_server.cc (right):

http://codereview.chromium.org/10073033/diff/4001/net/test/remote_test_server...
net/test/remote_test_server.cc:49: case BaseTestServer::TYPE_SAFEBROWSING:
nit: Keep this sorted.

[mattm, 2012-04-18 00:20:00]

+rsleevi for net/

http://codereview.chromium.org/10073033/diff/4001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_test.cc (right):

http://codereview.chromium.org/10073033/diff/4001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_test.cc:213: return server_host_;
On 2012/04/17 06:38:05, Paweł Hajdan Jr. wrote:
> nit: Why don't you just propagate things from test_server_? Those local
> server_host_ and server_port_ variables might get out of sync.

Done.

http://codereview.chromium.org/10073033/diff/4001/net/DEPS
File net/DEPS (right):

http://codereview.chromium.org/10073033/diff/4001/net/DEPS#newcode6
net/DEPS:6: "+third_party/safe_browsing/testing",
On 2012/04/17 06:38:05, Paweł Hajdan Jr. wrote:
> nit: Keep it sorted. Make sure this is reviewed by net/ OWNERS.

Done.

http://codereview.chromium.org/10073033/diff/4001/net/test/base_test_server.cc
File net/test/base_test_server.cc (right):

http://codereview.chromium.org/10073033/diff/4001/net/test/base_test_server.c...
net/test/base_test_server.cc:146: case TYPE_SAFEBROWSING:
On 2012/04/17 06:38:05, Paweł Hajdan Jr. wrote:
> nit: Sort.

Done.

http://codereview.chromium.org/10073033/diff/4001/net/test/base_test_server.h
File net/test/base_test_server.h (right):

http://codereview.chromium.org/10073033/diff/4001/net/test/base_test_server.h...
net/test/base_test_server.h:42: TYPE_SAFEBROWSING,
On 2012/04/17 06:38:05, Paweł Hajdan Jr. wrote:
> nit: Keep this sorted.

Done.

http://codereview.chromium.org/10073033/diff/4001/net/test/local_test_server.cc
File net/test/local_test_server.cc (right):

http://codereview.chromium.org/10073033/diff/4001/net/test/local_test_server....
net/test/local_test_server.cc:193:
AppendToPythonPath(pyproto_dir.AppendASCII("google"));
On 2012/04/17 06:38:05, Paweł Hajdan Jr. wrote:
> nit: Move it above "sync/protocol" if possible. Otherwise it's easy to misread
a
> "chrome/browser/policy/proto/google" path.

Done.

http://codereview.chromium.org/10073033/diff/4001/net/test/local_test_server_...
File net/test/local_test_server_posix.cc (right):

http://codereview.chromium.org/10073033/diff/4001/net/test/local_test_server_...
net/test/local_test_server_posix.cc:104: // Make python stdout and stderr
unbuffered, to prevent incomplete stderr on
On 2012/04/17 06:38:05, Paweł Hajdan Jr. wrote:
> I'm not sure this is the right fix. How about just making stderr and stdout go
> to the same fd?

I think this still wouldn't solve the issue of incomplete output.  (I just took
another look and both stdout and stderr are truncated, but the stdout is just
hard to tell since it's at the end of a super long line.)

> Also, the comment mentions Windows, but this file is POSIX-specific.

fixed.

http://codereview.chromium.org/10073033/diff/4001/net/test/local_test_server_...
File net/test/local_test_server_win.cc (right):

http://codereview.chromium.org/10073033/diff/4001/net/test/local_test_server_...
net/test/local_test_server_win.cc:98: // Make python stdout and stderr
unbuffered, to prevent incomplete stderr on
On 2012/04/17 06:38:05, Paweł Hajdan Jr. wrote:
> Do not duplicate this code if possible (and it is).

Done.

http://codereview.chromium.org/10073033/diff/4001/net/test/remote_test_server.cc
File net/test/remote_test_server.cc (right):

http://codereview.chromium.org/10073033/diff/4001/net/test/remote_test_server...
net/test/remote_test_server.cc:49: case BaseTestServer::TYPE_SAFEBROWSING:
On 2012/04/17 06:38:05, Paweł Hajdan Jr. wrote:
> nit: Keep this sorted.

Done.

[Ryan Sleevi, 2012-04-18 00:45:02]

Please see http://code.google.com/p/chromium/issues/detail?id=119403

This can create some weird layering violations. In effect, this is pushing
details of 'chrome' into 'net/' - at the Protobuf layer and at the 'safebrowsing
details eking into net'.

While I don't want to block this change, since de-flaking tests is a great thing
- and because I'm not actively working on resolving 119403 right now - I don't
think this is a good direction to be heading for new code.

Could you see what effort it would take to subclass TestServer.py and related? I
would imagine you could get away with the python path modifications directly in
the subclass.

http://codereview.chromium.org/10073033/diff/11001/net/test/local_test_server.h
File net/test/local_test_server.h (left):

http://codereview.chromium.org/10073033/diff/11001/net/test/local_test_server...
net/test/local_test_server.h:62: // |command_line|. Return true on success.
This comment is no longer accurate.

While I realize you changed it for Pawel's comments, this now does two things:
It adds the command line arguments for Python AND the command line arguments for
the Python test server.

Perhaps split into two methods, to keep that separation?

[mattm, 2012-08-03 19:41:03]

I took a hack at splitting it up.  Notes:

It only implements LocalSafeBrowsingTestServer.  Haven't really figured out how
the LocalTestServer/RemoteTestServer split should figure in with these
subclasses.  It should be okay since safebrowsing isn't used on android.

There's still a bunch of unnecessary stuff in BaseTestServer and LocalTestServer
that is specific to net/tools/testserver/testserver.py.  Those classes should
probably also be split up later.

http://codereview.chromium.org/10073033/diff/11001/net/test/local_test_server.h
File net/test/local_test_server.h (left):

http://codereview.chromium.org/10073033/diff/11001/net/test/local_test_server...
net/test/local_test_server.h:62: // |command_line|. Return true on success.
On 2012/04/18 00:45:02, Ryan Sleevi wrote:
> This comment is no longer accurate.
> 
> While I realize you changed it for Pawel's comments, this now does two things:
> It adds the command line arguments for Python AND the command line arguments
for
> the Python test server.
> 
> Perhaps split into two methods, to keep that separation?

Done.

[Ryan Sleevi, 2012-08-08 06:15:40]

Matt,

Thanks for taking another stab at this. I think high-level, this approach is
good. I will take a more closer look tomorrow on this, but I did want to give
one bit of feedback regarding how command-line arguments are being passed and to
see what your thoughts are.

http://codereview.chromium.org/10073033/diff/33002/chrome/browser/safe_browsi...
File chrome/browser/safe_browsing/local_safebrowsing_test_server.cc (right):

http://codereview.chromium.org/10073033/diff/33002/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/local_safebrowsing_test_server.cc:56: bool
LocalSafeBrowsingTestServer::AddCommandLineArguments(
Perhaps that, instead of having to override AddCommandLineArguments, have
BaseTestServer's GenerateArguments() call a protected virtual method (default:
no-op) that allows derived classes to append any derived-test specific
arguments.

The reason for doing it that way, rather than via AddCommandLineArguments, is
that GenerateArguments is shared between the LocalTestServer and
RemoteTestServer as the common way for the tests to communicate their args to
"the thing that will spawn tests".

For LocalTestServer, the GenerateArguments dictionary is converted straight into
command-line arguments. For the RemoteTestServer, the GenerateArguments is
passed as JSON in the "start a server of type X" request, and then the
server-spawner converts the JSON into command-line arguments (and adjusting
paths as appropriate) for the test server that will accept the remote
connections.

While this distinction is probably less important for safe browsing (since AIUI,
Android doesn't have safe browsing service), sync could also manage it's
command-line arguments this way, and that is shared.

http://codereview.chromium.org/10073033/diff/33002/chrome/browser/safe_browsi...
File chrome/browser/safe_browsing/safe_browsing_test.cc (right):

http://codereview.chromium.org/10073033/diff/33002/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/safe_browsing_test.cc:136:
InProcessBrowserTest::SetUp();
Running InProcBrowsertest::SetUp() after, rather than before [and conversely, in
TearDown, before rather than after] seems to be contrary to how one would think
of it naturally (ctors = base before derived, dtors = derived before base)

It wasn't clear why this needed to be in SetUp/TearDown as opposed to simply
heap allocating in the SafeBrowsingServiceTest constructor (and letting the dtor
stop)

[mattm, 2012-08-30 21:50:47]

http://codereview.chromium.org/10073033/diff/33002/chrome/browser/safe_browsi...
File chrome/browser/safe_browsing/local_safebrowsing_test_server.cc (right):

http://codereview.chromium.org/10073033/diff/33002/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/local_safebrowsing_test_server.cc:56: bool
LocalSafeBrowsingTestServer::AddCommandLineArguments(
On 2012/08/08 06:15:40, Ryan Sleevi wrote:
> Perhaps that, instead of having to override AddCommandLineArguments, have
> BaseTestServer's GenerateArguments() call a protected virtual method (default:
> no-op) that allows derived classes to append any derived-test specific
> arguments.
> 
> The reason for doing it that way, rather than via AddCommandLineArguments, is
> that GenerateArguments is shared between the LocalTestServer and
> RemoteTestServer as the common way for the tests to communicate their args to
> "the thing that will spawn tests".
> 
> For LocalTestServer, the GenerateArguments dictionary is converted straight
into
> command-line arguments. For the RemoteTestServer, the GenerateArguments is
> passed as JSON in the "start a server of type X" request, and then the
> server-spawner converts the JSON into command-line arguments (and adjusting
> paths as appropriate) for the test server that will accept the remote
> connections.
> 
> While this distinction is probably less important for safe browsing (since
AIUI,
> Android doesn't have safe browsing service), sync could also manage it's
> command-line arguments this way, and that is shared.

That sounds reasonable, changed.

I'm still unsure how the inheritance hierarchy should work out there, though,
since we don't use multiple inheritance.  Maybe instead of having a
LocalSafeBrowsingTestServer, you would have to make a SafeBrowsingTestServer
which inherits from the TestServer typedef and then use ifdefs to only define
the right functions for if you are actually compiling a Remote or Local one?
hmm.

http://codereview.chromium.org/10073033/diff/33002/chrome/browser/safe_browsi...
File chrome/browser/safe_browsing/safe_browsing_test.cc (right):

http://codereview.chromium.org/10073033/diff/33002/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/safe_browsing_test.cc:136:
InProcessBrowserTest::SetUp();
On 2012/08/08 06:15:40, Ryan Sleevi wrote:
> Running InProcBrowsertest::SetUp() after, rather than before [and conversely,
in
> TearDown, before rather than after] seems to be contrary to how one would
think
> of it naturally (ctors = base before derived, dtors = derived before base)
> 
> It wasn't clear why this needed to be in SetUp/TearDown as opposed to simply
> heap allocating in the SafeBrowsingServiceTest constructor (and letting the
dtor
> stop)

It can't do in the constructor, since you can't ASSERT_foo in the constructor..
though it could just attempt to do it in the constructor and then later assert
whether it worked or not..

However I guess it could just do this in SetUpCommandLine instead.  done.

[Ryan Sleevi, 2012-08-31 21:48:39]

+maruel, who I'm volunteering to check/review the Python portion for Python
readability and such

Looks good, but I'll take another stab once maruel has reviewed/it's been
checked for Python, just to make sure I didn't miss anything.

http://codereview.chromium.org/10073033/diff/49003/chrome/browser/safe_browsi...
File chrome/browser/safe_browsing/local_safebrowsing_test_server.cc (right):

http://codereview.chromium.org/10073033/diff/49003/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/local_safebrowsing_test_server.cc:19:
data_file_(data_file) {}
nit: {
}

Per
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constructor_In...
only {} is listed for the one-liners, but that may be an overly strict
interpretation

http://codereview.chromium.org/10073033/diff/49003/chrome/browser/safe_browsi...
File chrome/browser/safe_browsing/safe_browsing_test.cc (right):

http://codereview.chromium.org/10073033/diff/49003/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/safe_browsing_test.cc:251: server_port());
Minimally, this should be

base::StringPrintf("http://%s/safebrowsing",
                   test_server_->host_port_pair().ToString().c_str());

But the correct should be:

test_server_->GetURL("/safebrowsing")

This also applies for all the GURLs below

http://codereview.chromium.org/10073033/diff/49003/net/test/base_test_server.h
File net/test/base_test_server.h (right):

http://codereview.chromium.org/10073033/diff/49003/net/test/base_test_server....
net/test/base_test_server.h:215: base::DictionaryValue* arguments) const
WARN_UNUSED_RESULT;
Is there a reason this is public instead of protected?

http://codereview.chromium.org/10073033/diff/49003/net/test/local_test_server.h
File net/test/local_test_server.h (right):

http://codereview.chromium.org/10073033/diff/49003/net/test/local_test_server...
net/test/local_test_server.h:64: void AddPythonArguments(const FilePath&
testserver_path,
This seems like it should be private. Where is it called publicly?

[M-A Ruel, 2012-08-31 22:04:05]

http://codereview.chromium.org/10073033/diff/49003/net/tools/testserver/tests...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/10073033/diff/49003/net/tools/testserver/tests...
net/tools/testserver/testserver.py:2002: my_data_dir =
os.path.dirname(sys.argv[0])
it's not a good idea in general to do this. If an os.chdir() call occurred
before, sys.argv[0] is not valid anymore to use the file's directory.

The best way is to define a global variable like this at the top:
BASE_DIR = os.path.dirname(os.path.abspath(__file__))

http://codereview.chromium.org/10073033/diff/49003/net/tools/testserver/tests...
net/tools/testserver/testserver.py:2011: def create_server(self, server_data):
This function is really long and doesn't have a docstring, at a quick glance
it's not even clear what this function is returning, it can return None, 1 or an
http server.

I recommend splitting it up.

http://codereview.chromium.org/10073033/diff/49003/net/tools/testserver/tests...
net/tools/testserver/testserver.py:2015: self.__ocsp_server = None
If you want to make this code pylint-clean, which I recommend, you have to set
it to None in the __init__ function.

http://codereview.chromium.org/10073033/diff/49003/net/tools/testserver/tests...
net/tools/testserver/testserver.py:2049: (pem_cert_and_key, ocsp_der) = \
style nit:
(pem_cert_and_key, ocsp_der) = minica.GenerateCertKeyAndOCSP(
then align at +4.

http://codereview.chromium.org/10073033/diff/49003/net/tools/testserver/tests...
net/tools/testserver/testserver.py:2133: if self.__ocsp_server is not None:
if self.__ocsp_server:

http://codereview.chromium.org/10073033/diff/49003/net/tools/testserver/tests...
net/tools/testserver/testserver.py:2138: if self.__ocsp_server is not None:
same

http://codereview.chromium.org/10073033/diff/49003/net/tools/testserver/tests...
File net/tools/testserver/testserver_base.py (right):

http://codereview.chromium.org/10073033/diff/49003/net/tools/testserver/tests...
net/tools/testserver/testserver_base.py:17: import simplejson as json
We do not support python < 2.6 anymore, so please remove the code and just
import json.

http://codereview.chromium.org/10073033/diff/49003/net/tools/testserver/tests...
net/tools/testserver/testserver_base.py:23: class FileMultiplexer:
class FileMultiplexer(object):

and everywhere

http://codereview.chromium.org/10073033/diff/49003/net/tools/testserver/tests...
net/tools/testserver/testserver_base.py:28: def __del__(self) :
Why?

http://codereview.chromium.org/10073033/diff/49003/net/tools/testserver/tests...
net/tools/testserver/testserver_base.py:58: server_data = {}
server_data = {
  'host': self.options.host,
}

http://codereview.chromium.org/10073033/diff/49003/net/tools/testserver/tests...
net/tools/testserver/testserver_base.py:60: 
why

so

much

whitespace?

http://codereview.chromium.org/10073033/diff/49003/net/tools/testserver/tests...
net/tools/testserver/testserver_base.py:72: raise NotImplementedError
raise NotImplementedError()

http://codereview.chromium.org/10073033/diff/49003/net/tools/testserver/tests...
net/tools/testserver/testserver_base.py:82: self.option_parser.add_option('',
'--startup-pipe', type='int',
I'm not sure why all the "'', ".

http://codereview.chromium.org/10073033/diff/49003/net/tools/testserver/tests...
net/tools/testserver/testserver_base.py:90: self.option_parser.add_option('',
'--port', default='0', type='int',
default=0 since you used type int

http://codereview.chromium.org/10073033/diff/49003/net/tools/testserver/tests...
net/tools/testserver/testserver_base.py:103: if self.options.startup_pipe is not
None:
if self.options.startup_pipe:

[mattm, 2012-09-01 01:44:53]

Thanks, updated.

http://codereview.chromium.org/10073033/diff/49003/chrome/browser/safe_browsi...
File chrome/browser/safe_browsing/local_safebrowsing_test_server.cc (right):

http://codereview.chromium.org/10073033/diff/49003/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/local_safebrowsing_test_server.cc:19:
data_file_(data_file) {}
On 2012/08/31 21:48:39, Ryan Sleevi wrote:
> nit: {
> }
> 
> Per
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constructor_In...
> only {} is listed for the one-liners, but that may be an overly strict
> interpretation

Done.

http://codereview.chromium.org/10073033/diff/49003/chrome/browser/safe_browsi...
File chrome/browser/safe_browsing/safe_browsing_test.cc (right):

http://codereview.chromium.org/10073033/diff/49003/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/safe_browsing_test.cc:251: server_port());
On 2012/08/31 21:48:39, Ryan Sleevi wrote:
> Minimally, this should be
> 
> base::StringPrintf("http://%s/safebrowsing",
>                    test_server_->host_port_pair().ToString().c_str());
> 
> But the correct should be:
> 
> test_server_->GetURL("/safebrowsing")
> 
> This also applies for all the GURLs below

Done.

http://codereview.chromium.org/10073033/diff/49003/net/test/base_test_server.h
File net/test/base_test_server.h (right):

http://codereview.chromium.org/10073033/diff/49003/net/test/base_test_server....
net/test/base_test_server.h:215: base::DictionaryValue* arguments) const
WARN_UNUSED_RESULT;
On 2012/08/31 21:48:39, Ryan Sleevi wrote:
> Is there a reason this is public instead of protected?

is protected, though I had it in the wrong section in my derived class. Fixed.

http://codereview.chromium.org/10073033/diff/49003/net/test/local_test_server.h
File net/test/local_test_server.h (right):

http://codereview.chromium.org/10073033/diff/49003/net/test/local_test_server...
net/test/local_test_server.h:64: void AddPythonArguments(const FilePath&
testserver_path,
On 2012/08/31 21:48:39, Ryan Sleevi wrote:
> This seems like it should be private. Where is it called publicly?

Done.

http://codereview.chromium.org/10073033/diff/49003/net/tools/testserver/tests...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/10073033/diff/49003/net/tools/testserver/tests...
net/tools/testserver/testserver.py:2002: my_data_dir =
os.path.dirname(sys.argv[0])
On 2012/08/31 22:04:05, Marc-Antoine Ruel wrote:
> it's not a good idea in general to do this. If an os.chdir() call occurred
> before, sys.argv[0] is not valid anymore to use the file's directory.
> 
> The best way is to define a global variable like this at the top:
> BASE_DIR = os.path.dirname(os.path.abspath(__file__))

Done.

http://codereview.chromium.org/10073033/diff/49003/net/tools/testserver/tests...
net/tools/testserver/testserver.py:2011: def create_server(self, server_data):
On 2012/08/31 22:04:05, Marc-Antoine Ruel wrote:
> This function is really long and doesn't have a docstring, at a quick glance
> it's not even clear what this function is returning, it can return None, 1 or
an
> http server.
> 
> I recommend splitting it up.

Fixed the issue with returning weird things.

For the rest, wellll... it used to be an even larger main() function. I don't
want to get tooo much deeper into refactoring things in this CL..

http://codereview.chromium.org/10073033/diff/49003/net/tools/testserver/tests...
net/tools/testserver/testserver.py:2015: self.__ocsp_server = None
On 2012/08/31 22:04:05, Marc-Antoine Ruel wrote:
> If you want to make this code pylint-clean, which I recommend, you have to set
> it to None in the __init__ function.

Done.

http://codereview.chromium.org/10073033/diff/49003/net/tools/testserver/tests...
net/tools/testserver/testserver.py:2049: (pem_cert_and_key, ocsp_der) = \
On 2012/08/31 22:04:05, Marc-Antoine Ruel wrote:
> style nit:
> (pem_cert_and_key, ocsp_der) = minica.GenerateCertKeyAndOCSP(
> then align at +4.

Done.

http://codereview.chromium.org/10073033/diff/49003/net/tools/testserver/tests...
net/tools/testserver/testserver.py:2133: if self.__ocsp_server is not None:
On 2012/08/31 22:04:05, Marc-Antoine Ruel wrote:
> if self.__ocsp_server:

Done.

http://codereview.chromium.org/10073033/diff/49003/net/tools/testserver/tests...
net/tools/testserver/testserver.py:2138: if self.__ocsp_server is not None:
On 2012/08/31 22:04:05, Marc-Antoine Ruel wrote:
> same

Done.

http://codereview.chromium.org/10073033/diff/49003/net/tools/testserver/tests...
File net/tools/testserver/testserver_base.py (right):

http://codereview.chromium.org/10073033/diff/49003/net/tools/testserver/tests...
net/tools/testserver/testserver_base.py:17: import simplejson as json
On 2012/08/31 22:04:05, Marc-Antoine Ruel wrote:
> We do not support python < 2.6 anymore, so please remove the code and just
> import json.

Done.

http://codereview.chromium.org/10073033/diff/49003/net/tools/testserver/tests...
net/tools/testserver/testserver_base.py:23: class FileMultiplexer:
On 2012/08/31 22:04:05, Marc-Antoine Ruel wrote:
> class FileMultiplexer(object):
> 
> and everywhere

Done.

http://codereview.chromium.org/10073033/diff/49003/net/tools/testserver/tests...
net/tools/testserver/testserver_base.py:28: def __del__(self) :
On 2012/08/31 22:04:05, Marc-Antoine Ruel wrote:
> Why?

I don't know. This is just moved from testserver.py.

http://codereview.chromium.org/10073033/diff/49003/net/tools/testserver/tests...
net/tools/testserver/testserver_base.py:58: server_data = {}
On 2012/08/31 22:04:05, Marc-Antoine Ruel wrote:
> server_data = {
>   'host': self.options.host,
> }

Done.

http://codereview.chromium.org/10073033/diff/49003/net/tools/testserver/tests...
net/tools/testserver/testserver_base.py:60: 
On 2012/08/31 22:04:05, Marc-Antoine Ruel wrote:
> why
> 
> so
> 
> much
> 
> whitespace?

Done.

http://codereview.chromium.org/10073033/diff/49003/net/tools/testserver/tests...
net/tools/testserver/testserver_base.py:72: raise NotImplementedError
On 2012/08/31 22:04:05, Marc-Antoine Ruel wrote:
> raise NotImplementedError()

Done.

http://codereview.chromium.org/10073033/diff/49003/net/tools/testserver/tests...
net/tools/testserver/testserver_base.py:82: self.option_parser.add_option('',
'--startup-pipe', type='int',
On 2012/08/31 22:04:05, Marc-Antoine Ruel wrote:
> I'm not sure why all the "'', ".

dunno either, but fixed.

http://codereview.chromium.org/10073033/diff/49003/net/tools/testserver/tests...
net/tools/testserver/testserver_base.py:90: self.option_parser.add_option('',
'--port', default='0', type='int',
On 2012/08/31 22:04:05, Marc-Antoine Ruel wrote:
> default=0 since you used type int

Done.

http://codereview.chromium.org/10073033/diff/49003/net/tools/testserver/tests...
net/tools/testserver/testserver_base.py:103: if self.options.startup_pipe is not
None:
On 2012/08/31 22:04:05, Marc-Antoine Ruel wrote:
> if self.options.startup_pipe:

0 is a valid file descripton

[mattm, 2012-09-01 01:45:43]

http://codereview.chromium.org/10073033/diff/49003/net/tools/testserver/tests...
File net/tools/testserver/testserver_base.py (right):

http://codereview.chromium.org/10073033/diff/49003/net/tools/testserver/tests...
net/tools/testserver/testserver_base.py:103: if self.options.startup_pipe is not
None:
On 2012/09/01 01:44:54, mattm wrote:
> On 2012/08/31 22:04:05, Marc-Antoine Ruel wrote:
> > if self.options.startup_pipe:
> 
> 0 is a valid file descripton
s/descripton/descriptor/

[M-A Ruel, 2012-09-01 01:47:26]

python lgtm

[Ryan Sleevi, 2012-09-01 02:18:02]

net/ LGTM, mod minor nits

http://codereview.chromium.org/10073033/diff/37006/chrome/browser/safe_browsi...
File chrome/browser/safe_browsing/local_safebrowsing_test_server.h (right):

http://codereview.chromium.org/10073033/diff/37006/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/local_safebrowsing_test_server.h:32: FilePath
data_file_;
nit: include base/file_path.h directly, since you've got it as a member here.

http://codereview.chromium.org/10073033/diff/37006/chrome/browser/safe_browsi...
File chrome/browser/safe_browsing/safe_browsing_test.cc (right):

http://codereview.chromium.org/10073033/diff/37006/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/safe_browsing_test.cc:370: const net::TestServer&
test_server, int test_step) {
nit: per http://www.chromium.org/developers/coding-style, I believe these should
be on separate lines

net::URLRequestStatus FetchDBToVerify(
    const net::TestServer& test_server,
    int test_step) {

Or at least, the relation between the two was non-obvious to me.

[mattm, 2012-09-04 20:01:35]

+thakis for chrome/ OWNERS stamp

[Nico, 2012-09-04 20:11:20]

lgtm stamp

http://codereview.chromium.org/10073033/diff/41006/chrome/browser/safe_browsi...
File chrome/browser/safe_browsing/safe_browsing_testserver.py (right):

http://codereview.chromium.org/10073033/diff/41006/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/safe_browsing_testserver.py:18: class
ServerRunner(testserver_base.TestServerRunner):
docstring

[commit-bot: I haz the power, 2012-09-04 21:35:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/10073033/38036

[commit-bot: I haz the power, 2012-09-05 00:19:44]

Change committed as 154861