pkg/vm_service_client: add getSourceReport to VMServiceReference

lib/src/source_report.dart

Index: lib/src/source_report.dart
diff --git a/lib/src/source_report.dart b/lib/src/source_report.dart
new file mode 100644
index 0000000000000000000000000000000000000000..8e961a9d0b7be6e5b42b7bcb213059f9b4565060
--- /dev/null
+++ b/lib/src/source_report.dart
@@ -0,0 +1,107 @@
+// Copyright (c) 2016, 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.
+
+import 'script.dart';
+import 'scope.dart';
+
+VMSourceReport newSourceReport(Scope scope, Map json) {
+  if (json == null) return null;
+  assert(json["type"] == "SourceReport");
+  return new VMSourceReport._(scope, json);
+}
+
+VMSourceReportCoverage newSourceReportCoverage(Map json) {
+  if (json == null) return null;
+  return new VMSourceReportCoverage._(json);
+}
+
+VMSourceReportRange newSourceReportRange(Map json) {
+  if (json == null) return null;
+  return new VMSourceReportRange._(json);
+}
+
+/// Represents a set of reports tied to source locations in an isolate.

[nweiz, 2016/04/29 00:52:44]

This should be a noun phrase rather than "Represents ..."
(https://www.dartlang.org/effective-dart/documentation/#prefer-starting-librar...).

As before, it's weird here that we're using the plural "reports" to decide the
contents of a single object called a "report". Maybe something like "A report
composed of coverage information about ranges of source code."

[kevmoo, 2016/04/29 23:36:55]

Done.

+class VMSourceReport {
+  /// A list of ranges in the program source.  These ranges correspond
+  /// to ranges of executable code in the user's program (functions,
+  /// methods, constructors, etc.)

[nweiz, 2016/04/29 00:52:43]

The first paragraph of a doc comment should always be one sentence long
(https://www.dartlang.org/effective-dart/documentation/#do-make-the-first-sent...).
Also avoid abbreviations like "etc"
(https://www.dartlang.org/effective-dart/documentation/#avoid-abbreviations-an...).

Maybe something like "Ranges in the program's source, corresponding to
executable chunks of code such as functions and methods."

[kevmoo, 2016/04/29 23:36:54]

Done.

+  ///
+  /// Note that ranges may nest in other ranges, in the case of nested
+  /// functions.
+  ///
+  /// Note that ranges may be duplicated, in the case of mixins.

[nweiz, 2016/04/29 00:52:44]

Somewhere in here you should mention what information is attached to the ranges.
Otherwise a user reading this won't have a clear understanding of the purpose of
this class or where its information is exposed.

[kevmoo, 2016/04/29 23:36:55]

Done.

[nweiz, 2016/05/04 23:05:51]

I don't see this.

[kevmoo, 2016/05/05 20:46:25]

See latest patch

+  final List<VMSourceReportRange> ranges;
+
+  /// A list of scripts, referenced by index in the report's ranges.
+  final List<VMScriptRef> scripts;
+
+  VMSourceReport._(scope, Map json)
+      : scripts = (json['scripts'] as List)
+            .map((scriptItem) => newVMScriptRef(scope, scriptItem))
+            .toList(),

[nweiz, 2016/04/29 00:52:44]

These should be unmodifiable lists. See other classes' constructors for
examples.

Nit: it's a little confusing that the initializers are in the opposite order as
the field declarations.

[kevmoo, 2016/04/29 23:36:54]

Done.

+        ranges = (json['ranges'] as List)
+            .map((scriptItem) => newSourceReportRange(scriptItem))
+            .toList();
+}
+
+/// Represents a range of executable code (function, method, constructor, etc)
+/// in the running program.
+///
+/// It is part of a [VMSourceReport].

[nweiz, 2016/04/29 00:52:43]

"It" -> "This" or "Each range"

[kevmoo, 2016/04/29 23:36:54]

Done.

+class VMSourceReportRange {
+  /// An index into the script table of the SourceReport, indicating
+  /// which script contains this range of code.
+  final int scriptIndex;

[nweiz, 2016/04/29 00:52:43]

Automatically cross-reference this. Get rid of VMSourceReport.scripts and
replace this with a VMScriptRef.

[kevmoo, 2016/04/29 23:36:54]

Done.

+
+  /// The token position at which this range begins.
+  final int startPos;
+
+  /// The token position at which this range ends.  Inclusive.
+  final int endPos;

[nweiz, 2016/04/29 00:52:44]

Instead of having raw start and end positions, expose a VMSourceLocation. Then
you can also make VMSourceReportRange.script a getter that just returns
location.script.

[kevmoo, 2016/04/29 23:36:54]

Done.

+
+  /// Has this range been compiled by the Dart VM?

[nweiz, 2016/04/29 00:52:43]

Don't phrase documentation as questions.

Explain what this means, what information will be unavailable if it's not
compiled, and how to force it to be compiled.

[kevmoo, 2016/04/29 23:36:54]

Done.

+  final bool compiled;
+
+  /// Code coverage information for this range.
+  ///
+  /// Provided only when the
+  /// `includeCoverageReport` is true and the range has been

[nweiz, 2016/04/29 00:52:44]

It's not clear what "includeCoverageReport" is referring to here. Explicitly say
"the `includeCoverageReport` parameter to [VMIsolate.getSourceReport]".

[kevmoo, 2016/04/29 23:36:54]

Done - did not refer to just the isolate method since there is now also a method
on script

+  /// compiled.
+  final VMSourceReportCoverage coverage;

[nweiz, 2016/04/29 00:52:43]

I don't know if there's a lot of value in requiring another layer of indirection
to get at the coverage information. It would probably be easier for the user if
we just included hits and misses directly on the range, and nulled them out if
the coverage information wasn't available.

[kevmoo, 2016/04/29 23:36:54]

Done.

+
+  /// Possible breakpoint information for this range, represented as a

[nweiz, 2016/04/29 00:52:44]

"Possible breakpoint information" isn't very informative. Maybe something like
"Token locations at which breakpoints can be set".

[kevmoo, 2016/04/29 23:36:54]

Done.

+  /// sorted list of token positions.
+  ///
+  /// Provided only when the when the
+  /// `includePossibleBreakpoints` is `true` and the range has been
+  /// compiled.
+  final List<int> possibleBreakpoints;

[nweiz, 2016/04/29 00:52:44]

Make this a list of VMScriptTokens. Same with hits and misses.

[kevmoo, 2016/04/29 23:36:54]

Done.

+
+  VMSourceReportRange._(Map json)
+      : scriptIndex = json['scriptIndex'],
+        endPos = json['endPos'],
+        startPos = json['startPos'],
+        compiled = json['compiled'],
+        coverage = newSourceReportCoverage(json['coverage']),
+        possibleBreakpoints = json['possibleBreakpoints'];
+}
+
+/// Represents coverage information for one [VMSourceReportRange].
+class VMSourceReportCoverage {
+  /// A list of token positions in a SourceReportRange which have been
+  /// executed.
+  ///
+  /// The list is sorted.

[nweiz, 2016/04/29 00:52:44]

Sorted by what?

[kevmoo, 2016/04/29 23:36:54]

for ints, I think that's implicit. 

[nweiz, 2016/05/04 23:05:51]

It's certainly not obvious whether it's ascending or descending. A better way to
describe it would be to say "sorted in the order the tokens appear in the source
text".

[kevmoo, 2016/05/05 20:46:25]

Done.

+  final List<int> hits;
+
+  /// A list of token positions in a SourceReportRange which have not been

[nweiz, 2016/04/29 00:52:44]

"not been" -> "not yet been"

[kevmoo, 2016/04/29 23:36:54]

Done.

+  /// executed.
+  ///
+  /// The list is sorted.
+  final List<int> misses;
+
+  VMSourceReportCoverage._(Map json)
+      : hits = json['hits'],
+        misses = json['misses'];
+}