LiveEdit: update breakpoint positions for non-changed functions

LiveEdit: update breakpoint positions for non-changed functions

Committed: http://code.google.com/p/v8/source/detail?r=4359

[Søren Thygesen Gjesse, 2010-04-07 06:56:04]

LGTM with comments addressed.

http://codereview.chromium.org/1090003/diff/7001/8001
File src/debug-debugger.js (left):

http://codereview.chromium.org/1090003/diff/7001/8001#oldcode117
src/debug-debugger.js:117: BreakPoint.prototype.func = function() {
I can see that this is not used, but maybe we should ensure that function break
points actually have the func_ property set?

http://codereview.chromium.org/1090003/diff/7001/8001
File src/debug-debugger.js (right):

http://codereview.chromium.org/1090003/diff/7001/8001#newcode122
src/debug-debugger.js:122: BreakPoint.prototype.patch_source_position =
function(new_position, script) {
patch_source_position -> updateSourcePosition

http://codereview.chromium.org/1090003/diff/7001/8001#newcode594
src/debug-debugger.js:594: // Debug.findBreakPoint does look up script break
points as well.
This is actually OK, as the break point numbers are shared between function
break points and script break points. I don't know whether function break points
are supported in any of the developer tools except the D8 shell.

I have been wanting to refactor the representation of the break points (and also
maybe extend the types, e.g. RegExp matching on source to break in code
delivered through XMLHttpRequest), but never got round to it.

Also it is confusing that a function break point results in a BreakPoint object
and a script break point can result in a  number of BreakPoint objects.

http://codereview.chromium.org/1090003/diff/7001/8002
File src/liveedit-debugger.js (right):

http://codereview.chromium.org/1090003/diff/7001/8002#newcode183
src/liveedit-debugger.js:183: // TODO: explain what is happening.
Not related to this change, but how about using TODO(live_edit) or
TODO(LiveEdit) in places like this?

http://codereview.chromium.org/1090003/diff/7001/8003
File src/liveedit.cc (right):

http://codereview.chromium.org/1090003/diff/7001/8003#newcode625
src/liveedit.cc:625: static Handle<Object>
GetBreakPointObjectsForJS(Handle<BreakPointInfo> break_point_info) {
Several long lines here, please make sure it lints.

http://codereview.chromium.org/1090003/diff/7001/8005
File src/runtime.cc (right):

http://codereview.chromium.org/1090003/diff/7001/8005#newcode9648
src/runtime.cc:9648: // Drop line ends so that they would be recalculated.
would be -> will be/are

[Søren Thygesen Gjesse, 2010-04-07 07:00:39]

http://codereview.chromium.org/1090003/diff/7001/8002
File src/liveedit-debugger.js (right):

http://codereview.chromium.org/1090003/diff/7001/8002#newcode183
src/liveedit-debugger.js:183: // TODO: explain what is happening.
On 2010/04/07 06:56:04, Søren Gjesse wrote:
> Not related to this change, but how about using TODO(live_edit) or
> TODO(LiveEdit) in places like this?

I ment instead of TODO(635). Or change this to TODO(635).

[Peter Rybin, 2010-04-08 11:41:30]

http://codereview.chromium.org/1090003/diff/7001/8001
File src/debug-debugger.js (left):

http://codereview.chromium.org/1090003/diff/7001/8001#oldcode117
src/debug-debugger.js:117: BreakPoint.prototype.func = function() {
On 2010/04/07 06:56:04, Søren Gjesse wrote:
> I can see that this is not used, but maybe we should ensure that function
break
> points actually have the func_ property set?

There is a comment above:
"
NOTE: This object does not have a reference to the function having break
point as this would cause function not to be garbage collected when it is
not used any more. We do not want break points to keep functions alive.
"

That's why I thought this might be a stale getter.

http://codereview.chromium.org/1090003/diff/7001/8001
File src/debug-debugger.js (right):

http://codereview.chromium.org/1090003/diff/7001/8001#newcode122
src/debug-debugger.js:122: BreakPoint.prototype.patch_source_position =
function(new_position, script) {
On 2010/04/07 06:56:04, Søren Gjesse wrote:
> patch_source_position -> updateSourcePosition

Done.

http://codereview.chromium.org/1090003/diff/7001/8001#newcode594
src/debug-debugger.js:594: // Debug.findBreakPoint does look up script break
points as well.
I am probably missing a point here.

> This is actually OK, as the break point numbers are shared between function
> break points and script break points. I don't know 

I see only 2 types/classes of breakpoints: BreakPoint and ScriptBreakPoint. I
thought that function breakpoint is implemented as a BreakPoint.

I looks like we are calling findBreakPoint and findScriptBreakPoint here, and
findBreakPoint already includes functionality of findScriptBreakPoint.

> whether function break points
> are supported in any of the developer tools except the D8 shell.

Neither do I. In GUI debugger you can navigate from function to source and then
set usual breakpoint in source. At least this is supported in Eclipse.
 
However maybe I just should remove the comment in question :)

http://codereview.chromium.org/1090003/diff/7001/8002
File src/liveedit-debugger.js (right):

http://codereview.chromium.org/1090003/diff/7001/8002#newcode183
src/liveedit-debugger.js:183: // TODO: explain what is happening.
On 2010/04/07 06:56:04, Søren Gjesse wrote:
> Not related to this change, but how about using TODO(live_edit) or
> TODO(LiveEdit) in places like this?

I like this. I thought our format strictly requires issue numbers here.
"LiveEdit" is much better than ugly "635" referring to a feature-request issue.

http://codereview.chromium.org/1090003/diff/7001/8002#newcode183
src/liveedit-debugger.js:183: // TODO: explain what is happening.
On 2010/04/07 07:00:39, Søren Gjesse wrote:
> On 2010/04/07 06:56:04, Søren Gjesse wrote:
> > Not related to this change, but how about using TODO(live_edit) or
> > TODO(LiveEdit) in places like this?
> 
> I ment instead of TODO(635). Or change this to TODO(635).

Done.

http://codereview.chromium.org/1090003/diff/7001/8003
File src/liveedit.cc (right):

http://codereview.chromium.org/1090003/diff/7001/8003#newcode625
src/liveedit.cc:625: static Handle<Object>
GetBreakPointObjectsForJS(Handle<BreakPointInfo> break_point_info) {
On 2010/04/07 06:56:04, Søren Gjesse wrote:
> Several long lines here, please make sure it lints.

This is a second time in a row. Sorry about this.

http://codereview.chromium.org/1090003/diff/7001/8005
File src/runtime.cc (right):

http://codereview.chromium.org/1090003/diff/7001/8005#newcode9648
src/runtime.cc:9648: // Drop line ends so that they would be recalculated.
On 2010/04/07 06:56:04, Søren Gjesse wrote:
> would be -> will be/are

Done.