Enable WebSocket in test_shell

Enable WebSocket in test_shell

BUG=12497, 24756
TEST=LayoutTests/fast/websockets success

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

[ukai, 2009-10-06 09:14:52]

This CL depends on 243077 and 251087 and webkit bug 29841.

[Yuzo, 2009-10-06 09:33:02]

LGTM, but my review is limited to stylistic things
because I know almost nothing about test_shell.

http://codereview.chromium.org/243108/diff/1/4
File webkit/api/public/WebSocketStreamHandleClient.h (right):

http://codereview.chromium.org/243108/diff/1/4#newcode42
Line 42: virtual void willOpenStream(WebSocketStreamHandle*, const WebURL&) = 0;
Comment for this method?

http://codereview.chromium.org/243108/diff/1/4#newcode47
Line 47: virtual void didSendData(WebSocketStreamHandle*, int) = 0;
Ditto.

http://codereview.chromium.org/243108/diff/1/4#newcode55
Line 55: virtual void didFail(WebSocketStreamHandle*, const
WebSocketStreamError&) = 0;
Ditto.

[tyoshino (SeeGerritForStatus), 2009-10-06 10:27:30]

http://codereview.chromium.org/243108/diff/1/2
File webkit/api/public/WebSocketStreamError.h (right):

http://codereview.chromium.org/243108/diff/1/2#newcode29
Line 29: */
blank line here please

http://codereview.chromium.org/243108/diff/1/2#newcode45
Line 45: #endif  // WebSocketStreamError_h
single space before the comment.

http://codereview.chromium.org/243108/diff/1/4
File webkit/api/public/WebSocketStreamHandleClient.h (right):

http://codereview.chromium.org/243108/diff/1/4#newcode29
Line 29: */
blank line here please

http://codereview.chromium.org/243108/diff/1/6
File webkit/glue/websocketstreamhandle_delegate.h (right):

http://codereview.chromium.org/243108/diff/1/6#newcode1
Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved.  Use of
this
Return right before "Use of ..." sentense in this line for consistency.

http://codereview.chromium.org/243108/diff/1/9
File webkit/tools/test_shell/simple_socket_stream_bridge.cc (right):

http://codereview.chromium.org/243108/diff/1/9#newcode4
Line 4: //
extra comment line

http://codereview.chromium.org/243108/diff/1/9#newcode204
Line 204: }  // anonymous namespace
just // namespace if this file follows Chromium coding style.

http://codereview.chromium.org/243108/diff/1/10
File webkit/tools/test_shell/simple_socket_stream_bridge.h (right):

http://codereview.chromium.org/243108/diff/1/10#newcode6
Line 6: #define WEBKIT_TOOLS_TEST_SHELL_SIMPLE_SOCKET_STREAM_BRIDGE_H__
let's use single-underscore guard

[ukai, 2009-10-06 10:41:21]

http://codereview.chromium.org/243108/diff/1/2
File webkit/api/public/WebSocketStreamError.h (right):

http://codereview.chromium.org/243108/diff/1/2#newcode29
Line 29: */
On 2009/10/06 10:27:31, tyoshino wrote:
> blank line here please

Done.

http://codereview.chromium.org/243108/diff/1/2#newcode45
Line 45: #endif  // WebSocketStreamError_h
On 2009/10/06 10:27:31, tyoshino wrote:
> single space before the comment.

Seems most headers in this directory don't have this comment.
Removed.

http://codereview.chromium.org/243108/diff/1/4
File webkit/api/public/WebSocketStreamHandleClient.h (right):

http://codereview.chromium.org/243108/diff/1/4#newcode29
Line 29: */
On 2009/10/06 10:27:31, tyoshino wrote:
> blank line here please

Done.

http://codereview.chromium.org/243108/diff/1/4#newcode42
Line 42: virtual void willOpenStream(WebSocketStreamHandle*, const WebURL&) = 0;
On 2009/10/06 09:33:02, Yuzo wrote:
> Comment for this method?

Done.

http://codereview.chromium.org/243108/diff/1/4#newcode47
Line 47: virtual void didSendData(WebSocketStreamHandle*, int) = 0;
On 2009/10/06 09:33:02, Yuzo wrote:
> Ditto.

Done.

http://codereview.chromium.org/243108/diff/1/4#newcode55
Line 55: virtual void didFail(WebSocketStreamHandle*, const
WebSocketStreamError&) = 0;
On 2009/10/06 09:33:02, Yuzo wrote:
> Ditto.

Done.

http://codereview.chromium.org/243108/diff/1/6
File webkit/glue/websocketstreamhandle_delegate.h (right):

http://codereview.chromium.org/243108/diff/1/6#newcode1
Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved.  Use of
this
On 2009/10/06 10:27:31, tyoshino wrote:
> Return right before "Use of ..." sentense in this line for consistency.

Done.

http://codereview.chromium.org/243108/diff/1/9
File webkit/tools/test_shell/simple_socket_stream_bridge.cc (right):

