Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(118)

Issue 11415042: Issue 2399 part 1: In debugger allow modifying local variable values (Closed)

Created:
8 years, 1 month ago by Peter Rybin
Modified:
8 years ago
Reviewers:
ulan, Yang
CC:
v8-dev
Visibility:
Public.

Description

Issue 2399 part 1: In debugger allow modifying local variable values Committed: http://code.google.com/p/v8/source/detail?r=13122

Patch Set 1 #

Total comments: 6

Patch Set 2 : clean and test #

Patch Set 3 : formatx #

Total comments: 6

Patch Set 4 : follow code review #

Patch Set 5 : follow code review #

Total comments: 8

Patch Set 6 : follow codereview #

Unified diffs Side-by-side diffs Delta from patch set Stats (+421 lines, -11 lines) Patch
M src/debug-debugger.js View 1 2 7 chunks +89 lines, -11 lines 0 comments Download
M src/mirror-debugger.js View 1 3 chunks +25 lines, -0 lines 0 comments Download
M src/runtime.h View 1 chunk +1 line, -0 lines 0 comments Download
M src/runtime.cc View 1 2 3 4 5 3 chunks +130 lines, -0 lines 0 comments Download
A test/mjsunit/debug-set-variable-value.js View 1 2 1 chunk +176 lines, -0 lines 0 comments Download

Messages

Total messages: 16 (0 generated)
Peter Rybin
Hi Yang, this is a partial feature implementation. It has some open questions marked as ...
8 years, 1 month ago (2012-11-17 20:32:02 UTC) #1
Yang
First part of comments on the .js files. I have yet to take a closer ...
8 years, 1 month ago (2012-11-19 17:00:54 UTC) #2
Peter Rybin
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): ...
8 years, 1 month ago (2012-11-20 22:27:21 UTC) #3
Peter Rybin
ping
8 years, 1 month ago (2012-11-23 15:54:34 UTC) #4
Yang
Sorry about the delay... I took a look, and there are two things I noticed, ...
8 years ago (2012-11-27 16:46:23 UTC) #5
Peter Rybin
Thank you for review. You also might want to comment on several TODO(code review) items ...
8 years ago (2012-11-27 18:24:28 UTC) #6
Peter Rybin
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++) ...
8 years ago (2012-11-27 21:36:13 UTC) #7
ulan_google
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: // ...
8 years ago (2012-11-30 10:17:28 UTC) #8
ulan_google
Overall looks good. One question and few nits.
8 years ago (2012-11-30 10:17:28 UTC) #9
Peter Rybin
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 ...
8 years ago (2012-12-01 23:26:35 UTC) #10
ulan_google
> Done Could you please upload the updated patch set?
8 years ago (2012-12-03 09:58:27 UTC) #11
Peter Rybin
On 2012/12/03 09:58:27, ulan1 wrote: > > Done > Could you please upload the updated ...
8 years ago (2012-12-03 13:36:05 UTC) #12
Yang
On 2012/12/03 13:36:05, Peter Rybin wrote: > On 2012/12/03 09:58:27, ulan1 wrote: > > > ...
8 years ago (2012-12-03 13:53:05 UTC) #13
ulan
LGTM.
8 years ago (2012-12-03 14:11:10 UTC) #14
ulan
Please note that after you rebase on ToT, you would need to pass isolate to ...
8 years ago (2012-12-03 14:15:00 UTC) #15
Peter Rybin
8 years ago (2012-12-03 20:27:09 UTC) #16
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.

Powered by Google App Engine
This is Rietveld 408576698