[Webkit-unassigned] [Bug 28038] WebSocket API implementation
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Aug 31 14:16:30 PDT 2009
https://bugs.webkit.org/show_bug.cgi?id=28038
Alexey Proskuryakov <ap at webkit.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #38806|review? |review-
Flag| |
--- Comment #15 from Alexey Proskuryakov <ap at webkit.org> 2009-08-31 14:16:30 PDT ---
(From update of attachment 38806)
#include "JSWebKitPointConstructor.h"
+#include "JSWebSocketConstructor.h"
#include "JSWorkerConstructor.h"
JSWebSocketConstructor.h doesn't have an ENABLE(WEB_SOCKETS) include guard
inside, so this will break compilation (please add the guard).
+#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.
+ void connect(const KURL& url, ExceptionCode&);
+ void connect(const KURL& url, const String& protocol, ExceptionCode&);
Please remove url argument names - they do not convey additional information
over the type name.
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.
+ , m_handshake(WebSocketHandshake(url, protocol, context))
This constructs a WebSocketHandshake object, and then copies it in place.
Please make WebSocketHandshake noncopyable by inheriting from WTF::Noncopyable,
and either keep it in an OwnPtr, or construct it in place.
+ 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.
There is no much harm in reading the headers, but it doesn't seem likely to me
that authentication support will be implemented in the form of a synchronous
authenticate() function, so it is somewhat confusing.
+const char webSocketUpgradeHeader[] = "Upgrade: WebSocket\r\n"
+ "Connection: Upgrade\r\n";
These are two headers, so the variable shouldn't make it look as if it's just
one.
+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.
+ builder.append(url().path());
Some functions in WebSocketHandshake use m_url, and others use url(). Please
change it to use m_url consistently.
Same for host(), secure() and clientProtocol().
+ if (len < sizeof(webSocketServerHandshakeHeader) - 1) {
+ LOG(Network, "short response header len=%d", len);
+ return false;
This sounds more grave than it is - nothing is wrong with the header, it just
hasn't been received fully yet. Maybe there is even no need for logging this.
+ LOG(Network, "incomplete server handshake len=%d", len);
Same here. Perhaps it would be better to check this before parsing other
headers?
+ 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.
+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.
+KURL WebSocketHandshake::urlOverHTTP() const
I'd call this httpURLForAuthenticationAndCookies() - a bit wordy, but this
method isn't going to be used often.
+const char* WebSocketHandshake::parseHeader(const char* start, const char*
end, HTTPHeaderMap* headers)
This function doesn't parse one header, it parses all of them, so a name like
"readHTTPHeaders()" would be more appropriate. A comment should explain that it
reads all headers except for the two predefined ones.
+ LOG(Network, "CF doesn't follow LF p=%p end=%p", p, end);
CR
+ LOG(Network, "CR doesn't follow LF after value p=%p end=%p", p,
end);
This sounds like too much detail - I think that it's sufficient for a WebKit
developer to know that there's something wrong with handshake - from there,
they could look at a tcpdump trace. Seems fine to leave this in for now, as
both server and client are being developed at once, but the logging should be
revisited in the future.
+ LOG(Network, "Unexpected end of header");
Is this even possible? We have already checked that there's CRLFCRLF before
calling this function.
+ if (it->first == "websocket-origin") {
+ if (!m_wsOrigin.isNull())
+ return false;
Seems that this check is misplaced - there's always no more than one header
with a given name in an HTTPHeaderMap, so uniqueness of headers should be
verified before storing them in the map.
+ default:
+ LOG(Network, "unexpected mode: %d", m_mode);
+ return false;
We usually leave the default clause out to let the compiler complain about
missing cases. Then, we put ASSERT_NOT_REACHED below the switch (all cases
should end with "return" for this to work).
+void WebSocketHandshake::handshake()
I'd call this checkResponseHeaders().
+#include "CString.h"
CString is only used as a return value, I think that a forward declaration
would be sufficient.
+ WebSocketHandshake(const KURL&, const String& protocol,
ScriptExecutionContext* context);
Please leave out "context" (but not "protocol").
+ void setURL(const KURL& url);
Please leave out "url". There are many cases like this below, I won't enumerate
them all.
Almost all of the comments above are pretty straightforward - the only major
comment I had was about dividing responsibilities between handshake and channel
classes. I don't have any ideas right now, but I'll try to think about this
more, and if you have any, please don't hesitate to make suggestions, of
course.
--
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