Remove some obsolete TODOs from QUIC code that were owned by Frank and rewrite some others. gfe-relnote: n/a (Clean-up TODOs) PiperOrigin-RevId: 285181290 Change-Id: Ibc49c594fc914047e352a92eb34afe2567096140
diff --git a/quic/core/frames/quic_blocked_frame.h b/quic/core/frames/quic_blocked_frame.h index a4be664..30dcdf4 100644 --- a/quic/core/frames/quic_blocked_frame.h +++ b/quic/core/frames/quic_blocked_frame.h
@@ -30,13 +30,9 @@ // and non-zero when sent. QuicControlFrameId control_frame_id; - // The stream this frame applies to. 0 is a special case meaning the overall - // connection rather than a specific stream. - // - // For IETF QUIC, the stream_id controls whether an IETF QUIC - // BLOCKED or STREAM_BLOCKED frame is generated. - // If stream_id is 0 then a BLOCKED frame is generated and transmitted, - // if non-0, a STREAM_BLOCKED. + // 0 is a special case meaning the connection is blocked, rather than a + // stream. So stream_id 0 corresponds to a BLOCKED frame and non-0 + // corresponds to a STREAM_BLOCKED. // TODO(fkastenholz): This should be converted to use // QuicUtils::GetInvalidStreamId to get the correct invalid stream id value // and not rely on 0.
diff --git a/quic/core/quic_constants.h b/quic/core/quic_constants.h index 0c6c4ff..0b087b4 100644 --- a/quic/core/quic_constants.h +++ b/quic/core/quic_constants.h
@@ -223,7 +223,6 @@ const uint64_t kMaxIetfVarInt = UINT64_C(0x3fffffffffffffff); // The maximum stream id value that is supported - (2^32)-1 -// TODO(fkastenholz): Should update this to 64 bits for IETF Quic. const QuicStreamId kMaxQuicStreamId = 0xffffffff; // The maximum value that can be stored in a 32-bit QuicStreamCount.
diff --git a/quic/core/quic_framer.cc b/quic/core/quic_framer.cc index c5708b9..51f4aec 100644 --- a/quic/core/quic_framer.cc +++ b/quic/core/quic_framer.cc
@@ -3024,8 +3024,6 @@ if (!ProcessMaxDataFrame(reader, &frame)) { return RaiseError(QUIC_INVALID_MAX_DATA_FRAME_DATA); } - // TODO(fkastenholz): Or should we create a new visitor function, - // OnMaxDataFrame()? QUIC_DVLOG(2) << ENDPOINT << "Processing IETF max data frame " << frame; if (!visitor_->OnWindowUpdateFrame(frame)) { @@ -3040,8 +3038,6 @@ if (!ProcessMaxStreamDataFrame(reader, &frame)) { return RaiseError(QUIC_INVALID_MAX_STREAM_DATA_FRAME_DATA); } - // TODO(fkastenholz): Or should we create a new visitor function, - // OnMaxStreamDataFrame()? QUIC_DVLOG(2) << ENDPOINT << "Processing IETF max stream data frame " << frame; if (!visitor_->OnWindowUpdateFrame(frame)) { @@ -3113,7 +3109,6 @@ if (!ProcessStreamsBlockedFrame(reader, &frame, frame_type)) { return RaiseError(QUIC_STREAMS_BLOCKED_DATA); } - QUIC_CODE_COUNT_N(quic_streams_blocked_received, 1, 2); QUIC_DVLOG(2) << ENDPOINT << "Processing IETF streams blocked frame " << frame; if (!visitor_->OnStreamsBlockedFrame(frame)) { @@ -5944,9 +5939,6 @@ return false; } frame->unidirectional = (frame_type == IETF_STREAMS_BLOCKED_UNIDIRECTIONAL); - - // TODO(fkastenholz): handle properly when the STREAMS_BLOCKED - // frame is implemented and passed up to the stream ID manager. if (frame->stream_count > QuicUtils::GetMaxStreamCount( (frame_type == IETF_STREAMS_BLOCKED_UNIDIRECTIONAL),
diff --git a/quic/core/quic_packet_creator.cc b/quic/core/quic_packet_creator.cc index f95fe63..b223205 100644 --- a/quic/core/quic_packet_creator.cc +++ b/quic/core/quic_packet_creator.cc
@@ -329,17 +329,6 @@ return BytesFree() >= message_frame_size; } -// TODO(fkastenholz): this method should not use constant values for -// the last-frame-in-packet and data-length parameters to -// GetMinStreamFrameSize. Proper values should be plumbed in from -// higher up. This was left this way for now for a few reasons. First, -// higher up calls to StreamFramePacketOverhead() do not always know -// this information, leading to a cascade of changes and B) the -// higher-up software does not always loop, calling -// StreamFramePacketOverhead() once for every packet -- eg there is -// a test in quic_connection_test that calls it once and assumes that -// the value is the same for all packets. - // static size_t QuicPacketCreator::StreamFramePacketOverhead( QuicTransportVersion version, @@ -357,21 +346,10 @@ packet_number_length, retry_token_length_length, 0, length_length) + - // Assumes this is a packet with a single stream frame in it. Since - // last_frame_in_packet is set true, the size of the length field is - // not included in the calculation. This is OK because in other places - // in the code, the logic adds back 2 (the size of the Google QUIC - // length) when a frame is not the last frame of the packet. This is - // also acceptable for IETF Quic; even though the length field could be - // 8 bytes long, in practice it will not be longer than 2 bytes (enough - // to encode 16K). A length that would be encoded in 2 bytes (0xfff) - // is passed just for cleanliness. - // - // TODO(fkastenholz): This is very hacky and feels brittle. Ideally we - // would calculate the correct lengths at the correct time, based on - // the state at that time/place. + // Assumes a packet with a single stream frame, which omits the length, + // causing the data length argument to be ignored. QuicFramer::GetMinStreamFrameSize(version, 1u, offset, true, - kMaxOutgoingPacketSize); + kMaxOutgoingPacketSize /* unused */); } void QuicPacketCreator::CreateStreamFrame(QuicStreamId id,
diff --git a/quic/core/quic_stream_id_manager.cc b/quic/core/quic_stream_id_manager.cc index d9f3477..0d570d2 100644 --- a/quic/core/quic_stream_id_manager.cc +++ b/quic/core/quic_stream_id_manager.cc
@@ -69,8 +69,6 @@ const QuicStreamsBlockedFrame& frame) { // Ensure that the frame has the correct directionality. DCHECK_EQ(frame.unidirectional, unidirectional_); - QUIC_CODE_COUNT_N(quic_streams_blocked_received, 2, 2); - if (frame.stream_count > incoming_advertised_max_streams_) { // Peer thinks it can send more streams that we've told it. // This is a protocol error.
diff --git a/quic/core/quic_types.h b/quic/core/quic_types.h index b14e513..5a8ce07 100644 --- a/quic/core/quic_types.h +++ b/quic/core/quic_types.h
@@ -26,7 +26,6 @@ typedef uint32_t QuicMessageId; typedef uint64_t QuicDatagramFlowId; -// TODO(fkastenholz): Should update this to 64 bits for V99. typedef uint32_t QuicStreamId; // Count of stream IDs. Used in MAX_STREAMS and STREAMS_BLOCKED
diff --git a/quic/core/quic_utils.cc b/quic/core/quic_utils.cc index a69efcd..8529dca 100644 --- a/quic/core/quic_utils.cc +++ b/quic/core/quic_utils.cc
@@ -572,18 +572,10 @@ QuicStringPiece(connection_id.data(), connection_id.length())); } -// Returns the maximum value that a stream count may have, taking into account -// the fact that bidirectional, client initiated, streams have one fewer stream -// available than the others. This is because the old crypto streams, with ID == -// 0 are not included in the count. -// The version is not included in the call, nor does the method take the version -// into account, because this is called only from code used for IETF QUIC. -// TODO(fkastenholz): Remove this method and replace calls to it with direct -// references to kMaxQuicStreamIdCount when streamid 0 becomes a normal stream -// id. // static QuicStreamCount QuicUtils::GetMaxStreamCount(bool unidirectional, Perspective perspective) { + // gQUIC uses one client initiated bidi stream for the crypto stream. if (!unidirectional && perspective == Perspective::IS_CLIENT) { return kMaxQuicStreamCount >> 2; }