I like the idea of providing feedback on where a breakpoint set through a
source
location will actually end up, and handling most of the logic in the
JavaScript
code is good.
However I have some comments. First of all you should convey the
information on
the actual break point as both line and column, and I think we should
consider
including the information in the response to the setbreakpoint request.
Also in
the case where setbreakpoint is called with a type other than 'scriptId' we
need
to do something. For type 'function' and 'handle' at most one break point
will
be set, but for type 'script' there might be several different scripts with
the
same name resulting in several actual break points with different actual
locations.
Also adding information in the AfterCompile event on break points created
when
UpdateScriptBreakPoints was called should be considered. For the
listbreakpoints
request adding information on which actual break points have been added and
there actual positions will also be useful.
I know the data structures representing the break points are somewhat
cluttered,
and is in need of a refactoring, but that is another story.
Also you should add a number of test cases for converting a supplied break
position to the actual break position.
This change also has a fix for the "breakpoint after last function in a
script"
issue. I think maybe you should do that in a separate change. This also
does not
have a test for it. It should be possible to create a test case (in
test-debug.cc) which fails without that change. You might need to do a
number of
GC's (possibly > 5) to make sure that the code for the script itself is no
longer in the compilation cache (if we cache top level functions at all - I
don't remember) before setting a break point somewhere after the last
function
seeing the break in the last function before your change and in the script
(when
recompiled) after your change.
http://codereview.chromium.org/2859003/diff/1/2
File src/debug-debugger.js (right):
http://codereview.chromium.org/2859003/diff/1/2#newcode387
src/debug-debugger.js:387: if (pos === null) return -1;
Please define a constant for -1.
http://codereview.chromium.org/2859003/diff/1/2#newcode393
src/debug-debugger.js:393: if (pos === undefined) return -1;
Please use the macro IS_UNDEFINED here (found in macros.py)
http://codereview.chromium.org/2859003/diff/1/2#newcode395
src/debug-debugger.js:395: return script.lineFromPosition(pos);
Please keep track of both line and column of the actual position.
http://codereview.chromium.org/2859003/diff/1/5
File src/runtime.cc (right):
http://codereview.chromium.org/2859003/diff/1/5#newcode9109
src/runtime.cc:9109: // Make sure some candidate is selected.
This comment is somewhat misleading now.
http://codereview.chromium.org/2859003/diff/1/5#newcode9133
src/runtime.cc:9133: // args[2]: number: break point object
Please add comment on the return value.
http://codereview.chromium.org/2859003/show
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
