pkg/http_server: return future for VirtualDirectory serveRequest

pkg/http_server: return future for VirtualDirectory serveRequest

Also:
- Return the subscription from serve method
- Bumped version

BUG= https://code.google.com/p/dart/issues/detail?id=15893
BUG= https://code.google.com/p/dart/issues/detail?id=16005
R=ajohnsen@google.com

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

[sethladd, 2014-01-09 05:27:47]

https://codereview.chromium.org/124833003/diff/320001/pkg/http_server/lib/src...
File pkg/http_server/lib/src/virtual_directory.dart (right):

https://codereview.chromium.org/124833003/diff/320001/pkg/http_server/lib/src...
pkg/http_server/lib/src/virtual_directory.dart:160: void serveFile(File file,
HttpRequest request) {
why not return the future from here?

https://codereview.chromium.org/124833003/diff/320001/pkg/http_server/lib/src...
pkg/http_server/lib/src/virtual_directory.dart:300: .whenComplete(() {
can this be a one-line function?

https://codereview.chromium.org/124833003/diff/320001/pkg/http_server/test/ht...
File pkg/http_server/test/http_mock.dart (right):

https://codereview.chromium.org/124833003/diff/320001/pkg/http_server/test/ht...
pkg/http_server/test/http_mock.dart:1: library htt_mock;
is this a candidate to be its own package?

[Anders Johnsen, 2014-01-09 06:12:23]

https://codereview.chromium.org/124833003/diff/320001/pkg/http_server/lib/src...
File pkg/http_server/lib/src/virtual_directory.dart (right):

https://codereview.chromium.org/124833003/diff/320001/pkg/http_server/lib/src...
pkg/http_server/lib/src/virtual_directory.dart:160: void serveFile(File file,
HttpRequest request) {
On 2014/01/09 05:27:48, sethladd wrote:
> why not return the future from here?

response.done is the right way of doing it. Also, it'll make it easier for the
user to override dir-listing, etc., and make the future return value to work.

https://codereview.chromium.org/124833003/diff/320001/pkg/http_server/lib/src...
pkg/http_server/lib/src/virtual_directory.dart:285: dir.list(followLinks:
true).forEach((entity) {
This is not handling errors (they can happen if FS is modified while listing).

https://codereview.chromium.org/124833003/diff/320001/pkg/http_server/lib/src...
pkg/http_server/lib/src/virtual_directory.dart:301: return response.close();
Why return here? This value is not kept.

https://codereview.chromium.org/124833003/diff/320001/pkg/http_server/test/ht...
File pkg/http_server/test/http_mock.dart (right):

https://codereview.chromium.org/124833003/diff/320001/pkg/http_server/test/ht...
pkg/http_server/test/http_mock.dart:1: library htt_mock;
http_mock?

https://codereview.chromium.org/124833003/diff/320001/pkg/http_server/test/ut...
File pkg/http_server/test/utils.dart (right):

https://codereview.chromium.org/124833003/diff/320001/pkg/http_server/test/ut...
pkg/http_server/test/utils.dart:31: _testLogicRequestExpando[currentTestCase] =
true;
This stuff really needs some documentation. What is the goal of doing this?

https://codereview.chromium.org/124833003/diff/320001/pkg/http_server/test/ut...
pkg/http_server/test/utils.dart:96: if(_testLogicRequestExpando[currentTestCase]
== true) {
remove '== true'

https://codereview.chromium.org/124833003/diff/320001/pkg/http_server/test/ut...
pkg/http_server/test/utils.dart:115:
if(_testLogicRequestExpando[currentTestCase] == true) {
remove '== true'.

[kevmoo, 2014-01-10 05:01:59]

First set of review fixes

https://codereview.chromium.org/124833003/diff/320001/pkg/http_server/lib/src...
File pkg/http_server/lib/src/virtual_directory.dart (right):

https://codereview.chromium.org/124833003/diff/320001/pkg/http_server/lib/src...
pkg/http_server/lib/src/virtual_directory.dart:300: .whenComplete(() {
On 2014/01/09 05:27:48, sethladd wrote:
> can this be a one-line function?

Done.

https://codereview.chromium.org/124833003/diff/320001/pkg/http_server/lib/src...
pkg/http_server/lib/src/virtual_directory.dart:301: return response.close();
On 2014/01/09 06:12:23, Anders Johnsen wrote:
> Why return here? This value is not kept.

Done.

https://codereview.chromium.org/124833003/diff/320001/pkg/http_server/test/ht...
File pkg/http_server/test/http_mock.dart (right):

https://codereview.chromium.org/124833003/diff/320001/pkg/http_server/test/ht...
pkg/http_server/test/http_mock.dart:1: library htt_mock;
On 2014/01/09 05:27:48, sethladd wrote:
> is this a candidate to be its own package?

YES!

angular and route_hierarchial already have mocks of these. Should unify.

https://codereview.chromium.org/124833003/diff/320001/pkg/http_server/test/ht...
pkg/http_server/test/http_mock.dart:1: library htt_mock;
On 2014/01/09 06:12:23, Anders Johnsen wrote:
> http_mock?

Done.

https://codereview.chromium.org/124833003/diff/320001/pkg/http_server/test/ut...
File pkg/http_server/test/utils.dart (right):

https://codereview.chromium.org/124833003/diff/320001/pkg/http_server/test/ut...
pkg/http_server/test/utils.dart:96: if(_testLogicRequestExpando[currentTestCase]
== true) {
On 2014/01/09 06:12:23, Anders Johnsen wrote:
> remove '== true'

Done.

https://codereview.chromium.org/124833003/diff/320001/pkg/http_server/test/ut...
pkg/http_server/test/utils.dart:115:
if(_testLogicRequestExpando[currentTestCase] == true) {
On 2014/01/09 06:12:23, Anders Johnsen wrote:
> remove '== true'.

Done.

[kevmoo, 2014-01-10 18:12:31]

PTAL

https://codereview.chromium.org/124833003/diff/320001/pkg/http_server/lib/src...
File pkg/http_server/lib/src/virtual_directory.dart (right):

https://codereview.chromium.org/124833003/diff/320001/pkg/http_server/lib/src...
pkg/http_server/lib/src/virtual_directory.dart:285: dir.list(followLinks:
true).forEach((entity) {
I think I've addressed this, with TODOs for logging errors

https://codereview.chromium.org/124833003/diff/320001/pkg/http_server/test/ut...
File pkg/http_server/test/utils.dart (right):

https://codereview.chromium.org/124833003/diff/320001/pkg/http_server/test/ut...
pkg/http_server/test/utils.dart:31: _testLogicRequestExpando[currentTestCase] =
true;
On 2014/01/09 06:12:23, Anders Johnsen wrote:
> This stuff really needs some documentation. What is the goal of doing this?

Done.

[sethladd, 2014-01-10 18:19:35]

https://codereview.chromium.org/124833003/diff/560001/pkg/http_server/test/ht...
File pkg/http_server/test/http_mock.dart (right):

https://codereview.chromium.org/124833003/diff/560001/pkg/http_server/test/ht...
pkg/http_server/test/http_mock.dart:1: library http_mock;
Can you pull this out and put it into its own package? even if it's in the SDK,
we should make this easier to find and use outside of this package.

[Anders Johnsen, 2014-01-10 18:19:45]

lgtm

https://codereview.chromium.org/124833003/diff/560001/pkg/http_server/lib/src...
File pkg/http_server/lib/src/virtual_directory.dart (right):

https://codereview.chromium.org/124833003/diff/560001/pkg/http_server/lib/src...
pkg/http_server/lib/src/virtual_directory.dart:303: .handleError((error) {
Why is this better than the old version. Its longer and harder to  follow. Also,
please remove the old todo.

[kevmoo, 2014-01-10 18:51:24]

PTAL

https://codereview.chromium.org/124833003/diff/560001/pkg/http_server/lib/src...
File pkg/http_server/lib/src/virtual_directory.dart (right):

https://codereview.chromium.org/124833003/diff/560001/pkg/http_server/lib/src...
pkg/http_server/lib/src/virtual_directory.dart:303: .handleError((error) {
On 2014/01/10 18:19:45, Anders Johnsen wrote:
> Why is this better than the old version. Its longer and harder to  follow.
Also,
> please remove the old todo.

Went back to old impl...it was just personal preference.

Can the comment still be removed?

https://codereview.chromium.org/124833003/diff/560001/pkg/http_server/test/ht...
File pkg/http_server/test/http_mock.dart (right):

https://codereview.chromium.org/124833003/diff/560001/pkg/http_server/test/ht...
pkg/http_server/test/http_mock.dart:1: library http_mock;
Tracked for a separate CL https://code.google.com/p/dart/issues/detail?id=16009

On 2014/01/10 18:19:36, sethladd wrote:
> Can you pull this out and put it into its own package? even if it's in the
SDK,
> we should make this easier to find and use outside of this package.

[Anders Johnsen, 2014-01-10 18:53:34]

https://codereview.chromium.org/124833003/diff/560001/pkg/http_server/lib/src...
File pkg/http_server/lib/src/virtual_directory.dart (right):

https://codereview.chromium.org/124833003/diff/560001/pkg/http_server/lib/src...
pkg/http_server/lib/src/virtual_directory.dart:303: .handleError((error) {
On 2014/01/10 18:51:24, kevmoo wrote:
> On 2014/01/10 18:19:45, Anders Johnsen wrote:
> > Why is this better than the old version. Its longer and harder to  follow.
> Also,
> > please remove the old todo.
> 
> Went back to old impl...it was just personal preference.
> 
> Can the comment still be removed?

Yep :)

[kevmoo, 2014-01-10 19:27:11]

Committed patchset #16 manually as r31701 (presubmit successful).