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

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

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

[Peter Rybin, 2012-12-04 00:00:41]

Hi Yang

this change should complete the feature implementation. It covers 2 more native
scopes: local and catch.

Peter

[Yang, 2012-12-10 14:15:11]

LGTM with some comments.

https://codereview.chromium.org/11412310/diff/2001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/11412310/diff/2001/src/runtime.cc#newcode10858
src/runtime.cc:10858: Isolate* isolate,
I'd prefer having the first argument in the same line as function name, and
align the remaining ones to the first argument.

https://codereview.chromium.org/11412310/diff/2001/src/runtime.cc#newcode10880
src/runtime.cc:10880: Isolate* isolate,
Ditto about argument formatting.

https://codereview.chromium.org/11412310/diff/2001/src/runtime.cc#newcode10914
src/runtime.cc:10914: if (SetContextLocalValue(isolate, scope_info,
function_context,
could you put all arguments on the second line?

https://codereview.chromium.org/11412310/diff/2001/test/mjsunit/debug-set-var...
File test/mjsunit/debug-set-variable-value.js (right):

https://codereview.chromium.org/11412310/diff/2001/test/mjsunit/debug-set-var...
test/mjsunit/debug-set-variable-value.js:128:
ClosureTestCase.prototype.run_and_catch_ = function(runnable) {
Why does this end with an underscore?

https://codereview.chromium.org/11412310/diff/2001/test/mjsunit/debug-set-var...
test/mjsunit/debug-set-variable-value.js:132: try {
you could use
assertThrows(function() { runnable(); });
here.

https://codereview.chromium.org/11412310/diff/2001/test/mjsunit/debug-set-var...
test/mjsunit/debug-set-variable-value.js:282: 
stray edit?

[Peter Rybin, 2012-12-11 23:20:44]

https://codereview.chromium.org/11412310/diff/2001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/11412310/diff/2001/src/runtime.cc#newcode10858
src/runtime.cc:10858: Isolate* isolate,
On 2012/12/10 14:15:11, Yang wrote:
> I'd prefer having the first argument in the same line as function name, and
> align the remaining ones to the first argument.

Done.

https://codereview.chromium.org/11412310/diff/2001/src/runtime.cc#newcode10880
src/runtime.cc:10880: Isolate* isolate,
On 2012/12/10 14:15:11, Yang wrote:
> Ditto about argument formatting.

Done.

https://codereview.chromium.org/11412310/diff/2001/src/runtime.cc#newcode10914
src/runtime.cc:10914: if (SetContextLocalValue(isolate, scope_info,
function_context,
On 2012/12/10 14:15:11, Yang wrote:
> could you put all arguments on the second line?

Done.

https://codereview.chromium.org/11412310/diff/2001/test/mjsunit/debug-set-var...
File test/mjsunit/debug-set-variable-value.js (right):

https://codereview.chromium.org/11412310/diff/2001/test/mjsunit/debug-set-var...
test/mjsunit/debug-set-variable-value.js:128:
ClosureTestCase.prototype.run_and_catch_ = function(runnable) {
On 2012/12/10 14:15:11, Yang wrote:
> Why does this end with an underscore?

I mean it a private member.

https://codereview.chromium.org/11412310/diff/2001/test/mjsunit/debug-set-var...
test/mjsunit/debug-set-variable-value.js:132: try {
On 2012/12/10 14:15:11, Yang wrote:
> you could use
> assertThrows(function() { runnable(); });
> here.

Done.

https://codereview.chromium.org/11412310/diff/2001/test/mjsunit/debug-set-var...
test/mjsunit/debug-set-variable-value.js:282: 
On 2012/12/10 14:15:11, Yang wrote:
> stray edit?

Done.

[Yang, 2012-12-12 09:53:40]

LGTM.

https://codereview.chromium.org/11412310/diff/2001/test/mjsunit/debug-set-var...
File test/mjsunit/debug-set-variable-value.js (right):

https://codereview.chromium.org/11412310/diff/2001/test/mjsunit/debug-set-var...
test/mjsunit/debug-set-variable-value.js:128:
ClosureTestCase.prototype.run_and_catch_ = function(runnable) {
On 2012/12/11 23:20:44, Peter Rybin wrote:
> On 2012/12/10 14:15:11, Yang wrote:
> > Why does this end with an underscore?
> 
> I mean it a private member.

Well, javascript doesn't really have a private concept. But I guess you can keep
it this way if you insist.