Allow debugger evaluate expressions to mute local variables

Allow debugger evaluate expressions to mute local variables

R=yangguo@chromium.org

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

[Yang, 2013-06-25 11:15:00]

Looks good so far, but a test case would be great. I'm not entirely sure which
use case it would fix.

PS: sorry that the review took so long!

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

https://codereview.chromium.org/17636007/diff/3001/src/mirror-debugger.js#new...
src/mirror-debugger.js:1675: this.details_.frameId(),
indentation is off by one.

https://codereview.chromium.org/17636007/diff/3001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/17636007/diff/3001/src/runtime.cc#newcode12380
src/runtime.cc:12380: // those copies are returned to the caller to allow it
track the changes and
do you mean "... to allow tracking the changes..." ?

[Peter Rybin, 2013-06-25 11:40:32]

> Looks good so far, but a test case would be great. I'm not entirely sure which
> use case it would fix.
> 
> PS: sorry that the review took so long!

This is due to the simple user request that evaluate expressions and console
expressions should naturally change local variables as they change other kind of
variables.

[Yang, 2013-06-25 11:42:21]

On 2013/06/25 11:40:32, Peter Rybin wrote:
> > Looks good so far, but a test case would be great. I'm not entirely sure
which
> > use case it would fix.
> > 
> > PS: sorry that the review took so long!
> 
> This is due to the simple user request that evaluate expressions and console
> expressions should naturally change local variables as they change other kind
of
> variables.

I see. Could you include a test case where this is tested? Something like two
debug evaluates, one to set the local var, the second to check that the change
was indeed applied.

[Peter Rybin, 2013-06-25 12:22:40]

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

https://codereview.chromium.org/17636007/diff/3001/src/mirror-debugger.js#new...
src/mirror-debugger.js:1675: this.details_.frameId(),
On 2013/06/25 11:15:01, Yang wrote:
> indentation is off by one.

Done.

https://codereview.chromium.org/17636007/diff/3001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/17636007/diff/3001/src/runtime.cc#newcode12380
src/runtime.cc:12380: // those copies are returned to the caller to allow it
track the changes and
On 2013/06/25 11:15:01, Yang wrote:
> do you mean "... to allow tracking the changes..." ?

Yes. Done.

[Peter Rybin, 2013-06-25 12:24:49]

> I see. Could you include a test case where this is tested? Something like two
> debug evaluates, one to set the local var, the second to check that the change
> was indeed applied.

In fact I simply slightly extended the existing test, but it looks consistent to
me.

[Yang, 2013-06-25 12:30:58]

On 2013/06/25 12:22:40, Peter Rybin wrote:
> https://codereview.chromium.org/17636007/diff/3001/src/mirror-debugger.js
> File src/mirror-debugger.js (right):
> 
>
https://codereview.chromium.org/17636007/diff/3001/src/mirror-debugger.js#new...
> src/mirror-debugger.js:1675: this.details_.frameId(),
> On 2013/06/25 11:15:01, Yang wrote:
> > indentation is off by one.
> 
> Done.
> 
> https://codereview.chromium.org/17636007/diff/3001/src/runtime.cc
> File src/runtime.cc (right):
> 
> https://codereview.chromium.org/17636007/diff/3001/src/runtime.cc#newcode12380
> src/runtime.cc:12380: // those copies are returned to the caller to allow it
> track the changes and
> On 2013/06/25 11:15:01, Yang wrote:
> > do you mean "... to allow tracking the changes..." ?
> 
> Yes. Done.

LGTM!

[Peter.Rybin, 2013-06-25 13:48:52]

Committed patchset #4 manually as r15323 (presubmit successful).