SPDY cleanup: use ParseFrameType and SerializeFrameType

net/spdy/spdy_framer.cc

Index: net/spdy/spdy_framer.cc
diff --git a/net/spdy/spdy_framer.cc b/net/spdy/spdy_framer.cc
index 88a05ea85967912be077675e8602ea1928f61b19..bad34035937139b249e01b2f5c73977871b2a539 100644
--- a/net/spdy/spdy_framer.cc
+++ b/net/spdy/spdy_framer.cc
@@ -469,8 +469,6 @@ const char* SpdyFramer::FrameTypeToString(SpdyFrameType type) {
       return "RST_STREAM";
     case SETTINGS:
       return "SETTINGS";
-    case NOOP:
-      return "NOOP";
     case PING:
       return "PING";
     case GOAWAY:
@@ -661,7 +659,8 @@ size_t SpdyFramer::ProcessCommonHeader(const char* data, size_t len) {
   uint16 version = 0;
   bool is_control_frame = false;
 
-  uint16 control_frame_type_field = DATA;
+  uint16 control_frame_type_field =
+    SpdyConstants::DataFrameType(protocol_version());
   // ProcessControlFrameHeader() will set current_frame_type_ to the
   // correct value if this is a valid control frame.
   current_frame_type_ = DATA;
@@ -709,13 +708,17 @@ size_t SpdyFramer::ProcessCommonHeader(const char* data, size_t len) {
     bool successful_read = reader->ReadUInt16(&length_field);
     DCHECK(successful_read);
 
-    uint8 control_frame_type_field_uint8 = DATA;
+    uint8 control_frame_type_field_uint8 =
+      SpdyConstants::DataFrameType(protocol_version());
     successful_read = reader->ReadUInt8(&control_frame_type_field_uint8);
     DCHECK(successful_read);
     // We check control_frame_type_field's validity in
     // ProcessControlFrameHeader().
     control_frame_type_field = control_frame_type_field_uint8;
-    is_control_frame = (control_frame_type_field != DATA);
+    is_control_frame = (protocol_version() > SPDY3) ?
+        control_frame_type_field !=
+            SpdyConstants::SerializeFrameType(protocol_version(), DATA) :
+        control_frame_type_field != 0;

[Ryan Hamilton, 2014/06/25 22:31:33]

This is a bit complicated. Consider adding
SpdyConstants::IsControlFrame(frame_type)

[Johnny, 2014/06/26 15:57:24]

Hmm. Think this needs a deeper re-factor. It's probably better done as:

is_control_frame = (DATA == SpdyConstants::ParseFrameType(protocol_version(),
control_frame_type_field));

or better:

control_frame_type_ = SpdyConstants::ParseFrameType(protocol_version(),
control_frame_type_field);
is_control_frame = (DATA == control_frame_type_);

But we haven't yet validated that the frame type is valid (thus the gymnastics
here, I think). I'll pass this along.

 
     if (is_control_frame) {
       current_frame_length_ = length_field + GetControlFrameHeaderSize();
@@ -812,14 +815,10 @@ void SpdyFramer::ProcessControlFrameHeader(uint16 control_frame_type_field) {
   DCHECK_EQ(SPDY_NO_ERROR, error_code_);
   DCHECK_LE(GetControlFrameHeaderSize(), current_frame_buffer_length_);
 
+  // TODO(mlavan): Either remove credential frames from the code entirely,
+  // or add them to parsing + serialization methods for SPDY3.
   // Early detection of deprecated frames that we ignore.
   if (protocol_version() <= SPDY3) {
-    if (control_frame_type_field == NOOP) {
-      current_frame_type_ = NOOP;
-      DVLOG(1) << "NOOP control frame found. Ignoring.";
-      CHANGE_STATE(SPDY_AUTO_RESET);
-      return;
-    }
 
     if (control_frame_type_field == CREDENTIAL) {
       current_frame_type_ = CREDENTIAL;