I've made all the necessary changes except for breaking up long lines which
are
purely due to long strings in d8.js. I had a question regarding that in my
comments below. Please let me know what the preferred practice is there and
I'll make the remaining change. Thanks.
FYI, I also svn up to the latest to make sure I have no conflicts. As a
result,
some new diffs showed up that are not due to this change set.
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.
On 2011/01/03 08:56:07, Søren Gjesse wrote:
Long line. Some more below.
Done. Shortened the comment.
http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode386
src/d8.js:386: this.request_ = this.frameCommandToJSONRequest_('' +
(Debug.State.currentFrame + 1));
broke up this line to fit in 80 columns.
http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode391
src/d8.js:391: this.request_ = this.frameCommandToJSONRequest_('' +
(Debug.State.currentFrame - 1));
broke up this line to fit in 80 columns.
http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode452
src/d8.js:452: this.request_ =
this.changeBreakpointCommandToJSONRequest_(args, 'enable');
changed to fit into 80 columns.
http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode457
src/d8.js:457: this.request_ =
this.changeBreakpointCommandToJSONRequest_(args, 'disable');
changed to fit into 80 columns.
http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode461
src/d8.js:461: this.request_ =
this.changeBreakpointCommandToJSONRequest_(args, 'ignore');
changed to fit into 80 columns.
http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode608
src/d8.js:608: var stepcount = Number(args[0]);
On 2011/01/03 08:56:07, Søren Gjesse wrote:
Tab character sneaked in.
Done.
http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode788
src/d8.js:788: if (Debug.State.displaySourceEndLine == -1) {
On 2011/01/03 08:56:07, Søren Gjesse wrote:
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.
There was some accidental tabs in here. Fixed. The comments were
unclear. I've clarified it a little. It now reads:
// If we list forwards, we will start listing after the last source
end
// line. Set it to start from 5 lines before the current location.
// If we list backwards, we will start listing backwards from the
last
// source start line. Set it to start from 1 lines before the
current
// location.
Hence, no "if" clauses are needed here. I'm just setting up the
requisite initial conditions for the "if" clauses that follow below.
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.
On 2011/01/03 08:56:07, Søren Gjesse wrote:
This sets displaySourceStartLine if displaySourceEndLine is -1 - can't
that
cause displaySourceStartLine to stay as -1 - is that intended?
displaySourceStartLine and displaySourceEndLine are always set as a
pair. Hence, I only needed to check that one is uninitialized and
initialize both at the same time.
http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode975
src/d8.js:975:
DebugRequest.prototype.changeBreakpointCommandToJSONRequest_ =
function(args, command) {
broke up this line to fit in 80 columns.
http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode978
src/d8.js:978: // Check for exception breaks first:
On 2011/01/03 08:56:07, Søren Gjesse wrote:
Why do you want both
en exc all
and
en all exc
?
"en exc all" when we think in terms of types: enable exceptions of all
types.
"en all exc" when we think in terms how one would say it in English:
enable all exceptions.
I had it as "en exc all" initially but found that in usage, a human
would sometimes think the other way. I just added support for both to
make it easier to use.
http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode983
src/d8.js:983: if ((command == 'enable' || command == 'disable') && args
&& args.length > 1) {
broke up this line to fit in 80 columns.
http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode993
src/d8.js:993: var arg2 = (nextPos > 0) ? args.substring(nextPos + 1,
args.length) : 'all';
broke up this line to fit in 80 columns.
http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode1006
src/d8.js:1006: var arg2 = (nextPos > 0) ? args.substring(nextPos + 1,
args.length) : null;
broke up this line to fit in 80 columns.
http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode1176
src/d8.js:1176: print('Note: [] denotes optional parts of commands, or
optional options / arguments.');
How strict do you want to be on long strings like these exceeding the 80
columns? If you want me to break it up, should I use ' ' + ' ' notation
to join 2 strings? The reason I ask is because unlike C++ code, I
presume we actually have a runtime performance impact here (though it is
insignificant in this case). Just thought I'd ask to understand what is
considered the preferred practice here. Thanks.
http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode1227
src/d8.js:1227: print('trace compile');
On 2011/01/03 08:56:07, Søren Gjesse wrote:
Code in comment. PLease just use an "ordinary" for information on the
hidden
command.
I presume you mean to use an ordinary comment and not commented out code
here. I've applied that.
http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode1313
src/d8.js:1313: function refObjectToString_(protocolPackage, handle) {
On 2011/01/03 08:56:07, Søren Gjesse wrote:
Please use 2-char indentation.
Sorry. Some tabs snuck in. Fixed.
http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode1418
src/d8.js:1418: result += body.enabled ? 'enabled':'disabled';
On 2011/01/03 08:56:07, Søren Gjesse wrote:
Please add spaces on both sides of ':'.
Done.
http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode1468
src/d8.js:1468: }
On 2011/01/03 08:56:07, Søren Gjesse wrote:
Tabs.
Done.
http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode2096
src/d8.js:2096: if (property_value === null) {
On 2011/01/03 08:56:07, Søren Gjesse wrote:
Tab.
Done.
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;
On 2011/01/03 08:56:07, Søren Gjesse wrote:
Please avoid mixed case variables, see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Variable_Names.
Done.
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:
On 2011/01/03 08:56:07, Søren Gjesse wrote:
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.
I see what you mean. But yes, at least as a first pass, I would rather
just leave this as is for now. This can be re-implemented later if
needed.
http://codereview.chromium.org/5980006/diff/10002/src/debug-agent.cc#newcode199
src/debug-agent.cc:199: if (isBreakRequest && !Debug::InDebugger()) {
On 2011/01/03 08:56:07, Søren Gjesse wrote:
Is this required? Sending the command using v8::Debug::SendCommand
should
trigger a break of the DEBUGCOMMAND - is that not sufficient?
Hmmm. This used to be needed, but I just re-tested without it and
things seem to be working fine now. Perhaps the v8 source has mutated
to make this unnecessary since the time I added it in the early days.
I've removed this if statement along with all references to
isBreakRequest above.
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) {
On 2011/01/03 08:56:07, Søren Gjesse wrote:
Long line.
Done.
http://codereview.chromium.org/5980006/diff/10002/src/debug-debugger.js#newcode1735
src/debug-debugger.js:1735:
DebugCommandProcessor.prototype.setExceptionBreakRequest_ =
function(request, response) {
On 2011/01/03 08:56:07, Søren Gjesse wrote:
Ditto.
Done.
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__)
On 2011/01/03 08:56:07, Søren Gjesse wrote:
I think this change of the default is OK for all platforms.
Done. Made it unconditional.
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
On 2011/01/03 08:56:07, Søren Gjesse wrote:
I think there is a long section above guarded with #ifdef
ENABLE_DEBUGGER_SUPPORT/#endif - please move this up there.
Done.
http://codereview.chromium.org/5980006/diff/10002/src/runtime.cc#newcode10732
src/runtime.cc:10732: static MaybeObject* Runtime_SetFlag(Arguments
args) {
On 2011/01/03 08:56:07, Søren Gjesse wrote:
Please change name to plural.
Done.
http://codereview.chromium.org/5980006/diff/10002/src/runtime.cc#newcode10743
src/runtime.cc:10743: static MaybeObject* Runtime_DoGC(Arguments args) {
On 2011/01/03 08:56:07, Søren Gjesse wrote:
How about CollectGarbage (or just GC) instead of DoGC?
Done.
http://codereview.chromium.org/5980006/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
