-
Notifications
You must be signed in to change notification settings - Fork 182
Header tuning don't over engineer #1453
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
Conversation
aricart
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| public static final byte[] NOT_FOUND_CODE_BYTES = ("" + NOT_FOUND_CODE).getBytes(); | ||
| public static final byte[] BAD_JS_REQUEST_CODE_BYTES = ("" + BAD_JS_REQUEST_CODE).getBytes(); | ||
| public static final byte[] CONFLICT_CODE_BYTES = ("" + CONFLICT_CODE).getBytes(); | ||
| public static final byte[] EOB_CODE_BYTES = ("" + EOB_CODE).getBytes(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need these - there were not released in any version, just a PR in this cycle
| return valueAsString(); | ||
| } | ||
|
|
||
| public Integer getIntValue() throws NumberFormatException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed, this was just over engineering
| else if (b == 'F') { | ||
| if (valueEquals(FLOW_CONTROL_TEXT_BYTES)) { | ||
| return FLOW_CONTROL_TEXT; | ||
| if (valueLength > 11) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since they are known text, this whole block is a little smarter instead of doing a bunch of byte array compares.
No description provided.