http://codereview.chromium.org/243108/diff/1/9#newcode4
Line 4: //
On 2009/10/06 10:27:31, tyoshino wrote:
> extra comment line

Done.

http://codereview.chromium.org/243108/diff/1/9#newcode204
Line 204: }  // anonymous namespace
On 2009/10/06 10:27:31, tyoshino wrote:
> just // namespace if this file follows Chromium coding style.

Done.

http://codereview.chromium.org/243108/diff/1/10
File webkit/tools/test_shell/simple_socket_stream_bridge.h (right):

http://codereview.chromium.org/243108/diff/1/10#newcode6
Line 6: #define WEBKIT_TOOLS_TEST_SHELL_SIMPLE_SOCKET_STREAM_BRIDGE_H__
On 2009/10/06 10:27:31, tyoshino wrote:
> let's use single-underscore guard

Done.

[tyoshino (SeeGerritForStatus), 2009-10-08 05:21:17]

LGTM

http://codereview.chromium.org/243108/diff/7001/7002
File webkit/api/public/WebSocketStreamError.h (right):

http://codereview.chromium.org/243108/diff/7001/7002#newcode44
Line 44: }  // namespace WebKit
Single space before comment please. I looked at other files in this dir. Most
did so.

http://codereview.chromium.org/243108/diff/7001/7003
File webkit/api/public/WebSocketStreamHandle.h (right):

http://codereview.chromium.org/243108/diff/7001/7003#newcode55
Line 55: }  // namespace WebKit
ditto

http://codereview.chromium.org/243108/diff/7001/7004
File webkit/api/public/WebSocketStreamHandleClient.h (right):

http://codereview.chromium.org/243108/diff/7001/7004#newcode64
Line 64: }  // namespace WebKit
ditto

http://codereview.chromium.org/243108/diff/7001/7009
File webkit/tools/test_shell/simple_socket_stream_bridge.cc (right):

http://codereview.chromium.org/243108/diff/7001/7009#newcode161
Line 161: return;
indentation

[darin (slow to review), 2009-10-08 06:15:09]

http://codereview.chromium.org/243108/diff/7001/7002
File webkit/api/public/WebSocketStreamError.h (right):

http://codereview.chromium.org/243108/diff/7001/7002#newcode40
Line 40: // TODO(ukai): Define SocketStream Error codes.
nit: please use FIXME instead of TODO since this file is supposed to conform to
webkit style.

http://codereview.chromium.org/243108/diff/7001/7002#newcode41
Line 41: virtual int errorCode() const;
should this method be pure virtual instead?  where is the implementation?

http://codereview.chromium.org/243108/diff/7001/7004
File webkit/api/public/WebSocketStreamHandleClient.h (right):

http://codereview.chromium.org/243108/diff/7001/7004#newcode47
Line 47: virtual void didOpen(WebSocketStreamHandle*, int /*
max_pending_send_allowed */) = 0;
should this be called didOpenStream to match willOpenStream?  or should
willOpenStream just be called willOpen?

nit: max_pending_send_allowed is incorrectly using google style.  same for
amount_sent below.

http://codereview.chromium.org/243108/diff/7001/7004#newcode61
Line 61: // TODO(ukai): auth challenge for proxy
FIXME

http://codereview.chromium.org/243108/diff/7001/7006
File webkit/glue/websocketstreamhandle_delegate.h (right):

http://codereview.chromium.org/243108/diff/7001/7006#newcode12
Line 12: }  // namespace WebKit
nit: no need for the comment here.

http://codereview.chromium.org/243108/diff/7001/7006#newcode17
Line 17: public:
nit: indent by 1 space

http://codereview.chromium.org/243108/diff/7001/7006#newcode18
Line 18: WebSocketStreamHandleDelegate() {}
nit: indent by 2 spaces

[ukai, 2009-10-08 07:26:52]

Thanks for review!

I also add WebSocketStreamHandleImpl in this CL, so that test_shell can use
WebSocket with CL in 243077 and 251087.

http://codereview.chromium.org/243108/diff/7001/7002
File webkit/api/public/WebSocketStreamError.h (right):

http://codereview.chromium.org/243108/diff/7001/7002#newcode40
Line 40: // TODO(ukai): Define SocketStream Error codes.
On 2009/10/08 06:15:09, darin wrote:
> nit: please use FIXME instead of TODO since this file is supposed to conform
to
> webkit style.

Done.

http://codereview.chromium.org/243108/diff/7001/7002#newcode41
Line 41: virtual int errorCode() const;
On 2009/10/08 06:15:09, darin wrote:
> should this method be pure virtual instead?  where is the implementation?

Ah, it was not used yet. removed and comment it as FIXME.

http://codereview.chromium.org/243108/diff/7001/7002#newcode44
Line 44: }  // namespace WebKit
On 2009/10/08 05:21:17, tyoshino wrote:
> Single space before comment please. I looked at other files in this dir. Most
> did so.

