Expose the new _getSourceReport api in the service protocol.

Expose the new _getSourceReport api in the service protocol.

Allow multiple reports to be requested simultaneously.

I'm not exposing this publicly yet -- still nailing down the format a bit.

BUG=
R=johnmccutchan@google.com

Committed: https://github.com/dart-lang/sdk/commit/61d9752c7e14cb8fe1940c4fc3bf5cda364cfcb8

[turnidge, 2016-01-06 18:50:28]

turnidge@google.com changed reviewers:
+ johnmccutchan@google.com

[Cutch, 2016-01-06 19:18:26]

LGTM with a some comments and questions.

https://codereview.chromium.org/1566793002/diff/20001/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/1566793002/diff/20001/runtime/vm/service.cc#n...
runtime/vm/service.cc:578: const char* const* enums) {
Why pass in enums again? You store it in the constructor.

https://codereview.chromium.org/1566793002/diff/20001/runtime/vm/service.cc#n...
runtime/vm/service.cc:590: // Parse the string destructively.  Build the list of
elements.
Zone allocation is cheap, consider doing this without modifying the source
string?

https://codereview.chromium.org/1566793002/diff/20001/runtime/vm/service.cc#n...
runtime/vm/service.cc:612: const char* cp = value;
Skip whitespace before checking for '['

https://codereview.chromium.org/1566793002/diff/20001/runtime/vm/service.cc#n...
runtime/vm/service.cc:622: cp += strspn(cp, " \t\r\n");
Pull this constant string out to a kWhitespaceChars

https://codereview.chromium.org/1566793002/diff/20001/runtime/vm/service.cc#n...
runtime/vm/service.cc:642: for (intptr_t i = 0; enums[i] != NULL; i++) {
Maybe allow for enums to be NULL so that an arbitrary list could be parsed?

https://codereview.chromium.org/1566793002/diff/20001/runtime/vm/service.cc#n...
runtime/vm/service.cc:2349: return false;
These should return true or else we will hit the UNIMPLEMENTED() call in
InvokeMethod. Also, add a test for an error case too.

[turnidge, 2016-01-06 20:58:13]

https://codereview.chromium.org/1566793002/diff/20001/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/1566793002/diff/20001/runtime/vm/service.cc#n...
runtime/vm/service.cc:578: const char* const* enums) {
On 2016/01/06 19:18:26, Cutch wrote:
> Why pass in enums again? You store it in the constructor.

The rest of our Parse methods are static and I was going for consistency.  I
have changed it to an instance method and I have arranged to keep a
reports_parameter static global.

https://codereview.chromium.org/1566793002/diff/20001/runtime/vm/service.cc#n...
runtime/vm/service.cc:590: // Parse the string destructively.  Build the list of
elements.
On 2016/01/06 19:18:26, Cutch wrote:
> Zone allocation is cheap, consider doing this without modifying the source
> string?

This already is a copy of the string, check out the call to MakeCopyOfString
above.

https://codereview.chromium.org/1566793002/diff/20001/runtime/vm/service.cc#n...
runtime/vm/service.cc:612: const char* cp = value;
On 2016/01/06 19:18:26, Cutch wrote:
> Skip whitespace before checking for '['

The whitespace will be trimmed already by the time it gets this deep into the C
code.  But it is easy to add so I did.

https://codereview.chromium.org/1566793002/diff/20001/runtime/vm/service.cc#n...
runtime/vm/service.cc:622: cp += strspn(cp, " \t\r\n");
On 2016/01/06 19:18:25, Cutch wrote:
> Pull this constant string out to a kWhitespaceChars

Done.

https://codereview.chromium.org/1566793002/diff/20001/runtime/vm/service.cc#n...
runtime/vm/service.cc:642: for (intptr_t i = 0; enums[i] != NULL; i++) {
On 2016/01/06 19:18:26, Cutch wrote:
> Maybe allow for enums to be NULL so that an arbitrary list could be parsed?

Done.

https://codereview.chromium.org/1566793002/diff/20001/runtime/vm/service.cc#n...
runtime/vm/service.cc:2349: return false;
On 2016/01/06 19:18:25, Cutch wrote:
> These should return true or else we will hit the UNIMPLEMENTED() call in
> InvokeMethod. Also, add a test for an error case too.

Thanks.  Done.

[turnidge, 2016-01-06 21:01:38]

Description was changed from

==========
Expose the new _getSourceReport api in the service protocol.

Allow multiple reports to be requested simultaneously.

I'm not exposing this publicly yet -- still nailing down the format a bit.

BUG=
==========

to

==========
Expose the new _getSourceReport api in the service protocol.

Allow multiple reports to be requested simultaneously.

I'm not exposing this publicly yet -- still nailing down the format a bit.

BUG=
R=johnmccutchan@google.com

Committed:
https://github.com/dart-lang/sdk/commit/61d9752c7e14cb8fe1940c4fc3bf5cda364cfcb8
==========

[turnidge, 2016-01-06 21:01:41]

Committed patchset #3 (id:40001) manually as
61d9752c7e14cb8fe1940c4fc3bf5cda364cfcb8 (presubmit successful).