Refactor HTTP parser to hold current buffer

Refactor HTTP parser to hold current buffer

The HTTP parser now stores the current buffer internally. This change
is to prepare for stopping the parser when a request have been
received and resuming parsing when this response have been fully
processed.

R=ager@google.com

BUG=

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

[Mads Ager (google), 2012-11-07 13:02:51]

https://codereview.chromium.org/11359085/diff/1/sdk/lib/io/http_impl.dart
File sdk/lib/io/http_impl.dart (left):

https://codereview.chromium.org/11359085/diff/1/sdk/lib/io/http_impl.dart#old...
sdk/lib/io/http_impl.dart:1315: _destroy();
With this change we lose this _destroy call. If we get to the end of parse
without having parsed the entire buffer, we don't do anything now. Should we?

https://codereview.chromium.org/11359085/diff/1/sdk/lib/io/http_parser.dart
File sdk/lib/io/http_parser.dart (right):

https://codereview.chromium.org/11359085/diff/1/sdk/lib/io/http_parser.dart#n...
sdk/lib/io/http_parser.dart:444: _index++;
The reason for incrementing here and then avoiding the increment after the
switch is so that the headersComplete() callback can use readUnparsedData,
right?

https://codereview.chromium.org/11359085/diff/1/sdk/lib/io/http_parser.dart#n...
sdk/lib/io/http_parser.dart:572: // If all data is parsed there is not need to
hold on to the buffer.
there is not need -> there is no need

https://codereview.chromium.org/11359085/diff/1/sdk/lib/io/http_parser.dart#n...
sdk/lib/io/http_parser.dart:721: // The data currently parsed.
The data that is currently being parsed?

[Søren Gjesse, 2012-11-07 13:31:14]

https://codereview.chromium.org/11359085/diff/1/sdk/lib/io/http_impl.dart
File sdk/lib/io/http_impl.dart (left):

https://codereview.chromium.org/11359085/diff/1/sdk/lib/io/http_impl.dart#old...
sdk/lib/io/http_impl.dart:1315: _destroy();
On 2012/11/07 13:02:51, Mads Ager wrote:
> With this change we lose this _destroy call. If we get to the end of parse
> without having parsed the entire buffer, we don't do anything now. Should we?

The parser should always either successfuly parse all data (modulo upgrade) or
report an error. The destruction of the socket here should have resulted in a
onError callback from the HTTP parser.

https://codereview.chromium.org/11359085/diff/1/sdk/lib/io/http_parser.dart
File sdk/lib/io/http_parser.dart (right):

https://codereview.chromium.org/11359085/diff/1/sdk/lib/io/http_parser.dart#n...
sdk/lib/io/http_parser.dart:444: _index++;
On 2012/11/07 13:02:51, Mads Ager wrote:
> The reason for incrementing here and then avoiding the increment after the
> switch is so that the headersComplete() callback can use readUnparsedData,
> right?

Yes, but thats not good. Changed the handling of _index to get rid of this.

https://codereview.chromium.org/11359085/diff/1/sdk/lib/io/http_parser.dart#n...
sdk/lib/io/http_parser.dart:572: // If all data is parsed there is not need to
hold on to the buffer.
On 2012/11/07 13:02:51, Mads Ager wrote:
> there is not need -> there is no need

Done.

https://codereview.chromium.org/11359085/diff/1/sdk/lib/io/http_parser.dart#n...
sdk/lib/io/http_parser.dart:721: // The data currently parsed.
On 2012/11/07 13:02:51, Mads Ager wrote:
> The data that is currently being parsed?

Done.

[Mads Ager (google), 2012-11-07 13:36:43]

LGTM

https://codereview.chromium.org/11359085/diff/5/sdk/lib/io/http_parser.dart
File sdk/lib/io/http_parser.dart (right):

https://codereview.chromium.org/11359085/diff/5/sdk/lib/io/http_parser.dart#n...
sdk/lib/io/http_parser.dart:566: if (_index == _lastIndex) _releaseBuffer();
Can we make this stronger? Maybe add an assert that _state is FAILURE or _index
== _lastIndex or something similar. Is there any reason to not _releaseBuffer
always?

[Søren Gjesse, 2012-11-07 13:58:33]

https://codereview.chromium.org/11359085/diff/5/sdk/lib/io/http_parser.dart
File sdk/lib/io/http_parser.dart (right):

https://codereview.chromium.org/11359085/diff/5/sdk/lib/io/http_parser.dart#n...
sdk/lib/io/http_parser.dart:566: if (_index == _lastIndex) _releaseBuffer();
On 2012/11/07 13:36:43, Mads Ager wrote:
> Can we make this stronger? Maybe add an assert that _state is FAILURE or
_index
> == _lastIndex or something similar. Is there any reason to not _releaseBuffer
> always?

Changed to release buffer except when connection is upgraded.