SPDY cleanup: use ParseFrameType and SerializeFrameType

net/spdy/spdy_framer.cc

 // Copyright (c) 2012 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.
 
 #include "net/spdy/spdy_framer.h"
 
 #include "base/lazy_instance.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/metrics/stats_counters.h"
 #include "base/third_party/valgrind/memcheck.h"
@@ skipping 451 matching lines @@
     case DATA:
       return "DATA";
     case SYN_STREAM:
       return "SYN_STREAM";
     case SYN_REPLY:
       return "SYN_REPLY";
     case RST_STREAM:
       return "RST_STREAM";
     case SETTINGS:
       return "SETTINGS";
-    case NOOP:
-      return "NOOP";
     case PING:
       return "PING";
     case GOAWAY:
       return "GOAWAY";
     case HEADERS:
       return "HEADERS";
     case WINDOW_UPDATE:
       return "WINDOW_UPDATE";
     case CREDENTIAL:
       return "CREDENTIAL";
@@ skipping 170 matching lines @@
 
   // Using a scoped_ptr here since we may need to create a new SpdyFrameReader
   // when processing DATA frames below.
   scoped_ptr<SpdyFrameReader> reader(
       new SpdyFrameReader(current_frame_buffer_.get(),
                           current_frame_buffer_length_));
 
   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;
   if (protocol_version() <= SPDY3) {
     bool successful_read = reader->ReadUInt16(&version);
     DCHECK(successful_read);
     is_control_frame = (version & kControlFlagMask) != 0;
     version &= ~kControlFlagMask;  // Only valid for control frames.
     if (is_control_frame) {
       // We check version before we check validity: version can never be
@@ skipping 27 matching lines @@
     successful_read = reader->ReadUInt24(&length_field);
     DCHECK(successful_read);
     remaining_data_length_ = length_field;
     current_frame_length_ = remaining_data_length_ + reader->GetBytesConsumed();
   } else {
     version = protocol_version();
     uint16 length_field = 0;
     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();
     } else {
       current_frame_length_ = length_field + GetDataFrameMinimumSize();
     }
 
     successful_read = reader->ReadUInt8(&current_frame_flags_);
     DCHECK(successful_read);
 
@@ skipping 76 matching lines @@
     ProcessControlFrameHeader(control_frame_type_field);
   }
 
   return original_len - len;
 }
 
 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;
       DCHECK_EQ(SPDY3, protocol_version());
       DVLOG(1) << "CREDENTIAL control frame found. Ignoring.";
       CHANGE_STATE(SPDY_IGNORE_REMAINING_PAYLOAD);
       return;
     }
   }
 
@@ skipping 2406 matching lines @@
   builder->Seek(compressed_size);
   builder->RewriteLength(*this);
 
   pre_compress_bytes.Add(uncompressed_len);
   post_compress_bytes.Add(compressed_size);
 
   compressed_frames.Increment();
 }
 
 }  // namespace net