Issue #5171 Simplify GzipHandler user-agent handling #5196
Conversation
+ Remove User-Agent handling from GzipHandler + Allow Vary header to be set + Create rewrite MsieRule to remove Accept-Encoding from IE<=6 Signed-off-by: Greg Wilkins <[email protected]>
+ use for Vary field
+ improved comments Signed-off-by: Greg Wilkins <[email protected]>
| * passed. If the value needs to be merged with existing values, | ||
| * then a new field is created. | ||
| */ | ||
| public void ensure(HttpField field) |
There was a problem hiding this comment.
The semantic of this method is weird because not only ensures that a field exists, but also coalesces multiple fields into one - it should be clearly stated in the javadocs. Perhaps rename to ensureSingle()?
There was a problem hiding this comment.
I've gone with ensureField to match computeField
| computeField(field.getHeader(), (h, l) -> computeEnsure(field, field.getValues(), l)); | ||
| else | ||
| computeField(field.getName(), (h, l) -> computeEnsure(field, field.getValues(), l)); | ||
| } |
There was a problem hiding this comment.
I don't understand this logic.
The first if says "if the given field is single-valued"; but then computeEnsure() is passed the field, and there is no need to have two different computeEnsure(), the second with the values extracted from the field that is passed as first parameter 🤔
There was a problem hiding this comment.
The implementation of the single field and multiple field versions are a bit different. Single field can already exist or not. Multi valued might partially exist.
I could simplify a ensureField a little, but only at the expense of complicating the computeEnsure methods.
There was a problem hiding this comment.
Note that I now have 100% test coverage of ensureField and both computeEnsure.
| private static final int IEv6 = '6'; | ||
| private static final Trie<Boolean> __IE6_BadOS = new ArrayTernaryTrie<>(); | ||
| private static final HttpField CONNECTION_CLOSE = new HttpField(HttpHeader.CONNECTION, HttpHeaderValue.CLOSE); | ||
| public static final HttpField VARY_USER_AGENT = new PreEncodedHttpField(HttpHeader.VARY, HttpHeader.USER_AGENT.asString()); |
There was a problem hiding this comment.
Does it need to be public?
| } | ||
| return null; | ||
| } | ||
|
|
Issue #5171 Simplify GzipHandler user-agent handling:
Signed-off-by: Greg Wilkins [email protected]