Initial set of comments. I don't understand the messing with the context of
the
restarted function. Also, I don't like changing the scopeinfo for this.
Comments
on these below.
http://codereview.chromium.org/2943002/diff/8001/9012
File src/ia32/debug-ia32.cc (right):
http://codereview.chromium.org/2943002/diff/8001/9012#newcode275
src/ia32/debug-ia32.cc:275: // However height the frame was, reset the
stack pointer
However height the frame was -> Whatever the height of the frame was
http://codereview.chromium.org/2943002/diff/8001/9013
File src/liveedit-debugger.js (right):
http://codereview.chromium.org/2943002/diff/8001/9013#newcode899
src/liveedit-debugger.js:899: // Returns StackInfoWrapper for a future
stack manipulations or
for a -> for
http://codereview.chromium.org/2943002/diff/8001/9014
File src/liveedit.cc (right):
http://codereview.chromium.org/2943002/diff/8001/9014#newcode594
src/liveedit.cc:594: static Handle<JSArray> CreateSuccess(int
bottom_function_index,
Please add an empty lines between these methods for readability.
http://codereview.chromium.org/2943002/diff/8001/9014#newcode661
src/liveedit.cc:661: // TODO(LiveEdit): Move private method below.
Comment should be indented.
http://codereview.chromium.org/2943002/diff/8001/9014#newcode856
src/liveedit.cc:856: SharedFunctionInfo* expected_function);
Indentation.
http://codereview.chromium.org/2943002/diff/8001/9014#newcode916
src/liveedit.cc:916: Object*
LiveEdit::ReplaceFunctionCode(Handle<JSArray> new_compile_info_array,
Move this parameter to next line as well when using four space indent
for the arguments.
http://codereview.chromium.org/2943002/diff/8001/9014#newcode1329
src/liveedit.cc:1329: // If function was in some nested scope (e.g.
'with'), reset it back to
Why would you want to do that? That makes no sense to me. The function
is still in the same context.
http://codereview.chromium.org/2943002/diff/8001/9014#newcode1357
src/liveedit.cc:1357: int top_frame_index,
Indentation.
http://codereview.chromium.org/2943002/diff/8001/9014#newcode1443
src/liveedit.cc:1443: ResetFrameScope(bottom_js_frame);
I do not understand why you mess with the scope of the function here?
Could you please explain?
http://codereview.chromium.org/2943002/diff/8001/9014#newcode1456
src/liveedit.cc:1456: a < unused_stack_bottom;
Indentation.
http://codereview.chromium.org/2943002/diff/8001/9014#newcode1542
src/liveedit.cc:1542: SharedFunctionInfo* expected_function) {
Indentation.
http://codereview.chromium.org/2943002/diff/8001/9015
File src/liveedit.h (right):
http://codereview.chromium.org/2943002/diff/8001/9015#newcode89
src/liveedit.h:89: // in result the function gets restarted.
in result -> as a result
http://codereview.chromium.org/2943002/diff/8001/9015#newcode175
src/liveedit.h:175: #endif /* V*_LIVEEDIT_H_ */
V* -> V8? ;)
http://codereview.chromium.org/2943002/diff/8001/9018
File src/runtime.cc (right):
http://codereview.chromium.org/2943002/diff/8001/9018#newcode8493
src/runtime.cc:8493: FunctionScopeState scope_state =
LiveEdit::IsAtFrameResetPatch(it.frame())
I don't understand why we need to deal with this. Can't we make sure
that it is impossible to have a break point in the restart patch? That
way you don't have to mess with the scope info because at all
breakpoints you will have valid scope info?
http://codereview.chromium.org/2943002/diff/8001/9018#newcode10052
src/runtime.cc:10052: stack_drop_data_temp,
stack_drop_data_wrapped->value());
Could you use one line per argument or move 'stack_drop_data_temp' to
previous line?
http://codereview.chromium.org/2943002/diff/8001/9021
File src/scopeinfo.h (right):
http://codereview.chromium.org/2943002/diff/8001/9021#newcode49
src/scopeinfo.h:49: class ScopeInfoLiveEditHack {};
Where do you use this? Please remove.
I would prefer that live edit does not end up messing with scope info at
all. Can we make sure that the debugger cannot break until we are in a
consistent state after restarting?
http://codereview.chromium.org/2943002/show
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
