Use lowercase for charsets #11741 #12347
Conversation
Fix #11741 as per the WhatTFWG recommendations, use lower case for charset names. Took the opportunity for some minor optimizations: + use the already made HttpField instance in MimeTypes.Type rather than create a new one in the HttpParser.CACHE + keep the MimeType.Type associated with the pre encoded Content-Type fields
Fix #11741 as per the WhatTFWG recommendations, use lower case for charset names. Took the opportunity for some minor optimizations: + use the already made HttpField instance in MimeTypes.Type rather than create a new one in the HttpParser.CACHE + keep the MimeType.Type associated with the pre encoded Content-Type fields
Fix #11741 as per the WhatTFWG recommendations, use lower case for charset names. Took the opportunity for some minor optimizations: + use the already made HttpField instance in MimeTypes.Type rather than create a new one in the HttpParser.CACHE + keep the MimeType.Type associated with the pre encoded Content-Type fields
Fix #11741 as per the WhatTFWG recommendations, use lower case for charset names. Took the opportunity for some minor optimizations: + use the already made HttpField instance in MimeTypes.Type rather than create a new one in the HttpParser.CACHE + keep the MimeType.Type associated with the pre encoded Content-Type fields
Fix #11741 as per the WhatTFWG recommendations, use lower case for charset names. Took the opportunity for some minor optimizations: + use the already made HttpField instance in MimeTypes.Type rather than create a new one in the HttpParser.CACHE + keep the MimeType.Type associated with the pre encoded Content-Type fields
Fix #11741 as per the WhatTFWG recommendations, use lower case for charset names. Took the opportunity for some minor optimizations: + use the already made HttpField instance in MimeTypes.Type rather than create a new one in the HttpParser.CACHE + keep the MimeType.Type associated with the pre encoded Content-Type fields
There was a problem hiding this comment.
Strange location for the ContentTypeField.
Also, what happens if we use (in jetty-core Handler) the following?
response.getHeaders().put(HttpHeader.CONTENT_TYPE, "text/html; charset=UTF-8");Does that produce a UTF-8? or a utf-8 on the response headers that the client sees?
| @@ -876,4 +919,25 @@ else if (' ' != b) | |||
| return value; | |||
| return builder.toString(); | |||
| } | |||
|
|
|||
| public static class ContentTypeField extends PreEncodedHttpField | |||
There was a problem hiding this comment.
Wouldn't this make more sense as a standalone class?
After all we have all of the other specialized Fields as standalone classes, why is this one special?
There was a problem hiding this comment.
The only other extension of PreEncodedHttpField we have is ResponseHttpFields.PersesitentPreEncodedHttpField. Of the HttpField extensions, about half are sub-types.
So this is not unusual. It is a PreEncodedHttpField that is extended specifically to hold a MimeTypes.Type, so having it associated with MimeTypes seams reasonable.
There was a problem hiding this comment.
This ContentTypeField feels like the HostPortHttpField. (also a field used in HttpParser)
There was a problem hiding this comment.
I kind of agree that this would be better as standalone class, especially now that is the return value of a method: it's shorter to assign to a variable than MimeTypes.ContentTypeField.
There was a problem hiding this comment.
As this is a rather special purpose class, I've just made is package protected and provided static methods to extract the info if needed. I could make it fully private if we want to give up on the optimized lookup of content-types with spaces in HttpParser
Not that strange I think. I'll add some javadoc to explain the class and I think that will make the location seam reasonable
We will send headers as the app sets them. If they set them with a specific case, then we will send that case. But if they use APIs to setCharset etc. then we should generate as lowercase. |
Wont this cause a change in how the HttpClient parses responses too? |
| @@ -876,4 +919,25 @@ else if (' ' != b) | |||
| return value; | |||
| return builder.toString(); | |||
| } | |||
|
|
|||
| public static class ContentTypeField extends PreEncodedHttpField | |||
There was a problem hiding this comment.
I kind of agree that this would be better as standalone class, especially now that is the return value of a method: it's shorter to assign to a variable than MimeTypes.ContentTypeField.
| public boolean isCharsetAssumed() | ||
| { | ||
| return _assumedCharset; | ||
| } | ||
|
|
||
| public HttpField getContentTypeField() | ||
| public ContentTypeField getContentTypeField() |
There was a problem hiding this comment.
Should not the method below public HttpField getContentTypeField(Charset charset) also have its return type changed?
There was a problem hiding this comment.
Not really, because there are two aspect of ContentTypeField: it has a getMimeType method and it is a PreEncodedHttpField. For an arbitrary contentType returned from this method, it is not guaranteed that it will have a known mimeType and it is not known if it is worthwhile going to the effort of pre-encoding.
That is why I think this class belongs as a subtype of this class, as it is pretty special purpose for long held constants for known types.
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java
Outdated
Show resolved
Hide resolved
|
@sbordet I've made the class package protected. |
Fix #11741 as per the WhatTFWG recommendations, use lower case for charset names. Took the opportunity for some minor optimizations: