Add debugger protocol request for setting global flags.

Add debugger protocol request for setting global flags.

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

[yurys, 2010-07-01 16:40:45]

This change is fine with me. Although the client could achieve the same effect
by disabling all breakpoints one by one and later reenabling some of them, it
makes temporal deactivation of all breakpoints very simple and doesn't require
the client to store the breakpoints data between state changes. Since it's a
common feature in debugger to temporarily deactivate all breakpoints I think
it'd be good to have it in the protocol.

http://codereview.chromium.org/2880011/diff/1/2
File src/debug-debugger.js (right):

http://codereview.chromium.org/2880011/diff/1/2#newcode1310
src/debug-debugger.js:1310: } else if (request.command ==
'setbreakpointsactivated') {
We also need a way to get current state of the breakpoint, either in a separate
request or as part of 'listbreakpoints'

http://codereview.chromium.org/2880011/diff/1/2#newcode1651
src/debug-debugger.js:1651: response.body = {};
Probably send new state in the body?

http://codereview.chromium.org/2880011/diff/1/4
File test/mjsunit/debug-set-break-points-activated.js (right):

http://codereview.chromium.org/2880011/diff/1/4#newcode1
test/mjsunit/debug-set-break-points-activated.js:1: // Copyright 2008 the V8
project authors. All rights reserved.
2008->2010

http://codereview.chromium.org/2880011/diff/1/4#newcode36
test/mjsunit/debug-set-break-points-activated.js:36: var base_request =
'"seq":0,"type":"request","command":"setbreakpointsactivated"'
style: long line

[Peter Rybin, 2010-07-01 21:38:56]

Pavel,

thanks for doing this. I have long wanted to start it myself.

Please consider the following suggestion.

I guess there are a set of global parameters that we may want to control from a
debugger. I.e. not only break_points_activated.
It may make sense to generalize your protocol to update and read them all. We
could have a simple map of handlers on the server side that implements
reading/writing for them.
E.g. we may want:
- suspend_on_exceptions: boolean
- string_values_truncate_limit: integer

I can't tell whether I would recommend changing this CL or considering all this
for a future. However, since you are dealing with the protocol, maybe the former
would be safer (not to expose temporary commands).

Peter

http://codereview.chromium.org/2880011/diff/1/2
File src/debug-debugger.js (right):

http://codereview.chromium.org/2880011/diff/1/2#newcode1646
src/debug-debugger.js:1646: var activated = request.arguments.activated;
We may want a type enforcement here. User may have passed us an array here and
we will store it and possibly return back later.

[podivilov, 2010-07-02 10:11:36]

http://codereview.chromium.org/2880011/diff/1/2
File src/debug-debugger.js (right):

http://codereview.chromium.org/2880011/diff/1/2#newcode1310
src/debug-debugger.js:1310: } else if (request.command ==
'setbreakpointsactivated') {
On 2010/07/01 16:40:45, Yury Semikhatsky wrote:
> We also need a way to get current state of the breakpoint, either in a
separate
> request or as part of 'listbreakpoints'
I've added listdebuggerflags request for that.

http://codereview.chromium.org/2880011/diff/1/2#newcode1646
src/debug-debugger.js:1646: var activated = request.arguments.activated;
On 2010/07/01 21:38:57, Peter Rybin wrote:
> We may want a type enforcement here. User may have passed us an array here and
> we will store it and possibly return back later.

Done.

http://codereview.chromium.org/2880011/diff/1/2#newcode1651
src/debug-debugger.js:1651: response.body = {};
On 2010/07/01 16:40:45, Yury Semikhatsky wrote:
> Probably send new state in the body?

Done.

http://codereview.chromium.org/2880011/diff/1/4
File test/mjsunit/debug-set-break-points-activated.js (right):

http://codereview.chromium.org/2880011/diff/1/4#newcode1
test/mjsunit/debug-set-break-points-activated.js:1: // Copyright 2008 the V8
project authors. All rights reserved.
On 2010/07/01 16:40:45, Yury Semikhatsky wrote:
> 2008->2010

Done.

http://codereview.chromium.org/2880011/diff/1/4#newcode36
test/mjsunit/debug-set-break-points-activated.js:36: var base_request =
'"seq":0,"type":"request","command":"setbreakpointsactivated"'
On 2010/07/01 16:40:45, Yury Semikhatsky wrote:
> style: long line

Done.

[yurys, 2010-07-02 11:27:43]

Looks good.

http://codereview.chromium.org/2880011/diff/7001/8001
File src/debug-debugger.js (right):

http://codereview.chromium.org/2880011/diff/7001/8001#newcode181
src/debug-debugger.js:181: if (!Debug.getDebuggerFlag('break_points_activated'))
return false;
I think you can use ebugger_flags.break_points_activated directly, as long as it
happens in the debug-debugger.js

http://codereview.chromium.org/2880011/diff/7001/8001#newcode2143
src/debug-debugger.js:2143:
DebugCommandProcessor.prototype.setDebuggerFlagsRequest_ = function(request,
response) {
style: long line

http://codereview.chromium.org/2880011/diff/7001/8001#newcode2152
src/debug-debugger.js:2152: if (typeof request.arguments[flag] != "boolean") {
I don't think we need this check, there may be integer values or other values.

http://codereview.chromium.org/2880011/diff/7001/8001#newcode2167
src/debug-debugger.js:2167:
DebugCommandProcessor.prototype.listDebuggerFlagsRequest_ = function(request,
response) {
style: long line

[podivilov, 2010-07-02 14:51:02]

http://codereview.chromium.org/2880011/diff/7001/8001
File src/debug-debugger.js (right):

http://codereview.chromium.org/2880011/diff/7001/8001#newcode181
src/debug-debugger.js:181: if (!Debug.getDebuggerFlag('break_points_activated'))
return false;
On 2010/07/02 11:27:43, Yury Semikhatsky wrote:
> I think you can use ebugger_flags.break_points_activated directly, as long as
it
> happens in the debug-debugger.js

Done.

http://codereview.chromium.org/2880011/diff/7001/8001#newcode2143
src/debug-debugger.js:2143:
DebugCommandProcessor.prototype.setDebuggerFlagsRequest_ = function(request,
response) {
On 2010/07/02 11:27:43, Yury Semikhatsky wrote:
> style: long line

Done.

http://codereview.chromium.org/2880011/diff/7001/8001#newcode2152
src/debug-debugger.js:2152: if (typeof request.arguments[flag] != "boolean") {
On 2010/07/02 11:27:43, Yury Semikhatsky wrote:
> I don't think we need this check, there may be integer values or other values.

Done.

http://codereview.chromium.org/2880011/diff/7001/8001#newcode2167
src/debug-debugger.js:2167:
DebugCommandProcessor.prototype.listDebuggerFlagsRequest_ = function(request,
response) {
On 2010/07/02 11:27:43, Yury Semikhatsky wrote:
> style: long line

Done.

[yurys, 2010-07-02 15:10:34]

http://codereview.chromium.org/2880011/diff/13001/14001
File src/debug-debugger.js (right):

http://codereview.chromium.org/2880011/diff/13001/14001#newcode85
src/debug-debugger.js:85: getter: function() { return this.value; },
could you rename them to get/setValue?

http://codereview.chromium.org/2880011/diff/13001/14001#newcode2132
src/debug-debugger.js:2132: if (!request.arguments) {
As discussed offline please return all properties when there is no arguments
field.

http://codereview.chromium.org/2880011/diff/13001/14001#newcode2153
src/debug-debugger.js:2153: debugger_flags[name].setter(flags[i].value);
consider extracting debugger_flags[name] into a variable

http://codereview.chromium.org/2880011/diff/13001/14002
File test/mjsunit/debug-debugger-flags.js (right):

http://codereview.chromium.org/2880011/diff/13001/14002#newcode73
test/mjsunit/debug-debugger-flags.js:73: testArguments(dcp,
'{"flags":[{"name":"break_points_activated","value":true}]}', true);
mind 80 chars constraint

[podivilov, 2010-07-02 15:38:14]

http://codereview.chromium.org/2880011/diff/13001/14001
File src/debug-debugger.js (right):

http://codereview.chromium.org/2880011/diff/13001/14001#newcode85
src/debug-debugger.js:85: getter: function() { return this.value; },
On 2010/07/02 15:10:34, Yury Semikhatsky wrote:
> could you rename them to get/setValue?
> 

Done.

http://codereview.chromium.org/2880011/diff/13001/14001#newcode2132
src/debug-debugger.js:2132: if (!request.arguments) {
On 2010/07/02 15:10:34, Yury Semikhatsky wrote:
> As discussed offline please return all properties when there is no arguments
> field.

Done.

http://codereview.chromium.org/2880011/diff/13001/14001#newcode2153
src/debug-debugger.js:2153: debugger_flags[name].setter(flags[i].value);
On 2010/07/02 15:10:34, Yury Semikhatsky wrote:
> consider extracting debugger_flags[name] into a variable

Done.

[Søren Thygesen Gjesse, 2010-07-28 08:11:26]

In the C++ part of the debugger we already have a disable_break flag (normally
controlled through the DisableBreak stack allocated class) used to disable all
breaks. I therefore think that disabling breaks through the debugger protocol
should control this flag (you probably need another function in
runtime.h/runtime.cc for that) instead of having it being checked in the
JavaScript part of the debugger.

http://codereview.chromium.org/2880011/diff/18001/19001
File src/debug-debugger.js (right):

http://codereview.chromium.org/2880011/diff/18001/19001#newcode83
src/debug-debugger.js:83: break_points_activated: {
I think the name should be changed to breakPointsActive, as the protocol use
names without underscores (except in two cases).

http://codereview.chromium.org/2880011/diff/18001/19001#newcode1347
src/debug-debugger.js:1347: } else if (request.command == 'debuggerflags') {
How about changing the name to just 'flags'?

[podivilov, 2010-07-28 10:49:47]

All done.

[Søren Thygesen Gjesse, 2010-07-28 13:41:41]

LGTM