WebSocketBasicStream framing logic

net/websockets/websocket_basic_stream.h

+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef NET_WEBSOCKETS_WEBSOCKET_BASIC_STREAM_H_
+#define NET_WEBSOCKETS_WEBSOCKET_BASIC_STREAM_H_
+
+#include <string>
+
+#include "base/callback.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/scoped_vector.h"
+#include "net/websockets/websocket_frame_parser.h"
+#include "net/websockets/websocket_stream.h"
+
+namespace net {
+
+class ClientSocketHandle;
+class DrainableIOBuffer;
+class GrowableIOBuffer;
+class HttpRequestHeaders;
+class HttpResponseInfo;
+class IOBufferWithSize;
+struct WebSocketFrameChunk;
+
+// Implementation of WebSocketStream for non-multiplexed ws:// connections (or
+// the physical side of a multiplexed ws:// connection).
+class NET_EXPORT_PRIVATE WebSocketBasicStream : public WebSocketStream {
+ public:
+  // The type of a pointer to a function that generates a WebSocketMaskingKey.

[szym, 2013/08/27 22:54:04]

Spurious comment.

[Adam Rice, 2013/08/28 12:01:11]

Removed.

+  typedef WebSocketMaskingKey (*WebSocketMaskingKeyGeneratorFunction)();
+
+  // This class should not normally be constructed directly; see
+  // WebSocketStream::CreateAndConnectStream.
+  explicit WebSocketBasicStream(scoped_ptr<ClientSocketHandle> connection);
+
+  // The destructor has to make sure the connection is closed when we finish so
+  // that it does not get returned to the pool.
+  virtual ~WebSocketBasicStream();
+
+  // Reads one or more frame chunks. See WebSocketStream for the full
+  // specifications of the behaviour.

[szym, 2013/08/27 22:54:04]

Remove all comments on the OVERRIDEN methods if they implement the spec in the
base class. A comment is necessary if the behavior differs in any way from the
spec. If the general spec needs to be updated, do it in the base class.

[Adam Rice, 2013/08/28 12:01:11]

Thanks. That helps.

+  virtual int ReadFrames(ScopedVector<WebSocketFrameChunk>* frame_chunks,
+                         const CompletionCallback& callback) OVERRIDE;
+
+  // Writes one or more frame chunks. Either writes everything or fails.
+  virtual int WriteFrames(ScopedVector<WebSocketFrameChunk>* frame_chunks,
+                          const CompletionCallback& callback) OVERRIDE;
+
+  // Closes the stream, cancelling all pending IO operations without calling the
+  // callbacks.
+  virtual void Close() OVERRIDE;
+
+  // Returns the optional sub-protocol that was negotiated with the server.
+  virtual std::string GetSubProtocol() const OVERRIDE;
+
+  // Returns the extensions that were negotiated with the server.
+  virtual std::string GetExtensions() const OVERRIDE;
+
+  // Writes WebSocket handshake request HTTP-style to the connection. Adds
+  // "Sec-WebSocket-Key" header; this should not be included in |headers|.
+  //
+  // "callback" will only be called if this method returns ERR_IO_PENDING.
+  //
+  // |response_info| must remain valid until the callback from
+  // ReadHandshakeResponse has been called.
+  virtual int SendHandshakeRequest(const GURL& url,
+                                   const HttpRequestHeaders& headers,
+                                   HttpResponseInfo* response_info,
+                                   const CompletionCallback& callback) OVERRIDE;
+
+  // Reads WebSocket handshake response from the connection. The callback
+  // function completes when the response headers have been completely
+  // received. Must be called after SendHandshakeRequest() completes (including
+  // the callback, if relevant). If the result was not available synchronously,
+  // then it will be passed to the callback. Note that the actual response info
+  // is passed into the HttpResponseInfo object that was passed to
+  // SendHandshakeResponse.
+  virtual int ReadHandshakeResponse(
+      const CompletionCallback& callback) OVERRIDE;
+
+  ////////////////////////////////////////////////////////////////////////////
+  // Methods for testing only.
+
+  // Creates a WebSocketBasicStream with the specified |connection|,
+  // |http_read_buffer|, |sub_protocol|, |extensions| and
+  // |key_generator_function|.

[szym, 2013/08/27 22:54:04]

Spurious comment. 

[Adam Rice, 2013/08/28 12:01:11]

Removed.

+  static scoped_ptr<WebSocketBasicStream> CreateWebSocketBasicStreamForTesting(
+      scoped_ptr<ClientSocketHandle> connection,
+      const scoped_refptr<GrowableIOBuffer>& http_read_buffer,
+      const std::string& sub_protocol,
+      const std::string& extensions,
+      WebSocketMaskingKeyGeneratorFunction key_generator_function);
+
+ private:
+  // The semantics of WriteFrames() are such that it only returns OK or calls

[szym, 2013/08/27 22:54:04]

Do not elaborate on the implementation in the header comment.

"Returns OK or calls |callback| when the |buffer| is fully drained or something
has failed."

[Adam Rice, 2013/08/28 12:01:11]

Done.

+  // the callback when everything is written or something has failed. But
+  // Socket::Write() can do partial writes, so this method wraps the call in a
+  // loop.
+  int WriteEverything(const scoped_refptr<DrainableIOBuffer>& buffer,
+                      const CompletionCallback& callback);
+
+  // Wraps the |callback| to continue writing until everything has been written.
+  void WriteDone(const scoped_refptr<DrainableIOBuffer>& buffer,

[szym, 2013/08/27 22:54:04]

Suggest: OnWriteComplete or OnWriteDone

[Adam Rice, 2013/08/28 12:01:11]

Done.

+                 const CompletionCallback& callback,
+                 int result);
+
+  // Called when a read completes. Parses the result and (unless no complete
+  // header has been received) calls |callback|.
+  void ReadDone(ScopedVector<WebSocketFrameChunk>* frame_chunks,

[szym, 2013/08/27 22:54:04]

Suggest: OnReadComplete or OnReadDone

[Adam Rice, 2013/08/28 12:01:11]

Done.

+                const CompletionCallback& callback,
+                int result);
+
+  // Storage for pending reads. All active WebSockets spend all the time with a
+  // call to ReadFrames() pending, so there is no benefit in trying to share
+  // this between sockets.
+  scoped_refptr<IOBufferWithSize> read_buffer_;
+
+  // The connection, wrapped in a ClientSocketHandle so that we can prevent it
+  // from being returned to the pool.

[szym, 2013/08/27 22:54:04]

connection_ is used only for its socket_. The socket is always Disconnected
after passed to this WebSocketBasicStream, so instead of passing
ClientSocketHandle you should pass the StreamSocket.

Then you can use it as:

ClientSocketHandle handle;
new WebSocketBasicStream(handle.PassSocket());

[szym, 2013/08/27 23:51:30]

Disregard this comment. akalin@ explained to me in
https://codereview.chromium.org/18792002/ that that'd be illegal and using CSH
is preferred.

[Adam Rice, 2013/08/28 12:01:11]

Done.

+  scoped_ptr<ClientSocketHandle> connection_;
+
+  // Only used during handshake. Some data may be left in this buffer after the
+  // handshake, in which case it will be picked up during the first call to
+  // ReadFrames().
+  scoped_refptr<GrowableIOBuffer> http_read_buffer_;

[szym, 2013/08/27 22:54:04]

This appears to be used only in tests. Does it have a future non-test use case?

[Adam Rice, 2013/08/28 12:01:11]

Yes. I left it in this CL because the first call to ReadFrames() after the
handshake is complete may need to consume this data.

[szym, 2013/08/28 21:17:54]

I understand that ReadFrames would use it if set, but in what non-test use case
is this field ever non-NULL? Why does it need to be a GrowableIOBuffer?

[Adam Rice, 2013/08/29 13:03:23]

I added a comment indicating why it is a GrowableIOBuffer.

The code to initialise this field is not in this CL, but I am afraid to remove
the test. If I forget to put the test back again, everything will appear to work
until someone suffers data loss.

Thank you for your patience.

+
+  // This keeps the current parse state (including any incomplete headers) and
+  // parses frames.
+  WebSocketFrameParser parser_;
+
+  // The negotated sub-protocol, or empty for none.
+  std::string sub_protocol_;
+
+  // The extensions negotiated with the remote server.
+  std::string extensions_;
+
+  // This can be overridden in tests to make the output deterministic. We don't
+  // use a Callback here because a function pointer is faster and good enough
+  // for our purposes.
+  WebSocketMaskingKeyGeneratorFunction generate_websocket_masking_key_;
+};
+
+}  // namespace net
+
+#endif  // NET_WEBSOCKETS_WEBSOCKET_BASIC_STREAM_H_