Bug 34784 – WebSocketHandshake needs to provide request information

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Yuta Kitamura]

WebSocketHandshake needs to return handshake request as a ResourceRequest.

This will be necessary to let the Web Inspector show the content of handshake requests.

[Yuta Kitamura]

Created attachment 48462 [details]
Add a method to WebSocketHandshake that returns the handshake request as

[Yuta Kitamura]

Comment on attachment 48462 [details]
Add a method to WebSocketHandshake that returns the handshake request as

Oops, I forgot to include ChangeLog. Let me try again.

[Yuta Kitamura]

Created attachment 48463 [details]
Add a method to WebSocketHandshake that returns the handshake request as a ResourceRequest.

[Yuta Kitamura]

Ukai-san, can you review this?

[Fumitoshi Ukai]

Comment on attachment 48463 [details]
Add a method to WebSocketHandshake that returns the handshake request as a ResourceRequest.

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 730d46c..adbb714 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,26 @@
> [phone]  Yuta Kitamura  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Add a method to WebSocketHandshake that returns the handshake request as
> +        a ResourceRequest.
> +
> +        This method will be necessary to let the Web Inspector show the content of
> +        handshake requests.
> +
> +        No new tests, since the current tests will suffice (LayoutTests/websocket/*).
> +
> +        WebSocketHandshake needs to return handshake request as a ResourceRequest
> +        https://bugs.webkit.org/show_bug.cgi?id=34784
> +
> +        * platform/network/HTTPHeaderMap.h:
> +        (WebCore::HTTPHeaderMap::contains): Added an overloaded method.
> +        Without this, the one that receives AtomicString is hidden and cannot be called.
> +        * websockets/WebSocketHandshake.cpp:
> +        (WebCore::WebSocketHandshake::clientHandshakeRequest):
> +        (WebCore::WebSocketHandshake::clientHandshakeMessage):
> +        * websockets/WebSocketHandshake.h:
> +
>  2010-02-09  Darin Adler  <[e-mail]>
>  
>          * rendering/RenderObject.cpp:
> diff --git a/WebCore/platform/network/HTTPHeaderMap.h b/WebCore/platform/network/HTTPHeaderMap.h
> index dfde974..72a0ce9 100644
> --- a/WebCore/platform/network/HTTPHeaderMap.h
> +++ b/WebCore/platform/network/HTTPHeaderMap.h
> @@ -46,6 +46,11 @@ namespace WebCore {
>  
>          void adopt(std::auto_ptr<CrossThreadHTTPHeaderMapData>);
>          
> +        bool contains(const AtomicString& name) const
> +        {
> +            return HashMap<AtomicString, String, CaseFoldingHash>::contains(name);
> +        }
> +

Can't we use const char* version?


>          String get(const AtomicString& name) const
>          {
>              return HashMap<AtomicString, String, CaseFoldingHash>::get(name);
> diff --git a/WebCore/websockets/WebSocketHandshake.cpp b/WebCore/websockets/WebSocketHandshake.cpp
> index 883f84b..92aa01b 100644
> --- a/WebCore/websockets/WebSocketHandshake.cpp
> +++ b/WebCore/websockets/WebSocketHandshake.cpp
> @@ -41,9 +41,11 @@
>  #include "HTTPHeaderMap.h"
>  #include "KURL.h"
>  #include "Logging.h"
> +#include "ResourceRequest.h"
>  #include "ScriptExecutionContext.h"
>  #include "SecurityOrigin.h"
>  #include "StringBuilder.h"
> +#include <wtf/OwnPtr.h>
>  #include <wtf/StringExtras.h>
>  #include <wtf/Vector.h>
>  
> @@ -155,45 +157,79 @@ String WebSocketHandshake::clientLocation() const
>      return builder.toString();
>  }
>  
> -CString WebSocketHandshake::clientHandshakeMessage() const
> +PassOwnPtr<ResourceRequest> WebSocketHandshake::clientHandshakeRequest() const
>  {
> -    StringBuilder builder;
> -
> -    builder.append("GET ");
> -    builder.append(resourceName(m_url));
> -    builder.append(" HTTP/1.1\r\n");
> -    builder.append("Upgrade: WebSocket\r\n");
> -    builder.append("Connection: Upgrade\r\n");
> -    builder.append("Host: ");
> -    builder.append(m_url.host().lower());
> -    if (m_url.port()) {
> -        if ((!m_secure && m_url.port() != 80) || (m_secure && m_url.port() != 443)) {
> -            builder.append(":");
> -            builder.append(String::number(m_url.port()));
> -        }
> -    }
> -    builder.append("\r\n");
> -    builder.append("Origin: ");
> -    builder.append(clientOrigin());
> -    builder.append("\r\n");
> -    if (!m_clientProtocol.isEmpty()) {
> -        builder.append("WebSocket-Protocol: ");
> -        builder.append(m_clientProtocol);
> -        builder.append("\r\n");
> +    OwnPtr<ResourceRequest> request(new ResourceRequest(m_url));

setTargetType?

> +    request->setHTTPMethod("GET");
> +    request->setHTTPHeaderField("Upgrade", "WebSocket");
> +    request->setHTTPHeaderField("Connection", "Upgrade");
> +    StringBuilder host;
> +    host.append(m_url.host().lower());
> +    if (m_url.port() && ((!m_secure && m_url.port() != 80) || (m_secure && m_url.port() != 443))) {
> +        host.append(":");
> +        host.append(String::number(m_url.port()));
>      }
> +    request->setHTTPHeaderField("Host", host.toString());
> +    request->setHTTPHeaderField("Origin", clientOrigin());
> +    if (!m_clientProtocol.isEmpty())
> +        request->setHTTPHeaderField("WebSocket-Protocol", m_clientProtocol);
> +
>      KURL url = httpURLForAuthenticationAndCookies();
>      // FIXME: set authentication information or cookies for url.
>      // Set "Authorization: <credentials>" if authentication information exists for url.
>      if (m_context->isDocument()) {
>          Document* document = static_cast<Document*>(m_context);
>          String cookie = cookies(document, url);
> -        if (!cookie.isEmpty()) {
> -            builder.append("Cookie: ");
> -            builder.append(cookie);
> -            builder.append("\r\n");
> -        }
> +        if (!cookie.isEmpty())
> +            request->setHTTPHeaderField("Cookie", cookie);
>          // Set "Cookie2: <cookie>" if cookies 2 exists for url?
>      }
> +
> +    return request.release();
> +}
> +
> +namespace {
> +
> +void addHTTPHeaderField(StringBuilder& builder, const AtomicString& name, const String& value)

If we use const chat * for headers.get() below, we can use const char* name here?

> +{
> +    builder.append(name);
> +    builder.append(": ");
> +    builder.append(value);
> +    builder.append("\r\n");
> +}
> +
> +} // anonymous namespace
> +
> +CString WebSocketHandshake::clientHandshakeMessage() const
> +{
> +    OwnPtr<ResourceRequest> request(clientHandshakeRequest());
> +
> +    StringBuilder builder;
> +    builder.append(request->httpMethod());
> +    builder.append(" ");
> +    builder.append(resourceName(request->url()));
> +    builder.append(" HTTP/1.1\r\n");
> +
> +    // Current Web Socket protocol spec requires the first five (or possibly four) headers should appear in the following order:
> +    // Upgrade, Connection, Host, Origin, and WebSocket-Protocol (optional).
> +    const HTTPHeaderMap& headers(request->httpHeaderFields());
> +    AtomicString upgrade("Upgrade");
> +    addHTTPHeaderField(builder, upgrade, headers.get(upgrade));
> +    AtomicString connection("Connection");
> +    addHTTPHeaderField(builder, connection, headers.get(connection));
> +    AtomicString host("Host");
> +    addHTTPHeaderField(builder, host, headers.get(host));
> +    AtomicString origin("Origin");
> +    addHTTPHeaderField(builder, origin, headers.get(origin));
> +    AtomicString webSocketProtocol("WebSocket-Protocol");
> +    if (headers.contains(webSocketProtocol))
> +        addHTTPHeaderField(builder, webSocketProtocol, headers.get(webSocketProtocol));
> +
> +    // Print remaining header fields.
> +    for (HTTPHeaderMap::const_iterator it = headers.begin(), end = headers.end(); it != end; ++it)
> +        if (it->first != upgrade && it->first != connection && it->first != host && it->first != origin && it->first != webSocketProtocol)
> +            addHTTPHeaderField(builder, it->first, it->second);
> +
>      builder.append("\r\n");
>      return builder.toString().utf8();
>  }
> diff --git a/WebCore/websockets/WebSocketHandshake.h b/WebCore/websockets/WebSocketHandshake.h
> index bda320a..11070bd 100644
> --- a/WebCore/websockets/WebSocketHandshake.h
> +++ b/WebCore/websockets/WebSocketHandshake.h
> @@ -36,11 +36,13 @@
>  #include "KURL.h"
>  #include "PlatformString.h"
>  #include <wtf/Noncopyable.h>
> +#include <wtf/PassOwnPtr.h>
>  
>  namespace WebCore {
>  
>      class ScriptExecutionContext;
>      class HTTPHeaderMap;
> +    class ResourceRequest;
>  
>      class WebSocketHandshake : public Noncopyable {
>      public:
> @@ -63,6 +65,7 @@ namespace WebCore {
>          String clientOrigin() const;
>          String clientLocation() const;
>  
> +        PassOwnPtr<ResourceRequest> clientHandshakeRequest() const;
>          CString clientHandshakeMessage() const;
>  
>          void reset();

[Yuta Kitamura]

(In reply to comment #5)
> Can't we use const char* version?

Yes, certainly. (At first I thought AtomicString yields some performance benefits, but I changed my mind; now I think it's not significant anyway.)

> 
> setTargetType?

I suppose the default TargetType (TargetIsSubresource) is enough.

> 
> If we use const chat * for headers.get() below, we can use const char* name
> here?

Yes, I'll fix.

[Yuta Kitamura]

Created attachment 48467 [details]
Patch v2 (apply ukai's comments)

[WebKit Review Bot]

Attachment 48467 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/websockets/WebSocketHandshake.cpp:233:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/websockets/WebSocketHandshake.cpp:234:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 2


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yuta Kitamura]

(In reply to comment #6)
> (In reply to comment #5)
> > 
> > If we use const chat * for headers.get() below, we can use const char* name
> > here?
> 
> Yes, I'll fix.

It turned out to be impossible to remove AtomicString& version, since the following loop requires it.

> +    // Print remaining header fields.
> +    for (HTTPHeaderMap::const_iterator it = headers.begin(), end = headers.end(); it != end; ++it)
> +        if (it->first != upgrade && it->first != connection && it->first != host && it->first != origin && it->first != webSocketProtocol)
> +            addHTTPHeaderField(builder, it->first, it->second);

I also modified the comparisons above so that the headers are compared case-insensitively.

[Yuta Kitamura]

Comment on attachment 48467 [details]
Patch v2 (apply ukai's comments)

Oops, the style bot is angry at my patch. Let me fix it.

BTW check-webkit-style does not complain about it in my local environment. I surely ran it locally before submitting my patch, trust me :)

[Yuta Kitamura]

Created attachment 48468 [details]
Patch v3 (style fix)

[Fumitoshi Ukai]

(In reply to comment #11)
> Created an attachment (id=48468) [details]
> Patch v3 (style fix)

Looks good to me.

[Alexey Proskuryakov]

Comment on attachment 48468 [details]
Patch v3 (style fix)

+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.

[Yuta Kitamura]

Thank you for your comment. I'll reconsider the design.

[Yuta Kitamura]

Created attachment 49186 [details]
Patch v4 (Completely rewritten)

[WebKit Review Bot]

Attachment 49186 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/websockets/WebSocketHandshakeRequest.h:40:  wtf includes should be <wtf/file.h> instead of "wtf/file.h".  [build/include] [4]
WebCore/websockets/WebSocketHandshakeRequest.h:41:  wtf includes should be <wtf/file.h> instead of "wtf/file.h".  [build/include] [4]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yuta Kitamura]

Created attachment 49187 [details]
Patch v5 (Style fix)

[Yuta Kitamura]

(In reply to comment #17)
> Created an attachment (id=49187) [details]
> Patch v5 (Style fix)

Now I realize that I've been using check-webkit-style wrongly... (you need to use --git-commit=HEAD^ instead of HEAD)

Anyways, let me add some comments. In this patch, I did:
- create a class WebSocketHandshakeRequest that stores the request information.
- let WebSocketHandshake create and return a request object.
- let WebSocketChannel generate the request message using that request object.

After this patch is committed, I'm going to add WebSocketHandshakeResponse in the same fashion.

ukai: Do you have any comments on this patch?

[Fumitoshi Ukai]

Comment on attachment 49187 [details]
Patch v5 (Style fix)

looks good to me.

> +    const String& webSocketProtocol() const; // Empty string means the absence of WebSocket-Protocol field.

I think null string means the absence of websocket protocol.

[Yuta Kitamura]

(In reply to comment #19)
> (From update of attachment 49187 [details])
> looks good to me.
> 
> > +    const String& webSocketProtocol() const; // Empty string means the absence of WebSocket-Protocol field.
> 
> I think null string means the absence of websocket protocol.

As per current Web Socket protocol draft, field values for WebSocket-Protocol are non-empty.

   The /resource name/ and /protocol/ strings must be non-empty strings of
   ASCII characters in the range U+0020 to U+007E.

Maybe we should rewrite this comment as "Null string or empty string means...". Another idea is to normalize an empty string to the null string and not to return empty strings here. I think both are okay, and personally I like the latter.

[Fumitoshi Ukai]

(In reply to comment #20)
> (In reply to comment #19)
> > (From update of attachment 49187 [details] [details])
> > looks good to me.
> > 
> > > +    const String& webSocketProtocol() const; // Empty string means the absence of WebSocket-Protocol field.
> > 
> > I think null string means the absence of websocket protocol.
> 
> As per current Web Socket protocol draft, field values for WebSocket-Protocol
> are non-empty.
> 
>    The /resource name/ and /protocol/ strings must be non-empty strings of
>    ASCII characters in the range U+0020 to U+007E.
> 
> Maybe we should rewrite this comment as "Null string or empty string means...".
> Another idea is to normalize an empty string to the null string and not to
> return empty strings here. I think both are okay, and personally I like the
> latter.

empty string for protocol is not valid in WebSocket API.
I think isValidProtocolString() in WebSocket.cpp checks this.

[Yuta Kitamura]

(In reply to comment #21)
> 
> empty string for protocol is not valid in WebSocket API.
> I think isValidProtocolString() in WebSocket.cpp checks this.

Got it. After gathering more input from other reviewers, I'd like to update the patch.

It seems like WebCore/WebCore.xcodeproj/project.pbxproj becomes obsolete (failed to patch), so I want to fix this later too.

[Alexey Proskuryakov]

Comment on attachment 49187 [details]
Patch v5 (Style fix)

+    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.

[Yuta Kitamura]

Thank you for your comment.

The design decision was based on my intention to:
1) Minimize construction cost of WebSocketHandshakeRequest objects.
2) Make it clear that at some moment a request should get "frozen" and will no longer change (because changes to the request may confuse Web Inspector).

I was not sure if we can accept initial construction and copy-construction cost of request objects (copying a few string and a vector of extra header fields). Since some request object may live after the main WebSocket object is freed (Web Inspector may keep holding it), I decided to make the request object RefCounted.

However, after I read your comment, I started to think my approach was unnecessarily complicated. As you suggested, we can allocate request objects in the stack, and let Web Inspector responsible for holding them.

I want to rewrite the patch based on this new approach.

Response to each comment inline:

(In reply to comment #23)
> (From update of attachment 49187 [details])
> +    ASSERT(!m_request.get());
> 
> No need for .get() here and elsewhere.

Sure.

> 
> +    // 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.

Right, I mistakenly used that word. My original intention was to avoid multiple construction and remember the object in the handshake object.

> 
> 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.
>

Good point. It does not need to keep it in the handshake object. On-demand generation will suffice. This will be removed in the next patch.
 
> +        // 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.
> 

Your plan sounds better to me, too. A Vector of headers will suffice and I think it's a cleaner interface. I'd be happy to rewrite this in the next patch.

> +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.

Certainly.

> 
> +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.

As I mentioned above, the class will not be RefCounted.

[Yuta Kitamura]

Created attachment 49363 [details]
Patch v6 (second try)

[Alexey Proskuryakov]

Comment on attachment 49363 [details]
Patch v6 (second try)

+    if (!url.protocolIs("wss") && m_secure)
+        url.setProtocol("wss");
+    else if (!url.protocolIs("ws") && !m_secure)
+        url.setProtocol("ws");

Can this ever happen? If not, this should be an assertion instead.

+    const CString& handshakeMessage = m_handshake.clientHandshakeRequest().handshakeMessage();

Returning the handshake request object results in an unnecessary copy; I think that WebSocketHandshake could still have a clientHandshakeMessage() that would produce a CString without copying WebSocketHandshakeRequest around.

+    KURL cookieUrl = httpURLForAuthenticationAndCookies();

Should be cookieURL per WebKit style.

+    // FIXME: set authentication information or cookies for cookieUrl.
+    // Set "Authorization: <credentials>" if authentication information exists for cookieUrl.

You didn't change this comment, but it looks strange, I'm not sure what it actually means. We do set cookies right below the comment, and authorization has been dropped from the spec.

+    fields.insert(fields.size() - 1, m_extraHeaderFields);

This should use append().

+#include "CString.h"
+#include "HTTPHeaderMap.h"

I don't think you need to include CString.h, and you definitely do not need HTTPHeaderMap.h.

+    struct HeaderField {

I'd consider using std::pair instead of this struct - we use it quite frequently in WebCore.

+    // According to current Web Socket protocol spec, four mandatory headers (Upgrade, Connection, Host, and Origin) and

Please spell out "specification", not "spec".

+    // Build the character sequence for this handshake request.

Maybe "serialization"?

These comments are very minor. Still an r- to address them, but this is almost done.

[Alexey Proskuryakov]

+    // According to current Web Socket protocol spec, four mandatory headers
(Upgrade, Connection, Host, and Origin) and

Oh, and those are "header fields", not "headers". You've got it right elsewhere in the patch.

[Yuta Kitamura]

Thanks again! And, sorry for my late replies (I've been off for the last few days and have had no Internet access).

(In reply to comment #26)
> (From update of attachment 49363 [details])
> +    if (!url.protocolIs("wss") && m_secure)
> +        url.setProtocol("wss");
> +    else if (!url.protocolIs("ws") && !m_secure)
> +        url.setProtocol("ws");
> 
> Can this ever happen? If not, this should be an assertion instead.

When WebSocketHandshake::setSecure(bool) is called, this might be happen. As there's no code that uses this method (as far as I grepped around), probably I can delete this method and simplify the above code.

> 
> +    const CString& handshakeMessage =
> m_handshake.clientHandshakeRequest().handshakeMessage();
> 
> Returning the handshake request object results in an unnecessary copy; I think
> that WebSocketHandshake could still have a clientHandshakeMessage() that would
> produce a CString without copying WebSocketHandshakeRequest around.

Agreed.

> 
> +    KURL cookieUrl = httpURLForAuthenticationAndCookies();
> 
> Should be cookieURL per WebKit style.

Sure.

> 
> +    // FIXME: set authentication information or cookies for cookieUrl.
> +    // Set "Authorization: <credentials>" if authentication information exists
> for cookieUrl.
> 
> You didn't change this comment, but it looks strange, I'm not sure what it
> actually means. We do set cookies right below the comment, and authorization
> has been dropped from the spec.
> 

As I discussed with Fumitoshi (ukai@chromium) off-line, these lines are simply obsolete as you suggested and safe to erase.

> +    fields.insert(fields.size() - 1, m_extraHeaderFields);
> 
> This should use append().

Sure.

> 
> +#include "CString.h"
> +#include "HTTPHeaderMap.h"
> 
> I don't think you need to include CString.h, and you definitely do not need
> HTTPHeaderMap.h.

I've forgotten to erase them. Will be removed.

> 
> +    struct HeaderField {
> 
> I'd consider using std::pair instead of this struct - we use it quite
> frequently in WebCore.

Sure.

> 
> +    // According to current Web Socket protocol spec, four mandatory headers
> (Upgrade, Connection, Host, and Origin) and
> 
> Please spell out "specification", not "spec".

Sure.

> 
> +    // Build the character sequence for this handshake request.
> 
> Maybe "serialization"?

That sounds better.

> 
> These comments are very minor. Still an r- to address them, but this is almost
> done.

[Alexey Proskuryakov]

> When WebSocketHandshake::setSecure(bool) is called, this might be happen. As
> there's no code that uses this method (as far as I grepped around), probably I
> can delete this method and simplify the above code.

Yes, this sounds good.

[Yuta Kitamura]

Created attachment 49782 [details]
Patch v7 (Third try)

[Alexey Proskuryakov]

Comment on attachment 49782 [details]
Patch v7 (Third try)

--- a/WebCore/WebCore.pro
+++ b/WebCore/WebCore.pro
@@ -2678,6 +2678,7 @@ SOURCES += \
     websockets/WebSocket.cpp \
     websockets/WebSocketChannel.cpp \
     websockets/WebSocketHandshake.cpp \
+    websockets/WebSocketHandshakeRequest.cpp \

I've noticed that .pro file doesn't list any WebSocket-related headers. It probably should.

+    Vector<std::pair<AtomicString, String> > fields;

In .cpp files, we just do "using namespace std;".

At some point during this patch evolution, code paths for generating CString and request object became separate again. Maybe it's OK, since they no longer look that similar, but there is certain unfortunate duplication of logic, which can cause issues later on.

That said, I'm fine with this patch as is (please just remove std:: prefixes from .cpp). Is some reviewer working with you on Web Inspector side of the feature? If yes, they could give you a final review, as long as this interface between WebCore and Web Inspector looks good to them.

[Yuta Kitamura]

Created attachment 49789 [details]
Patch v8 (Fix 'std::' and WebCore.pro)

[Yuta Kitamura]

(In reply to comment #32)
> Created an attachment (id=49789) [details]
> Patch v8 (Fix 'std::' and WebCore.pro)

Please note that I blindly made changes to WebCore.pro. I want to see how the QT bot feels about these.

[Yuta Kitamura]

(In reply to comment #31)
> At some point during this patch evolution, code paths for generating CString
> and request object became separate again. Maybe it's OK, since they no longer
> look that similar, but there is certain unfortunate duplication of logic, which
> can cause issues later on.
> 
> That said, I'm fine with this patch as is (please just remove std:: prefixes
> from .cpp). Is some reviewer working with you on Web Inspector side of the
> feature? If yes, they could give you a final review, as long as this interface
> between WebCore and Web Inspector looks good to them.

+pfeldman

Pavel, could you please make comments on the patch? This batch is basically adding an interface to WebSocketHandshake so that the inspector can get the WebSocket's request information.

After this patch is landed, I'm going to create another patch to address handshake responses.

[Pavel Feldman]

Comment on attachment 49789 [details]
Patch v8 (Fix 'std::' and WebCore.pro)

Overall the patch looks good. Couple of nits:

- It shufflest WebCore/WebCore.vcproj/WebCore.vcproj a bit
- public typedefs for headers would be nice
- is there a reason you use AtomicString and String for header pairs?

Do you want me to be a formal reviewer? (need r+?)

[Yuta Kitamura]

(In reply to comment #35)
> (From update of attachment 49789 [details])
> Overall the patch looks good. Couple of nits:
> 
> - It shufflest WebCore/WebCore.vcproj/WebCore.vcproj a bit

I had no idea of why this happened. I just opened the .vcproj file by VS2005, added couple of files, and saved. Maybe I can try again?

> - public typedefs for headers would be nice

That sounds nice. I'll update the patch.

> - is there a reason you use AtomicString and String for header pairs?

I did this because HTTPHeaderMap did the same thing. The names of the header fields are limited and not arbitrary, so I think it's reasonable to use AtomicString here.

> 
> Do you want me to be a formal reviewer? (need r+?)

I don't care very much about who r+'s my patch, but probably Alexey (ap) is a better person to mark r+ (or r-). Anyway thank you for your proposal.

I'll address the typedef issue tomorrow (because I don't have developing environment right now). Dropping r? flag for now.

Thank you for reviewing my patch!

[Yuta Kitamura]

Created attachment 49879 [details]
Patch v9 (Add typedef)

[Yuta Kitamura]

(In reply to comment #37)
> Created an attachment (id=49879) [details]
> Patch v9 (Add typedef)

I tried to correct .vcproj file, but my VS2005 randomly changed some files in that .vcproj file. Hand-editing such a file sounded like a bad idea, so I decided to leave it as-is.

Alexey: Could you make a (hopefully) final review on this patch?

[Alexey Proskuryakov]

Comment on attachment 49879 [details]
Patch v9 (Add typedef)

Ok, r=me

[Yuta Kitamura]

Comment on attachment 49879 [details]
Patch v9 (Add typedef)

Thank you! Setting cq?.

[Fumitoshi Ukai]

Committed r55505: <http://trac.webkit.org/changeset/55505>