[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
https://bugs.webkit.org/show_bug.cgi?id=28038
Attachment 38471: WebSocket API implementation. depends on bug 28037
https://bugs.webkit.org/attachment.cgi?id=38471&action=review
------- 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
fine.
+#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
below.
+bool WebSocketHandshaker::headerProcess(const HTTPHeaderMap& headers)
Maybe this should be processHeader()?
More information about the webkit-reviews
mailing list