[webkit-reviews] review denied: [Bug 28038] WebSocket API implementation : [Attachment 38471] WebSocket API implementation. depends on bug 28037

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Aug 24 11:01:28 PDT 2009

Alexey Proskuryakov <ap at webkit.org> has denied Fumitoshi Ukai
<ukai at chromium.org>'s request for review:
Bug 28038: WebSocket API implementation

Attachment 38471: WebSocket API implementation. depends on bug 28037

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
+static bool IsValidProtocolString(const WebCore::String& protocol)

Function names should start with lowercase letters.

+    if (!protocol.isEmpty() && !IsValidProtocolString(protocol)) {

Would it make sense to move isEmpty() check to the helper function?

+WebSocketChannel::WebSocketChannel(ScriptExecutionContext* context,
WebSocketChannelClient* client, const KURL& url, const String& protocol)
+	 : m_context(context)
+	 , m_client(client)
+	 , m_handshaker(WebSocketHandshaker(url, protocol, context))
+	 , m_buffer(0)
+	 , m_buffer_size(0)

Please use 4 spaces for indentation.

+    static const char frame_type[1] = {'\0'};
+    static const char frame_end[1] = {'\xff'};

No need to have these static arrays - there's a version of Vector::append that
takes const U&.

+	 virtual void didReceivedData(SocketStreamHandle*, const char*, int);

Typo: should be didReceiveData.

+    ASSERT(handle == m_handle.get() || !m_handle.get());

What are the situations when callbacks are called with null m_handle? Is it
normal, or could it indicate problems?

+    KURL url(location);
+    m_handshaker.setURL(url);

I'd write this in one line.

+void WebSocketChannel::authenticate(const String& www_authenticate)

This will break the build, because www_authenticate is unused.

Also, this is wrong style, we use camelCase, not underlines for function and
variable names. Please grep your patch with some expression like "[a-ln-z]_" to
catch all cases, I won't point out each of them, as there are too many.

+#include "WebSocketHandshaker.h"

I had to google "handshaker", and although it's a word indeed, it seems to have
too many unrelated meanings :). I think that "WebSocketHandshake" would be just

+#include "StringBuilder.h"
+#include <wtf/Vector.h>

No need for an empty line here.

+	 return WebCore::String();

This code is already in WebCore namespace, no need for the prefix. Same issue

+bool WebSocketHandshaker::headerProcess(const HTTPHeaderMap& headers)

Maybe this should be processHeader()?

More information about the webkit-reviews mailing list