[webkit-reviews] review denied: [Bug 34784] WebSocketHandshake needs to provide request information : [Attachment 49187] Patch v5 (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 provide request information
https://bugs.webkit.org/show_bug.cgi?id=34784

Attachment 49187: Patch v5 (Style fix)
https://bugs.webkit.org/attachment.cgi?id=49187&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
+    ASSERT(!m_request.get());

No need for .get() here and elsewhere.

+    // If we have a cache, use it.
+    if (m_request)
+	 return m_request;

I wouldn't call it "cache" - it's just a request that has already been created.
"Cache" implies a somewhat different set of behaviors - such as having multiple
entries, or and ability to be cleared.

It seems that m_request does not need to exist for normal operation, it's only
needed for Web Inspector? It seems weird to have all this data in both
WebSocketHandshake and WebSocketHandshakeRequest if we're not going to use it.

+	 // Once clientHandshakeRequest() is called, you cannot change the
client request
+	 // (i.e. you cannot call setURL(), setClientProtocol() or setSecure())
until reset() is called.

This limitation would not be necessary if WebSocketHandshakeRequest weren't
saved in WebSocketHandshake. And again, if it were a "cache", then it would be
just invalidated when setting.

+String WebSocketHandshakeRequest::upgrade() const
+{
+    return "WebSocket";
+}

There is no need for this to be a member function, as it doesn't depend on the
object. The name is misleading - upgrade is a verb, but this function doesn't
upgrade anything.

+    builder.append("Upgrade: ");
+    builder.append(upgrade());
+    builder.append("\r\n");

I suggest doing builder.append("Upgrade: WebSocket\r\n").

The reason you added these as public accessors is likely that they will be
needed in Web Inspector. But I don't think that having these as functions adds
much to forward compatibility - for all we know, the names are less likely to
change than the whole design of WebSocket handshake. So, I'd just hardcode the
values in both handshakeMessage() and Web Inspector, and maybe add a comment in
handshakeMessage() saying that changes to handshake generation should also
include changes to Web Inspector presentation code.

I don't know your plans for using this in Web Inspector - I expected a Vector
of headers to be provided to it, rather than a request object such as this.
Maybe your approach makes more sense.

+const String& WebSocketHandshakeRequest::origin() const
+{
+    return m_origin;
+}

This method doesn't seem to be used outside of WebSocketHandshakeRequest, so
(1) it should be private, and (2) it's not needed at all, please just use
m_origin directly. An accessor can be always added later if it becomes
necessary.

Ditto for most other one-liners in this class.

+class WebSocketHandshakeRequest : public RefCounted<WebSocketHandshakeRequest>
{

I'm not sure if WebSocketHandshakeRequest needs to be RefCounted - for current
code, it would suffice if it could only be stack-allocated (provided that we
don't keep it after generating a CString). Maybe it will become necessary for
Web Inspector.

The approach here looks good to me, this just needs one or two more iterations
to polish the details.