Misc cleanup in net/spdy/core.

net/spdy/core/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/core/spdy_framer.h"
 
 #include <string.h>
 
 #include <algorithm>
 #include <cctype>
 #include <ios>
 #include <iterator>
 #include <list>
 #include <new>
-#include <vector>
 
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_util.h"
 #include "net/quic/platform/api/quic_flags.h"
 #include "net/spdy/chromium/spdy_flags.h"
 #include "net/spdy/core/hpack/hpack_constants.h"
 #include "net/spdy/core/hpack/hpack_decoder.h"
 #include "net/spdy/core/hpack/hpack_decoder3.h"
 #include "net/spdy/core/http2_frame_decoder_adapter.h"
 #include "net/spdy/core/spdy_bitmasks.h"
 #include "net/spdy/core/spdy_bug_tracker.h"
 #include "net/spdy/core/spdy_frame_builder.h"
 #include "net/spdy/core/spdy_frame_reader.h"
 #include "net/spdy/core/spdy_framer_decoder_adapter.h"
 #include "net/spdy/platform/api/spdy_estimate_memory_usage.h"
 #include "net/spdy/platform/api/spdy_ptr_util.h"
 #include "net/spdy/platform/api/spdy_string_utils.h"
 
-using std::vector;
-
 namespace net {
 
 namespace {
 
 // Pack parent stream ID and exclusive flag into the format used by HTTP/2
 // headers and priority frames.
 uint32_t PackStreamDependencyValues(bool exclusive,
                                     SpdyStreamId parent_stream_id) {
   // Make sure the highest-order bit in the parent stream id is zeroed out.
   uint32_t parent = parent_stream_id & 0x7fffffff;
@@ skipping 967 matching lines @@
           expect_continuation_ = current_frame_stream_id_;
           end_stream_when_done_ = current_frame_flags_ & CONTROL_FLAG_FIN;
         }
         if (current_frame_flags_ & HEADERS_FLAG_PADDED) {
           uint8_t pad_payload_len = 0;
           DCHECK_EQ(remaining_padding_payload_length_, 0u);
           successful_read = reader.ReadUInt8(&pad_payload_len);
           DCHECK(successful_read);
           remaining_padding_payload_length_ = pad_payload_len;
         }
-        const bool has_priority =
-            (current_frame_flags_ & HEADERS_FLAG_PRIORITY) != 0;
         int weight = 0;
         uint32_t parent_stream_id = 0;
         bool exclusive = false;
-        if (has_priority) {
+        if (current_frame_flags_ & HEADERS_FLAG_PRIORITY) {
           uint32_t stream_dependency;
           successful_read = reader.ReadUInt32(&stream_dependency);
           DCHECK(successful_read);
           UnpackStreamDependencyValues(stream_dependency, &exclusive,
                                        &parent_stream_id);
 
           uint8_t serialized_weight = 0;
           successful_read = reader.ReadUInt8(&serialized_weight);
           if (successful_read) {
             // Per RFC 7540 section 6.3, serialized weight value is actual
             // value - 1.
             weight = serialized_weight + 1;
           }
         }
         DCHECK(reader.IsDoneReading());
         if (debug_visitor_) {
           debug_visitor_->OnReceiveCompressedFrame(current_frame_stream_id_,
                                                    current_frame_type_,
                                                    current_frame_length_);
         }
         visitor_->OnHeaders(current_frame_stream_id_,
                             (current_frame_flags_ & HEADERS_FLAG_PRIORITY) != 0,
                             weight, parent_stream_id, exclusive,
                             (current_frame_flags_ & CONTROL_FLAG_FIN) != 0,
                             expect_continuation_ == 0);
+      } break;

[Biren Roy, 2017/05/24 15:25:09]

This line looks weird, but I see it's the same in the internal version.

[Bence, 2017/05/24 15:38:48]

Yeah, I agree.  Maybe if I move all local variable declaration outside of the
switch statement, then we can get rid of the per-case curly brackets.

+      case SpdyFrameType::PUSH_PROMISE: {
+        if (current_frame_stream_id_ == 0) {
+          set_error(SPDY_INVALID_CONTROL_FRAME);
+          return original_len - len;
         }
-        break;
-        case SpdyFrameType::PUSH_PROMISE: {
-          if (current_frame_stream_id_ == 0) {
-            set_error(SPDY_INVALID_CONTROL_FRAME);
-            return original_len - len;
-          }
-          bool successful_read = true;
-          if (current_frame_flags_ & PUSH_PROMISE_FLAG_PADDED) {
-            DCHECK_EQ(remaining_padding_payload_length_, 0u);
-            uint8_t pad_payload_len = 0;
-            successful_read = reader.ReadUInt8(&pad_payload_len);
-            DCHECK(successful_read);
-            remaining_padding_payload_length_ = pad_payload_len;
-          }
+        bool successful_read = true;
+        if (current_frame_flags_ & PUSH_PROMISE_FLAG_PADDED) {
+          DCHECK_EQ(remaining_padding_payload_length_, 0u);
+          uint8_t pad_payload_len = 0;
+          successful_read = reader.ReadUInt8(&pad_payload_len);
+          DCHECK(successful_read);
+          remaining_padding_payload_length_ = pad_payload_len;
         }
-        {
-          SpdyStreamId promised_stream_id = kInvalidStream;
-          bool successful_read = reader.ReadUInt31(&promised_stream_id);
-          DCHECK(successful_read);
-          DCHECK(reader.IsDoneReading());
-          if (promised_stream_id == 0) {
-            set_error(SPDY_INVALID_CONTROL_FRAME);
-            return original_len - len;
-          }
-          if (!(current_frame_flags_ & PUSH_PROMISE_FLAG_END_PUSH_PROMISE)) {
-            expect_continuation_ = current_frame_stream_id_;
-          }
-          if (debug_visitor_) {
-            debug_visitor_->OnReceiveCompressedFrame(
-                current_frame_stream_id_,
-                current_frame_type_,
-                current_frame_length_);
-          }
-          visitor_->OnPushPromise(current_frame_stream_id_,
-                                  promised_stream_id,
-                                  (current_frame_flags_ &
-                                   PUSH_PROMISE_FLAG_END_PUSH_PROMISE) != 0);
+        SpdyStreamId promised_stream_id = kInvalidStream;
+        successful_read = reader.ReadUInt31(&promised_stream_id);
+        DCHECK(successful_read);
+        DCHECK(reader.IsDoneReading());
+        if (promised_stream_id == 0) {
+          set_error(SPDY_INVALID_CONTROL_FRAME);
+          return original_len - len;
         }
-        break;
-        case SpdyFrameType::CONTINUATION: {
-          // Check to make sure the stream id of the current frame is
-          // the same as that of the preceding frame.
-          // If we're at this point we should already know that
-          // expect_continuation_ != 0, so this doubles as a check
-          // that current_frame_stream_id != 0.
-          if (current_frame_stream_id_ != expect_continuation_) {
-            set_error(SPDY_UNEXPECTED_FRAME);
-            return original_len - len;
-          }
-          if (current_frame_flags_ & HEADERS_FLAG_END_HEADERS) {
-            expect_continuation_ = 0;
-          }
-          if (debug_visitor_) {
-            debug_visitor_->OnReceiveCompressedFrame(
-                current_frame_stream_id_,
-                current_frame_type_,
-                current_frame_length_);
-          }
-          visitor_->OnContinuation(current_frame_stream_id_,
-                                   (current_frame_flags_ &
-                                    HEADERS_FLAG_END_HEADERS) != 0);
+        if (!(current_frame_flags_ & PUSH_PROMISE_FLAG_END_PUSH_PROMISE)) {
+          expect_continuation_ = current_frame_stream_id_;
         }
-        break;
+        if (debug_visitor_) {
+          debug_visitor_->OnReceiveCompressedFrame(current_frame_stream_id_,
+                                                   current_frame_type_,
+                                                   current_frame_length_);
+        }
+        visitor_->OnPushPromise(
+            current_frame_stream_id_, promised_stream_id,
+            (current_frame_flags_ & PUSH_PROMISE_FLAG_END_PUSH_PROMISE) != 0);
+      } break;
+      case SpdyFrameType::CONTINUATION: {
+        // Check to make sure the stream id of the current frame is
+        // the same as that of the preceding frame.
+        // If we're at this point we should already know that
+        // expect_continuation_ != 0, so this doubles as a check
+        // that current_frame_stream_id != 0.
+        if (current_frame_stream_id_ != expect_continuation_) {
+          set_error(SPDY_UNEXPECTED_FRAME);
+          return original_len - len;
+        }
+        if (current_frame_flags_ & HEADERS_FLAG_END_HEADERS) {
+          expect_continuation_ = 0;
+        }
+        if (debug_visitor_) {
+          debug_visitor_->OnReceiveCompressedFrame(current_frame_stream_id_,
+                                                   current_frame_type_,
+                                                   current_frame_length_);
+        }
+        visitor_->OnContinuation(
+            current_frame_stream_id_,
+            (current_frame_flags_ & HEADERS_FLAG_END_HEADERS) != 0);
+      } break;
       default:
 #ifndef NDEBUG
         LOG(FATAL) << "Invalid control frame type: " << current_frame_type_;
 #else
         set_error(SPDY_INVALID_CONTROL_FRAME);
         return original_len - len;
 #endif
     }
 
     if (current_frame_type_ != SpdyFrameType::CONTINUATION) {
@@ skipping 484 matching lines @@
                                   ? GetFrameSizeSansBlock()
                                   : framer_->GetContinuationMinimumSize();
   auto encoding = SpdyMakeUnique<SpdyString>();
   encoder_->Next(kMaxControlFrameSize - size_without_block, encoding.get());
   has_next_frame_ = encoder_->HasNext();
 
   if (framer_->debug_visitor_ != nullptr) {
     debug_total_size_ += size_without_block;
     debug_total_size_ += encoding->size();
     if (!has_next_frame_) {
-      auto* header_block_frame_ir =
-          static_cast<const SpdyFrameWithHeaderBlockIR*>(frame_ir);
       // TODO(birenroy) are these (here and below) still necessary?
       // HTTP2 uses HPACK for header compression. However, continue to
       // use GetSerializedLength() for an apples-to-apples comparision of
       // compression performance between HPACK and SPDY w/ deflate.
+      auto* header_block_frame_ir =
+          static_cast<const SpdyFrameWithHeaderBlockIR*>(frame_ir);
       size_t debug_payload_len =
           framer_->GetSerializedLength(&header_block_frame_ir->header_block());
       framer_->debug_visitor_->OnSendCompressedFrame(
           frame_ir->stream_id(), frame_ir->frame_type(), debug_payload_len,
           debug_total_size_);
     }
   }
 
   framer_->SetIsLastFrame(!has_next_frame_);
   const size_t free_bytes_before = output->BytesFree();
@@ skipping 1197 matching lines @@
   if (hpack_decoder_.get() == nullptr) {
     if (FLAGS_chromium_http2_flag_spdy_use_hpack_decoder3) {
       hpack_decoder_ = SpdyMakeUnique<HpackDecoder3>();
     } else {
       hpack_decoder_ = SpdyMakeUnique<HpackDecoder>();
     }
   }
   return hpack_decoder_.get();
 }
 
-void SpdyFramer::SetDecoderHeaderTableDebugVisitor(
-    std::unique_ptr<HpackHeaderTable::DebugVisitorInterface> visitor) {
-  if (decoder_adapter_ != nullptr) {
-    decoder_adapter_->SetDecoderHeaderTableDebugVisitor(std::move(visitor));
-  } else {
-    GetHpackDecoder()->SetHeaderTableDebugVisitor(std::move(visitor));
-  }
-}
-
-void SpdyFramer::SetEncoderHeaderTableDebugVisitor(
-    std::unique_ptr<HpackHeaderTable::DebugVisitorInterface> visitor) {
-  GetHpackEncoder()->SetHeaderTableDebugVisitor(std::move(visitor));
-}
-
-size_t SpdyFramer::EstimateMemoryUsage() const {
-  return SpdyEstimateMemoryUsage(current_frame_buffer_) +
-         SpdyEstimateMemoryUsage(settings_scratch_) +
-         SpdyEstimateMemoryUsage(altsvc_scratch_) +
-         SpdyEstimateMemoryUsage(hpack_encoder_) +
-         SpdyEstimateMemoryUsage(hpack_decoder_) +
-         SpdyEstimateMemoryUsage(decoder_adapter_);
-}
-
 void SpdyFramer::UpdateHeaderEncoderTableSize(uint32_t value) {
   GetHpackEncoder()->ApplyHeaderTableSizeSetting(value);
 }
 
 void SpdyFramer::UpdateHeaderDecoderTableSize(uint32_t value) {
   GetHpackDecoder()->ApplyHeaderTableSizeSetting(value);
 }
 
 size_t SpdyFramer::header_encoder_table_size() const {
   if (hpack_encoder_ == nullptr) {
     return kDefaultHeaderTableSizeSetting;
   } else {
     return hpack_encoder_->CurrentHeaderTableSizeSetting();
   }
 }
 
 void SpdyFramer::SerializeHeaderBlockWithoutCompression(
     SpdyFrameBuilder* builder,
     const SpdyHeaderBlock& header_block) const {
   // Serialize number of headers.
   builder->WriteUInt32(header_block.size());
 
   // Serialize each header.
   for (const auto& header : header_block) {
     builder->WriteStringPiece32(base::ToLowerASCII(header.first));
     builder->WriteStringPiece32(header.second);
   }
 }
 
+void SpdyFramer::SetDecoderHeaderTableDebugVisitor(
+    std::unique_ptr<HpackHeaderTable::DebugVisitorInterface> visitor) {
+  if (decoder_adapter_ != nullptr) {
+    decoder_adapter_->SetDecoderHeaderTableDebugVisitor(std::move(visitor));
+  } else {
+    GetHpackDecoder()->SetHeaderTableDebugVisitor(std::move(visitor));
+  }
+}
+
+void SpdyFramer::SetEncoderHeaderTableDebugVisitor(
+    std::unique_ptr<HpackHeaderTable::DebugVisitorInterface> visitor) {
+  GetHpackEncoder()->SetHeaderTableDebugVisitor(std::move(visitor));
+}
+
+size_t SpdyFramer::EstimateMemoryUsage() const {
+  return SpdyEstimateMemoryUsage(current_frame_buffer_) +
+         SpdyEstimateMemoryUsage(settings_scratch_) +
+         SpdyEstimateMemoryUsage(altsvc_scratch_) +
+         SpdyEstimateMemoryUsage(hpack_encoder_) +
+         SpdyEstimateMemoryUsage(hpack_decoder_) +
+         SpdyEstimateMemoryUsage(decoder_adapter_);
+}
+
 }  // namespace net