Issue 2399 part 1: In debugger allow modifying local variable values

Issue 2399 part 1: In debugger allow modifying local variable values

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

[Peter Rybin, 2012-11-17 20:32:02]

Hi Yang,

this is a partial feature implementation. It has some open questions marked as
TODO.

Test is following (in this change).

Peter

[Yang, 2012-11-19 17:00:54]

First part of comments on the .js files. I have yet to take a closer look at
runtime.cc.

A test would indeed be great. You can debug-backtrace.js as example to process
json debug commands.

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

http://codereview.chromium.org/11415042/diff/1/src/debug-debugger.js#newcode1958
src/debug-debugger.js:1958:
DebugCommandProcessor.prototype.resolveFrameFromScopeDescription_ =
function(scope_description) {
80 character limit.

http://codereview.chromium.org/11415042/diff/1/src/debug-debugger.js#newcode2045
src/debug-debugger.js:2045: if (!context_value_mirror) {
where does context_value_mirror come from?

you may want to add some tests running through this code path.

http://codereview.chromium.org/11415042/diff/1/src/mirror-debugger.js
File src/mirror-debugger.js (right):

http://codereview.chromium.org/11415042/diff/1/src/mirror-debugger.js#newcode...
src/mirror-debugger.js:1878: raw_res = %SetScopeVariableValue(this.break_id_,
this.frame_id_, this.inlined_frame_id_, this.index_, name, new_value);
80 char limit

[Peter Rybin, 2012-11-20 22:27:21]

Hi Yang

Now I added a test for this functionality.

Peter

https://codereview.chromium.org/11415042/diff/1/src/debug-debugger.js
File src/debug-debugger.js (right):

https://codereview.chromium.org/11415042/diff/1/src/debug-debugger.js#newcode...
src/debug-debugger.js:1958:
DebugCommandProcessor.prototype.resolveFrameFromScopeDescription_ =
function(scope_description) {
On 2012/11/19 17:00:55, Yang wrote:
> 80 character limit.

Done.

https://codereview.chromium.org/11415042/diff/1/src/debug-debugger.js#newcode...
src/debug-debugger.js:2045: if (!context_value_mirror) {
On 2012/11/19 17:00:55, Yang wrote:
> where does context_value_mirror come from?
> 
> you may want to add some tests running through this code path.

That's a bad error. Fixed. Added.

https://codereview.chromium.org/11415042/diff/1/src/mirror-debugger.js
File src/mirror-debugger.js (right):

https://codereview.chromium.org/11415042/diff/1/src/mirror-debugger.js#newcod...
src/mirror-debugger.js:1878: raw_res = %SetScopeVariableValue(this.break_id_,
this.frame_id_, this.inlined_frame_id_, this.index_, name, new_value);
On 2012/11/19 17:00:55, Yang wrote:
> 80 char limit

Done.

[Peter Rybin, 2012-11-23 15:54:34]

ping

[Yang, 2012-11-27 16:46:23]

Sorry about the delay... I took a look, and there are two things I noticed, for
now. I'll continue with the review tomorrow.

https://codereview.chromium.org/11415042/diff/4002/src/debug-debugger.js
File src/debug-debugger.js (right):

https://codereview.chromium.org/11415042/diff/4002/src/debug-debugger.js#newc...
src/debug-debugger.js:2088: var scope_holder =
this.resolveScopeHolder_(scope_description);
Somewhat confused. In here you pass request.arguments.scope to
resolveScopeHolder_ but in some places you pass request.arguments to
resolveScopeHolder_.

https://codereview.chromium.org/11415042/diff/4002/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/11415042/diff/4002/src/runtime.cc#newcode10814
src/runtime.cc:10814: static bool SetClosureVariableValue(Isolate* isolate,
Handle<Context> context,
One argument per line please.

https://codereview.chromium.org/11415042/diff/4002/src/runtime.cc#newcode10847
src/runtime.cc:10847: for (int i = 0; i < keys->length(); i++) {
Instead of iterating over all the keys, wouldn't it be possible to use
JSReceiver::HasProperty to check whether the property exists, and then set the
property using SetProperty?

[Peter Rybin, 2012-11-27 18:24:28]

Thank you for review.

You also might want to comment on several TODO(code review) items -- decisions I
had doubts about and decided to write them explicitly.

https://codereview.chromium.org/11415042/diff/4002/src/debug-debugger.js
File src/debug-debugger.js (right):

https://codereview.chromium.org/11415042/diff/4002/src/debug-debugger.js#newc...
src/debug-debugger.js:2088: var scope_holder =
this.resolveScopeHolder_(scope_description);
On 2012/11/27 16:46:23, Yang wrote:
> Somewhat confused. In here you pass request.arguments.scope to
> resolveScopeHolder_ but in some places you pass request.arguments to
> resolveScopeHolder_.

I checked it again. It should be OK. For the new commands I'm trying to put
scope holder reference in a separate object (to better structure it and to make
scopes replacable). Old commands (such as 'scopes' and 'scope') have the same
properties on the arguments level. I should support both cases.

Do you think some comment is required about this?

https://codereview.chromium.org/11415042/diff/4002/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/11415042/diff/4002/src/runtime.cc#newcode10814
src/runtime.cc:10814: static bool SetClosureVariableValue(Isolate* isolate,
Handle<Context> context,
On 2012/11/27 16:46:23, Yang wrote:
> One argument per line please.

Done.

[Peter Rybin, 2012-11-27 21:36:13]

https://codereview.chromium.org/11415042/diff/4002/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/11415042/diff/4002/src/runtime.cc#newcode10847
src/runtime.cc:10847: for (int i = 0; i < keys->length(); i++) {
On 2012/11/27 16:46:23, Yang wrote:
> Instead of iterating over all the keys, wouldn't it be possible to use
> JSReceiver::HasProperty to check whether the property exists, and then set the
> property using SetProperty?

Done.

[ulan_google, 2012-11-30 10:17:28]

Overall looks good. One question and few nits.

https://codereview.chromium.org/11415042/diff/13001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/11415042/diff/13001/src/runtime.cc#newcode10810
src/runtime.cc:10810: // TODO(code review): alternative approach is to
generalize MaterializeClosure
I don't like code duplication, but it looks like the generalization wouldn't
bring much.

https://codereview.chromium.org/11415042/diff/13001/src/runtime.cc#newcode10844
src/runtime.cc:10844: Handle<FixedArray> keys =
The keys are not used.

https://codereview.chromium.org/11415042/diff/13001/src/runtime.cc#newcode10850
src/runtime.cc:10850: // TODO(code review): should we do it more accurately?
I would expect checking and setting only local properties of "ext". Are there
cases where we need to examine prototypes?

https://codereview.chromium.org/11415042/diff/13001/src/runtime.cc#newcode11408
src/runtime.cc:11408: int n = 0;
"int n = 0" can be moved into the for loop as it is not used anywhere else.

[ulan_google, 2012-11-30 10:17:28]

Overall looks good. One question and few nits.

[Peter Rybin, 2012-12-01 23:26:35]

https://codereview.chromium.org/11415042/diff/13001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/11415042/diff/13001/src/runtime.cc#newcode10810
src/runtime.cc:10810: // TODO(code review): alternative approach is to
generalize MaterializeClosure
On 2012/11/30 10:17:28, ulan1 wrote:
> I don't like code duplication, but it looks like the generalization wouldn't
> bring much.

Done.

https://codereview.chromium.org/11415042/diff/13001/src/runtime.cc#newcode10844
src/runtime.cc:10844: Handle<FixedArray> keys =
On 2012/11/30 10:17:28, ulan1 wrote:
> The keys are not used.

Done.

https://codereview.chromium.org/11415042/diff/13001/src/runtime.cc#newcode10850
src/runtime.cc:10850: // TODO(code review): should we do it more accurately?
On 2012/11/30 10:17:28, ulan1 wrote:
> I would expect checking and setting only local properties of "ext". Are there
> cases where we need to examine prototypes?

I only copy from the method above. The method above iterated keys with
INCLUDE_PROTOS option. Don't see a reason to do other way here.

https://codereview.chromium.org/11415042/diff/13001/src/runtime.cc#newcode11408
src/runtime.cc:11408: int n = 0;
On 2012/11/30 10:17:28, ulan1 wrote:
> "int n = 0" can be moved into the for loop as it is not used anywhere else.

I just keep the style around.
Done

[ulan_google, 2012-12-03 09:58:27]

> Done
Could you please upload the updated patch set?

[Peter Rybin, 2012-12-03 13:36:05]

On 2012/12/03 09:58:27, ulan1 wrote:
> > Done
> Could you please upload the updated patch set?

I'm sorry. I missed this step :(
Done

[Yang, 2012-12-03 13:53:05]

On 2012/12/03 13:36:05, Peter Rybin wrote:
> On 2012/12/03 09:58:27, ulan1 wrote:
> > > Done
> > Could you please upload the updated patch set?
> 
> I'm sorry. I missed this step :(
> Done

LGTM

[ulan, 2012-12-03 14:11:10]

LGTM.

[ulan, 2012-12-03 14:15:00]

Please note that after you rebase on ToT, you would need to pass isolate to
SetProperty() in runtime.cc, otherwise, you'll get compile error.

[Peter Rybin, 2012-12-03 20:27:09]

On 2012/12/03 14:15:00, ulan wrote:
> Please note that after you rebase on ToT, you would need to pass isolate to
> SetProperty() in runtime.cc, otherwise, you'll get compile error.

Thank you. Exactly how you said.