Subject: Re: [quicwg/base-drafts] QPACK [editorial] Update text on instructions and representations. (#2941)

From: afrind <[email-masked]>
Date: Tue, 13 Aug 2019 14:58:43 -0700

afrind commented on this pull request.

> @@ -821,10 +828,13 @@ connection error of type `HTTP_QPACK_DECODER_STREAM_ERROR`.
 
 ## Header Block Representations
 
-Header blocks contain compressed representations of header lists and are carried
-in frames on streams defined by the enclosing protocol.  These representations
-reference the static table, or dynamic table in a particular state without
-modifying it.
+A header block consists of a prefix and a possibly empty sequence of
+representations defined in this section.  Each representation corresponds to a
+single header field.  These representations can reference the static table, or
+the dynamic table in a particular state without modifying it.

Wondering if we still need 'without modifying it'.  True HPACK instructions both emitted header fields and modified the state, but it might be redundant in the context of this document, and it makes it read awkwardly.

> @@ -968,8 +978,8 @@ decoded header list, as described in Section 3.2 of [RFC7541].
 If the entry is in the static table, or in the dynamic table with an absolute
 index less than the Base, this representation starts with the '1' 1-bit pattern,
 followed by the `S` bit indicating whether the reference is into the static or
-dynamic table.  The 6-bit prefix integer (see Section 5.1 of [RFC7541]) that
-follows is used to locate the table entry for the header name.  When S=1, the
+dynamic table.  The 6-bit prefix integer (see {{prefixed-integers}}) that
+follows is used to locate the table entry for the header field.  When S=1, the

nice catch!

>  The following bit, 'N', indicates whether an intermediary is permitted to add
 this header to the dynamic header table on subsequent hops. When the 'N' bit is
-set, the encoded header MUST always be encoded with a literal representation. In
-particular, when a peer sends a header field that it received represented as a

It was copied verbatim from HPACK.  Maybe we should leave it in.

>  used to locate the table entry for the header name.  When S=1, the number
 represents the static table index; when S=0, the number is the relative index of
-the entry in the dynamic table.
+the entry in the dynamic table.  Only the header field name stored in the static
+or dynamic table is used. Any header field value MUST be ignored.

I feel like 'Any header field value MUST be ignored' might be confusing since it doesn't say the header field value from the static or dynamic table -- this representation also contains a value.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2941#pullrequestreview-274577499

---

From: Mike Bishop <[email-masked]>
Date: Wed, 14 Aug 2019 11:57:51 -0700

MikeBishop approved this pull request.

Mostly good improvements; a few things to fix up further.

 

The difference from HPACK seems worth noting, but perhaps the sentence could be reworded to flow better.  How about something like "These representations reference the static table and the dynamic table in a particular state, but do not modify the state of the dynamic table."

I see what you mean about it being a restatement, but it's a ramification of "MUST always" that isn't immediately obvious.  If it's not stated, people will mess this up, I guarantee.  (They do anyway, because not all HPACK libraries tell you whether this bit was set on the way in.)

It's not my favorite feature of HPACK, but let's either keep the clarification or remove the requirement entirely.  If you want to discuss the latter, I would suggest doing so in a separate issue and PR.

Or perhaps make an affirmative statement instead: "Only the header field name is taken from the static or dynamic table entry; the header field value is encoded as a string literal following the index."  There's currently no text that I see referring to the value portion of the instruction.

 ### Literal Header Field With Post-Base Name Reference
 
 If the name entry is in the dynamic table with an absolute index greater than or
-equal to the Base, the representation starts with the '0000' four-bit
-pattern. The fifth bit is the 'N' bit as described in
-{{literal-name-reference}}.  Finally, the header field name is represented using
-the post-base index of that entry (see {{post-base}}) encoded as an integer with
-a 3-bit prefix.
+equal to the Base, the representation starts with the '0000' four-bit pattern.
+The fifth bit is the 'N' bit as described in {{literal-name-reference}}.
+Finally, the header field name is represented using the post-base index of that
+entry (see {{post-base}}) encoded as an integer with a 3-bit prefix (see
+{{prefixed-integers}}).

Likewise, no mention here of the string literal for the value.  (Pre-existing problem, not one you introduced.)

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2941#pullrequestreview-275081269

---

From: Bence Béky <[email-masked]>
Date: Thu, 15 Aug 2019 10:02:41 -0700

@bencebeky pushed 1 commit.

8ead640f4f0f378defdc93f5bc420d39f5a565f3  Incorporate first round of feedback by MikeBishop and afrind.

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2941/files/96df67439cf7ee1ad993f7a59d44c73f04992fd5..8ead640f4f0f378defdc93f5bc420d39f5a565f3

---

From: Bence Béky <[email-masked]>
Date: Thu, 15 Aug 2019 10:02:58 -0700

bencebeky commented on this pull request.

Thank you both for the great feedback!

 

How about "These representations reference the static table or the dynamic table in a particular state, but do not modify that state."?

I'd rather not find out whether people would actually mess this up.  Let me revert the removal of this sentence.

Thank you for the suggestion, that affirmative statement really flows better.

 

Good catch.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2941#pullrequestreview-275541222