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 http://codereview.chromium.org/1800007/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
