Add log support to debugger

Add log support to debugger

- Add 'log' command to control log level.
- Manage Logging stream subscription whenever isolate changes.
- Rename _VMStreamPrinter to _ConsoleStreamPrinter.
- Pipe log events to _ConsoleStreamPrinter.

R=turnidge@google.com

Committed: https://github.com/dart-lang/sdk/commit/33831aaa6b766f0c36982837d2ed441ef61532f3

[Cutch, 2015-08-03 13:36:28]

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

[turnidge, 2015-08-03 18:16:22]

https://codereview.chromium.org/1258063005/diff/1/runtime/observatory/lib/src...
File runtime/observatory/lib/src/elements/debugger.dart (right):

https://codereview.chromium.org/1258063005/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/debugger.dart:355: 
Remove extra blank line here?

https://codereview.chromium.org/1258063005/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/debugger.dart:366:
debugger.console.print('Not implemented.');
Different message here for consistency w/ other commands?  "log expects blah"

https://codereview.chromium.org/1258063005/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/debugger.dart:381: if (level.name ==
levelName) {
Consider adding a case-insensitive match so we can say "log all" in addition to
"log ALL"

https://codereview.chromium.org/1258063005/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/debugger.dart:395: if
(level.name.startsWith(prefix)) {
Consider either

- completing against lowercase versions of the level name as well

or

- using a case-insensitive prefix match.

These differ in terms of the completions shown for "log <tab>".  They also
differ on whether the case provided by the user is "corrected".

https://codereview.chromium.org/1258063005/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/debugger.dart:420: '- Display all log
messages.\n';
The tabbing/layout here is odd - the commands are indented and their comments
follow them and are not indented.  It confuses the eye about whether the comment
applies above or below.

Maybe something like this:

Syntax: log             # Display the current minimum log level
        log <level>     # Set the mimimum log level to <level>

Calling out OFF and ALL explicitly seems like it doesn't belong in the syntax
section.  They are documented as levels above.

https://codereview.chromium.org/1258063005/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/debugger.dart:959: Level _minimumLogLevel =
Level.OFF;
OFF is ok.  Did you consider a different default, e.g. SEVERE?

https://codereview.chromium.org/1258063005/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/debugger.dart:982: data =
event.logRecord["message"].valueAsString;
I'm wondering why valueAsString is showing up here...  Are these messages
getting sent down as heap strings?  Do we want to do that?

https://codereview.chromium.org/1258063005/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/debugger.dart:1092: onEvent);
Streams are per-vm, not per-isolate, so it is not necessary to subscribe and
unsubscribe as the isolate is updated.  Instead, please move subscription
management down with the rest of the subscriptions (e.g. kStdoutStream, etc.)

https://codereview.chromium.org/1258063005/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/debugger.dart:1341:
Future<StreamSubscription> _logSubscriptionFuture;
Move this with other subscription futures.

https://codereview.chromium.org/1258063005/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/debugger.dart:1343: _ConsoleStreamPrinter
_stdioPrinter;
Rename _stdioPrinter to _consolePrinter?

[Cutch, 2015-08-03 19:29:56]

https://codereview.chromium.org/1258063005/diff/1/runtime/observatory/lib/src...
File runtime/observatory/lib/src/elements/debugger.dart (right):

https://codereview.chromium.org/1258063005/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/debugger.dart:355: 
On 2015/08/03 18:16:22, turnidge wrote:
> Remove extra blank line here?

Done.

https://codereview.chromium.org/1258063005/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/debugger.dart:366:
debugger.console.print('Not implemented.');
On 2015/08/03 18:16:22, turnidge wrote:
> Different message here for consistency w/ other commands?  "log expects blah"

Done.

https://codereview.chromium.org/1258063005/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/debugger.dart:381: if (level.name ==
levelName) {
On 2015/08/03 18:16:22, turnidge wrote:
> Consider adding a case-insensitive match so we can say "log all" in addition
to
> "log ALL"

Done.

https://codereview.chromium.org/1258063005/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/debugger.dart:395: if
(level.name.startsWith(prefix)) {
On 2015/08/03 18:16:22, turnidge wrote:
> Consider either
> 
> - completing against lowercase versions of the level name as well
> 
> or
> 
> - using a case-insensitive prefix match.
> 
> These differ in terms of the completions shown for "log <tab>".  They also
> differ on whether the case provided by the user is "corrected".

case-insensitive prefix match.

https://codereview.chromium.org/1258063005/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/debugger.dart:420: '- Display all log
messages.\n';
On 2015/08/03 18:16:22, turnidge wrote:
> The tabbing/layout here is odd - the commands are indented and their comments
> follow them and are not indented.  It confuses the eye about whether the
comment
> applies above or below.
> 
> Maybe something like this:
> 
> Syntax: log             # Display the current minimum log level
>         log <level>     # Set the mimimum log level to <level>
> 
> Calling out OFF and ALL explicitly seems like it doesn't belong in the syntax
> section.  They are documented as levels above.

Done.

https://codereview.chromium.org/1258063005/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/debugger.dart:959: Level _minimumLogLevel =
Level.OFF;
On 2015/08/03 18:16:22, turnidge wrote:
> OFF is ok.  Did you consider a different default, e.g. SEVERE?

I'll start with OFF but we might want to reconsider.

https://codereview.chromium.org/1258063005/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/debugger.dart:982: data =
event.logRecord["message"].valueAsString;
On 2015/08/03 18:16:22, turnidge wrote:
> I'm wondering why valueAsString is showing up here...  Are these messages
> getting sent down as heap strings?  Do we want to do that?

The messages are transmitted as instances. I think this is okay for now.

https://codereview.chromium.org/1258063005/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/debugger.dart:1092: onEvent);
On 2015/08/03 18:16:22, turnidge wrote:
> Streams are per-vm, not per-isolate, so it is not necessary to subscribe and
> unsubscribe as the isolate is updated.  Instead, please move subscription
> management down with the rest of the subscriptions (e.g. kStdoutStream, etc.)

Done.

https://codereview.chromium.org/1258063005/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/debugger.dart:1341:
Future<StreamSubscription> _logSubscriptionFuture;
On 2015/08/03 18:16:22, turnidge wrote:
> Move this with other subscription futures.

Done.

https://codereview.chromium.org/1258063005/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/debugger.dart:1343: _ConsoleStreamPrinter
_stdioPrinter;
On 2015/08/03 18:16:22, turnidge wrote:
> Rename _stdioPrinter to _consolePrinter?

Done.

[turnidge, 2015-08-03 19:41:49]

lgtm

https://codereview.chromium.org/1258063005/diff/20001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/debugger.dart (right):

https://codereview.chromium.org/1258063005/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/debugger.dart:364:
debugger.console.print("set expects zero or one arguments");
maybe "log" instead of "set"?

[Cutch, 2015-08-03 19:43:04]

Committed patchset #3 (id:40001) manually as
33831aaa6b766f0c36982837d2ed441ef61532f3 (presubmit successful).