[Webkit-unassigned] [Bug 28038] WebSocket API implementation

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=28038


Alexey Proskuryakov <ap at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #38471|review?                     |review-
               Flag|                            |




--- Comment #8 from Alexey Proskuryakov <ap at webkit.org>  2009-08-24 11:01:28 PDT ---
(From update of attachment 38471)
+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()?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.