[webkit-reviews] review denied: [Bug 34784] WebSocketHandshake needs to return handshake request as a ResourceRequest : [Attachment 48468] Patch v3 (style fix)

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Yuta Kitamura
<yutak at chromium.org>'s request for review:
Bug 34784: WebSocketHandshake needs to return handshake request as a
ResourceRequest
https://bugs.webkit.org/show_bug.cgi?id=34784

Attachment 48468: Patch v3 (style fix)
https://bugs.webkit.org/attachment.cgi?id=48468&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
+namespace {

We don't use anonymous namespaces, we use static functions. I'm not entirely
sure why, I've been told it somehow makes debugging easier - but please change
this, if only for consistency.

+void addHTTPHeaderField(StringBuilder& builder, const char* name, const
String& value)
+{
+    builder.append(name);
+    builder.append(": ");
+    builder.append(value);

Once the code gets abstracted into a function with a generic name, it should
become resilient against broken input (such as newlines in header field
values). Otherwise, it's too easy to misuse later, introducing security
problems.

+    for (HTTPHeaderMap::const_iterator it = headers.begin(), end =
headers.end(); it != end; ++it)
+	 if (!equalIgnoringCase(it->first, "Upgrade") &&
!equalIgnoringCase(it->first, "Connection")
+	     && !equalIgnoringCase(it->first, "Host") &&
!equalIgnoringCase(it->first, "Origin")
+	     && !equalIgnoringCase(it->first, "WebSocket-Protocol"))

This is an unnecessarily slow way to compare AtomicStrings to constant values.
You should use AtomicStrings for the header field name constants, too.

I'm not particularly happy with how this code works. It's as if ResourceRequest
version were the real thing, and CString one were a debugging aid of some kind,
which is obviously not true. With the current ResourceRequest implementation,
we get lucky that no other WebSocket peculiar requirements are broken (besides
ordering, WebSocket forbids continuations, for example). This also introduces a
performance hit just for Web Inspector aid.

At the very least, there should be extensive and clear comments about this
design problem. But I'd much prefer to see a solution that builds WebSocket
handshake without round-tripping it via ResourceRequest. One way to do this is
to abstract request builder into a class that can build either a
ResourceRequest or a CString. Then you would pass it as a parameter to a
function that knows how to build the request (either as a template parameter,
or as a pointer to an interface).

Just as a point for consideration, it doesn't seem that ResourceRequest is
necessarily a good class to use with Web Inspector. Either Web Inspector will
have incorrect order of WebSocket headers, which will be confusing to
developers, or it will need yet another copy of the code that reorders them.
Perhaps there can be a different way to pass the data to Web Inspector.

r- for the concrete points I mentioned first, but I also strongly suggest
revisiting the design to avoid round-tripping.