Date: Thu, 09 Apr 2020 06:58:28 -0700
From: ianswett <notifications@github.com>
Reply-To: quicwg/base-drafts
 <reply+AFTOJKYZAIBCYN245SKENGN4TMFQJEVBNHHCHFHWFE@reply.github.com>
To: quicwg/base-drafts <base-drafts@noreply.github.com>
Cc: Subscribed <subscribed@noreply.github.com>
Message-ID: <quicwg/base-drafts/pull/3573/review/390813431@github.com>
In-Reply-To: <quicwg/base-drafts/pull/3573@github.com>
References: <quicwg/base-drafts/pull/3573@github.com>
Subject: Re: [quicwg/base-drafts] Alternative packet format presentation
 (#3573)
Mime-Version: 1.0
Content-Type: multipart/alternative;
 boundary="--==_mimepart_5e8f2a0462f6f_1ff33f908f2cd96812555f";
 charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: list
Archived-At: <https://mailarchive.ietf.org/arch/msg/quic-issues/-4O8P7K1mScWc53d6v7oyJm0axY>


----==_mimepart_5e8f2a0462f6f_1ff33f908f2cd96812555f
Content-Type: text/plain;
 charset=UTF-8
Content-Transfer-Encoding: 7bit

@ianswett approved this pull request.

I'd prefer to differentiate Type from Stream Type in H3, but otherwise LGTM.

> @@ -1035,11 +1035,9 @@ integer at the start of the stream. The format and structure of data that
 follows this integer is determined by the stream type.
 
 ~~~~~~~~~~ drawing
- 0                   1                   2                   3
- 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-|                        Stream Type (i)                      ...
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+Unidirectional Stream Header {
+  Type (i),

```suggestion
  Stream Type (i),
```
To avoid changing the name or be confused with the 'Type' used in all the frames.

> @@ -1124,13 +1122,10 @@ Only servers can push; if a server receives a client-initiated push stream, this
 MUST be treated as a connection error of type H3_STREAM_CREATION_ERROR.
 
 ~~~~~~~~~~ drawing
- 0                   1                   2                   3
- 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-|                           0x01 (i)                          ...
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-|                          Push ID (i)                        ...
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+Push Stream Header {
+  Type (i) = 0x01,

```suggestion
  Stream Type (i) = 0x01,
```

-- 
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/3573#pullrequestreview-390813431
----==_mimepart_5e8f2a0462f6f_1ff33f908f2cd96812555f
Content-Type: text/html;
 charset=UTF-8
Content-Transfer-Encoding: quoted-printable

<p></p>=0D
<p><b>@ianswett</b> approved this pull request.</p>=0D
=0D
<p>I'd prefer to differentiate Type from Stream Type in H3, but otherwise=
 LGTM.</p><hr>=0D
=0D
<p>In <a href=3D"https://github.com/quicwg/base-drafts/pull/3573#discussi=
on_r406221086">draft-ietf-quic-http.md</a>:</p>=0D
<pre style=3D'color:#555'>&gt; @@ -1035,11 +1035,9 @@ integer at the star=
t of the stream. The format and structure of data that=0D
 follows this integer is determined by the stream type.=0D
 =0D
 ~~~~~~~~~~ drawing=0D
- 0                   1                   2                   3=0D
- 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1=0D
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+=0D
-|                        Stream Type (i)                      ...=0D
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+=0D
+Unidirectional Stream Header {=0D
+  Type (i),=0D
</pre>=0D
=E2=AC=87=EF=B8=8F Suggested change=0D
<pre style=3D"color: #555">-  Type (i),=0D
+  Stream Type (i),=0D
</pre>=0D
=0D
<p>To avoid changing the name or be confused with the 'Type' used in all =
the frames.</p>=0D
=0D
<hr>=0D
=0D
<p>In <a href=3D"https://github.com/quicwg/base-drafts/pull/3573#discussi=
on_r406221691">draft-ietf-quic-http.md</a>:</p>=0D
<pre style=3D'color:#555'>&gt; @@ -1124,13 +1122,10 @@ Only servers can p=
ush; if a server receives a client-initiated push stream, this=0D
 MUST be treated as a connection error of type H3_STREAM_CREATION_ERROR.=0D=

 =0D
 ~~~~~~~~~~ drawing=0D
- 0                   1                   2                   3=0D
- 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1=0D
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+=0D
-|                           0x01 (i)                          ...=0D
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+=0D
-|                          Push ID (i)                        ...=0D
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+=0D
+Push Stream Header {=0D
+  Type (i) =3D 0x01,=0D
</pre>=0D
=E2=AC=87=EF=B8=8F Suggested change=0D
<pre style=3D"color: #555">-  Type (i) =3D 0x01,=0D
+  Stream Type (i) =3D 0x01,=0D
</pre>=0D
=0D
=0D
<p style=3D"font-size:small;-webkit-text-size-adjust:none;color:#666;">&m=
dash;<br />You are receiving this because you are subscribed to this thre=
ad.<br />Reply to this email directly, <a href=3D"https://github.com/quic=
wg/base-drafts/pull/3573#pullrequestreview-390813431">view it on GitHub</=
a>, or <a href=3D"https://github.com/notifications/unsubscribe-auth/AFTOJ=
KZ64PISBXVV3JLHST3RLXIAJANCNFSM4MELX4HA">unsubscribe</a>.<img src=3D"http=
s://github.com/notifications/beacon/AFTOJK5IV2VN4RLZMX7WRKTRLXIAJA5CNFSM4=
MELX4HKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZ=
GOC5FVN5Y.gif" height=3D"1" width=3D"1" alt=3D"" /></p>=0D
<script type=3D"application/ld+json">[=0D
{=0D
"@context": "http://schema.org",=0D
"@type": "EmailMessage",=0D
"potentialAction": {=0D
"@type": "ViewAction",=0D
"target": "https://github.com/quicwg/base-drafts/pull/3573#pullrequestrev=
iew-390813431",=0D
"url": "https://github.com/quicwg/base-drafts/pull/3573#pullrequestreview=
-390813431",=0D
"name": "View Pull Request"=0D
},=0D
"description": "View this Pull Request on GitHub",=0D
"publisher": {=0D
"@type": "Organization",=0D
"name": "GitHub",=0D
"url": "https://github.com"=0D
}=0D
}=0D
]</script>=

----==_mimepart_5e8f2a0462f6f_1ff33f908f2cd96812555f--