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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 19064004: Support re-reading scope variables in protocol and on backed. (Closed)

Created:
7 years, 5 months ago by Peter.Rybin
Modified:
7 years, 5 months ago
Reviewers:
yurys
CC:
blink-reviews, Nils Barth (inactive), marja+watch_chromium.org, caseq+blink_chromium.org, Nate Chapin, loislo+blink_chromium.org, jsbell+bindings_chromium.org, eustas+blink_chromium.org, alph+blink_chromium.org, eae+blinkwatch, yurys+blink_chromium.org, lushnikov+blink_chromium.org, abarth-chromium, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, dglazkov+blink, paulirish+reviews_chromium.org, apavlov+blink_chromium.org, adamk+blink_chromium.org, haraken, aandrey+blink_chromium.org, do-not-use
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

Support re-reading scope variables in protocol and on backed. BUG=246745 Test depends on another change: https://codereview.chromium.org/18729002/

Patch Set 1 #

Patch Set 2 : clean #

Total comments: 12

Patch Set 3 : follow code review #

Unified diffs Side-by-side diffs Delta from patch set Stats (+174 lines, -30 lines) Patch
A LayoutTests/inspector-protocol/debugger/updateCallFrameScopes.html View 1 2 1 chunk +70 lines, -0 lines 0 comments Download
A LayoutTests/inspector-protocol/debugger/updateCallFrameScopes-expected.txt View 1 chunk +6 lines, -0 lines 0 comments Download
M Source/bindings/v8/DebuggerScript.js View 3 chunks +10 lines, -2 lines 0 comments Download
M Source/core/inspector/InjectedScript.h View 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/inspector/InjectedScript.cpp View 1 chunk +16 lines, -0 lines 0 comments Download
M Source/core/inspector/InjectedScriptExterns.js View 1 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/inspector/InjectedScriptSource.js View 11 chunks +38 lines, -28 lines 0 comments Download
M Source/core/inspector/InspectorDebuggerAgent.h View 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/inspector/InspectorDebuggerAgent.cpp View 1 chunk +10 lines, -0 lines 0 comments Download
M Source/core/inspector/JavaScriptCallFrame.h View 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/inspector/JavaScriptCallFrame.cpp View 1 chunk +7 lines, -0 lines 0 comments Download
M Source/core/inspector/JavaScriptCallFrame.idl View 1 chunk +1 line, -0 lines 0 comments Download
M Source/devtools/protocol.json View 1 chunk +11 lines, -0 lines 0 comments Download

Messages

Total messages: 3 (0 generated)
Peter.Rybin
https://codereview.chromium.org/19064004/diff/2001/Source/devtools/protocol.json File Source/devtools/protocol.json (right): https://codereview.chromium.org/19064004/diff/2001/Source/devtools/protocol.json#newcode3034 Source/devtools/protocol.json:3034: "hidden": true, We may want to make it public, ...
7 years, 5 months ago (2013-07-14 23:13:15 UTC) #1
yurys
https://codereview.chromium.org/19064004/diff/2001/LayoutTests/inspector-protocol/debugger/updateCallFrameScopes-expected.txt File LayoutTests/inspector-protocol/debugger/updateCallFrameScopes-expected.txt (right): https://codereview.chromium.org/19064004/diff/2001/LayoutTests/inspector-protocol/debugger/updateCallFrameScopes-expected.txt#newcode5 LayoutTests/inspector-protocol/debugger/updateCallFrameScopes-expected.txt:5: New variable is 55, expected is 55, old was: ...
7 years, 5 months ago (2013-07-16 07:45:05 UTC) #2
Peter.Rybin
7 years, 5 months ago (2013-07-16 12:02:27 UTC) #3 
https://codereview.chromium.org/19064004/diff/2001/LayoutTests/inspector-prot...
File LayoutTests/inspector-protocol/debugger/updateCallFrameScopes-expected.txt
(right):

https://codereview.chromium.org/19064004/diff/2001/LayoutTests/inspector-prot...
LayoutTests/inspector-protocol/debugger/updateCallFrameScopes-expected.txt:5:
New variable is 55, expected is 55, old was: 2
On 2013/07/16 07:45:05, Yury Semikhatsky wrote:
> Please prefix with SUCCESS or FAIL.

Done.

https://codereview.chromium.org/19064004/diff/2001/LayoutTests/inspector-prot...
File LayoutTests/inspector-protocol/debugger/updateCallFrameScopes.html (right):

https://codereview.chromium.org/19064004/diff/2001/LayoutTests/inspector-prot...
LayoutTests/inspector-protocol/debugger/updateCallFrameScopes.html:16:
InspectorTest.eventHandler["Debugger.paused"] = HandleDebuggerPaused;
On 2013/07/16 07:45:05, Yury Semikhatsky wrote:
> style: handleDebuggerPaused here an in other places.

Done.

https://codereview.chromium.org/19064004/diff/2001/LayoutTests/inspector-prot...
LayoutTests/inspector-protocol/debugger/updateCallFrameScopes.html:29: if
(useMutableLocalVariables)
On 2013/07/16 07:45:05, Yury Semikhatsky wrote:
> Remove this check, it is always true.

Done.

https://codereview.chromium.org/19064004/diff/2001/LayoutTests/inspector-prot...
LayoutTests/inspector-protocol/debugger/updateCallFrameScopes.html:43: var
localScope = response.result.scopeChain[0];
On 2013/07/16 07:45:05, Yury Semikhatsky wrote:
> Wrong alignment here and below.

Done.

https://codereview.chromium.org/19064004/diff/2001/Source/devtools/protocol.json
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/19064004/diff/2001/Source/devtools/protocol.j...
Source/devtools/protocol.json:3027: "name": "updateCallFrameScopes",
On 2013/07/16 07:45:05, Yury Semikhatsky wrote:
> Why is it called updateCallFrameScopes here and rereadScopes in all other
> places. Also 'updateFoo' sounds to me more like request to set new value. Why
> not rereadCallFrameScopes or getCallFrameScopes. On the other hand there may
be
> a better verb than 'reread'. In any case we should be consistent in naming. 

I guess we should discuss naming.

Powered by Google App Engine
This is Rietveld 408576698