WebSocketChannel implementation

net/websockets/websocket_channel.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_CHANNEL_H_
+#define NET_WEBSOCKETS_WEBSOCKET_CHANNEL_H_
+
+#include <string>
+#include <vector>
+
+#include "base/basictypes.h"
+#include "base/callback.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/scoped_vector.h"
+#include "googleurl/src/gurl.h"
+#include "net/base/net_export.h"
+#include "net/websockets/websocket_frame.h"
+#include "net/websockets/websocket_stream.h"
+
+namespace net {
+
+class URLRequestContext;
+class WebSocketEventInterface;
+
+// Transport-independent implementation of WebSockets. Implements protocol
+// semantics that do not depend on the underlying transport. Provides the
+// interface to the content layer.

[szym, 2013/07/09 15:59:51]

Add link to the RFC: http://tools.ietf.org/html/rfc6455
so that concepts such as "Close frame" and "Closing Handshake" have reference.

[Adam Rice, 2013/07/11 13:28:17]

Done.

+class NET_EXPORT WebSocketChannel {
+ public:
+  // Creates a new WebSocketChannel with the specified parameters.
+  // SendAddChannelRequest() must be called immediately afterwards to start the
+  // connection process.
+  WebSocketChannel(const GURL& socket_url,
+                   scoped_ptr<WebSocketEventInterface> event_interface);
+  virtual ~WebSocketChannel();
+
+  // Starts the connection process.
+  void SendAddChannelRequest(
+      const std::vector<std::string>& requested_protocols,
+      const GURL& origin,
+      URLRequestContext* url_request_context);
+
+  // Sends a data frame to the remote side. The frame should usually be no
+  // larger than 32KB to avoid blocking the IO thread. This method has a hard

[szym, 2013/07/09 15:59:51]

Why would this block the IO thread? Do you mean the time spent by the CPU
executing the syscalls, or do you imply this call would actually idly block
until a network event occurs?

[Adam Rice, 2013/07/11 13:28:17]

I am mostly talking about the memory copy overhead. I'm not sure whether I
should also be concerned about blocking the IPC channel. I have changed the
wording of the comment to try to make it clear I am not talking about blocking
IO or similar.

+  // limit of 2GB. It is the responsibility of the caller to ensure that they
+  // have sufficient send quota to send this data, otherwise the connection will
+  // be closed without sending.
+  void SendFrame(bool fin,
+                 WebSocketFrameHeader::OpCode op_code,
+                 const std::vector<char>& data);
+
+  // Sends "quota" units of flow control to the remote side. If the underlying

[szym, 2013/07/09 15:59:51]

nit: Use pipes, i.e.:
Sends |quota| units.

Are those units bytes? If so, use "bytes" instead of "units" (or "quota amount")
everywhere. If not, explain the relationship somewhere.

[Adam Rice, 2013/07/11 13:28:17]

Currently they are implemented as bytes. When we have multiplexing implemented
we might have a situation where the method to count quota depends on the type of
multiplexing in use (ie. WebSocket multiplexing or SPDY/HTTP2.0). In which case
we will have to tell the rendering process in the OnAddChannelResponse() message
what kind of quota counting to use for the channel.

I have changed the description to "quota units" everywhere and noted that "quota
units" currently means bytes in a couple of places.

+  // transport has a concept of "quota", then it permits the remote server to
+  // send up to "quota" units of data.
+  void SendFlowControl(int64 quota);
+
+  // Start the closing handshake for a client-initiated shutdown of the
+  // connection. There is no API to close the connection without a closing
+  // handshake, but destroying the WebSocketChannel object while connected will
+  // effectively do that. |code| must be in the range 1000-4999. |reason| should
+  // be a valid UTF-8 string or empty.
+  //
+  // This does *not* trigger the event OnClosingHandshake(). The caller should
+  // assume that the closing handshake has started and perform the equivalent
+  // processing to OnClosingHandshake() if necessary.
+  void StartClosingHandshake(uint16 code, const std::string& reason);
+
+  // Starts the connection process, using a different factory function from the
+  // default. This is used for testing.

[szym, 2013/07/09 15:59:51]

Change "a different factory" to "specified factory", and 
"used for testing" to "exposed for testing." 
The goal is to remove doubt that this is *only* for testing.

I'd also suggest making this private and exposing it only as
SendAddChannelRequestForTesting, so that the presubmit check can verify that
this is called from tests only.

[Adam Rice, 2013/07/11 13:28:17]

That is better, thank you.

+  void SendAddChannelRequestWithFactory(
+      const std::vector<std::string>& requested_protocols,
+      const GURL& origin,
+      URLRequestContext* url_request_context,
+      base::Callback<scoped_ptr<WebSocketStreamRequest>(

[szym, 2013/07/09 15:59:51]

Add a typedef for this Callback type, even if it's used for testing only.

[Adam Rice, 2013/07/11 13:28:17]

Done.

+          const GURL&,
+          const std::vector<std::string>&,
+          const GURL&,
+          URLRequestContext*,
+          const BoundNetLog&,
+          scoped_ptr<WebSocketStream::ConnectDelegate>)> factory);
+
+ private:
+  // We have a simple linear progression of states from FRESHLY_CONSTRUCTED to
+  // CLOSED, except that the SEND_CLOSED and RECV_CLOSED states may be skipped
+  // in case of error.
+  enum State {
+    FRESHLY_CONSTRUCTED,
+    CONNECTING,
+    CONNECTED,
+    SEND_CLOSED,  // We have sent a Close frame but not received a Close frame.
+    RECV_CLOSED,  // Used briefly between receiving a Close frame and sending
+                  // the response. Once we have responded, the state changes
+                  // to CLOSED.
+    CLOSED,  // The Closing Handshake has completed or the connection is failed.
+  };
+
+  // When failing a channel, we may or may not want to send the real reason for
+  // failing to the remote server. This enum is used by FailChannel() to
+  // choose.
+  enum ExposeError {
+    SEND_REAL_ERROR,
+    SEND_GOING_AWAY,
+  };
+
+  // Our implementation of WebSocketStream::ConnectDelegate. We do not inherit
+  // from WebSocketStream::ConnectDelegate directly to avoid cluttering our
+  // public interface with the implementation of those methods, and because the
+  // lifetime of a WebSocketChannel is longer than the lifetime of the
+  // connection process.
+  class ConnectDelegate;
+
+  // Success callback from WebSocketStream::CreateAndConnectStream(). Reports
+  // success to the event interface.
+  void OnConnectSuccess(scoped_ptr<WebSocketStream> stream);
+
+  // Failure callback from WebSocketStream::CreateAndConnectStream(). Reports
+  // failure to the event interface.
+  void OnConnectFailure(uint16 websocket_error);
+
+  // Calls WebSocketStream::WriteFrames() with the appropriate arguments
+  void WriteFrames();
+
+  // Callback from WebSocketStream::WriteFrames. Sends pending data or adjusts
+  // the send quota of the renderer channel as appropriate. |result| is a met
+  // error code, usually OK. If |synchronous| is true, then OnWriteDone() is
+  // being called from within the WriteFrames() loop and does not need to call
+  // WriteFrames() itself.
+  void OnWriteDone(bool synchronous, int result);
+
+  // Calls WebSocketStream::ReadFrames() with the appropriate arguments.
+  void ReadFrames();
+
+  // Callback from WebSocketStream::ReadFrames. Handles any errors and processes
+  // the returned chunks appropriately to their type. If |synchronous| is true,
+  // is being called directly from within ReadFrames(). |result| is a net error

[szym, 2013/07/09 15:59:51]

Since this is also called from a loop within ReadFrames (just a WriteFrames), I
suggest keeping the comment parallel to the one above OnWriteDone, or referring
this mechanic here. 

[Adam Rice, 2013/07/11 13:28:17]

Done.

+  // code.
+  void OnReadDone(bool synchronous, int result);
+
+  // Processes a single chunk that has been read from the stream.
+  void ProcessFrameChunk(scoped_ptr<WebSocketFrameChunk> chunk);
+
+  // Handle a frame that we have received enough of to process. May call
+  // event_interface_ methods, send responses to the server, and change the
+  // value of state_.
+  void HandleFrame(const WebSocketFrameHeader::OpCode opcode,
+                   const bool is_first_chunk,
+                   const bool is_final_chunk,

[szym, 2013/07/09 15:59:51]

remove consts from variables passed by value

[Adam Rice, 2013/07/11 13:28:17]

Done.

+                   const scoped_refptr<IOBufferWithSize>& data_buffer);
+
+  // Low-level method to send a single frame. Used for both data and control
+  // frames. Either sends the frame immediately or buffers it to be scheduled
+  // when the current write finishes.

[szym, 2013/07/09 15:59:51]

What does |fin| do?

[Adam Rice, 2013/07/11 13:28:17]

I added some explanation to SendFrame(), and the referred back to it from here.

+  void SendIOBufferWithSize(bool fin,
+                            WebSocketFrameHeader::OpCode op_code,
+                            const scoped_refptr<IOBufferWithSize>& buffer);
+
+  // Perform the "Fail the WebSocket Connection" operation as defined in
+  // RFC6455. The supplied code and reason are sent back to the renderer in an
+  // OnDropChannel message. If state_ is CONNECTED then a Close message is sent
+  // to the remote host. If |expose| is SEND_REAL_ERROR then the remote host is
+  // given the same status code we gave the renderer; otherwise it is sent a
+  // fixed "Going Away" code.  Resets current_frame_header_, closes the
+  // stream_, and sets state_ to CLOSED.

[szym, 2013/07/09 15:59:51]

Suggest adding a comment that OnDropChannel is allowed to destroy this
WebSocketChannel.

[Adam Rice, 2013/07/11 13:28:17]

I did the opposite and took away OnDropChannel's destroying privileges, although
I could still go the other way.

+  void FailChannel(ExposeError expose, uint16 code, const std::string& reason);
+
+  // Sends a close frame to Start the WebSocket Closing Handshake, or to respond
+  // to a close frame from the server.

[szym, 2013/07/09 15:59:51]

Be consistent: "a Close frame".

[Adam Rice, 2013/07/11 13:28:17]

Done.

+  void SendClose(uint16 code, const std::string& reason);
+
+  // Parses a Close frame. If no status code is supplied, then |code| is set to
+  // 1005 (No status code) with empty |reason|. If the supplied code is
+  // outside the valid range, then 1002 (Protocol error) is set instead. If the
+  // reason text is not valid UTF-8, then |reason| is set to an empty string
+  // instead.
+  void ParseClose(const scoped_refptr<IOBufferWithSize>& buffer,
+                  uint16* code,
+                  std::string* reason);
+
+  // The URL to which we connect.
+  const GURL socket_url_;
+
+  // The event_interface_ object to call back into.

[szym, 2013/07/09 15:59:51]

"event_interface_" is unnecessary in the comment. Suggest:
// The object receiving events.

[Adam Rice, 2013/07/11 13:28:17]

Good point. Done.

+  const scoped_ptr<WebSocketEventInterface> event_interface_;
+
+  // The WebSocketStream to which we are sending/receiving data.
+  scoped_ptr<WebSocketStream> stream_;
+
+  // A data structure containing a vector of frames to be sent and the total
+  // number of bytes contained in the vector.
+  struct SendBuffer;
+  // Data that is currently pending write, or NULL if no write is pending.
+  scoped_ptr<SendBuffer> data_being_sent_;
+  // Data that is queued up to write after the current write completes.
+  // Only non-NULL when such data actually exists.
+  scoped_ptr<SendBuffer> data_to_send_next_;
+
+  // Destination for the current call to WebSocketStream::ReadFrames
+  ScopedVector<WebSocketFrameChunk> read_frame_chunks_;
+  // Frame header for the frame currently being received. Only non-NULL while we
+  // are processing the frame. If the frame arrives in multiple chunks, can
+  // remain non-NULL while we wait for additional chunks to arrive. If the
+  // header of the frame was invalid, this is set to NULL, the channel is
+  // failed, and subsequent chunks of the same frame will be ignored.
+  scoped_ptr<WebSocketFrameHeader> current_frame_header_;
+  // Handle to an in-progress WebSocketStream creation request. Only non-NULL
+  // during the connection process.
+  scoped_ptr<WebSocketStreamRequest> stream_request_;
+  // Although it will almost never happen in practice, we can be passed an
+  // incomplete control frame, in which case we need to keep the data around
+  // long enough to reassemble it. This variable will be NULL the rest of the
+  // time.
+  scoped_refptr<IOBufferWithSize> incomplete_control_frame_body_;
+  // The point at which we give the renderer a quota refresh (bytes).
+  int send_quota_low_water_mark_;
+  // The amount which we refresh the quota to when it reaches the
+  // low_water_mark (bytes).
+  int send_quota_high_water_mark_;
+  // The current amount of quota that the renderer has available for sending
+  // on this logical channel (bytes).
+  int current_send_quota_;

[szym, 2013/07/09 15:59:51]

Why not use an unsigned type for the quotas so that you don't need checked
casts?

If it's actually possible for it to go negative, then you should not use checked
casts without first checking that case.

[Adam Rice, 2013/07/11 13:28:17]

The style guide seems to be pulling me in both directions here.

In favour of int:

1. "Do not use unsigned types to mean "this value should never be < 0"." from
http://www.chromium.org/developers/coding-style .
2. "You should not use the unsigned integer types such as uint32_t, unless there
is a valid reason such as representing a bit pattern rather than a number"
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Intege...
3. IOBufferWithSize uses int as its size type.

In favour of size_t:

1. "Use unsigned integer types (preferably size_t) for ... object counts... .
The signed types are incorrect and unsafe for these purposes." from
http://www.chromium.org/developers/coding-style
2. std::vector<char> uses size_t as its size type.

+
+  // Storage for the status code and reason from the time we receive the Close
+  // frame until the connection is closed and we can call OnDropChannel().
+  uint16 closing_code_;
+  std::string closing_reason_;
+
+  // The current state of the channel. Mainly used for sanity checking, but also
+  // used to track the close state.
+  State state_;
+
+  DISALLOW_COPY_AND_ASSIGN(WebSocketChannel);
+};
+
+}  // namespace net
+
+#endif  // NET_WEBSOCKETS_WEBSOCKET_CHANNEL_H_