Dartium: do not call back into Dart code to parse or generate a stack trace.

Dartium: do not call back into Dart code to parse or generate a stack trace.

R=asiva@google.com, vsm@google.com

Committed: https://src.chromium.org/viewvc/multivm/branches/1650/blink?view=rev&revision=1611

[rmacnak, 2013-11-07 21:30:40]

https://codereview.chromium.org/55433007/diff/90001/Source/bindings/dart/Dart...
File Source/bindings/dart/DartUtilities.cpp (right):

https://codereview.chromium.org/55433007/diff/90001/Source/bindings/dart/Dart...
Source/bindings/dart/DartUtilities.cpp:986: // FIXME
The console Sources tab sometimes tries to highlight the line information
relative to the HTML file rather than the script tag. Perhaps this is the cause?

https://codereview.chromium.org/55433007/diff/90001/Source/bindings/dart/Dart...
Source/bindings/dart/DartUtilities.cpp:989: if (columnNumber != -1)
When stack traces were parsed by a Dart regex, the regex effectively skipped
frames without a column number. For stacks involving calls to console.log or
print, this happens to make the top-of-stack be the caller of console.log/print,
which is expected by a few tests. Presumably they would break if Dartium were
run without a snapshot. The stack is requested by WebKit code, which embodies a
bad assumption that console.log will be implemented directly as a native and
that happens to hold for V8.

[rmacnak, 2013-11-07 21:31:48]

Goes with https://codereview.chromium.org/51793002/.

[vsm, 2013-11-07 22:51:50]

https://codereview.chromium.org/55433007/diff/140001/Source/bindings/dart/Dar...
File Source/bindings/dart/DartUtilities.cpp (right):

https://codereview.chromium.org/55433007/diff/140001/Source/bindings/dart/Dar...
Source/bindings/dart/DartUtilities.cpp:980: String functionName(cstr);
There's a DartUtilities method to go from Dart_Handle to String.

https://codereview.chromium.org/55433007/diff/140001/Source/bindings/dart/Dar...
Source/bindings/dart/DartUtilities.cpp:988: // FIXME
Can you add some text to these fixmes?  Not clear what you intend to fix in the
latter case?

https://codereview.chromium.org/55433007/diff/140001/Source/bindings/dart/Dar...
Source/bindings/dart/DartUtilities.cpp:989: if (columnNumber != -1)
Under what circumstance is this true?

If this check is needed, it should be earlier - no need to do all the string
conversion above to throw away the results.

[siva, 2013-11-08 18:17:17]

https://codereview.chromium.org/55433007/diff/140001/Source/bindings/dart/Dar...
File Source/bindings/dart/DartUtilities.cpp (right):

https://codereview.chromium.org/55433007/diff/140001/Source/bindings/dart/Dar...
Source/bindings/dart/DartUtilities.cpp:961: return 0;
Why do we need this API function at all why not just go with
DartStacktraceFrameInfo(....) returning an error when frameIndex has reached the
end of the stacktrace.

The loop at the bottom can converted into a
do {
  ...
} while(!error && frameIndex < ScriptCallStack::maxCallStackSizeToCapture);

[rmacnak, 2013-11-12 00:05:42]

https://codereview.chromium.org/55433007/diff/140001/Source/bindings/dart/Dar...
File Source/bindings/dart/DartUtilities.cpp (right):

https://codereview.chromium.org/55433007/diff/140001/Source/bindings/dart/Dar...
Source/bindings/dart/DartUtilities.cpp:961: return 0;
On 2013/11/08 18:17:17, siva wrote:
> Why do we need this API function at all why not just go with
> DartStacktraceFrameInfo(....) returning an error when frameIndex has reached
the
> end of the stacktrace.
> 
> The loop at the bottom can converted into a
> do {
>   ...
> } while(!error && frameIndex < ScriptCallStack::maxCallStackSizeToCapture);

As discussed, now sharing with the debugging API's Dart_StackTraceLength.

