Moving logic for writing source maps from SourceMapBuilder into the Mapping class.

pkg/source_maps/lib/parser.dart

Index: pkg/source_maps/lib/parser.dart
diff --git a/pkg/source_maps/lib/parser.dart b/pkg/source_maps/lib/parser.dart
index c92a8bba0ba917699ae7ffa516f2c1740db296ef..9f70d23a3c537c3a1fec05e0f1e9b8f9b3bd29c8 100644
--- a/pkg/source_maps/lib/parser.dart
+++ b/pkg/source_maps/lib/parser.dart
@@ -5,8 +5,10 @@
 /// Contains the top-level function to parse source maps version 3.
 library source_maps.parser;
 
+import 'dart:collection';
 import 'dart:convert';
 
+import 'builder.dart';
 import 'span.dart';
 import 'src/utils.dart';
 import 'src/vlq.dart';
@@ -41,7 +43,7 @@ Mapping parseJson(Map map, {Map<String, Map> otherMaps}) {
 }
 
 
-/// A mapping parsed our of a source map.
+/// A mapping parsed out of a source map.
 abstract class Mapping {
   Span spanFor(int line, int column, {Map<String, SourceFile> files});
 
@@ -145,6 +147,46 @@ class SingleMapping extends Mapping {
   /// Entries indicating the beginning of each span.
   final List<TargetLineEntry> lines = <TargetLineEntry>[];
 
+  SingleMapping._internal(this.targetUrl, this.urls, this.names);
+
+  factory SingleMapping.fromBuilder(SourceMapBuilder builder, [String fileUrl]) {

[Siggi Cherem (dart-lang), 2014/07/08 00:31:01]

how about switching to take a list of entries instead of a builder?

[tjblasi, 2014/07/08 16:36:20]

Done.

+    List<Entry> sourceEntries = builder.entries;
+    List<TargetLineEntry> lines = [];

[Siggi Cherem (dart-lang), 2014/07/08 00:31:01]

style nit: use 'var' for local variables

[tjblasi, 2014/07/08 16:36:20]

Done.

+
+    // Indices associated with file urls that will be part of the source map. We
+    // use a linked hash-map so that `_urls.keys[_urls[u]] == u`
+    Map<String, int> urls = new LinkedHashMap<String, int>();

[Siggi Cherem (dart-lang), 2014/07/08 00:31:00]

same here and below (var urls = ...)

[tjblasi, 2014/07/08 16:36:20]

Done.

+
+    // Indices associated with identifiers that will be part of the source map.
+    // We use a linked hash-map so that `_names.keys[_names[n]] == n`
+    Map<String, int> names = new LinkedHashMap<String, int>();
+
+    // The encoding needs to be sorted by the target offsets.
+    sourceEntries.sort();

[Siggi Cherem (dart-lang), 2014/07/08 00:31:00]

since sorting modifies the list in-place, we might want to make a copy of the
list at the beginning. I think something like:
  entries = entries.toList();

will do.

[tjblasi, 2014/07/08 16:36:20]

Done.

+    for (var sourceEntry in sourceEntries) {
+      if (lines.isEmpty || sourceEntry.target.line > lines.last.line) {
+        lines.add(new TargetLineEntry(sourceEntry.target.line, []));
+      }
+
+      if (sourceEntry.source == null) {
+        lines.last.entries.add(new TargetEntry(sourceEntry.target.column));

[Siggi Cherem (dart-lang), 2014/07/08 00:31:01]

since `lines.last` can be expensive, maybe we could follow a similar pattern as
the `fromJson` constructor and have a couple variables around for these commonly
accessed properties.

For example, I'd consider having `line` to track the current line and a list of
`entries` of the current line, both of which can be reset above when you
determine that the target.line changed.

[tjblasi, 2014/07/08 16:36:20]

Done.

+      } else {
+        var urlId = urls.putIfAbsent(sourceEntry.source.sourceUrl, () => urls.length);

[Siggi Cherem (dart-lang), 2014/07/08 00:31:00]

nit: 80 columns (here and elsewhere)

[tjblasi, 2014/07/08 16:36:20]

Done.

+        var srcNameId = sourceEntry.identifierName != null ?

[Siggi Cherem (dart-lang), 2014/07/08 00:31:01]

minor nit: maybe swapping this branch might make it a bit easier to read this
expression.

var srcNameId = sourceEntry.identifierName == null ? null :
    names.putIfAbsent(sourceEntry.identifierName, () => names.length);

Unlike other operators, when it helps I wrap before the ? : operators. Not sure
if matters here though:

var srcNameId = sourceEntry.identifierName == null
    ? null
    : names.putIfAbsent(sourceEntry.identifierName, () => names.length);

[tjblasi, 2014/07/08 16:36:20]

Done.

+            names.putIfAbsent(sourceEntry.identifierName, () => names.length) : null;
+        lines.last.entries.add(new TargetEntry(
+            sourceEntry.target.column,
+            urlId,
+            sourceEntry.source.line,
+            sourceEntry.source.column,
+            srcNameId));
+      }
+    }
+    var mapping = new SingleMapping._internal(fileUrl, urls.keys.toList(), names.keys.toList());
+    mapping.lines.addAll(lines);

[Siggi Cherem (dart-lang), 2014/07/08 00:31:01]

nit: maybe move the default initialization above to the fromJson constructor,
and add 'lines' also as an argument for the _internal constructor?

[tjblasi, 2014/07/08 16:36:20]

Done.

+    return mapping;
+  }
+
   SingleMapping.fromJson(Map map)
       : targetUrl = map['file'],
         // TODO(sigmund): add support for 'sourceRoot'
@@ -215,6 +257,66 @@ class SingleMapping extends Mapping {
     }
   }
 
+  /// Encodes the Mapping mappings as a json map.
+  Map toMap() {

[Siggi Cherem (dart-lang), 2014/07/08 00:31:00]

suggestion: maybe rename as toJson (to make it contrast the 'fromJson'
constructor)

[tjblasi, 2014/07/08 16:36:20]

Done.

+    var buff = new StringBuffer();
+    var line = 0;
+    var column = 0;
+    var srcLine = 0;
+    var srcColumn = 0;
+    var srcUrlId = 0;
+    var srcNameId = 0;
+    var first = true;
+
+    for (var entry in lines) {
+      int nextLine = entry.line;
+      if (nextLine > line) {
+        for (int i = line; i < nextLine; ++i) {
+          buff.write(';');
+        }
+        line = nextLine;
+        column = 0;
+        first = true;
+      }
+
+      for (var segment in entry.entries) {
+        if (!first) buff.write(',');
+        first = false;
+        column = _append(buff, column, segment.column);
+
+        // Encoding can be just the column offset if there is no source
+        // information.
+        var newUrlId = segment.sourceUrlId;
+        if (newUrlId == null) continue;
+        srcUrlId = _append(buff, srcUrlId, newUrlId);
+        srcLine = _append(buff, srcLine, segment.sourceLine);
+        srcColumn = _append(buff, srcColumn, segment.sourceColumn);
+
+        if (segment.sourceNameId == null) continue;
+        srcNameId = _append(buff, srcNameId, segment.sourceNameId);
+      }
+    }
+
+    var result = {
+      'version': 3,
+      'sourceRoot': '',
+      'sources': urls,
+      'names' : names,
+      'mappings' : buff.toString()
+    };
+    if (targetUrl != null) {
+      result['file'] = targetUrl;
+    }
+    return result;
+  }
+
+  /// Appends to [buff] a VLQ encoding of [newValue] using the difference
+  /// between [oldValue] and [newValue]
+  static int _append(StringBuffer buff, int oldValue, int newValue) {
+    buff.writeAll(encodeVlq(newValue - oldValue));
+    return newValue;
+  }
+
   _segmentError(int seen, int line) => new StateError(
       'Invalid entry in sourcemap, expected 1, 4, or 5'
       ' values, but got $seen.\ntargeturl: $targetUrl, line: $line');