Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10407002:
Add special handling of the content type HTTP header (Closed)
|
Created:
8 years, 7 months ago by Søren Gjesse Modified:
8 years, 7 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionAdd special handling of the content type HTTP header
This adds parsing of HTTP headers of the form:
Name=value; parameter1=value1; parameter2=value2
Specialize the implemented header value class into a content type
class.
One could argue whether the "synchronization" between the string
representation and the content type object representation is the best
way, but if the exposed parameters map should just be the standard map
implementation there is no way of detection changes,
R=ager@google.com, ajohnsen@google.com
BUG=dart:2045
TEST=tests/standalone/io/http_test.dart, tests/standalone/io/http_headers_test.dart
Committed: https://code.google.com/p/dart/source/detail?r=7851
Patch Set 1 #
Total comments: 82
Patch Set 2 : Addressed review comments #
Total comments: 4
Patch Set 3 : Removed the content type caching to simplify. #
Messages
Total messages: 8 (0 generated)
LGTM! http://codereview.chromium.org/10407002/diff/1/runtime/bin/http.dart File runtime/bin/http.dart (right): http://codereview.chromium.org/10407002/diff/1/runtime/bin/http.dart#newcode306 runtime/bin/http.dart:306: ContentType contentType; IMO, this is a good way of exposing this. http://codereview.chromium.org/10407002/diff/1/runtime/bin/http.dart#newcode315 runtime/bin/http.dart:315: interface HeaderValue default _HeaderValue { The name HeaderValue confuses me. It sounds as it should be what you get from all HttpHeader's get methods, not something that is only associated to ContentType. Maybe rename to ContentTypeValue? http://codereview.chromium.org/10407002/diff/1/runtime/bin/http.dart#newcode329 runtime/bin/http.dart:329: String value; Have a toString that maps to the full content? http://codereview.chromium.org/10407002/diff/1/runtime/bin/http.dart#newcode341 runtime/bin/http.dart:341: interface ContentType extends HeaderValue default _ContentType { Here, I think we should have a toString as well. http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart File runtime/bin/http_impl.dart (right): http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:124: void get contentType() { Would it be better/simpler to not have the field _contentType, and just generate it every time contentType is called? http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:294: while (index < s.length) { !done() http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:302: while (index < s.length) { !done() http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:310: if (index == s.length) throw new HttpException("YYY"); Some real string here :) http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:310: if (index == s.length) throw new HttpException("YYY"); if(done()) ? http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:311: if (s[index] != expected) throw new HttpException("XXX $expected [${s[index]}]"); Ditto. http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:322: while (index < s.length) { !done() http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:334: while (index < s.length) { Maybe also !done() here, but that's 100% up to you - just though we might as well use it. http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:335: if (s[index] == "\\") { Are we actually handling this correctly? Should we escape some symbols? http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:336: if (index + 1 == s.length) throw new HttpException("ZZZ"); Real string. http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:347: // Parse non-quoted value. Change this to 'return parseValue()'? http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:388: _ContentType.fromString(String value) { : super.fromString(value); ? http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:397: if (index == -1 || s.length == index - 1) { Did you mean 'index == s.length - 1'? http://codereview.chromium.org/10407002/diff/1/runtime/bin/io.dart File runtime/bin/io.dart (right): http://codereview.chromium.org/10407002/diff/1/runtime/bin/io.dart#newcode16 runtime/bin/io.dart:16: #import("dart:builtin"); Did you mean to do this? http://codereview.chromium.org/10407002/diff/1/tests/standalone/io/http_test.... File tests/standalone/io/http_test.dart (right): http://codereview.chromium.org/10407002/diff/1/tests/standalone/io/http_test.... tests/standalone/io/http_test.dart:590: /*testStartStop(); We should still run every test ;)
https://chromiumcodereview.appspot.com/10407002/diff/1/runtime/bin/http.dart File runtime/bin/http.dart (right): https://chromiumcodereview.appspot.com/10407002/diff/1/runtime/bin/http.dart#... runtime/bin/http.dart:315: interface HeaderValue default _HeaderValue { Should we expose this interface? At this point only ContentType extends it and it has getters/setters for the value and parameter. It is not clear to me why this interface is useful to the user? https://chromiumcodereview.appspot.com/10407002/diff/1/runtime/bin/http.dart#... runtime/bin/http.dart:322: * Creates a new header value object from parsing a header value. What is the format of the input string? Does it contain the initial "name=" part of is it only the "value; parameter1=value1; ..." part? Maybe explicitly give an example input string? https://chromiumcodereview.appspot.com/10407002/diff/1/runtime/bin/http.dart#... runtime/bin/http.dart:351: * header value. As primary type, sub type and parameter names and Maybe give a concrete example of a Content-Type header value string? https://chromiumcodereview.appspot.com/10407002/diff/1/runtime/bin/http.dart#... runtime/bin/http.dart:373: * Gets and sets the character set. Can we specify the valid values in the interface? Or at least add a link to the http 1.1 docs on the character sets? Should we have a charset type? That might be the easiest way to communicate the possible charsets. https://chromiumcodereview.appspot.com/10407002/diff/1/runtime/bin/http_impl.... File runtime/bin/http_impl.dart (right): https://chromiumcodereview.appspot.com/10407002/diff/1/runtime/bin/http_impl.... runtime/bin/http_impl.dart:40: if (name == "content-type") contentType == null; This looks strange. If you are setting content-type you clear the contentType? Also, shouldn't this be _contentType (add test)? Maybe introduce a _clearContentTypeCache method to make it clearer what is going on? https://chromiumcodereview.appspot.com/10407002/diff/1/runtime/bin/http_impl.... runtime/bin/http_impl.dart:53: if (name == "content-type") contentType == null; _contentType? https://chromiumcodereview.appspot.com/10407002/diff/1/runtime/bin/http_impl.... runtime/bin/http_impl.dart:60: contentType == null; _contentType? https://chromiumcodereview.appspot.com/10407002/diff/1/runtime/bin/http_impl.... runtime/bin/http_impl.dart:231: _syncContentType() { Is there any reason to allow ContentType to be mutable? If it was immutable you do not need these sync methods. If you want a new one you create a new one (potentially based on an old one) and explicitly set it as the new one? https://chromiumcodereview.appspot.com/10407002/diff/1/runtime/bin/http_impl.... runtime/bin/http_impl.dart:310: if (index == s.length) throw new HttpException("YYY"); More useful error message? https://chromiumcodereview.appspot.com/10407002/diff/1/runtime/bin/http_impl.... runtime/bin/http_impl.dart:311: if (s[index] != expected) throw new HttpException("XXX $expected [${s[index]}]"); Ditto and a long line. https://chromiumcodereview.appspot.com/10407002/diff/1/runtime/bin/http_impl.... runtime/bin/http_impl.dart:335: if (s[index] == "\\") { Does the backslash have no special meaning? It doesn't escape anything? It is just ignored in the code. https://chromiumcodereview.appspot.com/10407002/diff/1/runtime/bin/http_impl.... runtime/bin/http_impl.dart:336: if (index + 1 == s.length) throw new HttpException("ZZZ"); Update message. https://chromiumcodereview.appspot.com/10407002/diff/1/runtime/bin/http_impl.... runtime/bin/http_impl.dart:398: primaryType = s; s.trim().toLowerCase()? https://chromiumcodereview.appspot.com/10407002/diff/1/runtime/bin/http_impl.... runtime/bin/http_impl.dart:404: Extra space https://chromiumcodereview.appspot.com/10407002/diff/1/runtime/bin/io.dart File runtime/bin/io.dart (right): https://chromiumcodereview.appspot.com/10407002/diff/1/runtime/bin/io.dart#ne... runtime/bin/io.dart:16: #import("dart:builtin"); Remove. :-) https://chromiumcodereview.appspot.com/10407002/diff/1/tests/standalone/io/ht... File tests/standalone/io/http_headers_test.dart (right): https://chromiumcodereview.appspot.com/10407002/diff/1/tests/standalone/io/ht... tests/standalone/io/http_headers_test.dart:243: contentType = new _ContentType.fromString(" text/html ; charset = utf-8 "); Long lines. https://chromiumcodereview.appspot.com/10407002/diff/1/tests/standalone/io/ht... File tests/standalone/io/http_test.dart (right): https://chromiumcodereview.appspot.com/10407002/diff/1/tests/standalone/io/ht... tests/standalone/io/http_test.dart:174: response.headers.set(HttpHeaders.CONTENT_TYPE, "text/html; charset = utf-8"); Long line. Also multiple long lines below. https://chromiumcodereview.appspot.com/10407002/diff/1/tests/standalone/io/ht... tests/standalone/io/http_test.dart:590: /*testStartStop(); Code in comments.
https://chromiumcodereview.appspot.com/10407002/diff/1/runtime/bin/http_impl.... File runtime/bin/http_impl.dart (right): https://chromiumcodereview.appspot.com/10407002/diff/1/runtime/bin/http_impl.... runtime/bin/http_impl.dart:40: if (name == "content-type") contentType == null; =, not ==. https://chromiumcodereview.appspot.com/10407002/diff/1/runtime/bin/http_impl.... runtime/bin/http_impl.dart:53: if (name == "content-type") contentType == null; =, not ==. https://chromiumcodereview.appspot.com/10407002/diff/1/runtime/bin/http_impl.... runtime/bin/http_impl.dart:60: contentType == null; =, not ==.
Thanks for all the comments - PTAL. http://codereview.chromium.org/10407002/diff/1/runtime/bin/http.dart File runtime/bin/http.dart (right): http://codereview.chromium.org/10407002/diff/1/runtime/bin/http.dart#newcode306 runtime/bin/http.dart:306: ContentType contentType; On 2012/05/21 07:37:30, ajohnsen wrote: > IMO, this is a good way of exposing this. Thanks! http://codereview.chromium.org/10407002/diff/1/runtime/bin/http.dart#newcode315 runtime/bin/http.dart:315: interface HeaderValue default _HeaderValue { On 2012/05/21 07:37:30, ajohnsen wrote: > The name HeaderValue confuses me. It sounds as it should be what you get from > all HttpHeader's get methods, not something that is only associated to > ContentType. Maybe rename to ContentTypeValue? ContentType extend HeaderValue. It should be possible to use an instance of HeaderValue to set any header. I will describe this more carefuly. http://codereview.chromium.org/10407002/diff/1/runtime/bin/http.dart#newcode315 runtime/bin/http.dart:315: interface HeaderValue default _HeaderValue { On 2012/05/21 07:40:39, Mads Ager wrote: > Should we expose this interface? At this point only ContentType extends it and > it has getters/setters for the value and parameter. It is not clear to me why > this interface is useful to the user? Currently this interface is extended by ContentType. However a lot of the HTTP headers are on this form, and HeaderValue is useful for handling headers for which no special treatment is provided. E.g. the Accept header can have a value like this: text/*;q=0.3, text/plain;q=0.7, text/html HttpClientRequest request; var v = new HeaderValue(); v.value = "text/*"; v.parameters["q"] = "0.3" request.headers.add(HttpHeaders.ACCEPT, v); v = new HeaderValue(); v.value = "text/plain"; v.parameters["q"] = "0.7" request.headers.add(HttpHeaders.ACCEPT, v); request.headers.add(HttpHeaders.ACCEPT, "text/html"); Likewise for parsing HttpRequest request; var l = request.headers[HttpHeaders.ACCEPT]; l.forEach((value) { HeaderValue v = new HeaderValue.fromString(value); ... v.value ... ... v.parameters ... }); I will update the the documentation to describe this. http://codereview.chromium.org/10407002/diff/1/runtime/bin/http.dart#newcode322 runtime/bin/http.dart:322: * Creates a new header value object from parsing a header value. On 2012/05/21 07:40:39, Mads Ager wrote: > What is the format of the input string? Does it contain the initial "name=" part > of is it only the "value; parameter1=value1; ..." part? > > Maybe explicitly give an example input string? Added example to interface description. http://codereview.chromium.org/10407002/diff/1/runtime/bin/http.dart#newcode329 runtime/bin/http.dart:329: String value; On 2012/05/21 07:37:30, ajohnsen wrote: > Have a toString that maps to the full content? toString was already there. Documentation added. http://codereview.chromium.org/10407002/diff/1/runtime/bin/http.dart#newcode341 runtime/bin/http.dart:341: interface ContentType extends HeaderValue default _ContentType { On 2012/05/21 07:37:30, ajohnsen wrote: > Here, I think we should have a toString as well. toString is now documented in the HeaderValue interface. http://codereview.chromium.org/10407002/diff/1/runtime/bin/http.dart#newcode351 runtime/bin/http.dart:351: * header value. As primary type, sub type and parameter names and On 2012/05/21 07:40:39, Mads Ager wrote: > Maybe give a concrete example of a Content-Type header value string? Done. http://codereview.chromium.org/10407002/diff/1/runtime/bin/http.dart#newcode373 runtime/bin/http.dart:373: * Gets and sets the character set. On 2012/05/21 07:40:39, Mads Ager wrote: > Can we specify the valid values in the interface? Or at least add a link to the > http 1.1 docs on the character sets? Should we have a charset type? That might > be the easiest way to communicate the possible charsets. This is not really possible. http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart File runtime/bin/http_impl.dart (right): http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:40: if (name == "content-type") contentType == null; On 2012/05/21 07:43:21, ajohnsen wrote: > =, not ==. Done. http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:40: if (name == "content-type") contentType == null; On 2012/05/21 07:40:39, Mads Ager wrote: > This looks strange. If you are setting content-type you clear the contentType? > Also, shouldn't this be _contentType (add test)? Maybe introduce a > _clearContentTypeCache method to make it clearer what is going on? Added _clearHeaderValueCache and added test. http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:53: if (name == "content-type") contentType == null; On 2012/05/21 07:40:39, Mads Ager wrote: > _contentType? Used _clearHeaderValueCache. http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:53: if (name == "content-type") contentType == null; On 2012/05/21 07:43:21, ajohnsen wrote: > =, not ==. Done. http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:60: contentType == null; On 2012/05/21 07:40:39, Mads Ager wrote: > _contentType? Used _clearHeaderValueCache. http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:60: contentType == null; On 2012/05/21 07:43:21, ajohnsen wrote: > =, not ==. Done. http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:124: void get contentType() { On 2012/05/21 07:37:30, ajohnsen wrote: > Would it be better/simpler to not have the field _contentType, and just generate > it every time contentType is called? It would, however for code patterns like this ... response.headers.contentType.value ... ... response.headers.contentType.parameters["charset"] ... we would then parse the same string twice for simple access. http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:231: _syncContentType() { On 2012/05/21 07:40:39, Mads Ager wrote: > Is there any reason to allow ContentType to be mutable? If it was immutable you > do not need these sync methods. If you want a new one you create a new one > (potentially based on an old one) and explicitly set it as the new one? As the HttpHeaders object is mutable it would be strange if the contentType property was not. http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:294: while (index < s.length) { On 2012/05/21 07:37:30, ajohnsen wrote: > !done() Done. http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:302: while (index < s.length) { On 2012/05/21 07:37:30, ajohnsen wrote: > !done() Done. http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:310: if (index == s.length) throw new HttpException("YYY"); On 2012/05/21 07:37:30, ajohnsen wrote: > Some real string here :) Done. http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:310: if (index == s.length) throw new HttpException("YYY"); On 2012/05/21 07:37:30, ajohnsen wrote: > if(done()) ? Done. http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:310: if (index == s.length) throw new HttpException("YYY"); On 2012/05/21 07:40:39, Mads Ager wrote: > More useful error message? Done. http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:311: if (s[index] != expected) throw new HttpException("XXX $expected [${s[index]}]"); On 2012/05/21 07:37:30, ajohnsen wrote: > Ditto. Done. http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:311: if (s[index] != expected) throw new HttpException("XXX $expected [${s[index]}]"); On 2012/05/21 07:40:39, Mads Ager wrote: > Ditto and a long line. Done and done. http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:322: while (index < s.length) { On 2012/05/21 07:37:30, ajohnsen wrote: > !done() Done. http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:334: while (index < s.length) { On 2012/05/21 07:37:30, ajohnsen wrote: > Maybe also !done() here, but that's 100% up to you - just though we might as > well use it. Done. http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:335: if (s[index] == "\\") { On 2012/05/21 07:37:30, ajohnsen wrote: > Are we actually handling this correctly? Should we escape some symbols? According to the spec the backslash escapes the following character no matter what it is which is what effectively happens here by just skipping it. http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:335: if (s[index] == "\\") { On 2012/05/21 07:40:39, Mads Ager wrote: > Does the backslash have no special meaning? It doesn't escape anything? It is > just ignored in the code. See other comment. http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:336: if (index + 1 == s.length) throw new HttpException("ZZZ"); On 2012/05/21 07:37:30, ajohnsen wrote: > Real string. Done. http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:336: if (index + 1 == s.length) throw new HttpException("ZZZ"); On 2012/05/21 07:40:39, Mads Ager wrote: > Update message. Done. http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:347: // Parse non-quoted value. On 2012/05/21 07:37:30, ajohnsen wrote: > Change this to 'return parseValue()'? Done. http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:388: _ContentType.fromString(String value) { On 2012/05/21 07:37:30, ajohnsen wrote: > : super.fromString(value); ? Done. http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:397: if (index == -1 || s.length == index - 1) { On 2012/05/21 07:37:30, ajohnsen wrote: > Did you mean 'index == s.length - 1'? No, this is either not found or last char. http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:398: primaryType = s; On 2012/05/21 07:40:39, Mads Ager wrote: > s.trim().toLowerCase()? Done. http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:404: On 2012/05/21 07:40:39, Mads Ager wrote: > Extra space Done. http://codereview.chromium.org/10407002/diff/1/runtime/bin/io.dart File runtime/bin/io.dart (right): http://codereview.chromium.org/10407002/diff/1/runtime/bin/io.dart#newcode16 runtime/bin/io.dart:16: #import("dart:builtin"); On 2012/05/21 07:37:30, ajohnsen wrote: > Did you mean to do this? No, removed. http://codereview.chromium.org/10407002/diff/1/runtime/bin/io.dart#newcode16 runtime/bin/io.dart:16: #import("dart:builtin"); On 2012/05/21 07:40:39, Mads Ager wrote: > Remove. :-) Done - sorry for always leaving this here. http://codereview.chromium.org/10407002/diff/1/tests/standalone/io/http_heade... File tests/standalone/io/http_headers_test.dart (right): http://codereview.chromium.org/10407002/diff/1/tests/standalone/io/http_heade... tests/standalone/io/http_headers_test.dart:243: contentType = new _ContentType.fromString(" text/html ; charset = utf-8 "); On 2012/05/21 07:40:39, Mads Ager wrote: > Long lines. Done. http://codereview.chromium.org/10407002/diff/1/tests/standalone/io/http_test.... File tests/standalone/io/http_test.dart (right): http://codereview.chromium.org/10407002/diff/1/tests/standalone/io/http_test.... tests/standalone/io/http_test.dart:174: response.headers.set(HttpHeaders.CONTENT_TYPE, "text/html; charset = utf-8"); On 2012/05/21 07:40:39, Mads Ager wrote: > Long line. Also multiple long lines below. Done. http://codereview.chromium.org/10407002/diff/1/tests/standalone/io/http_test.... tests/standalone/io/http_test.dart:590: /*testStartStop(); On 2012/05/21 07:37:30, ajohnsen wrote: > We should still run every test ;) Done. http://codereview.chromium.org/10407002/diff/1/tests/standalone/io/http_test.... tests/standalone/io/http_test.dart:590: /*testStartStop(); On 2012/05/21 07:40:39, Mads Ager wrote: > Code in comments. Done.
http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart File runtime/bin/http_impl.dart (right): http://codereview.chromium.org/10407002/diff/1/runtime/bin/http_impl.dart#new... runtime/bin/http_impl.dart:397: if (index == -1 || s.length == index - 1) { On 2012/05/21 11:11:06, Søren Gjesse wrote: > On 2012/05/21 07:37:30, ajohnsen wrote: > > Did you mean 'index == s.length - 1'? > > No, this is either not found or last char. But the last char is the case where index is s.length - 1? What we have not is either not found or index == s.length + 1, two chars after the end. That, or brain meltdown here :)
LGTM Considering immutable header values could still be a good idea to avoid tracking changes to objects. What if you extract ContentType, then change it, extract another and then modify the first? https://chromiumcodereview.appspot.com/10407002/diff/9001/runtime/bin/http_im... File runtime/bin/http_impl.dart (right): https://chromiumcodereview.appspot.com/10407002/diff/9001/runtime/bin/http_im... runtime/bin/http_impl.dart:404: if (index == -1 || s.length == index - 1) { I agree with Anders about this being: index == (s.length - 1) However, it will probably work without that check because the substring for the subType below will be the empty string. :-) https://chromiumcodereview.appspot.com/10407002/diff/9001/tests/standalone/io... File tests/standalone/io/http_test.dart (right): https://chromiumcodereview.appspot.com/10407002/diff/9001/tests/standalone/io... tests/standalone/io/http_test.dart:560: Expect.equals("utf-8", response.headers.contentType.parameters["charset"]); Long line.
Ended up removing the caching of the parsed content type. Now it is recreated from the string on each access and setting it will change the header string directly. https://chromiumcodereview.appspot.com/10407002/diff/1/runtime/bin/http_impl.... File runtime/bin/http_impl.dart (right): https://chromiumcodereview.appspot.com/10407002/diff/1/runtime/bin/http_impl.... runtime/bin/http_impl.dart:397: if (index == -1 || s.length == index - 1) { On 2012/05/21 11:27:03, ajohnsen wrote: > On 2012/05/21 11:11:06, Søren Gjesse wrote: > > On 2012/05/21 07:37:30, ajohnsen wrote: > > > Did you mean 'index == s.length - 1'? > > > > No, this is either not found or last char. > > But the last char is the case where index is s.length - 1? What we have not is > either not found or index == s.length + 1, two chars after the end. That, or > brain meltdown here :) My brain meltdown - sorry for being stupid. https://chromiumcodereview.appspot.com/10407002/diff/9001/runtime/bin/http_im... File runtime/bin/http_impl.dart (right): https://chromiumcodereview.appspot.com/10407002/diff/9001/runtime/bin/http_im... runtime/bin/http_impl.dart:404: if (index == -1 || s.length == index - 1) { On 2012/05/21 11:49:33, Mads Ager wrote: > I agree with Anders about this being: index == (s.length - 1) > > However, it will probably work without that check because the substring for the > subType below will be the empty string. :-) Of cause you are both right. https://chromiumcodereview.appspot.com/10407002/diff/9001/tests/standalone/io... File tests/standalone/io/http_test.dart (right): https://chromiumcodereview.appspot.com/10407002/diff/9001/tests/standalone/io... tests/standalone/io/http_test.dart:560: Expect.equals("utf-8", response.headers.contentType.parameters["charset"]); On 2012/05/21 11:49:33, Mads Ager wrote: > Long line. Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
