Please take a look at the stack traces used for stack traces in JavaScript exceptions (Runtime_CollectStackTrace in runtime.cc and the related code in messages.js) so that we might use this methid for collecting stack traces in there as well.
http://codereview.chromium.org/1694011/diff/35007/38002 File include/v8.h (right): http://codereview.chromium.org/1694011/diff/35007/38002#newcode710 include/v8.h:710: enum FrameOptions { Rename FrameOptions to StackTraceOptions. Then naming of enum values in v8.h is quite a mess. However according to http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Enumerator_Names#Enumerator_Names the preferred naming convention is the constant-like one kXXX, so we should use that going forward. http://codereview.chromium.org/1694011/diff/35007/38002#newcode711 include/v8.h:711: LINE_NUMBER = 0x0001, Please use shift to generate these flags, kLineNumber = 1 << 1, http://codereview.chromium.org/1694011/diff/35007/38002#newcode712 include/v8.h:712: COLUMN_OFFSET = 0x0003, // COLUMN_OFFSET implicitly includes LINE_NUMBER. Use already defined constants kColumn = 1 << 2 | kLine, Just use the name column, as that is what is used in the class Message. http://codereview.chromium.org/1694011/diff/35007/38002#newcode717 include/v8.h:717: // TODO(jaimeyap): Add ability to lookup the method name, No TODOs with name. Eiter remove it or create a bug and use the bug number. http://codereview.chromium.org/1694011/diff/35007/38002#newcode719 include/v8.h:719: // and function arguements. Please put the conbined flags here kOverview = kSourceLine | kSourceColumn | ... http://codereview.chromium.org/1694011/diff/35007/38002#newcode733 include/v8.h:733: * Returns StackTrace as a v8::Array. Please extend the comment with information that the array contain StackFrame objects. http://codereview.chromium.org/1694011/diff/35007/38002#newcode737 include/v8.h:737: /** I don't think this note is required. This is a prerequisite for most function in the API. http://codereview.chromium.org/1694011/diff/35007/38002#newcode751 include/v8.h:751: static const FrameOptions OVERVIEW = Please move this to the enum definition. http://codereview.chromium.org/1694011/diff/35007/38002#newcode758 include/v8.h:758: static const FrameOptions DETAILED = Ditto. http://codereview.chromium.org/1694011/diff/35007/38002#newcode779 include/v8.h:779: * This method will return kNotFound if it is unable to retrieve the column kNotFound -> Message::kNoLineNumberInfo http://codereview.chromium.org/1694011/diff/35007/38002#newcode808 include/v8.h:808: static const int kNotFound; In the profiler API this is called CpuProfileNode::kNoLineNumberInfo and the class Message can also return a line number. We should only have one constant for this, and I suggest you place it in the class Message and call it kNoLineNumberInfo. Then update the comment in class Message and make sure a consistent value is returned (either 0 or -1 - I am not sure what message does). http://codereview.chromium.org/1694011/diff/35007/38001 File src/api.cc (right): http://codereview.chromium.org/1694011/diff/35007/38001#newcode1535 src/api.cc:1535: i::Handle<i::JSObject> obj(i::JSObject::cast(self->GetElement(index))); Please add a range check on index and return undefined if out of bounds. http://codereview.chromium.org/1694011/diff/35007/38003 File src/top.cc (right): http://codereview.chromium.org/1694011/diff/35007/38003#newcode367 src/top.cc:367: These constants are only used in a single place, so why not put the constants directly in the Factory::LookupAsciiSymbol calls below. http://codereview.chromium.org/1694011/diff/35007/38003#newcode368 src/top.cc:368: static const char* kLineKeyData = "line"; line -> lineNumber to match the name of the names in the API. http://codereview.chromium.org/1694011/diff/35007/38003#newcode376 src/top.cc:376: Local<StackTrace> Top::CaptureCurrentStackTrace(int frame_limit, Break line just after ( to have both arguments on the next line if the second argument cannot be aligned with the first. http://codereview.chromium.org/1694011/diff/35007/38003#newcode385 src/top.cc:385: Handle<String> column_key = Factory::NewStringFromAscii( Use Factory::LookupAsciiSymbol(kXXX) here. http://codereview.chromium.org/1694011/diff/35007/38003#newcode416 src/top.cc:416: if (line_number == script_line_offset) { Two spaces after ==. http://codereview.chromium.org/1694011/diff/35007/38003#newcode447 src/top.cc:447: Handle<String> eval_key = Factory::NewStringFromAscii( Use Factory::LookupAsciiSymbol(kXXX). Maybe move it up to where the other symbols are initialized. http://codereview.chromium.org/1694011/diff/35007/38003#newcode452 src/top.cc:452: if (options & StackTrace::IS_CONSTRUCTOR) { The frame has an IsConstructor() method, does that not give the expected result? http://codereview.chromium.org/1694011/diff/35007/38005 File src/top.h (right): http://codereview.chromium.org/1694011/diff/35007/38005#newcode269 src/top.h:269: // NOTE: You must enter a Context::Scope before calling this function! No need for this comment here. http://codereview.chromium.org/1694011/diff/35007/38006 File test/cctest/test-api.cc (right): http://codereview.chromium.org/1694011/diff/35007/38006#newcode9607 test/cctest/test-api.cc:9607: const char* origin = "capture-stack-trace-test"; koverview -> kOverview, kdetailed -> kDetailed http://codereview.chromium.org/1694011/diff/35007/38006#newcode9619 test/cctest/test-api.cc:9619: checkStackFrame(origin, "bar", 2, 3, false, false, frame0); Change frame0 -> stackTrace->GetFrame(0) and avoid the local variable? http://codereview.chromium.org/1694011/diff/35007/38006#newcode9661 test/cctest/test-api.cc:9661: "function bar() {\n" You can avoid dumplcating the test source by using AnalyzeStackInNativeCode(x), then set x from C++ code, and call a JavaScript function in the compiled code which does the "eval('new foo();')". http://codereview.chromium.org/1694011/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
