Clean up HpackVarintEncoder interface.
Turns out no caller makes use of the incremental encoding feature.
gfe-relnote: No functional change; not flag protected.
PiperOrigin-RevId: 254798717
Change-Id: I07c0519be47321f25ec006061fda39791202c348
diff --git a/http2/hpack/tools/hpack_block_builder.cc b/http2/hpack/tools/hpack_block_builder.cc
index 5a79f78..be62346 100644
--- a/http2/hpack/tools/hpack_block_builder.cc
+++ b/http2/hpack/tools/hpack_block_builder.cc
@@ -18,20 +18,7 @@
EXPECT_LE(prefix_length, 8);
HpackVarintEncoder varint_encoder;
-
- unsigned char c =
- varint_encoder.StartEncoding(high_bits, prefix_length, varint);
- buffer_.push_back(c);
-
- if (!varint_encoder.IsEncodingInProgress()) {
- return;
- }
-
- // After the prefix, at most 63 bits can remain to be encoded.
- // Each octet holds 7 bits, so at most 9 octets are necessary.
- // TODO(bnc): Move this into a constant in HpackVarintEncoder.
- varint_encoder.ResumeEncoding(/* max_encoded_bytes = */ 10, &buffer_);
- DCHECK(!varint_encoder.IsEncodingInProgress());
+ varint_encoder.Encode(high_bits, prefix_length, varint, &buffer_);
}
void HpackBlockBuilder::AppendEntryTypeAndVarint(HpackEntryType entry_type,
diff --git a/http2/hpack/varint/hpack_varint_encoder.cc b/http2/hpack/varint/hpack_varint_encoder.cc
index 2e7ad3e..f69c734 100644
--- a/http2/hpack/varint/hpack_varint_encoder.cc
+++ b/http2/hpack/varint/hpack_varint_encoder.cc
@@ -4,6 +4,8 @@
#include "net/third_party/quiche/src/http2/hpack/varint/hpack_varint_encoder.h"
+#include <limits>
+
#include "net/third_party/quiche/src/http2/platform/api/http2_logging.h"
namespace http2 {
@@ -11,6 +13,20 @@
HpackVarintEncoder::HpackVarintEncoder()
: varint_(0), encoding_in_progress_(false) {}
+void HpackVarintEncoder::Encode(uint8_t high_bits,
+ uint8_t prefix_length,
+ uint64_t varint,
+ Http2String* output) {
+ unsigned char first_byte = StartEncoding(high_bits, prefix_length, varint);
+ output->push_back(first_byte);
+ if (!IsEncodingInProgress()) {
+ return;
+ }
+
+ ResumeEncoding(std::numeric_limits<size_t>::max(), output);
+ DCHECK(!IsEncodingInProgress());
+}
+
unsigned char HpackVarintEncoder::StartEncoding(uint8_t high_bits,
uint8_t prefix_length,
uint64_t varint) {
diff --git a/http2/hpack/varint/hpack_varint_encoder.h b/http2/hpack/varint/hpack_varint_encoder.h
index 68f7474..a5a9cd8 100644
--- a/http2/hpack/varint/hpack_varint_encoder.h
+++ b/http2/hpack/varint/hpack_varint_encoder.h
@@ -5,6 +5,7 @@
#ifndef QUICHE_HTTP2_HPACK_VARINT_HPACK_VARINT_ENCODER_H_
#define QUICHE_HTTP2_HPACK_VARINT_HPACK_VARINT_ENCODER_H_
+#include <cstddef>
#include <cstdint>
#include "net/third_party/quiche/src/http2/platform/api/http2_export.h"
@@ -19,6 +20,16 @@
public:
HpackVarintEncoder();
+ // Encode |varint|, appending encoded data to |*output|. Appends between 1
+ // and 11 bytes in total. Must not be called when another encoding task
+ // previously started by StartEncoding() is still in progress. No encoding
+ // will be in progress after this method finishes.
+ void Encode(uint8_t high_bits,
+ uint8_t prefix_length,
+ uint64_t varint,
+ Http2String* output);
+
+ private:
// Start encoding an integer. Return the first encoded byte (composed of
// optional high bits and 1 to 8 bit long prefix). It is possible that this
// completes the encoding. Must not be called when previously started
@@ -36,7 +47,6 @@
// Returns true if encoding an integer has started and is not completed yet.
bool IsEncodingInProgress() const;
- private:
// The original integer shifted to the right by the number of bits already
// encoded. The lower bits shifted away have already been encoded, and
// |varint_| has the higher bits that remain to be encoded.
diff --git a/http2/hpack/varint/hpack_varint_encoder_test.cc b/http2/hpack/varint/hpack_varint_encoder_test.cc
index a6043a0..96ade85 100644
--- a/http2/hpack/varint/hpack_varint_encoder_test.cc
+++ b/http2/hpack/varint/hpack_varint_encoder_test.cc
@@ -13,12 +13,6 @@
namespace test {
namespace {
-// Freshly constructed encoder is not in the process of encoding.
-TEST(HpackVarintEncoderTest, Done) {
- HpackVarintEncoder varint_encoder;
- EXPECT_FALSE(varint_encoder.IsEncodingInProgress());
-}
-
struct {
uint8_t high_bits;
uint8_t prefix_length;
@@ -39,11 +33,12 @@
HpackVarintEncoder varint_encoder;
for (size_t i = 0; i < HTTP2_ARRAYSIZE(kShortTestData); ++i) {
- EXPECT_EQ(kShortTestData[i].expected_encoding,
- varint_encoder.StartEncoding(kShortTestData[i].high_bits,
- kShortTestData[i].prefix_length,
- kShortTestData[i].value));
- EXPECT_FALSE(varint_encoder.IsEncodingInProgress());
+ Http2String output;
+ varint_encoder.Encode(kShortTestData[i].high_bits,
+ kShortTestData[i].prefix_length,
+ kShortTestData[i].value, &output);
+ ASSERT_EQ(1u, output.size());
+ EXPECT_EQ(kShortTestData[i].expected_encoding, output[0]);
}
}
@@ -111,34 +106,17 @@
// Test encoding byte by byte, also test encoding in
// a single ResumeEncoding() call.
- for (bool byte_by_byte : {true, false}) {
for (size_t i = 0; i < HTTP2_ARRAYSIZE(kLongTestData); ++i) {
Http2String expected_encoding =
Http2HexDecode(kLongTestData[i].expected_encoding);
- ASSERT_FALSE(expected_encoding.empty());
-
- EXPECT_EQ(static_cast<unsigned char>(expected_encoding[0]),
- varint_encoder.StartEncoding(kLongTestData[i].high_bits,
- kLongTestData[i].prefix_length,
- kLongTestData[i].value));
- EXPECT_TRUE(varint_encoder.IsEncodingInProgress());
Http2String output;
- if (byte_by_byte) {
- while (varint_encoder.IsEncodingInProgress()) {
- EXPECT_EQ(1u, varint_encoder.ResumeEncoding(1, &output));
- }
- } else {
- // TODO(bnc): Factor out maximum number of extension bytes into a
- // constant in HpackVarintEncoder.
- EXPECT_EQ(expected_encoding.size() - 1,
- varint_encoder.ResumeEncoding(10, &output));
- EXPECT_FALSE(varint_encoder.IsEncodingInProgress());
- }
- EXPECT_EQ(expected_encoding.size() - 1, output.size());
- EXPECT_EQ(expected_encoding.substr(1), output);
+ varint_encoder.Encode(kLongTestData[i].high_bits,
+ kLongTestData[i].prefix_length,
+ kLongTestData[i].value, &output);
+
+ EXPECT_EQ(expected_encoding, output);
}
- }
}
struct {
@@ -158,21 +136,33 @@
HpackVarintEncoder varint_encoder;
for (size_t i = 0; i < HTTP2_ARRAYSIZE(kLastByteIsZeroTestData); ++i) {
- EXPECT_EQ(
- kLastByteIsZeroTestData[i].expected_encoding_first_byte,
- varint_encoder.StartEncoding(kLastByteIsZeroTestData[i].high_bits,
- kLastByteIsZeroTestData[i].prefix_length,
- kLastByteIsZeroTestData[i].value));
- EXPECT_TRUE(varint_encoder.IsEncodingInProgress());
-
Http2String output;
- EXPECT_EQ(1u, varint_encoder.ResumeEncoding(1, &output));
- ASSERT_EQ(1u, output.size());
- EXPECT_EQ(0b00000000, output[0]);
- EXPECT_FALSE(varint_encoder.IsEncodingInProgress());
+ varint_encoder.Encode(kLastByteIsZeroTestData[i].high_bits,
+ kLastByteIsZeroTestData[i].prefix_length,
+ kLastByteIsZeroTestData[i].value, &output);
+ ASSERT_EQ(2u, output.size());
+ EXPECT_EQ(kLastByteIsZeroTestData[i].expected_encoding_first_byte,
+ output[0]);
+ EXPECT_EQ(0b00000000, output[1]);
}
}
+// Test that encoder appends correctly to non-empty string.
+TEST(HpackVarintEncoderTest, Append) {
+ HpackVarintEncoder varint_encoder;
+ Http2String output("foo");
+ EXPECT_EQ(Http2HexDecode("666f6f"), output);
+
+ varint_encoder.Encode(0b10011000, 3, 103, &output);
+ EXPECT_EQ(Http2HexDecode("666f6f9f60"), output);
+
+ varint_encoder.Encode(0b10100000, 5, 8, &output);
+ EXPECT_EQ(Http2HexDecode("666f6f9f60a8"), output);
+
+ varint_encoder.Encode(0b10011000, 3, 202147110, &output);
+ EXPECT_EQ(Http2HexDecode("666f6f9f60a89f9f8ab260"), output);
+}
+
} // namespace
} // namespace test
} // namespace http2
diff --git a/quic/core/qpack/qpack_constants.h b/quic/core/qpack/qpack_constants.h
index 2812e63..35e4d56 100644
--- a/quic/core/qpack/qpack_constants.h
+++ b/quic/core/qpack/qpack_constants.h
@@ -77,11 +77,6 @@
// Every possible input must match exactly one instruction.
using QpackLanguage = std::vector<const QpackInstruction*>;
-// TODO(bnc): Move this into HpackVarintEncoder.
-// The integer encoder can encode up to 2^64-1, which can take up to 10 bytes
-// (each carrying 7 bits) after the prefix.
-const uint8_t kMaxExtensionBytesForVarintEncoding = 10;
-
// Wire format defined in
// https://quicwg.org/base-drafts/draft-ietf-quic-qpack.html#rfc.section.5
diff --git a/quic/core/qpack/qpack_instruction_encoder.cc b/quic/core/qpack/qpack_instruction_encoder.cc
index 400a828..b2acdbd 100644
--- a/quic/core/qpack/qpack_instruction_encoder.cc
+++ b/quic/core/qpack/qpack_instruction_encoder.cc
@@ -42,11 +42,8 @@
case State::kSbit:
DoStaticBit();
break;
- case State::kVarintStart:
- DoVarintStart(output);
- break;
- case State::kVarintResume:
- DoVarintResume(output);
+ case State::kVarintEncode:
+ DoVarintEncode(output);
break;
case State::kStartString:
DoStartString();
@@ -73,7 +70,7 @@
return;
case QpackInstructionFieldType::kVarint:
case QpackInstructionFieldType::kVarint2:
- state_ = State::kVarintStart;
+ state_ = State::kVarintEncode;
return;
case QpackInstructionFieldType::kName:
case QpackInstructionFieldType::kValue:
@@ -95,13 +92,11 @@
state_ = State::kStartField;
}
-void QpackInstructionEncoder::DoVarintStart(std::string* output) {
+void QpackInstructionEncoder::DoVarintEncode(std::string* output) {
DCHECK(field_->type == QpackInstructionFieldType::kVarint ||
field_->type == QpackInstructionFieldType::kVarint2 ||
field_->type == QpackInstructionFieldType::kName ||
field_->type == QpackInstructionFieldType::kValue);
- DCHECK(!varint_encoder_.IsEncodingInProgress());
-
uint64_t integer_to_encode;
switch (field_->type) {
case QpackInstructionFieldType::kVarint:
@@ -115,35 +110,9 @@
break;
}
- output->push_back(
- varint_encoder_.StartEncoding(byte_, field_->param, integer_to_encode));
+ varint_encoder_.Encode(byte_, field_->param, integer_to_encode, output);
byte_ = 0;
- if (varint_encoder_.IsEncodingInProgress()) {
- state_ = State::kVarintResume;
- return;
- }
-
- if (field_->type == QpackInstructionFieldType::kVarint ||
- field_->type == QpackInstructionFieldType::kVarint2) {
- ++field_;
- state_ = State::kStartField;
- return;
- }
-
- state_ = State::kWriteString;
-}
-
-void QpackInstructionEncoder::DoVarintResume(std::string* output) {
- DCHECK(field_->type == QpackInstructionFieldType::kVarint ||
- field_->type == QpackInstructionFieldType::kVarint2 ||
- field_->type == QpackInstructionFieldType::kName ||
- field_->type == QpackInstructionFieldType::kValue);
- DCHECK(varint_encoder_.IsEncodingInProgress());
-
- varint_encoder_.ResumeEncoding(std::numeric_limits<size_t>::max(), output);
- DCHECK(!varint_encoder_.IsEncodingInProgress());
-
if (field_->type == QpackInstructionFieldType::kVarint ||
field_->type == QpackInstructionFieldType::kVarint2) {
++field_;
@@ -169,7 +138,7 @@
string_to_write_ = huffman_encoded_string_;
}
- state_ = State::kVarintStart;
+ state_ = State::kVarintEncode;
}
void QpackInstructionEncoder::DoWriteString(std::string* output) {
diff --git a/quic/core/qpack/qpack_instruction_encoder.h b/quic/core/qpack/qpack_instruction_encoder.h
index ae37b9e..a831a07 100644
--- a/quic/core/qpack/qpack_instruction_encoder.h
+++ b/quic/core/qpack/qpack_instruction_encoder.h
@@ -43,11 +43,9 @@
kStartField,
// Write static bit to |byte_|.
kSbit,
- // Start encoding an integer (|varint_| or |varint2_| or string length) with
- // a prefix, using |byte_| for the high bits.
- kVarintStart,
- // Resume encoding an integer.
- kVarintResume,
+ // Encode an integer (|varint_| or |varint2_| or string length) with a
+ // prefix, using |byte_| for the high bits.
+ kVarintEncode,
// Determine if Huffman encoding should be used for |name_| or |value_|, set
// up |name_| or |value_| and |huffman_encoded_string_| accordingly, and
// write the Huffman bit to |byte_|.
@@ -61,8 +59,7 @@
void DoOpcode();
void DoStartField();
void DoStaticBit();
- void DoVarintStart(std::string* output);
- void DoVarintResume(std::string* output);
+ void DoVarintEncode(std::string* output);
void DoStartString();
void DoWriteString(std::string* output);
