LiveEdit: breakpoints updates and fixes for related problems

LiveEdit: breakpoints updates and fixes for related problems

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

[Peter Rybin, 2010-04-28 00:59:22]

Hi Soren

This CL more or less fully implements breakpoints update.

I had to rollback my previous approach with breakpoints though.

Also I had to alter LiveEdit tracker configuration in compiler/codegen, because
now I need SharedFunctionInfo instances as well. I realized I should be more
accurate with functions and never allow 2 versions of the same function in heap
linked to the same script (breakpoints goes crazy otherwise).

This CL also fixes a handful of bugs and stupid bugs.

Peter

[Søren Thygesen Gjesse, 2010-04-28 08:54:05]

LGTM

http://codereview.chromium.org/1800007/diff/6001/7003
File src/debug-debugger.js (right):

http://codereview.chromium.org/1800007/diff/6001/7003#newcode399
src/debug-debugger.js:399: ScriptBreakPoint.prototype.cloneForOtherScript =
function (other_script) {
Here you are creating a new script break point object which is not constructed
from the SriptBreakPoint function, but from some other function and have its
prototype be another script break point. First of all I don't think you need the
function ScriptBreakPointCopy can't you just create a plain object and set
__proto__ to this?

However if we need to have script break point copies which share some data with
another script break point why not split all script break points into two
objects so that they all have the same structure?

http://codereview.chromium.org/1800007/diff/6001/7003#newcode401
src/debug-debugger.js:401: function ScriptBreakPointCopy() {
Indentation.

http://codereview.chromium.org/1800007/diff/6001/7003#newcode404
src/debug-debugger.js:404: this.script_name_ = void(0);
Why undefined here?

http://codereview.chromium.org/1800007/diff/6001/7005
File src/liveedit-debugger.js (right):

http://codereview.chromium.org/1800007/diff/6001/7005#newcode97
src/liveedit-debugger.js:97: // LiveEdit itself believs that any function in
heap that points to a
believs -> believe

http://codereview.chromium.org/1800007/diff/6001/7005#newcode513
src/liveedit-debugger.js:513: PosTranslator.default_inside_chunk_handler =
function(pos, diff_chunk) {
Please use UpperCamelCase for functions.

http://codereview.chromium.org/1800007/diff/6001/7005#newcode517
src/liveedit-debugger.js:517: PosTranslator.shift_with_top_inside_chunk_handler
=
Ditto.

http://codereview.chromium.org/1800007/diff/6001/7005#newcode552
src/liveedit-debugger.js:552: this.unmatched_new_nodes = void(0);
Actually void is not a function but a unary operator which returns undefined
after evaluating the expression, so I suggest writing void 0 instead of void(0).

http://codereview.chromium.org/1800007/diff/6001/7008
File src/runtime.cc (right):

http://codereview.chromium.org/1800007/diff/6001/7008#newcode9685
src/runtime.cc:9685: Handle<Object> old_script_name(args[2]);
On checks needed any more?

http://codereview.chromium.org/1800007/diff/6001/7008#newcode9688
src/runtime.cc:9688:
Handle<Script>(Script::cast(original_script_value->value()));
I think you need a checked conversion for this cast.

http://codereview.chromium.org/1800007/diff/6001/7008#newcode9723
src/runtime.cc:9723: if (script_object->IsJSValue()) {
Should this if be nested?

http://codereview.chromium.org/1800007/diff/6001/7008#newcode9725
src/runtime.cc:9725:
Handle<Object>(Script::cast(JSValue::cast(*script_object)->value()));
I think you need a checked conversion for this cast.

http://codereview.chromium.org/1800007/diff/6001/7011
File test/mjsunit/debug-liveedit-breakpoints.js (right):

http://codereview.chromium.org/1800007/diff/6001/7011#newcode92
test/mjsunit/debug-liveedit-breakpoints.js:92:
assertTrue(break_position_map[11]);
Do you want to check that there are exactly 2 break points in this script?

[Peter Rybin, 2010-04-28 11:23:09]

http://codereview.chromium.org/1800007/diff/6001/7003
File src/debug-debugger.js (right):

http://codereview.chromium.org/1800007/diff/6001/7003#newcode399
src/debug-debugger.js:399: ScriptBreakPoint.prototype.cloneForOtherScript =
function (other_script) {
On 2010/04/28 08:54:05, Søren Gjesse wrote:
> Here you are creating a new script break point object which is not constructed
> from the SriptBreakPoint function, but from some other function and have its
> prototype be another script break point. First of all I don't think you need
the
> function ScriptBreakPointCopy can't you just create a plain object and set
> __proto__ to this?

I'm afraid to reveal that I was just trying to avoid non-standard "__proto__"
field.

> However if we need to have script break point copies which share some data
with
> another script break point why not split all script break points into two
> objects so that they all have the same structure?

Actually I just didn't want to copy fields one-by-one in fear I miss some
property or future property.

Anyway, I reimplemented it in more traditional way, but moved closer to
constructor.

http://codereview.chromium.org/1800007/diff/6001/7003#newcode401
src/debug-debugger.js:401: function ScriptBreakPointCopy() {
On 2010/04/28 08:54:05, Søren Gjesse wrote:
> Indentation.

Done.

http://codereview.chromium.org/1800007/diff/6001/7003#newcode404
src/debug-debugger.js:404: this.script_name_ = void(0);
On 2010/04/28 08:54:05, Søren Gjesse wrote:
> Why undefined here?

Done.

http://codereview.chromium.org/1800007/diff/6001/7005
File src/liveedit-debugger.js (right):

http://codereview.chromium.org/1800007/diff/6001/7005#newcode97
src/liveedit-debugger.js:97: // LiveEdit itself believs that any function in
heap that points to a
On 2010/04/28 08:54:05, Søren Gjesse wrote:
> believs -> believe

Done.

http://codereview.chromium.org/1800007/diff/6001/7005#newcode513
src/liveedit-debugger.js:513: PosTranslator.default_inside_chunk_handler =
function(pos, diff_chunk) {
On 2010/04/28 08:54:05, Søren Gjesse wrote:
> Please use UpperCamelCase for functions.

Done.

http://codereview.chromium.org/1800007/diff/6001/7005#newcode517
src/liveedit-debugger.js:517: PosTranslator.shift_with_top_inside_chunk_handler
=
On 2010/04/28 08:54:05, Søren Gjesse wrote:
> Ditto.

Done.

http://codereview.chromium.org/1800007/diff/6001/7005#newcode552
src/liveedit-debugger.js:552: this.unmatched_new_nodes = void(0);
On 2010/04/28 08:54:05, Søren Gjesse wrote:
> Actually void is not a function but a unary operator which returns undefined
> after evaluating the expression, so I suggest writing void 0 instead of
void(0).

Thanks. I didn't know.
Done

http://codereview.chromium.org/1800007/diff/6001/7008
File src/runtime.cc (right):

http://codereview.chromium.org/1800007/diff/6001/7008#newcode9685
src/runtime.cc:9685: Handle<Object> old_script_name(args[2]);
On 2010/04/28 08:54:05, Søren Gjesse wrote:
> On checks needed any more?

I allow Object instead of String now. I need null or undefined to be a possible
input.

http://codereview.chromium.org/1800007/diff/6001/7008#newcode9688
src/runtime.cc:9688:
Handle<Script>(Script::cast(original_script_value->value()));
On 2010/04/28 08:54:05, Søren Gjesse wrote:
> I think you need a checked conversion for this cast.

Done.

http://codereview.chromium.org/1800007/diff/6001/7008#newcode9723
src/runtime.cc:9723: if (script_object->IsJSValue()) {
On 2010/04/28 08:54:05, Søren Gjesse wrote:
> Should this if be nested?

I added commenting "else"

http://codereview.chromium.org/1800007/diff/6001/7008#newcode9725
src/runtime.cc:9725:
Handle<Object>(Script::cast(JSValue::cast(*script_object)->value()));
On 2010/04/28 08:54:05, Søren Gjesse wrote:
> I think you need a checked conversion for this cast.

Done.

http://codereview.chromium.org/1800007/diff/6001/7011
File test/mjsunit/debug-liveedit-breakpoints.js (right):

http://codereview.chromium.org/1800007/diff/6001/7011#newcode92
test/mjsunit/debug-liveedit-breakpoints.js:92:
assertTrue(break_position_map[11]);
On 2010/04/28 08:54:05, Søren Gjesse wrote:
> Do you want to check that there are exactly 2 break points in this script?

Good idea. More accurately there should be 3 of them, but I we do not know
expected position of one of them.
Done