Replace WorkerWebSocketHttpLayoutTest with WorkerTest.WebSocketSharedWorker

Replace WorkerWebSocketHttpLayoutTest with WorkerTest.WebSocketSharedWorker.

WorkerWebSocketHttpLayoutTest was disable.
Actually, shared-worker-simple.html is the only test which is useful to run now.
Also, providing fixed port for WebSocket test server is not so easy.
So, I decided to remove existing WorkerWebSocketHttpLayoutTest, then introduce
WorkerTest.WebSocketSharedWorker as a compatible test with shared-worker-simple.html.

BUG=155014, 137639
TEST=content_browsertests --gtest_filter='WorkerTest.WebSocketSharedWorker'

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

[Takashi Toyoshima, 2012-10-15 07:07:04]

Hi, bashi and yutak.
Could you take a look?

WorkerWebSocketHttpLayoutTest is disabled for a long time.
And actually, these tests are meaningless except for shared-worker-simple.html
because other cases can be covered by WebKit layout tests now.

Additionally, newly introduced net::TestServer doesn't support fixed port for
serving WebSocket service. This feature is required to run WebKit layout tests.
To prepare a change to support fixed port, we should make hack for remote
testing, and looks a cost for only one test.

So I decide to remove them. Instead of layout tests, I will introduce one
WorkerTest for shared worker. It's written in typical browser test style and
compatible with old shared-worker-simple.html layout test.

I hope you guys check this new test is compatible enough. After your check, I'll
ask OWNERS to take a look.

Thanks.

[Yuta Kitamura, 2012-10-15 07:44:37]

Generally looks fine. Just a few comments.

https://codereview.chromium.org/11028111/diff/19005/net/data/websocket/websoc...
File net/data/websocket/websocket_shared_worker.html (right):

https://codereview.chromium.org/11028111/diff/19005/net/data/websocket/websoc...
net/data/websocket/websocket_shared_worker.html:6: {
nit: I think this brace needs to be placed at the end of the previous line.

https://codereview.chromium.org/11028111/diff/19005/net/data/websocket/websoc...
File net/data/websocket/websocket_worker_simple.js (right):

https://codereview.chromium.org/11028111/diff/19005/net/data/websocket/websoc...
net/data/websocket/websocket_worker_simple.js:24: // Receive echoed "hello".
Don't you check the received message?

https://codereview.chromium.org/11028111/diff/19005/net/data/websocket/websoc...
net/data/websocket/websocket_worker_simple.js:27: ws.onclose = function(e) {
Also it's nice to check |e| if possible.

[bashi, 2012-10-15 08:02:20]

LGTM.

https://codereview.chromium.org/11028111/diff/19005/content/browser/worker_ho...
File content/browser/worker_host/test/worker_browsertest.cc (right):

https://codereview.chromium.org/11028111/diff/19005/content/browser/worker_ho...
content/browser/worker_host/test/worker_browsertest.cc:456: FilePath
test_file_path = content::GetTestFilePath("workers", "");
Is |test_file_path| used?

[Takashi Toyoshima, 2012-10-15 08:14:34]

Fixed.
Also update TEST line of description to point new test.

https://codereview.chromium.org/11028111/diff/19005/content/browser/worker_ho...
File content/browser/worker_host/test/worker_browsertest.cc (right):

https://codereview.chromium.org/11028111/diff/19005/content/browser/worker_ho...
content/browser/worker_host/test/worker_browsertest.cc:456: FilePath
test_file_path = content::GetTestFilePath("workers", "");
Good catch!
I will remove this line.
I took a new direction and it was needed for old one because net::TestServer
requires relative path against test files.

https://codereview.chromium.org/11028111/diff/19005/net/data/websocket/websoc...
File net/data/websocket/websocket_shared_worker.html (right):

https://codereview.chromium.org/11028111/diff/19005/net/data/websocket/websoc...
net/data/websocket/websocket_shared_worker.html:6: {
This style follows chrome/test/data/workers/*.html.
function definition requires line break before brace, and assign like var foo =
function() {...} doesn't as line 18.

So, I'd like to keep this style for consistency.

https://codereview.chromium.org/11028111/diff/19005/net/data/websocket/websoc...
File net/data/websocket/websocket_worker_simple.js (right):

https://codereview.chromium.org/11028111/diff/19005/net/data/websocket/websoc...
net/data/websocket/websocket_worker_simple.js:24: // Receive echoed "hello".
On 2012/10/15 07:44:37, Yuta Kitamura wrote:
> Don't you check the received message?

Done.

https://codereview.chromium.org/11028111/diff/19005/net/data/websocket/websoc...
net/data/websocket/websocket_worker_simple.js:27: ws.onclose = function(e) {
I'll check e.wasClean here.

If you think we should check also code and reason, I need to import another
pywebsocket handler for checking closing handshake.
Can I omit them?

[Takashi Toyoshima, 2012-10-15 08:24:11]

Hi jianli,
Could you review this CL on worker_browsertest.cc ?
I will remove WebSocket layout tests and introduce another specific WorkerTest
for shared worker.

Hi, Ryan,
This is a part of net::TestServer migration CLs.
Could you take a look as a net/ owner?

[Yuta Kitamura, 2012-10-15 08:45:19]

Patch Set 6 LGTM

[Ryan Sleevi, 2012-10-15 16:52:27]

rubberstamp lgtm

[Takashi Toyoshima, 2012-10-16 01:15:42]

+levin

Hi jianli and levin,
Could you take a look this CL as content/browser/worker_host/test/OWNERS?

I already get LGTM from network guys.

[jianli, 2012-10-16 21:37:27]

lgtm

[commit-bot: I haz the power, 2012-10-17 06:39:23]

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

[commit-bot: I haz the power, 2012-10-17 08:35:01]

Change committed as 162326