Fixes for forgotten review comments.

pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_impl.dart

Index: pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_impl.dart
diff --git a/pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_impl.dart b/pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_impl.dart
index 1898ad1b5fe6314baf7287df597c96d764a52056..2ee0ae23920745dfc1602f759d0dd875d52b91d9 100644
--- a/pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_impl.dart
+++ b/pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_impl.dart
@@ -28,19 +28,21 @@ import 'utils.dart';
 void runServerApplication(String targetPath, String outPath) {
   var targetFolder = pathos.dirname(targetPath);
   var targetName = pathos.basename(targetPath);
-  new CoverageServer(targetFolder, targetPath, outPath)
-      .start()
+  new CoverageServer(targetFolder, targetPath, outPath).start()
       .then((port) {
         var options = new Options();
         var targetArgs = ['http://127.0.0.1:$port/$targetName'];
         var dartExecutable = options.executable;
-        Process.start(dartExecutable, targetArgs).then((Process process) {
-          process.exitCode.then(exit);
-          // Redirect process streams.
-          stdin.pipe(process.stdin);
-          process.stdout.pipe(stdout);
-          process.stderr.pipe(stderr);
-        });
+        return Process.start(dartExecutable, targetArgs)
+            .then((Process process) {

[Bob Nystrom, 2013/06/18 15:58:25]

If you want, you can move this .then() call one level up. We tend to do that to
flatten out a series of async operations, like:

.then((port) {
  ...
  return Process.start(...);
}).then((process) {
  ...
  return process.exitCode;
}).catchError((e) {
  ...
});

The semantics are the same either way, but this way it's a little less nested.

[scheglov, 2013/06/18 16:36:11]

Done.

+              process.exitCode.then(exit);
+              // Redirect process streams.
+              stdin.pipe(process.stdin);
+              process.stdout.pipe(stdout);
+              process.stderr.pipe(stderr);
+            });
+      }).catchError((e) {
+        log.severe('Error starting $targetPath. $e');
       });
 }
 
@@ -69,13 +71,12 @@ abstract class RewriteServer {
     });
   }
 
-  handlePostRequest(HttpRequest request);
+  void handlePostRequest(HttpRequest request);
 
