GTTF: Convert WebContentsImplBrowserTest to use EmbeddedTestServer.

GTTF: Convert WebContentsImplBrowserTest to use EmbeddedTestServer.

This eliminates a whole class of problems with SpawnedTestServer
failing to start.

BUG=96594, 230200
R=avi@chromium.org, satorux@chromium.org

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

[Avi (use Gerrit), 2013-05-15 21:55:41]

lgtm with nit

<meme template="cheering minions">
EmbeddedTestServer!
</meme>

https://codereview.chromium.org/14914010/diff/1/content/browser/web_contents/...
File content/browser/web_contents/web_contents_impl_browsertest.cc (right):

https://codereview.chromium.org/14914010/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_impl_browsertest.cc:20: #include
"net/test/spawned_test_server/spawned_test_server.h"
are we using this include any more?

[satorux1, 2013-05-16 04:57:58]

This change is great but I have some comments for the embedded test server. I'd
suggest to create a separate patch for the changes in the embedded test server. 
I think the new feature in the embedded test server deserves a separate patch.
:)

https://codereview.chromium.org/14914010/diff/1/content/test/content_browser_...
File content/test/content_browser_test.cc (right):

https://codereview.chromium.org/14914010/diff/1/content/test/content_browser_...
content/test/content_browser_test.cc:47:
EXPECT_TRUE(embedded_test_server()->ServeFilesFromDirectory(
Not sure if it's a good idea to use EXPECT* in the test constructor. If this
fails, is this considered a test failure? The semantic is unclear to me. 

I'd suggest

FilePath test_data_dir_path;
CHECK(PathService::Get(DIR_TEST_DATA, &test_dir_path));
embedded_test_server()->ServeFilesFromDirectory(test_data_dir_path);

https://codereview.chromium.org/14914010/diff/1/net/test/embedded_test_server...
File net/test/embedded_test_server/embedded_test_server.cc (right):

https://codereview.chromium.org/14914010/diff/1/net/test/embedded_test_server...
net/test/embedded_test_server/embedded_test_server.cc:41:
scoped_ptr<HttpResponse> HandleFileRequest(const base::FilePath& file_path,
function comment is missing.

https://codereview.chromium.org/14914010/diff/1/net/test/embedded_test_server...
net/test/embedded_test_server/embedded_test_server.cc:41:
scoped_ptr<HttpResponse> HandleFileRequest(const base::FilePath& file_path,
file_path -> directory_path ?

https://codereview.chromium.org/14914010/diff/1/net/test/embedded_test_server...
net/test/embedded_test_server/embedded_test_server.cc:46: std::string
file_contents;
Maybe: 

// Trim the first byte ('/').

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

https://codereview.chromium.org/14914010/diff/1/net/test/embedded_test_server...
net/test/embedded_test_server/embedded_test_server.h:116: bool
ServeFilesFromDirectory(
I think we need a unit test for this function. EmbeddedTestServer has good test
coverage, and we should keep it. :)

https://codereview.chromium.org/14914010/diff/1/net/test/embedded_test_server...
net/test/embedded_test_server/embedded_test_server.h:117: int root_key, const
base::FilePath& directory) WARN_UNUSED_RESULT;
I think this function should just take a directory path, so that this function
can be used from places where PathService is not involved.

It's just two lines to resolve the PathService path on the caller side anyway.

[Paweł Hajdan Jr., 2013-05-16 21:24:37]

PTAL

I see the point for a separate patch, but I also prefer to have the new
functionality and its usage in a single patch.

Otherwise things easily get tricky when the interfaces change, like here.

[satorux1, 2013-05-17 06:02:25]

LGTM with nits

https://codereview.chromium.org/14914010/diff/1007/net/data/test.html
File net/data/test.html (right):

https://codereview.chromium.org/14914010/diff/1007/net/data/test.html#newcode1
net/data/test.html:1: <p>Hello World!</p>
There seems to be no file directly under 'net/data':

http://src.chromium.org/viewvc/chrome/trunk/src/net/data/

Maybe create a sub directory, like net/data/test?
Alternatively, net/test/data would be a better place? 

I'm also fine with putting this under 'net/data', though.

https://codereview.chromium.org/14914010/diff/1007/net/test/embedded_test_ser...
File net/test/embedded_test_server/embedded_test_server.h (right):

https://codereview.chromium.org/14914010/diff/1007/net/test/embedded_test_ser...
net/test/embedded_test_server/embedded_test_server.h:114: // Registers request
handler which serves files from |directory|.
Would be nice to make it clearer:

For instance, a request to "/foo.html" is served by "foo.html" under
|directory|. Files under sub directories are also handled in the same way (i.e.
"/foo/bar.html" is served by "foo/bar.html" under |directory|).

[satorux1, 2013-05-17 06:04:42]

On 2013/05/16 21:24:37, Paweł Hajdan Jr. wrote:
> PTAL
> 
> I see the point for a separate patch, but I also prefer to have the new
> functionality and its usage in a single patch.
> 
> Otherwise things easily get tricky when the interfaces change, like here.

FWIW, with git, interface changes like this is easy to manage locally, but I'm
fine with not splitting.

[Paweł Hajdan Jr., 2013-05-17 17:34:07]

https://codereview.chromium.org/14914010/diff/1007/net/test/embedded_test_ser...
File net/test/embedded_test_server/embedded_test_server.h (right):

https://codereview.chromium.org/14914010/diff/1007/net/test/embedded_test_ser...
net/test/embedded_test_server/embedded_test_server.h:114: // Registers request
handler which serves files from |directory|.
On 2013/05/17 06:02:26, satorux1 wrote:
> Would be nice to make it clearer:
> 
> For instance, a request to "/foo.html" is served by "foo.html" under
> |directory|. Files under sub directories are also handled in the same way
(i.e.
> "/foo/bar.html" is served by "foo/bar.html" under |directory|).

Done.

[satorux1, 2013-05-20 03:48:19]

LGTM. Thank you for updating the comment.

[Paweł Hajdan Jr., 2013-05-20 16:45:42]

Committed patchset #8 manually as r201099.