https://codereview.chromium.org/55433007/diff/140001/Source/bindings/dart/Dar...
Source/bindings/dart/DartUtilities.cpp:980: String functionName(cstr);
On 2013/11/07 22:51:50, vsm wrote:
> There's a DartUtilities method to go from Dart_Handle to String.

D'oh!

https://codereview.chromium.org/55433007/diff/140001/Source/bindings/dart/Dar...
Source/bindings/dart/DartUtilities.cpp:988: // FIXME
On 2013/11/07 22:51:50, vsm wrote:
> Can you add some text to these fixmes?  Not clear what you intend to fix in
the
> latter case?

See the comments in earlier patch sets. Also added some discussion to the code.

[siva, 2013-11-12 00:37:45]

lgtm

https://codereview.chromium.org/55433007/diff/190001/Source/bindings/dart/Dar...
File Source/bindings/dart/DartUtilities.cpp (right):

https://codereview.chromium.org/55433007/diff/190001/Source/bindings/dart/Dar...
Source/bindings/dart/DartUtilities.cpp:960: fprintf(stderr, "-- %d \n",
frameCount);
fprintf is temporary debug code to be removed?

https://codereview.chromium.org/55433007/diff/190001/Source/bindings/dart/Dar...
Source/bindings/dart/DartUtilities.cpp:983: fprintf(stderr, "GetFrameInfo
failed\n");
Ditto comment about fprintf?

https://codereview.chromium.org/55433007/diff/190001/Source/bindings/dart/Dar...
Source/bindings/dart/DartUtilities.cpp:991: // skipped by this clause.
The comment says FIXME, is there anything to be fixed here?

[rmacnak, 2013-11-12 22:32:45]

https://codereview.chromium.org/55433007/diff/190001/Source/bindings/dart/Dar...
File Source/bindings/dart/DartUtilities.cpp (right):

https://codereview.chromium.org/55433007/diff/190001/Source/bindings/dart/Dar...
Source/bindings/dart/DartUtilities.cpp:960: fprintf(stderr, "-- %d \n",
frameCount);
On 2013/11/12 00:37:45, siva wrote:
> fprintf is temporary debug code to be removed?

Woops. Removed.

https://codereview.chromium.org/55433007/diff/190001/Source/bindings/dart/Dar...
Source/bindings/dart/DartUtilities.cpp:991: // skipped by this clause.
On 2013/11/12 00:37:45, siva wrote:
> The comment says FIXME, is there anything to be fixed here?

Removed the "FIXME" part. The problem really lies either in WebKit or the Dart
bindings, depending on whether one believes WebKit's assumption is fair.

[vsm, 2013-11-12 23:07:26]

lgtm

https://chromiumcodereview.appspot.com/55433007/diff/230001/Source/bindings/d...
File Source/bindings/dart/DartUtilities.cpp (right):

https://chromiumcodereview.appspot.com/55433007/diff/230001/Source/bindings/d...
Source/bindings/dart/DartUtilities.cpp:995: // FIXME: Cause of sources tab
sometimes interpreting line/column information relative to the page instead of
the script tag body?
Are we seeing the wrong offset for inline scripts?  If so, can you file a bug?

[rmacnak, 2013-11-15 23:48:25]

https://chromiumcodereview.appspot.com/55433007/diff/230001/Source/bindings/d...
File Source/bindings/dart/DartUtilities.cpp (right):

https://chromiumcodereview.appspot.com/55433007/diff/230001/Source/bindings/d...
Source/bindings/dart/DartUtilities.cpp:995: // FIXME: Cause of sources tab
sometimes interpreting line/column information relative to the page instead of
the script tag body?
On 2013/11/12 23:07:27, vsm wrote:
> Are we seeing the wrong offset for inline scripts?  If so, can you file a bug?

Filed http://dartbug.com/15106

[rmacnak, 2013-11-18 17:37:30]

Committed patchset #6 manually as r1611 (presubmit successful).