Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce the number of offset lengths in a stream frame #430

Merged
merged 2 commits into from
Apr 14, 2017

Conversation

ianswett
Copy link
Contributor

@ianswett ianswett commented Apr 2, 2017

Also fixes #414

Copy link
Contributor

@MikeBishop MikeBishop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, modulo nits.

These bits are parsed as follows:

* The leftmost bit must be set to 1, indicating that this is a STREAM frame.
* The first two bit must be set to 11, indicating that this is a STREAM frame.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bits, not bit


* The first two bits must be set to 01 indicating that this is an ACK frame.
* The first two bits must be set to 101 indicating that this is an ACK frame.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

three bits

@ianswett ianswett merged commit fac7f9d into master Apr 14, 2017
@martinthomson martinthomson deleted the ianswett-stream-offsets branch April 21, 2017 05:27
@martinthomson martinthomson added design An issue that affects the design of the protocol; resolution requires consensus. -transport labels May 22, 2017
@martinthomson
Copy link
Member

I was a little surprised to see this change was merged.

@ianswett, this change to the transport doc wasn't reviewed by either @janaiyengar or myself; nor do I see any discussion on the list. In this case, I think that it's a big improvement, so I've no objection, but I don't think we should be merging changes to documents without the approval of at least one editor.

(And sorry for not reviewing this, I don't know how I missed it.)

@ianswett
Copy link
Contributor Author

I remember Jana and I discussing it, but next time I'll make sure one of you approves it on Github.

@martinthomson
Copy link
Member

:) If Jana said it's OK, that's fine. I also think that it's OK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-transport design An issue that affects the design of the protocol; resolution requires consensus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stream frame can have an offset length equal 0
3 participants