Add support to trace warnings in TraceBuffer.

Add support to trace warnings in TraceBuffer.

R=johnmccutchan@google.com

Committed: https://code.google.com/p/dart/source/detail?r=37298

[regis, 2014-06-10 19:56:23]

John,

I am not sure the way I nest the warning as a message is the correct one. Let me
know.

Thanks,
Regis

[regis, 2014-06-10 22:12:03]

John,

I cannot find an email trace of this cl I sent hours ago. Strange.

Anyway, embedding the warning json string as the message does not work, because
the string gets escaped.

I'd rather not add objects (e.g. script) even temporarily to TraceBufferEntry.
Let's chat in person tomorrow about the best way to encode warnings as trace
messages.

Thanks,
Regis

[Cutch, 2014-06-11 21:45:03]

https://codereview.chromium.org/328663008/diff/80001/runtime/vm/trace_buffer.cc
File runtime/vm/trace_buffer.cc (right):

https://codereview.chromium.org/328663008/diff/80001/runtime/vm/trace_buffer....
runtime/vm/trace_buffer.cc:104: void TraceBuffer::TraceWarningV(Isolate*
isolate,
  JSONStream js;
  {
    JSONObject trace_warning(&js);
    trace_warning.AddProperty("type", "CompileError");
    trace_warning.AddProperty("script", script);
    trace_warning.AddProperty("tokenPos", token_pos);
    const char* msg = ...;
    trace_warning.AddProperty("message", msg);
  }
  Trace(micros, js.ToCString());

https://codereview.chromium.org/328663008/diff/80001/runtime/vm/trace_buffer....
runtime/vm/trace_buffer.cc:164: trace_entry.AddProperty("message",
entry.message);
Here, we can do:

if (entry.message_is_escaped) {
  AddPropertyNoEscape("message", entry.message);
} else {
  AddProperty("message", entry.message);
}

https://codereview.chromium.org/328663008/diff/80001/runtime/vm/trace_buffer.h
File runtime/vm/trace_buffer.h (right):

https://codereview.chromium.org/328663008/diff/80001/runtime/vm/trace_buffer....
runtime/vm/trace_buffer.h:19: char* message;
add a:

bool message_is_escaped;

https://codereview.chromium.org/328663008/diff/80001/runtime/vm/trace_buffer_...
File runtime/vm/trace_buffer_test.cc (right):

https://codereview.chromium.org/328663008/diff/80001/runtime/vm/trace_buffer_...
runtime/vm/trace_buffer_test.cc:120: trace_buffer->PrintToJSONStream(&js);
For testing, we could add accessors:

At(intptr_t i) const;
intptr_t Length() const;

to TraceBuffer.

https://codereview.chromium.org/328663008/diff/80001/runtime/vm/trace_buffer_...
runtime/vm/trace_buffer_test.cc:126:
"\\\"script_url\\\":\\\"Plug\\\",\\\"token_pos\\\":0,"
Please also add a test that exercises PrintToJSONStream

[regis, 2014-06-11 23:48:35]

Thanks!

PTAL. Is is what you have in mind?
I still need to move the code out of trace_buffer.cc

-- Regis

https://codereview.chromium.org/328663008/diff/80001/runtime/vm/trace_buffer.cc
File runtime/vm/trace_buffer.cc (right):

https://codereview.chromium.org/328663008/diff/80001/runtime/vm/trace_buffer....
runtime/vm/trace_buffer.cc:104: void TraceBuffer::TraceWarningV(Isolate*
isolate,
On 2014/06/11 21:45:03, Cutch wrote:
>   JSONStream js;
>   {
>     JSONObject trace_warning(&js);
>     trace_warning.AddProperty("type", "CompileError");
>     trace_warning.AddProperty("script", script);
>     trace_warning.AddProperty("tokenPos", token_pos);
>     const char* msg = ...;
>     trace_warning.AddProperty("message", msg);
>   }
>   Trace(micros, js.ToCString());

Done.

https://codereview.chromium.org/328663008/diff/80001/runtime/vm/trace_buffer....
runtime/vm/trace_buffer.cc:164: trace_entry.AddProperty("message",
entry.message);
On 2014/06/11 21:45:03, Cutch wrote:
> Here, we can do:
> 
> if (entry.message_is_escaped) {
>   AddPropertyNoEscape("message", entry.message);
> } else {
>   AddProperty("message", entry.message);
> } 

Done.

https://codereview.chromium.org/328663008/diff/80001/runtime/vm/trace_buffer_...
File runtime/vm/trace_buffer_test.cc (right):

https://codereview.chromium.org/328663008/diff/80001/runtime/vm/trace_buffer_...
runtime/vm/trace_buffer_test.cc:120: trace_buffer->PrintToJSONStream(&js);
On 2014/06/11 21:45:03, Cutch wrote:
> For testing, we could add accessors:
> 
> At(intptr_t i) const;
> intptr_t Length() const;
> 
> to TraceBuffer.

Done.

https://codereview.chromium.org/328663008/diff/80001/runtime/vm/trace_buffer_...
runtime/vm/trace_buffer_test.cc:126:
"\\\"script_url\\\":\\\"Plug\\\",\\\"token_pos\\\":0,"
On 2014/06/11 21:45:03, Cutch wrote:
> Please also add a test that exercises PrintToJSONStream

Done.

[Cutch, 2014-06-12 21:09:06]

lgtm after addressing my comment and moving the code out of trace_buffer and
into the parser/compiler/wherever.

https://codereview.chromium.org/328663008/diff/100001/runtime/vm/trace_buffer.cc
File runtime/vm/trace_buffer.cc (right):

https://codereview.chromium.org/328663008/diff/100001/runtime/vm/trace_buffer...
runtime/vm/trace_buffer.cc:119: trace_warning.AddProperty("type", "Warning");
Please use a different type than "Warning". We will use this type in Observatory
to dispatch, so you might want to call it something more specific, for example:

"CompileError"

or

"JavaScriptComptabilityError",

[regis, 2014-06-13 00:04:38]

Thanks!

I've moved the new code to exceptions.cc for now, but I want to move more code
out of exceptions.cc to a new source file in a later cl.

https://codereview.chromium.org/328663008/diff/100001/runtime/vm/trace_buffer.cc
File runtime/vm/trace_buffer.cc (right):

https://codereview.chromium.org/328663008/diff/100001/runtime/vm/trace_buffer...
runtime/vm/trace_buffer.cc:119: trace_warning.AddProperty("type", "Warning");
On 2014/06/12 21:09:05, Cutch wrote:
> Please use a different type than "Warning". We will use this type in
Observatory
> to dispatch, so you might want to call it something more specific, for
example:
> 
> "CompileError"
> 
> or
> 
> "JavaScriptComptabilityError",
> 

I've called it JSCompatibilityWarning for now.
A later cl will refactor reporting of compilation errors and warnings into a new
source file.

[regis, 2014-06-13 00:47:29]

Committed patchset #7 manually as r37298 (presubmit successful).