http://codereview.chromium.org/242034/diff/2009/2010 File src/debug-delay.js (right):
http://codereview.chromium.org/242034/diff/2009/2010#newcode798 Line 798: ExecutionState.prototype.debugCommandProcessor = function(is_running_initial) { On 2009/09/30 09:19:15, Søren Gjesse wrote: > How about changing the name of the argument to auto_continue as it indicates > whether an inplicit continue command is in effect. Also consider making it > optional (just prefixing it with opt_). I'm not sure it's perfect. All around here we deal with "running" variables. Here we do not actually care what it means. We do not run a cycle in debug-delay.js, so we do not know what "auto_continue" means here. I would prefer to keep "auto_continue" in debug.cc code and here to work with "running". What do you think? http://codereview.chromium.org/242034/diff/2009/2010#newcode1086 Line 1086: this.running_ = is_running_initial; On 2009/09/30 09:19:15, Søren Gjesse wrote: > Consider making this optional and default to false. > this.running_ = opt_auto_continue || false; Done. http://codereview.chromium.org/242034/diff/2009/2010#newcode1319 Line 1319: response.new_running_value = true; On 2009/09/30 09:19:15, Søren Gjesse wrote: > I still cannot see why just assigning response.running here will not work. It will surely work for computer but I'm not sure well enough for human. It gets set and read in a unobvious sequence. Having a single "running" variable is like a variable reuse. I tried to have each variable set only once. Done http://codereview.chromium.org/242034/diff/2009/2010#newcode1538 Line 1538: if (this.isRunning()) { On 2009/09/30 09:19:15, Søren Gjesse wrote: > This should be removed. I see no reason for not allowing backtrace requests at > all times. Even with this check the command sequence suspend, backtrace, > continue will provide the same information. I wasn't sure we want random-time backtrace commands. Done http://codereview.chromium.org/242034/diff/2009/2010#newcode1539 Line 1539: // No stacktrace when running On 2009/09/30 09:19:15, Søren Gjesse wrote: > Even if we want to disallow some commands in some situations the response should > have success=false and a message explaining the cause of the failure. I like it. However, I guess we think we shouldn't do this. http://codereview.chromium.org/242034/diff/2009/2010#newcode1918 Line 1918: // TODO(peter.rybin): probably we need some body field here. On 2009/09/30 09:19:15, Søren Gjesse wrote: > I think you should just remove this comment. A response to the suspend command > containing success=true and running=false should be all the information required > for the debugger client. Thanks Done http://codereview.chromium.org/242034/diff/2009/2010#newcode1919 Line 1919: response.new_running_value = false; On 2009/09/30 09:19:15, Søren Gjesse wrote: > Also here, I cannot see why just assigning response.running will not work. Done. http://codereview.chromium.org/242034/diff/2009/2012 File test/mjsunit/debug-backtrace.js (right): http://codereview.chromium.org/242034/diff/2009/2012#newcode101 Line 101: dcp = exec_state.debugCommandProcessor(false); On 2009/09/30 09:19:15, Søren Gjesse wrote: > Consider argument default to false. I did it optional. However, I think explicit is better... http://codereview.chromium.org/242034/diff/2009/2013 File test/mjsunit/debug-changebreakpoint.js (right): http://codereview.chromium.org/242034/diff/2009/2013#newcode62 Line 62: var dcp = exec_state.debugCommandProcessor("unspecified_running_state"); On 2009/09/30 09:19:15, Søren Gjesse wrote: > Passing other values than true or false does not make any sense. Please use true or false in all the tests. I'm not sure here. It does make sense as a "unique" value. I check that handler doesn't alter "running" field whether to "true" or "false", because it shouldn't. I think that debug command processor should not necessarily have boolean "running", because it doesn't use it. Do you think I should document it? http://codereview.chromium.org/242034/diff/2009/2017 File test/mjsunit/debug-evaluate-recursive.js (right): http://codereview.chromium.org/242034/diff/2009/2017#newcode151 Line 151: assertTrue(listenerComplete, "msg5"); On 2009/09/30 09:19:15, Søren Gjesse wrote: > msg5? We don't have messages here and we don't have stacktrace. I thought this mark-up helps tracing error and it's better than nothing. Done. http://codereview.chromium.org/242034/diff/2009/2017#newcode166 Line 166: assertTrue(listenerComplete, "msg1"); On 2009/09/30 09:19:15, Søren Gjesse wrote: > msg1? Done. http://codereview.chromium.org/242034/diff/2009/2025 File test/mjsunit/debug-suspend.js (right): http://codereview.chromium.org/242034/diff/2009/2025#newcode58 Line 58: var response = safeEval(dcp.processDebugJSONRequest(request)); On 2009/09/30 09:19:15, Søren Gjesse wrote: > This does not really test that the suspend command changes the running state, as > the running state is false before as this code is running in a debug event > listener and not a message handler with auto break enabled. To test suspend you > could issue a continue command before the suspend command to see running go from > false to true to false. Done http://codereview.chromium.org/242034/diff/2009/2025#newcode61 Line 61: } else { On 2009/09/30 09:19:15, Søren Gjesse wrote: > Empty else part. Done. http://codereview.chromium.org/242034/diff/2009/2025#newcode67 Line 67: if (event == Debug.DebugEvent.Break) { On 2009/09/30 09:19:15, Søren Gjesse wrote: > Please indent the try code. Done. http://codereview.chromium.org/242034 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
