Some comments.
There is some strange indentation in liveedit-delay.js, maybe some tab
issue.
That file also have some long lines.
http://codereview.chromium.org/652027/diff/3033/3034
File samples/shell.cc (right):
http://codereview.chromium.org/652027/diff/3033/3034#newcode52
samples/shell.cc:52: v8::Debug::EnableAgent("shell", 9222, true);
Please revert this. We don't want the sample shell starting the debug
agent.
http://codereview.chromium.org/652027/diff/3033/3036
File src/compiler.cc (right):
http://codereview.chromium.org/652027/diff/3033/3036#newcode121
src/compiler.cc:121: Handle<Code> MakeCodeForLiveEdit(CompilationInfo*
info) {
Should the decleration in compiler.h be in #ifdef
ENABLE_DEBUGGER_SUPPORT/#endif as well?
http://codereview.chromium.org/652027/diff/3033/3038
File src/debug-delay.js (right):
http://codereview.chromium.org/652027/diff/3033/3038#newcode750
src/debug-delay.js:750: Debug.change_script_live = function(script,
change_pos, change_len, new_str, change_log) {
Why wrap the call to LiveEditChangeScript?
http://codereview.chromium.org/652027/diff/3033/3038#newcode1987
src/debug-delay.js:1987: response.failed('Script not found by id');
Maybe remove " by id".
http://codereview.chromium.org/652027/diff/3033/3038#newcode1992
src/debug-delay.js:1992: Debug.change_script_live(the_script,
change_pos, change_len, new_string,
Indentation of change_log.
http://codereview.chromium.org/652027/diff/3033/3039
File src/debug.cc (right):
http://codereview.chromium.org/652027/diff/3033/3039#newcode716
src/debug.cc:716: Handle<Script>
script(Script::cast(function->shared()->script()));
Why does the liveedit.js script need to have type normal?
If so please don't match on name, but add an argument to
CompileDebuggerScript.
http://codereview.chromium.org/652027/diff/3033/3039#newcode1966
src/debug.cc:1966: // Handle debugger actions when a new script is
compiled.
Please don't put named TODO's in the code. If fun is not used, you might
as well remove it.
http://codereview.chromium.org/652027/diff/3033/3040
File src/debug.h (right):
http://codereview.chromium.org/652027/diff/3033/3040#newcode609
src/debug.h:609: bool send_from_debugger);
I don't like the boolean flag, as it does not provide any information
when used. Maybe you could add an enum as we do in other places, e.g.
Please add an enum
enum AfterCompileFlag {
NO_AFTER_COMPILE_FLAG,
SEND_WHEN_DEBUGGING
};
or maybe make a separate function for the special case of sending
compiled code when debugging.
http://codereview.chromium.org/652027/diff/3033/3041
File src/liveedit-delay.js (right):
http://codereview.chromium.org/652027/diff/3033/3041#newcode31
src/liveedit-delay.js:31: /**
Please use // style comments.
http://codereview.chromium.org/652027/diff/3033/3041#newcode47
src/liveedit-delay.js:47: Debug.LiveEditChangeScript = function
LiveEditChangeScript(script, change_pos,
Normally the function is without name.
http://codereview.chromium.org/652027/diff/3033/3041#newcode50
src/liveedit-delay.js:50: function Assert(condition, message) {
Indentation.
http://codereview.chromium.org/652027/diff/3033/3041#newcode97
src/liveedit-delay.js:97: function DebugGatherCompileInfo2(source) {
Why does the name end with 2?
Comment indentation (tab?).
http://codereview.chromium.org/652027/diff/3033/3041#newcode102
src/liveedit-delay.js:102: // Sort function infos as by start position
field.
Remove as.
http://codereview.chromium.org/652027/diff/3033/3041#newcode110
src/liveedit-delay.js:110: for (var i = 0; i < compile_info.length; i++)
{
Will it be possible to use Array.sort here?
http://codereview.chromium.org/652027/diff/3033/3041#newcode154
src/liveedit-delay.js:154: return compile_info;
Indentation.
http://codereview.chromium.org/652027/diff/3033/3041#newcode175
src/liveedit-delay.js:175: // the one, that comes last in this list.
comes last -> is later/is further down
http://codereview.chromium.org/652027/diff/3033/3041#newcode181
src/liveedit-delay.js:181: // Now we have stand at the last function
that begins before the change
have stand -> are
http://codereview.chromium.org/652027/diff/3033/3041#newcode183
src/liveedit-delay.js:183: // this function or enclosing one.
or enclosing -> or the enclosing
http://codereview.chromium.org/652027/diff/3033/3041#newcode191
src/liveedit-delay.js:191: // A list of function that must be dropped
from stack before changes are
from stack -> from the stack
http://codereview.chromium.org/652027/diff/3033/3041#newcode195
src/liveedit-delay.js:195: // An index of a single function, that is
going to have its Code replaced.
Code -> code
http://codereview.chromium.org/652027/diff/3033/3044
File src/runtime.cc (right):
http://codereview.chromium.org/652027/diff/3033/3044#newcode8032
src/runtime.cc:8032: if (counter < buffer_size) {
Please add an ASSERT for the object not in new space when skipping a
write barrier.
http://codereview.chromium.org/652027/diff/3033/3044#newcode8040
src/runtime.cc:8040: /**
Please use // for comments.
http://codereview.chromium.org/652027/diff/3033/3044#newcode8071
src/runtime.cc:8071: * For a script calculates compilation infomation
about all its functions
calcolates -> calculate, infomation -> information
http://codereview.chromium.org/652027/diff/3033/3044#newcode8097
src/runtime.cc:8097: static Object*
Runtime_LiveEditReplaceScriptLive(Arguments args) {
These functions names both starts and ends with Live. How about dropping
the last Live?
http://codereview.chromium.org/652027/diff/3033/3044#newcode8124
src/runtime.cc:8124: * Replaces Code of SharedFunctionInfo with a new
one.
Code -> code
http://codereview.chromium.org/652027/diff/3033/3044#newcode8138
src/runtime.cc:8138: * Connectes SharedFunctionInfo to another script
(representing the old
Connectes -> Connects
http://codereview.chromium.org/652027/diff/3033/3044#newcode8141
src/runtime.cc:8141: static Object*
Runtime_LiveEditLinkToOldScriptLive(Arguments args) {
I suggest remove Old from the function name, as the caller can pass any
script.
http://codereview.chromium.org/652027/diff/3033/3048
File tools/js2c.py (right):
http://codereview.chromium.org/652027/diff/3033/3048#newcode295
tools/js2c.py:295: # lines = minifier.JSMinify(lines)
???
http://codereview.chromium.org/652027
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
