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

[bugzilla-daemon at webkit.org]

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





--- Comment #3 from Fumitoshi Ukai <ukai at chromium.org>  2009-08-12 02:35:33 PDT ---
(In reply to comment #2)
> (From update of attachment 34548 [details])
> +    $(WebCore)/websockets \
>      $(WebCore)/svg \
>      $(WebCore)/websockets \
> 
> It's already there.
> 
> +    WebCore/websockets/WebSocketRequest.cpp \
> +    WebCore/websockets/WebSocketRequest.h \
> +    WebCore/websockets/WebSocketResponse.cpp \
> +    WebCore/websockets/WebSocketResponse.h
> 
> This looks quite wrong - WebSocket does not use a request/response model, so
> such class names are highly misguiding.

Hmm, how about WebSocketClientHandshake and WebSocketServerHandshake?
What name do you recommend?

> 
> +#if ENABLE(WEB_SOCKETS)
> +#include "JSWebSocket.h"
> +#include "WebSocket.h"
> 
> In some places, generated files are included without guards, and in others,
> with guards. The guards are only needed if the headers are not generated in
> ports that do not have this feature enabled - otherwise, similar guards within
> headers themselves would do the job.
> 
> There's never a need to protect e.g. WebSocket.h with external include guards.
> 
> +bool IsValidProtocolString(const WebCore::String& protocol)
> 
> This should be static, and inside the WebCore namespace. We don't normally use
> anonymous namespaces.
> 
> +    // FIXME: need dispatchEvent?
> 
> Absolutely - it will dispatch to handlers installed via addEventListener(), as
> opposed to setting socket.onopen or similar.

Should I add the implementation in this patch, or ok in another patch?

> 
> +bool CheckWebSocketHandshake(const WebCore::WebSocketRequest &req, const
> WebCore::WebSocketResponse& resp)
> 
> Capitalization, argument naming, '&' positioning. This should also be static.
> 
> There are many style issues in protocol implementation, I'm not pointing out
> most, because they are the same as in other patches.
> 
> +            // FIXME: handle overflow.
> 
> Please do!

What is the good way to handle overflow?
May I close the connection if too long length is detected?

Do we need to set maximum length of m_buffer? (512KB or so?)

> 
> +KURL WebSocketRequest::URLoverHTTP() const
> 
> In WebKit style, this would be urlOverHTTP. I'm not sure what this is use for
> though, hopefully an even better name can be found.
> 
> +    if (url().hasFragmentIdentifier()) {
> +        builder.append("#");
> +        builder.append(url().fragmentIdentifier());
> +    }
> 
> What are fragment identifiers good for when using WebSockets? Seems that they
> should be forbidden, or dropped immediately when passed in.

Sure.

> +const char* WebSocketResponse::parseHeader(const char* start, const char* end,
> Vector<std::pair<String, String> >* headers)
> 
> Are WebSockets headers case sensitive? Otherwise, it may be better to use
> HTTPHeaderMap.

Thanks for suggestion.

> 
> I didn't closely review all the code; please fix style, possibly using
> check-webkit-style script. The direction looks good to me, except for
> request/response classes.

I used check-webkit-style script, but it didn't catch some style issues.

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