[inspector] support setTimeout in Debugger.scheduleStepIntoAsync method

[inspector] support setTimeout in Debugger.scheduleStepIntoAsync method

BUG=chromium:432469
R=dgozman@chromium.org

Review-Url: https://codereview.chromium.org/2746743002
Cr-Commit-Position: refs/heads/master@{#44270}
Committed: https://chromium.googlesource.com/v8/v8/+/95120a7e06f0d91562b4ac867c043deb12c7cefe

[kozy, 2017-03-12 08:48:29]

Dmitry, please take a look.

[kozy, 2017-03-28 21:55:42]

Dmitry, please take a look.

[kozy, 2017-03-28 21:55:46]

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

[commit-bot: I haz the power, 2017-03-28 21:55:52]

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-03-28 22:22:28]

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

[commit-bot: I haz the power, 2017-03-28 22:22:29]

Dry run: This issue passed the CQ dry run.

[dgozman, 2017-03-28 23:20:50]

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

https://codereview.chromium.org/2746743002/diff/40001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:371: !m_targetContextGroupId);
Merge with previous DCHECK and simplify it :-)

https://codereview.chromium.org/2746743002/diff/40001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:381: DCHECK(m_targetContextGroupId ==
targetContextGroupId ||
everywhere

https://codereview.chromium.org/2746743002/diff/40001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:687: void
V8Debugger::handleAsyncTaskStepping(v8::Local<v8::Context> context,
handlePromiseAsyncStepping

[kozy, 2017-03-28 23:49:50]

all done, please take another look.

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

https://codereview.chromium.org/2746743002/diff/40001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:371: !m_targetContextGroupId);
On 2017/03/28 23:20:50, dgozman wrote:
> Merge with previous DCHECK and simplify it :-)

this dcheck is redundant so removed.

https://codereview.chromium.org/2746743002/diff/40001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:381: DCHECK(m_targetContextGroupId ==
targetContextGroupId ||
On 2017/03/28 23:20:50, dgozman wrote:
> everywhere

Done.

