Fixes for forgotten review comments.

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

 // Copyright (c) 2013, 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.
 
 /// A library for code coverage support for Dart.
 library runtime.coverage.impl;
 
 import 'dart:async';
 import 'dart:collection' show SplayTreeMap;
 import 'dart:io';
@@ skipping 10 matching lines @@
 import '../log.dart' as log;
 import 'models.dart';
 import 'utils.dart';
 
 /// Run the [targetPath] with code coverage rewriting.
 /// Redirects stdandard process streams.
 /// On process exit dumps coverage statistics into the [outPath].
 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');
       });
 }
 
 
 /// Abstract server to listen requests and serve files, may be rewriting them.
 abstract class RewriteServer {
   final String basePath;
   int port;
 
   RewriteServer(this.basePath);
 
   /// Runs the HTTP server on the ephemeral port and returns [Future] with it.
   Future<int> start() {
     return HttpServer.bind('127.0.0.1', 0).then((server) {
       port = server.port;
       log.info('RewriteServer is listening at: $port.');
       server.listen((request) {
         if (request.method == 'GET') {
           handleGetRequest(request);
         }
         if (request.method == 'POST') {
           handlePostRequest(request);
         }
       });
       return port;
     });
   }
 
-  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.
     {
       var content = rewritePathContent(path);
       if (content != null) {
         log.info('[$path] Request served by path.');
         response.write(content);
         response.close();
         return;
       }
     }
     // Serve from file.
     log.info('[$path] Serving file.');
     var file = new File(path);
     file.exists().then((found) {
       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);
     });
   }
 
   bool shouldRewriteFile(String path);
 
   /// Subclasses implement this method to rewrite the provided [code] of the
   /// file with [path]. Returns some content or `null` if file content
   /// should be requested.
   String rewritePathContent(String path);
 
   /// Subclasses implement this method to rewrite the provided [code] of the
   /// file with [path].
   String rewriteFileContent(String path, String code);
 }
 
 
+/// 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 {
   final appInfo = new AppInfo();
   final String targetPath;
   final String outPath;
 
   CoverageServer(String basePath, this.targetPath, this.outPath)
       : super(basePath);
 
   void handlePostRequest(HttpRequest request) {
     var id = 0;
     var executedIds = new Set<int>();
     request.listen((data) {
       log.fine('Received statistics, ${data.length} bytes.');
       while (true) {
         var listIndex = id ~/ 8;
         if (listIndex >= data.length) break;
         var bitIndex = id % 8;
         if ((data[listIndex] & (1 << bitIndex)) != 0) {
           executedIds.add(id);
         }
         id++;
       }
     }).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();
+      });
     });
   }
 
   String rewritePathContent(String path) {
     if (path.endsWith('__coverage_lib.dart')) {
       String implPath = pathos.joinAll([
           pathos.dirname(new Options().script),
           '..', '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;
   }
 
   bool shouldRewriteFile(String path) {
     if (pathos.extension(path).toLowerCase() != '.dart') return false;
     // Rewrite target itself, only to send statistics.
     if (path == targetPath) {
       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) {
     var unit = _parseCode(code);
     log.finest('[$path] Parsed.');
     var injector = new CodeInjector(code);
     // Inject imports.
     var directives = unit.directives;
     if (directives.isNotEmpty && directives[0] is LibraryDirective) {
       injector.inject(directives[0].end,
           'import "package:unittest/unittest.dart" as __cc_ut;'
           'import "http://127.0.0.1:$port/__coverage_lib.dart" as __cc;');
     }
     // Inject statistics sender.
     var isTargetScript = path == targetPath;
     if (isTargetScript) {
       for (var node in unit.declarations) {
         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);
           }
         }
       }
     }
     // Inject touch() invocations.
     if (!isTargetScript) {
       appInfo.enterUnit(path, code);
       unit.accept(new InsertTouchInvocationsVisitor(appInfo, injector));
     }
     // Done.
@@ skipping 88 matching lines @@
     var lastOffset = 0;
     offsetFragmentMap.forEach((offset, fragment) {
       sb.write(_code.substring(lastOffset, offset));
       sb.write(fragment);
       lastOffset = offset;
     });
     sb.write(_code.substring(lastOffset, _code.length));
     return sb.toString();
   }
 }