Make the TestServer use an absolute document root path.

Make the TestServer use an absolute document root path.

This change allows having ScopedTempDirs outside the source dir as the docroot,
so that the files being served can be modified during tests.

BUG=None
TEST=Nothing breaks, tests with TestServers still pass

[Joao da Silva, 2013-02-08 16:50:21]

Hi Paweł, can you review this?

I went through git history looking for a reason for the relative path, and
AFAICT it was just initially implemented this way and eventually it was
documented that it must be so; but the code should still work with absolute
paths outside the src dir, and my tests still pass when I do that.

I'd like to allow absolute paths outside the src dir so that tests can create
scoped_temp_dirs outside the src dir and modify the files served from there. Let
me know if this isn't the best solution for that.

@Mattias: FYI, and please review the policy/ change.

Thanks!

[Paweł Hajdan Jr., 2013-02-11 09:02:40]

https://codereview.chromium.org/12210088/diff/1/net/test/local_test_server.h
File net/test/local_test_server.h (right):

https://codereview.chromium.org/12210088/diff/1/net/test/local_test_server.h#...
net/test/local_test_server.h:33: // |document_root| must be an absolute path, or
a relative path under the
No "or" here. Please standardize on either absolute or relative paths and
enforce it. I keep saying this to various people trying to modify this code.

[Joao da Silva, 2013-02-11 12:54:56]

@Pawel: I've made the TestServer require an absolute path. To make the
refactoring easier I've also added a GetSourcePath() helper to get
DIR_SOURCE_ROOT; this is required mostly for classes that initialize a
TestServer in a constructor initializer list.

"chrome/test/data" is the most common docroot used, so I've also added a helper
for that. Everything else gets a relative path under GetSourcePath().

If you agree to this change and the bots like it too then I'll ask a src/OWNER
to review this as well.

Please have a look, thanks!

https://codereview.chromium.org/12210088/diff/1/net/test/local_test_server.h
File net/test/local_test_server.h (right):

https://codereview.chromium.org/12210088/diff/1/net/test/local_test_server.h#...
net/test/local_test_server.h:33: // |document_root| must be an absolute path, or
a relative path under the
On 2013/02/11 09:02:40, Paweł Hajdan Jr. wrote:
> No "or" here. Please standardize on either absolute or relative paths and
> enforce it. I keep saying this to various people trying to modify this code.

Ah interesting, so more people would like to have this, that's great :-)

[Paweł Hajdan Jr., 2013-02-11 15:36:53]

https://codereview.chromium.org/12210088/diff/7050/net/test/base_test_server.cc
File net/test/base_test_server.cc (right):

https://codereview.chromium.org/12210088/diff/7050/net/test/base_test_server....
net/test/base_test_server.cc:210: CHECK(PathService::Get(base::DIR_SOURCE_ROOT,
&src_dir));
Is it really needed to create a wrapper like that around PathService?

https://codereview.chromium.org/12210088/diff/7050/net/test/base_test_server....
net/test/base_test_server.cc:216: return
GetSourcePath().Append(FILE_PATH_LITERAL("chrome/test/data"));
net shouldn't know about chrome

https://codereview.chromium.org/12210088/diff/7050/net/test/base_test_server.h
File net/test/base_test_server.h (right):

https://codereview.chromium.org/12210088/diff/7050/net/test/base_test_server....
net/test/base_test_server.h:175: static base::FilePath GetSourcePath();
nit: These need comments.

https://codereview.chromium.org/12210088/diff/7050/net/test/remote_test_server.h
File net/test/remote_test_server.h (right):

https://codereview.chromium.org/12210088/diff/7050/net/test/remote_test_serve...
net/test/remote_test_server.h:21: // |document_root| must be an absolute path.
I just realized this may be a problem for the use case of RemoteTestServer. Make
sure people using it are OK with this.