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

[Mike Bishop <[e-mail]>]

MikeBishop approved this pull request.

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

> @@ -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.

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."

>  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

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.

>  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.

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