Add special handling of the content type HTTP header

runtime/bin/http_impl.dart

Index: runtime/bin/http_impl.dart
diff --git a/runtime/bin/http_impl.dart b/runtime/bin/http_impl.dart
index 05c0a8f3907ac8e08d5e94d19a022374a5f9430a..baf042dc29924417eca1c65810a6739881220536 100644
--- a/runtime/bin/http_impl.dart
+++ b/runtime/bin/http_impl.dart
@@ -12,6 +12,7 @@ class _HttpHeaders implements HttpHeaders {
 
   String value(String name) {
     name = name.toLowerCase();
+    if (name == "content-type") _syncContentType();
     List<String> values = _headers[name];
     if (values == null) return null;
     if (values.length > 1) {
@@ -32,9 +33,11 @@ class _HttpHeaders implements HttpHeaders {
   }
 
   void set(String name, Object value) {
+    name = name.toLowerCase();
     _checkMutable();
     removeAll(name);
     add(name, value);
+    if (name == "content-type") contentType == null;

[Mads Ager (google), 2012/05/21 07:40:39]

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?

[Anders Johnsen, 2012/05/21 07:43:21]

=, not ==.

[Søren Gjesse, 2012/05/21 11:11:06]

Done.

[Søren Gjesse, 2012/05/21 11:11:06]

Added _clearHeaderValueCache and added test.

   }
 
   void remove(String name, Object value) {
@@ -47,15 +50,18 @@ class _HttpHeaders implements HttpHeaders {
         values.removeRange(index, 1);
       }
     }
+    if (name == "content-type") contentType == null;

[Mads Ager (google), 2012/05/21 07:40:39]

_contentType?

[Anders Johnsen, 2012/05/21 07:43:21]

=, not ==.

[Søren Gjesse, 2012/05/21 11:11:06]

Used _clearHeaderValueCache.

[Søren Gjesse, 2012/05/21 11:11:06]

Done.

   }
 
   void removeAll(String name) {
     _checkMutable();
     name = name.toLowerCase();
     _headers.remove(name);
+    contentType == null;

[Mads Ager (google), 2012/05/21 07:40:39]

_contentType?

[Anders Johnsen, 2012/05/21 07:43:21]

=, not ==.

[Søren Gjesse, 2012/05/21 11:11:06]

Used _clearHeaderValueCache.

[Søren Gjesse, 2012/05/21 11:11:06]

Done.

   }
 
   void forEach(void f(String name, List<String> values)) {
+    _syncContentType();
     _headers.forEach(f);
   }
 
@@ -115,6 +121,18 @@ class _HttpHeaders implements HttpHeaders {
     _set("expires", formatted);
   }
 
+  void get contentType() {

[Anders Johnsen, 2012/05/21 07:37:30]

Would it be better/simpler to not have the field _contentType, and just generate
it every time contentType is called?

[Søren Gjesse, 2012/05/21 11:11:06]

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.

+    if (_contentType == null) {
+      var values = _headers["content-type"];
+      if (values != null) {
+        _contentType = new ContentType.fromString(values[0]);
+      } else {
+        _contentType = new ContentType();
+      }
+    }
+    return _contentType;
+  }
+
   void _add(String name, Object value) {
     // TODO(sgjesse): Add immutable state throw HttpException is immutable.
     if (name.toLowerCase() == "date") {
@@ -155,6 +173,9 @@ class _HttpHeaders implements HttpHeaders {
         }
         _set("host", value);
       }
+    } else if (name.toLowerCase() == "content-type") {
+      _set("content-type", value);
+      _contentType = null;
     } else {
       name = name.toLowerCase();
       List<String> values = _headers[name];
@@ -188,6 +209,8 @@ class _HttpHeaders implements HttpHeaders {
     final COMMASP = const [_CharCode.COMMA, _CharCode.SP];
     final CRLF = const [_CharCode.CR, _CharCode.LF];
 
+    _syncContentType();
+
     // Format headers.
     _headers.forEach((String name, List<String> values) {
       List<int> data;
@@ -205,7 +228,14 @@ class _HttpHeaders implements HttpHeaders {
     });
   }
 
+  _syncContentType() {

[Mads Ager (google), 2012/05/21 07:40:39]

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?

[Søren Gjesse, 2012/05/21 11:11:06]

As the HttpHeaders object is mutable it would be strange if the contentType
property was not.

+    if (_contentType != null) {
+      _set("content-type", _contentType.toString());
+    }
+  }
+
   String toString() {
+    _syncContentType();
     StringBuffer sb = new StringBuffer();
     _headers.forEach((String name, List<String> values) {
       sb.add(name);
@@ -226,6 +256,173 @@ class _HttpHeaders implements HttpHeaders {
 
   String _host;
   int _port;
+  ContentType _contentType;
+}
+
+
+class _HeaderValue implements HeaderValue {
+  _HeaderValue([String this.value = ""]);
+
+  _HeaderValue.fromString(String value) {
+    // Parse the string.
+    _parse(value);
+  }
+
+  Map<String, String> get parameters() {
+    if (_parameters == null) _parameters = new Map<String, String>();
+    return _parameters;
+  }
+
+  String toString() {
+    StringBuffer sb = new StringBuffer();
+    sb.add(value);
+    if (parameters != null && parameters.length > 0) {
+      _parameters.forEach((String name, String value) {
+        sb.add("; ");
+        sb.add(name);
+        sb.add("=");
+        sb.add(value);
+      });
+    }
+    return sb.toString();
+  }
+
+  void _parse(String s) {
+    int index = 0;
+
+    void skipWS() {
+      while (index < s.length) {

[Anders Johnsen, 2012/05/21 07:37:30]

!done()

[Søren Gjesse, 2012/05/21 11:11:06]

Done.

+        if (s[index] != " " && s[index] != "\t") return;
+        index++;
+      }
+    }
+
+    String parseValue() {
+      int start = index;
+      while (index < s.length) {

[Anders Johnsen, 2012/05/21 07:37:30]

!done()

[Søren Gjesse, 2012/05/21 11:11:06]

Done.

+        if (s[index] == " " || s[index] == "\t" || s[index] == ";") break;
+        index++;
+      }
+      return s.substring(start, index).toLowerCase();
+    }
+
+    void expect(String expected) {
+      if (index == s.length) throw new HttpException("YYY");

[Anders Johnsen, 2012/05/21 07:37:30]

Some real string here :)

[Anders Johnsen, 2012/05/21 07:37:30]

if(done()) ?

[Mads Ager (google), 2012/05/21 07:40:39]

More useful error message?

[Søren Gjesse, 2012/05/21 11:11:06]

Done.

[Søren Gjesse, 2012/05/21 11:11:06]

Done.

[Søren Gjesse, 2012/05/21 11:11:06]

Done.

+      if (s[index] != expected) throw new HttpException("XXX $expected [${s[index]}]");

[Anders Johnsen, 2012/05/21 07:37:30]

Ditto.

[Mads Ager (google), 2012/05/21 07:40:39]

Ditto and a long line.

[Søren Gjesse, 2012/05/21 11:11:06]

Done.

[Søren Gjesse, 2012/05/21 11:11:06]

Done and done.

+      index++;
+    }
+
+    bool done() => index == s.length;
+
+    void parseParameters() {
+      _parameters = new Map<String, String>();
+
+      String parseParameterName() {
+        int start = index;
+        while (index < s.length) {

[Anders Johnsen, 2012/05/21 07:37:30]

!done()

[Søren Gjesse, 2012/05/21 11:11:06]

Done.

+          if (s[index] == " " || s[index] == "\t" || s[index] == "=") break;
+          index++;
+        }
+        return s.substring(start, index).toLowerCase();
+      }
+
+      String parseParameterValue() {
+        if (s[index] == "\"") {
+          // Parse quoted value.
+          StringBuffer sb = new StringBuffer();
+          index++;
+          while (index < s.length) {

[Anders Johnsen, 2012/05/21 07:37:30]

Maybe also !done() here, but that's 100% up to you - just though we might as
well use it. 

[Søren Gjesse, 2012/05/21 11:11:06]

Done.

+            if (s[index] == "\\") {

[Anders Johnsen, 2012/05/21 07:37:30]

Are we actually handling this correctly? Should we escape some symbols?

[Mads Ager (google), 2012/05/21 07:40:39]

Does the backslash have no special meaning? It doesn't escape anything? It is
just ignored in the code.

[Søren Gjesse, 2012/05/21 11:11:06]

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.

[Søren Gjesse, 2012/05/21 11:11:06]

See other comment.

+              if (index + 1 == s.length) throw new HttpException("ZZZ");

[Anders Johnsen, 2012/05/21 07:37:30]

Real string.

[Mads Ager (google), 2012/05/21 07:40:39]

Update message.

[Søren Gjesse, 2012/05/21 11:11:06]

Done.

[Søren Gjesse, 2012/05/21 11:11:06]

Done.

+              index++;
+            } else if (s[index] == "\"") {
+              index++;
+              break;
+            }
+            sb.add(s[index]);
+            index++;
+          }
+          return sb.toString();
+        } else {
+          // Parse non-quoted value.

[Anders Johnsen, 2012/05/21 07:37:30]

Change this to 'return parseValue()'?

[Søren Gjesse, 2012/05/21 11:11:06]

Done.

+          int start = index;
+          while (index < s.length) {
+            if (s[index] == " " || s[index] == "\t" || s[index] == ";") break;
+            index++;
+          }
+          return s.substring(start, index).toLowerCase();
+        }
+      }
+
+      while (!done()) {
+        skipWS();
+        if (done()) return;
+        String name = parseParameterName();
+        skipWS();
+        expect("=");
+        skipWS();
+        String value = parseParameterValue();
+        _parameters[name] = value;
+        skipWS();
+        if (done()) return;
+        expect(";");
+      }
+    }
+
+    skipWS();
+    value = parseValue();
+    skipWS();
+    if (done()) return;
+    expect(";");
+    parseParameters();
+  }
+
+  String value;
+  Map<String, String> _parameters;
+}
+
+
+class _ContentType extends _HeaderValue implements ContentType {
+  _ContentType([String this._primaryType = "", String this._subType = ""]);
+
+  _ContentType.fromString(String value) {

[Anders Johnsen, 2012/05/21 07:37:30]

: super.fromString(value); ?

[Søren Gjesse, 2012/05/21 11:11:06]

Done.

+    // Parse the string.
+    _parse(value);
+  }
+
+  String get value() => "$_primaryType/$_subType";
+
+  void set value(String s) {
+    int index = s.indexOf("/");
+    if (index == -1 || s.length == index - 1) {

[Anders Johnsen, 2012/05/21 07:37:30]

Did you mean 'index == s.length - 1'?

[Søren Gjesse, 2012/05/21 11:11:06]

No, this is either not found or last char.

[Anders Johnsen, 2012/05/21 11:27:03]

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 :)

[Søren Gjesse, 2012/05/22 12:47:16]

My brain meltdown - sorry for being stupid.

+      primaryType = s;

[Mads Ager (google), 2012/05/21 07:40:39]

s.trim().toLowerCase()?

[Søren Gjesse, 2012/05/21 11:11:06]

Done.

+    } else {
+      primaryType = s.substring(0, index).trim().toLowerCase();
+      subType = s.substring(index + 1).trim().toLowerCase();
+    }
+  }
+

[Mads Ager (google), 2012/05/21 07:40:39]

Extra space

[Søren Gjesse, 2012/05/21 11:11:06]

Done.

+
+  String get primaryType() => _primaryType;
+
+  void set primaryType(String s) {
+    _primaryType = s;
+  }
+
+  String get subType() => _subType;
+
+  void set subType(String s) {
+    _subType = s;
+  }
+
+  String get charset() => parameters["charset"];
+
+  void set charset(String s) {
+    parameters["charset"] = s;
+  }
+
+  String _primaryType = "";
+  String _subType = "";
 }