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

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

R=sigmund@google.com

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

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

very nice! Some suggestions below, mainly minor code-style nits.

https://codereview.chromium.org/372153002/diff/1/pkg/source_maps/lib/builder....
File pkg/source_maps/lib/builder.dart (right):

https://codereview.chromium.org/372153002/diff/1/pkg/source_maps/lib/builder....
pkg/source_maps/lib/builder.dart:20: // TODO(tjblasi): This is clearly bad.
see the suggestion in SingleMapping, maybe that helps :)?

https://codereview.chromium.org/372153002/diff/1/pkg/source_maps/lib/parser.dart
File pkg/source_maps/lib/parser.dart (right):

https://codereview.chromium.org/372153002/diff/1/pkg/source_maps/lib/parser.d...
pkg/source_maps/lib/parser.dart:136: // TODO(sigmund): integrate mapping and
sourcemap builder?
delete TODO :-)

https://codereview.chromium.org/372153002/diff/1/pkg/source_maps/lib/parser.d...
pkg/source_maps/lib/parser.dart:152: factory
SingleMapping.fromBuilder(SourceMapBuilder builder, [String fileUrl]) {
how about switching to take a list of entries instead of a builder?

https://codereview.chromium.org/372153002/diff/1/pkg/source_maps/lib/parser.d...
pkg/source_maps/lib/parser.dart:154: List<TargetLineEntry> lines = [];
style nit: use 'var' for local variables

https://codereview.chromium.org/372153002/diff/1/pkg/source_maps/lib/parser.d...
pkg/source_maps/lib/parser.dart:158: Map<String, int> urls = new
LinkedHashMap<String, int>();
same here and below (var urls = ...)

https://codereview.chromium.org/372153002/diff/1/pkg/source_maps/lib/parser.d...
pkg/source_maps/lib/parser.dart:165: sourceEntries.sort();
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.

https://codereview.chromium.org/372153002/diff/1/pkg/source_maps/lib/parser.d...
pkg/source_maps/lib/parser.dart:172: lines.last.entries.add(new
TargetEntry(sourceEntry.target.column));
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.

https://codereview.chromium.org/372153002/diff/1/pkg/source_maps/lib/parser.d...
pkg/source_maps/lib/parser.dart:174: var urlId =
urls.putIfAbsent(sourceEntry.source.sourceUrl, () => urls.length);
nit: 80 columns (here and elsewhere)

https://codereview.chromium.org/372153002/diff/1/pkg/source_maps/lib/parser.d...
pkg/source_maps/lib/parser.dart:175: var srcNameId = sourceEntry.identifierName
!= null ?
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);

https://codereview.chromium.org/372153002/diff/1/pkg/source_maps/lib/parser.d...
pkg/source_maps/lib/parser.dart:186: mapping.lines.addAll(lines);
nit: maybe move the default initialization above to the fromJson constructor,
and add 'lines' also as an argument for the _internal constructor?

https://codereview.chromium.org/372153002/diff/1/pkg/source_maps/lib/parser.d...
pkg/source_maps/lib/parser.dart:261: Map toMap() {
suggestion: maybe rename as toJson (to make it contrast the 'fromJson'
constructor)

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

Thanks for the review!

https://codereview.chromium.org/372153002/diff/1/pkg/source_maps/lib/builder....
File pkg/source_maps/lib/builder.dart (right):

https://codereview.chromium.org/372153002/diff/1/pkg/source_maps/lib/builder....
pkg/source_maps/lib/builder.dart:20: // TODO(tjblasi): This is clearly bad.
On 2014/07/08 00:31:00, Siggi Cherem (dart-lang) wrote:
> see the suggestion in SingleMapping, maybe that helps :)?

Done.

https://codereview.chromium.org/372153002/diff/1/pkg/source_maps/lib/parser.dart
File pkg/source_maps/lib/parser.dart (right):

https://codereview.chromium.org/372153002/diff/1/pkg/source_maps/lib/parser.d...
pkg/source_maps/lib/parser.dart:136: // TODO(sigmund): integrate mapping and
sourcemap builder?
On 2014/07/08 00:31:00, Siggi Cherem (dart-lang) wrote:
> delete TODO :-)

Done.

https://codereview.chromium.org/372153002/diff/1/pkg/source_maps/lib/parser.d...
pkg/source_maps/lib/parser.dart:152: factory
SingleMapping.fromBuilder(SourceMapBuilder builder, [String fileUrl]) {
On 2014/07/08 00:31:01, Siggi Cherem (dart-lang) wrote:
> how about switching to take a list of entries instead of a builder?

Done.

https://codereview.chromium.org/372153002/diff/1/pkg/source_maps/lib/parser.d...
pkg/source_maps/lib/parser.dart:154: List<TargetLineEntry> lines = [];
On 2014/07/08 00:31:01, Siggi Cherem (dart-lang) wrote:
> style nit: use 'var' for local variables

Done.

https://codereview.chromium.org/372153002/diff/1/pkg/source_maps/lib/parser.d...
pkg/source_maps/lib/parser.dart:158: Map<String, int> urls = new
LinkedHashMap<String, int>();
On 2014/07/08 00:31:00, Siggi Cherem (dart-lang) wrote:
> same here and below (var urls = ...)

Done.

https://codereview.chromium.org/372153002/diff/1/pkg/source_maps/lib/parser.d...
pkg/source_maps/lib/parser.dart:165: sourceEntries.sort();
On 2014/07/08 00:31:00, Siggi Cherem (dart-lang) wrote:
> 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.

Done.

https://codereview.chromium.org/372153002/diff/1/pkg/source_maps/lib/parser.d...
pkg/source_maps/lib/parser.dart:172: lines.last.entries.add(new
TargetEntry(sourceEntry.target.column));
On 2014/07/08 00:31:01, Siggi Cherem (dart-lang) wrote:
> 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.

Done.

https://codereview.chromium.org/372153002/diff/1/pkg/source_maps/lib/parser.d...
pkg/source_maps/lib/parser.dart:174: var urlId =
urls.putIfAbsent(sourceEntry.source.sourceUrl, () => urls.length);
On 2014/07/08 00:31:00, Siggi Cherem (dart-lang) wrote:
> nit: 80 columns (here and elsewhere)

Done.

https://codereview.chromium.org/372153002/diff/1/pkg/source_maps/lib/parser.d...
pkg/source_maps/lib/parser.dart:175: var srcNameId = sourceEntry.identifierName
!= null ?
On 2014/07/08 00:31:01, Siggi Cherem (dart-lang) wrote:
> 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);

Done.

https://codereview.chromium.org/372153002/diff/1/pkg/source_maps/lib/parser.d...
pkg/source_maps/lib/parser.dart:186: mapping.lines.addAll(lines);
On 2014/07/08 00:31:01, Siggi Cherem (dart-lang) wrote:
> nit: maybe move the default initialization above to the fromJson constructor,
> and add 'lines' also as an argument for the _internal constructor?

Done.

https://codereview.chromium.org/372153002/diff/1/pkg/source_maps/lib/parser.d...
pkg/source_maps/lib/parser.dart:261: Map toMap() {
On 2014/07/08 00:31:00, Siggi Cherem (dart-lang) wrote:
> suggestion: maybe rename as toJson (to make it contrast the 'fromJson'
> constructor)

Done.

[Siggi Cherem (dart-lang), 2014-07-08 20:35:44]

awesome, lgtm!

[tjblasi, 2014-07-08 22:14:08]

Committed patchset #2 manually as r38073 (presubmit successful).