pkg/vm_service_client: add getSourceReport to VMServiceReference

lib/src/isolate.dart

Index: lib/src/isolate.dart
diff --git a/lib/src/isolate.dart b/lib/src/isolate.dart
index 53fa415941f91ec47958b5681d0c0d77349e8493..17d4073969d3f42fd302bb89ed6abaadfbb50984 100644
--- a/lib/src/isolate.dart
+++ b/lib/src/isolate.dart
@@ -16,6 +16,7 @@ import 'library.dart';
 import 'pause_event.dart';
 import 'scope.dart';
 import 'sentinel.dart';
+import 'source_report.dart';
 import 'stack.dart';
 import 'stream_manager.dart';
 import 'utils.dart';
@@ -342,6 +343,38 @@ class VMIsolateRef {
     return _scope.sendRequest(method, params);
   }
 
+  /// Generates a set of reports tied to source locations in an isolate.

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

It's confusing that this says "a set of reports" but the return value a single
report object.

It's not clear what "tied to" means here. What information does the source
report include? Which locations does it include information about?

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

Acknowledged.

+  ///
+  /// Set [includeCoverageReport] to `true` to include code coverage
+  /// information.

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

We don't usually talk about "setting" parameters. Usually we say something like
"If [includeCoverageReport] is `true`, the report includes coverage
information."

Describe what it means for coverage information to be included, in terms of the
returned [VMSourceReport] object. Depending on the form the information takes,
it might also be worth explaining what that field (or whatever) is if this is
`false`.

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

Done.

+  ///
+  /// Set [includePossibleBreakpoints] to `true` to  provide a list of token
+  /// positions which correspond to possible breakpoints.

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

"Provide" is usually refers to the caller providing something to the function;
it's confusing to use it to refer to information that's returned from the
function.

As above, explain where these token positions are created in terms of the
[VMSourceReport].

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

Done.

+  ///
+  /// Set [forceCompilation] to `true` to force compilation of all functions in

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

The parameter below is "forceCompile", not "forceCompilation".

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

Done.

+  /// the range of the report.

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

Nit: Don't add unnecessary newlines within paragraphs.

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

Done.

+  /// Forcing compilation can cause a compilation error, which could terminate
+  /// the running Dart program.

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

Explain what the difference is between forcing compilation and not doing so, in
terms of the returned object. Under what circumstances will what information be
different? This may require some empirical experimentation if the VM service
docs don't go into detail.

[kevmoo, 2016/05/03 22:34:50]

Done.

+  Future<VMSourceReport> getSourceReport(
+      {bool includeCoverageReport,
+      bool includePossibleBreakpoints,
+      bool forceCompile}) async {

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

Give these default values and don't use "== true" below.

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

Done.

+    var reports = <String>[];
+    if (includeCoverageReport == true) {
+      reports.add('Coverage');
+    }
+
+    if (includePossibleBreakpoints == true) {
+      reports.add('PossibleBreakpoints');
+    }
+
+    var params = <String, dynamic>{'reports': reports};

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

This map literal has values, so it doesn't need to be explicitly typed.

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

Done.

+

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

Nit: You probably don't need every line of code in the function to be separated
by an empty line.

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

Acknowledged.

+    var json = await _scope.sendRequest('getSourceReport', params);
+
+    return newSourceReport(_scope, json);
+  }
+
   bool operator ==(other) => other is VMIsolateRef &&
       other._scope.isolateId == _scope.isolateId;