-  handleGetRequest(HttpRequest request) {
+  void handleGetRequest(HttpRequest request) {
     var response = request.response;
     // Prepare path.
-    var path = basePath + '/' + request.uri.path;
-    path = pathos.normalize(path);
+    var path = getFilePath(request.uri);
     log.info('[$path] Requested.');
     // May be serve using just path.
     {
@@ -94,30 +95,43 @@ abstract class RewriteServer {
       if (found) {
         // May be this files should be sent as is.
         if (!shouldRewriteFile(path)) {
-          sendFile(request, file);
-          return;
+          return sendFile(request, file);
         }
         // Rewrite content of the file.
-        file.readAsString().then((content) {
+        return file.readAsString().then((content) {
           log.finest('[$path] Done reading ${content.length} characters.');
           content = rewriteFileContent(path, content);
           log.fine('[$path] Rewritten.');
           response.write(content);
-          response.close();
+          return response.close();
         });
       } else {
         log.severe('[$path] File not found.');
         response.statusCode = HttpStatus.NOT_FOUND;
-        response.close();
+        return response.close();
       }
+    }).catchError((e) {
+      log.severe('[$path] $e.');
+      response.statusCode = HttpStatus.INTERNAL_SERVER_ERROR;
+      return response.close();
     });
   }
 
-  void sendFile(HttpRequest request, File file) {
+  String getFilePath(Uri uri) {
+    var path = uri.path;
+    // URIs usually have leading '/'.
+    if (path.startsWith('/')) path = path.substring(1);
+    // Convert from URI to system.
+    var parts = new pathos.Builder(style: pathos.Style.url).split(path);
+    path = pathos.joinAll(parts);

[Bob Nystrom, 2013/06/18 15:58:25]

The "Builder" name is a bit confusing in pathos, but you should use the same
builder object for these method calls too. Basically, anywhere you're doing
"pathos." use the builder instead. That way you control the style (URL) that it
uses.

[scheglov, 2013/06/18 16:36:11]

Hm...
I use URL style to convert URI to path parts.
But after this, I want to work with file system paths, so this is why I use
"pathos" functions.
Actually... I don't need even Builder to split the URI path, it is always '/',
right?
Or, even better - use Uri.pathSegments :-)

[Bob Nystrom, 2013/06/18 17:22:27]

Oh, sorry, right. I wasn't reading the code closely.
Yes! :)

+    // Prepend with base path.
+    path = pathos.join(basePath, path);
+    return pathos.normalize(path);
+  }
+
+  Future sendFile(HttpRequest request, File file) {
     file.fullPath().then((fullPath) {
-      file.openRead()
-        .pipe(request.response)
-        .catchError((e) {});
+      return file.openRead().pipe(request.response);
     });
   }
 
@@ -134,6 +148,18 @@ abstract class RewriteServer {
 }
 
 
+/// Here `CCC` means 'code coverage configuration'.
+const TEST_UNIT_CCC = '''
+class __CCC extends __cc_ut.Configuration {
+  void onDone(bool success) {
+    __cc.postStatistics();
+    super.onDone(success);
+  }
+}''';
+
+const TEST_UNIT_CCC_SET = '__cc_ut.unittestConfiguration = new __CCC();';
+
+
 /// Server that rewrites Dart code so that it reports execution of statements
 /// and other nodes.
 class CoverageServer extends RewriteServer {
@@ -160,11 +186,14 @@ class CoverageServer extends RewriteServer {
       }
     }).onDone(() {
       log.fine('Received all statistics.');
-      var sb = new StringBuffer();
-      appInfo.write(sb, executedIds);
-      new File(outPath).writeAsString(sb.toString());
-      log.fine('Results are written to $outPath.');
-      request.response.close();
+      var buffer = new StringBuffer();
+      appInfo.write(buffer, executedIds);
+      new File(outPath).writeAsString(buffer.toString()).then((_) {
+        return request.response.close();
+      }).catchError((e) {
+        log.severe('Error in receiving statistics $e.');
+        return request.response.close();
+      });
     });
   }
 
@@ -175,8 +204,7 @@ class CoverageServer extends RewriteServer {
           '..', 'lib', 'src', 'services', 'runtime', 'coverage',
           'coverage_lib.dart']);
       var content = new File(implPath).readAsStringSync();
-      content = content.replaceAll('0; // replaced during rewrite', '$port;');
-      return content;
+      return content.replaceAll('0; // replaced during rewrite', '$port;');
     }
     return null;
   }
@@ -188,10 +216,7 @@ class CoverageServer extends RewriteServer {
       return true;
     }
     // TODO(scheglov) use configuration
-    if (path.contains('/packages/analyzer_experimental/')) {
-      return true;
-    }
-    return false;
+    return path.contains('/packages/analyzer_experimental/');
   }
 
   String rewriteFileContent(String path, String code) {
@@ -212,16 +237,8 @@ class CoverageServer extends RewriteServer {
         if (node is FunctionDeclaration) {
           var body = node.functionExpression.body;
           if (node.name.name == 'main' && body is BlockFunctionBody) {
-            injector.inject(node.offset,
-                'class __CCC extends __cc_ut.Configuration {'
-                '  void onDone(bool success) {'
-                '    __cc.postStatistics();'
-                '    super.onDone(success);'
-                '  }'
-                '}');
-            injector.inject(
-                body.offset + 1,
-                '__cc_ut.unittestConfiguration = new __CCC();');
+            injector.inject(node.offset, TEST_UNIT_CCC);
+            injector.inject(body.offset + 1, TEST_UNIT_CCC_SET);
           }
         }
       }