https://codereview.chromium.org/2746743002/diff/40001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:687: void
V8Debugger::handleAsyncTaskStepping(v8::Local<v8::Context> context,
On 2017/03/28 23:20:50, dgozman wrote:
> handlePromiseAsyncStepping

removed this method.

[kozy, 2017-03-29 01:02:51]

kozyatinskiy@chromium.org changed reviewers:
+ yangguo@chromium.org

[kozy, 2017-03-29 01:02:52]

Yang, please take a look on api.cc. It looks like we don't need to be on break
to be able to clear stepping.

[dgozman, 2017-03-29 20:47:07]

lgtm

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

https://codereview.chromium.org/2746743002/diff/60001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:401: m_targetContextGroupId = targetContextGroupId;
Let's add a test for scheduleStepIntoAsync+resume.

https://codereview.chromium.org/2746743002/diff/60001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:976: void
V8Debugger::asyncTaskFinishedForStepping(void* task) {
Should we clear m_taskWithScheduledBreak here? If so, we must clear it even when
m_breakRequested to avoid stale pointer.

https://codereview.chromium.org/2746743002/diff/60001/test/inspector/debugger...
File test/inspector/debugger/schedule-step-into-async-set-timeout-expected.txt
(right):

https://codereview.chromium.org/2746743002/diff/60001/test/inspector/debugger...
test/inspector/debugger/schedule-step-into-async-set-timeout-expected.txt:19:
#debugger
//should-break-here

https://codereview.chromium.org/2746743002/diff/60001/test/inspector/debugger...
test/inspector/debugger/schedule-step-into-async-set-timeout-expected.txt:24:
debugger; setTimeout(() => 1#, 0);debugger;
What is this about?

[kozy, 2017-03-29 22:33:34]

all done!
Yang, please take a look at api.cc.

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

https://codereview.chromium.org/2746743002/diff/60001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:401: m_targetContextGroupId = targetContextGroupId;
On 2017/03/29 20:47:06, dgozman wrote:
> Let's add a test for scheduleStepIntoAsync+resume.

Done.

https://codereview.chromium.org/2746743002/diff/60001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:976: void
V8Debugger::asyncTaskFinishedForStepping(void* task) {
On 2017/03/29 20:47:06, dgozman wrote:
> Should we clear m_taskWithScheduledBreak here? If so, we must clear it even
when
> m_breakRequested to avoid stale pointer.

Done.

https://codereview.chromium.org/2746743002/diff/60001/test/inspector/debugger...
File test/inspector/debugger/schedule-step-into-async-set-timeout-expected.txt
(right):

https://codereview.chromium.org/2746743002/diff/60001/test/inspector/debugger...
test/inspector/debugger/schedule-step-into-async-set-timeout-expected.txt:19:
#debugger
On 2017/03/29 20:47:06, dgozman wrote:
> //should-break-here

Done.

https://codereview.chromium.org/2746743002/diff/60001/test/inspector/debugger...
test/inspector/debugger/schedule-step-into-async-set-timeout-expected.txt:24:
debugger; setTimeout(() => 1#, 0);debugger;
On 2017/03/29 20:47:06, dgozman wrote:
> What is this about?

fixed

[kozy, 2017-03-29 22:33:42]

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

[commit-bot: I haz the power, 2017-03-29 22:33:45]

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-03-29 23:02:55]

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

[commit-bot: I haz the power, 2017-03-29 23:02:57]

Dry run: This issue passed the CQ dry run.

[Yang, 2017-03-30 09:53:10]

On 2017/03/29 22:33:34, kozy wrote:
> all done!
> Yang, please take a look at api.cc.
> 
>
https://codereview.chromium.org/2746743002/diff/60001/src/inspector/v8-debugg...
> File src/inspector/v8-debugger.cc (right):
> 
>
https://codereview.chromium.org/2746743002/diff/60001/src/inspector/v8-debugg...
> src/inspector/v8-debugger.cc:401: m_targetContextGroupId =
targetContextGroupId;
> On 2017/03/29 20:47:06, dgozman wrote:
> > Let's add a test for scheduleStepIntoAsync+resume.
> 
> Done.
> 
>
https://codereview.chromium.org/2746743002/diff/60001/src/inspector/v8-debugg...
> src/inspector/v8-debugger.cc:976: void
> V8Debugger::asyncTaskFinishedForStepping(void* task) {
> On 2017/03/29 20:47:06, dgozman wrote:
> > Should we clear m_taskWithScheduledBreak here? If so, we must clear it even
> when
> > m_breakRequested to avoid stale pointer.
> 
> Done.
> 
>
https://codereview.chromium.org/2746743002/diff/60001/test/inspector/debugger...
> File test/inspector/debugger/schedule-step-into-async-set-timeout-expected.txt
> (right):
> 
>
https://codereview.chromium.org/2746743002/diff/60001/test/inspector/debugger...
> test/inspector/debugger/schedule-step-into-async-set-timeout-expected.txt:19:
> #debugger
> On 2017/03/29 20:47:06, dgozman wrote:
> > //should-break-here
> 
> Done.
> 
>
https://codereview.chromium.org/2746743002/diff/60001/test/inspector/debugger...
> test/inspector/debugger/schedule-step-into-async-set-timeout-expected.txt:24:
> debugger; setTimeout(() => 1#, 0);debugger;
> On 2017/03/29 20:47:06, dgozman wrote:
> > What is this about?
> 
> fixed

api.cc lgtm.

[kozy, 2017-03-30 14:40:47]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2017-03-30 14:40:47]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/2746743002/#ps80001
(title: "addressed comments")

[commit-bot: I haz the power, 2017-03-30 14:40:49]

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-03-30 14:42:25]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490884847300900,
"parent_rev": "5ca9632e5114f3e2efaa581fe2d3f37004edfc9b", "commit_rev":
"95120a7e06f0d91562b4ac867c043deb12c7cefe"}

[commit-bot: I haz the power, 2017-03-30 14:42:32]

Description was changed from

==========
[inspector] support setTimeout in Debugger.scheduleStepIntoAsync method

BUG=chromium:432469
R=dgozman@chromium.org
==========

to

==========
[inspector] support setTimeout in Debugger.scheduleStepIntoAsync method

BUG=chromium:432469
R=dgozman@chromium.org

Review-Url: https://codereview.chromium.org/2746743002
Cr-Commit-Position: refs/heads/master@{#44270}
Committed:
https://chromium.googlesource.com/v8/v8/+/95120a7e06f0d91562b4ac867c043deb12c...
==========

[commit-bot: I haz the power, 2017-03-30 14:42:33]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/v8/v8/+/95120a7e06f0d91562b4ac867c043deb12c...