[inspector] V8DebuggerAgent cleanup

[inspector] V8DebuggerAgent cleanup

V8DebuggerAgentImpl::m_skipAllPaused is moved to V8Debugger.
V8DebuggerAgentImpl::didPaused doesn't return shouldBreak flag and called only when break is required and stack trace presented.
V8DebuggerAgentImpl doesn't store paused context.
Logic of conversion step-next at return into step-in is moved to debug.cc.

BUG=none
R=dgozman@chromium.org,yangguo@chromium.org

Review-Url: https://codereview.chromium.org/2668763003
Cr-Commit-Position: refs/heads/master@{#42907}
Committed: https://chromium.googlesource.com/v8/v8/+/3a4f5fafe0a13992917511a42e8fd6ee0c65968a

[kozy, 2017-02-01 07:25:13]

Description was changed from

==========
[inspector] V8DebuggerAgent cleanup

BUG=none
R=dgozman@chromium.org,yangguo@chromium.org
==========

to

==========
[inspector] V8DebuggerAgent cleanup

V8DebuggerAgentImpl::m_skipAllPaused is moved to V8Debugger.
V8DebuggerAgentImpl::didPaused doesn't return shouldBreak flag and called only
when break is required and stack trace presented.
V8DebuggerAgentImpl doesn't store paused context.

BUG=none
R=dgozman@chromium.org,yangguo@chromium.org
==========

[kozy, 2017-02-01 07:25:54]

Description was changed from

==========
[inspector] V8DebuggerAgent cleanup

V8DebuggerAgentImpl::m_skipAllPaused is moved to V8Debugger.
V8DebuggerAgentImpl::didPaused doesn't return shouldBreak flag and called only
when break is required and stack trace presented.
V8DebuggerAgentImpl doesn't store paused context.

BUG=none
R=dgozman@chromium.org,yangguo@chromium.org
==========

to

==========
[inspector] V8DebuggerAgent cleanup

V8DebuggerAgentImpl::m_skipAllPaused is moved to V8Debugger.
V8DebuggerAgentImpl::didPaused doesn't return shouldBreak flag and called only
when break is required and stack trace presented.
V8DebuggerAgentImpl doesn't store paused context.
Logic of conversion step-next at return into step-in is moved to debug.cc.

BUG=none
R=dgozman@chromium.org,yangguo@chromium.org
==========

[kozy, 2017-02-01 07:29:43]

Yang, please take a look. I think stepping part could be interesting to you.

https://codereview.chromium.org/2668763003/diff/1/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2668763003/diff/1/src/debug/debug.cc#newcode1035
src/debug/debug.cc:1035: if (step_action == StepNext && location.IsReturn()) {
Yang, WDYT about this part? This code is just moved from V8DebuggerAgentImpl but
it doesn't sound right to me.

https://codereview.chromium.org/2668763003/diff/1/src/debug/debug.cc#newcode1836
src/debug/debug.cc:1836: if (it.done()) return;
Yang, do you know when we can have empty stack here?

[Yang, 2017-02-01 07:46:12]

https://codereview.chromium.org/2668763003/diff/1/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2668763003/diff/1/src/debug/debug.cc#newcode1035
src/debug/debug.cc:1035: if (step_action == StepNext && location.IsReturn()) {
On 2017/02/01 07:29:43, kozy wrote:
> Yang, WDYT about this part? This code is just moved from V8DebuggerAgentImpl
but
> it doesn't sound right to me.

for the microtask case, we should make a check at the microtask runner, where we
would turn pending step-next into a step-in.

for the blackbox case, it's arguable whether step-next should take us into the
next call that the blackboxed caller makes. That could be accomplished by
clicking on step-in. The current behavior assumes that a step-next is actually a
step-next to the caller, and then a step-in into the next non-blackboxed call.

https://codereview.chromium.org/2668763003/diff/1/src/debug/debug.cc#newcode1836
src/debug/debug.cc:1836: if (it.done()) return;
On 2017/02/01 07:29:43, kozy wrote:
> Yang, do you know when we can have empty stack here?

might be possible, e.g. if we call directly into a function, that is implemented
as C++ built-in, and that throws?

lets move this check to the previous JavaScriptFrameIterator.

[kozy, 2017-02-02 06:58:56]

adressed comments

https://codereview.chromium.org/2668763003/diff/1/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2668763003/diff/1/src/debug/debug.cc#newcode1035
src/debug/debug.cc:1035: if (step_action == StepNext && location.IsReturn()) {
On 2017/02/01 07:46:12, Yang wrote:
> On 2017/02/01 07:29:43, kozy wrote:
> > Yang, WDYT about this part? This code is just moved from V8DebuggerAgentImpl
> but
> > it doesn't sound right to me.
> 
> for the microtask case, we should make a check at the microtask runner, where
we
> would turn pending step-next into a step-in.
> for the blackbox case, it's arguable whether step-next should take us into the
> next call that the blackboxed caller makes. That could be accomplished by
> clicking on step-in. The current behavior assumes that a step-next is actually
a
> step-next to the caller, and then a step-in into the next non-blackboxed call.

I think that case with blackboxed code could be considered in the same way as
microtask case. Why we'd like to support special step-next behavior in one case
and wouldn't in another?
Async step into will enable step-in target chained callback without iterating
through all other callbacks.

Maybe we could just remove this code and suggest users to use step-in when they
actually would like to go to next chained callback or not blackboxed function. I
prefer this.

https://codereview.chromium.org/2668763003/diff/1/src/debug/debug.cc#newcode1836
src/debug/debug.cc:1836: if (it.done()) return;
On 2017/02/01 07:46:12, Yang wrote:
> On 2017/02/01 07:29:43, kozy wrote:
> > Yang, do you know when we can have empty stack here?
> 
> might be possible, e.g. if we call directly into a function, that is
implemented
> as C++ built-in, and that throws?
> 
> lets move this check to the previous JavaScriptFrameIterator.

Done.

[Yang, 2017-02-02 12:10:49]

LGTM. I'd be ok with addressing the step-in issue in a separate CL. I expect
that some test expectations may need to be changed.

https://codereview.chromium.org/2668763003/diff/1/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2668763003/diff/1/src/debug/debug.cc#newcode1035
src/debug/debug.cc:1035: if (step_action == StepNext && location.IsReturn()) {
On 2017/02/02 06:58:56, kozy wrote:
> On 2017/02/01 07:46:12, Yang wrote:
> > On 2017/02/01 07:29:43, kozy wrote:
> > > Yang, WDYT about this part? This code is just moved from
V8DebuggerAgentImpl
> > but
> > > it doesn't sound right to me.
> > 
> > for the microtask case, we should make a check at the microtask runner,
where
> we
> > would turn pending step-next into a step-in.
> > for the blackbox case, it's arguable whether step-next should take us into
the
> > next call that the blackboxed caller makes. That could be accomplished by
> > clicking on step-in. The current behavior assumes that a step-next is
actually
> a
> > step-next to the caller, and then a step-in into the next non-blackboxed
call.
> 
> I think that case with blackboxed code could be considered in the same way as
> microtask case. Why we'd like to support special step-next behavior in one
case
> and wouldn't in another?
> Async step into will enable step-in target chained callback without iterating
> through all other callbacks.
> 
> Maybe we could just remove this code and suggest users to use step-in when
they
> actually would like to go to next chained callback or not blackboxed function.
I
> prefer this.

You are right. Step-in at the return position to indicate that the user wants to
step into the next callback in queue or the next call from blackboxed caller
makes sense.

[kozy, 2017-02-02 16:36:21]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2017-02-02 16:36:21]

The patchset sent to the CQ was uploaded after l-g-t-m from yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/2668763003/#ps20001
(title: "reverted semantic change - only cleanup")

[commit-bot: I haz the power, 2017-02-02 16:36:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kozy, 2017-02-02 16:36:43]

Ok, I'll change step-next semantic at return position in follow up.

[kozy, 2017-02-02 16:38:22]

The CQ bit was unchecked by kozyatinskiy@chromium.org

[dgozman, 2017-02-02 17:18:30]

https://codereview.chromium.org/2668763003/diff/20001/src/inspector/v8-debugg...
File src/inspector/v8-debugger-agent-impl.cc (right):

https://codereview.chromium.org/2668763003/diff/20001/src/inspector/v8-debugg...
src/inspector/v8-debugger-agent-impl.cc:832: if (m_javaScriptPauseScheduled &&
!m_debugger->isPaused()) {
Why do we have m_debugger->isPaused() and this->isPaused(). Should use one
maybe?

https://codereview.chromium.org/2668763003/diff/20001/src/inspector/v8-debugg...
File src/inspector/v8-debugger.cc (right):

https://codereview.chromium.org/2668763003/diff/20001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:107: m_skipAllPauses = false;
This should be per-agent. Similar to asyncStackDepth. Or we can have a
collection of "pausing" agents (like we have a collection of "enabled" ones).

https://codereview.chromium.org/2668763003/diff/20001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:512: agent =
m_inspector->enabledDebuggerAgentForGroup(
Let's reuse groupId from above.

[kozy, 2017-02-02 18:31:15]

The CQ bit was checked by kozyatinskiy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-02 18:31:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kozy, 2017-02-02 18:31:56]

all done, please take a look.

https://codereview.chromium.org/2668763003/diff/20001/src/inspector/v8-debugg...
File src/inspector/v8-debugger-agent-impl.cc (right):

https://codereview.chromium.org/2668763003/diff/20001/src/inspector/v8-debugg...
src/inspector/v8-debugger-agent-impl.cc:832: if (m_javaScriptPauseScheduled &&
!m_debugger->isPaused()) {
On 2017/02/02 17:18:29, dgozman_slow wrote:
> Why do we have m_debugger->isPaused() and this->isPaused(). Should use one
> maybe?

Done.

https://codereview.chromium.org/2668763003/diff/20001/src/inspector/v8-debugg...
File src/inspector/v8-debugger.cc (right):

https://codereview.chromium.org/2668763003/diff/20001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:107: m_skipAllPauses = false;
On 2017/02/02 17:18:29, dgozman_slow wrote:
> This should be per-agent. Similar to asyncStackDepth. Or we can have a
> collection of "pausing" agents (like we have a collection of "enabled" ones).

Done.

https://codereview.chromium.org/2668763003/diff/20001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:512: agent =
m_inspector->enabledDebuggerAgentForGroup(
On 2017/02/02 17:18:30, dgozman_slow wrote:
> Let's reuse groupId from above.

Done.

[commit-bot: I haz the power, 2017-02-02 19:20:02]

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

[commit-bot: I haz the power, 2017-02-02 19:20:02]

Dry run: This issue passed the CQ dry run.

[kozy, 2017-02-02 22:25:36]

Dmitry, please take a look, ready for review.

[dgozman, 2017-02-02 23:40:36]

https://codereview.chromium.org/2668763003/diff/60001/src/inspector/v8-debugg...
File src/inspector/v8-debugger-agent-impl.cc (left):

https://codereview.chromium.org/2668763003/diff/60001/src/inspector/v8-debugg...
src/inspector/v8-debugger-agent-impl.cc:617: if (m_javaScriptPauseScheduled ||
m_debugger->isPaused())
Let's merge V8Debugger::isPaused() with m_runningNestedMessageLoop, and make it
private.

https://codereview.chromium.org/2668763003/diff/60001/src/inspector/v8-debugg...
File src/inspector/v8-debugger-agent-impl.cc (right):

https://codereview.chromium.org/2668763003/diff/60001/src/inspector/v8-debugg...
src/inspector/v8-debugger-agent-impl.cc:237: return
m_state->getBoolean(DebuggerAgentState::skipAllPauses, &value) &&
Let's have m_skipAllPauses, as this is slow but method is hot.

https://codereview.chromium.org/2668763003/diff/60001/src/inspector/v8-debugg...
src/inspector/v8-debugger-agent-impl.cc:636: return stepInto();
Please undo.

https://codereview.chromium.org/2668763003/diff/60001/src/inspector/v8-debugg...
src/inspector/v8-debugger-agent-impl.cc:1130: if (!enabled() ||
!m_debugger->canBreakProgram()) return;
Check for skipAllPauses.

[kozy, 2017-02-03 01:04:08]

all done! :)

https://codereview.chromium.org/2668763003/diff/60001/src/inspector/v8-debugg...
File src/inspector/v8-debugger-agent-impl.cc (left):

https://codereview.chromium.org/2668763003/diff/60001/src/inspector/v8-debugg...
src/inspector/v8-debugger-agent-impl.cc:617: if (m_javaScriptPauseScheduled ||
m_debugger->isPaused())
On 2017/02/02 23:40:36, dgozman_slow wrote:
> Let's merge V8Debugger::isPaused() with m_runningNestedMessageLoop, and make
it
> private.

Done.

https://codereview.chromium.org/2668763003/diff/60001/src/inspector/v8-debugg...
File src/inspector/v8-debugger-agent-impl.cc (right):

https://codereview.chromium.org/2668763003/diff/60001/src/inspector/v8-debugg...
src/inspector/v8-debugger-agent-impl.cc:237: return
m_state->getBoolean(DebuggerAgentState::skipAllPauses, &value) &&
On 2017/02/02 23:40:36, dgozman_slow wrote:
> Let's have m_skipAllPauses, as this is slow but method is hot.

Done.

https://codereview.chromium.org/2668763003/diff/60001/src/inspector/v8-debugg...
src/inspector/v8-debugger-agent-impl.cc:636: return stepInto();
On 2017/02/02 23:40:36, dgozman_slow wrote:
> Please undo.

Done.

https://codereview.chromium.org/2668763003/diff/60001/src/inspector/v8-debugg...
src/inspector/v8-debugger-agent-impl.cc:1130: if (!enabled() ||
!m_debugger->canBreakProgram()) return;
On 2017/02/02 23:40:36, dgozman_slow wrote:
> Check for skipAllPauses.

Done.

[dgozman, 2017-02-03 02:23:08]

lgtm

https://codereview.chromium.org/2668763003/diff/100001/src/inspector/v8-debug...
File src/inspector/v8-debugger-agent-impl.cc (left):

https://codereview.chromium.org/2668763003/diff/100001/src/inspector/v8-debug...
src/inspector/v8-debugger-agent-impl.cc:842: if (m_javaScriptPauseScheduled &&
!m_skipAllPauses &&
Bring back m_skipAllPauses check?

[kozy, 2017-02-03 03:22:23]

all done.

https://codereview.chromium.org/2668763003/diff/100001/src/inspector/v8-debug...
File src/inspector/v8-debugger-agent-impl.cc (left):

https://codereview.chromium.org/2668763003/diff/100001/src/inspector/v8-debug...
src/inspector/v8-debugger-agent-impl.cc:842: if (m_javaScriptPauseScheduled &&
!m_skipAllPauses &&
On 2017/02/03 02:23:08, dgozman_slow wrote:
> Bring back m_skipAllPauses check?

Done.

[kozy, 2017-02-03 03:22:33]

The CQ bit was checked by kozyatinskiy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-03 03:22:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-03 03:39:20]

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

[commit-bot: I haz the power, 2017-02-03 03:39:21]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)

[kozy, 2017-02-03 06:36:42]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2017-02-03 06:36:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
yangguo@chromium.org, dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/2668763003/#ps140001
(title: "removed redundant dcheck")

[commit-bot: I haz the power, 2017-02-03 06:36:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kozy, 2017-02-03 06:37:09]

The CQ bit was unchecked by kozyatinskiy@chromium.org

[kozy, 2017-02-03 06:38:31]

Patchset #8 (id:140001) has been deleted

[kozy, 2017-02-03 06:38:37]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2017-02-03 06:38:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
dgozman@chromium.org, yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/2668763003/#ps160001
(title: "cleanup-debugger-agent")

[commit-bot: I haz the power, 2017-02-03 06:38:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-03 07:09:16]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1486103917238820,
"parent_rev": "7c32e07d31974b98b5e4a3ea812ac429880b2d8c", "commit_rev":
"3a4f5fafe0a13992917511a42e8fd6ee0c65968a"}

[commit-bot: I haz the power, 2017-02-03 07:09:21]

Description was changed from

==========
[inspector] V8DebuggerAgent cleanup

V8DebuggerAgentImpl::m_skipAllPaused is moved to V8Debugger.
V8DebuggerAgentImpl::didPaused doesn't return shouldBreak flag and called only
when break is required and stack trace presented.
V8DebuggerAgentImpl doesn't store paused context.
Logic of conversion step-next at return into step-in is moved to debug.cc.

BUG=none
R=dgozman@chromium.org,yangguo@chromium.org
==========

to

==========
[inspector] V8DebuggerAgent cleanup

V8DebuggerAgentImpl::m_skipAllPaused is moved to V8Debugger.
V8DebuggerAgentImpl::didPaused doesn't return shouldBreak flag and called only
when break is required and stack trace presented.
V8DebuggerAgentImpl doesn't store paused context.
Logic of conversion step-next at return into step-in is moved to debug.cc.

BUG=none
R=dgozman@chromium.org,yangguo@chromium.org

Review-Url: https://codereview.chromium.org/2668763003
Cr-Commit-Position: refs/heads/master@{#42907}
Committed:
https://chromium.googlesource.com/v8/v8/+/3a4f5fafe0a13992917511a42e8fd6ee0c6...
==========

[commit-bot: I haz the power, 2017-02-03 07:09:22]

Committed patchset #8 (id:160001) as
https://chromium.googlesource.com/v8/v8/+/3a4f5fafe0a13992917511a42e8fd6ee0c6...