Re: [quicwg/base-drafts] QPACK [editorial] Misc minor editorial changes. (#2942)

afrind <notifications@github.com> Tue, 15 October 2019 23:40 UTC

Return-Path: <noreply@github.com>
X-Original-To: quic-issues@ietfa.amsl.com
Delivered-To: quic-issues@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 0532D12081C for <quic-issues@ietfa.amsl.com>; Tue, 15 Oct 2019 16:40:32 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -8
X-Spam-Level:
X-Spam-Status: No, score=-8 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.001, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, HTML_MESSAGE=0.001, MAILING_LIST_MULTI=-1, RCVD_IN_DNSWL_HI=-5, SPF_HELO_NONE=0.001, SPF_PASS=-0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (1024-bit key) header.d=github.com
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id NT9f9yla6nmR for <quic-issues@ietfa.amsl.com>; Tue, 15 Oct 2019 16:40:29 -0700 (PDT)
Received: from out-19.smtp.github.com (out-19.smtp.github.com [192.30.252.202]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 14298120019 for <quic-issues@ietf.org>; Tue, 15 Oct 2019 16:40:29 -0700 (PDT)
Received: from github-lowworker-fb56993.ac4-iad.github.net (github-lowworker-fb56993.ac4-iad.github.net [10.52.19.31]) by smtp.github.com (Postfix) with ESMTP id 62CE252110D for <quic-issues@ietf.org>; Tue, 15 Oct 2019 16:40:28 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=github.com; s=pf2014; t=1571182828; bh=0hfp3iVZwGQbsXbUBsFyZGQ4/KtM35fgN/zbrgBx7BM=; h=Date:From:Reply-To:To:Cc:In-Reply-To:References:Subject:List-ID: List-Archive:List-Post:List-Unsubscribe:From; b=q29ebLSJNPC4Jb3QBJhxOG0F0+0T1hUQ/vQA3dMpdkEcpMtVncMoJwKA/i0EVPnia H+QgXr0gBkBDLgvVE6zi52Wto0q37K0VXu6E4oA5r7Xg7vNZ5B8+h+rQDnn5DBfaXS CSGiUpy5qKc98i3toyWjaXeu7L9SrqHxk3M3b1Q0=
Date: Tue, 15 Oct 2019 16:40:28 -0700
From: afrind <notifications@github.com>
Reply-To: quicwg/base-drafts <reply+AFTOJKZFJGGQB2B5RFQJMD53WOMXZEVBNHHBYU6MYI@reply.github.com>
To: quicwg/base-drafts <base-drafts@noreply.github.com>
Cc: Subscribed <subscribed@noreply.github.com>
Message-ID: <quicwg/base-drafts/pull/2942/review/302247684@github.com>
In-Reply-To: <quicwg/base-drafts/pull/2942@github.com>
References: <quicwg/base-drafts/pull/2942@github.com>
Subject: Re: [quicwg/base-drafts] QPACK [editorial] Misc minor editorial changes. (#2942)
Mime-Version: 1.0
Content-Type: multipart/alternative; boundary="--==_mimepart_5da658ec53a61_76c33f7f106cd968608b3"; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: list
X-GitHub-Sender: afrind
X-GitHub-Recipient: quic-issues
X-GitHub-Reason: subscribed
X-Auto-Response-Suppress: All
X-GitHub-Recipient-Address: quic-issues@ietf.org
Archived-At: <https://mailarchive.ietf.org/arch/msg/quic-issues/ramZydAJ15Ekah7lGSE1yYKM_EU>
X-BeenThere: quic-issues@ietf.org
X-Mailman-Version: 2.1.29
List-Id: Notification list for GitHub issues related to the QUIC WG <quic-issues.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/quic-issues>, <mailto:quic-issues-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/quic-issues/>
List-Post: <mailto:quic-issues@ietf.org>
List-Help: <mailto:quic-issues-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/quic-issues>, <mailto:quic-issues-request@ietf.org?subject=subscribe>
X-List-Received-Date: Tue, 15 Oct 2019 23:40:33 -0000

afrind commented on this pull request.



> @@ -445,19 +443,23 @@ encoder sends a Set Dynamic Table Capacity instruction
 ({{set-dynamic-capacity}}) with a non-zero capacity to begin using the dynamic
 table.
 
-Before a new entry is added to the dynamic table, entries are evicted from the
-end of the dynamic table until the size of the dynamic table is less than or
-equal to (table capacity - size of new entry). The encoder MUST NOT evict a
-blocking dynamic table entry (see {{blocked-insertion}}).  The entry is then
-added to the table.  It is an error if the encoder attempts to add an entry that
-is larger than the dynamic table capacity; the decoder MUST treat this as a
+When a new entry is added to the dynamic table, the minimum number of entries
+are evicted from the end of the dynamic table so that the final table size
+including the new entry does not exceed the table capacity.  This can be

This is an awfully long sentence without punctuation.  Maybe set off 'including the new entry' with commas?

> +including the new entry does not exceed the table capacity.  This can be
+achieved by adding the new entry and then removing entries from the end until
+the table size is less than or equal to the table capacity.  Another option is
+to first evict entries until the size of the dynamic table is less than or equal
+to (table capacity - size of new entry), then add the new entry.  Note that a

Actually, this language comes directly from HPACK.  Why is it necessary to offer the add+remove option, instead of remove+add?  There isn't normative language here, so an implementation can do it either way and still be compliant.

> @@ -603,7 +607,8 @@ is used without modification.
 This document expands the definition of string literals and permits them to
 begin other than on a byte boundary.  An "N-bit prefix string literal" begins
 with the same Huffman flag, followed by the length encoded as an (N-1)-bit
-prefix integer.  The remainder of the string literal is unmodified.
+prefix integer.  The prefix size, N, can have a value between 2 and 8.  The

I didn't see a 2-bit prefix anywhere, should this be 3 and 8 (inclusive)?

But I see this as a bit of overkill.  If we allow 1 or 2 in QPACK2 we have to remember to change this.

>  
 A header block that does not reference the dynamic table can use any value for
-the Base; setting Delta Base to zero is the most efficient encoding.
+the Base; setting Delta Base to zero is one of the most efficient encodings.

It's true there are other 2 byte encodings, but I think we should encourage 0/0.  Maybe we should even require it?

> @@ -952,10 +954,8 @@ entry.
 
 ### Indexed Header Field
 
-An indexed header field representation identifies an entry in either the static
-table or the dynamic table and causes that header field to be added to the
-decoded header list, as described in Section 3.2 of [RFC7541].
-<!-- is the 7541 reference still helpful here -->
+An indexed header field instruction identifies an entry in either the static

Why did you change this from representation to instruction?  Though this isn't currently spelled out directly, the encoder/decoder streams contain "instructions" - which cause the receiver to take an action.  Header blocks contain representations, which emit headers fields.  The word instruction doesn't appear in this section.

The other representations don't seem to call themselves such, however.  And only this one contains the language about adding to the decoded header list.  I guess I prefer emit also, but maybe we need to make another consistency pass.

-- 
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/2942#pullrequestreview-302247684