Rename closing_* in WebSocketChannel to received_close_ and do some refactoring

Rename closing_* in WebSocketChannel to received_close_* and do some refactoring

- Variable renaming: they should be prefixed with received_ so that
  reviewer can understand that they store values received from the
  server
- HandleFrame() is just a part of ProcessFrame(). I want them to have
  names from which we can see that
- Factor out the switch-clause in HandleFrame building a frame type
  string as the method is very long

BUG=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=250983

[tyoshino (SeeGerritForStatus), 2014-02-10 11:38:21]

Some refactoring suggested.

[Adam Rice, 2014-02-10 12:00:28]

https://codereview.chromium.org/131333010/diff/30001/net/websockets/websocket...
File net/websockets/websocket_channel.h (right):

https://codereview.chromium.org/131333010/diff/30001/net/websockets/websocket...
net/websockets/websocket_channel.h:201: ChannelState HandleFrameBottomHalf(
"BottomHalf" implies that it's just an arbitrary split, which is not the case.
There's a logical divide between the first method and the second:

1. The first method performs sanity checks on the frame that are needed
regardless of the current state.
2. The second method receives only the information about the frame that it
needs, and performs the appropriate action(s) depending on the current state.

perhaps the second method could be called HandleFrameByState or if that is too
ambiguous HandleFrameAccordingToState?

https://codereview.chromium.org/131333010/diff/30001/net/websockets/websocket...
net/websockets/websocket_channel.h:306: uint16 received_close_code_;
This rename is good.

[tyoshino (SeeGerritForStatus), 2014-02-12 02:31:25]

https://codereview.chromium.org/131333010/diff/30001/net/websockets/websocket...
File net/websockets/websocket_channel.h (right):

https://codereview.chromium.org/131333010/diff/30001/net/websockets/websocket...
net/websockets/websocket_channel.h:201: ChannelState HandleFrameBottomHalf(
On 2014/02/10 12:00:28, Adam Rice wrote:
> "BottomHalf" implies that it's just an arbitrary split, which is not the case.
> There's a logical divide between the first method and the second:
> 
> 1. The first method performs sanity checks on the frame that are needed
> regardless of the current state.
> 2. The second method receives only the information about the frame that it
> needs, and performs the appropriate action(s) depending on the current state.
> 

Oh, got it!

> perhaps the second method could be called HandleFrameByState or if that is too
> ambiguous HandleFrameAccordingToState?

Renamed.

[Adam Rice, 2014-02-12 03:15:30]

lgtm

[tyoshino (SeeGerritForStatus), 2014-02-12 04:38:45]

The CQ bit was checked by tyoshino@chromium.org

[commit-bot: I haz the power, 2014-02-12 04:39:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tyoshino@chromium.org/131333010/90001

[commit-bot: I haz the power, 2014-02-12 04:39:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-12 04:39:33]

Failed to apply patch for net/websockets/websocket_channel.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file net/websockets/websocket_channel.cc
  Hunk #1 succeeded at 80 (offset 2 lines).
  Hunk #2 succeeded at 257 with fuzz 2 (offset 2 lines).
  Hunk #3 succeeded at 595 (offset 22 lines).
  Hunk #4 FAILED at 600.
  Hunk #5 succeeded at 653 (offset 24 lines).
  Hunk #6 succeeded at 668 (offset 24 lines).
  Hunk #7 succeeded at 742 (offset 39 lines).
  Hunk #8 succeeded at 751 (offset 39 lines).
  1 out of 8 hunks FAILED -- saving rejects to file
net/websockets/websocket_channel.cc.rej

Patch:       net/websockets/websocket_channel.cc
Index: net/websockets/websocket_channel.cc
diff --git a/net/websockets/websocket_channel.cc
b/net/websockets/websocket_channel.cc
index
94baafd5e72e9a96adaceed792c74fa9f9826ec7..836bf21c16e9f0f05b6ca90591bfe3796b330799
100644
--- a/net/websockets/websocket_channel.cc
+++ b/net/websockets/websocket_channel.cc
@@ -78,6 +78,38 @@ bool IsStrictlyValidCloseStatusCode(int code) {
 // This function avoids a bunch of boilerplate code.
 void AllowUnused(ChannelState ALLOW_UNUSED unused) {}
 
+// Sets |name| to the name of the frame type for the given |opcode|. Note that
+// for all of Text, Binary and Continuation opcode, this method returns
+// "Data frame".
+void GetFrameTypeForOpcode(WebSocketFrameHeader::OpCode opcode,
+                           std::string* name) {
+  switch (opcode) {
+    case WebSocketFrameHeader::kOpCodeText:    // fall-thru
+    case WebSocketFrameHeader::kOpCodeBinary:  // fall-thru
+    case WebSocketFrameHeader::kOpCodeContinuation:
+      *name = "Data frame";
+      break;
+
+    case WebSocketFrameHeader::kOpCodePing:
+      *name = "Ping";
+      break;
+
+    case WebSocketFrameHeader::kOpCodePong:
+      *name = "Pong";
+      break;
+
+    case WebSocketFrameHeader::kOpCodeClose:
+      *name = "Close";
+      break;
+
+    default:
+      *name = "Unknown frame type";
+      break;
+  }
+
+  return;
+}
+
 }  // namespace
 
 // A class to encapsulate a set of frames and information about the size of
@@ -223,7 +255,7 @@ WebSocketChannel::WebSocketChannel(
       send_quota_high_water_mark_(kDefaultSendQuotaHighWaterMark),
       current_send_quota_(0),
       timeout_(base::TimeDelta::FromSeconds(kClosingHandshakeTimeoutSeconds)),
-      closing_code_(0),
+      received_close_code_(0),
       state_(FRESHLY_CONSTRUCTED),
       notification_sender_(new HandshakeNotificationSender(this)) {}
 
@@ -541,7 +573,7 @@ ChannelState WebSocketChannel::OnReadDone(bool synchronous,
int result) {
       for (size_t i = 0; i < read_frames_.size(); ++i) {
         scoped_ptr<WebSocketFrame> frame(read_frames_[i]);
         read_frames_[i] = NULL;
-        if (ProcessFrame(frame.Pass()) == CHANNEL_DELETED)
+        if (HandleFrame(frame.Pass()) == CHANNEL_DELETED)
           return CHANNEL_DELETED;
       }
       read_frames_.clear();
@@ -568,15 +600,16 @@ ChannelState WebSocketChannel::OnReadDone(bool
synchronous, int result) {
       state_ = CLOSED;
       uint16 code = kWebSocketErrorAbnormalClosure;
       std::string reason = "";
-      if (closing_code_ != 0) {
-        code = closing_code_;
-        reason = closing_reason_;
+      if (received_close_code_ != 0) {
+        code = received_close_code_;
+        reason = received_close_reason_;
       }
       return DoDropChannel(code, reason);
   }
 }
 
-ChannelState WebSocketChannel::ProcessFrame(scoped_ptr<WebSocketFrame> frame) {
+ChannelState WebSocketChannel::HandleFrame(
+    scoped_ptr<WebSocketFrame> frame) {
   if (frame->header.masked) {
     // RFC6455 Section 5.1 "A client MUST close a connection if it detects a
     // masked frame."
@@ -597,11 +630,11 @@ ChannelState
WebSocketChannel::ProcessFrame(scoped_ptr<WebSocketFrame> frame) {
   }
 
   // Respond to the frame appropriately to its type.
-  return HandleFrame(
+  return HandleFrameByState(
       opcode, frame->header.final, frame->data, frame->header.payload_length);
 }
 
-ChannelState WebSocketChannel::HandleFrame(
+ChannelState WebSocketChannel::HandleFrameByState(
     const WebSocketFrameHeader::OpCode opcode,
     bool final,
     const scoped_refptr<IOBuffer>& data_buffer,
@@ -612,29 +645,8 @@ ChannelState WebSocketChannel::HandleFrame(
   DCHECK_NE(CLOSED, state_);
   if (state_ == CLOSE_WAIT) {
     std::string frame_name;
-    switch (opcode) {
-      case WebSocketFrameHeader::kOpCodeText:    // fall-thru
-      case WebSocketFrameHeader::kOpCodeBinary:  // fall-thru
-      case WebSocketFrameHeader::kOpCodeContinuation:
-        frame_name = "Data frame";
-        break;
-
-      case WebSocketFrameHeader::kOpCodePing:
-        frame_name = "Ping";
-        break;
-
-      case WebSocketFrameHeader::kOpCodePong:
-        frame_name = "Pong";
-        break;
-
-      case WebSocketFrameHeader::kOpCodeClose:
-        frame_name = "Close";
-        break;
-
-      default:
-        frame_name = "Unknown frame type";
-        break;
-    }
+    GetFrameTypeForOpcode(opcode, &frame_name);
+
     // FailChannel() won't send another Close frame.
     return FailChannel(
         frame_name + " received after close", kWebSocketErrorProtocolError,
"");
@@ -692,8 +704,8 @@ ChannelState WebSocketChannel::HandleFrame(
             return CHANNEL_DELETED;
           if (event_interface_->OnClosingHandshake() == CHANNEL_DELETED)
             return CHANNEL_DELETED;
-          closing_code_ = code;
-          closing_reason_ = reason;
+          received_close_code_ = code;
+          received_close_reason_ = reason;
           break;
 
         case SEND_CLOSED:
@@ -701,8 +713,8 @@ ChannelState WebSocketChannel::HandleFrame(
           // From RFC6455 section 7.1.5: "Each endpoint
           // will see the status code sent by the other end as _The WebSocket
           // Connection Close Code_."
-          closing_code_ = code;
-          closing_reason_ = reason;
+          received_close_code_ = code;
+          received_close_reason_ = reason;
           break;
 
         default:

[tyoshino (SeeGerritForStatus), 2014-02-13 08:55:18]

The CQ bit was checked by tyoshino@chromium.org

[commit-bot: I haz the power, 2014-02-13 08:55:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tyoshino@chromium.org/131333010/210001

[commit-bot: I haz the power, 2014-02-13 14:24:29]

Change committed as 250983