WebSocket test server migration on ExtensionApiTest.WebSocket

WebSocket test server migration on ExtensionApiTest.WebSocket (fourth trial)

WebSocket test server migration from content::TestWebSocketServer to
net::TestServer.

This is the first change to use net::TestServer as a WebSocket test server.
Other changes will follow to obsoelte content::TestWebSocketServer.

BUG=137639
TEST=browser_tests --gtest_filter='ExtensionApiTest.WebSocket'


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

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

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

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

[Takashi Toyoshima, 2012-10-09 13:54:47]

Hi Ryan, and Sky.
Could you review this CL?

Sky,
This is a change to introduce WebSocket test server migration from
content::TestWebSocketServer to net::TestServer which contains pywebsocket now.
Could you review chrome/ files? Changes on these files are trivial.

Ryan,
Could you review net/ files?
Firstly, I do migration on existing ExtensionApiTest.
After this change, I'll work on Pepper and SSL WebSocket tests.

https://codereview.chromium.org/11087027/diff/1/chrome/test/data/extensions/a...
File chrome/test/data/extensions/api_test/websocket/test.js (left):

https://codereview.chromium.org/11087027/diff/1/chrome/test/data/extensions/a...
chrome/test/data/extensions/api_test/websocket/test.js:38:
chrome.test.getConfig(function(config) {
Oops.
This was wrong.
Old implementation always passes virtually.

[sky, 2012-10-09 16:10:26]

I'm not a good reviewer for this. I randomly added Matt.

[Matt Perry, 2012-10-09 18:22:52]

As long as the tests still pass, LGTM

[Ryan Sleevi, 2012-10-09 19:39:07]

A few nits, hopefully a quick turnaround.

https://codereview.chromium.org/11087027/diff/10001/chrome/browser/extensions...
File chrome/browser/extensions/extension_apitest.h (right):

https://codereview.chromium.org/11087027/diff/10001/chrome/browser/extensions...
chrome/browser/extensions/extension_apitest.h:160: // chrome.test.getConfig().
nit: I think the original comment is cleaner, as it describes what this method
does by itself (eg: independent of StartTestServer), which I think is the right
approach.

https://codereview.chromium.org/11087027/diff/10001/chrome/browser/extensions...
File chrome/browser/extensions/extension_websocket_apitest.cc (right):

https://codereview.chromium.org/11087027/diff/10001/chrome/browser/extensions...
chrome/browser/extensions/extension_websocket_apitest.cc:12:
FilePath().AppendASCII("net/data/websocket")));
Could you explain why you're no longer using PathService? Are you relying on the
TestServer to have a current working directory of some value? If so, I don't
think that's guaranteed by the test server.

If you really did mean to do this, why not

StartWebSocketServer(FILE_PATH_LITERAL("net/data/websocket"));

Although AIUI, including forward slashes in literals is discouraged

FilePath(FILE_PATH_LITERAL("net"))
    .Append(FILE_PATH_LITERAL("data"))
    .Append(FILE_PATH_LITERAL("websocket"));

https://codereview.chromium.org/11087027/diff/10001/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/websocket/test.js (right):

https://codereview.chromium.org/11087027/diff/10001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/websocket/test.js:6: var url =
"ws://localhost:" + port + "/echo";
nit: For some tests, we also parameterize the hostname used by the test server
(either through DNS fakery or for working around some weird winsock issues on
Windows)

A future CL would be good to considering making host and port parameterized

https://codereview.chromium.org/11087027/diff/10001/net/data/websocket/echo_w...
File net/data/websocket/echo_wsh.py (right):

https://codereview.chromium.org/11087027/diff/10001/net/data/websocket/echo_w...
net/data/websocket/echo_wsh.py:47: request.ws_stream.send_message(line,
binary=True)
nit: I'm never sure what the prevailing wind for Chromium Python style is, but
this does look "different" than the other Python files (four space vs two space,
camel caps for methods, no docblocks, etc)

Please check with someone who has Python readability to review this.

[Takashi Toyoshima, 2012-10-10 01:34:48]

+akalin for Python readability check.

Akalin, I think major part of testserver.py was written by you. Could you also
review on net/data/websocket/echo_wsh.py as a Python expert?

https://codereview.chromium.org/11087027/diff/10001/chrome/browser/extensions...
File chrome/browser/extensions/extension_apitest.h (right):

https://codereview.chromium.org/11087027/diff/10001/chrome/browser/extensions...
chrome/browser/extensions/extension_apitest.h:160: // chrome.test.getConfig().
OK, I revert this.

https://codereview.chromium.org/11087027/diff/10001/chrome/browser/extensions...
File chrome/browser/extensions/extension_websocket_apitest.cc (right):

https://codereview.chromium.org/11087027/diff/10001/chrome/browser/extensions...
chrome/browser/extensions/extension_websocket_apitest.cc:12:
FilePath().AppendASCII("net/data/websocket")));
I follows BrowserTestBase::CreateTestServer() style here.
I'm not sure why they use relative path, but this runs in the same situation, so
I just keep consistency. I think we should fix them to use PathService and
FilePath().Append()... style.

So could you allow me to keep this as is for now?

https://codereview.chromium.org/11087027/diff/10001/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/websocket/test.js (right):

https://codereview.chromium.org/11087027/diff/10001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/websocket/test.js:6: var url =
"ws://localhost:" + port + "/echo";
I see.
Thank you for advice.

https://codereview.chromium.org/11087027/diff/10001/net/data/websocket/echo_w...
File net/data/websocket/echo_wsh.py (right):

https://codereview.chromium.org/11087027/diff/10001/net/data/websocket/echo_w...
net/data/websocket/echo_wsh.py:47: request.ws_stream.send_message(line,
binary=True)
Oops.
I forgot that Chromium style was different from pywebsocket.

[Takashi Toyoshima, 2012-10-10 04:14:41]

https://codereview.chromium.org/11087027/diff/10001/chrome/browser/extensions...
File chrome/browser/extensions/extension_websocket_apitest.cc (right):

https://codereview.chromium.org/11087027/diff/10001/chrome/browser/extensions...
chrome/browser/extensions/extension_websocket_apitest.cc:12:
FilePath().AppendASCII("net/data/websocket")));
The reason of relative path exists in LocalTestServer::Init().
There, document_root.IsAbosolute() check the specified path.
So, the path should be relative here.
Also, BrowserTestBase comment says it's relative to src.

[Ryan Sleevi, 2012-10-10 18:13:30]

lgtm

[commit-bot: I haz the power, 2012-10-11 04:18:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/11087027/6005

[commit-bot: I haz the power, 2012-10-11 06:10:48]

Change committed as 161304

[commit-bot: I haz the power, 2012-10-12 06:36:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/11087027/16001

[commit-bot: I haz the power, 2012-10-12 09:19:33]

Change committed as 161542

[commit-bot: I haz the power, 2012-10-12 14:02:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/11087027/16001

[commit-bot: I haz the power, 2012-10-12 14:02:28]

Change committed as 161572

[Takashi Toyoshima, 2012-10-13 16:10:29]

Debug build failed because of DCHECK failure.
This is because testWebSocketPort was not registered to test.json.
The problem existed before this CL, but as I said in the first comment, another
bug which skiped this test hid this problem.

I updaded test.json and set default value 0 in order to avoid another DCHECK
failure against missing property.

[commit-bot: I haz the power, 2012-10-13 18:37:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/11087027/25001

[commit-bot: I haz the power, 2012-10-13 20:52:21]

Change committed as 161773