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

[bugzilla-daemon at webkit.org]

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





--- Comment #16 from Fumitoshi Ukai <ukai at chromium.org>  2009-09-01 01:30:10 PDT ---
Thanks for review.

(In reply to comment #15)

> +#if ENABLE(WEB_SOCKETS)
> +#include "JSWebSocket.h"
> +#include "WebSocket.h"
> +#endif
> 
> It would likely be easier to generate WebSocket files on all platforms than to
> maintain ENABLE(WEB_SOCKETS) around header includes.

Other features in the same file has guard. Is it ok to remove the guard?

>  void WebSocket::dispatchCloseEvent()
>  {
> -    // FIXME: implement this.
> +    RefPtr<Event> evt = Event::create(eventNames().closeEvent, false, false);
> 
> According to the spec, the events for open and close should be queued, as
> opposed to dispatched synchronously. I'm not sure if this spec provision makes
> sense, but we should fix either the code of the spec.

Ah, that's right. I think event for message also should be queued.

> +        case WebSocketHandshake::Redirect:
> +            LOG(Network, "Redirect to %s",
> m_handshake.redirectLocation().utf8().data());
> +            redirect(m_handshake.redirectLocation());
> +            return;
> +        case WebSocketHandshake::Authenticate:
> +            authenticate(m_handshake.wwwAuthenticate());
> +            return;
> 
> I'd suggest removing code related to redirects and authentication for now.
> Support for redirect has been removed from the spec, and authentication doesn't
> seem to be in an implementable state either.

Thanks for pointing out redirect has been removed from the spec.
I removed code related to redirects and authentication for now.

> +static bool hadServerHandshake(const char* start, const char* end)
> 
> There are two problems with this function's name:
> - due to the use of past tense, it sounds as if it consults some internal state
> for events that happened earlier;
> - it's not clear what it means to have a server handshake (which turns out to
> be that it has been received in full).
> 
> This is a simple string processing function, a name like "contains two CRLF
> pairs" would be better. However, with a name like this, it becomes obvious that
> you can just use strnstr instead of a hand-rolled helper function.

I don't think strnstr exists in common.  I found memmem, but it is GNU
extension.
Renamed containsTwoCRLFPairs().


> +        LOG(Network, "incomplete server handshake len=%d", len);
> 
> Same here. Perhaps it would be better to check this before parsing other
> headers?

not sure. by checking upgrade header and connection header first, we can find
bad handshake as soon as possible.

> +        LOG(Network, "header process failed");
> 
> It is fine to have errors logged to console - but it is only available for
> WebKit developers. Some day, we should log these to Inspector console for the
> benefit of Web developers.

Yes, that would be great. Will do in later.

> +size_t WebSocketHandshake::serverHandshakeLength() const
> +{
> +    return m_serverHandshakeLength;
> +}
> 
> The need to have public accessors for low-level data members like this seems to
> indicate that the responsibilities between WebSocketHandshake and
> WebSocketChannel are divided less than perfectly. I don't have any specific
> suggestions though.

Changed to return header length in readServerHandshake() (or return -1 if it
has not receive full response header).

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