pkg/vm_service_client: add getSourceReport to VMServiceReference

lib/src/isolate.dart

 // Copyright (c) 2015, 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 'dart:async';
 import 'dart:collection';
 import 'dart:convert';
 
 import 'package:async/async.dart';
 import 'package:json_rpc_2/json_rpc_2.dart' as rpc;
 
 import 'breakpoint.dart';
 import 'error.dart';
 import 'exceptions.dart';
 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';
 
 VMIsolateRef newVMIsolateRef(rpc.Peer peer, StreamManager streams, Map json) {
   if (json == null) return null;
   assert(json["type"] == "@Isolate" || json["type"] == "Isolate");
   var scope = new Scope(peer, streams, json["id"]);
   return new VMIsolateRef._(scope, json);
 }
@@ skipping 306 matching lines @@
   ///
   /// This is supported as of VM service version 3.1, or Dart SDK version 1.14.
   Future<Object> invokeExtension(String method, [Map<String, String> params]) {
     if (!method.startsWith('ext.')) {
       throw new ArgumentError.value(method, 'method',
           'must begin with "ext." prefix');
     }
     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;
 
   int get hashCode => _scope.isolateId.hashCode;
 
   String toString() => name;
 }
 
 /// A full isolate on the remote VM.
 class VMIsolate extends VMIsolateRef {
@@ skipping 106 matching lines @@
   /// The kind, which identifies the type of event and its source.
   final String kind;
 
   /// The event's payload.
   final Map data;
 
   VMExtensionEvent._(Map json)
       : kind = json['extensionKind'],
         data = json['extensionData'];
 }