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;
}