Done.

http://codereview.chromium.org/243108/diff/7001/7003
File webkit/api/public/WebSocketStreamHandle.h (right):

http://codereview.chromium.org/243108/diff/7001/7003#newcode55
Line 55: }  // namespace WebKit
On 2009/10/08 05:21:17, tyoshino wrote:
> ditto

Done.

http://codereview.chromium.org/243108/diff/7001/7004
File webkit/api/public/WebSocketStreamHandleClient.h (right):

http://codereview.chromium.org/243108/diff/7001/7004#newcode47
Line 47: virtual void didOpen(WebSocketStreamHandle*, int /*
max_pending_send_allowed */) = 0;
On 2009/10/08 06:15:09, darin wrote:
> should this be called didOpenStream to match willOpenStream?  or should
> willOpenStream just be called willOpen?
> 
> nit: max_pending_send_allowed is incorrectly using google style.  same for
> amount_sent below.

Done.

http://codereview.chromium.org/243108/diff/7001/7004#newcode61
Line 61: // TODO(ukai): auth challenge for proxy
On 2009/10/08 06:15:09, darin wrote:
> FIXME

Done.

http://codereview.chromium.org/243108/diff/7001/7004#newcode64
Line 64: }  // namespace WebKit
On 2009/10/08 05:21:17, tyoshino wrote:
> ditto

Done

http://codereview.chromium.org/243108/diff/7001/7006
File webkit/glue/websocketstreamhandle_delegate.h (right):

http://codereview.chromium.org/243108/diff/7001/7006#newcode12
Line 12: }  // namespace WebKit
On 2009/10/08 06:15:09, darin wrote:
> nit: no need for the comment here.

Done.

http://codereview.chromium.org/243108/diff/7001/7006#newcode17
Line 17: public:
On 2009/10/08 06:15:09, darin wrote:
> nit: indent by 1 space

Done.

http://codereview.chromium.org/243108/diff/7001/7006#newcode18
Line 18: WebSocketStreamHandleDelegate() {}
On 2009/10/08 06:15:09, darin wrote:
> nit: indent by 2 spaces

Done.

http://codereview.chromium.org/243108/diff/7001/7009
File webkit/tools/test_shell/simple_socket_stream_bridge.cc (right):

http://codereview.chromium.org/243108/diff/7001/7009#newcode161
Line 161: return;
On 2009/10/08 05:21:17, tyoshino wrote:
> indentation

Done.

[darin (slow to review), 2009-10-09 07:14:55]

WebSocketStreamHandleImpl seems good to me.

http://codereview.chromium.org/243108/diff/7018/8013
File webkit/glue/websocketstreamhandle_impl.cc (right):

http://codereview.chromium.org/243108/diff/7018/8013#newcode23
Line 23: // WebSocketHandleImpl::Context
---------------------------------------------
nit: WebSocketStreamHandleImpl

[ukai, 2009-10-09 07:40:36]

http://codereview.chromium.org/243108/diff/7018/8013
File webkit/glue/websocketstreamhandle_impl.cc (right):

http://codereview.chromium.org/243108/diff/7018/8013#newcode23
Line 23: // WebSocketHandleImpl::Context
---------------------------------------------
On 2009/10/09 07:14:56, darin wrote:
> nit: WebSocketStreamHandleImpl

Done.

[ukai, 2009-10-15 01:10:36]

ping

[tyoshino (SeeGerritForStatus), 2009-10-15 07:44:56]

LGTM

http://codereview.chromium.org/243108/diff/13002/13004
File chrome/renderer/renderer_glue.cc (right):

http://codereview.chromium.org/243108/diff/13002/13004#newcode249
Line 249: webkit_glue::WebSocketStreamHandleBridge*
WebSocketStreamHandleBridge::Create(
is namespace prefix necessary?

http://codereview.chromium.org/243108/diff/13002/13004#newcode251
Line 251: webkit_glue::WebSocketStreamHandleDelegate* delegate) {
ditto

[ukai, 2009-10-15 09:03:48]

http://codereview.chromium.org/243108/diff/13002/13004
File chrome/renderer/renderer_glue.cc (right):

http://codereview.chromium.org/243108/diff/13002/13004#newcode249
Line 249: webkit_glue::WebSocketStreamHandleBridge*
WebSocketStreamHandleBridge::Create(
On 2009/10/15 07:44:56, tyoshino wrote:
> is namespace prefix necessary?

Done.

http://codereview.chromium.org/243108/diff/13002/13004#newcode251
Line 251: webkit_glue::WebSocketStreamHandleDelegate* delegate) {
On 2009/10/15 07:44:56, tyoshino wrote:
> ditto

Done.

[dglazkov, 2009-10-21 15:29:15]

Is it possible that this CL introduced a layout test crash? The flakiness
dashboard seems to think so:

http://src.chromium.org/viewvc/chrome/trunk/src/webkit/tools/layout_tests/fla...