WebSocket implementation in WebKit

WebSocket implementation in WebKit

work-in-progress change for WebSocket implementation in WebKit.

[Yuzo, 2009-07-06 06:58:52]

My comments are mostly about style.

http://codereview.chromium.org/155079/diff/1/3
File WebCore/bindings/v8/custom/V8CustomBinding.h (right):

http://codereview.chromium.org/155079/diff/1/3#newcode497
Line 497: DECLARE_PROPERTY_ACCESSOR(WebSocketonopen);
I guess we should name this WebSocketOnopen?
(I see MessagePortOnmessage, WorkerOnmessage, etc. above.)

Ditto for WebSocketOnMessage and WebSocketOnclose.

http://codereview.chromium.org/155079/diff/1/4
File WebCore/dom/EventNames.h (right):

http://codereview.chromium.org/155079/diff/1/4#newcode132
Line 132: macro(open) \
I guess we should move this above macro(pause) to
sort the list alphabetically.

http://codereview.chromium.org/155079/diff/1/6
File WebCore/platform/network/WebSocketHandle.h (right):

http://codereview.chromium.org/155079/diff/1/6#newcode2
Line 2: * Copyright (C) 2009 Google Inc.  All rights reserved.
I don't know what copyright to use for WebKit, but
is this OK?

Ditto for other header files.

http://codereview.chromium.org/155079/diff/1/9
File WebCore/platform/network/WebSocketResponseBase.h (right):

http://codereview.chromium.org/155079/diff/1/9#newcode36
Line 36: class WebSocketResponseBase {
WebKit Coding Style Guidelines mandates that namespace
in a header must be indented. (while that in an implementaion is not).

Ditto for other header files.

http://codereview.chromium.org/155079/diff/1/9#newcode44
Line 44: const String& websocket_location() const;
WebKit Coding Style Guidelines mandates that
names must be CamelCase.

Ditto for others.

http://codereview.chromium.org/155079/diff/1/10
File WebCore/websockets/WebSocket.cpp (right):

http://codereview.chromium.org/155079/diff/1/10#newcode71
Line 71: connect(url, "", ec);
"" is an invalid protocol string as per the API spec.

Hence, the test on line 84 should always fail.

I think we need a different way of representing "protocol not specified".

[tyoshino-do-not-use, 2009-07-06 07:50:04]

http://codereview.chromium.org/155079/diff/1/4
File WebCore/dom/EventNames.h (right):

http://codereview.chromium.org/155079/diff/1/4#newcode132
Line 132: macro(open) \
On 2009/07/06 06:58:52, Yuzo wrote:
> I guess we should move this above macro(pause) to
> sort the list alphabetically.

Looks like categorized for each type (generic, video related, etc). This should
be kept isolated from others.

http://trac.webkit.org/changeset/27277#file18

http://codereview.chromium.org/155079/diff/1/8
File WebCore/platform/network/WebSocketRequestBase.h (right):

http://codereview.chromium.org/155079/diff/1/8#newcode36
Line 36: class WebSocketRequestBase {
forward declaration of String here?

http://codereview.chromium.org/155079/diff/1/10
File WebCore/websockets/WebSocket.cpp (right):

http://codereview.chromium.org/155079/diff/1/10#newcode56
Line 56: : ActiveDOMObject(context, this)
too long indentation

http://codereview.chromium.org/155079/diff/1/12
File WebCore/websockets/WebSocket.idl (right):

http://codereview.chromium.org/155079/diff/1/12#newcode36
Line 36: ] WebSocket : events:EventTarget {
is this qualifier "events:" required?
core::Node and events::EventTarget are in different modules, but there's no
qualifier in Node's InterfaceInheritance.
If syntax is the same as WebIDL spec at W3C, this should be
::events::EventTarget.

Is the identifier separator of WebKit IDL single colon?

http://codereview.chromium.org/155079/diff/1/13
File WebCore/websockets/WebSocketChannel.cpp (right):

http://codereview.chromium.org/155079/diff/1/13#newcode47
Line 47: : m_context(context)
too long indentation

[Yuzo, 2009-07-06 07:53:49]

http://codereview.chromium.org/155079/diff/1/4
File WebCore/dom/EventNames.h (right):

http://codereview.chromium.org/155079/diff/1/4#newcode132
Line 132: macro(open) \
Ah, you are right. Please ignore my comment.

On 2009/07/06 07:50:04, tyoshino wrote:
> On 2009/07/06 06:58:52, Yuzo wrote:
> > I guess we should move this above macro(pause) to
> > sort the list alphabetically.
> 
> Looks like categorized for each type (generic, video related, etc). This
should
> be kept isolated from others.
> 
> http://trac.webkit.org/changeset/27277#file18

[ukai, 2009-09-11 10:43:15]

Code has been landed in webkit as r48270

On 2009/07/06 07:53:49, Yuzo wrote:
> http://codereview.chromium.org/155079/diff/1/4
> File WebCore/dom/EventNames.h (right):
> 
> http://codereview.chromium.org/155079/diff/1/4#newcode132
> Line 132: macro(open) \
> Ah, you are right. Please ignore my comment.
> 
> On 2009/07/06 07:50:04, tyoshino wrote:
> > On 2009/07/06 06:58:52, Yuzo wrote:
> > > I guess we should move this above macro(pause) to
> > > sort the list alphabetically.
> > 
> > Looks like categorized for each type (generic, video related, etc). This
> should
> > be kept isolated from others.
> > 
> > http://trac.webkit.org/changeset/27277#file18