http://codereview.chromium.org/5980006/diff/10002/src/d8.js File src/d8.js (right):
http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode302 src/d8.js:302: cmd_line = 'break'; // If not in debugger mode, break with a frame request. Long line. Some more below. http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode608 src/d8.js:608: var stepcount = Number(args[0]); Tab character sneaked in. http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode788 src/d8.js:788: if (Debug.State.displaySourceEndLine == -1) { Comment indented more that source line (This comment and the next). Also I am not sure I fully understand these two comments as one says "If we list forwards" and the other "If we list backwards", but they are not inside different "if" clauses. http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode791 src/d8.js:791: // If we list backwards, start from 1 lines before the current location. This sets displaySourceStartLine if displaySourceEndLine is -1 - can't that cause displaySourceStartLine to stay as -1 - is that intended? http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode978 src/d8.js:978: // Check for exception breaks first: Why do you want both en exc all and en all exc ? http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode1227 src/d8.js:1227: print('trace compile'); Code in comment. PLease just use an "ordinary" for information on the hidden command. http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode1313 src/d8.js:1313: function refObjectToString_(protocolPackage, handle) { Please use 2-char indentation. http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode1418 src/d8.js:1418: result += body.enabled ? 'enabled':'disabled'; Please add spaces on both sides of ':'. http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode1468 src/d8.js:1468: } Tabs. http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode2096 src/d8.js:2096: if (property_value === null) { Tab. http://codereview.chromium.org/5980006/diff/10002/src/debug-agent.cc File src/debug-agent.cc (right): http://codereview.chromium.org/5980006/diff/10002/src/debug-agent.cc#newcode173 src/debug-agent.cc:173: const char *msg = *message; Please avoid mixed case variables, see http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Variable_Names. http://codereview.chromium.org/5980006/diff/10002/src/debug-agent.cc#newcode182 src/debug-agent.cc:182: // Check if we're getting a break request: These checks seems pretty fragile, and depends on the debugger client formatting of the JSON. There is now a separate JSON parser in C++ (see Runtime_ParseJson where it is used), and maybe that can be used instead, but that might require more refactoring as we don't want to parse the request twice. We can probably have it like this for now. http://codereview.chromium.org/5980006/diff/10002/src/debug-agent.cc#newcode199 src/debug-agent.cc:199: if (isBreakRequest && !Debug::InDebugger()) { Is this required? Sending the command using v8::Debug::SendCommand should trigger a break of the DEBUGCOMMAND - is that not sufficient? http://codereview.chromium.org/5980006/diff/10002/src/debug-debugger.js File src/debug-debugger.js (right): http://codereview.chromium.org/5980006/diff/10002/src/debug-debugger.js#newcode1729 src/debug-debugger.js:1729: DebugCommandProcessor.prototype.disconnectRequest_ = function(request, response) { Long line. http://codereview.chromium.org/5980006/diff/10002/src/debug-debugger.js#newcode1735 src/debug-debugger.js:1735: DebugCommandProcessor.prototype.setExceptionBreakRequest_ = function(request, response) { Ditto. http://codereview.chromium.org/5980006/diff/10002/src/debug.cc File src/debug.cc (right): http://codereview.chromium.org/5980006/diff/10002/src/debug.cc#newcode625 src/debug.cc:625: #if defined(WEBOS__) I think this change of the default is OK for all platforms. http://codereview.chromium.org/5980006/diff/10002/src/runtime.cc File src/runtime.cc (right): http://codereview.chromium.org/5980006/diff/10002/src/runtime.cc#newcode10730 src/runtime.cc:10730: #ifdef ENABLE_DEBUGGER_SUPPORT I think there is a long section above guarded with #ifdef ENABLE_DEBUGGER_SUPPORT/#endif - please move this up there. http://codereview.chromium.org/5980006/diff/10002/src/runtime.cc#newcode10732 src/runtime.cc:10732: static MaybeObject* Runtime_SetFlag(Arguments args) { Please change name to plural. http://codereview.chromium.org/5980006/diff/10002/src/runtime.cc#newcode10743 src/runtime.cc:10743: static MaybeObject* Runtime_DoGC(Arguments args) { How about CollectGarbage (or just GC) instead of DoGC? http://codereview.chromium.org/5980006/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
