Start refactoring of the HTTP header handling

Start refactoring of the HTTP header handling

This change moves the headers into a separate object.

There will be follow-up changes for handling whether the headers are
read-only or mutable. Also additional tests will be added later.

R=ajohnsen@google.com

BUG=
TEST=

Committed: https://code.google.com/p/dart/source/detail?r=6567

[Anders Johnsen, 2012-04-13 11:47:53]

LGTM, This will simplify the http implementation, very neat! :)

https://chromiumcodereview.appspot.com/10082003/diff/1/runtime/bin/http.dart
File runtime/bin/http.dart (right):

https://chromiumcodereview.appspot.com/10082003/diff/1/runtime/bin/http.dart#...
runtime/bin/http.dart:110: interface HttpHeaders default _HttpHeaders {
Very neat, I like how you expose all normal headers, including categories!
Should headers related WebSockets be grouped together?

https://chromiumcodereview.appspot.com/10082003/diff/1/runtime/bin/http.dart#...
runtime/bin/http.dart:215: List<String> operator[](String name);
Now we have no trivial way of getting the only header available (the most common
case). I'm not saying it's bad, since the user should consider all cases. I'm
just not convinced it's ideal. What do you think?

https://chromiumcodereview.appspot.com/10082003/diff/1/runtime/bin/http.dart#...
runtime/bin/http.dart:225: void add(String name, value);
Would it be better to say either var or Object? Having no type is kind of
catching the eye, giving everything else is types.

https://chromiumcodereview.appspot.com/10082003/diff/1/runtime/bin/http.dart#...
runtime/bin/http.dart:236: void set(String name, value);
Can value be a List?

https://chromiumcodereview.appspot.com/10082003/diff/1/runtime/bin/http_impl....
File runtime/bin/http_impl.dart (right):

https://chromiumcodereview.appspot.com/10082003/diff/1/runtime/bin/http_impl....
runtime/bin/http_impl.dart:75: Date get expires() {
expires is not defined in interface. Should it? Same with host and port.

https://chromiumcodereview.appspot.com/10082003/diff/1/runtime/bin/http_impl....
runtime/bin/http_impl.dart:119: Date _expires;
Can we do with keeping a String in _headers, and not have any custom fields?

https://chromiumcodereview.appspot.com/10082003/diff/1/runtime/bin/http_impl....
runtime/bin/http_impl.dart:181: for (int i = 0; i < values.length; i++) {
We could move this to the _HttpHeader class, with some private members, say
parse and stringify?

[Søren Gjesse, 2012-04-16 11:37:16]

ptal

https://chromiumcodereview.appspot.com/10082003/diff/1/runtime/bin/http.dart
File runtime/bin/http.dart (right):

https://chromiumcodereview.appspot.com/10082003/diff/1/runtime/bin/http.dart#...
runtime/bin/http.dart:110: interface HttpHeaders default _HttpHeaders {
On 2012/04/13 11:47:53, ajohnsen wrote:
> Very neat, I like how you expose all normal headers, including categories!
> Should headers related WebSockets be grouped together?

They should, but maybe they should be part of the WebSocket interfaces/classes
and not HttpHeaders.

https://chromiumcodereview.appspot.com/10082003/diff/1/runtime/bin/http.dart#...
runtime/bin/http.dart:215: List<String> operator[](String name);
On 2012/04/13 11:47:53, ajohnsen wrote:
> Now we have no trivial way of getting the only header available (the most
common
> case). I'm not saying it's bad, since the user should consider all cases. I'm
> just not convinced it's ideal. What do you think?

I agree, in the tests I had to use [0] in quite a few places which was annoying.
However I have no good suggestion other than adding an additional method to
return a string.

https://chromiumcodereview.appspot.com/10082003/diff/1/runtime/bin/http.dart#...
runtime/bin/http.dart:225: void add(String name, value);
On 2012/04/13 11:47:53, ajohnsen wrote:
> Would it be better to say either var or Object? Having no type is kind of
> catching the eye, giving everything else is types.

Added type Object

https://chromiumcodereview.appspot.com/10082003/diff/1/runtime/bin/http.dart#...
runtime/bin/http.dart:236: void set(String name, value);
On 2012/04/13 11:47:53, ajohnsen wrote:
> Can value be a List?

That should be fine. It should then add each element. That has been added. Same
for add where adding a List will add as separate elements.

https://chromiumcodereview.appspot.com/10082003/diff/1/runtime/bin/http_impl....
File runtime/bin/http_impl.dart (right):

https://chromiumcodereview.appspot.com/10082003/diff/1/runtime/bin/http_impl....
runtime/bin/http_impl.dart:75: Date get expires() {
On 2012/04/13 11:47:53, ajohnsen wrote:
> expires is not defined in interface. Should it? Same with host and port.

The interface has

Date expires;
String host;
int port;

which maps to these getters and setters.

https://chromiumcodereview.appspot.com/10082003/diff/1/runtime/bin/http_impl....
runtime/bin/http_impl.dart:119: Date _expires;
On 2012/04/13 11:47:53, ajohnsen wrote:
> Can we do with keeping a String in _headers, and not have any custom fields?

We could, but I am not sure it would be less complicated. _expires is just a
cache for the parsed date. For setting host and port one would have to parse and
split the Host header each time.

https://chromiumcodereview.appspot.com/10082003/diff/1/runtime/bin/http_impl....
runtime/bin/http_impl.dart:181: for (int i = 0; i < values.length; i++) {
On 2012/04/13 11:47:53, ajohnsen wrote:
> We could move this to the _HttpHeader class, with some private members, say
> parse and stringify?

Added a _write method which takes a _HttpConnectionBase. Also added toString for
debugging.

[Anders Johnsen, 2012-04-16 11:50:46]

LGTM, very nice!

https://chromiumcodereview.appspot.com/10082003/diff/1/runtime/bin/http.dart
File runtime/bin/http.dart (right):

https://chromiumcodereview.appspot.com/10082003/diff/1/runtime/bin/http.dart#...
runtime/bin/http.dart:215: List<String> operator[](String name);
On 2012/04/16 11:37:16, Søren Gjesse wrote:
> On 2012/04/13 11:47:53, ajohnsen wrote:
> > Now we have no trivial way of getting the only header available (the most
> common
> > case). I'm not saying it's bad, since the user should consider all cases.
I'm
> > just not convinced it's ideal. What do you think?
> 
> I agree, in the tests I had to use [0] in quite a few places which was
annoying.
> However I have no good suggestion other than adding an additional method to
> return a string.

I think we should just do that, and throw an exception if we have multiple
values.

https://chromiumcodereview.appspot.com/10082003/diff/1/runtime/bin/http.dart#...
runtime/bin/http.dart:236: void set(String name, value);
On 2012/04/16 11:37:16, Søren Gjesse wrote:
> On 2012/04/13 11:47:53, ajohnsen wrote:
> > Can value be a List?
> 
> That should be fine. It should then add each element. That has been added.
Same
> for add where adding a List will add as separate elements.

Ty!

https://chromiumcodereview.appspot.com/10082003/diff/1/runtime/bin/http_impl....
File runtime/bin/http_impl.dart (right):

https://chromiumcodereview.appspot.com/10082003/diff/1/runtime/bin/http_impl....
runtime/bin/http_impl.dart:75: Date get expires() {
On 2012/04/16 11:37:16, Søren Gjesse wrote:
> On 2012/04/13 11:47:53, ajohnsen wrote:
> > expires is not defined in interface. Should it? Same with host and port.
> 
> The interface has
> 
> Date expires;
> String host;
> int port;
> 
> which maps to these getters and setters.

Ahh, I see.

https://chromiumcodereview.appspot.com/10082003/diff/1/runtime/bin/http_impl....
runtime/bin/http_impl.dart:119: Date _expires;
On 2012/04/16 11:37:16, Søren Gjesse wrote:
> On 2012/04/13 11:47:53, ajohnsen wrote:
> > Can we do with keeping a String in _headers, and not have any custom fields?
> 
> We could, but I am not sure it would be less complicated. _expires is just a
> cache for the parsed date. For setting host and port one would have to parse
and
> split the Host header each time.

Okay, let's keep as is then.

https://chromiumcodereview.appspot.com/10082003/diff/1/runtime/bin/http_impl....
runtime/bin/http_impl.dart:181: for (int i = 0; i < values.length; i++) {
On 2012/04/16 11:37:16, Søren Gjesse wrote:
> On 2012/04/13 11:47:53, ajohnsen wrote:
> > We could move this to the _HttpHeader class, with some private members, say
> > parse and stringify?
> 
> Added a _write method which takes a _HttpConnectionBase. Also added toString
for
> debugging.

Thank you!

https://chromiumcodereview.appspot.com/10082003/diff/4001/runtime/bin/http_im...
File runtime/bin/http_impl.dart (right):

https://chromiumcodereview.appspot.com/10082003/diff/4001/runtime/bin/http_im...
runtime/bin/http_impl.dart:143: String toString() {
Looks good, and yes, let's use \n for line feed here. I do however thing we
should return 'sb.toString()' :)

[Søren Gjesse, 2012-04-16 12:17:31]

https://chromiumcodereview.appspot.com/10082003/diff/1/runtime/bin/http.dart
File runtime/bin/http.dart (right):

https://chromiumcodereview.appspot.com/10082003/diff/1/runtime/bin/http.dart#...
runtime/bin/http.dart:215: List<String> operator[](String name);
On 2012/04/16 11:50:46, ajohnsen wrote:
> On 2012/04/16 11:37:16, Søren Gjesse wrote:
> > On 2012/04/13 11:47:53, ajohnsen wrote:
> > > Now we have no trivial way of getting the only header available (the most
> > common
> > > case). I'm not saying it's bad, since the user should consider all cases.
> I'm
> > > just not convinced it's ideal. What do you think?
> > 
> > I agree, in the tests I had to use [0] in quite a few places which was
> annoying.
> > However I have no good suggestion other than adding an additional method to
> > return a string.
> 
> I think we should just do that, and throw an exception if we have multiple
> values.

Added method value for now.

https://chromiumcodereview.appspot.com/10082003/diff/4001/runtime/bin/http_im...
File runtime/bin/http_impl.dart (right):

https://chromiumcodereview.appspot.com/10082003/diff/4001/runtime/bin/http_im...
runtime/bin/http_impl.dart:143: String toString() {
On 2012/04/16 11:50:46, ajohnsen wrote:
> Looks good, and yes, let's use \n for line feed here. I do however thing we
> should return 'sb.toString()' :)

Good catch. Added return of sb.toString().