[Webkit-unassigned] [Bug 28038] WebSocket API implementation
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Sep 1 01:30:10 PDT 2009
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.
More information about the webkit-unassigned
mailing list