Add special handling of the content type HTTP header

runtime/bin/http_impl.dart

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 class _HttpHeaders implements HttpHeaders {
   _HttpHeaders() : _headers = new Map<String, List<String>>();
 
   List<String> operator[](String name) {
     name = name.toLowerCase();
     return _headers[name];
   }
 
   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) {
       throw new HttpException("More than one value for header $name");
     }
     return values[0];
   }
 
   void add(String name, Object value) {
     _checkMutable();
     if (value is List) {
       for (int i = 0; i < value.length; i++) {
         _add(name, value[i]);
       }
     } else {
       _add(name, value);
     }
   }
 
   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) {
     _checkMutable();
     name = name.toLowerCase();
     List<String> values = _headers[name];
     if (values != null) {
       int index = values.indexOf(value);
       if (index != -1) {
         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);
   }
 
   String get host() => _host;
 
   void set host(String host) {
     _checkMutable();
     _host = host;
     _updateHostHeader();
   }
@@ skipping 39 matching lines @@
   }
 
   void set expires(Date expires) {
     _checkMutable();
     // Format "Expires" header with date in Greenwich Mean Time (GMT).
     String formatted =
         _HttpUtils.formatDate(expires.changeTimeZone(new TimeZone.utc()));
     _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") {
       if (value is Date) {
         date = value;
       } else if (value is String) {
         _set("date", value);
       } else {
         throw new HttpException("Unexpected type for header named $name");
       }
@@ skipping 20 matching lines @@
           _port = HttpClient.DEFAULT_HTTP_PORT;
         } else {
           try {
             _port = Math.parseInt(value.substring(pos + 1));
           } catch (BadNumberFormatException e) {
             _port = null;
           }
         }
         _set("host", value);
       }
+    } else if (name.toLowerCase() == "content-type") {
+      _set("content-type", value);
+      _contentType = null;
     } else {
       name = name.toLowerCase();
       List<String> values = _headers[name];
       if (values == null) {
         values = new List<String>();
         _headers[name] = values;
       }
       values.add(value.toString());
     }
   }
@@ skipping 13 matching lines @@
     bool defaultPort = _port == null || _port == HttpClient.DEFAULT_HTTP_PORT;
     String portPart = defaultPort ? "" : ":$_port";
     _set("host", "$host$portPart");
   }
 
   _write(_HttpConnectionBase connection) {
     final COLONSP = const [_CharCode.COLON, _CharCode.SP];
     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;
       data = name.charCodes();
       connection._write(data);
       connection._write(COLONSP);
       for (int i = 0; i < values.length; i++) {
         if (i > 0) {
           connection._write(COMMASP);
         }
         data = values[i].charCodes();
         connection._write(data);
       }
       connection._write(CRLF);
     });
   }
 
+  _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);
       sb.add(": ");
       for (int i = 0; i < values.length; i++) {
         if (i > 0) {
           sb.add(", ");
         }
         sb.add(values[i]);
       }
       sb.add("\n");
     });
     return sb.toString();
   }
 
   bool _mutable = true;  // Are the headers currently mutable?
   Map<String, List<String>> _headers;
 
   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 = "";
+}
+
+
 class _HttpRequestResponseBase {
   final int START = 0;
   final int HEADER_SENT = 1;
   final int DONE = 2;
   final int UPGRADED = 3;
 
   _HttpRequestResponseBase(_HttpConnectionBase this._httpConnection)
       : _headers = new _HttpHeaders() {
     _state = START;
   }
@@ skipping 1405 matching lines @@
 
 
 class _RedirectInfo implements RedirectInfo {
   const _RedirectInfo(int this.statusCode,
                       String this.method,
                       Uri this.location);
   final int statusCode;
   final String method;
   final Uri location;
 }