[DevTools] Move restartFrame and setCallFrameVariableValue to V8DebuggerAgent

[DevTools] Move restartFrame and setCallFrameVariableValue to V8DebuggerAgent

Methods are moved from InjectedScriptSource.js to native implementation in V8DebuggerAgent.
Removed setVariableValue by function object.

R=dgozman@crhomium.org
BUG=595206
Committed: https://crrev.com/c71d2197b5bec2f76d5b8850f2e3a716b4899ded
Cr-Commit-Position: refs/heads/master@{#381400}

[kozy, 2016-03-11 21:58:26]

ptal

[pfeldman, 2016-03-11 22:04:43]

pfeldman@chromium.org changed reviewers:
+ pfeldman@chromium.org

[pfeldman, 2016-03-11 22:04:44]

https://codereview.chromium.org/1786243002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp
(right):

https://codereview.chromium.org/1786243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp:1016: if
(!injectedScript->resolveCallArgument(errorString,
newValueArgument.get()).ToLocal(&newValue))
Can you extract resolving of the value using its injected script? Seems to be
shared among the two. Or merge them into a single method above.

[kozy, 2016-03-11 23:55:06]

Description was changed from

==========
[DevTools] Move restartFrame and setCallFrameVariableValue to V8DebuggerAgent

Methods are moved from InjectedScriptSource.js to native implementation in
V8DebuggerAgent.

R=dgozman@crhomium.org
==========

to

==========
[DevTools] Move restartFrame and setCallFrameVariableValue to V8DebuggerAgent

Methods are moved from InjectedScriptSource.js to native implementation in
V8DebuggerAgent.
Removed setVariableValue by function object.

R=dgozman@crhomium.org
==========

[kozy, 2016-03-11 23:55:06]

kozyatinskiy@chromium.org changed reviewers:
+ dgozman@chromium.org
- dgozman@crhomium.org

[kozy, 2016-03-11 23:55:17]

all done.
ptal.

https://codereview.chromium.org/1786243002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp
(right):

https://codereview.chromium.org/1786243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp:1016: if
(!injectedScript->resolveCallArgument(errorString,
newValueArgument.get()).ToLocal(&newValue))
On 2016/03/11 22:04:44, pfeldman wrote:
> Can you extract resolving of the value using its injected script? Seems to be
> shared among the two. Or merge them into a single method above.

Removed setFunctionVariableValue.

[pfeldman, 2016-03-12 01:23:26]

https://codereview.chromium.org/1786243002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector-protocol/debugger/debugger-setVariableValue.html
(left):

https://codereview.chromium.org/1786243002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector-protocol/debugger/debugger-setVariableValue.html:3:
<script type="text/javascript"
src="../../http/tests/inspector-protocol/inspector-protocol-test.js"></script>
You are removing this test, but leaving the protocol method. Does it have the
test coverage?

[kozy, 2016-03-12 01:30:15]

https://codereview.chromium.org/1786243002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector-protocol/debugger/debugger-setVariableValue.html
(left):

https://codereview.chromium.org/1786243002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector-protocol/debugger/debugger-setVariableValue.html:3:
<script type="text/javascript"
src="../../http/tests/inspector-protocol/inspector-protocol-test.js"></script>
On 2016/03/12 01:23:26, pfeldman wrote:
> You are removing this test, but leaving the protocol method. Does it have the
> test coverage?

I haven't changed coverage of this protocol method when it's called with
callFrameId.
This method is tested in different places:
inspector/sources/debugger-pause/debugger-change-variable.html
 and inspector-protocol/debugger/access-obsolete-frame.html.

[kozy, 2016-03-15 17:19:12]

ping?

[pfeldman, 2016-03-15 19:12:23]

lgtm

https://codereview.chromium.org/1786243002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/InjectedScript.cpp (right):

https://codereview.chromium.org/1786243002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/InjectedScript.cpp:481: value =
"Number(" + value + ")";
What is this for?

https://codereview.chromium.org/1786243002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp
(right):

https://codereview.chromium.org/1786243002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp:653:
v8::Context::Scope contextScope(localContext);
Why do you have to enter context here?

https://codereview.chromium.org/1786243002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp:955:
v8::Context::Scope contextScope(localContext);
ditto

[kozy, 2016-03-16 02:15:15]

All done.

https://codereview.chromium.org/1786243002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/InjectedScript.cpp (right):

https://codereview.chromium.org/1786243002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/InjectedScript.cpp:481: value =
"Number(" + value + ")";
On 2016/03/15 19:12:22, pfeldman wrote:
> What is this for?

It repeats InjectedScriptSource.js behavior for -Infinity, e.t.c.

https://codereview.chromium.org/1786243002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp
(right):

https://codereview.chromium.org/1786243002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp:653:
v8::Context::Scope contextScope(localContext);
On 2016/03/15 19:12:22, pfeldman wrote:
> Why do you have to enter context here?

Removed!

https://codereview.chromium.org/1786243002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp:955:
v8::Context::Scope contextScope(localContext);
On 2016/03/15 19:12:22, pfeldman wrote:
> ditto

Nice catch! Removed!

[kozy, 2016-03-16 02:15:20]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2016-03-16 02:15:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/1786243002/#ps60001
(title: " ")

[commit-bot: I haz the power, 2016-03-16 02:15:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1786243002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1786243002/60001

[commit-bot: I haz the power, 2016-03-16 02:25:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-16 02:25:31]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[kozy, 2016-03-16 02:52:47]

Description was changed from

==========
[DevTools] Move restartFrame and setCallFrameVariableValue to V8DebuggerAgent

Methods are moved from InjectedScriptSource.js to native implementation in
V8DebuggerAgent.
Removed setVariableValue by function object.

R=dgozman@crhomium.org
==========

to

==========
[DevTools] Move restartFrame and setCallFrameVariableValue to V8DebuggerAgent

Methods are moved from InjectedScriptSource.js to native implementation in
V8DebuggerAgent.
Removed setVariableValue by function object.

R=dgozman@crhomium.org
BUG=595206
==========

[kozy, 2016-03-16 02:53:33]

The CQ bit was checked by kozyatinskiy@chromium.org

[commit-bot: I haz the power, 2016-03-16 02:53:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1786243002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1786243002/60001

[commit-bot: I haz the power, 2016-03-16 05:28:21]

Description was changed from

==========
[DevTools] Move restartFrame and setCallFrameVariableValue to V8DebuggerAgent

Methods are moved from InjectedScriptSource.js to native implementation in
V8DebuggerAgent.
Removed setVariableValue by function object.

R=dgozman@crhomium.org
BUG=595206
==========

to

==========
[DevTools] Move restartFrame and setCallFrameVariableValue to V8DebuggerAgent

Methods are moved from InjectedScriptSource.js to native implementation in
V8DebuggerAgent.
Removed setVariableValue by function object.

R=dgozman@crhomium.org
BUG=595206
==========

[commit-bot: I haz the power, 2016-03-16 05:28:22]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-16 05:30:29]

Description was changed from

==========
[DevTools] Move restartFrame and setCallFrameVariableValue to V8DebuggerAgent

Methods are moved from InjectedScriptSource.js to native implementation in
V8DebuggerAgent.
Removed setVariableValue by function object.

R=dgozman@crhomium.org
BUG=595206
==========

to

==========
[DevTools] Move restartFrame and setCallFrameVariableValue to V8DebuggerAgent

Methods are moved from InjectedScriptSource.js to native implementation in
V8DebuggerAgent.
Removed setVariableValue by function object.

R=dgozman@crhomium.org
BUG=595206

Committed: https://crrev.com/c71d2197b5bec2f76d5b8850f2e3a716b4899ded
Cr-Commit-Position: refs/heads/master@{#381400}
==========

[commit-bot: I haz the power, 2016-03-16 05:30:29]

Patchset 4 (id:??) landed as
https://crrev.com/c71d2197b5bec2f76d5b8850f2e3a716b4899ded
Cr-Commit-Position: refs/heads/master@{#381400}